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

Sascha Hauer s.hauer at pengutronix.de
Mon Jul 1 01:47:07 PDT 2024 

On Mon, Jul 01, 2024 at 07:45:20AM +0000, Pankaj Gupta wrote:
> 
> 
> > -----Original Message-----
> > From: Sascha Hauer <s.hauer at pengutronix.de>
> > Sent: Tuesday, June 18, 2024 2:02 PM
> > To: Pankaj Gupta <pankaj.gupta at nxp.com>
> > Cc: Jonathan Corbet <corbet at lwn.net>; Rob Herring <robh at kernel.org>;
> > Krzysztof Kozlowski <krzk+dt at kernel.org>; Conor Dooley
> > <conor+dt at kernel.org>; Shawn Guo <shawnguo at kernel.org>; Pengutronix
> > Kernel Team <kernel at pengutronix.de>; Fabio Estevam <festevam at gmail.com>;
> > Rob Herring <robh+dt at kernel.org>; Krzysztof Kozlowski
> > <krzysztof.kozlowski+dt at linaro.org>; devicetree at vger.kernel.org;
> > imx at lists.linux.dev; linux-kernel at vger.kernel.org; linux-arm-
> > kernel at lists.infradead.org; linux-doc at vger.kernel.org
> > 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 at nxp.com>
> > > ---
> > >  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.

> > > +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?

> > > +     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                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

More information about the linux-arm-kernel mailing list