| 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 |
On Tue, 2024-09-24 at 17:05 +0100, David Woodhouse 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. The CLEAN_INV_MEMREGION and CLEAN_INV_MEMREGION_ATTRIBUTES functions were added in the alpha release of the spec but have been dropped in the beta release, and are not included in the final spec. So IMO the uapi header file should not contain these definitions. The same goes for the TIMEOUT, RATE_LIMITED and BUSY error values.
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 10:55 +0200, Francesco Lavra wrote: > On Tue, 2024-09-24 at 17:05 +0100, David Woodhouse 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. > > The CLEAN_INV_MEMREGION and CLEAN_INV_MEMREGION_ATTRIBUTES > functions were added in the alpha release of the spec but have been > dropped in the beta release, and are not included in the final spec. So > IMO the uapi header file should not contain these definitions. > The same goes for the TIMEOUT, RATE_LIMITED and BUSY error values. Thanks. I'll drop those.
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 */