Re: [PATCH v5 09/11] firmware: imx: enclave-fw: add handling for save/restore IMEM region

Krzysztof Kozlowski
2023-08-24T18:37:57Z

On 23/08/2023 09:33, Pankaj Gupta wrote:
> Some IMEM region is lost during kernel power down. Due to this,
> firmware's functionaity cannot work correctly.
>
> Saving encrypted IMEM region in kernel memory during power down,
> and restore IMEM region on resume.
>
> Signed-off-by: Gaurav Jain <gaurav.jain@nxp.com>
> Signed-off-by: Pankaj Gupta <pankaj.gupta@nxp.com>

> @@ -959,6 +962,17 @@ static int se_probe_cleanup(struct platform_device *pdev)
> priv->flags &= (~RESERVED_DMA_POOL);
> }
>
> + /* free the buffer in ele-mu remove, previously allocated
> + * in ele-mu probe to store encrypted IMEM
> + */

Use Linux coding style comments.

> + if (priv->imem.buf) {
> + dmam_free_coherent(&pdev->dev,
> + ELE_IMEM_SIZE,
> + priv->imem.buf,
> + priv->imem.phyaddr);
> + priv->imem.buf = NULL;
> + }
> +
> if (priv->ctxs) {
> for (i = 0; i < priv->max_dev_ctx; i++) {
> if (priv->ctxs[i])
> @@ -1160,6 +1174,19 @@ static int se_fw_probe(struct platform_device *pdev)
> dev_err(dev, "Failed to init ele-trng\n");
> }
>
> + if (info->imem_mgmt) {
> + /* allocate buffer where ELE store encrypted IMEM */
> + priv->imem.buf = dmam_alloc_coherent(dev, ELE_IMEM_SIZE,
> + &priv->imem.phyaddr,
> + GFP_KERNEL);
> + if (!priv->imem.buf) {
> + dev_err(dev,
> + "dmam-alloc-failed: To store encr-IMEM.\n");
> + ret = -ENOMEM;
> + goto exit;
> + }
> + }
> +
> pr_info("i.MX secure-enclave: %s's mu#%d interface to firmware, configured.\n",
> info->se_name,
> priv->ele_mu_id);
> @@ -1196,17 +1223,31 @@ static int se_fw_remove(struct platform_device *pdev)
> #ifdef CONFIG_PM_SLEEP
> static int se_fw_suspend(struct device *dev)
> {
> + struct ele_mu_priv *priv = dev_get_drvdata(dev);
> + const struct of_device_id *of_id = of_match_device(se_fw_match, dev);

No.

> + struct imx_info *info = (of_id != NULL) ? (struct imx_info *)of_id->data
> + : NULL;
> +
> + if (info && info->imem_mgmt)
> + priv->imem.size = save_imem(dev);
> +
> return 0;
> }
>
> static int se_fw_resume(struct device *dev)
> {
> struct ele_mu_priv *priv = dev_get_drvdata(dev);
> + const struct of_device_id *of_id = of_match_device(se_fw_match, dev);

Why do you keep matching device every time? Don't.

> + struct imx_info *info = (of_id != NULL) ? (struct imx_info *)of_id->data
> + : NULL;
> int i;
>
> for (i = 0; i < priv->max_dev_ctx; i++)
> wake_up_interruptible(&priv->ctxs[i]->wq);
>
> + if (info && info->imem_mgmt)
> + restore_imem(dev, info->pool_name);
> +
> return 0;
> }
> #endif
> diff --git a/drivers/firmware/imx/se_fw.h b/drivers/firmware/imx/se_fw.h
> index b3502affbc85..acb967f2357c 100644
> --- a/drivers/firmware/imx/se_fw.h
> +++ b/drivers/firmware/imx/se_fw.h
> @@ -165,4 +165,12 @@ struct ele_mu_priv {
> struct ele_imem_buf imem;
> };
>
> +phys_addr_t get_phy_buf_mem_pool(struct device *dev,
> + char *mem_pool_name,
> + u32 **buf,
> + uint32_t size);
> +void free_phybuf_mem_pool(struct device *dev,
> + char *mem_pool_name,
> + u32 *buf,
> + uint32_t size);
> #endif
> diff --git a/include/linux/firmware/imx/ele_base_msg.h b/include/linux/firmware/imx/ele_base_msg.h
> index 8a5c385210fc..6fbea7a8d7c9 100644
> --- a/include/linux/firmware/imx/ele_base_msg.h
> +++ b/include/linux/firmware/imx/ele_base_msg.h
> @@ -37,12 +37,23 @@
> #define ELE_GET_TRNG_STATE_RETRY_COUNT 0x5
> #define CSAL_TRNG_STATE_MASK 0x0000ffff
>
> +#define ELE_SERVICE_SWAP_REQ 0xDF
> +#define ELE_SERVICE_SWAP_REQ_MSG_SZ 0x03
> +#define ELE_IMEM_SIZE 0x10000
> +#define ELE_IMEM_STATE_OK 0xCA
> +#define ELE_IMEM_STATE_BAD 0xFE
> +#define ELE_IMEM_STATE_WORD 0x27
> +#define ELE_IMEM_STATE_MASK 0x00ff0000
> +#define ELE_IMEM_EXPORT 0x1
> +#define ELE_IMEM_IMPORT 0x2
> +
> #define ELE_BASE_API_VERSION 0x6
> -#define ELE_SUCCESS_IND 0xD6
> -#define ELE_FAILURE_IND 0x29

No, you just added them. Don't add wrong code to remove it in next patch.

Best regards,
Krzysztof