[PATCH -fixes v4 0/3] riscv: cbo.zero fixes

[PATCH -fixes v4 0/3] riscv: cbo.zero fixes

[Samuel Holland]

This series fixes a couple of issues related to using the cbo.zero
instruction in userspace. The first patch fixes a bug where the wrong
enable bit gets set if the kernel is running in M-mode. The remaining
patches fix a bug where the enable bit gets reset to its default value
after a nonretentive idle state. I have hardware which reproduces this:

Before this series:
  $ tools/testing/selftests/riscv/hwprobe/cbo
  TAP version 13
  1..3
  ok 1 Zicboz block size
  # Zicboz block size: 64
  Illegal instruction

After applying this series:
  $ tools/testing/selftests/riscv/hwprobe/cbo
  TAP version 13
  1..3
  ok 1 Zicboz block size
  # Zicboz block size: 64
  ok 2 cbo.zero
  ok 3 cbo.zero check
  # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0

Changes in v4:
 - Add a patch defining and setting the Xlinuxenvcfg ISA extension bit
 - Check for Xlinuxenvcfg instead of Zicboz

Changes in v3:
 - Drop patches added in v2
 - Check for Zicboz instead of the privileged ISA version

Changes in v2:
 - Add patches to allow parsing the privileged ISA version from the DT
 - Check for privileged ISA v1.12 instead of the specific CSR
 - Use riscv_has_extension_likely() instead of new ALTERNATIVE()s

Samuel Holland (3):
  riscv: Fix enabling cbo.zero when running in M-mode
  riscv: Add a custom ISA extension for the [ms]envcfg CSR
  riscv: Save/restore envcfg CSR during CPU suspend

 arch/riscv/include/asm/csr.h     |  2 ++
 arch/riscv/include/asm/hwcap.h   |  2 ++
 arch/riscv/include/asm/suspend.h |  1 +
 arch/riscv/kernel/cpufeature.c   | 16 +++++++++++++---
 arch/riscv/kernel/suspend.c      |  4 ++++
 5 files changed, 22 insertions(+), 3 deletions(-)

-- 
2.43.1

[PATCH -fixes v4 1/3] riscv: Fix enabling cbo.zero when running in M-mode

[Samuel Holland]

When the kernel is running in M-mode, the CBZE bit must be set in the
menvcfg CSR, not in senvcfg.

Cc: <stable@vger.kernel.org>
Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

(no changes since v1)

 arch/riscv/include/asm/csr.h   | 2 ++
 arch/riscv/kernel/cpufeature.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 510014051f5d..2468c55933cd 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -424,6 +424,7 @@
 # define CSR_STATUS	CSR_MSTATUS
 # define CSR_IE		CSR_MIE
 # define CSR_TVEC	CSR_MTVEC
+# define CSR_ENVCFG	CSR_MENVCFG
 # define CSR_SCRATCH	CSR_MSCRATCH
 # define CSR_EPC	CSR_MEPC
 # define CSR_CAUSE	CSR_MCAUSE
@@ -448,6 +449,7 @@
 # define CSR_STATUS	CSR_SSTATUS
 # define CSR_IE		CSR_SIE
 # define CSR_TVEC	CSR_STVEC
+# define CSR_ENVCFG	CSR_SENVCFG
 # define CSR_SCRATCH	CSR_SSCRATCH
 # define CSR_EPC	CSR_SEPC
 # define CSR_CAUSE	CSR_SCAUSE
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 89920f84d0a3..c5b13f7dd482 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -950,7 +950,7 @@ arch_initcall(check_unaligned_access_all_cpus);
 void riscv_user_isa_enable(void)
 {
 	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
-		csr_set(CSR_SENVCFG, ENVCFG_CBZE);
+		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
 }
 
 #ifdef CONFIG_RISCV_ALTERNATIVE
-- 
2.43.1

Re: [PATCH -fixes v4 1/3] riscv: Fix enabling cbo.zero when running in M-mode

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 462 bytes --]

On Tue, Feb 27, 2024 at 10:55:33PM -0800, Samuel Holland wrote:
> When the kernel is running in M-mode, the CBZE bit must be set in the
> menvcfg CSR, not in senvcfg.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

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

[PATCH -fixes v4 2/3] riscv: Add a custom ISA extension for the [ms]envcfg CSR

[Samuel Holland]

The [ms]envcfg CSR was added in version 1.12 of the RISC-V privileged
ISA (aka S[ms]1p12). However, bits in this CSR are defined by several
other extensions which may be implemented separately from any particular
version of the privileged ISA (for example, some unrelated errata may
prevent an implementation from claiming conformance with Ss1p12). As a
result, Linux cannot simply use the privileged ISA version to determine
if the CSR is present. It must also check if any of these other
extensions are implemented. It also cannot probe the existence of the
CSR at runtime, because Linux does not require Sstrict, so (in the
absence of additional information) it cannot know if a CSR at that
address is [ms]envcfg or part of some non-conforming vendor extension.

Since there are several standard extensions that imply the existence of
the [ms]envcfg CSR, it becomes unwieldy to check for all of them
wherever the CSR is accessed. Instead, define a custom Xlinuxenvcfg ISA
extension bit that is implied by the other extensions and denotes that
the CSR exists as defined in the privileged ISA, containing at least one
of the fields common between menvcfg and senvcfg.

This extension does not need to be parsed from the devicetree or ISA
string because it can only be implemented as a subset of some other
standard extension.

Cc: <stable@vger.kernel.org> # v6.7+
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v4:
 - New patch for v4

 arch/riscv/include/asm/hwcap.h |  2 ++
 arch/riscv/kernel/cpufeature.c | 14 ++++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 5340f818746b..1f2d2599c655 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -81,6 +81,8 @@
 #define RISCV_ISA_EXT_ZTSO		72
 #define RISCV_ISA_EXT_ZACAS		73
 
+#define RISCV_ISA_EXT_XLINUXENVCFG	127
+
 #define RISCV_ISA_EXT_MAX		128
 #define RISCV_ISA_EXT_INVALID		U32_MAX
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index c5b13f7dd482..dacffef68ce2 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -201,6 +201,16 @@ static const unsigned int riscv_zvbb_exts[] = {
 	RISCV_ISA_EXT_ZVKB
 };
 
+/*
+ * While the [ms]envcfg CSRs were not defined until version 1.12 of the RISC-V
+ * privileged ISA, the existence of the CSRs is implied by any extension which
+ * specifies [ms]envcfg bit(s). Hence, we define a custom ISA extension for the
+ * existence of the CSR, and treat it as a subset of those other extensions.
+ */
+static const unsigned int riscv_xlinuxenvcfg_exts[] = {
+	RISCV_ISA_EXT_XLINUXENVCFG
+};
+
 /*
  * The canonical order of ISA extension names in the ISA string is defined in
  * chapter 27 of the unprivileged specification.
@@ -250,8 +260,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
 	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
 	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
-	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
-	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
+	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
+	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
 	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
 	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
 	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
-- 
2.43.1

Re: [PATCH -fixes v4 2/3] riscv: Add a custom ISA extension for the [ms]envcfg CSR

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 4095 bytes --]

On Tue, Feb 27, 2024 at 10:55:34PM -0800, Samuel Holland wrote:
> The [ms]envcfg CSR was added in version 1.12 of the RISC-V privileged
> ISA (aka S[ms]1p12). However, bits in this CSR are defined by several
> other extensions which may be implemented separately from any particular
> version of the privileged ISA (for example, some unrelated errata may
> prevent an implementation from claiming conformance with Ss1p12). As a
> result, Linux cannot simply use the privileged ISA version to determine
> if the CSR is present. It must also check if any of these other
> extensions are implemented. It also cannot probe the existence of the
> CSR at runtime, because Linux does not require Sstrict, so (in the
> absence of additional information) it cannot know if a CSR at that
> address is [ms]envcfg or part of some non-conforming vendor extension.
> 
> Since there are several standard extensions that imply the existence of
> the [ms]envcfg CSR, it becomes unwieldy to check for all of them
> wherever the CSR is accessed. Instead, define a custom Xlinuxenvcfg ISA
> extension bit that is implied by the other extensions and denotes that
> the CSR exists as defined in the privileged ISA, containing at least one
> of the fields common between menvcfg and senvcfg.

> This extension does not need to be parsed from the devicetree or ISA
> string because it can only be implemented as a subset of some other
> standard extension.

NGL, every time I look at the superset stuff I question whether or not
it is a good implementation, but it is nice to see that it at least
makes the creation of quasi-extension flags like this straightforward.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.


> 
> Cc: <stable@vger.kernel.org> # v6.7+
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
> Changes in v4:
>  - New patch for v4
> 
>  arch/riscv/include/asm/hwcap.h |  2 ++
>  arch/riscv/kernel/cpufeature.c | 14 ++++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 5340f818746b..1f2d2599c655 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -81,6 +81,8 @@
>  #define RISCV_ISA_EXT_ZTSO		72
>  #define RISCV_ISA_EXT_ZACAS		73
>  
> +#define RISCV_ISA_EXT_XLINUXENVCFG	127
> +
>  #define RISCV_ISA_EXT_MAX		128
>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>  
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index c5b13f7dd482..dacffef68ce2 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -201,6 +201,16 @@ static const unsigned int riscv_zvbb_exts[] = {
>  	RISCV_ISA_EXT_ZVKB
>  };
>  
> +/*
> + * While the [ms]envcfg CSRs were not defined until version 1.12 of the RISC-V
> + * privileged ISA, the existence of the CSRs is implied by any extension which
> + * specifies [ms]envcfg bit(s). Hence, we define a custom ISA extension for the
> + * existence of the CSR, and treat it as a subset of those other extensions.
> + */
> +static const unsigned int riscv_xlinuxenvcfg_exts[] = {
> +	RISCV_ISA_EXT_XLINUXENVCFG
> +};
> +
>  /*
>   * The canonical order of ISA extension names in the ISA string is defined in
>   * chapter 27 of the unprivileged specification.
> @@ -250,8 +260,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>  	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
>  	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
>  	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
> -	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
> -	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
> +	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
> +	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
>  	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
>  	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
>  	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> -- 
> 2.43.1
> 

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

Re: [PATCH -fixes v4 2/3] riscv: Add a custom ISA extension for the [ms]envcfg CSR

[Palmer Dabbelt]

On Wed, 28 Feb 2024 02:12:14 PST (-0800), Conor Dooley wrote:
> On Tue, Feb 27, 2024 at 10:55:34PM -0800, Samuel Holland wrote:
>> The [ms]envcfg CSR was added in version 1.12 of the RISC-V privileged
>> ISA (aka S[ms]1p12). However, bits in this CSR are defined by several
>> other extensions which may be implemented separately from any particular
>> version of the privileged ISA (for example, some unrelated errata may
>> prevent an implementation from claiming conformance with Ss1p12). As a
>> result, Linux cannot simply use the privileged ISA version to determine
>> if the CSR is present. It must also check if any of these other
>> extensions are implemented. It also cannot probe the existence of the
>> CSR at runtime, because Linux does not require Sstrict, so (in the
>> absence of additional information) it cannot know if a CSR at that
>> address is [ms]envcfg or part of some non-conforming vendor extension.
>> 
>> Since there are several standard extensions that imply the existence of
>> the [ms]envcfg CSR, it becomes unwieldy to check for all of them
>> wherever the CSR is accessed. Instead, define a custom Xlinuxenvcfg ISA
>> extension bit that is implied by the other extensions and denotes that
>> the CSR exists as defined in the privileged ISA, containing at least one
>> of the fields common between menvcfg and senvcfg.
>
>> This extension does not need to be parsed from the devicetree or ISA
>> string because it can only be implemented as a subset of some other
>> standard extension.
>
> NGL, every time I look at the superset stuff I question whether or not
> it is a good implementation, but it is nice to see that it at least
> makes the creation of quasi-extension flags like this straightforward.

We can always add it to the DT list as a proper extension, but I think 
for this sort of stuff it's good enough for now -- we've already got a 
bunch of complexity for the proper ISA-defined extension dependencies, 
so it's not like we could really get away from it entirely.

> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> Cheers,
> Conor.
>
>
>> 
>> Cc: <stable@vger.kernel.org> # v6.7+
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>> 
>> Changes in v4:
>>  - New patch for v4
>> 
>>  arch/riscv/include/asm/hwcap.h |  2 ++
>>  arch/riscv/kernel/cpufeature.c | 14 ++++++++++++--
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>> index 5340f818746b..1f2d2599c655 100644
>> --- a/arch/riscv/include/asm/hwcap.h
>> +++ b/arch/riscv/include/asm/hwcap.h
>> @@ -81,6 +81,8 @@
>>  #define RISCV_ISA_EXT_ZTSO		72
>>  #define RISCV_ISA_EXT_ZACAS		73
>>  
>> +#define RISCV_ISA_EXT_XLINUXENVCFG	127
>> +
>>  #define RISCV_ISA_EXT_MAX		128
>>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>>  
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index c5b13f7dd482..dacffef68ce2 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -201,6 +201,16 @@ static const unsigned int riscv_zvbb_exts[] = {
>>  	RISCV_ISA_EXT_ZVKB
>>  };
>>  
>> +/*
>> + * While the [ms]envcfg CSRs were not defined until version 1.12 of the RISC-V
>> + * privileged ISA, the existence of the CSRs is implied by any extension which
>> + * specifies [ms]envcfg bit(s). Hence, we define a custom ISA extension for the
>> + * existence of the CSR, and treat it as a subset of those other extensions.
>> + */
>> +static const unsigned int riscv_xlinuxenvcfg_exts[] = {
>> +	RISCV_ISA_EXT_XLINUXENVCFG
>> +};
>> +
>>  /*
>>   * The canonical order of ISA extension names in the ISA string is defined in
>>   * chapter 27 of the unprivileged specification.
>> @@ -250,8 +260,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>>  	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
>>  	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
>>  	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
>> -	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
>> -	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
>> +	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
>> +	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
>>  	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
>>  	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
>>  	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
>> -- 
>> 2.43.1
>> 

Re: [PATCH -fixes v4 2/3] riscv: Add a custom ISA extension for the [ms]envcfg CSR

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2512 bytes --]

On Thu, Feb 29, 2024 at 10:23:39AM -0800, Palmer Dabbelt wrote:
> On Wed, 28 Feb 2024 02:12:14 PST (-0800), Conor Dooley wrote:
> > On Tue, Feb 27, 2024 at 10:55:34PM -0800, Samuel Holland wrote:
> > > The [ms]envcfg CSR was added in version 1.12 of the RISC-V privileged
> > > ISA (aka S[ms]1p12). However, bits in this CSR are defined by several
> > > other extensions which may be implemented separately from any particular
> > > version of the privileged ISA (for example, some unrelated errata may
> > > prevent an implementation from claiming conformance with Ss1p12). As a
> > > result, Linux cannot simply use the privileged ISA version to determine
> > > if the CSR is present. It must also check if any of these other
> > > extensions are implemented. It also cannot probe the existence of the
> > > CSR at runtime, because Linux does not require Sstrict, so (in the
> > > absence of additional information) it cannot know if a CSR at that
> > > address is [ms]envcfg or part of some non-conforming vendor extension.
> > > 
> > > Since there are several standard extensions that imply the existence of
> > > the [ms]envcfg CSR, it becomes unwieldy to check for all of them
> > > wherever the CSR is accessed. Instead, define a custom Xlinuxenvcfg ISA
> > > extension bit that is implied by the other extensions and denotes that
> > > the CSR exists as defined in the privileged ISA, containing at least one
> > > of the fields common between menvcfg and senvcfg.
> > 
> > > This extension does not need to be parsed from the devicetree or ISA
> > > string because it can only be implemented as a subset of some other
> > > standard extension.
> > 
> > NGL, every time I look at the superset stuff I question whether or not
> > it is a good implementation, but it is nice to see that it at least
> > makes the creation of quasi-extension flags like this straightforward.
> 
> We can always add it to the DT list as a proper extension, but I think for
> this sort of stuff it's good enough for now

Perhaps good enough forever. I was not advocating for adding it as a
permitted DT property - I was just saying that I didn't the complexity
that you mention below, but I was pleasantly surprised that the stuff
?Evan? and I came up with allows for this kind of inferred "extension"
without any changes.

> -- we've already got a bunch of
> complexity for the proper ISA-defined extension dependencies, so it's not
> like we could really get away from it entirely.

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

Re: [PATCH -fixes v4 2/3] riscv: Add a custom ISA extension for the [ms]envcfg CSR

[Palmer Dabbelt]

On Thu, 29 Feb 2024 10:30:10 PST (-0800), Conor Dooley wrote:
> On Thu, Feb 29, 2024 at 10:23:39AM -0800, Palmer Dabbelt wrote:
>> On Wed, 28 Feb 2024 02:12:14 PST (-0800), Conor Dooley wrote:
>> > On Tue, Feb 27, 2024 at 10:55:34PM -0800, Samuel Holland wrote:
>> > > The [ms]envcfg CSR was added in version 1.12 of the RISC-V privileged
>> > > ISA (aka S[ms]1p12). However, bits in this CSR are defined by several
>> > > other extensions which may be implemented separately from any particular
>> > > version of the privileged ISA (for example, some unrelated errata may
>> > > prevent an implementation from claiming conformance with Ss1p12). As a
>> > > result, Linux cannot simply use the privileged ISA version to determine
>> > > if the CSR is present. It must also check if any of these other
>> > > extensions are implemented. It also cannot probe the existence of the
>> > > CSR at runtime, because Linux does not require Sstrict, so (in the
>> > > absence of additional information) it cannot know if a CSR at that
>> > > address is [ms]envcfg or part of some non-conforming vendor extension.
>> > > 
>> > > Since there are several standard extensions that imply the existence of
>> > > the [ms]envcfg CSR, it becomes unwieldy to check for all of them
>> > > wherever the CSR is accessed. Instead, define a custom Xlinuxenvcfg ISA
>> > > extension bit that is implied by the other extensions and denotes that
>> > > the CSR exists as defined in the privileged ISA, containing at least one
>> > > of the fields common between menvcfg and senvcfg.
>> > 
>> > > This extension does not need to be parsed from the devicetree or ISA
>> > > string because it can only be implemented as a subset of some other
>> > > standard extension.
>> > 
>> > NGL, every time I look at the superset stuff I question whether or not
>> > it is a good implementation, but it is nice to see that it at least
>> > makes the creation of quasi-extension flags like this straightforward.
>> 
>> We can always add it to the DT list as a proper extension, but I think for
>> this sort of stuff it's good enough for now
>
> Perhaps good enough forever. I was not advocating for adding it as a
> permitted DT property - I was just saying that I didn't the complexity
> that you mention below, but I was pleasantly surprised that the stuff
> ?Evan? and I came up with allows for this kind of inferred "extension"
> without any changes.

Ya, I'm in the same boat.  I think we can get away without putting these 
into DT until we end up with something odd going on, like some other 
flavor of *envcf from some vendor being weird.

>> -- we've already got a bunch of
>> complexity for the proper ISA-defined extension dependencies, so it's not
>> like we could really get away from it entirely.

Re: [PATCH -fixes v4 2/3] riscv: Add a custom ISA extension for the [ms]envcfg CSR

[Andrew Jones]

On Tue, Feb 27, 2024 at 10:55:34PM -0800, Samuel Holland wrote:
> The [ms]envcfg CSR was added in version 1.12 of the RISC-V privileged
> ISA (aka S[ms]1p12). However, bits in this CSR are defined by several
> other extensions which may be implemented separately from any particular
> version of the privileged ISA (for example, some unrelated errata may
> prevent an implementation from claiming conformance with Ss1p12). As a
> result, Linux cannot simply use the privileged ISA version to determine
> if the CSR is present. It must also check if any of these other
> extensions are implemented. It also cannot probe the existence of the
> CSR at runtime, because Linux does not require Sstrict, so (in the
> absence of additional information) it cannot know if a CSR at that
> address is [ms]envcfg or part of some non-conforming vendor extension.
> 
> Since there are several standard extensions that imply the existence of
> the [ms]envcfg CSR, it becomes unwieldy to check for all of them
> wherever the CSR is accessed. Instead, define a custom Xlinuxenvcfg ISA
> extension bit that is implied by the other extensions and denotes that
> the CSR exists as defined in the privileged ISA, containing at least one
> of the fields common between menvcfg and senvcfg.
> 
> This extension does not need to be parsed from the devicetree or ISA
> string because it can only be implemented as a subset of some other
> standard extension.
> 
> Cc: <stable@vger.kernel.org> # v6.7+
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
> Changes in v4:
>  - New patch for v4
> 
>  arch/riscv/include/asm/hwcap.h |  2 ++
>  arch/riscv/kernel/cpufeature.c | 14 ++++++++++++--
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 5340f818746b..1f2d2599c655 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -81,6 +81,8 @@
>  #define RISCV_ISA_EXT_ZTSO		72
>  #define RISCV_ISA_EXT_ZACAS		73
>  
> +#define RISCV_ISA_EXT_XLINUXENVCFG	127

Since 128 is just the current max and will need to be bumped someday,
xlinuxenvcfg will end up in the middle of the list at some point anyway
(since bumping it too would be unnecessary churn). With that in mind,
I'd probably have just assigned it 74, but either way is fine by me.

> +
>  #define RISCV_ISA_EXT_MAX		128
>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>  
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index c5b13f7dd482..dacffef68ce2 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -201,6 +201,16 @@ static const unsigned int riscv_zvbb_exts[] = {
>  	RISCV_ISA_EXT_ZVKB
>  };
>  
> +/*
> + * While the [ms]envcfg CSRs were not defined until version 1.12 of the RISC-V
> + * privileged ISA, the existence of the CSRs is implied by any extension which
> + * specifies [ms]envcfg bit(s). Hence, we define a custom ISA extension for the
> + * existence of the CSR, and treat it as a subset of those other extensions.
> + */
> +static const unsigned int riscv_xlinuxenvcfg_exts[] = {
> +	RISCV_ISA_EXT_XLINUXENVCFG
> +};
> +
>  /*
>   * The canonical order of ISA extension names in the ISA string is defined in
>   * chapter 27 of the unprivileged specification.
> @@ -250,8 +260,8 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>  	__RISCV_ISA_EXT_DATA(c, RISCV_ISA_EXT_c),
>  	__RISCV_ISA_EXT_DATA(v, RISCV_ISA_EXT_v),
>  	__RISCV_ISA_EXT_DATA(h, RISCV_ISA_EXT_h),
> -	__RISCV_ISA_EXT_DATA(zicbom, RISCV_ISA_EXT_ZICBOM),
> -	__RISCV_ISA_EXT_DATA(zicboz, RISCV_ISA_EXT_ZICBOZ),
> +	__RISCV_ISA_EXT_SUPERSET(zicbom, RISCV_ISA_EXT_ZICBOM, riscv_xlinuxenvcfg_exts),
> +	__RISCV_ISA_EXT_SUPERSET(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts),
>  	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
>  	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
>  	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
> -- 
> 2.43.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH -fixes v4 3/3] riscv: Save/restore envcfg CSR during CPU suspend

[Samuel Holland]

The value of the [ms]envcfg CSR is lost when entering a nonretentive
idle state, so the CSR must be rewritten when resuming the CPU.

Cc: <stable@vger.kernel.org> # v6.7+
Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v4:
 - Check for Xlinuxenvcfg instead of Zicboz

Changes in v3:
 - Check for Zicboz instead of the privileged ISA version

Changes in v2:
 - Check for privileged ISA v1.12 instead of the specific CSR
 - Use riscv_has_extension_likely() instead of new ALTERNATIVE()s

 arch/riscv/include/asm/suspend.h | 1 +
 arch/riscv/kernel/suspend.c      | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
index 02f87867389a..491296a335d0 100644
--- a/arch/riscv/include/asm/suspend.h
+++ b/arch/riscv/include/asm/suspend.h
@@ -14,6 +14,7 @@ struct suspend_context {
 	struct pt_regs regs;
 	/* Saved and restored by high-level functions */
 	unsigned long scratch;
+	unsigned long envcfg;
 	unsigned long tvec;
 	unsigned long ie;
 #ifdef CONFIG_MMU
diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
index 239509367e42..299795341e8a 100644
--- a/arch/riscv/kernel/suspend.c
+++ b/arch/riscv/kernel/suspend.c
@@ -15,6 +15,8 @@
 void suspend_save_csrs(struct suspend_context *context)
 {
 	context->scratch = csr_read(CSR_SCRATCH);
+	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_XLINUXENVCFG))
+		context->envcfg = csr_read(CSR_ENVCFG);
 	context->tvec = csr_read(CSR_TVEC);
 	context->ie = csr_read(CSR_IE);
 
@@ -36,6 +38,8 @@ void suspend_save_csrs(struct suspend_context *context)
 void suspend_restore_csrs(struct suspend_context *context)
 {
 	csr_write(CSR_SCRATCH, context->scratch);
+	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_XLINUXENVCFG))
+		csr_write(CSR_ENVCFG, context->envcfg);
 	csr_write(CSR_TVEC, context->tvec);
 	csr_write(CSR_IE, context->ie);
 
-- 
2.43.1

Re: [PATCH -fixes v4 3/3] riscv: Save/restore envcfg CSR during CPU suspend

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 449 bytes --]

On Tue, Feb 27, 2024 at 10:55:35PM -0800, Samuel Holland wrote:
> The value of the [ms]envcfg CSR is lost when entering a nonretentive
> idle state, so the CSR must be rewritten when resuming the CPU.
> 
> Cc: <stable@vger.kernel.org> # v6.7+
> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

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

Re: [PATCH -fixes v4 3/3] riscv: Save/restore envcfg CSR during CPU suspend

[Andrew Jones]

On Tue, Feb 27, 2024 at 10:55:35PM -0800, Samuel Holland wrote:
> The value of the [ms]envcfg CSR is lost when entering a nonretentive
> idle state, so the CSR must be rewritten when resuming the CPU.
> 
> Cc: <stable@vger.kernel.org> # v6.7+
> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
> Changes in v4:
>  - Check for Xlinuxenvcfg instead of Zicboz
> 
> Changes in v3:
>  - Check for Zicboz instead of the privileged ISA version
> 
> Changes in v2:
>  - Check for privileged ISA v1.12 instead of the specific CSR
>  - Use riscv_has_extension_likely() instead of new ALTERNATIVE()s
> 
>  arch/riscv/include/asm/suspend.h | 1 +
>  arch/riscv/kernel/suspend.c      | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
> index 02f87867389a..491296a335d0 100644
> --- a/arch/riscv/include/asm/suspend.h
> +++ b/arch/riscv/include/asm/suspend.h
> @@ -14,6 +14,7 @@ struct suspend_context {
>  	struct pt_regs regs;
>  	/* Saved and restored by high-level functions */
>  	unsigned long scratch;
> +	unsigned long envcfg;
>  	unsigned long tvec;
>  	unsigned long ie;
>  #ifdef CONFIG_MMU
> diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
> index 239509367e42..299795341e8a 100644
> --- a/arch/riscv/kernel/suspend.c
> +++ b/arch/riscv/kernel/suspend.c
> @@ -15,6 +15,8 @@
>  void suspend_save_csrs(struct suspend_context *context)
>  {
>  	context->scratch = csr_read(CSR_SCRATCH);
> +	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_XLINUXENVCFG))
> +		context->envcfg = csr_read(CSR_ENVCFG);
>  	context->tvec = csr_read(CSR_TVEC);
>  	context->ie = csr_read(CSR_IE);
>  
> @@ -36,6 +38,8 @@ void suspend_save_csrs(struct suspend_context *context)
>  void suspend_restore_csrs(struct suspend_context *context)
>  {
>  	csr_write(CSR_SCRATCH, context->scratch);
> +	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_XLINUXENVCFG))
> +		csr_write(CSR_ENVCFG, context->envcfg);
>  	csr_write(CSR_TVEC, context->tvec);
>  	csr_write(CSR_IE, context->ie);
>  
> -- 
> 2.43.1
>

Picking _likely vs. _unlikely sometimes feels like flipping a coin, but
we'll presumably be increasing the likelihood of xlinuxenvcfg being
present as we add more and more envcfg using extensions, so maybe we
should use _likely here now, lest we forget to change it someday. But,
either way,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

Re: [PATCH -fixes v4 0/3] riscv: cbo.zero fixes

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Tue, 27 Feb 2024 22:55:32 -0800 you wrote:
> This series fixes a couple of issues related to using the cbo.zero
> instruction in userspace. The first patch fixes a bug where the wrong
> enable bit gets set if the kernel is running in M-mode. The remaining
> patches fix a bug where the enable bit gets reset to its default value
> after a nonretentive idle state. I have hardware which reproduces this:
> 
> Before this series:
>   $ tools/testing/selftests/riscv/hwprobe/cbo
>   TAP version 13
>   1..3
>   ok 1 Zicboz block size
>   # Zicboz block size: 64
>   Illegal instruction
> 
> [...]

Here is the summary with links:
  - [-fixes,v4,1/3] riscv: Fix enabling cbo.zero when running in M-mode
    https://git.kernel.org/riscv/c/3fb3f7164edc
  - [-fixes,v4,2/3] riscv: Add a custom ISA extension for the [ms]envcfg CSR
    https://git.kernel.org/riscv/c/4774848fef60
  - [-fixes,v4,3/3] riscv: Save/restore envcfg CSR during CPU suspend
    https://git.kernel.org/riscv/c/05ab803d1ad8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html