Re: [PATCH v7 02/12] firmware: qcom: scm: enable the TZ mem allocator

[Bjorn Andersson]

On Mon, Feb 05, 2024 at 07:28:00PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> 
> Select the TrustZone memory allocator in Kconfig and create a pool of
> memory shareable with the TrustZone when probing the SCM driver.
> 
> This will allow a gradual conversion of all relevant SCM calls to using
> the dedicated allocator.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> Reviewed-by: Andrew Halaney <ahalaney@xxxxxxxxxx>
> Tested-by: Andrew Halaney <ahalaney@xxxxxxxxxx> # sc8280xp-lenovo-thinkpad-x13s
> Tested-by: Deepti Jaggi <quic_djaggi@xxxxxxxxxxx> #sa8775p-ride
> Reviewed-by: Elliot Berman <quic_eberman@xxxxxxxxxxx>
> ---
>  drivers/firmware/qcom/Kconfig    |  1 +
>  drivers/firmware/qcom/qcom_scm.c | 16 ++++++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/firmware/qcom/Kconfig b/drivers/firmware/qcom/Kconfig
> index f18686edf415..d24d83223867 100644
> --- a/drivers/firmware/qcom/Kconfig
> +++ b/drivers/firmware/qcom/Kconfig
> @@ -7,6 +7,7 @@
>  menu "Qualcomm firmware drivers"
>  
>  config QCOM_SCM
> +	select QCOM_TZMEM
>  	tristate
>  
>  config QCOM_TZMEM
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 520de9b5633a..0d4c028be0c1 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -8,8 +8,10 @@
>  #include <linux/completion.h>
>  #include <linux/cpumask.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/err.h>
>  #include <linux/export.h>
>  #include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/firmware/qcom/qcom_tzmem.h>
>  #include <linux/init.h>
>  #include <linux/interconnect.h>
>  #include <linux/interrupt.h>
> @@ -20,9 +22,11 @@
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/reset-controller.h>
> +#include <linux/sizes.h>
>  #include <linux/types.h>
>  
>  #include "qcom_scm.h"
> +#include "qcom_tzmem.h"
>  
>  static bool download_mode = IS_ENABLED(CONFIG_QCOM_SCM_DOWNLOAD_MODE_DEFAULT);
>  module_param(download_mode, bool, 0);
> @@ -41,6 +45,8 @@ struct qcom_scm {
>  	int scm_vote_count;
>  
>  	u64 dload_mode_addr;
> +
> +	struct qcom_tzmem_pool *mempool;
>  };
>  
>  struct qcom_scm_current_perm_info {
> @@ -1887,6 +1893,16 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  	if (of_property_read_bool(pdev->dev.of_node, "qcom,sdi-enabled"))
>  		qcom_scm_disable_sdi();
>  
> +	ret = qcom_tzmem_enable(__scm->dev);
> +	if (ret)
> +		return dev_err_probe(__scm->dev, ret,
> +				     "Failed to enable the TrustZone memory allocator\n");
> +
> +	__scm->mempool = devm_qcom_tzmem_pool_new(__scm->dev, SZ_256K);

As we're not moving from the callers freely allocating what they need,
to a fixed sized pool of 256kb. Please document why 256kb was choosen,
so that we have something to fall back on when someone runs out of this
space, or wonders "why not 128kb?".

Regards,
Bjorn

> +	if (IS_ERR(__scm->mempool))
> +		return dev_err_probe(__scm->dev, PTR_ERR(__scm->mempool),
> +				     "Failed to create the SCM memory pool\n");
> +
>  	/*
>  	 * Initialize the QSEECOM interface.
>  	 *
> -- 
> 2.40.1
>