drivers: implement HiSilicon Queue Management (QM) module by xiaoxuZeng · Pull Request #6368 · OP-TEE/optee_os

xiaoxuZeng · 2023-10-11T12:52:27Z

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.

xiaoxuZeng · 2023-10-11T13:01:08Z

This patch is based on the following 2 PRs:
#6297
#6212

We warmly welcome your comments，thanks.

etienne-lms

2 main comments to be addressed. Otherwise the implementation LGTM.
@xiaoxuZeng, maybe some comments apply to hisi_qm part1 P-R.

ZijjWang · 2023-10-17T02:31:24Z

LGTM

review_by: wangzijian <wangzijian22@huawei.com>

etienne-lms

Few remaining nitpicking comments and question around allocated memory: should it be zeored before it's used?

etienne-lms

Hi @xiaoxuZeng, I think you made too many changes in this last series. It seems all uint32_t have changed into unsigned int despite most of them are expected to be 32bit values used with io_read32()/write32(). I've started to puts some comments but there are too many occurrences. Could you come back to the series previous version and change s/uint32_t/unsigned int/ only for the few function local variables used as index in for or like loops?

etienne-lms

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

xiaoxuZeng · 2023-11-14T08:15:26Z

Hi @jforissier

Please review this patch when you are convenience. We look forward to your guidance.

Thanks.

jforissier

Hi @xiaoxuZeng, please see my comments below. Overall this looks good to me.
Please replace the commit subject " The Hisilicon QM is a Queue Management module." with "drivers: implement HiSilicon Queue Management (QM) module".

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier · 2023-11-14T08:38:33Z

+		}
+	}
+
+	if (qm->qp_num == 0 || qm->sqe_size == 0) {


Prefer if (!qm->qp_num || !qm->sqe_size) {

jforissier · 2023-11-14T08:38:59Z

+	}
+
+	if (qm->qp_num == 0 || qm->sqe_size == 0) {
+		EMSG("Invalid qm parameters");


Prefer uppercase QM: Invalid QM parameters

jforissier · 2023-11-14T08:40:04Z

+	for (i = QM_SQC_VFT; i <= QM_CQC_VFT; i++) {
+		ret = qm_set_vft_common(qm, i, function, base, num);
+		if (ret) {
+			EMSG("QM set type%d fail", i);


Missing space: type %d

jforissier · 2023-11-14T08:45:11Z

+	*base = (sqc_vft >> QM_SQC_VFT_START_SQN_SHIFT) & QM_SQC_VFT_BASE_MASK;
+	*num = ((sqc_vft >> QM_SQC_VFT_SQ_NUM_SHIFT) & QM_SQC_VFT_NUM_MASK) + 1;


@jenswi-linaro should we define SHIFTR_U64() for such right-shift operations? Or are they safe to do like this?

jforissier · 2023-11-14T08:48:05Z

+	free(qp->cqe);
+}
+
+static enum hisi_drv_status qp_memory_init(struct hisi_qm *qm, uint32_t id)


I suggest s/qp_memory_init/qp_alloc/ and s/qp_memory_uninit/qp_free/ because the names are simpler and better reflect the fact that you are indeed allocating/freeing from the heap, not just initializing/clearing some data.

jforissier · 2023-11-14T08:48:31Z

+	free(qm->cqc);
+}
+
+static enum hisi_drv_status qm_memory_init(struct hisi_qm *qm)


Same suggestion here: qm_alloc() and qm_free().

jforissier · 2023-11-14T08:50:07Z

+{
+	uint32_t val = 0;
+
+	/* return 0 mailbox ready, HISI_QM_DRVCRYPT_ETMOUT hardware timeout */


Better to start comments with an uppercase letter: /* Return 0... */

jforissier · 2023-11-14T08:55:19Z

Please also rebase on master to fix the IBART CI error. Thanks!

xiaoxuZeng · 2023-11-15T04:10:27Z

Please also rebase on master to fix the IBART CI error. Thanks!

Thank you for your guidance, and we will complete the remaining issues in the near future.

etienne-lms · 2023-11-22T07:53:20Z

+	{ .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 }


s/{ .reg_name = NULL, 0 }/{ }/

etienne-lms · 2023-11-22T07:56:32Z

+/**
+ * @Description: Init QM for Kunpeng drv
+ * @param qm: Handle of Queue Management module
+ * @return success: 0，fail: HISI_QM_DRVCRYPT_EBUSY/HISI_QM_DRVCRYPT_EINVAL


s/0/HISI_QM_DRVCRYPT_NO_ERR/ ?
Dittto at 248 and 256

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> Acked-by: Etienne Carriere <etienne.carriere@foss.st.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

jforissier · 2023-11-23T08:48:17Z

@xiaoxuZeng thanks, I am merging this now (the CI error is unrelated and will go away when the code is rebased to include #6484).

xiaoxuZeng force-pushed the master branch 4 times, most recently from 96fddf6 to 1183f4e Compare October 12, 2023 03:08

xiaoxuZeng mentioned this pull request Oct 13, 2023

[part1] The Hisilicon QM is a Queue Management module. #6377

Closed

etienne-lms reviewed Oct 13, 2023

View reviewed changes

xiaoxuZeng force-pushed the master branch 2 times, most recently from a707419 to cd21284 Compare October 16, 2023 08:18

etienne-lms reviewed Oct 17, 2023

View reviewed changes

xiaoxuZeng force-pushed the master branch from cd21284 to 90b9620 Compare October 18, 2023 03:44

etienne-lms suggested changes Oct 18, 2023

View reviewed changes

Comment thread core/drivers/crypto/hisilicon/hisi_qm.c

Comment thread core/drivers/crypto/hisilicon/hisi_qm.c Outdated

Comment thread core/drivers/crypto/hisilicon/hisi_qm.c Outdated

xiaoxuZeng force-pushed the master branch 3 times, most recently from 877a2ae to 8761fa9 Compare October 25, 2023 03:47

xiaoxuZeng requested a review from etienne-lms October 25, 2023 06:20

etienne-lms reviewed Oct 31, 2023

View reviewed changes

Comment thread core/arch/arm/plat-d06/conf.mk

xiaoxuZeng force-pushed the master branch from 8761fa9 to 399d403 Compare November 2, 2023 08:24

jforissier reviewed Nov 14, 2023

View reviewed changes

xiaoxuZeng closed this Nov 21, 2023

xiaoxuZeng force-pushed the master branch from 0b2b23d to bce2f88 Compare November 21, 2023 13:16

xiaoxuZeng reopened this Nov 22, 2023

xiaoxuZeng changed the title ~~drivers: crypto: hisilicon: implement QM driver~~ drivers: implement HiSilicon Queue Management (QM) module Nov 22, 2023

xiaoxuZeng force-pushed the master branch 2 times, most recently from 98d1747 to 79e5acd Compare November 22, 2023 06:52

etienne-lms reviewed Nov 22, 2023

View reviewed changes

xiaoxuZeng force-pushed the master branch from 79e5acd to f2e74ed Compare November 23, 2023 02:41

jforissier merged commit c7f9abc into OP-TEE:master Nov 23, 2023

This was referenced Nov 30, 2023

crypto: fix qm cache start and done define #6511

Closed

crypto: fix qm cache start and done define #6519

Closed

		*base = (sqc_vft >> QM_SQC_VFT_START_SQN_SHIFT) & QM_SQC_VFT_BASE_MASK;
		*num = ((sqc_vft >> QM_SQC_VFT_SQ_NUM_SHIFT) & QM_SQC_VFT_NUM_MASK) + 1;

Uh oh!

Conversation

xiaoxuZeng commented Oct 11, 2023

Uh oh!

xiaoxuZeng commented Oct 11, 2023

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZijjWang commented Oct 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

etienne-lms left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiaoxuZeng commented Nov 14, 2023

Uh oh!

jforissier left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jforissier commented Nov 14, 2023

Uh oh!

xiaoxuZeng commented Nov 15, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jforissier commented Nov 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ZijjWang commented Oct 17, 2023 •

edited

Loading