| Message ID | 20231012072148.7010-4-ajones@ventanamicro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: Introduce system suspend support | expand |
Yo, On Thu, Oct 12, 2023 at 09:21:50AM +0200, Andrew Jones wrote: > When the SUSP SBI extension is present it implies that the standard > "suspend to RAM" type is available. Wire it up to the generic > platform suspend support, also applying the already present support > for non-retentive CPU suspend. When the kernel is built with > CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend. > Resumption will occur when a platform-specific wake-up event arrives. > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > +static int __init sbi_system_suspend_init(void) > +{ > + if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) { Random thought I had reading this, was that it'll be possible to have a firmware that implements SBI < 2.0 that provides the SUSP extension. FWIW, I don't think that that is problematic, but maybe I am missing something that would make it so. Cheers, Conor.
On Thu, Oct 12, 2023 at 02:30:02PM +0100, Conor Dooley wrote: > Yo, > > On Thu, Oct 12, 2023 at 09:21:50AM +0200, Andrew Jones wrote: > > When the SUSP SBI extension is present it implies that the standard > > "suspend to RAM" type is available. Wire it up to the generic > > platform suspend support, also applying the already present support > > for non-retentive CPU suspend. When the kernel is built with > > CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend. > > Resumption will occur when a platform-specific wake-up event arrives. > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > > +static int __init sbi_system_suspend_init(void) > > +{ > > + if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) { > > Random thought I had reading this, was that it'll be possible to have a > firmware that implements SBI < 2.0 that provides the SUSP extension. > FWIW, I don't think that that is problematic, but maybe I am missing > something that would make it so. Hmm, next patch I look at is from Anup's debug console series, and he does check that the SBI implementation is at least version 2.0 before probing for the extension. We should probably have the same policy everywhere.
On Thu, Oct 12, 2023 at 02:30:02PM +0100, Conor Dooley wrote: > Yo, > > On Thu, Oct 12, 2023 at 09:21:50AM +0200, Andrew Jones wrote: > > When the SUSP SBI extension is present it implies that the standard > > "suspend to RAM" type is available. Wire it up to the generic > > platform suspend support, also applying the already present support > > for non-retentive CPU suspend. When the kernel is built with > > CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend. > > Resumption will occur when a platform-specific wake-up event arrives. > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > > +static int __init sbi_system_suspend_init(void) > > +{ > > + if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) { > > Random thought I had reading this, was that it'll be possible to have a > firmware that implements SBI < 2.0 that provides the SUSP extension. > FWIW, I don't think that that is problematic, but maybe I am missing > something that would make it so. Right, it shouldn't matter for SUSP. In fact, I sort of wish SBI was extension probing only (no version checks), but Anup tells me that PMU requires also checking the version to know which functions are available. Thanks, drew
On Thu, Oct 12, 2023 at 02:32:46PM +0100, Conor Dooley wrote: > On Thu, Oct 12, 2023 at 02:30:02PM +0100, Conor Dooley wrote: > > Yo, > > > > On Thu, Oct 12, 2023 at 09:21:50AM +0200, Andrew Jones wrote: > > > When the SUSP SBI extension is present it implies that the standard > > > "suspend to RAM" type is available. Wire it up to the generic > > > platform suspend support, also applying the already present support > > > for non-retentive CPU suspend. When the kernel is built with > > > CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend. > > > Resumption will occur when a platform-specific wake-up event arrives. > > > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > > > > +static int __init sbi_system_suspend_init(void) > > > +{ > > > + if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) { > > > > Random thought I had reading this, was that it'll be possible to have a > > firmware that implements SBI < 2.0 that provides the SUSP extension. > > FWIW, I don't think that that is problematic, but maybe I am missing > > something that would make it so. > > Hmm, next patch I look at is from Anup's debug console series, and he > does check that the SBI implementation is at least version 2.0 before > probing for the extension. We should probably have the same policy > everywhere. Yeah, the main reason I wrote in the other response that I'd prefer not to always check version when probing is because in most (I think all except PMU) cases it would only reduce the platforms where the extension can be used, but without any reason to do so. For example, right now QEMU bundles an OpenSBI v1.3.1 binary and that version supports both SUSP and DBCN, but, if we were to add SBI 2.0 checks in Linux for those extensions, then users will need to update their SBI implementations in order to use them. While requiring users to update their SBI implementations makes sense for new functionality or fixes, it seems like a lot to ask for them to just get a bigger number in their version check. (And then there's downstream SBI implementations which may end up cherry picking extensions, so their version numbers would be hard to define.) So my vote for Linux policy would be to only do version checks when necessary. And my preference for SBI would be to try and avoid specifying extensions which require clients to check versions. Thanks, drew
On Thu, Oct 12, 2023 at 9:31 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > On Thu, Oct 12, 2023 at 02:32:46PM +0100, Conor Dooley wrote: > > On Thu, Oct 12, 2023 at 02:30:02PM +0100, Conor Dooley wrote: > > > Yo, > > > > > > On Thu, Oct 12, 2023 at 09:21:50AM +0200, Andrew Jones wrote: > > > > When the SUSP SBI extension is present it implies that the standard > > > > "suspend to RAM" type is available. Wire it up to the generic > > > > platform suspend support, also applying the already present support > > > > for non-retentive CPU suspend. When the kernel is built with > > > > CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend. > > > > Resumption will occur when a platform-specific wake-up event arrives. > > > > > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > > > > > > +static int __init sbi_system_suspend_init(void) > > > > +{ > > > > + if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) { > > > > > > Random thought I had reading this, was that it'll be possible to have a > > > firmware that implements SBI < 2.0 that provides the SUSP extension. > > > FWIW, I don't think that that is problematic, but maybe I am missing > > > something that would make it so. > > > > Hmm, next patch I look at is from Anup's debug console series, and he > > does check that the SBI implementation is at least version 2.0 before > > probing for the extension. We should probably have the same policy > > everywhere. > > Yeah, the main reason I wrote in the other response that I'd prefer not > to always check version when probing is because in most (I think all > except PMU) cases it would only reduce the platforms where the extension > can be used, but without any reason to do so. For example, right now QEMU > bundles an OpenSBI v1.3.1 binary and that version supports both SUSP and > DBCN, but, if we were to add SBI 2.0 checks in Linux for those extensions, > then users will need to update their SBI implementations in order to use > them. While requiring users to update their SBI implementations makes > sense for new functionality or fixes, it seems like a lot to ask for them > to just get a bigger number in their version check. > > (And then there's downstream SBI implementations which may end up cherry > picking extensions, so their version numbers would be hard to define.) > > So my vote for Linux policy would be to only do version checks when > necessary. And my preference for SBI would be to try and avoid specifying > extensions which require clients to check versions. In addition to SBI PMU, even SBI STA and SBI NACL requires spec version check because these new SBI extensions use SBI_ERR_NO_SHMEM error code which is not available in SBI v1.0 (or lower). My recommendation is that SBI implementation should comply with the minimum SBI spec version required by the SBI extensions which it implements. Regards, Anup
On Thu, Oct 12, 2023 at 10:09:44PM +0530, Anup Patel wrote: > On Thu, Oct 12, 2023 at 9:31 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > On Thu, Oct 12, 2023 at 02:32:46PM +0100, Conor Dooley wrote: > > > On Thu, Oct 12, 2023 at 02:30:02PM +0100, Conor Dooley wrote: > > > > Yo, > > > > > > > > On Thu, Oct 12, 2023 at 09:21:50AM +0200, Andrew Jones wrote: > > > > > When the SUSP SBI extension is present it implies that the standard > > > > > "suspend to RAM" type is available. Wire it up to the generic > > > > > platform suspend support, also applying the already present support > > > > > for non-retentive CPU suspend. When the kernel is built with > > > > > CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend. > > > > > Resumption will occur when a platform-specific wake-up event arrives. > > > > > > > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > > > > > > > > +static int __init sbi_system_suspend_init(void) > > > > > +{ > > > > > + if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) { > > > > > > > > Random thought I had reading this, was that it'll be possible to have a > > > > firmware that implements SBI < 2.0 that provides the SUSP extension. > > > > FWIW, I don't think that that is problematic, but maybe I am missing > > > > something that would make it so. > > > > > > Hmm, next patch I look at is from Anup's debug console series, and he > > > does check that the SBI implementation is at least version 2.0 before > > > probing for the extension. We should probably have the same policy > > > everywhere. > > > > Yeah, the main reason I wrote in the other response that I'd prefer not > > to always check version when probing is because in most (I think all > > except PMU) cases it would only reduce the platforms where the extension > > can be used, but without any reason to do so. For example, right now QEMU > > bundles an OpenSBI v1.3.1 binary and that version supports both SUSP and > > DBCN, but, if we were to add SBI 2.0 checks in Linux for those extensions, > > then users will need to update their SBI implementations in order to use > > them. While requiring users to update their SBI implementations makes > > sense for new functionality or fixes, it seems like a lot to ask for them > > to just get a bigger number in their version check. > > > > (And then there's downstream SBI implementations which may end up cherry > > picking extensions, so their version numbers would be hard to define.) > > > > So my vote for Linux policy would be to only do version checks when > > necessary. And my preference for SBI would be to try and avoid specifying > > extensions which require clients to check versions. > > In addition to SBI PMU, even SBI STA and SBI NACL requires spec version > check because these new SBI extensions use SBI_ERR_NO_SHMEM > error code which is not available in SBI v1.0 (or lower). But when those extensions are present, they must implement their specifications, which means SBI_ERR_NO_SHMEM must also be present. From a client's perspective getting an affirmative that the extension is present is enough. Also checking the SBI version is basically turning the client into an SBI implementation sanity checker. > > My recommendation is that SBI implementation should comply with the > minimum SBI spec version required by the SBI extensions which it > implements. > I agree, but SBI implementation versions don't get bumped until after spec freezes, even though they incorporate everything needed for the new extensions and the extensions themselves. Linux can certainly choose to be strict about the SBI implementations from which it enables extensions, but I'm still not sure it's necessary. Thanks, drew
On 2023-10-12 2:21 AM, Andrew Jones wrote: > When the SUSP SBI extension is present it implies that the standard > "suspend to RAM" type is available. Wire it up to the generic > platform suspend support, also applying the already present support > for non-retentive CPU suspend. When the kernel is built with > CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend. > Resumption will occur when a platform-specific wake-up event arrives. > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > --- > arch/riscv/Kconfig | 5 ++++- > arch/riscv/include/asm/sbi.h | 9 ++++++++ > arch/riscv/kernel/suspend.c | 43 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 56 insertions(+), 1 deletion(-) Tested-by: Samuel Holland <samuel.holland@sifive.com>
On Thu, Oct 12, 2023 at 07:25:11PM +0200, Andrew Jones wrote: > On Thu, Oct 12, 2023 at 10:09:44PM +0530, Anup Patel wrote: > > On Thu, Oct 12, 2023 at 9:31 PM Andrew Jones <ajones@ventanamicro.com> wrote: > > > > > > On Thu, Oct 12, 2023 at 02:32:46PM +0100, Conor Dooley wrote: > > > > On Thu, Oct 12, 2023 at 02:30:02PM +0100, Conor Dooley wrote: > > > > > Yo, > > > > > > > > > > On Thu, Oct 12, 2023 at 09:21:50AM +0200, Andrew Jones wrote: > > > > > > When the SUSP SBI extension is present it implies that the standard > > > > > > "suspend to RAM" type is available. Wire it up to the generic > > > > > > platform suspend support, also applying the already present support > > > > > > for non-retentive CPU suspend. When the kernel is built with > > > > > > CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend. > > > > > > Resumption will occur when a platform-specific wake-up event arrives. > > > > > > > > > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com> > > > > > > > > > > > +static int __init sbi_system_suspend_init(void) > > > > > > +{ > > > > > > + if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) { > > > > > > > > > > Random thought I had reading this, was that it'll be possible to have a > > > > > firmware that implements SBI < 2.0 that provides the SUSP extension. > > > > > FWIW, I don't think that that is problematic, but maybe I am missing > > > > > something that would make it so. > > > > > > > > Hmm, next patch I look at is from Anup's debug console series, and he > > > > does check that the SBI implementation is at least version 2.0 before > > > > probing for the extension. We should probably have the same policy > > > > everywhere. > > > > > > Yeah, the main reason I wrote in the other response that I'd prefer not > > > to always check version when probing is because in most (I think all > > > except PMU) cases it would only reduce the platforms where the extension > > > can be used, but without any reason to do so. For example, right now QEMU > > > bundles an OpenSBI v1.3.1 binary and that version supports both SUSP and > > > DBCN, but, if we were to add SBI 2.0 checks in Linux for those extensions, > > > then users will need to update their SBI implementations in order to use > > > them. While requiring users to update their SBI implementations makes > > > sense for new functionality or fixes, it seems like a lot to ask for them > > > to just get a bigger number in their version check. > > > > > > (And then there's downstream SBI implementations which may end up cherry > > > picking extensions, so their version numbers would be hard to define.) > > > > > > So my vote for Linux policy would be to only do version checks when > > > necessary. And my preference for SBI would be to try and avoid specifying > > > extensions which require clients to check versions. > > > > In addition to SBI PMU, even SBI STA and SBI NACL requires spec version > > check because these new SBI extensions use SBI_ERR_NO_SHMEM > > error code which is not available in SBI v1.0 (or lower). > > But when those extensions are present, they must implement their > specifications, which means SBI_ERR_NO_SHMEM must also be present. > From a client's perspective getting an affirmative that the extension is > present is enough. Also checking the SBI version is basically turning the > client into an SBI implementation sanity checker. > > > > > My recommendation is that SBI implementation should comply with the > > minimum SBI spec version required by the SBI extensions which it > > implements. > > > > I agree, but SBI implementation versions don't get bumped until after > spec freezes, even though they incorporate everything needed for the > new extensions and the extensions themselves. > > Linux can certainly choose to be strict about the SBI implementations > from which it enables extensions, but I'm still not sure it's necessary. > I was just reviewing the DBCN series and was about to raise my thoughts there again about not being so strict about SBI implementation versions, but then I thought some more about it. I guess we should be strict about the version since it's otherwise not possible to any confidence that the extension advertised is the extension described in the frozen/ratified version of the spec (there could be implementations of draft spec versions which are not compatible with the final version and those will have the same extension ID). The most confidence Linux can have in an SBI extension's implementation being what it expects to be will come from both the extension's presence and the SBI implementation's version being at least as big as the version in which the extension was frozen. Long story short, I'll change the above condition to look for 2.0 and ask QEMU folks to put an OpenSBI binary which advertises 2.0 into its repo as soon as possible. Thanks, drew
Hey Drew, Anup, On Thu, Oct 19, 2023 at 10:36:50AM +0200, Andrew Jones wrote: > I was just reviewing the DBCN series and was about to raise my thoughts > there again about not being so strict about SBI implementation versions, > but then I thought some more about it. I guess we should be strict about > the version since it's otherwise not possible to any confidence that the > extension advertised is the extension described in the frozen/ratified > version of the spec (there could be implementations of draft spec versions > which are not compatible with the final version and those will have the > same extension ID). TBH, I don't think that having the version check is a sufficient test to stop things like that happening, but being insufficient does not make it not worthwhile. > The most confidence Linux can have in an SBI > extension's implementation being what it expects to be will come from > both the extension's presence and the SBI implementation's version > being at least as big as the version in which the extension was frozen. I am not sure what the status of the patches are on the OpenSBI side, but this is kinda how I feel about the Andestech PMU series. They're intending advertising having the PMU extension using the same interface as the standard PMU stuff - at least, that is the basis on which the kernel patches worked. The software doing the ecall to probe for support does really need to be able to trust that when it is told the extension is present that it is in fact the standard, or at least standard-compatible, extension. <20230906111455.4161641-1-peterlin@andestech.com> is the OpenSBI series implementing this and I voiced my objection to the kernel patches at https://lore.kernel.org/linux-riscv/20230911-worry-reformat-fbb5c473085d@wendy > Long story short, I'll change the above condition to look for 2.0 and > ask QEMU folks to put an OpenSBI binary which advertises 2.0 into its > repo as soon as possible. Cool, thanks. BTW Anup, do you think we could get the OpenSBI mailing lists archived on lore.kernel.org? Cheers, Conor.
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index d607ab0f7c6d..a96368b662d8 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -63,7 +63,7 @@ config RISCV select CLINT_TIMER if !MMU select CLONE_BACKWARDS select COMMON_CLK - select CPU_PM if CPU_IDLE || HIBERNATION + select CPU_PM if CPU_IDLE || HIBERNATION || SUSPEND select EDAC_SUPPORT select FRAME_POINTER if PERF_EVENTS || (FUNCTION_TRACER && !DYNAMIC_FTRACE) select GENERIC_ARCH_TOPOLOGY @@ -900,6 +900,9 @@ config PORTABLE select MMU select OF +config ARCH_SUSPEND_POSSIBLE + def_bool RISCV_SBI + menu "Power management options" source "kernel/power/Kconfig" diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h index 5b4a1bf5f439..808ec1cb1d0d 100644 --- a/arch/riscv/include/asm/sbi.h +++ b/arch/riscv/include/asm/sbi.h @@ -29,6 +29,7 @@ enum sbi_ext_id { SBI_EXT_RFENCE = 0x52464E43, SBI_EXT_HSM = 0x48534D, SBI_EXT_SRST = 0x53525354, + SBI_EXT_SUSP = 0x53555350, SBI_EXT_PMU = 0x504D55, /* Experimentals extensions must lie within this range */ @@ -113,6 +114,14 @@ enum sbi_srst_reset_reason { SBI_SRST_RESET_REASON_SYS_FAILURE, }; +enum sbi_ext_susp_fid { + SBI_EXT_SUSP_SYSTEM_SUSPEND = 0, +}; + +enum sbi_ext_susp_sleep_type { + SBI_SUSP_SLEEP_TYPE_SUSPEND_TO_RAM = 0, +}; + enum sbi_ext_pmu_fid { SBI_EXT_PMU_NUM_COUNTERS = 0, SBI_EXT_PMU_COUNTER_GET_INFO, diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c index 3c89b8ec69c4..0a0f4cf6dc58 100644 --- a/arch/riscv/kernel/suspend.c +++ b/arch/riscv/kernel/suspend.c @@ -4,8 +4,12 @@ * Copyright (c) 2022 Ventana Micro Systems Inc. */ +#define pr_fmt(fmt) "suspend: " fmt + #include <linux/ftrace.h> +#include <linux/suspend.h> #include <asm/csr.h> +#include <asm/sbi.h> #include <asm/suspend.h> void suspend_save_csrs(struct suspend_context *context) @@ -85,3 +89,42 @@ int cpu_suspend(unsigned long arg, return rc; } + +#ifdef CONFIG_RISCV_SBI +static int sbi_system_suspend(unsigned long sleep_type, + unsigned long resume_addr, + unsigned long opaque) +{ + struct sbiret ret; + + ret = sbi_ecall(SBI_EXT_SUSP, SBI_EXT_SUSP_SYSTEM_SUSPEND, + sleep_type, resume_addr, opaque, 0, 0, 0); + if (ret.error) + return sbi_err_map_linux_errno(ret.error); + + return ret.value; +} + +static int sbi_system_suspend_enter(suspend_state_t state) +{ + return cpu_suspend(SBI_SUSP_SLEEP_TYPE_SUSPEND_TO_RAM, sbi_system_suspend); +} + +static const struct platform_suspend_ops sbi_system_suspend_ops = { + .valid = suspend_valid_only_mem, + .enter = sbi_system_suspend_enter, +}; + +static int __init sbi_system_suspend_init(void) +{ + if (!sbi_spec_is_0_1() && sbi_probe_extension(SBI_EXT_SUSP) > 0) { + pr_info("SBI SUSP extension detected\n"); + if (IS_ENABLED(CONFIG_SUSPEND)) + suspend_set_ops(&sbi_system_suspend_ops); + } + + return 0; +} + +arch_initcall(sbi_system_suspend_init); +#endif /* CONFIG_RISCV_SBI */
When the SUSP SBI extension is present it implies that the standard "suspend to RAM" type is available. Wire it up to the generic platform suspend support, also applying the already present support for non-retentive CPU suspend. When the kernel is built with CONFIG_SUSPEND, one can do 'echo mem > /sys/power/state' to suspend. Resumption will occur when a platform-specific wake-up event arrives. Signed-off-by: Andrew Jones <ajones@ventanamicro.com> --- arch/riscv/Kconfig | 5 ++++- arch/riscv/include/asm/sbi.h | 9 ++++++++ arch/riscv/kernel/suspend.c | 43 ++++++++++++++++++++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-)