On Fri, 2024-09-27 at 12:43 +0000, Miguel Luis wrote: > Hi David, > > > On 26 Sep 2024, at 16:30, David Woodhouse <dwmw2@xxxxxxxxxxxxx> 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.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
