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