Message-ID: <c31638c715c9a78a8244ca47270085b73972e747.camel@infradead.org>
Date: Fri, 27 Sep 2024 14:20:28 +0100
From: David Woodhouse <dwmw2@...radead.org>
To: Miguel Luis <miguel.luis@...cle.com>
Cc: Souvik Chakravarty <Souvik.Chakravarty@....com>, Paolo Bonzini
 <pbonzini@...hat.com>, Jonathan Corbet <corbet@....net>, Marc Zyngier
 <maz@...nel.org>, 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>,
 Pavel Machek <pavel@....cz>, Len Brown <len.brown@...el.com>, Shuah Khan
 <shuah@...nel.org>,  "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
 "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>, 
 "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>, "kvmarm@...ts.linux.dev"
 <kvmarm@...ts.linux.dev>, "linux-pm@...r.kernel.org"
 <linux-pm@...r.kernel.org>,  "linux-kselftest@...r.kernel.org"
 <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH v4 1/6] firmware/psci: Add definitions for PSCI v1.3
 specification

On Fri, 2024-09-27 at 12:43 +0000, Miguel Luis wrote:
> Hi David,
> 
> > On 26 Sep 2024, at 16:30, David Woodhouse <dwmw2@...radead.org> wrote:
> > 
> > On Thu, 2024-09-26 at 09:56 +0000, Miguel Luis wrote:
> > > 
> > > > +/* PSCI v1.3 hibernate type for SYSTEM_OFF2 */
> > > > +#define PSCI_1_3_HIBERNATE_TYPE_OFF 0
> > > 
> > > Should it be 1 as hibernate type?
> > 
> > It is in discovery, as BIT(PSCI_1_3_HIBERNATE_TYPE_OFF) == 1<<0 == 1.
> > 
> 
> Now I see the definition for PSCI_1_3_HIBERNATE_TYPE_OFF was misleading for me
> when BIT(PSCI_1_3_HIBERNATE_TYPE_OFF) works for both discovery and as argument
> for SYSTEM_OFF2.

That *wasn't* the intent, as I understood it.

An early version of the spec just returned PSCI_1_3_HIBERNATE_TYPE_OFF
(zero) for discovery and also used it as the argument for SYSTEM_OFF2.

Obviously that doesn't allow for supporting any other types (at least,
not unless an implementation had to support *all* types up to the one
it advertises). So for *discovery* it was changed to a bitmap,
returning BIT(PSCI_1_3_HIBERNATE_TYPE_OFF), and explicitly documented
as "this field is a bitmap".

We discussed that, and settled on the changes, and I had completely
failed to spot that the beta spec then also quietly changed the actual
argument to SYSTEM_OFF2 from 0x0 to 0x1 for HIBERNATE_OFF too. I do not
recall that change ever being discussed, so thanks for catching it!

> The common factor being the bit offset in the bitmap for SYSTEM_OFF2 discovery
> and argument to call SYSTEM_OFF2 as well. Would it be clearer something like:
> 
> #define  PSCI_1_3_HIBERNATE_TYPE_OFF BIT(0)
> 
> Assuming future definitions would keep the same common factor can be helpful, however
> please let me know whether I am missing something.

Right. If the spec is going to stay as it is, then just defining it as
BIT(0) probably makes sense.

In practice, as I said, it doesn't make a lot of difference because the
KVM code handling SYSTEM_OFF2 doesn't even look at the argument. It
just sets a flag to let userspace know it was a SYSTEM_OFF2 call
instead of SYSTEM_OFF. Precisely the same way that SYSTEM_RESET2 is
handled.

If userspace wants to know the precise argument, I think it's supposed
to go digging in the registers for itself? And the only implementation
in existence that I know of doesn't bother; it treats *all* SYSTEM_OFF2
calls just the same, regardless of the argument. Since there is only
one possibility anyway.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)

Powered by blists - more mailing lists