Re: [PATCH v6 6/6] arm64: Use SYSTEM_OFF2 PSCI call to power off for hibernate

[Oliver Upton <oliver.upton@linux.dev>]

On Fri, Oct 25, 2024 at 08:13:03AM +0200, David Woodhouse wrote:
> On 24 October 2024 21:57:38 CEST, Oliver Upton <oliver.upton@linux.dev> wrote:
> >On Thu, Oct 24, 2024 at 05:56:09PM +0200, David Woodhouse wrote:
> >> On 24 October 2024 17:44:49 CEST, Oliver Upton <oliver.upton@linux.dev> wrote:
> >> >IIUC, you're really wanting to 0x0 because there are hypervisors out
> >> >there that violate the final spec and *only* accept this value.
> >> >
> >> >That's perfectly fine, but it'd help avoid confusion if the supporting
> >> >comment was a bit more direct:
> >> >
> >> >	/*
> >> >	 * If no hibernate type is specified SYSTEM_OFF2 defaults to
> >> >	 * selecting HIBERNATE_OFF.
> >> >	 *
> >> >	 * There are hypervisors in the wild that violate the spec and
> >> >	 * reject calls that explicitly provide a hibernate type. For
> >> >	 * compatibility with these nonstandard implementations, pass 0
> >> >	 * as the type.
> >> >	 */
> >> >	 if (system_entering_hibernation())
> >> >		invoke_psci_fn(PSCI_FN_NATIVE(1_3, SYSTEM_OFF2), 0 , 0, 0);
> >> 
> >> By the time this makes it into released versions of the guest Linux kernel, that comment won't be true any more.
> >
> >Then does it even matter? What is the problem you're trying to solve
> >with using a particular value for the hibernate type?
> >
> >Either the goal of this is to make the PSCI client code compatible with
> >your hypervisor today (and any other implementation based on 'F ALP1') or
> >we don't care and go with whatever value we want.
> >
> >Even if the comment eventually becomes stale, there is a ton of value in
> >documenting the exact implementation decision being made.
> >
> 
> Eventually it won't matter and we can go with whatever value we want. But yes, the goal is to be compatible with the hypervisor *today* until it catches up the changes to the final versions of the spec. I didn't spend much time overthinking the comment. What was it....
> 
> 	/*
> 	 * Zero is an acceptable alternative to PSCI_1_3_OFF_TYPE_HIBERNATE_OFF
> 	 * and is supported by hypervisors implementing an earlier version
> 	 * of the pSCI v1.3 spec.
> 	 */
> 
> That seems to cover it just fine, I think.

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

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.

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.

-- 
Thanks,
Oliver