From: Marc Zyngier <maz@kernel.org> To: David Woodhouse <dwmw2@infradead.org> Cc: linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>, Jonathan Corbet <corbet@lwn.net>, Oliver Upton <oliver.upton@linux.dev>, James Morse <james.morse@arm.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Zenghui Yu <yuzenghui@huawei.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Lorenzo Pieralisi <lpieralisi@kernel.org>, "Rafael J. Wysocki" <rafael@kernel.org>, Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>, Mostafa Saleh <smostafa@google.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, kvmarm@lists.linux.dev, linux-pm@vger.kernel.org Subject: Re: [RFC PATCH v3 5/5] arm64: Use SYSTEM_OFF2 PSCI call to power off for hibernate Date: Fri, 22 Mar 2024 16:37:19 +0000 [thread overview] Message-ID: <86edc2z0hs.wl-maz@kernel.org> (raw) In-Reply-To: <9efb39597fa7b36b6c4202ab73fae6610194e45e.camel@infradead.org> On Fri, 22 Mar 2024 16:12:44 +0000, David Woodhouse <dwmw2@infradead.org> wrote: > > On Fri, 2024-03-22 at 16:02 +0000, Marc Zyngier wrote: > > On Tue, 19 Mar 2024 12:59:06 +0000, > > David Woodhouse <dwmw2@infradead.org> wrote: > > > > [...] > > > > > +static void __init psci_init_system_off2(void) > > > +{ > > > + int ret; > > > + > > > + ret = psci_features(PSCI_FN_NATIVE(1_3, SYSTEM_OFF2)); > > > + > > > + if (ret != PSCI_RET_NOT_SUPPORTED) > > > + psci_system_off2_supported = true; > > > > It'd be worth considering the (slightly broken) case where SYSTEM_OFF2 > > is supported, but HIBERNATE_OFF is not set in the response, as the > > spec doesn't say that this bit is mandatory (it seems legal to > > implement SYSTEM_OFF2 without any hibernate type, making it similar to > > SYSTEM_OFF). > > Such is not my understanding. If SYSTEM_OFF2 is supported, then > HIBERNATE_OFF *is* mandatory. > > The next update to the spec is turning the PSCI_FEATURES response into > a *bitmap* of the available features, and I believe it will mandate > that bit zero is set. The bitmap is already present in the current Alpha spec: <quote> 5.16.2 Implementation responsibilities [...] Bits[31] Reserved, must be zero. Bits[30:0] Hibernate types supported. - 0x0 - HIBERNATE_OFF All other values are reserved for future use. </quote> and doesn't say (yet) that HIBERNATE_OFF is mandatory. Furthermore, <quote> 5.11.2 Caller responsibilities The calling OS uses the PSCI_FEATURES API, with the SYSTEM_OFF2 function ID, to discover whether the function is present: - If the function is implemented, PSCI_FEATURES returns the hibernate types supported. - If the function is not implemented, PSCI_FEATURES returns NOT_SUPPORTED. </quote> which doesn't say anything about which hibernate type must be implemented. Which makes sense, as I expect it to, in the fine ARM tradition, grow things such as "HIBERNATE_WITH_ROT13_ENCRYPTION" and even "HIBERNATE_WITH_ERRATA_XYZ", because firmware is where people dump their crap. And we will need some special handling for these tasty variants. > And if for whatever reason that SYSTEM_OFF2/HIBERNATE_OFF call > *doesn't* work, Linux will end up doing a 'real' poweroff, first > through EFI and then finally as a last resort with a PSCI SYSTEM_OFF. > So it would be OK to have false positives in the detection. I agree that nothing really breaks, but I also hold the view that broken firmware implementations should be given the finger, specially given that you have done this work *ahead* of the spec. I would really like this to fail immediately on these and not even try to suspend. With that in mind, if doesn't really matter whether HIBERNATE_OFF is mandatory or not. We really should check for it and pretend it doesn't exist if the correct flag isn't set. M. -- Without deviation from the norm, progress is not possible.
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: David Woodhouse <dwmw2@infradead.org> Cc: linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>, Jonathan Corbet <corbet@lwn.net>, Oliver Upton <oliver.upton@linux.dev>, James Morse <james.morse@arm.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Zenghui Yu <yuzenghui@huawei.com>, Catalin Marinas <catalin.marinas@arm.com>, Will Deacon <will@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Lorenzo Pieralisi <lpieralisi@kernel.org>, "Rafael J. Wysocki" <rafael@kernel.org>, Len Brown <len.brown@intel.com>, Pavel Machek <pavel@ucw.cz>, Mostafa Saleh <smostafa@google.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, kvmarm@lists.linux.dev, linux-pm@vger.kernel.org Subject: Re: [RFC PATCH v3 5/5] arm64: Use SYSTEM_OFF2 PSCI call to power off for hibernate Date: Fri, 22 Mar 2024 16:37:19 +0000 [thread overview] Message-ID: <86edc2z0hs.wl-maz@kernel.org> (raw) In-Reply-To: <9efb39597fa7b36b6c4202ab73fae6610194e45e.camel@infradead.org> On Fri, 22 Mar 2024 16:12:44 +0000, David Woodhouse <dwmw2@infradead.org> wrote: > > On Fri, 2024-03-22 at 16:02 +0000, Marc Zyngier wrote: > > On Tue, 19 Mar 2024 12:59:06 +0000, > > David Woodhouse <dwmw2@infradead.org> wrote: > > > > [...] > > > > > +static void __init psci_init_system_off2(void) > > > +{ > > > + int ret; > > > + > > > + ret = psci_features(PSCI_FN_NATIVE(1_3, SYSTEM_OFF2)); > > > + > > > + if (ret != PSCI_RET_NOT_SUPPORTED) > > > + psci_system_off2_supported = true; > > > > It'd be worth considering the (slightly broken) case where SYSTEM_OFF2 > > is supported, but HIBERNATE_OFF is not set in the response, as the > > spec doesn't say that this bit is mandatory (it seems legal to > > implement SYSTEM_OFF2 without any hibernate type, making it similar to > > SYSTEM_OFF). > > Such is not my understanding. If SYSTEM_OFF2 is supported, then > HIBERNATE_OFF *is* mandatory. > > The next update to the spec is turning the PSCI_FEATURES response into > a *bitmap* of the available features, and I believe it will mandate > that bit zero is set. The bitmap is already present in the current Alpha spec: <quote> 5.16.2 Implementation responsibilities [...] Bits[31] Reserved, must be zero. Bits[30:0] Hibernate types supported. - 0x0 - HIBERNATE_OFF All other values are reserved for future use. </quote> and doesn't say (yet) that HIBERNATE_OFF is mandatory. Furthermore, <quote> 5.11.2 Caller responsibilities The calling OS uses the PSCI_FEATURES API, with the SYSTEM_OFF2 function ID, to discover whether the function is present: - If the function is implemented, PSCI_FEATURES returns the hibernate types supported. - If the function is not implemented, PSCI_FEATURES returns NOT_SUPPORTED. </quote> which doesn't say anything about which hibernate type must be implemented. Which makes sense, as I expect it to, in the fine ARM tradition, grow things such as "HIBERNATE_WITH_ROT13_ENCRYPTION" and even "HIBERNATE_WITH_ERRATA_XYZ", because firmware is where people dump their crap. And we will need some special handling for these tasty variants. > And if for whatever reason that SYSTEM_OFF2/HIBERNATE_OFF call > *doesn't* work, Linux will end up doing a 'real' poweroff, first > through EFI and then finally as a last resort with a PSCI SYSTEM_OFF. > So it would be OK to have false positives in the detection. I agree that nothing really breaks, but I also hold the view that broken firmware implementations should be given the finger, specially given that you have done this work *ahead* of the spec. I would really like this to fail immediately on these and not even try to suspend. With that in mind, if doesn't really matter whether HIBERNATE_OFF is mandatory or not. We really should check for it and pretend it doesn't exist if the correct flag isn't set. M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-03-22 16:37 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-03-19 12:59 [RFC PATCH v3 0/5] Add PSCI v1.3 SYSTEM_OFF2 support for hibernation David Woodhouse 2024-03-19 12:59 ` David Woodhouse 2024-03-19 12:59 ` [RFC PATCH v3 1/5] firmware/psci: Add definitions for PSCI v1.3 specification (ALPHA) David Woodhouse 2024-03-19 12:59 ` David Woodhouse 2024-03-19 12:59 ` [RFC PATCH v3 2/5] KVM: arm64: Add support for PSCI v1.2 and v1.3 David Woodhouse 2024-03-19 12:59 ` David Woodhouse 2024-03-19 15:42 ` Oliver Upton 2024-03-19 15:42 ` Oliver Upton 2024-03-19 15:52 ` David Woodhouse 2024-03-19 15:52 ` David Woodhouse 2024-03-22 16:05 ` Marc Zyngier 2024-03-22 16:05 ` Marc Zyngier 2024-03-22 16:14 ` David Woodhouse 2024-03-22 16:14 ` David Woodhouse 2024-03-19 12:59 ` [RFC PATCH v3 3/5] KVM: arm64: Add PSCI v1.3 SYSTEM_OFF2 function for hibernation David Woodhouse 2024-03-19 12:59 ` David Woodhouse 2024-03-22 16:06 ` Marc Zyngier 2024-03-22 16:06 ` Marc Zyngier 2024-03-19 12:59 ` [RFC PATCH v3 4/5] KVM: arm64: nvhe: Pass through PSCI v1.3 SYSTEM_OFF2 call David Woodhouse 2024-03-19 12:59 ` David Woodhouse 2024-03-19 12:59 ` [RFC PATCH v3 5/5] arm64: Use SYSTEM_OFF2 PSCI call to power off for hibernate David Woodhouse 2024-03-19 12:59 ` David Woodhouse 2024-03-22 16:02 ` Marc Zyngier 2024-03-22 16:02 ` Marc Zyngier 2024-03-22 16:12 ` David Woodhouse 2024-03-22 16:12 ` David Woodhouse 2024-03-22 16:37 ` Marc Zyngier [this message] 2024-03-22 16:37 ` Marc Zyngier 2024-03-22 16:55 ` David Woodhouse 2024-03-22 16:55 ` David Woodhouse 2024-03-22 17:08 ` Sudeep Holla 2024-03-22 17:08 ` Sudeep Holla 2024-03-22 17:05 ` Sudeep Holla 2024-03-22 17:05 ` Sudeep Holla 2024-03-19 15:27 ` [RFC PATCH v3 0/5] Add PSCI v1.3 SYSTEM_OFF2 support for hibernation Oliver Upton 2024-03-19 15:27 ` Oliver Upton 2024-03-19 17:14 ` David Woodhouse 2024-03-19 17:14 ` David Woodhouse 2024-03-19 19:41 ` Oliver Upton 2024-03-19 19:41 ` Oliver Upton 2024-03-22 10:17 ` David Woodhouse 2024-03-22 10:17 ` David Woodhouse 2024-03-22 16:09 ` Marc Zyngier 2024-03-22 16:09 ` Marc Zyngier 2024-03-22 17:33 ` David Woodhouse 2024-03-22 17:33 ` David Woodhouse
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=86edc2z0hs.wl-maz@kernel.org \ --to=maz@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=corbet@lwn.net \ --cc=dwmw2@infradead.org \ --cc=james.morse@arm.com \ --cc=jean-philippe@linaro.org \ --cc=kvm@vger.kernel.org \ --cc=kvmarm@lists.linux.dev \ --cc=len.brown@intel.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pm@vger.kernel.org \ --cc=lpieralisi@kernel.org \ --cc=mark.rutland@arm.com \ --cc=oliver.upton@linux.dev \ --cc=pavel@ucw.cz \ --cc=pbonzini@redhat.com \ --cc=rafael@kernel.org \ --cc=smostafa@google.com \ --cc=suzuki.poulose@arm.com \ --cc=will@kernel.org \ --cc=yuzenghui@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.