Message-ID: <7jjxjkk6qwym2mt6xp7t2t4wckyrvwaj2ydubkimnx2oybitab@u4nhj5mib64l>
Date: Sat, 28 Jun 2025 18:03:40 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Johan Hovold <johan@...nel.org>
Cc: Bjorn Andersson <andersson@...nel.org>,
        Maximilian Luz <luzmaximilian@...il.com>,
        Konrad Dybcio <konradybcio@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>, Ard Biesheuvel <ardb@...nel.org>,
        Steev Klimaszewski <steev@...i.org>, linux-arm-msm@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-efi@...r.kernel.org,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.com>
Subject: Re: [PATCH v4 6/8] firmware: qcom: scm: add modparam to control
 QSEECOM enablement

On Fri, Jun 27, 2025 at 02:46:38PM +0200, Johan Hovold wrote:
> On Fri, Jun 27, 2025 at 02:33:27AM +0300, Dmitry Baryshkov wrote:
> > On Thu, Jun 26, 2025 at 02:58:52PM +0200, Johan Hovold wrote:
> > > On Thu, Jun 26, 2025 at 02:08:23PM +0300, Dmitry Baryshkov wrote:
> > > > On Thu, Jun 26, 2025 at 12:11:20PM +0200, Johan Hovold wrote:
> > > > > On Wed, Jun 25, 2025 at 01:53:25AM +0300, Dmitry Baryshkov wrote:
> > > > > > From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> > > > > > 
> > > > > > In preparation to enabling QSEECOM for the platforms rather than
> > > > > > individual machines provide a mechanism for the user to override default
> > > > > > selection. Allow users to use qcom_scm.qseecom modparam.
> > > > > > 
> > > > > > Setting it to 'force' will enable QSEECOM even if it disabled or not
> > > > > > handled by the allowlist.
> > > > > > 
> > > > > > Setting it to 'off' will forcibly disable the QSEECOM interface,
> > > > > > allowing incompatible machines to function.
> > > > > > 
> > > > > > Setting it to 'roefivars' will enable the QSEECOM interface, making UEFI
> > > > > > variables read-only.
> > > > > > 
> > > > > > All other values mean 'auto', trusting the allowlist in the module.
> > > > > 
> > > > > I don't see the need for this. The kernel should just provide sensible
> > > > > defaults.
> > > > 
> > > > It does provide _defaults_. However with the next commit we mass-enable
> > > > QSEECOM for SoC families, which includes untested WoA devices. If the
> > > > user observes a misbehaviour of the UEFI vars or any other
> > > > QSEECOM-related driver on those platforms, it is much easier to let
> > > > users test and workaround UEFI misbehaviour.
> > > 
> > > You basically know by now which machines supports qseecom and which do
> > > not, right (e.g. UFS storage means non-persistent EFI vars)?
> 
> Do you have a theory about why on some platforms, like the one you're
> currently adding support for, writing UEFI variables does not work?
> 
> Can you please include that information in the series so we can consider
> alternate routes for replacing the current whitelist with this black and
> white thing you're going for.
> 
> Is it related to UFS at all, for example?

Strictly speaking I have no confirmation (yet), but there are two
theories:

- UFS vs SPI-NOR
- a edk2 PCD which controls whether SetVariable commits immediately or
  whether it just buffers data until EBS (or other call).

> 
> > > And it's a pretty bad user experience to have people trying to write
> > > efivariables when setting up a machine and then spend hours trying to
> > > debug why they don't persist after a reboot.
> > > 
> > > I don't think that's fair to users.
> > 
> > So, is it a user or a developer, trying to port Linux to a new hardware?
> > Also, R/O implementation makes it obvious, that the variables do not
> > persist.
> 
> A developer enabling support for a new platform can patch the driver and
> does not need a command line option.

Yes. But it's easier to debug things this way. Consider all ACPI-related
or UEFI-related kernel options that we have.

> 
> If you enable it by default, suddenly a bunch of end-users are going to
> have to debug why storing efi variables silently fails. That would not
> be fair to them.

I'm enabling this only for platforms where all existing devices are
listed in the current whitelist.

> 
> > > Let whoever brings up a new machine figure this out. It's just one
> > > entry, no scaling issues, and we get accurate information (unless
> > > Qualcomm, who sits on the documentation, is willing to provide it
> > > upfront).
> > 
> > And that's not really scallable. All other parts of a particular device
> > are described by the DT only (that's especially true on the PMIC GLINK
> > machines). If we are to support new laptop in e.g. distro kernel, we
> > need to provide a DT... and a patch for qcom-scm driver. I'd very much
> > prefer to do it other way around: provide a DT and patch qcom-scm if the
> > laptop is any way different from other laptops. E.g. we know that all
> > X1Elite laptops support R/W EFI variables.
> 
> But this is just kicking the can and putting the burden on someone else.
> Now a user or distro would need to pass command line parameters after
> spending time debugging why efi variable updates do not persist after a
> reboot.

The original developer for new DTS will have to do that anyway, if
something fails. And once it is done, we can add a quirk for that pure
platform. However the majority of the case can go without extra quirks.
As you can see, all X-Elite / X-Plus and majority of SC8280XP platforms
already are in the whitelist. Once we sort out SC8280XP-CRD issue, all
SC8280XP platforms supported upstream will have an entry in the
allowlist, which means we can convert them to the wildcard + quirks.

> If we know with reasonable certainty that all, say X1E, devices works,
> then that that's one thing.

Yes, we do. You can hand-compare the lists too (I did).

> But if this series now enables broken EFI variable support on every
> other device then I don't think that's ok (even if you provide a command
> line parameter that each user now have to pass).
> 
> Then I'd rather see a proposal for how to determine which machines
> support this or not, information which was not available when this
> interface was reverse engineered and where a conservative whitelist
> approach made perfect sense.

WIP

> 
> > Except for X1-CRD, which deserves an entry in the driver.
> 
> I think you meant my sc8280xp CRD here.

Yes.

> 
> Johan

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists