Message-ID: <d0f29ed7c2e05d18e8f627fa4fa0e210445fb885.camel@infradead.org>
Date: Thu, 31 Oct 2024 13:00:20 -0500
From: David Woodhouse <dwmw2@...radead.org>
To: Oliver Upton <oliver.upton@...ux.dev>
Cc: Miguel Luis <miguel.luis@...cle.com>, Paolo Bonzini
 <pbonzini@...hat.com>,  Jonathan Corbet <corbet@....net>, Marc Zyngier
 <maz@...nel.org>, 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>, Francesco Lavra
 <francescolavra.fl@...il.com>
Subject: Re: [PATCH v6 6/6] arm64: Use SYSTEM_OFF2 PSCI call to power off
 for hibernate

On Fri, 2024-10-25 at 13:40 -0700, Oliver Upton wrote:
> 
> No. You're leaving the work for the reader to:
> 
>  (1) Figure out what you're talking about
>  (2) Go dig up an "earlier version" of the spec
>  (3) Realise that it means certain hypervisors only take 0x0 as an
>      argument

No. There's no need to dig up an 'earlier version' of the spec. The
current F.b release says, "if the value of this parameter is 0x0, the
implementation defaults to selecting HIBERNATE_OFF". That's what makes
it an acceptable alternative.

> If you speak *directly* about the problem you're trying to address then
> reviewers are less likely to get hung up on what you're trying to do.

The "problem" this comment addresses is a reader who looks at this code
and wonders why it uses zero instead of HIBERNATE_OFF.

Firstly, that reader needs to spot the text, in the *current* version
of the spec as cited above, which makes it clear that it's a perfectly
acceptable alternative.

Secondly, that reader needs to know why we chose zero over
HIBERNATE_OFF, which is also explained fairly succinctly: because it's
compatible with hypervisors implementing an earlier version of the
spec.

> I'm genuinely at a loss for why you would want to present this as an
> "acceptable alterantive" rather than a functional requirement for this
> driver to run on your hypervisor.

Because "my" hypervisor is a live product which actually gets security
fixes and updates. If it wasn't for the silly season of "thou shalt not
ship *anything* except security fixes and stuff that's going to be
announced at a big conference at the end of the month", the change to
make it accept the new value of HIBERNATE_OFF would already have been
deployed.

And *even* with the silly season delaying non-critical hypervisor
changes, your suggested wording will *still* probably not be true by
the time the comment is included in an actual release of the Linux
kernel.

But honestly, it isn't a hill I'm prepared to die on. If you insist on
changing the comment to your 'There are hypervisors in the wile that
violate the spec...' version, by all means go ahead and do so. I'll
follow up with a patch to correct it in a few weeks when it becomes
obsolete.

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

Powered by blists - more mailing lists