-----Original Message-----
From: Sascha Hauer <s.hauer@pengutronix.de>
Sent: Monday, July 1, 2024 2:17 PM
To: Pankaj Gupta <pankaj.gupta@nxp.com>
Cc: Jonathan Corbet <corbet@lwn.net>; Rob Herring <robh@kernel.org>;
Krzysztof Kozlowski [off-list ref]; Conor Dooley
[off-list ref]; Shawn Guo [off-list ref]; Pengutronix
Kernel Team [off-list ref]; Fabio Estevam
[off-list ref]; Rob Herring [off-list ref]; Krzysztof
Kozlowski [off-list ref]; devicetree@vger.kernel.org;
imx@lists.linux.dev; linux-kernel@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; linux-doc@vger.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:

quoted

quoted

-----Original Message-----
From: Sascha Hauer <s.hauer@pengutronix.de>
Sent: Tuesday, June 18, 2024 2:02 PM
To: Pankaj Gupta <pankaj.gupta@nxp.com>
Cc: Jonathan Corbet <corbet@lwn.net>; Rob Herring <robh@kernel.org>;
Krzysztof Kozlowski [off-list ref]; Conor Dooley
[off-list ref]; Shawn Guo [off-list ref]; Pengutronix
Kernel Team [off-list ref]; Fabio Estevam
[off-list ref]; Rob Herring [off-list ref]; Krzysztof
Kozlowski [off-list ref];
devicetree@vger.kernel.org; imx@lists.linux.dev;
linux-kernel@vger.kernel.org; linux-arm- kernel@lists.infradead.org;
linux-doc@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:

quoted

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.

quoted

quoted

quoted

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@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

++++++++++++++++++++++++++++++++++++

quoted

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

quoted

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

quoted

+          - EdgeLock Enclave Firmware (for i.MX8ULP, i.MX93),
+          like base, HSM, V2X & SHE using the SAB protocol via
+ the shared

Messaging

quoted

+          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

quoted

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

quoted

- These other Linux layers have to choose from multiple similar devices per

secure-enclave.

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

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

quoted

It will help review the 'if' condition based on these structure member

variable.

quoted

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 |