by Robert Marko

[permalink] [raw]

Subject: Re: [PATCH 01/13] firmware: qcom: scm: Introduce pas_metadata context

On 28. 01. 2022. 03:55, Bjorn Andersson wrote:
> Starting with Qualcomm SM8450, some new security enhancements has been
> done in the secure world, which results in the requirement to keep the
> metadata segment accessible by the secure world from init_image() until
> auth_and_reset().
>
> Introduce a "PAS metadata context" object that can be passed to
> init_image() for tracking the mapped memory and a related release
> function for client drivers to release the mapping once either
> auth_and_reset() has been invoked or in error handling paths on the way
> there.
>
> Signed-off-by: Bjorn Andersson <[email protected]>

Hi Bjorn,
I know a lot of time has passed since this patch, but this patch breaks
qcom_scm_pas_auth_and_reset SCM call.
I have had MDT loader and SCM changes from this series reverted for a
long time but was unable to
find the exact culprit nor how to fix it.
I even ported the TZ log driver to see if anything is obvious there but
nope, calling qcom_scm_pas_auth_and_reset
after this patch is just failling which is then causing Q6 WCSS on
IPQ8074 to fail probing so WLAN doesnt work.

Since I am out of ideas, I was hoping that you maybe have an idea what
could be the issue, I can provide the TZ log
with all of the SCM calls when it works and when not if required.

Regards,
Robert

> ---
> drivers/firmware/qcom_scm.c | 39 ++++++++++++++++++++++++++++++-----
> drivers/soc/qcom/mdt_loader.c | 2 +-
> include/linux/qcom_scm.h | 10 ++++++++-
> 3 files changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
> index 7db8066b19fd..3218d13cbf83 100644
> --- a/drivers/firmware/qcom_scm.c
> +++ b/drivers/firmware/qcom_scm.c
> @@ -435,10 +435,16 @@ static void qcom_scm_set_download_mode(bool enable)
> * 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
> *
> - * Returns 0 on success.
> + * Return: 0 on success.
> + *
> + * Upon successful return, the PAS metadata context (@ctx) will be used to
> + * track the metadata allocation, this needs to be released by invoking
> + * qcom_scm_pas_metadata_release() by the caller.
> */
> -int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
> +int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size,
> + struct qcom_scm_pas_metadata *ctx)
> {
> dma_addr_t mdata_phys;
> void *mdata_buf;
> @@ -467,7 +473,7 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
>
> ret = qcom_scm_clk_enable();
> if (ret)
> - goto free_metadata;
> + goto out;
>
> desc.args[1] = mdata_phys;
>
> @@ -475,13 +481,36 @@ int qcom_scm_pas_init_image(u32 peripheral, const void *metadata, size_t size)
>
> qcom_scm_clk_disable();
>
> -free_metadata:
> - dma_free_coherent(__scm->dev, size, mdata_buf, mdata_phys);
> +out:
> + 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;
> + }
>
> return ret ? : res.result[0];
> }
> EXPORT_SYMBOL(qcom_scm_pas_init_image);
>
> +/**
> + * qcom_scm_pas_metadata_release() - release metadata context
> + * @ctx: metadata context
> + */
> +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx)
> +{
> + if (!ctx->ptr)
> + return;
> +
> + dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
> +
> + ctx->ptr = NULL;
> + ctx->phys = 0;
> + ctx->size = 0;
> +}
> +EXPORT_SYMBOL(qcom_scm_pas_metadata_release);
> +
> /**
> * qcom_scm_pas_mem_setup() - Prepare the memory related to a given peripheral
> * for firmware loading
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index 72fc2b539213..b00586db5391 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -171,7 +171,7 @@ static int __qcom_mdt_load(struct device *dev, const struct firmware *fw,
> goto out;
> }
>
> - ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len);
> + ret = qcom_scm_pas_init_image(pas_id, metadata, metadata_len, NULL);
>
> kfree(metadata);
> if (ret) {
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index 81cad9e1e412..4d8371410b05 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -68,8 +68,16 @@ extern int qcom_scm_set_warm_boot_addr(void *entry, const cpumask_t *cpus);
> extern void qcom_scm_cpu_power_down(u32 flags);
> extern int qcom_scm_set_remote_state(u32 state, u32 id);
>
> +struct qcom_scm_pas_metadata {
> + void *ptr;
> + dma_addr_t phys;
> + ssize_t size;
> +};
> +
> extern int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
> - size_t size);
> + size_t size,
> + struct qcom_scm_pas_metadata *ctx);
> +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_metadata *ctx);
> extern int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
> phys_addr_t size);
> extern int qcom_scm_pas_auth_and_reset(u32 peripheral);