linux-kernel - Re: [PATCH v2 09/11] remoteproc: pas: Extend parse

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aKWI-izL5BooL61p@linaro.org>
Date: Wed, 20 Aug 2025 10:36:10 +0200
From: Stephan Gerhold <stephan.gerhold@...aro.org>
To: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
Cc: 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>,
	Abhinav Kumar <abhinav.kumar@...ux.dev>,
	Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
	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 09/11] remoteproc: pas: Extend parse_fw callback to
 parse resource table

On Tue, Aug 19, 2025 at 10:24:44PM +0530, Mukesh Ojha wrote:
> Extend parse_fw callback to include SMC call to get resource
> table from TrustZone to leverage resource table parse and
> mapping and unmapping code reuse from the framework.
> 
> Signed-off-by: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
> ---
>  drivers/remoteproc/qcom_q6v5_pas.c  | 33 +++++++++++++++++++++++++++--
>  drivers/soc/qcom/mdt_loader.c       |  1 -
>  include/linux/soc/qcom/mdt_loader.h |  2 ++
>  3 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c
> index 09cada92dfd5..1e0f09bf1ef2 100644
> --- a/drivers/remoteproc/qcom_q6v5_pas.c
> +++ b/drivers/remoteproc/qcom_q6v5_pas.c
> @@ -408,6 +408,35 @@ static void *qcom_pas_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is
>  	return pas->mem_region + offset;
>  }
>  
> +static int qcom_pas_parse_firmware(struct rproc *rproc, const struct firmware *fw)
> +{
> +	struct qcom_pas *pas = rproc->priv;
> +	size_t output_rt_size = MAX_RSCTABLE_SIZE;
> +	void *output_rt;
> +	int ret;
> +
> +	ret = qcom_register_dump_segments(rproc, fw);
> +	if (ret) {
> +		dev_err(pas->dev, "Error in registering dump segments\n");
> +		return ret;
> +	}
> +
> +	if (!rproc->has_iommu)
> +		return ret;
> +
> +	ret = qcom_scm_pas_get_rsc_table(pas->pas_id, NULL, 0, &output_rt, &output_rt_size);

In PATCH 07/11 you have support for "static" resources that can be part
of the firmware binary, but then you never make use of it. Like in the
iris patch you just give in NULL, 0 for input_rt, even though,
(presumably?) the remoteproc framework has support for parsing the
resource table from the ELF firmware image.

I would suggest adding a comment here justifying this and perhaps
something to the commit message. I do see value in having the
qcom_scm_pas_get_rsc_table() properly defined with input RT support, but
it's not obvious from the description of your patches that this is
effectively dead code right now(?).

> +	if (ret) {
> +		dev_err(pas->dev, "error %d getting resource_table\n", ret);
> +		return ret;
> +	}
> +
> +	rproc->cached_table = output_rt;
> +	rproc->table_ptr = rproc->cached_table;
> +	rproc->table_sz = output_rt_size;
> +
> +	return ret;
> +}
> +
>  static unsigned long qcom_pas_panic(struct rproc *rproc)
>  {
>  	struct qcom_pas *pas = rproc->priv;
> @@ -420,7 +449,7 @@ static const struct rproc_ops qcom_pas_ops = {
>  	.start = qcom_pas_start,
>  	.stop = qcom_pas_stop,
>  	.da_to_va = qcom_pas_da_to_va,
> -	.parse_fw = qcom_register_dump_segments,
> +	.parse_fw = qcom_pas_parse_firmware,
>  	.load = qcom_pas_load,
>  	.panic = qcom_pas_panic,
>  };
> @@ -430,7 +459,7 @@ static const struct rproc_ops qcom_pas_minidump_ops = {
>  	.start = qcom_pas_start,
>  	.stop = qcom_pas_stop,
>  	.da_to_va = qcom_pas_da_to_va,
> -	.parse_fw = qcom_register_dump_segments,
> +	.parse_fw = qcom_pas_parse_firmware,
>  	.load = qcom_pas_load,
>  	.panic = qcom_pas_panic,
>  	.coredump = qcom_pas_minidump,
> diff --git a/drivers/soc/qcom/mdt_loader.c b/drivers/soc/qcom/mdt_loader.c
> index ea7034c4b996..8456cca3f3e0 100644
> --- a/drivers/soc/qcom/mdt_loader.c
> +++ b/drivers/soc/qcom/mdt_loader.c
> @@ -22,7 +22,6 @@
>  #include <linux/slab.h>
>  #include <linux/soc/qcom/mdt_loader.h>
>  
> -#define MAX_RSCTABLE_SIZE	SZ_16K;

I'm confused why there is a semicolon here suddenly. Did you edit this
patch by hand?

Applying: remoteproc: pas: Extend parse_fw callback to parse resource table
Patch failed at 0009 remoteproc: pas: Extend parse_fw callback to parse resource table
error: patch failed: drivers/soc/qcom/mdt_loader.c:22
error: drivers/soc/qcom/mdt_loader.c: patch does not apply

>  #define RSC_TABLE_HASH_BITS	     5  // 32 buckets
>  
>  DEFINE_HASHTABLE(qcom_pas_rsc_table_map, RSC_TABLE_HASH_BITS);
> diff --git a/include/linux/soc/qcom/mdt_loader.h b/include/linux/soc/qcom/mdt_loader.h
> index 62f239f64dfb..92ad862e733e 100644
> --- a/include/linux/soc/qcom/mdt_loader.h
> +++ b/include/linux/soc/qcom/mdt_loader.h
> @@ -8,6 +8,8 @@
>  #define QCOM_MDT_TYPE_HASH	(2 << 24)
>  #define QCOM_MDT_RELOCATABLE	BIT(27)
>  
> +#define MAX_RSCTABLE_SIZE	SZ_16K
> +
>  struct device;
>  struct firmware;
>  struct qcom_scm_pas_ctx;

You added this define yourself in PATCH 08/11, so just add it in the
right place directly. Make sure you scroll through your patch set before
sending to make sure all changes are in the right commit. :-)

Thanks,
Stephan