| Message ID | 20240924160512.4138879-1-dwmw2@infradead.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,1/6] firmware/psci: Add definitions for PSCI v1.3 specification | expand |
Hi David, > On 24 Sep 2024, at 16:05, David Woodhouse <dwmw2@infradead.org> wrote: > > From: David Woodhouse <dwmw@amazon.co.uk> > > The v1.3 PSCI spec (https://developer.arm.com/documentation/den0022) adds > SYSTEM_OFF2, CLEAN_INV_MEMREGION and CLEAN_INV_MEMREGION_ATTRIBUTES > functions. Add definitions for them and their parameters, along with the > new TIMEOUT, RATE_LIMITED and BUSY error values. > DEN0022F REL superseded DEN0022F ALP1 which doesn’t describe CLEAN_INV_MEMREGION or CLEAN_INV_MEMREGION_ATTRIBUTES. Defining those at another time shouldn’t be a blocker for the rest of this patchset. > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > --- > include/uapi/linux/psci.h | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h > index 42a40ad3fb62..082ed689fdaf 100644 > --- a/include/uapi/linux/psci.h > +++ b/include/uapi/linux/psci.h > @@ -59,6 +59,8 @@ > #define PSCI_1_1_FN_SYSTEM_RESET2 PSCI_0_2_FN(18) > #define PSCI_1_1_FN_MEM_PROTECT PSCI_0_2_FN(19) > #define PSCI_1_1_FN_MEM_PROTECT_CHECK_RANGE PSCI_0_2_FN(20) > +#define PSCI_1_3_FN_SYSTEM_OFF2 PSCI_0_2_FN(21) > +#define PSCI_1_3_FN_CLEAN_INV_MEMREGION_ATTRIBUTES PSCI_0_2_FN(23) > > #define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND PSCI_0_2_FN64(12) > #define PSCI_1_0_FN64_NODE_HW_STATE PSCI_0_2_FN64(13) > @@ -68,6 +70,8 @@ > > #define PSCI_1_1_FN64_SYSTEM_RESET2 PSCI_0_2_FN64(18) > #define PSCI_1_1_FN64_MEM_PROTECT_CHECK_RANGE PSCI_0_2_FN64(20) > +#define PSCI_1_3_FN64_SYSTEM_OFF2 PSCI_0_2_FN64(21) > +#define PSCI_1_3_FN64_CLEAN_INV_MEMREGION PSCI_0_2_FN64(22) > > /* PSCI v0.2 power state encoding for CPU_SUSPEND function */ > #define PSCI_0_2_POWER_STATE_ID_MASK 0xffff > @@ -100,6 +104,19 @@ > #define PSCI_1_1_RESET_TYPE_SYSTEM_WARM_RESET 0 > #define PSCI_1_1_RESET_TYPE_VENDOR_START 0x80000000U > > +/* PSCI v1.3 hibernate type for SYSTEM_OFF2 */ > +#define PSCI_1_3_HIBERNATE_TYPE_OFF 0 Should it be 1 as hibernate type? > + > +/* PSCI v1.3 flags for CLEAN_INV_MEMREGION */ > +#define PSCI_1_3_CLEAN_INV_MEMREGION_FLAG_DRY_RUN BIT(0) > + > +/* PSCI v1.3 attributes for CLEAN_INV_MEMREGION_ATTRIBUTES */ > +#define PSCI_1_3_CLEAN_INV_MEMREGION_ATTR_OP_TYPE 0 > +#define PSCI_1_3_CLEAN_INV_MEMREGION_ATTR_CPU_RDVZ 1 > +#define PSCI_1_3_CLEAN_INV_MEMREGION_ATTR_LATENCY 2 > +#define PSCI_1_3_CLEAN_INV_MEMREGION_ATTR_RATE_LIMIT 3 > +#define PSCI_1_3_CLEAN_INV_MEMREGION_ATTR_TIMEOUT 4 > + > /* PSCI version decoding (independent of PSCI version) */ > #define PSCI_VERSION_MAJOR_SHIFT 16 > #define PSCI_VERSION_MINOR_MASK \ > @@ -133,5 +150,8 @@ > #define PSCI_RET_NOT_PRESENT -7 > #define PSCI_RET_DISABLED -8 > #define PSCI_RET_INVALID_ADDRESS -9 > +#define PSCI_RET_TIMEOUT -10 > +#define PSCI_RET_RATE_LIMITED -11 > +#define PSCI_RET_BUSY -12 > Thanks Miguel > #endif /* _UAPI_LINUX_PSCI_H */ > -- > 2.44.0 > >
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. But using a bitmask was only supposed to be for the discovery with PSCI_FEATURES, as that has to advertise all the available hibernation types. The actual SYSTEM_OFF2 call was supposed to just take the numeric value as an argument, since obviously *that* one isn't a bitmask. Except... I see that now the spec has finally been updated, it seems to say that 0x1 is the value to pass to the SYSTEM_OFF2 call for HIBERNATE_OFF, not 0x0. Which doesn't seem to make much sense, and I don't recall it being what we discussed. Souvik, what happened there? My understanding was that for each supported hibernation type #n, for which HIBERERNATE_OFF is zero), the PSCI_FEATURES query would include the bit (1<<n) to indicate that it is supported, and then the actual SYSTEM_OFF2 call parameter would be (n) itself, precisely as implemented here. But the spec now seems to say that HIBERNATE_OFF is advertised as (1<<0) in PSCI_FEATURES, but invoked with the value (1). Is it too late to fix? If it isn't just a thinko, what is the intent in the current spec? If we have new hibernate types such that #define PSCI_1_3_HIBERNATE_TYPE_OFF 0 #define PSCI_1_3_HIBERNATE_TYPE_FOO 1 #define PSCI_1_3_HIBERNATE_TYPE_BAR 2 It seems obvious that the PSCI_FEATURES response will contain (1<<0), (1<<1) and (1<<2) for them respectively, but what is supposed to be passed to the actual SYSTEM_OFF2 call? Is it always just going to be (PSCI_1_3_HIBERNATE_TYPE_xxx + 1)? I think we should just fix §5.1.10 to report that 0x0 is HIBERNATE_OFF, yes?
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. 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. Thanks, Miguel > But using a bitmask was only supposed to be for the discovery with > PSCI_FEATURES, as that has to advertise all the available hibernation > types. > > The actual SYSTEM_OFF2 call was supposed to just take the numeric value > as an argument, since obviously *that* one isn't a bitmask. > > Except... I see that now the spec has finally been updated, it seems to > say that 0x1 is the value to pass to the SYSTEM_OFF2 call for > HIBERNATE_OFF, not 0x0. Which doesn't seem to make much sense, and I > don't recall it being what we discussed. Souvik, what happened there? > > My understanding was that for each supported hibernation type #n, for > which HIBERERNATE_OFF is zero), the PSCI_FEATURES query would include > the bit (1<<n) to indicate that it is supported, and then the actual > SYSTEM_OFF2 call parameter would be (n) itself, precisely as > implemented here. > > But the spec now seems to say that HIBERNATE_OFF is advertised as > (1<<0) in PSCI_FEATURES, but invoked with the value (1). > > Is it too late to fix? > > If it isn't just a thinko, what is the intent in the current spec? > > If we have new hibernate types such that > > #define PSCI_1_3_HIBERNATE_TYPE_OFF 0 > #define PSCI_1_3_HIBERNATE_TYPE_FOO 1 > #define PSCI_1_3_HIBERNATE_TYPE_BAR 2 > > It seems obvious that the PSCI_FEATURES response will contain (1<<0), > (1<<1) and (1<<2) for them respectively, but what is supposed to be > passed to the actual SYSTEM_OFF2 call? Is it always just going to be > (PSCI_1_3_HIBERNATE_TYPE_xxx + 1)? > > I think we should just fix §5.1.10 to report that 0x0 is HIBERNATE_OFF, > yes? >
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.
On Fri, 2024-09-27 at 12:43 +0000, Miguel Luis wrote: > > 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) I've updated the tree at https://git.infradead.org/users/dwmw2/linux.git/shortlog/refs/heads/psci-hibernate to do it that way. As I did so, I realised that KVM *does* care about the argument to SYSTEM_OFF2. This is a straight copy of the SYSTEM_RESET2 handling. Although it doesn't pass the argument up to userspace (presumably userspace is expected to look at the registers if it cares), it *does* check it's within the range of permitted values and return PSCI_RET_INVALID_PARAMS if not. I've changed that check to: arg = smccc_get_arg1(vcpu); /* * Permit zero to mean HIBERNATE_OFF as well as the bitmap * form which was introduced in PSCI v1.3 beta. */ if (arg && arg != PSCI_1_3_HIBERNATE_TYPE_OFF) { val = PSCI_RET_INVALID_PARAMS; break; } kvm_psci_system_off2(vcpu); On the guest side, I've changed the invocation to: static int psci_sys_hibernate(struct sys_off_data *data) { /* * Zero is an acceptable alternative to PSCI_1_3_HIBERNATE_TYPE_OFF * and is supported by hypervisors implementing an earlier version * of the pSCI v1.3 spec. */ if (system_entering_hibernation()) invoke_psci_fn(PSCI_FN_NATIVE(1_3, SYSTEM_OFF2), 0 /*PSCI_1_3_HIBERNATE_TYPE_OFF*/, 0, 0); return NOTIFY_DONE; } I'm going to do some careful interop testing with existing and new hypervisors before reposting this version.
diff --git a/include/uapi/linux/psci.h b/include/uapi/linux/psci.h index 42a40ad3fb62..082ed689fdaf 100644 --- a/include/uapi/linux/psci.h +++ b/include/uapi/linux/psci.h @@ -59,6 +59,8 @@ #define PSCI_1_1_FN_SYSTEM_RESET2 PSCI_0_2_FN(18) #define PSCI_1_1_FN_MEM_PROTECT PSCI_0_2_FN(19) #define PSCI_1_1_FN_MEM_PROTECT_CHECK_RANGE PSCI_0_2_FN(20) +#define PSCI_1_3_FN_SYSTEM_OFF2 PSCI_0_2_FN(21) +#define PSCI_1_3_FN_CLEAN_INV_MEMREGION_ATTRIBUTES PSCI_0_2_FN(23) #define PSCI_1_0_FN64_CPU_DEFAULT_SUSPEND PSCI_0_2_FN64(12) #define PSCI_1_0_FN64_NODE_HW_STATE PSCI_0_2_FN64(13) @@ -68,6 +70,8 @@ #define PSCI_1_1_FN64_SYSTEM_RESET2 PSCI_0_2_FN64(18) #define PSCI_1_1_FN64_MEM_PROTECT_CHECK_RANGE PSCI_0_2_FN64(20) +#define PSCI_1_3_FN64_SYSTEM_OFF2 PSCI_0_2_FN64(21) +#define PSCI_1_3_FN64_CLEAN_INV_MEMREGION PSCI_0_2_FN64(22) /* PSCI v0.2 power state encoding for CPU_SUSPEND function */ #define PSCI_0_2_POWER_STATE_ID_MASK 0xffff @@ -100,6 +104,19 @@ #define PSCI_1_1_RESET_TYPE_SYSTEM_WARM_RESET 0 #define PSCI_1_1_RESET_TYPE_VENDOR_START 0x80000000U +/* PSCI v1.3 hibernate type for SYSTEM_OFF2 */ +#define PSCI_1_3_HIBERNATE_TYPE_OFF 0 + +/* PSCI v1.3 flags for CLEAN_INV_MEMREGION */ +#define PSCI_1_3_CLEAN_INV_MEMREGION_FLAG_DRY_RUN BIT(0) + +/* PSCI v1.3 attributes for CLEAN_INV_MEMREGION_ATTRIBUTES */ +#define PSCI_1_3_CLEAN_INV_MEMREGION_ATTR_OP_TYPE 0 +#define PSCI_1_3_CLEAN_INV_MEMREGION_ATTR_CPU_RDVZ 1 +#define PSCI_1_3_CLEAN_INV_MEMREGION_ATTR_LATENCY 2 +#define PSCI_1_3_CLEAN_INV_MEMREGION_ATTR_RATE_LIMIT 3 +#define PSCI_1_3_CLEAN_INV_MEMREGION_ATTR_TIMEOUT 4 + /* PSCI version decoding (independent of PSCI version) */ #define PSCI_VERSION_MAJOR_SHIFT 16 #define PSCI_VERSION_MINOR_MASK \ @@ -133,5 +150,8 @@ #define PSCI_RET_NOT_PRESENT -7 #define PSCI_RET_DISABLED -8 #define PSCI_RET_INVALID_ADDRESS -9 +#define PSCI_RET_TIMEOUT -10 +#define PSCI_RET_RATE_LIMITED -11 +#define PSCI_RET_BUSY -12 #endif /* _UAPI_LINUX_PSCI_H */