Add support NXP S32 HSE CRYPTO driver for S32Z270 #79351

haduongquang · 2024-10-03T02:14:40Z

This PR introduces NXP S32 HSE CRYTO driver for SoC NXP S32Z27 and enables its usage for board s32z270dc2.
Supports cryptographic operations, including hashing and symmetric ciphers, with capabilities for ECB, CBC, and CTR modes using RAM-based key catalogs with 128-bit key lengths.

tests\crypto\crypto_hash:

SUITE PASS - 100.00% [crypto_hash]: pass = 1, fail = 0, skip = 0, total = 1 duration = 0.001 seconds
 PASS - [crypto_hash.test_hash] duration = 0.001 seconds

samples\drivers\crypto:

I: Cipher Sample
I: ECB Mode
I: Output length (encryption): 0
I: ECB mode ENCRYPT - Match
I: Output length (decryption): 0
I: ECB mode DECRYPT - Match
I: CBC Mode
I: Output length (encryption): 80
I: CBC mode ENCRYPT - Match
I: Output length (decryption): 0
I: CBC mode DECRYPT - Match
I: CTR Mode
I: Output length (encryption): 0
I: CTR mode ENCRYPT - Match
I: Output length (decryption): 0
I: CTR mode DECRYPT - Match

zephyrbot · 2024-10-03T02:15:32Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff
hal_nxp	zephyrproject-rtos/hal_nxp@`4a4741f` (master)	zephyrproject-rtos/hal_nxp#443	zephyrproject-rtos/hal_nxp#443/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

haduongquang · 2024-10-03T08:09:51Z

Fixed compliance, clang and build fail.

Dat-NguyenDuy · 2024-10-07T00:22:25Z

drivers/crypto/Kconfig.nxp_hse

+
+config CRYPTO_NXP_HSE_FORMAT_KEY_CATALOG
+	bool "NXP HSE crypto driver supports formatting all key catalogs during initialization."
+	default n


remove this, already n by default

Dat-NguyenDuy · 2024-10-07T00:24:46Z

drivers/crypto/Kconfig.nxp_hse

@@ -0,0 +1,30 @@
+menuconfig CRYPTO_NXP_HSE


Pls use nxp_s32 for Kconfig name and file (for consistent with the rest of driver specific for nxp s32 platform)

I updated kconfig, file name and driver also.

Dat-NguyenDuy · 2024-10-07T00:39:48Z

dts/arm/nxp/nxp_s32z27x_r52.dtsi

@@ -1116,5 +1116,53 @@
 			clock-frequency = <I2C_BITRATE_STANDARD>;
 			status = "disabled";
 		};
+
+		mu0_mua: mu@23258000 {


should be mub? mua is accessible from HSE only

Dat-NguyenDuy · 2024-10-07T00:55:47Z

drivers/crypto/crypto_nxp_hse.c

+
+struct crypto_nxp_hse_data {
+	struct crypto_nxp_hse_session sessions[CONFIG_CRYPTO_NXP_HSE_MAX_SESSION];
+	uint8_t mu_instance;


The instance can be moved to config imo

I'm not using the config now, so I think we can keep it in data is better.

this is read only, it must be moved to the device config

Dat-NguyenDuy · 2024-10-07T00:57:34Z

drivers/crypto/crypto_nxp_hse.c

+	if (mu_channel != HSE_IP_INVALID_MU_CHANNEL_U8) {
+		session = &data->sessions[mu_channel - 1];
+		session->in_use = true;
+		session->channel = mu_channel;


It seems to me this can be done at build time

Dat-NguyenDuy · 2024-10-07T01:05:14Z

drivers/crypto/crypto_nxp_hse.c

+	}
+	default: {
+		return -ENOTSUP;
+	}


pls fix indentation

not the wrong indentation, it is the correct format.

Dat-NguyenDuy · 2024-10-07T01:05:45Z

drivers/crypto/crypto_nxp_hse.c

+
+	session = ctx->drv_sessn_state;
+	k_mutex_lock(&session->crypto_lock, K_FOREVER);
+	memset(&session->req_type, 0, sizeof(Hse_Ip_ReqType));


why you need to clear the whole descriptor? it seems to me this is not needed

To ensure the new description is correct, and unaffected by the old.

Dat-NguyenDuy · 2024-10-07T01:10:37Z

boards/nxp/s32z2xxdc2/doc/index.rst

+with 128-bit key lengths.
+
+.. note::
+   Applications must format key catalogs before executing the first service.


I see you have defined an Kconfig to format key catalog for testing, should we documented it here?

I documented it.

Dat-NguyenDuy · 2024-10-07T01:13:15Z

boards/nxp/s32z2xxdc2/doc/index.rst

+.. note::
+   Applications must format key catalogs before executing the first service.
+
+   By default, MU0 is used, and other MUs are deactivated. If applications want to use other MUs,


What do you meant by default, MU0 is used? is used by who, the crypto driver?

If applications want to use other MUs, must activate them first: why don't active the MU inside crypto driver?

manuargue · 2024-10-10T03:14:06Z

drivers/crypto/Kconfig.nxp_s32_hse

+config CRYPTO_NXP_S32_HSE_FORMAT_KEY_CATALOG
+	bool "NXP S32 HSE crypto driver supports formatting all key catalogs during initialization."
+	depends on CRYPTO_NXP_S32_HSE
+	help
+	  Enable supports formatting all key catalogs during initialization.
+	  This feature is primarily used for testing purposes.
+
+config CRYPTO_NXP_S32_HSE_OUTPUT_BUFFER_SIZE
+	int "The output buffer size for storing the output data of HSE crypto service"
+	default 128
+	depends on CRYPTO_NXP_S32_HSE
+	help
+	  The output buffer size for storing the output data of HSE crypto service.
+
+config CRYPTO_NXP_S32_HSE_AES_KEY_GROUP_ID
+	int "The AES 128-bits Key Group ID within RAM Key Catalog"
+	range 0 255
+	default 0
+	depends on CRYPTO_NXP_S32_HSE
+	help
+	  The AES 128-bits Key Group ID within RAM Key Catalog.


guard with CRYPTO_NXP_S32_HSE so all these options don't appear at toplevel and remove the depends on CRYPTO_NXP_S32_HSE

manuargue · 2024-10-10T03:19:33Z

samples/drivers/crypto/prj.conf

@@ -3,4 +3,4 @@ CONFIG_CRYPTO_LOG_LEVEL_DBG=y
 CONFIG_LOG=y
 CONFIG_LOG_MODE_MINIMAL=y

-CONFIG_MAIN_STACK_SIZE=4096
+CONFIG_MAIN_STACK_SIZE=8192


if this is only needed for a specific platform/board, then I'd suggest to add overlays

manuargue · 2024-10-10T03:26:47Z

drivers/crypto/crypto_nxp_s32_hse.c

please run clang-format on this new file, to conform with the suggested changes annotated by the CI

I have run the clang-format at locally, but some points are different with upstream.

1f7c960

if it's not consistent, please report the version you are using in an issue

manuargue · 2024-10-10T03:29:26Z

drivers/crypto/crypto_nxp_s32_hse.c

+
+#define CRYPTO_NXP_S32_HSE_SESSION_CFG(indx, _)                                                    \
+	{                                                                                          \
+		.channel = indx + 1, .out_buff = &crypto_out_buff[indx][0],                        \


this is a multiinstance driver, but the buffer crypto_out_buff is shared accross all instances (e.g. all instances channel 0 will get assigned the same buffer &crypto_out_buff[0][0]). There should be a buffer per instance and per channel.

manuargue · 2024-10-10T03:31:20Z

drivers/crypto/crypto_nxp_s32_hse.c

+#define CRYPTO_NXP_S32_HSE_MU_GET_INSTANCE(n)                                                      \
+	LISTIFY(__DEBRACKET HSE_IP_NUM_OF_MU_INSTANCES, CRYPTO_NXP_S32_HSE_MU_INSTANCE_CHECK, (|), n)
+
+#define CRYPTO_NXP_S32_HSE_BLOCK_KEY_LEN_BYTES 16U


instead of hardcoding it, it can be exposed as a dt property to use either 128 or 256 bits

I have added a Kconfig to do it

manuargue · 2024-10-10T03:32:20Z

drivers/crypto/crypto_nxp_s32_hse.c

+		return -EIO;
+	}
+
+	/* Update crypto crypto description input */


nitpicking

Suggested change

/* Update crypto crypto description input */

/* Update HSE descriptor */

maybe? and everywhere else where this is mentioned as "description" instead of "descriptor"

manuargue · 2024-10-10T03:43:59Z

drivers/crypto/crypto_nxp_s32_hse.c

+	Hse_Ip_ReqType crypto_req_type;
+	hseFormatKeyCatalogsSrv_t *format_key_serv =
+		&(crypto_serv_desc.hseSrv.formatKeyCatalogsReq);
+	uint8_t channel = Hse_Ip_GetFreeChannel(data->mu_instance);


this channel is never released, see Hse_Ip_ReleaseChannel

manuargue · 2024-10-10T03:45:51Z

drivers/crypto/crypto_nxp_s32_hse.c

+
+static inline void free_session(struct crypto_nxp_s32_hse_session *session)
+{
+	session->in_use = false;


MU channels are never released, see Hse_Ip_ReleaseChannel

manuargue · 2024-10-10T03:48:54Z

drivers/crypto/crypto_nxp_s32_hse.c

+static struct crypto_nxp_s32_hse_session *crypto_nxp_s32_hse_get_session(const struct device *dev)
+{
+	struct crypto_nxp_s32_hse_data *data = dev->data;
+	uint8_t mu_channel = Hse_Ip_GetFreeChannel(data->mu_instance);


this call seems not to be thread safe (ie. two threads could compete for the same MU channel with unknown results)

manuargue · 2024-10-10T03:54:36Z

drivers/crypto/Kconfig.nxp_s32_hse

+	depends on CRYPTO_NXP_S32_HSE
+	help
+	  Enable supports formatting all key catalogs during initialization.
+	  This feature is primarily used for testing purposes.


From the board's doc you added:

Applications must format key catalogs before executing the first service.

From this kconfig:

Enable supports formatting all key catalogs during initialization.
This feature is primarily used for testing purposes.

I don't think having test code inside the driver is a good idea. If formatting the key catalog must be done in the application layer and you want to show how is possible to do this, then perhaps create a sample.

I updated the samples/crypto to show how to do this.

I updated the samples/crypto to show how to do this.

IMO having NXP specific HAL calls in a generic sample is definetly not a good idea. If this sample is really necessary it should be at least in samples/boards/nxp.

Yep, that is a generic sample and should not have Hal specific calls.

manuargue · 2024-10-10T04:23:57Z

@haduongquang I'd suggest you to look at how the HSE was integrated into Linux in regards to devicetree. I think it's a cleaner approach than the current one proposed on this pr because it clearly shows that the MU's are coupled with the HSE, and there's also explicit description of the RAM reserved to communicate between the CPU host and the HSE.

haduongquang · 2024-10-16T02:39:56Z

@haduongquang I'd suggest you to look at how the HSE was integrated into Linux in regards to devicetree. I think it's a cleaner approach than the current one proposed on this pr because it clearly shows that the MU's are coupled with the HSE, and there's also explicit description of the RAM reserved to communicate between the CPU host and the HSE.

https://github.com/nxp-auto-linux/linux/blob/49d9f8bb25e0de43587acd9238949752f914a6d2/arch/arm64/boot/dts/freescale/s32cc.dtsi#L596

https://github.com/nxp-auto-linux/linux/blob/49d9f8bb25e0de43587acd9238949752f914a6d2/arch/arm64/boot/dts/freescale/s32cc.dtsi#L105-L109

For the first point, I have updated the device tree.
For the second point, regarding RAM reserved. Some other platforms require the descriptor to be put in a dedicated RAM region in order to communicate with HSE, this is not the case for S32Z so a RAM reserved is not needed.

manuargue · 2024-10-17T03:47:35Z

drivers/crypto/crypto_nxp_s32_hse.c

+	struct crypto_nxp_s32_hse_data *data = dev->data;
+	struct crypto_nxp_s32_hse_session *session;
+
+	if (Hse_Ip_Init(data->mu_instance, &data->mu_state) != HSE_IP_STATUS_SUCCESS) {


It's not enough to just call this initialiation but also the MUs that are not active by default must be activated through a request to HSE. As it is right now implemented, if the user tries to use the Zephyr Crypto API on a HSE-MU device that is not active, it will fail. In general, the device init function should ensure that all needed resources are allocated and ready in order for the driver to operate after exiting this function.

Add device tree node for MU instances that will be used by HSE and RTU for s32z270. Add support hash crypto for NXP S32 with Algo 2: SHA224, SHA256, SHA384 and SHA512. Add support cipher crypto with ECB, CBC and CTR mode by using ram key catalog. Signed-off-by: Ha Duong Quang <[email protected]>

Enable test for s32z270 hash crypto. Enable samples for cipher cryptoEndable samples for EBC, CBC, CTR mode of cipher crypto. Signed-off-by: Ha Duong Quang <[email protected]>

haduongquang requested review from mmahadevan108, dleach02 and manuargue as code owners October 3, 2024 02:14

zephyrbot added platform: NXP Drivers NXP Semiconductors, drivers area: Samples Samples area: mbox area: Crypto / RNG platform: NXP S32 NXP Semiconductors, S32 labels Oct 3, 2024

zephyrbot requested review from bperseghetti, carlocaione, ceolin, danieldegrasse, Dat-NguyenDuy, dbaluta, decsny, kartben, MarkWangChinese, nashif and PetervdPerk-NXP October 3, 2024 02:15

zephyrbot assigned ceolin Oct 3, 2024

zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Oct 3, 2024

haduongquang changed the title ~~Add support for NXP S32 HSE CRYPTO driver for S32Z270~~ Add support NXP S32 HSE CRYPTO driver for S32Z270 Oct 3, 2024

decsny removed their request for review October 3, 2024 05:26

haduongquang force-pushed the support-hse-driver-for-s32z270 branch 3 times, most recently from dc8d870 to 2ba0f4f Compare October 3, 2024 08:01

Dat-NguyenDuy requested changes Oct 7, 2024

View reviewed changes

haduongquang force-pushed the support-hse-driver-for-s32z270 branch 2 times, most recently from 19dbc6b to f861b34 Compare October 9, 2024 01:44

manuargue requested changes Oct 10, 2024

View reviewed changes

haduongquang force-pushed the support-hse-driver-for-s32z270 branch from f861b34 to 8fd4348 Compare October 16, 2024 02:20

manuargue requested changes Oct 17, 2024

View reviewed changes

haduongquang added 2 commits October 28, 2024 09:26

boards: enbables support for s32z270 hse crypto

949dba0

Enable test for s32z270 hash crypto. Enable samples for cipher cryptoEndable samples for EBC, CBC, CTR mode of cipher crypto. Signed-off-by: Ha Duong Quang <[email protected]>

haduongquang force-pushed the support-hse-driver-for-s32z270 branch from 8fd4348 to 949dba0 Compare October 28, 2024 02:30

	/* Update crypto crypto description input */
	/* Update HSE descriptor */

Add support NXP S32 HSE CRYPTO driver for S32Z270 #79351

Are you sure you want to change the base?

Add support NXP S32 HSE CRYPTO driver for S32Z270 #79351

Conversation

haduongquang commented Oct 3, 2024 • edited by Dat-NguyenDuy Loading

zephyrbot commented Oct 3, 2024 • edited Loading

haduongquang commented Oct 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuargue Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuargue Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuargue Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuargue commented Oct 10, 2024 • edited Loading

haduongquang commented Oct 16, 2024

manuargue Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

haduongquang commented Oct 3, 2024 •

edited by Dat-NguyenDuy

Loading

zephyrbot commented Oct 3, 2024 •

edited

Loading

manuargue Oct 10, 2024 •

edited

Loading

manuargue Oct 10, 2024 •

edited

Loading

manuargue Oct 16, 2024 •

edited

Loading

manuargue commented Oct 10, 2024 •

edited

Loading

manuargue Oct 17, 2024 •

edited

Loading