> -----Original Message-----
> From: Pankaj Gupta
> Sent: Monday, July 15, 2024 2:49 PM
> To: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> 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
>
>
>
> > -----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.
>
Will add for NULL check for priv data, as a sanity check.
> >
> > > > > +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 |
