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

[Krzysztof Kozlowski]

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@xxxxxxx>
> Signed-off-by: Pankaj Gupta <pankaj.gupta@xxxxxxx>


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