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 SEC driver #5894

Closed
wants to merge 3 commits into from
Closed

Conversation

xiaoxuZeng
Copy link
Contributor

@xiaoxuZeng xiaoxuZeng commented Mar 16, 2023

Sec is a Security Engine for cipher, digest, HMAC, authenc.

Qm is a queue management device include by Sec.

This PR just implement sec cipher function.

digest, HMAC, authenc. will add on next PR, when we finished development and test.

@xiaoxuZeng xiaoxuZeng force-pushed the master branch 4 times, most recently from cde343e to fef17de Compare March 20, 2023 13:12
@xiaoxuZeng xiaoxuZeng changed the title drivers: implement qm driver drivers: implement SEC driver Mar 23, 2023
@jenswi-linaro
Copy link
Contributor

What's kunpeng? Are there more kunpengs than HiSilicon kunpengs?

@xiaoxuZeng
Copy link
Contributor Author

xiaoxuZeng commented Mar 27, 2023

What's kunpeng? Are there more kunpengs than HiSilicon kunpengs?

HiSilicon is a vendor, and Kunpeng is a product.
I'll refer to the naming methods of other vendors in the open source community and expect to provide feedback by Wednesday.

Thanks for your review.

@jenswi-linaro
Copy link
Contributor

If I've understood it correctly this is all HiSilicon specific. How about putting the files below core/drivers/crypto/hisilicon or core/drivers/crypto/hisi? I'm not sure that further subdirectories are needed.

Public functions and structs prefixed with hisi_. I see that's what you're mostly doing already, but I'd appreciate it if we could be fully consistent.

Public headers (used outside the driver) should preferably go into core/include/drivers/ even if there are examples of public include directories below core/drivers/crypto/. An advantage of keeping public include files below core/include/drivers/ is that it's easier to find things there rather than having to search for .h files all over the place.

Private headers (used inside the driver only) should go into the driver directory and should hopefully not need to be reached outside of that directory.

@xiaoxuZeng
Copy link
Contributor Author

If I've understood it correctly this is all HiSilicon specific. How about putting the files below core/drivers/crypto/hisilicon or core/drivers/crypto/hisi? I'm not sure that further subdirectories are needed.

Public functions and structs prefixed with hisi_. I see that's what you're mostly doing already, but I'd appreciate it if we could be fully consistent.

Public headers (used outside the driver) should preferably go into core/include/drivers/ even if there are examples of public include directories below core/drivers/crypto/. An advantage of keeping public include files below core/include/drivers/ is that it's easier to find things there rather than having to search for .h files all over the place.

Private headers (used inside the driver only) should go into the driver directory and should hopefully not need to be reached outside of that directory.

Thank very much for your patient guidance.

  1. I refer to the Linux community. The directory is hisilicon. Subdirectories are divided by module. Your opinion is absolutely
    correct. I plan to use core/drivers/crypto/hisilicon;
  2. The current algorithm file does not use the prefix hisi_. I will change sec_cipher.c to hisi_sec_cipher.c.
  3. The API of SEC is registered into crypto. Currently, the API provided by the QM is invoked only by SEC. Therefore, the two header files are private header files.

Please help me review it at your convenience. Is the above correct?

@jenswi-linaro
Copy link
Contributor

  1. I refer to the Linux community. The directory is hisilicon. Subdirectories are divided by module. Your opinion is absolutely
    correct. I plan to use core/drivers/crypto/hisilicon;

OK

  1. The current algorithm file does not use the prefix hisi_. I will change sec_cipher.c to hisi_sec_cipher.c.

There's no need to change that since you're using a hisilicon directory. Up to you what you prefer.

  1. The API of SEC is registered into crypto. Currently, the API provided by the QM is invoked only by SEC. Therefore, the two header files are private header files.

All non-static symbols should have a hisi_ prefix, I guess that becomes hisi_sec_ for SEC then. To be sure to avoid conflicts it's good to prefix the private .h files with hisi_ too.

@xiaoxuZeng xiaoxuZeng force-pushed the master branch 4 times, most recently from fe036de to 4682670 Compare April 4, 2023 06:40
@xiaoxuZeng
Copy link
Contributor Author

If I've understood it correctly this is all HiSilicon specific. How about putting the files below core/drivers/crypto/hisilicon or core/drivers/crypto/hisi? I'm not sure that further subdirectories are needed.

Public functions and structs prefixed with hisi_. I see that's what you're mostly doing already, but I'd appreciate it if we could be fully consistent.

Public headers (used outside the driver) should preferably go into core/include/drivers/ even if there are examples of public include directories below core/drivers/crypto/. An advantage of keeping public include files below core/include/drivers/ is that it's easier to find things there rather than having to search for .h files all over the place.

Private headers (used inside the driver only) should go into the driver directory and should hopefully not need to be reached outside of that directory.

update:
1.Change directory name to hisilicon.
2.All non-static symbols add a hisi_ prefix.

@xiaoxuZeng
Copy link
Contributor Author

The commits with review tag will be rebase into previous commites.

@jenswi-linaro
Copy link
Contributor

Please squash in the review commits to make it easier to review the individual commits.

@jforissier
Copy link
Contributor

drivers: implement qm driver

drivers: crypto: hisilicon: implement QM driver

The Hisilicon QM is a Queue Management module.
In order to unify the interface between accelerator and software,
a unified queue management module QM is used to interact with software.
Each accelerator module integrates a QM. Software issues tasks to the SQ,

Please define SQ.

and the QM obtains the address of the SQE. The BD information is sent to

Please define SQE and BD.

the accelerator. After the task processing is complete, the accelerator
applies for a write-back address from the QM to write back the SQ.

@jforissier
Copy link
Contributor

drivers: implement sec driver

drivers: crypto: hisilicon: implement SEC driver

The Hisilicon SEC(Security Engine) is a hardware security acceleration

Missing space: "SEC (Security Engine)"

module, which is used to enhance the security-related functions of the
processor.

@jforissier
Copy link
Contributor

Third commit subject should be "drivers: crypto: hisilicon: implement SEC ciphers".

@jforissier
Copy link
Contributor

drivers: implement for kunpeng compile

  1. add compile files for sec qm;

Why not done in the previous commits?

  1. Implement GIC handle for d06;

s/d06/D06/ and should be a separate commit.

  1. Register SEC base address.

I think a better description for what's left would be "d06: crypto: enable SEC acceleration", wouldn't it?

@xiaoxuZeng
Copy link
Contributor Author

drivers: implement for kunpeng compile

  1. add compile files for sec qm;

Why not done in the previous commits?

  1. Implement GIC handle for d06;

s/d06/D06/ and should be a separate commit.

  1. Register SEC base address.

I think a better description for what's left would be "d06: crypto: enable SEC acceleration", wouldn't it?

Good point,I'll deal with it immediately!

Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

Comments on "drivers: implement for kunpeng compile".

core/drivers/crypto/sub.mk Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/sub.mk Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/sub.mk Outdated Show resolved Hide resolved
Copy link
Contributor

@jforissier jforissier left a comment

Choose a reason for hiding this comment

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

Some more comments on "drivers: implement qm driver". Similar comments apply to SEC too.

core/drivers/crypto/hisilicon/qm/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/qm/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/qm/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/qm/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/qm/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/qm/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/qm/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/qm/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/qm/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/qm/hisi_qm.c Outdated Show resolved Hide resolved
@xiaoxuZeng xiaoxuZeng force-pushed the master branch 5 times, most recently from f0f548a to a96707c Compare April 12, 2023 06:54
@@ -5,6 +5,7 @@
*/
#include "hisi_qm.h"

#define CFG_RDY_BIT 0x1
Copy link
Contributor

Choose a reason for hiding this comment

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

CFG_ prefix is reserved to configuration switches. Use QM_FVT_CFG_RDY_BIT IIUC.

C_MODE_CCM = 0x5,
C_MODE_GCM = 0x6,
C_MODE_XTS = 0x7,
C_MODE_CBC_CS = 0x9,

This comment was marked as resolved.

};

enum hisi_drv_status {
DRVCRYPT_NO_ERR = 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

These are HISI_QM specific error code. Please prefix with HISI_QM_DRVCRYPT_ or like.

struct hisi_qp *qp_array;
struct mutex qp_lock; /* protect the qp instance */

int32_t (*dev_status_check)(struct hisi_qm *qm);
Copy link
Contributor

Choose a reason for hiding this comment

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

is expected to return an enum hisi_drv_status, not a raw int32 right?

Copy link
Contributor

Choose a reason for hiding this comment

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

please fix straight in 1st commit "drivers: crypto: hisilicon: implement QM driver"

*@param qm: Handle of Queue Management module
*@return success: 0,fail: -DRVCRYPT_EBUSY/DRVCRYPT_EINVAL
*/
int32_t hisi_qm_init(struct hisi_qm *qm);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not returning type enum hisi_drv_status. It would make the implementation better and prevent one messes up between several kinds of return value enums. This comment applies to all hsis_xxx() function returning these error codes.

Note I don't think you need a negative value. Testing a non-zero retrun code would be sufficient to detect an error condition.

core/drivers/crypto/hisilicon/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/hisi_qm.c Outdated Show resolved Hide resolved
core/drivers/crypto/hisilicon/hisi_qm.c Outdated Show resolved Hide resolved
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.

Main issue to address: many functions are declared to return a TEE_Result value whereas they return an enum hisi_drv_status value.

@@ -288,6 +288,11 @@ static inline __noprof void dsb_ishst(void)
asm volatile ("dsb ishst" : : : "memory");
}

static inline __noprof void dsb_osh(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

db_osh() deserves a specific commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea, I will handle it as soon as possible.

core/arch/arm/plat-d06/main.c Outdated Show resolved Hide resolved
{
uint32_t val = 0;

/* return 0 mailbox ready, -ETIMEDOUT hardware timeout */
Copy link
Contributor

Choose a reason for hiding this comment

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

this function is expected to return a TEE_result compliant value.

Copy link
Contributor Author

@xiaoxuZeng xiaoxuZeng May 22, 2023

Choose a reason for hiding this comment

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

Did I misunderstand your meaning?

static TEE_Result qm_mb(struct hisi_qm *qm, uint8_t cmd, vaddr_t dma_addr,
			uint16_t qn, uint8_t op)

to

static enum hisi_drv_status qm_mb(struct hisi_qm *qm, uint8_t cmd, vaddr_t dma_addr,
			uint16_t qn, uint8_t op)

Is this what you meant? @etienne-lms

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I was not very clear. I'll try to make it short
The many functions that return a value like HISI_QM_DRVCRYPT_xxx should be declared as:
enum hisi_drv_status <function_label>(...).
All functions that return a value like TEE_SUCCESS or TEE_ERROR_xxxshould be declared as:TEE_Result <function_label>(...)`
Make sure not to mix both unless what return values are not reliable.

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 I understand now. Thank you for your patient explanation. I will handle it as soon as possible!


if (qm_wait_mb_ready(qm)) {
EMSG("QM mailbox is busy");
return HISI_QM_DRVCRYPT_EBUSY;

This comment was marked as resolved.

case QM_SQC_VFT:
data = (SHIFT_U64(base, QM_SQC_VFT_START_SQN_SHIFT) |
QM_SQC_VFT_VALID |
SHIFT_U64((number - 1), QM_SQC_VFT_SQ_NUM_SHIFT));
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove outer parentheses

{
struct sec_cipher_ctx *c_ctx = NULL;
size_t padding_size = 0;
TEE_Result ret = 0;
Copy link
Contributor

@etienne-lms etienne-lms May 22, 2023

Choose a reason for hiding this comment

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

prefer use a TEE_Result defined value rather than 0. E.g. TEE_SUCCESS, or TEE_ERROR_GENERIC, or ...

struct acc_device *sec_dev = NULL, *cur_dev = NULL;
uint32_t max_qp_num = 0;
uint32_t free_qp_num;
struct hisi_qm *qm;
Copy link
Contributor

Choose a reason for hiding this comment

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

init values

{
struct hisi_qm *qm = &sec_dev->qm;
uint32_t val = 0;
uint32_t ret = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommendation: enum hisi_drv_status ret = HISI_QM_DRVCRYPT_NO_ERRere and at many other places.

uint8_t t = 0;
int i = 0;

for (i = 0, t = 0; i < AES_SM4_IV_SIZE; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can remove t = 0, it is already inittialilzed to 0.

@xiaoxuZeng xiaoxuZeng force-pushed the master branch 2 times, most recently from 8e0424c to 975f2f1 Compare May 22, 2023 08:35

register_phys_mem_pgdir(MEM_AREA_IO_NSEC, LPC_BASE, LPC_SIZE);
register_phys_mem_pgdir(MEM_AREA_IO_SEC, SEC_BASE, SEC_SIZE);

void itr_core_handler(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

please rebase in master tip. Platforms no more need to implement itr_core_handler() since #6030 has been merged.

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'll do it as soon as possible.


$(call force,CFG_CRYPTO_DRIVER,y)
CFG_CRYPTO_DRIVER_DEBUG ?= 0

Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking: i suggestion to remove the empty lines above and below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, is a kind suggestion, thx

*/
#include "hisi_qm.h"

#define QM_FVT_CFG_RDY_BIT 0x1
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking: using a tabulation bewteen macro name and assigned value make the file easier to read:

#define QM_FVT_CFG_RDY_BIT	0x1
/* Doorbell */
#define QM_DOORBELL_SQ_CQ_BASE	0x1000
#define QM_DB_CMD_SHIFT		12
#define QM_DB_RAND_DATA_SHIFT	16
#define QM_DB_INDEX_SHIFT	32
#define QM_DB_PRIORITY_SHIFT	48
#define QM_DB_RAND_DATA		0x5a
#define QM_DOORBELL_CMD_SQ	0
#define QM_DOORBELL_CMD_CQ	1
(...)

{.reg_name = "QM_DFX_WB_SQE_FROM_ACC_CNT", .reg_offset = 0x104058},
{.reg_name = "QM_DFX_ACC_FINISH_CNT ", .reg_offset = 0x104060},
{.reg_name = "QM_DFX_CQE_ERR_CNT ", .reg_offset = 0x1040b4},
{.reg_name = NULL, 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

could replace all 2 space chars with a signle one (applies from line 109 to line 119).

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, on coding style, add a space char after an opening brace { and another space char before the closing brace }.

	{ .reg_name = "QM_DFX_ACC_FINISH_CNT     ", .reg_offset = 0x104060 },
	{ .reg_name = "QM_DFX_CQE_ERR_CNT        ", .reg_offset = 0x1040b4 },
	{ .reg_name =  NULL, 0 }

{
uint32_t val = 0;

/* return 0 mailbox ready, HISI_QM_DRVCRYPT_ETMOUT hardware timeout */

This comment was marked as resolved.

}

ret = cipher_algo_check(algo);
if (ret != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ret != 0/ret/

}

c_ctx->offs = 0;
(*ctx) = c_ctx;
Copy link
Contributor

Choose a reason for hiding this comment

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

parentheses not needed

return;

hisi_qm_release_qp(c_ctx->qp);
memset(c_ctx->key, 0, c_ctx->key_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

should use memzero_explicit() instead of memset().

static TEE_Result sec_cipher_init(struct drvcrypt_cipher_init *dinit)
{
struct sec_cipher_ctx *c_ctx = NULL;
TEE_Result ret = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

= TEE_SUCCESS

dupdate->src.data, dupdate->src.length);

ret = sec_do_cipher_task(c_ctx->qp, c_ctx);
if (ret != 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ret != 0/ret/ here and at all other places where applicable.

@etienne-lms
Copy link
Contributor

@xiaoxuZeng, please don't use commit merge in your patch series. You need to rebase your series on top of master branch tip instead.

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.

I found some issues. Please rebase your changes, i'll review them afterward.


if (!iv && iv_len != 0) {
EMSG("Iv is NULL");
if (!iv && iv_len != TEE_SUCCESS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: iv_len is not a TEE_Resuslt value.
Replace with if (!iv && iv_len)

paddr_t sqe_dma;
paddr_t cqe_dma;

enum TEE_Result (*fill_sqe)(void *sqe, void *msg);
Copy link
Contributor

Choose a reason for hiding this comment

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

s/TEE_Result/enum hisi_drv_status/

Ditto at line below.

#define __HISI_SEC_H__

#include "hisi_qm.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.

swap the 2 lines.
include <> first, then include "".

@xiaoxuZeng
Copy link
Contributor Author

@xiaoxuZeng, please don't use commit merge in your patch series. You need to rebase your series on top of master branch tip instead.

I will handle it as soon as possible. Thank you!

@etienne-lms
Copy link
Contributor

@xiaoxuZeng, please don't use commit merges in your patch series. You need to rebase your series on top of master branch tip instead.

@xiaoxuZeng xiaoxuZeng force-pushed the master branch 3 times, most recently from e73e3d4 to 2ecb37f Compare July 6, 2023 13:05
@jenswi-linaro
Copy link
Contributor

For commit "core: arm64: add dsb_osh", in the description I think it should say "osh data barrier" instead of "ishst memory barrier". Feel free to create a new pull request with this commit only. That would allow getting that commit merged before the rest of this PR and help the reviewing a little bit.

@xiaoxuZeng
Copy link
Contributor Author

For commit "core: arm64: add dsb_osh", in the description I think it should say "osh data barrier" instead of "ishst memory barrier". Feel free to create a new pull request with this commit only. That would allow getting that commit merged before the rest of this PR and help the reviewing a little bit.

yeh, I have created a new PR, Thanks.
#6176

The Hisilicon QM is a Queue Management module.
In order to unify the interface between accelerator and software,
a unified queue management module QM is used to interact with software.
Each accelerator module integrates a QM. Software issues tasks to the
SQ (Submmision Queue),and the QM obtains the address of the SQE (Submmision
Queue Element). The BD (Buffer Description, same as SQE) information is
sent to the accelerator. After the task processing is complete, the
accelerator applies for a write-back address from the QM to write back
the SQ.

Signed-off-by: Xiaoxu Zeng <zengxiaoxu@huawei.com>
The Hisilicon SEC (Security Engine) is a hardware security acceleration
module, which is used to enhance the security-related functions of the
processor.

Signed-off-by: Xiaoxu Zeng <zengxiaoxu@huawei.com>
Add the Hisilicon SEC as a drvcrypt cipher provider.
Supported for DES-ECB/CBC 3DES-ECB/CBC AES-ECB/CBC/CTR
SM4-ECB/CBC/CTR.

Signed-off-by: Xiaoxu Zeng <zengxiaoxu@huawei.com>
@github-actions
Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 30 days with no activity. Remove the stale label or add a comment, otherwise this pull request will automatically be closed in 5 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Aug 31, 2023
@github-actions github-actions bot closed this Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants