RE: RE: [EXT] Re: [PATCH 4/4] firmware: imx: add driver for NXP EdgeLock Enclave

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Sent: Monday, May 20, 2024 4:32 PM
> To: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> Cc: Jonathan Corbet <corbet@xxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Sascha Hauer
> <s.hauer@xxxxxxxxxxxxxx>; Pengutronix Kernel Team
> <kernel@xxxxxxxxxxxxxx>; Fabio Estevam <festevam@xxxxxxxxx>; linux-
> doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: RE: [EXT] Re: [PATCH 4/4] firmware: imx: add driver for NXP
> EdgeLock Enclave
> 
> On 17.05.2024 11:24:46, Pankaj Gupta wrote:
> > > > new file mode 100644
> > > > index 000000000000..0463f26d93c7
> > > > --- /dev/null
> > > > +++ b/drivers/firmware/imx/ele_base_msg.c
> > > > @@ -0,0 +1,287 @@
> > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > +/*
> > > > + * Copyright 2024 NXP
> > > > + */
> > > > +
> > > > +#include <linux/types.h>
> > > > +#include <linux/completion.h>
> > > > +#include <linux/dma-mapping.h>
> > > > +
> > > > +#include "ele_base_msg.h"
> > > > +#include "ele_common.h"
> > > > +
> > > > +int ele_get_info(struct device *dev, struct soc_info *s_info) {
> > > > +	struct se_if_priv *priv = dev_get_drvdata(dev);
> > > > +	struct se_api_msg *tx_msg __free(kfree);
> > > > +	struct se_api_msg *rx_msg __free(kfree);
> > > > +	phys_addr_t get_info_addr;
> > > > +	u32 *get_info_data;
> > > > +	u32 status;
> > > > +	int ret;
> > > > +
> > > > +	if (!priv || !s_info)
> > > > +		goto exit;
> > >
> > > You should code properly, so that this doesn't happen, your cleanup
> > > is broken, it will work on uninitialized data, as Sascha already mentioned.
> >
> > The API(s) part of this file will be later exported and might get used by
> driver/crypto/ele/*.c.
> > Still if you think, this check should be removed, I will do it in v2.
> 
> It makes no sense to call these functions with NULL pointers, if you do so, it's
> a mistake by the caller. If it's used by some other part of the ele driver that
> should be coded properly.
> 
Will remove this change in v2.

> > > > +
> > > > +	memset(s_info, 0x0, sizeof(*s_info));
> > > > +
> > > > +	if (priv->mem_pool_name)
> > > > +		get_info_data = get_phy_buf_mem_pool(dev,
> > > > +						     priv->mem_pool_name,
> > > > +						     &get_info_addr,
> > > > +						     ELE_GET_INFO_BUFF_SZ);
> > > > +	else
> > > > +		get_info_data = dmam_alloc_coherent(dev,
> > > > +						    ELE_GET_INFO_BUFF_SZ,
> > > > +						    &get_info_addr,
> > > > +						    GFP_KERNEL);
> > >
> > > It's better style to move the init of the dma memory into the probe
> > > function.
> >
> > It is not DMA init. It is DMA allocation.
> 
> It's better style to move the allocation of the dma memory into the probe
> function.
> 
The buffer 'get_info_data', is allocated and freed within this function.
This API is called multiple times:
- as part of probe.
- as part of suspend/resume.

Why to keep the memory retained?

> 
> [...]
> 
> > > > +	priv->rx_msg = rx_msg;
> > > > +	ret = imx_ele_msg_send_rcv(priv, tx_msg);
> > >
> > > This API looks strange, why put the tx_msg as a parameter the rx_msg
> > > into the private struct?
> >
> > The rx_msg is the populated in the interrupt context. Hence, it kept
> > as part of private structure; which is in-turn associated with
> > mbox_client.
> 
> These are implementation details, it just feels strange to pass one parameter
> via an arguments and put the other in the private pointer.
> 
> > Though, in v2 moving the rx_msg setting to imx_ele_msg_send_rcv(priv,
> > tx_msg, rx_msg);
> 
> fine
> 
> [...]
> 
> > > > +	if (status != priv->success_tag) {
> > > > +		dev_err(dev, "Command Id[%d], Response Failure = 0x%x",
> > > > +			ELE_GET_INFO_REQ, status);
> > > > +		ret = -1;
> > > > +	}
> > > > +
> > > > +	s_info->imem_state = (get_info_data[ELE_IMEM_STATE_WORD]
> > > > +				& ELE_IMEM_STATE_MASK) >> 16;
> > >
> > > can you use a struct for get_info_data and use FIELD_GET() (if
> > > needed)
> >
> > Re-write the structure soc_info, matching the information provided in
> > response to this api.
> 
> Looks better. Please compile the driver and check with "pahole" that the
> layout of these structures doesn't contain any unwanted padding.
> Otherwise add "__packed" and if you can guarantee "__aligned(4)".
> 

Structure is copied from the already deployed user-space library.
Validated that each variable is printing correct value.
Even below code changes are done:
-       tx_msg->data[2] = ELE_GET_INFO_READ_SZ;
+       tx_msg->data[2] = sizeof(struct soc_info);

> > struct dev_info {
> >         uint8_t  cmd;
> >         uint8_t  ver;
> >         uint16_t length;
> >         uint16_t soc_id;
> >         uint16_t soc_rev;
> >         uint16_t lmda_val;
> >         uint8_t  ssm_state;
> >         uint8_t  dev_atts_api_ver;
> >         uint8_t  uid[MAX_UID_SIZE];
> >         uint8_t  sha_rom_patch[DEV_GETINFO_ROM_PATCH_SHA_SZ];
> >         uint8_t  sha_fw[DEV_GETINFO_FW_SHA_SZ]; };
> >
> > struct dev_addn_info {
> >         uint8_t  oem_srkh[DEV_GETINFO_OEM_SRKH_SZ];
> >         uint8_t  trng_state;
> >         uint8_t  csal_state;
> >         uint8_t  imem_state;
> >         uint8_t  reserved2;
> > };
> >
> > struct soc_info {
> >         struct dev_info d_info;
> >         struct dev_addn_info d_addn_info; };
> 
> [...]
> 
> > > > +int imx_ele_msg_send(struct se_if_priv *priv, void *mssg) {
> > > > +	bool is_cmd_lock_tobe_taken = false;
> > > > +	int err;
> > > > +
> > > > +	if (!priv->waiting_rsp_dev || priv->no_dev_ctx_used) {
> > > > +		is_cmd_lock_tobe_taken = true;
> > > > +		mutex_lock(&priv->se_if_cmd_lock);
> > > > +	}
> > > > +	scoped_guard(mutex, &priv->se_if_lock);
> > > > +
> > > > +	err = mbox_send_message(priv->tx_chan, mssg);
> > > > +	if (err < 0) {
> > > > +		dev_err(priv->dev, "Error: mbox_send_message failure.\n");
> > > > +		if (is_cmd_lock_tobe_taken)
> > > > +			mutex_unlock(&priv->se_if_cmd_lock);
> > >
> > > Only dropping the lock in case of failure doesn't look right to me.
> >
> > The callers of this function, takes the execution flow to aborting the
> > operation on getting return code < 0. No next action is expected under
> > this aborted operation. Unlocking the lock here is not an issue
> >
> > > It seems you should better move the lock to the callers of this function.
> >
> > Accepted, and moved to the caller of the function for:
> >    - locking
> >    - unlocking in case of error.
> >
> > Unlocking in the read API, once response is successfully received and
> > read.
> 
> A better design would be: imx_ele_msg_rcv() imx_ele_msg_send() are
> expected to be called locked. Add lockdep_assert_held() to these function to
> document/check this.
> 
> The callers of imx_ele_msg_rcv() and imx_ele_msg_send() have to take care
> of the locking.
> 
> [...]
> 
The locking/unlocking of se_if_cmd_lock, is taken care by the callers only:
- imx_ele_msg_send_rcv calls both the functions:
  --imx_ele_msg_send.
  --imx_ele_msg_rcv.

But the lockdep_assert_held, cannot be added to imx_ele_msg_send, as its another caller function imx_ele_miscdev_msg_send calls if for sending:
 --- command (here command lock is taken).
 --- response to a command (here command lock is not taken).

Will add lockdep_assert_held, to receive path, in v2.

> > > > +static const struct imx_se_node_info_list imx8ulp_info = {
> > > > +	.num_mu = 1,
> > > > +	.soc_id = SOC_ID_OF_IMX8ULP,
> > > > +	.info = {
> > > > +			{
> > > > +				.se_if_id = 2,
> > > > +				.se_if_did = 7,
> > > > +				.max_dev_ctx = 4,
> > > > +				.cmd_tag = 0x17,
> > > > +				.rsp_tag = 0xe1,
> > > > +				.success_tag = 0xd6,
> > > > +				.base_api_ver = MESSAGING_VERSION_6,
> > > > +				.fw_api_ver = MESSAGING_VERSION_7,
> > > > +				.se_name = "hsm1",
> > > > +				.mbox_tx_name = "tx",
> > > > +				.mbox_rx_name = "rx",
> > > > +				.pool_name = "sram",
> > > > +				.fw_name_in_rfs = IMX_ELE_FW_DIR\
> > >                                                                 ^
> > >                                                            not
> > > needed
> >
> > It is needed for i.MX8ULP, dual FW support.
> 
> The backslash is not needed.
Accepted. Will correct in v2.

> 
> >
> > > > +						  "mx8ulpa2ext-ahab-
> container.img",
> >
> >
> > > > +				.soc_register = true,
> > > > +				.reserved_dma_ranges = true,
> > > > +				.imem_mgmt = true,
> > > > +			},
> > > > +	},
> > > > +};
> > > > +
> > > > +static const struct imx_se_node_info_list imx93_info = {
> > > > +	.num_mu = 1,
> > > > +	.soc_id = SOC_ID_OF_IMX93,
> > > > +	.info = {
> > > > +			{
> > > > +				.se_if_id = 2,
> > > > +				.se_if_did = 3,
> > > > +				.max_dev_ctx = 4,
> > > > +				.cmd_tag = 0x17,
> > > > +				.rsp_tag = 0xe1,
> > > > +				.success_tag = 0xd6,
> > > > +				.base_api_ver = MESSAGING_VERSION_6,
> > > > +				.fw_api_ver = MESSAGING_VERSION_7,
> > > > +				.se_name = "hsm1",
> > > > +				.mbox_tx_name = "tx",
> > > > +				.mbox_rx_name = "rx",
> > > > +				.reserved_dma_ranges = true,
> > > > +				.imem_mgmt = true,
> > > > +				.soc_register = true,
> > > > +			},
> > > > +	},
> > >
> > >
> > > Some (most?) members of these structs are the same. Why do you have
> > > this abstraction if it's not needed right now?
> >
> > It is needed as the values is different for different NXP SoC
> > compatible. It will be needed for NXP i.MX95 platform, whose code will
> > be next in pipeline.
> 
> How does the imx95 .info look like?
> 
Copied from the internal repo.
static const struct imx_info_list imx95_info = {
        .num_mu = 4,
        .soc_id = SOC_ID_OF_IMX95,
        .info = {
                        {
                                .socdev = false,
                                .mu_id = 2,
                                .mu_did = 3,
                                .max_dev_ctx = 4,
                                .cmd_tag = 0x17,
                                .rsp_tag = 0xe1,
                                .success_tag = 0xd6,
                                .base_api_ver = MESSAGING_VERSION_6,
                                .fw_api_ver = MESSAGING_VERSION_7,
                                .se_name = "hsm1",
                                .mbox_tx_name = "tx",
                                .mbox_rx_name = "rx",
                                .pool_name = NULL,
                                .reserved_dma_ranges = false,
                                .init_fw = true,
                                .v2x_state_check = true,
                                .start_rng = ele_start_rng,
                                .enable_ele_trng = true,
                                .imem_mgmt = false,
                                .mu_buff_size = 0,
                                .fw_name_in_rfs = NULL,
                        },
                        {
                                .socdev = false,
                                .mu_id = 0,
                                .mu_did = 0,
                                .max_dev_ctx = 0,
                                .cmd_tag = 0x17,
                                .rsp_tag = 0xe1,
                                .success_tag = 0xd6,
                                .base_api_ver = 0x2,
                                .fw_api_ver = 0x2,
                                .se_name = "v2x_dbg",
                                .pool_name = NULL,
                                .mbox_tx_name = "tx",
                                .mbox_rx_name = "rx",
                                .reserved_dma_ranges = false,
                                .init_fw = false,
                                .v2x_state_check = true,
                                .start_rng = v2x_start_rng,
                                .enable_ele_trng = false,
                                .imem_mgmt = false,
                                .mu_buff_size = 0,
                                .fw_name_in_rfs = NULL,
                        },
                        {
                                .socdev = false,
                                .mu_id = 4,
                                .mu_did = 0,
                                .max_dev_ctx = 4,
                                .cmd_tag = 0x18,
                                .rsp_tag = 0xe2,
                                .success_tag = 0xd6,
                                .base_api_ver = 0x2,
                                .fw_api_ver = 0x2,
                                .se_name = "v2x_sv0",
                                .pool_name = NULL,
                                .mbox_tx_name = "tx",
                                .mbox_rx_name = "rx",
                                .reserved_dma_ranges = false,
                                .init_fw = false,
                                .v2x_state_check = true,
                                .start_rng = NULL,
                                .enable_ele_trng = false,
                                .imem_mgmt = false,
                                .mu_buff_size = 16,
                                .fw_name_in_rfs = NULL,
                        },
                        {
                                .socdev = false,
                                .mu_id = 6,
                                .mu_did = 0,
                                .max_dev_ctx = 4,
                                .cmd_tag = 0x1a,
                                .rsp_tag = 0xe4,
                                .success_tag = 0xd6,
                                .base_api_ver = 0x2,
                                .fw_api_ver = 0x2,
                                .se_name = "v2x_she",
                                .pool_name = NULL,
                                .mbox_tx_name = "tx",
		   .mbox_rx_name = "rx",
                                .reserved_dma_ranges = false,
                                .init_fw = false,
                                .v2x_state_check = true,
                                .start_rng = NULL,
                                .enable_ele_trng = false,
                                .imem_mgmt = false,
                                .mu_buff_size = 16,
                                .fw_name_in_rfs = NULL,
                        },
                        {
                                .socdev = false,
                                .mu_id = 6,
                                .mu_did = 0,
                                .max_dev_ctx = 4,
                                .cmd_tag = 0x1a,
                                .rsp_tag = 0xe4,
                                .success_tag = 0xd6,
                                .base_api_ver = 0x2,
                                .fw_api_ver = 0x2,
                                .se_name = "v2x_she",
                                .pool_name = NULL,
                                .mbox_tx_name = "tx",
                                .mbox_rx_name = "rx",
                                .reserved_dma_ranges = false,
                                .init_fw = false,
                                .v2x_state_check = true,
                                .start_rng = NULL,
                                .enable_ele_trng = false,
                                .imem_mgmt = false,
                                .mu_buff_size = 256,
                                .fw_name_in_rfs = NULL,
                        },
        }
};
> 
> > > > +static int imx_fetch_soc_info(struct device *dev) {
> > > > +	struct se_if_priv *priv = dev_get_drvdata(dev);
> > > > +	struct imx_se_node_info_list *info_list;
> > > > +	const struct imx_se_node_info *info;
> > > > +	struct soc_device_attribute *attr;
> > > > +	struct soc_device *sdev;
> > > > +	struct soc_info s_info;
> > > > +	int err = 0;
> > > > +
> > > > +	info = priv->info;
> > > > +	info_list = (struct imx_se_node_info_list *)
> > > > +				device_get_match_data(dev->parent);
> > >
> > > I think cast is not needed.
> >
> > It returns memory reference with const attribute. SoC revision member
> > of 'info_list', is required to be updated. Thus type casted.
> 
> Have you considered that this memory is marked as const for a reason?
> It's const, you cannot change it. Place any values that have to changed into
> your priv.

Created a static variable g_soc_rev in the se_ctrl.c.
Accepted and will correct it in v2.

> 
> > > > +	if (info_list->soc_rev)
> > > > +		return err;
> > >
Will change the above condition to g_soc_rev.

> > > What does this check do? You'll only get data you put in the
> > > info_list in the first place.
> 
> > info_list->soc_rev, is equal to zero for the first call to this
> > function. To return from this function if this function is already
> > executed.
> 
> This looks wrong, see above.

Accepted and will correct it in v2.

> 
> > > > +	err = ele_get_info(dev, &s_info);
> > > > +	if (err)
> > > > +		s_info.major_ver = DEFAULT_IMX_SOC_VER;
> > >
> > > Why continue here in case of error?
> >
> > To continue with SoC registration for the default values (without
> > fetching information from ELE).
> 
> Have you tested the driver that it will work, if this fails?
Tested in unit testing by making err equal to non-zero.
Showing soc revision and serial number are shown as zeros.

But, I agree with you to return failure. As there is no point continuing if the SE probe failed.
Earlier I was thinking to allow other modules depending on soc registration info, can work.

Accepted and will not continue in case of failure in V2.

> 
> > > > +
> > > > +	info_list->soc_rev = s_info.soc_rev;
> > > > +
> > > > +	if (!info->soc_register)
> > > > +		return 0;
> > > > +
> > > > +	attr = devm_kzalloc(dev, sizeof(*attr), GFP_KERNEL);
> > > > +	if (!attr)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	if (s_info.minor_ver)
> > > > +		attr->revision = devm_kasprintf(dev, GFP_KERNEL, "%x.%x",
> > > > +					   s_info.major_ver,
> > > > +					   s_info.minor_ver);
> > > > +	else
> > > > +		attr->revision = devm_kasprintf(dev, GFP_KERNEL, "%x",
> > > > +					   s_info.major_ver);
> > > > +
> > > > +	switch (s_info.soc_id) {
> > > > +	case SOC_ID_OF_IMX8ULP:
> > > > +		attr->soc_id = devm_kasprintf(dev, GFP_KERNEL,
> > > > +					      "i.MX8ULP");
> > > > +		break;
> > > > +	case SOC_ID_OF_IMX93:
> > > > +		attr->soc_id = devm_kasprintf(dev, GFP_KERNEL,
> > > > +					      "i.MX93");
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	err = of_property_read_string(of_root, "model",
> > > > +				      &attr->machine);
> > > > +	if (err) {
> > > > +		devm_kfree(dev, attr);
> > >
> > > Why do you do a manual cleanup of devm managed resources? Same
> > > applies to the other devm managed resources, too.
> > >
> > Used devm managed memory, as this function is called as part probe.
> > Post device registration, this devm managed memory is un-necessarily
> > blocked. It is better to release it as part of clean-up, under this
> > function only.
> 
> Why do you allocate the memory with devm in the first place, if it's not
> needed after probe?

Sorry to confuse you. Actually the devm_memory will be needed for the case of soc_registration.
Meaning, memory with devm, will be needed post probing as well.

If this function fails, the probing will fail too. It will be auto cleaned.

Accepted, will remove the devm_free in v2.

> 
> > Other devm managed memory clean-up, under se_probe_cleanup, will be
> > removed, as suggested.
> 
> regards,
> Marc
> 
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux