[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <35f28f0b-1014-4058-a073-96464a278a0a@marvell.com>
Date: Mon, 22 Jul 2024 17:07:31 +0530
From: Amit Singh Tomar <amitsinght@...vell.com>
To: Pankaj Gupta <pankaj.gupta@....com>, 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>,
Sascha Hauer <s.hauer@...gutronix.de>,
Pengutronix Kernel Team <kernel@...gutronix.de>,
Fabio Estevam <festevam@...il.com>, Rob Herring <robh+dt@...nel.org>
Cc: linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, imx@...ts.linux.dev,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [EXTERNAL] [PATCH v6 5/5] firmware: imx: adds miscdev
Hi Pankaj,
>
> Adds the driver for communication interface to secure-enclave,
> for exchanging messages with NXP secure enclave HW IP(s) like
> EdgeLock Enclave from:
> - User-Space Applications via character driver.
>
> ABI documentation for the NXP secure-enclave driver.
>
> User-space library using this driver:
> - i.MX Secure Enclave library:
> -- URL:https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsecure-2Denclave.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=C67hc24yMATzUglvGvywzpn0Efjurb6sOLm2V_9VpsI&e= <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsecure-2Denclave.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=C67hc24yMATzUglvGvywzpn0Efjurb6sOLm2V_9VpsI&e=>,
> - i.MX Secure Middle-Ware:
> -- URL:https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsmw.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=NACAFfnEzGKFI7FlqdL4kxlt8PtxeXRorc3IWanqgtY&e= <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsmw.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=NACAFfnEzGKFI7FlqdL4kxlt8PtxeXRorc3IWanqgtY&e=>
>
> Signed-off-by: Pankaj Gupta <pankaj.gupta@....com>
> ---
> Documentation/ABI/testing/se-cdev | 43 +++
> drivers/firmware/imx/ele_common.c | 192 ++++++++++-
> drivers/firmware/imx/ele_common.h | 4 +
> drivers/firmware/imx/se_ctrl.c | 677 ++++++++++++++++++++++++++++++++++++++
> drivers/firmware/imx/se_ctrl.h | 46 +++
> include/uapi/linux/se_ioctl.h | 94 ++++++
> 6 files changed, 1053 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/ABI/testing/se-cdev b/Documentation/ABI/testing/se-cdev
> new file mode 100644
> index 000000000000..3451c909ccc4
> --- /dev/null
> +++ b/Documentation/ABI/testing/se-cdev
> @@ -0,0 +1,43 @@
> +What: /dev/<se>_mu[0-9]+_ch[0-9]+
> +Date: May 2024
> +KernelVersion: 6.8
> +Contact: linux-imx@....com, pankaj.gupta@....com
> +Description:
> + NXP offers multiple hardware IP(s) for secure enclaves like EdgeLock-
> + Enclave(ELE), SECO. The character device file descriptors
> + /dev/<se>_mu*_ch* are the interface between userspace NXP's secure-
> + enclave shared library and the kernel driver.
> +
> + The ioctl(2)-based ABI is defined and documented in
> + [include]<linux/firmware/imx/ele_mu_ioctl.h>.
> + ioctl(s) are used primarily for:
> + - shared memory management
> + - allocation of I/O buffers
> + - getting mu info
> + - setting a dev-ctx as receiver to receive all the commands from FW
> + - getting SoC info
> + - send command and receive command response
> +
> + The following file operations are supported:
> +
> + open(2)
> + Currently the only useful flags are O_RDWR.
> +
> + read(2)
> + Every read() from the opened character device context is waiting on
> + wait_event_interruptible, that gets set by the registered mailbox callback
> + function, indicating a message received from the firmware on message-
> + unit.
> +
> + write(2)
> + Every write() to the opened character device context needs to acquire
> + mailbox_lock before sending message on to the message unit.
> +
> + close(2)
> + Stops and frees up the I/O contexts that were associated
> + with the file descriptor.
> +
> +Users: https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsecure-2Denclave.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=C67hc24yMATzUglvGvywzpn0Efjurb6sOLm2V_9VpsI&e= <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsecure-2Denclave.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=C67hc24yMATzUglvGvywzpn0Efjurb6sOLm2V_9VpsI&e=>,
> + https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsmw.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=NACAFfnEzGKFI7FlqdL4kxlt8PtxeXRorc3IWanqgtY&e= <https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_nxp-2Dimx_imx-2Dsmw.git&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=V_GK7jRuCHDErm6txmgDK1-MbUihtnSQ3gPgB-A-JKU&m=hqz6ztDhob0jujItfaaf7PHhtqSHj4aoWie1-b4nAGXTUrSyBQtV9W-WqLaM_utz&s=NACAFfnEzGKFI7FlqdL4kxlt8PtxeXRorc3IWanqgtY&e=>
> + crypto/skcipher,
> + drivers/nvmem/imx-ocotp-ele.c
> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index 3a6584d6f6f2..8167ae201b83 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
> @@ -78,6 +78,149 @@ int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, void *rx_msg)
> return err;
> }
>
> +int ele_miscdev_msg_rcv(struct se_if_device_ctx *dev_ctx,
> + void *rx_buf,
> + int rx_buf_sz)
> +{
> + struct se_msg_hdr *header;
> + int err;
> +
> + err = wait_event_interruptible(dev_ctx->wq, dev_ctx->pending_hdr != 0);
> + if (err) {
> + dev_err(dev_ctx->dev,
> + "%s: Err[0x%x]:Interrupted by signal.\n",
> + dev_ctx->miscdev.name, err);
> + goto exit;
> + }
> +
> + header = (struct se_msg_hdr *) dev_ctx->temp_resp;
> +
> + if (header->tag == dev_ctx->priv->rsp_tag) {
> + if (dev_ctx->priv->waiting_rsp_dev && dev_ctx->priv->waiting_rsp_dev != dev_ctx) {
> + dev_warn(dev_ctx->dev,
> + "Dev-ctx waiting for response mismatch (%s != %s).\n",
> + dev_ctx->miscdev.name, dev_ctx->priv->waiting_rsp_dev->miscdev.name);
> + err = -EPERM;
> + goto exit;
> + }
> + }
> +
> + dev_dbg(dev_ctx->dev,
> + "%s: %s %s\n",
> + dev_ctx->miscdev.name,
> + __func__,
> + "message received, start transmit to user");
> +
> + /*
> + * Check that the size passed as argument is larger than
> + * the one carried in the message.
> + *
> + * In case of US-command/response, the dev_ctx->temp_resp_size
> + * is set before sending the command.
> + *
> + * In case of NVM Slave-command/response, the dev_ctx->temp_resp_size
> + * is set after receing the message from mailbox.
> + */
> + if (dev_ctx->temp_resp_size > rx_buf_sz) {
> + dev_err(dev_ctx->dev,
> + "%s: User buffer too small (%d < %d)\n",
> + dev_ctx->miscdev.name,
> + rx_buf_sz, dev_ctx->temp_resp_size);
> + dev_ctx->temp_resp_size = rx_buf_sz;
> + }
> +
> + /* We may need to copy the output data to user before
> + * delivering the completion message.
> + */
> + err = se_dev_ctx_cpy_out_data(dev_ctx, true);
> + if (err < 0)
> + goto exit;
> +
> + /* Copy data from the buffer */
> + print_hex_dump_debug("to user ", DUMP_PREFIX_OFFSET, 4, 4,
> + dev_ctx->temp_resp, dev_ctx->temp_resp_size, false);
> + if (copy_to_user(rx_buf, dev_ctx->temp_resp, dev_ctx->temp_resp_size)) {
> + dev_err(dev_ctx->dev,
> + "%s: Failed to copy to user\n",
> + dev_ctx->miscdev.name);
> + err = -EFAULT;
> + goto exit;
> + }
> +
> + err = dev_ctx->temp_resp_size;
> +exit:
> + if (err < 0)
> + se_dev_ctx_cpy_out_data(dev_ctx, false);
> +
> + /* free memory allocated on the shared buffers. */
> + dev_ctx->secure_mem.pos = 0;
> + dev_ctx->non_secure_mem.pos = 0;
> +
> + dev_ctx->pending_hdr = 0;
> + se_dev_ctx_shared_mem_cleanup(dev_ctx);
> +
> + return err;
> +}
> +
> +int ele_miscdev_msg_send(struct se_if_device_ctx *dev_ctx,
> + void *tx_msg, int tx_msg_sz)
> +{
> + struct se_if_priv *priv = dev_ctx->priv;
> + struct se_msg_hdr *header;
> + u32 size_to_send;
> + int err;
> +
> + header = (struct se_msg_hdr *) tx_msg;
> +
> + /*
> + * Check that the size passed as argument matches the size
> + * carried in the message.
> + */
> + size_to_send = header->size << 2;
> +
> + if (size_to_send != tx_msg_sz) {
> + err = -EINVAL;
> + dev_err(priv->dev,
> + "%s: User buf hdr(0x%x) sz mismatced with input-sz (%d != %d).\n",
> + dev_ctx->miscdev.name, *(u32 *)header, size_to_send, tx_msg_sz);
> + goto exit;
> + }
> +
> + /* Check the message is valid according to tags */
> + if (header->tag == priv->rsp_tag) {
> + /* Check the device context can send the command */
> + if (dev_ctx != priv->cmd_receiver_dev) {
> + dev_err(priv->dev,
> + "%s: Channel not configured to send resp to FW.",
> + dev_ctx->miscdev.name);
> + err = -EPERM;
> + goto exit;
> + }
> + } else if (header->tag == priv->cmd_tag) {
> + if (priv->waiting_rsp_dev != dev_ctx) {
> + dev_err(priv->dev,
> + "%s: Channel not configured to send cmd to FW.",
> + dev_ctx->miscdev.name);
> + err = -EPERM;
> + goto exit;
> + }
> + lockdep_assert_held(&priv->se_if_cmd_lock);
> + } else {
> + dev_err(priv->dev,
> + "%s: The message does not have a valid TAG\n",
> + dev_ctx->miscdev.name);
> + err = -EINVAL;
> + goto exit;
> + }
> + err = ele_msg_send(priv, tx_msg);
> + if (err < 0)
> + goto exit;
> +
> + err = size_to_send;
> +exit:
> + return err;
> +}
> +
> static bool exception_for_size(struct se_if_priv *priv,
> struct se_msg_hdr *header)
> {
> @@ -99,6 +242,7 @@ static bool exception_for_size(struct se_if_priv *priv,
> void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
> {
> struct device *dev = mbox_cl->dev;
> + struct se_if_device_ctx *dev_ctx;
> struct se_if_priv *priv;
> struct se_msg_hdr *header;
> u32 rx_msg_sz;
> @@ -114,8 +258,50 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
> header = msg;
> rx_msg_sz = header->size << 2;
>
> - if (header->tag == priv->rsp_tag) {
> - if (!priv->waiting_rsp_dev) {
> + /* Incoming command: wake up the receiver if any. */
> + if (header->tag == priv->cmd_tag) {
> + dev_dbg(dev, "Selecting cmd receiver\n");
> + dev_ctx = priv->cmd_receiver_dev;
> + /* Pre-allocated buffer of MAX_NVM_MSG_LEN
> + * as the NVM command are initiated by FW.
> + * Size is revealed as part of this call function.
> + */
> + if (rx_msg_sz > MAX_NVM_MSG_LEN) {
> + dev_err(dev,
> + "%s: Msg recvd hdr(0x%x) with greater[%d] than allocated buf-sz.\n",
> + dev_ctx->miscdev.name,
> + *(u32 *) header,
> + rx_msg_sz);
> + } else
> + memcpy(dev_ctx->temp_resp, msg, rx_msg_sz);
It is categorically stated (in the Linux kernel coding style guide) that
this rule does not apply if only one branch of a conditional statement
consists of a single statement. In such cases, you should categorically
use braces for both branches of the conditional statement:
if (condition) {
do_this();
do_that();
} else {
otherwise();
}
Also, made a similar comment on the earlier version (v5) as well:
https://patchwork.kernel.org/project/imx/patch/20240712-imx-se-if-v5-4-66a79903a872@nxp.com/
Thanks
-Amit
Powered by blists - more mailing lists