Message-ID:
 <AM9PR04MB860407B8EEB86B3E7DD1E12695AD2@AM9PR04MB8604.eurprd04.prod.outlook.com>
Date: Fri, 19 Jul 2024 04:49:13 +0000
From: Pankaj Gupta <pankaj.gupta@....com>
To: Sascha Hauer <s.hauer@...gutronix.de>
CC: Jonathan Corbet <corbet@....net>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Shawn Guo
	<shawnguo@...nel.org>, Pengutronix Kernel Team <kernel@...gutronix.de>, Fabio
 Estevam <festevam@...il.com>, Rob Herring <robh+dt@...nel.org>, Krzysztof
 Kozlowski <krzysztof.kozlowski+dt@...aro.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>
Subject: RE: [EXT] Re: [PATCH v3 4/5] firmware: imx: add driver for NXP
 EdgeLock Enclave



> -----Original Message-----
> From: Pankaj Gupta
> Sent: Monday, July 15, 2024 2:49 PM
> To: Sascha Hauer <s.hauer@...gutronix.de>
> Cc: Jonathan Corbet <corbet@....net>; Rob Herring <robh@...nel.org>;
> Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley
> <conor+dt@...nel.org>; Shawn Guo <shawnguo@...nel.org>; Pengutronix
> Kernel Team <kernel@...gutronix.de>; Fabio Estevam
> <festevam@...il.com>; Rob Herring <robh+dt@...nel.org>; Krzysztof
> Kozlowski <krzysztof.kozlowski+dt@...aro.org>; devicetree@...r.kernel.org;
> imx@...ts.linux.dev; linux-kernel@...r.kernel.org; linux-arm-
> kernel@...ts.infradead.org; linux-doc@...r.kernel.org
> Subject: RE: [EXT] Re: [PATCH v3 4/5] firmware: imx: add driver for NXP
> EdgeLock Enclave
> 
> 
> 
> > -----Original Message-----
> > From: Sascha Hauer <s.hauer@...gutronix.de>
> > Sent: Monday, July 1, 2024 2:17 PM
> > To: Pankaj Gupta <pankaj.gupta@....com>
> > Cc: Jonathan Corbet <corbet@....net>; Rob Herring <robh@...nel.org>;
> > Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor Dooley
> > <conor+dt@...nel.org>; Shawn Guo <shawnguo@...nel.org>; Pengutronix
> > Kernel Team <kernel@...gutronix.de>; Fabio Estevam
> > <festevam@...il.com>; Rob Herring <robh+dt@...nel.org>; Krzysztof
> > Kozlowski <krzysztof.kozlowski+dt@...aro.org>;
> > devicetree@...r.kernel.org; imx@...ts.linux.dev;
> > linux-kernel@...r.kernel.org; linux-arm- kernel@...ts.infradead.org;
> > linux-doc@...r.kernel.org
> > 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@...gutronix.de>
> > > > Sent: Tuesday, June 18, 2024 2:02 PM
> > > > To: Pankaj Gupta <pankaj.gupta@....com>
> > > > Cc: Jonathan Corbet <corbet@....net>; Rob Herring
> > > > <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>; Conor
> > > > Dooley <conor+dt@...nel.org>; Shawn Guo <shawnguo@...nel.org>;
> > > > Pengutronix Kernel Team <kernel@...gutronix.de>; Fabio Estevam
> > > > <festevam@...il.com>; Rob Herring <robh+dt@...nel.org>; Krzysztof
> > > > Kozlowski <krzysztof.kozlowski+dt@...aro.org>;
> > > > devicetree@...r.kernel.org; imx@...ts.linux.dev;
> > > > linux-kernel@...r.kernel.org; linux-arm-
> > > > kernel@...ts.infradead.org; linux-doc@...r.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@....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.
> 
> 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 |

Powered by blists - more mailing lists