Message-ID: <de4b4872-061c-4f03-ae1d-1ad93b35ed71@linaro.org>
Date: Thu, 21 Aug 2025 16:23:53 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Mukesh Ojha <mukesh.ojha@....qualcomm.com>,
 Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>,
 Vikash Garodia <quic_vgarodia@...cinc.com>,
 Dikshita Agarwal <quic_dikshita@...cinc.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>,
 Mathieu Poirier <mathieu.poirier@...aro.org>
Cc: Abhinav Kumar <abhinav.kumar@...ux.dev>, linux-kernel@...r.kernel.org,
 linux-arm-msm@...r.kernel.org, linux-media@...r.kernel.org,
 linux-remoteproc@...r.kernel.org
Subject: Re: [PATCH v2 05/11] firmware: qcom_scm: Add shmbridge support to
 pas_init/release function

On 19/08/2025 17:54, Mukesh Ojha wrote:
> Qualcomm SoCs running with QHEE (Qualcomm Hypervisor Execution
> Environment—a library present in the Gunyah hypervisor) utilize the
> Peripheral Authentication Service (PAS) from Qualcomm TrustZone (TZ)
> also called QTEE(Qualcomm Trusted Execution Environment firmware)
> to securely authenticate and reset remote processors via a sequence
> of SMC calls such as qcom_scm_pas_init_image(), qcom_scm_pas_mem_setup(),
> and qcom_scm_pas_auth_and_reset().
> 
> For memory passed to Qualcomm TrustZone, it must either be part of a
> pool registered with TZ or be directly registered via SHMbridge SMC
> calls.
> 
> When QHEE is present, PAS SMC calls from Linux running at EL1 are
> trapped by QHEE (running at EL2), which then creates or retrieves memory
> from the SHM bridge for both metadata and remoteproc carveout memory
> before passing them to TZ. However, when the SoC runs with a
> non-QHEE-based hypervisor, Linux must create the SHM bridge for both
> metadata (before it is passed to TZ in qcom_scm_pas_init_image()) and
> for remoteproc memory (before the call is made to TZ in
> qcom_scm_pas_auth_and_reset()).
> 
> For the qcom_scm_pas_init_image() call, metadata content must be copied
> to a buffer allocated from the SHM bridge before making the SMC call.
> This buffer should be freed either immediately after the call or during
> the qcom_scm_pas_metadata_release() function, depending on the context
> parameter passed to qcom_scm_pas_init_image(). Convert the metadata
> context parameter to use PAS context data structure so that it will also
> be possible to decide whether to get memory from SHMbridge pool or not.
> 
> When QHEE is present, it manages the IOMMU translation context so, in
> absence of it device driver will be aware through device tree that its
> translation context is managed by Linux and it need to create SHMbridge
> before passing any buffer to TZ, So, remote processor driver should
> appropriately set ctx->has_iommu to let PAS SMC function to take care of
> everything ready for the call to work.
> 
> Lets convert qcom_scm_pas_init_image() and qcom_scm_pas_metadata_release()
> to have these awareness.

I like the effort in the commit log here but its also a bit too long.

Please go through these paragraphs and try to reduce down the amount of 
text you are generating.

> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
> ---
>   drivers/firmware/qcom/qcom_scm.c       | 71 +++++++++++++++++++++-----
>   drivers/remoteproc/qcom_q6v5_pas.c     | 14 ++---
>   drivers/soc/qcom/mdt_loader.c          |  4 +-
>   include/linux/firmware/qcom/qcom_scm.h |  9 ++--
>   4 files changed, 73 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 7827699e277c..301d440f62f3 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
> @@ -616,6 +616,35 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
>   	return ret;
>   }
>   
> +static int qcom_scm_pas_prep_and_init_image(struct qcom_scm_pas_ctx *ctx,
> +					    const void *metadata, size_t size)
> +{
> +	struct qcom_scm_pas_metadata *mdt_ctx;
> +	struct qcom_scm_res res;
> +	phys_addr_t mdata_phys;
> +	void *mdata_buf;
> +	int ret;
> +
> +	mdt_ctx = ctx->metadata;
> +	mdata_buf = qcom_tzmem_alloc(__scm->mempool, size, GFP_KERNEL);
> +	if (!mdata_buf)
> +		return -ENOMEM;
> +
> +	memcpy(mdata_buf, metadata, size);
> +	mdata_phys = qcom_tzmem_to_phys(mdata_buf);
> +
> +	ret = __qcom_scm_pas_init_image(ctx->peripheral, mdata_phys, mdata_buf, size, &res);
> +	if (ret < 0 || !mdt_ctx) {

if ret is an error or mdt_ctx is null free the memory

> +		qcom_tzmem_free(mdata_buf);
> +	} else if (mdt_ctx) {

if mdt_ctx is valid do this

> +		mdt_ctx->ptr = mdata_buf;
> +		mdt_ctx->addr.phys_addr = mdata_phys;
> +		mdt_ctx->size = size;
> +	}
> +
> +	return ret ? : res.result[0];

so we can have ctx_mtd valid but return the value at ret but also mtd 
valid and return the res.result[0]

That seems like an odd choice - surely if you are enumerating the 
data-structure the result code we care about is res.result[0] instead of 
ret ?

OK I see this return logic comes from below..

But

drivers/soc/qcom/mdt_loader.c::qcom_mdt_pas_init

ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, ctx);
kfree(metadata);
if (ret) {
     /* Invalid firmware metadata */
     dev_err(dev, "error %d initializing firmware %s\n", ret, fw_name);
     goto out;
}

So if ret as returned from your function is > 0 you will leak the memory 
allocated @ mdata_buf ..

Do you expect something else to come along and call 
qcom_scm_pas_metadata_release() ?

> +}
> +
>   /**
>    * qcom_scm_pas_init_image() - Initialize peripheral authentication service
>    *			       state machine for a given peripheral, using the
> @@ -625,7 +654,7 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
>    *		and optional blob of data used for authenticating the metadata
>    *		and the rest of the firmware
>    * @size:	size of the metadata
> - * @ctx:	optional metadata context
> + * @ctx:	optional pas context
>    *
>    * Return: 0 on success.
>    *
> @@ -634,13 +663,19 @@ static int __qcom_scm_pas_init_image(u32 peripheral, dma_addr_t mdata_phys,
>    * qcom_scm_pas_metadata_release() by the caller.
>    */
>   int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> -			    struct qcom_scm_pas_metadata *ctx)
> +			    struct qcom_scm_pas_ctx *ctx)
>   {
> +	struct qcom_scm_pas_metadata *mdt_ctx;
>   	struct qcom_scm_res res;
>   	dma_addr_t mdata_phys;
>   	void *mdata_buf;
>   	int ret;
>   
> +	if (ctx && ctx->has_iommu) {
> +		ret = qcom_scm_pas_prep_and_init_image(ctx, metadata, size);
> +		return ret;
> +	}
> +
>   	/*
>   	 * During the scm call memory protection will be enabled for the meta
>   	 * data blob, so make sure it's physically contiguous, 4K aligned and
> @@ -663,10 +698,11 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
>   	ret = __qcom_scm_pas_init_image(peripheral, mdata_phys, mdata_buf, size, &res);
>   	if (ret < 0 || !ctx) {
>   		dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
> -	} else if (ctx) {
> -		ctx->ptr = mdata_buf;
> -		ctx->phys = mdata_phys;
> -		ctx->size = size;
> +	} else if (ctx->metadata) {
> +		mdt_ctx = ctx->metadata;
> +		mdt_ctx->ptr = mdata_buf;
> +		mdt_ctx->addr.dma_addr = mdata_phys;
> +		mdt_ctx->size = size;
>   	}
>   
>   	return ret ? : res.result[0];

is this return path still valid now that you've functionally decomposed 
into qcom_sm_pas_prep_and_init ?

> @@ -675,18 +711,27 @@ EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
>   
>   /**
>    * qcom_scm_pas_metadata_release() - release metadata context
> - * @ctx:	metadata context
> + * @ctx:	pas context
>    */
> -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
> +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx)
>   {
> -	if (!ctx->ptr)
> +	struct qcom_scm_pas_metadata *mdt_ctx;
> +
> +	mdt_ctx = ctx->metadata;
> +	if (!mdt_ctx->ptr)
>   		return;
>   
> -	dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
> +	if (ctx->has_iommu) {
> +		qcom_tzmem_free(mdt_ctx->ptr);
> +		mdt_ctx->addr.phys_addr = 0;
> +	} else {
> +		dma_free_coherent(__scm->dev, mdt_ctx->size, mdt_ctx->ptr,
> +				  mdt_ctx->addr.dma_addr);
> +		mdt_ctx->addr.dma_addr = 0;
> +	}
>   
> -	ctx->ptr = NULL;
> -	ctx->phys = 0;
> -	ctx->size = 0;
> +	mdt_ctx->ptr = NULL;
> +	mdt_ctx->size = 0;
>   }
>   EXPORT_SYMBOL_GPL(qcom_scm_pas_metadata_release);
>   
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index e376c0338576..09cada92dfd5 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -209,9 +209,9 @@ static int qcom_pas_unprepare(struct rproc *rproc)
>   	 * auth_and_reset() was successful, but in other cases clean it up
>   	 * here.
>   	 */
> -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> +	qcom_scm_pas_metadata_release(pas->pas_ctx);
>   	if (pas->dtb_pas_id)
> -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>   
>   	return 0;
>   }
> @@ -244,7 +244,7 @@ static int qcom_pas_load(struct rproc *rproc, const struct firmware *fw)
>   	return 0;
>   
>   release_dtb_metadata:
> -	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> +	qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>   	release_firmware(pas->dtb_firmware);
>   
>   	return ret;
> @@ -313,9 +313,9 @@ static int qcom_pas_start(struct rproc *rproc)
>   		goto release_pas_metadata;
>   	}
>   
> -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> +	qcom_scm_pas_metadata_release(pas->pas_ctx);
>   	if (pas->dtb_pas_id)
> -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>   
>   	/* firmware is used to pass reference from qcom_pas_start(), drop it now */
>   	pas->firmware = NULL;
> @@ -323,9 +323,9 @@ static int qcom_pas_start(struct rproc *rproc)
>   	return 0;
>   
>   release_pas_metadata:
> -	qcom_scm_pas_metadata_release(pas->pas_ctx->metadata);
> +	qcom_scm_pas_metadata_release(pas->pas_ctx);
>   	if (pas->dtb_pas_id)
> -		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx->metadata);
> +		qcom_scm_pas_metadata_release(pas->dtb_pas_ctx);
>   disable_px_supply:
>   	if (pas->px_supply)
>   		regulator_disable(pas->px_supply);
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 509ff85d9bf6..a1718db91b3e 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -240,7 +240,7 @@ EXPORT_SYMBOL_GPL(qcom_mdt_read_metadata);
>    */
>   static int __qcom_mdt_pas_init(struct device *dev, const struct firmware *fw,
>   			       const char *fw_name, int pas_id, phys_addr_t mem_phys,
> -			       struct qcom_scm_pas_metadata *ctx)
> +			       struct qcom_scm_pas_ctx *ctx)
>   {
>   	const struct elf32_phdr *phdrs;
>   	const struct elf32_phdr *phdr;
> @@ -491,7 +491,7 @@ int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
>   	int ret;
>   
>   	ret = __qcom_mdt_pas_init(ctx->dev, fw, firmware, ctx->peripheral,
> -				  ctx->mem_phys, ctx->metadata);
> +				  ctx->mem_phys, ctx);
>   	if (ret)
>   		return ret;
>   
> diff --git a/include/linux/firmware/qcom/qcom_scm.h b/include/linux/firmware/qcom/qcom_scm.h
> index a31006fe49a9..bd3417d9c3f9 100644
> --- a/include/linux/firmware/qcom/qcom_scm.h
> +++ b/include/linux/firmware/qcom/qcom_scm.h
> @@ -68,7 +68,10 @@ int qcom_scm_set_remote_state(u32 state, u32 id);
>   
>   struct qcom_scm_pas_metadata {
>   	void *ptr;
> -	dma_addr_t phys;
> +	union {
> +		dma_addr_t dma_addr;
> +		phys_addr_t phys_addr;
> +	} addr;
>   	ssize_t size;
>   };
>   
> @@ -85,8 +88,8 @@ struct qcom_scm_pas_ctx {
>   void *qcom_scm_pas_ctx_init(struct device *dev, u32 peripheral, phys_addr_t mem_phys,
>   			    size_t mem_size, bool save_mdt_ctx);
>   int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> -			    struct qcom_scm_pas_metadata *ctx);
> -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
> +			    struct qcom_scm_pas_ctx *ctx);
> +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_ctx *ctx);
>   int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr, phys_addr_t size);
>   int qcom_scm_pas_prepare_and_auth_reset(struct qcom_scm_pas_ctx *ctx);
>   int qcom_scm_pas_auth_and_reset(u32 peripheral);

Please review the error paths here especially WRT to qcom_mdt_pas_init();

---
bod

Powered by blists - more mailing lists