[PATCH v1 0/1] riscv: Introduce system suspend support

[PATCH v1 0/1] riscv: Introduce system suspend support

[Andrew Jones]

OpenSBI v1.3 and later supports the SUSP SBI extension which has recently
been frozen with the freezing of SBI 2.0. This one patch series adds
system suspend support to Linux, which implements "suspend-to-RAM". To
use it, build the kernel with CONFIG_SUSPEND, boot on a platform which
supports system suspend, and then issue 'echo mem > /sys/power/state'.
It's also possible to test this Linux support on a platform without
system suspend by using OpenSBI's system suspend test support. To enable
test support add

      opensbi-domains {
          compatible = "opensbi,domain,config";
          system-suspend-test;
      };

to the chosen node of the device tree. With the test node present,
OpenSBI will wait 5 seconds on a suspend and then kick a resume.

Note: Resume may fail without the fix for probing misaligned access
speed[1]. It's best to apply that fix before attempting system suspend.

[1] https://lore.kernel.org/all/20230920193801.3035093-1-evan@rivosinc.com/

Changes for v1:
  - Rebase on v6.6-rc1 -- only minor Kconfig change needed

Changes for RFC-v2:
  - RISCV_SBI dependency [Conor]
  - Rename SBI_EXT_SUSP_SUSPEND to SBI_EXT_SUSP_SYSTEM_SUSPEND and
    SBI_SUSP_SLEEP_TYPE_SUSPEND to SBI_SUSP_SLEEP_TYPE_SUSPEND_TO_RAM [Ley Foon]

Andrew Jones (1):
  riscv: sbi: Introduce system suspend support

 arch/riscv/Kconfig           |  5 ++++-
 arch/riscv/include/asm/sbi.h |  9 ++++++++
 arch/riscv/kernel/suspend.c  | 43 ++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)

-- 
2.41.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Andrew Jones]

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(-)

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 */
-- 
2.41.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 947 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1256 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Andrew Jones]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Andrew Jones]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Anup Patel]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Andrew Jones]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Samuel Holland]

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>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Andrew Jones]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 1/1] riscv: sbi: Introduce system suspend support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2083 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/1] riscv: Introduce system suspend support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1400 bytes --]

On Thu, Oct 12, 2023 at 09:21:49AM +0200, Andrew Jones wrote:
> OpenSBI v1.3 and later supports the SUSP SBI extension which has recently
> been frozen with the freezing of SBI 2.0. This one patch series adds
> system suspend support to Linux, which implements "suspend-to-RAM". To
> use it, build the kernel with CONFIG_SUSPEND, boot on a platform which
> supports system suspend, and then issue 'echo mem > /sys/power/state'.
> It's also possible to test this Linux support on a platform without
> system suspend by using OpenSBI's system suspend test support. To enable
> test support add
> 
>       opensbi-domains {
>           compatible = "opensbi,domain,config";
>           system-suspend-test;
>       };
> 
> to the chosen node of the device tree. With the test node present,
> OpenSBI will wait 5 seconds on a suspend and then kick a resume.
> 
> Note: Resume may fail without the fix for probing misaligned access
> speed[1]. It's best to apply that fix before attempting system suspend.
> 
> [1] https://lore.kernel.org/all/20230920193801.3035093-1-evan@rivosinc.com/
> 
> Changes for v1:
>   - Rebase on v6.6-rc1 -- only minor Kconfig change needed

Would you mind resending this rebased on -rc1 again? It looks as if the
patchwork CI threw a fit do to differing expectations about function
signatures for the misaligned access probing.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1 0/1] riscv: Introduce system suspend support

[Andrew Jones]

On Mon, Nov 27, 2023 at 10:52:52AM +0000, Conor Dooley wrote:
> On Thu, Oct 12, 2023 at 09:21:49AM +0200, Andrew Jones wrote:
> > OpenSBI v1.3 and later supports the SUSP SBI extension which has recently
> > been frozen with the freezing of SBI 2.0. This one patch series adds
> > system suspend support to Linux, which implements "suspend-to-RAM". To
> > use it, build the kernel with CONFIG_SUSPEND, boot on a platform which
> > supports system suspend, and then issue 'echo mem > /sys/power/state'.
> > It's also possible to test this Linux support on a platform without
> > system suspend by using OpenSBI's system suspend test support. To enable
> > test support add
> > 
> >       opensbi-domains {
> >           compatible = "opensbi,domain,config";
> >           system-suspend-test;
> >       };
> > 
> > to the chosen node of the device tree. With the test node present,
> > OpenSBI will wait 5 seconds on a suspend and then kick a resume.
> > 
> > Note: Resume may fail without the fix for probing misaligned access
> > speed[1]. It's best to apply that fix before attempting system suspend.
> > 
> > [1] https://lore.kernel.org/all/20230920193801.3035093-1-evan@rivosinc.com/
> > 
> > Changes for v1:
> >   - Rebase on v6.6-rc1 -- only minor Kconfig change needed
> 
> Would you mind resending this rebased on -rc1 again? It looks as if the
> patchwork CI threw a fit do to differing expectations about function
> signatures for the misaligned access probing.

Thanks for the poke. I need to repost anyway with the improved condition,
checking the SBI version. I'll try to get to this today.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv