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

From: Pankaj Gupta
Date: Mon Jul 15 2024 - 05:19:41 EST

Next message: Markus Elfring: "[PATCH] bpf: Simplify character output in seq_print_delegate_opts()"
Previous message: Stephan Gerhold: "Re: [PATCH] power: supply: qcom_battmgr: Ignore extra __le32 in info payload"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> -----Original Message-----
> From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> Sent: Monday, July 1, 2024 2:17 PM
> To: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> Cc: Jonathan Corbet <corbet@xxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Pengutronix
> Kernel Team <kernel@xxxxxxxxxxxxxx>; Fabio Estevam
> <festevam@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>; devicetree@xxxxxxxxxxxxxxx;
> imx@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx
> Subject: Re: [EXT] Re: [PATCH v3 4/5] firmware: imx: add driver for NXP
> EdgeLock Enclave
>
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
>
>
> On Mon, Jul 01, 2024 at 07:45:20AM +0000, Pankaj Gupta wrote:
> >
> >
> > > -----Original Message-----
> > > From: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > > Sent: Tuesday, June 18, 2024 2:02 PM
> > > To: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> > > Cc: Jonathan Corbet <corbet@xxxxxxx>; Rob Herring <robh@xxxxxxxxxx>;
> > > Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor Dooley
> > > <conor+dt@xxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Pengutronix
> > > Kernel Team <kernel@xxxxxxxxxxxxxx>; Fabio Estevam
> > > <festevam@xxxxxxxxx>; Rob Herring <robh+dt@xxxxxxxxxx>; Krzysztof
> > > Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>;
> > > devicetree@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> > > linux-kernel@xxxxxxxxxxxxxxx; linux-arm- kernel@xxxxxxxxxxxxxxxxxxx;
> > > linux-doc@xxxxxxxxxxxxxxx
> > > Subject: [EXT] Re: [PATCH v3 4/5] firmware: imx: add driver for NXP
> > > EdgeLock Enclave
> > >
> > > Caution: This is an external email. Please take care when clicking
> > > links or opening attachments. When in doubt, report the message
> > > using the 'Report this email' button
> > >
> > >
> > > Hi Pankaj,
> > >
> > > Here's some review feedback. I think it'll take some more rounds to
> > > get this into shape.
> > >
> > > On Mon, Jun 17, 2024 at 12:59:42PM +0530, Pankaj Gupta wrote:
> > > > NXP hardware IP(s) for secure-enclaves like Edgelock Enclave(ELE),
> > > > are embedded in the SoC to support the features like HSM, SHE &
> > > > V2X, using message based communication interface.
> > > >
> > > > The secure enclave FW communicates on a dedicated messaging
> > > > unit(MU) based interface(s) with application core, where kernel is
> running.
> > > > It exists on specific i.MX processors. e.g. i.MX8ULP, i.MX93.
> > > >
> > > > This patch adds the driver for communication interface to
> > > > secure-enclave, for exchanging messages with NXP secure enclave HW
> > > > IP(s) like EdgeLock Enclave (ELE) from Kernel-space, used by
> > > > kernel management layers like
> > > > - DM-Crypt.
> > > >
> > > > Signed-off-by: Pankaj Gupta <pankaj.gupta@xxxxxxx>
> > > > ---
> > > > drivers/firmware/imx/Kconfig | 12 +
> > > > drivers/firmware/imx/Makefile | 2 +
> > > > drivers/firmware/imx/ele_base_msg.c | 284 +++++++++++++++++++
> > > > drivers/firmware/imx/ele_base_msg.h | 90 ++++++
> > > > drivers/firmware/imx/ele_common.c | 233 ++++++++++++++++
> > > > drivers/firmware/imx/ele_common.h | 45 +++
> > > > drivers/firmware/imx/se_ctrl.c | 536
> > > ++++++++++++++++++++++++++++++++++++
> > > > drivers/firmware/imx/se_ctrl.h | 99 +++++++
> > > > include/linux/firmware/imx/se_api.h | 14 +
> > > > 9 files changed, 1315 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/imx/Kconfig
> > > > b/drivers/firmware/imx/Kconfig index 183613f82a11..56bdca9bd917
> > > > 100644
> > > > --- a/drivers/firmware/imx/Kconfig
> > > > +++ b/drivers/firmware/imx/Kconfig
> > > > @@ -22,3 +22,15 @@ config IMX_SCU
> > > >
> > > > This driver manages the IPC interface between host CPU and the
> > > > SCU firmware running on M4.
> > > > +
> > > > +config IMX_SEC_ENCLAVE
> > > > + tristate "i.MX Embedded Secure Enclave - EdgeLock Enclave
> > > > +Firmware
> > > driver."
> > > > + depends on IMX_MBOX && ARCH_MXC && ARM64
> > > > + default m if ARCH_MXC
> > > > +
> > > > + help
> > > > + It is possible to use APIs exposed by the iMX Secure
> > > > + Enclave HW IP
> > > called:
> > > > + - EdgeLock Enclave Firmware (for i.MX8ULP, i.MX93),
> > > > + like base, HSM, V2X & SHE using the SAB protocol via
> > > > + the shared
> > > Messaging
> > > > + Unit. This driver exposes these interfaces via a set of file descriptors
> > > > + allowing to configure shared memory, send and receive messages.
> > > > diff --git a/drivers/firmware/imx/Makefile
> > > > b/drivers/firmware/imx/Makefile index 8f9f04a513a8..aa9033e0e9e3
> > > > 100644
> > > > --- a/drivers/firmware/imx/Makefile
> > > > +++ b/drivers/firmware/imx/Makefile
> > > > @@ -1,3 +1,5 @@
> > > > # SPDX-License-Identifier: GPL-2.0
> > > > obj-$(CONFIG_IMX_DSP) += imx-dsp.o
> > > > obj-$(CONFIG_IMX_SCU) += imx-scu.o misc.o imx-scu-irq.o rm.o
> > > imx-scu-soc.o
> > > > +sec_enclave-objs = se_ctrl.o ele_common.o ele_base_msg.o
> > > > +obj-${CONFIG_IMX_SEC_ENCLAVE} += sec_enclave.o
> > > > diff --git a/drivers/firmware/imx/ele_base_msg.c
> > > > b/drivers/firmware/imx/ele_base_msg.c
> > > > new file mode 100644
> > > > index 000000000000..5bfd9c7e3f7e
> > > > --- /dev/null
> > > > +++ b/drivers/firmware/imx/ele_base_msg.c
> > > > @@ -0,0 +1,284 @@
> > > > +// 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 ele_dev_info *s_info)
> > > > +{
> > >
> > > I think all currently exported functions should take a struct
> > > se_if_priv
> > > * as context pointer.
> > > I can't find any place in which any of these functions is called
> > > differently than with priv->dev.
> >
> > All the API(s) that construct a message to be exchanged over the
> > device-interface to FW,
> > - will be the exported symbols in the next patch-set, to be used by other
> Linux kernel modules like: NVMEM driver, linux crypto framework,
> security/keys etc.
> > - These other Linux layers have to choose from multiple similar devices per
> secure-enclave.
> >
> > Kindly Consider these API(s), to be the EXPORT SYMBOLS, in later patches,
> when used outside of this driver.
>
> In that case you could still add a function which translates a struct device *
> into a struct se_if_priv *.
>
> > >
> > > > + struct se_if_priv *priv = dev_get_drvdata(dev);
>
> This function should also include some sanity checks. It's not good that an
> exported function takes some struct device *, blindly assumes that it is of type
> se_if_priv, and if not just crashes the Kernel.

Will add a wrapper function over "struct se_if_priv *priv = dev_get_drvdata(dev);", to add some safety checks.
Will fix this in V6.

>
> > > > +static int imx_fetch_se_soc_info(struct se_if_priv *priv,
> > > > + const struct imx_se_node_info_list
> > > > +*info_list) {
> > > > + const struct imx_se_node_info *info;
> > > > + struct soc_device_attribute *attr;
> > > > + struct soc_device *sdev;
> > > > + u64 serial_num;
> > > > + u16 soc_rev;
> > > > + int err = 0;
> > > > +
> > > > + info = priv->info;
> > > > +
> > > > + /* This function should be called once.
> > > > + * Check if the soc_rev is zero to continue.
> > > > + */
> > > > + if (priv->soc_rev)
> > > > + return err;
> > >
> > > Just return 0 here. It takes one step less to understand what this is about.
> > Replacing "err" with "ret", in better understanding.
>
> What I meant that you should return the constant '0' here instead of the
> content of a variable. It safes a reader from looking up the value of the variable
> which means it's one step less for the brain to understand the code.
>
> > > > +
> > > > + if (info->se_fetch_soc_info) {
> > > > + err = info->se_fetch_soc_info(priv->dev, &soc_rev, &serial_num);
> > > > + if (err < 0) {
> > > > + dev_err(priv->dev, "Failed to fetch SoC Info.");
> > > > + return err;
> > > > + }
> > > > + } else {
> > > > + dev_err(priv->dev, "Failed to fetch SoC revision.");
> > > > + if (info->soc_register)
> > > > + dev_err(priv->dev, "Failed to do SoC registration.");
> > > > + err = -EINVAL;
> > > > + return err;
> > > > + }
> > >
> > > i.MX93 doesn't have a info->se_fetch_soc_info. Does this mean it
> > > doesn't work on this SoC?
> > >
> > Yes.
>
> Will you fix this?
For i.MX93, SoC registration is not done through this driver.
This is implemented as this only. Nothing to be fixed.

>
> > > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > > > + if (!priv) {
> > > > + ret = -ENOMEM;
> > > > + goto exit;
> > > > + }
> > > > +
> > > > + dev_set_drvdata(dev, priv);
> > > > +
> > > > + /* Mailbox client configuration */
> > > > + priv->se_mb_cl.dev = dev;
> > > > + priv->se_mb_cl.tx_block = false;
> > > > + priv->se_mb_cl.knows_txdone = true;
> > > > + priv->se_mb_cl.rx_callback = se_if_rx_callback;
> > > > +
> > > > + ret = se_if_request_channel(dev, &priv->tx_chan,
> > > > + &priv->se_mb_cl, info->mbox_tx_name);
> > > > + if (ret)
> > > > + goto exit;
> > > > +
> > > > + ret = se_if_request_channel(dev, &priv->rx_chan,
> > > > + &priv->se_mb_cl, info->mbox_rx_name);
> > > > + if (ret)
> > > > + goto exit;
> > > > +
> > > > + priv->dev = dev;
> > > > + priv->info = info;
> > > > +
> > > > + mutex_init(&priv->se_if_lock);
> > > > + mutex_init(&priv->se_if_cmd_lock);
> > > > +
> > > > + priv->cmd_receiver_dev = NULL;
> > > > + priv->waiting_rsp_dev = NULL;
> > >
> > > These are NULL already.
> > For code readability, it is good to know when and with what value it is
> initialized.
> > It will help review the 'if' condition based on these structure member
> variable.
> > Will covert this information into comments.
>
> We already know they are NULL because you used kzalloc to allocate the
> struct. No need to comment that.
>
> Sascha
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 |
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> pengutronix.de%2F&data=05%7C02%7Cpankaj.gupta%40nxp.com%7Cc8b7
> b605e99744ccf94e08dc99aa66f0%7C686ea1d3bc2b4c6fa92cd99c5c30163
> 5%7C0%7C0%7C638554204358183687%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3
> D%7C0%7C%7C%7C&sdata=Nl8R%2FcwuT69VVUxe00AichgoSEEJexZ0TfhjfuI
> BqoY%3D&reserved=0 |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Next message: Markus Elfring: "[PATCH] bpf: Simplify character output in seq_print_delegate_opts()"
Previous message: Stephan Gerhold: "Re: [PATCH] power: supply: qcom_battmgr: Ignore extra __le32 in info payload"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]