linux-kernel - Re: [PATCH v2 02/11] soc: qcom: mdtloader: Add context aware qcom_mdt_pas

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: <20250904095215.ekkhrt5ql65ap74k@hu-mojha-hyd.qualcomm.com>
Date: Thu, 4 Sep 2025 15:22:15 +0530
From: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
To: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
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>, 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 02/11] soc: qcom: mdtloader: Add context aware
 qcom_mdt_pas_load() helper

On Wed, Sep 03, 2025 at 04:03:05PM +0100, Bryan O'Donoghue wrote:
> On 19/08/2025 17:54, Mukesh Ojha wrote:
> > Currently, remoteproc and non-remoteproc subsystems use different
> > variants of the MDT loader helper API, primarily due to the handling of
> > the metadata context. Remoteproc subsystems retain this context until
> > authentication and reset, while non-remoteproc subsystems (e.g., video,
> > graphics) do not require it.
> > 
> > Add context aware qcom_mdt_pas_load() function which uses context
> > returned from qcom_scm_pas_ctx_init() and use it till subsystems
> > is out of set. This will also help in unifying the API used by
> > remoteproc and non-remoteproc subsystems drivers.
> > 
> > Signed-off-by: Mukesh Ojha <mukesh.ojha@....qualcomm.com>
> > ---
> 
> I'm struggling with the logic here a little bit.
> 
> You take this function which calls qcom_mtd_load_no_init();
> 
> > -		ret = qcom_mdt_pas_init(pas->dev, pas->dtb_firmware, pas->dtb_firmware_name,
> > -					pas->dtb_pas_id, pas->dtb_mem_phys,
> > -					&pas->dtb_pas_metadata);
> > -		if (ret)
> > -			goto release_dtb_firmware;
> > -
> > -		ret = qcom_mdt_load_no_init(pas->dev, pas->dtb_firmware, pas->dtb_firmware_name,
> > -					    pas->dtb_mem_region, pas->dtb_mem_phys,
> > -					    pas->dtb_mem_size, &pas->dtb_mem_reloc);
> > +		ret = qcom_mdt_pas_load(pas->dtb_pas_ctx, pas->dtb_firmware, pas->dtb_firmware_name,
> > +					pas->dtb_mem_region, &pas->dtb_mem_reloc);
> 
> and then turn it into
> 
> > +int qcom_mdt_pas_load(struct qcom_scm_pas_ctx *ctx, const struct firmware *fw,
> > +		      const char *firmware, void *mem_region, phys_addr_t *reloc_base)
> > +{
> > +	int ret;
> > +
> > +	ret = __qcom_mdt_pas_init(ctx->dev, fw, firmware, ctx->peripheral,
> > +				  ctx->mem_phys, ctx->metadata);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return __qcom_mdt_load(ctx->dev, fw, firmware, mem_region, ctx->mem_phys,
> > +			       ctx->mem_size, reloc_base);
> 
> Surely you want to qcom_mdt_load_no_init ?
> 
> On current kernel
> return __qcom_mdt_load(ctx->dev, fw, firmware, mem_region, ctx,
> 		       mem_phys, ctx->mem_size,
> 		       reloc_base, true);
> 
> but that's a functional change
> 
> > +}
> > +EXPORT_SYMBOL_GPL(qcom_mdt_pas_load);
> the no_init version of this looks like this:
> 
> int qcom_mdt_load_no_init(struct device *dev, const struct firmware *fw,
>                           const char *firmware, int pas_id,
>                           void *mem_region, phys_addr_t mem_phys,
>                           size_t mem_size, phys_addr_t *reloc_base)
> {
>         return __qcom_mdt_load(dev, fw, firmware, pas_id, mem_region,
> 			       mem_phys, mem_size, reloc_base, false);
> }
> EXPORT_SYMBOL_GPL(qcom_mdt_load_no_init);
> 
> So is it really the intention of this patch to change the callsites where
> qcom_mdt_pas_load() away from the init version to the no_init version ?
> 
> Maybe its a symptom of patch collision but hmm, having a hard time
> cherry-picking this to test.

My intention is to unify all subsystems whether it's remoteproc, video,
or others using Secure PIL, so that they all use the same set of APIs
via context (cxt).

Like, we first initialize the context, and then use it for other PIL-related
SMC calls such as pas_init, mem_setup, auth_and_reset, or even for the
new rsc_table SMC call. This way, everything is connected, and it
becomes clear which functions need to be called and it will also be
extensible via a small handling for SHMbridge on gunyah absence. Similar
changes would also apply to the MDT loader APIs.

That's why I asked here after "---" in this patch if this approach is
preferred, I will apply it consistently and eliminate redundant APIs.

Let me know your thought.

> 
> ---
> bod
> 

-- 
-Mukesh Ojha