[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <86edc2z0hs.wl-maz@kernel.org>
Date: Fri, 22 Mar 2024 16:37:19 +0000
From: Marc Zyngier <maz@...nel.org>
To: David Woodhouse <dwmw2@...radead.org>
Cc: linux-arm-kernel@...ts.infradead.org,
kvm@...r.kernel.org,
Paolo Bonzini
<pbonzini@...hat.com>,
Jonathan Corbet <corbet@....net>,
Oliver Upton
<oliver.upton@...ux.dev>,
James Morse <james.morse@....com>,
Suzuki K
Poulose <suzuki.poulose@....com>,
Zenghui Yu <yuzenghui@...wei.com>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Lorenzo Pieralisi
<lpieralisi@...nel.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Len Brown
<len.brown@...el.com>,
Pavel Machek <pavel@....cz>,
Mostafa Saleh
<smostafa@...gle.com>,
Jean-Philippe Brucker <jean-philippe@...aro.org>,
linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org,
kvmarm@...ts.linux.dev,
linux-pm@...r.kernel.org
Subject: Re: [RFC PATCH v3 5/5] arm64: Use SYSTEM_OFF2 PSCI call to power off for hibernate
On Fri, 22 Mar 2024 16:12:44 +0000,
David Woodhouse <dwmw2@...radead.org> wrote:
>
> On Fri, 2024-03-22 at 16:02 +0000, Marc Zyngier wrote:
> > On Tue, 19 Mar 2024 12:59:06 +0000,
> > David Woodhouse <dwmw2@...radead.org> wrote:
> >
> > [...]
> >
> > > +static void __init psci_init_system_off2(void)
> > > +{
> > > + int ret;
> > > +
> > > + ret = psci_features(PSCI_FN_NATIVE(1_3, SYSTEM_OFF2));
> > > +
> > > + if (ret != PSCI_RET_NOT_SUPPORTED)
> > > + psci_system_off2_supported = true;
> >
> > It'd be worth considering the (slightly broken) case where SYSTEM_OFF2
> > is supported, but HIBERNATE_OFF is not set in the response, as the
> > spec doesn't say that this bit is mandatory (it seems legal to
> > implement SYSTEM_OFF2 without any hibernate type, making it similar to
> > SYSTEM_OFF).
>
> Such is not my understanding. If SYSTEM_OFF2 is supported, then
> HIBERNATE_OFF *is* mandatory.
>
> The next update to the spec is turning the PSCI_FEATURES response into
> a *bitmap* of the available features, and I believe it will mandate
> that bit zero is set.
The bitmap is already present in the current Alpha spec:
<quote>
5.16.2 Implementation responsibilities
[...]
Bits[31] Reserved, must be zero.
Bits[30:0] Hibernate types supported.
- 0x0 - HIBERNATE_OFF
All other values are reserved for future use.
</quote>
and doesn't say (yet) that HIBERNATE_OFF is mandatory. Furthermore,
<quote>
5.11.2 Caller responsibilities
The calling OS uses the PSCI_FEATURES API, with the SYSTEM_OFF2
function ID, to discover whether the function is present:
- If the function is implemented, PSCI_FEATURES returns the hibernate
types supported.
- If the function is not implemented, PSCI_FEATURES returns
NOT_SUPPORTED.
</quote>
which doesn't say anything about which hibernate type must be
implemented. Which makes sense, as I expect it to, in the fine ARM
tradition, grow things such as "HIBERNATE_WITH_ROT13_ENCRYPTION" and
even "HIBERNATE_WITH_ERRATA_XYZ", because firmware is where people
dump their crap. And we will need some special handling for these
tasty variants.
> And if for whatever reason that SYSTEM_OFF2/HIBERNATE_OFF call
> *doesn't* work, Linux will end up doing a 'real' poweroff, first
> through EFI and then finally as a last resort with a PSCI SYSTEM_OFF.
> So it would be OK to have false positives in the detection.
I agree that nothing really breaks, but I also hold the view that
broken firmware implementations should be given the finger, specially
given that you have done this work *ahead* of the spec. I would really
like this to fail immediately on these and not even try to suspend.
With that in mind, if doesn't really matter whether HIBERNATE_OFF is
mandatory or not. We really should check for it and pretend it doesn't
exist if the correct flag isn't set.
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists