Received-SPF: pass (google.com: domain of linux-kernel+bounces-70178-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sat, 17 Feb 2024 21:25:12 -0600
From: Bjorn Andersson <andersson@kernel.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Andy Gross <agross@kernel.org>, 
	Konrad Dybcio <konrad.dybcio@linaro.org>, Elliot Berman <quic_eberman@quicinc.com>, 
	Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, Guru Das Srinagesh <quic_gurus@quicinc.com>, 
	Andrew Halaney <ahalaney@redhat.com>, Maximilian Luz <luzmaximilian@gmail.com>, 
	Alex Elder <elder@linaro.org>, Srini Kandagatla <srinivas.kandagatla@linaro.org>, 
	Arnd Bergmann <arnd@arndb.de>, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, 
	linux-arm-kernel@lists.infradead.org, kernel@quicinc.com, 
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>, Deepti Jaggi <quic_djaggi@quicinc.com>
Subject: Re: [PATCH v7 02/12] firmware: qcom: scm: enable the TZ mem allocator
Message-ID: <tnylagkmj36lve2ub5voumtkqjdy6j3hr6yyk4mqkaabvc3gdv@voaiu2oqi73o>
References: <20240205182810.58382-1-brgl@bgdev.pl>
 <20240205182810.58382-3-brgl@bgdev.pl>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240205182810.58382-3-brgl@bgdev.pl>

On Mon, Feb 05, 2024 at 07:28:00PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> 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@linaro.org>
> Reviewed-by: Andrew Halaney <ahalaney@redhat.com>
> Tested-by: Andrew Halaney <ahalaney@redhat.com> # sc8280xp-lenovo-thinkpad-x13s
> Tested-by: Deepti Jaggi <quic_djaggi@quicinc.com> #sa8775p-ride
> Reviewed-by: Elliot Berman <quic_eberman@quicinc.com>
> ---
>  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
>