On Fri, 2024-09-27 at 12:43 +0000, Miguel Luis wrote:
Hi David,
On 26 Sep 2024, at 16:30, David Woodhouse dwmw2@infradead.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.