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] [day] [month] [year] [list]

Message-ID: <20250904114324.qtizk6wm44xa4ryj@hu-mojha-hyd.qualcomm.com>
Date: Thu, 4 Sep 2025 17:13:24 +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 Thu, Sep 04, 2025 at 11:15:29AM +0100, Bryan O'Donoghue wrote:
> On 04/09/2025 10:52, Mukesh Ojha wrote:
> > > 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.
> 
> For me its a question of digesting the change.
> 
> Your series says "Add context aware qcom_mdt_pas_load()" but, it does more
> than add - it changes logic.
> 
> At a minimum I'd suggest splitting the addition of the function from the
> changing of the existing logic so that the two could be disambiguated.

I agree, I did more than just add even used in current patch itself.
Will split it.

> 
> The other comment I have is, is this change really required to enable pil
> loading @ EL2 ?

Not exactly, but to looks things cleaner..

As the other way was carry extra boolean "rproc->has_iommu" in qcom_mdt_pas_init() 
for rproc and qcom_mdt_load() for video and other smc function to tell what needs
to be done extra when Linux at EL2.

> 
> You could for example structure this series to implement the changes you
> need for EL2 - and then have a last patch at the end to make the code "more
> beautiful" or even a second series to do that.
> 
> So I'd suggest one of
> 
> 1. Splitting the addition of the new helper and its use into
>    separate patches in the same series.
> 
> or
> 
> 2. Doing the full EL2 conversion and then applying the
>    "make the code look nice" patch last.
>    So that we could for example take 11 of 13 patches
> 
> or
> 
> 3. Making the EL2 conversion and the posting a second series
>    with your proposed tidy up
> 
> Either way for me splicing both the addition and the usage here is a bit
> hard to parse, especially since I can't seem to find a public SHA where this
> series cleanly applies.

I'm fine with 2 and 3 as well only if non-cleaner way with EL2
enablement gets accepted which may look ugly with lots of if's
in the code or just do 1 for now.

> 
> ---
> bod

-- 
-Mukesh Ojha