On Thu, Aug 21, 2025 at 04:23:53PM +0100, Bryan O'Donoghue wrote: > 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.
I was writing to set context for each commit and for the record and hence, the repetition of text you would see in some of the lines used. I will take your suggestion and reduce it. > > > > > Signed-off-by: Mukesh Ojha <[email protected]> > > --- > > 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 Nothing change, it is similar to the earlier code. > > > + 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() ? You just identified a bug in the existing code where qcom_mdt_pas_init() does not call qcom_scm_pas_metadata_release() for firmware image for failure case from qcom_q6v5_pas(). > > > +} > > + > > /** > > * 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(); Sure, will send the fix patch for the existing bug. > > --- > bod -- -Mukesh Ojha
