Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

drivers:implement HiSilicon Security Engine(SEC) module. #6776

Merged
merged 1 commit into from
Apr 30, 2024

Conversation

LeiSen62
Copy link
Contributor

@LeiSen62 LeiSen62 commented Apr 1, 2024

HiSilicon SEC is used in security applications such as authentication and data encryption and decryption. This module implement the hardware initialization of the SEC.

@LeiSen62
Copy link
Contributor Author

LeiSen62 commented Apr 1, 2024

I`m sorry to close the previous PR by mistake. I submitted a new PR based on HPRE.Please review it again. Thank you very much.#6689

return hisi_qm_init(qm);
}

static struct acc_device *sec_pre_init(void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpicking: for consistency, I suggest to rename this function sec_alloc() and implement its counter part:

static void sec_free(struct acc_device *sec_dev)
{
	SLIST_REMOVE_HEAD(&sec_list, link);
	free(sec_dev);
}

#ifndef __SEC_MAIN_H
#define __SEC_MAIN_H

#include <initcall.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initcall.h not needed in the header file. Prefer include it from sec_main.c

Add #include <stdint.h>.

* Copyright 2022-2024 HiSilicon Limited.
* Kunpeng hardware accelerator SEC module init.
*/
#include "sec_main.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+#include <io.h>
+#include <malloc.h>
+#include <sys/queue.h>
+#include <trace.h>
+#include <util.h>
 #include "sec_main.h"

@LeiSen62
Copy link
Contributor Author

LeiSen62 commented Apr 9, 2024

The review comments have been modified. Please review it again. Thank you very much. @etienne-lms @jenswi-linaro

#include <trace.h>
#include <util.h>
#include <initcall.h>
#include "sec_main.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sec_main.h is in the include path so <> should be used instead.

#ifndef __SEC_MAIN_H
#define __SEC_MAIN_H

#include "hisi_qm.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hisi_qm.h is in the include path so please use <> instead.

{
struct hisi_qm *qm = &sec_dev->qm;

qm->io_base = (uintptr_t)phys_to_virt_io(sec_dev->io_base,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(vaddr_t)


val = io_read32(qm->io_base + SEC_CORE_INT_SOURCE);
if (val & SEC_RAS_NFE_ENB_MASK) {
EMSG("SEC NFE RAS happened, need to reset");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is someone supposed to read this and do something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a debug info.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use DMSG() for debug info.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function returns an error code so I think it's fair it uses here the EMSG() macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, and I didn't make the change.

qm->vfs_num = sec_dev->vfs_num;
qm->sqe_size = SEC_SQE_SIZE;
qm->sqe_log2_size = SEC_SQE_LOG2_SIZE;
if (qm->fun_type == HISI_QM_HW_PF) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is always true, but I assume that might change later. If this is false, does it still make sense to print something about the version or the state of the initialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is false, it won't print the information.

@LeiSen62
Copy link
Contributor Author

LeiSen62 commented Apr 22, 2024

Hi, does this patch still need to be modified? If not, please merge it. @jenswi-linaro

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2 comments from @jenswi-linaro that have no answers.
I've added a minor conment.

core/drivers/crypto/hisilicon/sec_main.c Show resolved Hide resolved
@jenswi-linaro
Copy link
Contributor

Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
With or without my last comment addressed.

@LeiSen62
Copy link
Contributor Author

Hi @jforissier ,all review comments have been modified. Can you help me merge the patch?

@jforissier
Copy link
Contributor

jforissier commented Apr 26, 2024

@LeiSen62 please add @jenswi-linaro's Acked-by. Perhaps @etienne-lms wants to give one, too.

HiSilicon SEC is used in security applications such as
authentication and data encryption and decryption. This
module implement the hardware initialization of the SEC.

Signed-off-by: leisen <leisen1@huawei.com>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
@LeiSen62
Copy link
Contributor Author

Hi @jforissier , I had added jenswi-linaro's and etienne-lms`s Acked-by.

@jforissier
Copy link
Contributor

Hi @jforissier , I had added jenswi-linaro's and etienne-lms`s Acked-by.

@etienne-lms has not given an Acked-by yet unless I missed something.

@LeiSen62
Copy link
Contributor Author

Hi @etienne-lms, your review comments have been modified, Can you give an Acked-by?

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all good to me.
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>

@LeiSen62
Copy link
Contributor Author

Hi @jforissier, can you help me to merge the patch?

@jforissier jforissier merged commit 9b6221a into OP-TEE:master Apr 30, 2024
7 checks passed
@jforissier
Copy link
Contributor

Hi @jforissier, can you help me to merge the patch?

Done, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants