[PATCH v2 0/6] riscv: Apply Zawrs when available

[PATCH v2 0/6] riscv: Apply Zawrs when available

[Andrew Jones]

Zawrs provides two instructions (wrs.nto and wrs.sto), where both are
meant to allow the hart to enter a low-power state while waiting on a
store to a memory location. The instructions also both wait an
implementation-defined "short" duration (unless the implementation
terminates the stall for another reason). The difference is that while
wrs.sto will terminate when the duration elapses, wrs.nto, depending on
configuration, will either just keep waiting or an ILL exception will be
raised. Linux will use wrs.nto, so if platforms have an implementation
which falls in the "just keep waiting" category, then it should _not_
advertise Zawrs in the hardware description. Text to that effect has
been added to the Zawrs DT definition.

Like wfi (and with the same {m,h}status bits to configure it), when
wrs.nto is configured to raise exceptions it's expected that the higher
privilege level will see the instruction was a wait instruction, do
something, and then resume execution following the instruction. For
example, KVM does configure exceptions for wfi (hstatus.VTW=1) and
therefore also for wrs.nto. KVM does this for wfi since it's better to
allow other tasks to be scheduled while a VCPU waits for an interrupt.
For waits such as those where wrs.nto/sto would be used, which are
typically locks, it is also a good idea for KVM to be involved, as it
can attempt to schedule the lock holding VCPU.

This series starts with Christoph's addition of riscv smp_cond_load*
functions which apply wrs.sto when available. That patch has been
reworked to use wrs.nto and to use the same approach as Arm for the
wait loop, since we can't have arbitrary C code between the load-
reserved and the wrs. Then, hwprobe support is added (since the
instructions are also usable from usermode), and finally KVM is
taught about wrs.nto, allowing guests to see and use the Zawrs
extension.

We still don't have test results from hardware, and it's not possible to
prove that using Zawrs is a win when testing on QEMU, not even when
oversubscribing VCPUs to guests. However, it is possible to use KVM
selftests to force a scenario where we can prove Zawrs does its job and
does it well. [4] is a test which does this and, on my machine, without
Zawrs it takes 16 seconds to complete and with Zawrs it takes 0.25
seconds.

This series is also available here [1]. In order to use QEMU for testing
a build with [2] is needed. In order to enable guests to use Zawrs with
KVM using kvmtool, the branch at [3] may be used.

[1] https://github.com/jones-drew/linux/commits/riscv/zawrs-v2/
[2] https://lore.kernel.org/all/20240312152901.512001-2-ajones@ventanamicro.com/
[3] https://github.com/jones-drew/kvmtool/commits/riscv/zawrs/
[4] https://github.com/jones-drew/linux/commit/9311702bcd118bdbfa8b9be4a8ec355c40559499

Thanks,
drew

v2:
 - Added DT bindings patch with additional Linux specifications due
   to wrs.nto potentially never terminating, as suggested by Palmer
 - Added patch to share pause insn definition
 - Rework main Zawrs support patch to use Arm approach (which is
   also the approach that Andrea Parri suggested)
 - Dropped the riscv implementation of smp_cond_load_acquire().
   afaict, the generic implementation, which will use the riscv
   implementation of smp_cond_load_relaxed() is sufficient for riscv.
 - The rework was large enough (IMO) to drop Heiko's s-o-b and to
   add myself as a co-developer


Andrew Jones (5):
  riscv: Provide a definition for 'pause'
  dt-bindings: riscv: Add Zawrs ISA extension description
  riscv: hwprobe: export Zawrs ISA extension
  KVM: riscv: Support guest wrs.nto
  KVM: riscv: selftests: Add Zawrs extension to get-reg-list test

Christoph Müllner (1):
  riscv: Add Zawrs support for spinlocks

 Documentation/arch/riscv/hwprobe.rst          |  4 ++
 .../devicetree/bindings/riscv/extensions.yaml | 12 +++++
 arch/riscv/Kconfig                            | 20 +++++---
 arch/riscv/Makefile                           |  3 --
 arch/riscv/include/asm/barrier.h              | 45 ++++++++++------
 arch/riscv/include/asm/cmpxchg.h              | 51 +++++++++++++++++++
 arch/riscv/include/asm/hwcap.h                |  1 +
 arch/riscv/include/asm/insn-def.h             |  4 ++
 arch/riscv/include/asm/kvm_host.h             |  1 +
 arch/riscv/include/asm/vdso/processor.h       |  8 +--
 arch/riscv/include/uapi/asm/hwprobe.h         |  1 +
 arch/riscv/include/uapi/asm/kvm.h             |  1 +
 arch/riscv/kernel/cpufeature.c                |  1 +
 arch/riscv/kernel/sys_hwprobe.c               |  1 +
 arch/riscv/kvm/vcpu.c                         |  1 +
 arch/riscv/kvm/vcpu_insn.c                    | 15 ++++++
 arch/riscv/kvm/vcpu_onereg.c                  |  2 +
 .../selftests/kvm/riscv/get-reg-list.c        |  4 ++
 18 files changed, 144 insertions(+), 31 deletions(-)

-- 
2.44.0

[PATCH v2 1/6] riscv: Provide a definition for 'pause'

[Andrew Jones]

If we're going to provide the encoding for 'pause' in cpu_relax()
anyway, then we can drop the toolchain checks and just always use
it. The advantage of doing this is that other code that need
pause don't need to also define it (yes, another use is coming).
Add the definition to insn-def.h since it's an instruction
definition and also because insn-def.h doesn't include much, so
it's safe to include from asm/vdso/processor.h without concern for
circular dependencies.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/Kconfig                      | 7 -------
 arch/riscv/Makefile                     | 3 ---
 arch/riscv/include/asm/insn-def.h       | 2 ++
 arch/riscv/include/asm/vdso/processor.h | 8 ++------
 4 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index be09c8836d56..7427d8088337 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -639,13 +639,6 @@ config RISCV_ISA_ZICBOZ
 
 	   If you don't know what to do here, say Y.
 
-config TOOLCHAIN_HAS_ZIHINTPAUSE
-	bool
-	default y
-	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zihintpause)
-	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zihintpause)
-	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23600
-
 config TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI
 	def_bool y
 	# https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=aed44286efa8ae8717a77d94b51ac3614e2ca6dc
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 252d63942f34..f1792ac03335 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -75,9 +75,6 @@ else
 riscv-march-$(CONFIG_TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI) := $(riscv-march-y)_zicsr_zifencei
 endif
 
-# Check if the toolchain supports Zihintpause extension
-riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause
-
 # Remove F,D,V from isa string for all. Keep extensions between "fd" and "v" by
 # matching non-v and non-multi-letter extensions out with the filter ([^v_]*)
 KBUILD_CFLAGS += -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)fd([^v_]*)v?/\1\2/')
diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
index e27179b26086..64dffaa21bfa 100644
--- a/arch/riscv/include/asm/insn-def.h
+++ b/arch/riscv/include/asm/insn-def.h
@@ -196,4 +196,6 @@
 	INSN_I(OPCODE_MISC_MEM, FUNC3(2), __RD(0),		\
 	       RS1(base), SIMM12(4))
 
+#define RISCV_PAUSE	".4byte 0x100000f"
+
 #endif /* __ASM_INSN_DEF_H */
diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
index 96b65a5396df..8f383f05a290 100644
--- a/arch/riscv/include/asm/vdso/processor.h
+++ b/arch/riscv/include/asm/vdso/processor.h
@@ -5,6 +5,7 @@
 #ifndef __ASSEMBLY__
 
 #include <asm/barrier.h>
+#include <asm/insn-def.h>
 
 static inline void cpu_relax(void)
 {
@@ -14,16 +15,11 @@ static inline void cpu_relax(void)
 	__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
 #endif
 
-#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
 	/*
 	 * Reduce instruction retirement.
 	 * This assumes the PC changes.
 	 */
-	__asm__ __volatile__ ("pause");
-#else
-	/* Encoding of the pause instruction */
-	__asm__ __volatile__ (".4byte 0x100000F");
-#endif
+	__asm__ __volatile__ (RISCV_PAUSE);
 	barrier();
 }
 
-- 
2.44.0

[PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Andrew Jones]

Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension
which was ratified in commit 98918c844281 of riscv-isa-manual.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 .../devicetree/bindings/riscv/extensions.yaml        | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
index 468c646247aa..584da2f539e5 100644
--- a/Documentation/devicetree/bindings/riscv/extensions.yaml
+++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
@@ -177,6 +177,18 @@ properties:
             is supported as ratified at commit 5059e0ca641c ("update to
             ratified") of the riscv-zacas.
 
+        - const: zawrs
+          description: |
+            The Zawrs extension for entering a low-power state or for trapping
+            to a hypervisor while waiting on a store to a memory location, as
+            ratified in commit 98918c844281 ("Merge pull request #1217 from
+            riscv/zawrs") of riscv-isa-manual. Linux assumes that WRS.NTO will
+            either always eventually terminate the stall due to the reservation
+            set becoming invalid, implementation-specific other reasons, or
+            because a higher privilege level has configured it to cause an
+            illegal instruction exception after an implementation-specific
+            bounded time limit.
+
         - const: zba
           description: |
             The standard Zba bit-manipulation extension for address generation
-- 
2.44.0

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Conor Dooley]

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

On Fri, Apr 19, 2024 at 03:53:24PM +0200, Andrew Jones wrote:
> Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension
> which was ratified in commit 98918c844281 of riscv-isa-manual.
> 
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  .../devicetree/bindings/riscv/extensions.yaml        | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> index 468c646247aa..584da2f539e5 100644
> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> @@ -177,6 +177,18 @@ properties:
>              is supported as ratified at commit 5059e0ca641c ("update to
>              ratified") of the riscv-zacas.
>  
> +        - const: zawrs
> +          description: |
> +            The Zawrs extension for entering a low-power state or for trapping
> +            to a hypervisor while waiting on a store to a memory location, as
> +            ratified in commit 98918c844281 ("Merge pull request #1217 from
> +            riscv/zawrs") of riscv-isa-manual.

This part is fine...


> Linux assumes that WRS.NTO will
> +            either always eventually terminate the stall due to the reservation
> +            set becoming invalid, implementation-specific other reasons, or
> +            because a higher privilege level has configured it to cause an
> +            illegal instruction exception after an implementation-specific
> +            bounded time limit.

...but I don't like this bit. The binding should just describe what the
property means for the hardware, not discuss specifics about a
particular OS.

And with my dt-bindings hat off and my kernel hat on, I think that if we
want to have more specific requirements than the extension provides we
either need to a) document that zawrs means that it will always
terminate or b) additionally document a "zawrs-always-terminates" that
has that meaning and look for it to enable the behaviour.

Documenting something and immediately turning around and saying "this
isn't sufficient, let's assume it means more than it does" just seems
like we should make firmware tell us exactly what we want.

Cheers,
Conor.

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

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Andrew Jones]

On Fri, Apr 19, 2024 at 03:45:46PM +0100, Conor Dooley wrote:
> On Fri, Apr 19, 2024 at 03:53:24PM +0200, Andrew Jones wrote:
> > Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension
> > which was ratified in commit 98918c844281 of riscv-isa-manual.
> > 
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  .../devicetree/bindings/riscv/extensions.yaml        | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > index 468c646247aa..584da2f539e5 100644
> > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > @@ -177,6 +177,18 @@ properties:
> >              is supported as ratified at commit 5059e0ca641c ("update to
> >              ratified") of the riscv-zacas.
> >  
> > +        - const: zawrs
> > +          description: |
> > +            The Zawrs extension for entering a low-power state or for trapping
> > +            to a hypervisor while waiting on a store to a memory location, as
> > +            ratified in commit 98918c844281 ("Merge pull request #1217 from
> > +            riscv/zawrs") of riscv-isa-manual.
> 
> This part is fine...
> 
> 
> > Linux assumes that WRS.NTO will
> > +            either always eventually terminate the stall due to the reservation
> > +            set becoming invalid, implementation-specific other reasons, or
> > +            because a higher privilege level has configured it to cause an
> > +            illegal instruction exception after an implementation-specific
> > +            bounded time limit.
> 
> ...but I don't like this bit. The binding should just describe what the
> property means for the hardware, not discuss specifics about a
> particular OS.
> 
> And with my dt-bindings hat off and my kernel hat on, I think that if we
> want to have more specific requirements than the extension provides we
> either need to a) document that zawrs means that it will always
> terminate or b) additionally document a "zawrs-always-terminates" that
> has that meaning and look for it to enable the behaviour.

IIUC, the text above mostly just needs to remove 'Linux assumes' in order
to provide what we want for (a)? I'm not sure about (b). If Zawrs is
unusable as is, then we should probably just go back to the specs and get
a new standard extension name for a new version which includes the changes
we need.

Thanks,
drew

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Conor Dooley]

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

On Fri, Apr 19, 2024 at 05:16:05PM +0200, Andrew Jones wrote:
> On Fri, Apr 19, 2024 at 03:45:46PM +0100, Conor Dooley wrote:
> > On Fri, Apr 19, 2024 at 03:53:24PM +0200, Andrew Jones wrote:
> > > Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension
> > > which was ratified in commit 98918c844281 of riscv-isa-manual.
> > > 
> > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > ---
> > >  .../devicetree/bindings/riscv/extensions.yaml        | 12 ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > index 468c646247aa..584da2f539e5 100644
> > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > @@ -177,6 +177,18 @@ properties:
> > >              is supported as ratified at commit 5059e0ca641c ("update to
> > >              ratified") of the riscv-zacas.
> > >  
> > > +        - const: zawrs
> > > +          description: |
> > > +            The Zawrs extension for entering a low-power state or for trapping
> > > +            to a hypervisor while waiting on a store to a memory location, as
> > > +            ratified in commit 98918c844281 ("Merge pull request #1217 from
> > > +            riscv/zawrs") of riscv-isa-manual.
> > 
> > This part is fine...
> > 
> > 
> > > Linux assumes that WRS.NTO will
> > > +            either always eventually terminate the stall due to the reservation
> > > +            set becoming invalid, implementation-specific other reasons, or
> > > +            because a higher privilege level has configured it to cause an
> > > +            illegal instruction exception after an implementation-specific
> > > +            bounded time limit.
> > 
> > ...but I don't like this bit. The binding should just describe what the
> > property means for the hardware, not discuss specifics about a
> > particular OS.
> > 
> > And with my dt-bindings hat off and my kernel hat on, I think that if we
> > want to have more specific requirements than the extension provides we
> > either need to a) document that zawrs means that it will always
> > terminate or b) additionally document a "zawrs-always-terminates" that
> > has that meaning and look for it to enable the behaviour.
> 
> IIUC, the text above mostly just needs to remove 'Linux assumes' in order
> to provide what we want for (a)? I'm not sure about (b). If Zawrs is
> unusable as is, then we should probably just go back to the specs and get
> a new standard extension name for a new version which includes the changes
> we need.

An (official) new name for the behaviour that you actually want, especially
if the patchset sent the other day does not have the more stringent
requirement (I won't even pretend to understand Zawrs well enough to know
whether it does or not), sounds like the ideal outcome. That way you're
also sorted on the ACPI side.

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

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Charlie Jenkins]

On Fri, Apr 19, 2024 at 04:19:52PM +0100, Conor Dooley wrote:
> On Fri, Apr 19, 2024 at 05:16:05PM +0200, Andrew Jones wrote:
> > On Fri, Apr 19, 2024 at 03:45:46PM +0100, Conor Dooley wrote:
> > > On Fri, Apr 19, 2024 at 03:53:24PM +0200, Andrew Jones wrote:
> > > > Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension
> > > > which was ratified in commit 98918c844281 of riscv-isa-manual.
> > > > 
> > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > > ---
> > > >  .../devicetree/bindings/riscv/extensions.yaml        | 12 ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > index 468c646247aa..584da2f539e5 100644
> > > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > @@ -177,6 +177,18 @@ properties:
> > > >              is supported as ratified at commit 5059e0ca641c ("update to
> > > >              ratified") of the riscv-zacas.
> > > >  
> > > > +        - const: zawrs
> > > > +          description: |
> > > > +            The Zawrs extension for entering a low-power state or for trapping
> > > > +            to a hypervisor while waiting on a store to a memory location, as
> > > > +            ratified in commit 98918c844281 ("Merge pull request #1217 from
> > > > +            riscv/zawrs") of riscv-isa-manual.
> > > 
> > > This part is fine...
> > > 
> > > 
> > > > Linux assumes that WRS.NTO will
> > > > +            either always eventually terminate the stall due to the reservation
> > > > +            set becoming invalid, implementation-specific other reasons, or
> > > > +            because a higher privilege level has configured it to cause an
> > > > +            illegal instruction exception after an implementation-specific
> > > > +            bounded time limit.
> > > 
> > > ...but I don't like this bit. The binding should just describe what the
> > > property means for the hardware, not discuss specifics about a
> > > particular OS.
> > > 
> > > And with my dt-bindings hat off and my kernel hat on, I think that if we
> > > want to have more specific requirements than the extension provides we
> > > either need to a) document that zawrs means that it will always
> > > terminate or b) additionally document a "zawrs-always-terminates" that
> > > has that meaning and look for it to enable the behaviour.
> > 
> > IIUC, the text above mostly just needs to remove 'Linux assumes' in order
> > to provide what we want for (a)? I'm not sure about (b). If Zawrs is
> > unusable as is, then we should probably just go back to the specs and get
> > a new standard extension name for a new version which includes the changes
> > we need.
> 
> An (official) new name for the behaviour that you actually want, especially
> if the patchset sent the other day does not have the more stringent
> requirement (I won't even pretend to understand Zawrs well enough to know
> whether it does or not), sounds like the ideal outcome. That way you're
> also sorted on the ACPI side.

What would be the purpose of a vendor implementing WRS.NTO (and putting
it in the DT) that never terminates? The spec says "Then a subsequent
WRS.NTO instruction would cause the hart to temporarily stall execution
in a low- power state until a store occurs to the reservation set or an
interrupt is observed." Why is this wording for WRS.NTO not sufficient
to assume that an implementation of this instruction would eventually
terminate?

- Charlie

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Andrew Jones]

On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
> On Fri, Apr 19, 2024 at 04:19:52PM +0100, Conor Dooley wrote:
> > On Fri, Apr 19, 2024 at 05:16:05PM +0200, Andrew Jones wrote:
> > > On Fri, Apr 19, 2024 at 03:45:46PM +0100, Conor Dooley wrote:
> > > > On Fri, Apr 19, 2024 at 03:53:24PM +0200, Andrew Jones wrote:
> > > > > Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension
> > > > > which was ratified in commit 98918c844281 of riscv-isa-manual.
> > > > > 
> > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > > > ---
> > > > >  .../devicetree/bindings/riscv/extensions.yaml        | 12 ++++++++++++
> > > > >  1 file changed, 12 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > index 468c646247aa..584da2f539e5 100644
> > > > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > @@ -177,6 +177,18 @@ properties:
> > > > >              is supported as ratified at commit 5059e0ca641c ("update to
> > > > >              ratified") of the riscv-zacas.
> > > > >  
> > > > > +        - const: zawrs
> > > > > +          description: |
> > > > > +            The Zawrs extension for entering a low-power state or for trapping
> > > > > +            to a hypervisor while waiting on a store to a memory location, as
> > > > > +            ratified in commit 98918c844281 ("Merge pull request #1217 from
> > > > > +            riscv/zawrs") of riscv-isa-manual.
> > > > 
> > > > This part is fine...
> > > > 
> > > > 
> > > > > Linux assumes that WRS.NTO will
> > > > > +            either always eventually terminate the stall due to the reservation
> > > > > +            set becoming invalid, implementation-specific other reasons, or
> > > > > +            because a higher privilege level has configured it to cause an
> > > > > +            illegal instruction exception after an implementation-specific
> > > > > +            bounded time limit.
> > > > 
> > > > ...but I don't like this bit. The binding should just describe what the
> > > > property means for the hardware, not discuss specifics about a
> > > > particular OS.
> > > > 
> > > > And with my dt-bindings hat off and my kernel hat on, I think that if we
> > > > want to have more specific requirements than the extension provides we
> > > > either need to a) document that zawrs means that it will always
> > > > terminate or b) additionally document a "zawrs-always-terminates" that
> > > > has that meaning and look for it to enable the behaviour.
> > > 
> > > IIUC, the text above mostly just needs to remove 'Linux assumes' in order
> > > to provide what we want for (a)? I'm not sure about (b). If Zawrs is
> > > unusable as is, then we should probably just go back to the specs and get
> > > a new standard extension name for a new version which includes the changes
> > > we need.
> > 
> > An (official) new name for the behaviour that you actually want, especially
> > if the patchset sent the other day does not have the more stringent
> > requirement (I won't even pretend to understand Zawrs well enough to know
> > whether it does or not), sounds like the ideal outcome. That way you're
> > also sorted on the ACPI side.
> 
> What would be the purpose of a vendor implementing WRS.NTO (and putting
> it in the DT) that never terminates? The spec says "Then a subsequent
> WRS.NTO instruction would cause the hart to temporarily stall execution
> in a low- power state until a store occurs to the reservation set or an
> interrupt is observed." Why is this wording for WRS.NTO not sufficient
> to assume that an implementation of this instruction would eventually
> terminate?
>

We can invoke smp_cond_load_relaxed(addr, VAL || anything_we_want()). This
means we may not expect VAL ever to be written, which rules out "until a
store occurs". As for "an interrupt is observed", we don't know which one
to expect to arrive within a "reasonable" amount of time. We need to know
which one(s), since, while wrs.nto will terminate even when interrupts are
globally disabled, we still need to have the interrupt(s) we expect to be
locally enabled. And, the interrupts should arrive in a "reasonable"
amount of time since we want to poll anything_we_want() at a "reasonable"
frequency.

So, we need firmware to promise to enable exceptions if there aren't any
such interrupts. Or, we could require hardware descriptions to identify
which interrupt(s) would be good to have enabled before calling wrs.nto.
Maybe there's already some way to describe something like that?

Thanks,
drew

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Charlie Jenkins]

On Sun, Apr 21, 2024 at 12:20:03PM +0200, Andrew Jones wrote:
> On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
> > On Fri, Apr 19, 2024 at 04:19:52PM +0100, Conor Dooley wrote:
> > > On Fri, Apr 19, 2024 at 05:16:05PM +0200, Andrew Jones wrote:
> > > > On Fri, Apr 19, 2024 at 03:45:46PM +0100, Conor Dooley wrote:
> > > > > On Fri, Apr 19, 2024 at 03:53:24PM +0200, Andrew Jones wrote:
> > > > > > Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension
> > > > > > which was ratified in commit 98918c844281 of riscv-isa-manual.
> > > > > > 
> > > > > > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > > > > > ---
> > > > > >  .../devicetree/bindings/riscv/extensions.yaml        | 12 ++++++++++++
> > > > > >  1 file changed, 12 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > index 468c646247aa..584da2f539e5 100644
> > > > > > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > > > > > @@ -177,6 +177,18 @@ properties:
> > > > > >              is supported as ratified at commit 5059e0ca641c ("update to
> > > > > >              ratified") of the riscv-zacas.
> > > > > >  
> > > > > > +        - const: zawrs
> > > > > > +          description: |
> > > > > > +            The Zawrs extension for entering a low-power state or for trapping
> > > > > > +            to a hypervisor while waiting on a store to a memory location, as
> > > > > > +            ratified in commit 98918c844281 ("Merge pull request #1217 from
> > > > > > +            riscv/zawrs") of riscv-isa-manual.
> > > > > 
> > > > > This part is fine...
> > > > > 
> > > > > 
> > > > > > Linux assumes that WRS.NTO will
> > > > > > +            either always eventually terminate the stall due to the reservation
> > > > > > +            set becoming invalid, implementation-specific other reasons, or
> > > > > > +            because a higher privilege level has configured it to cause an
> > > > > > +            illegal instruction exception after an implementation-specific
> > > > > > +            bounded time limit.
> > > > > 
> > > > > ...but I don't like this bit. The binding should just describe what the
> > > > > property means for the hardware, not discuss specifics about a
> > > > > particular OS.
> > > > > 
> > > > > And with my dt-bindings hat off and my kernel hat on, I think that if we
> > > > > want to have more specific requirements than the extension provides we
> > > > > either need to a) document that zawrs means that it will always
> > > > > terminate or b) additionally document a "zawrs-always-terminates" that
> > > > > has that meaning and look for it to enable the behaviour.
> > > > 
> > > > IIUC, the text above mostly just needs to remove 'Linux assumes' in order
> > > > to provide what we want for (a)? I'm not sure about (b). If Zawrs is
> > > > unusable as is, then we should probably just go back to the specs and get
> > > > a new standard extension name for a new version which includes the changes
> > > > we need.
> > > 
> > > An (official) new name for the behaviour that you actually want, especially
> > > if the patchset sent the other day does not have the more stringent
> > > requirement (I won't even pretend to understand Zawrs well enough to know
> > > whether it does or not), sounds like the ideal outcome. That way you're
> > > also sorted on the ACPI side.
> > 
> > What would be the purpose of a vendor implementing WRS.NTO (and putting
> > it in the DT) that never terminates? The spec says "Then a subsequent
> > WRS.NTO instruction would cause the hart to temporarily stall execution
> > in a low- power state until a store occurs to the reservation set or an
> > interrupt is observed." Why is this wording for WRS.NTO not sufficient
> > to assume that an implementation of this instruction would eventually
> > terminate?
> >
> 
> We can invoke smp_cond_load_relaxed(addr, VAL || anything_we_want()). This
> means we may not expect VAL ever to be written, which rules out "until a
> store occurs". As for "an interrupt is observed", we don't know which one
> to expect to arrive within a "reasonable" amount of time. We need to know
> which one(s), since, while wrs.nto will terminate even when interrupts are
> globally disabled, we still need to have the interrupt(s) we expect to be
> locally enabled. And, the interrupts should arrive in a "reasonable"
> amount of time since we want to poll anything_we_want() at a "reasonable"
> frequency.
> 
> So, we need firmware to promise to enable exceptions if there aren't any
> such interrupts. Or, we could require hardware descriptions to identify
> which interrupt(s) would be good to have enabled before calling wrs.nto.
> Maybe there's already some way to describe something like that?
> 
> Thanks,
> drew

Ahh okay I am caught up now. So the wording we are looking at in the
spec is:

"When executing in VS or VU mode, if the VTW bit is set in hstatus, the
TW bit in mstatus is clear, and the WRS.NTO does not complete within an
implementation-specific bounded time limit, the WRS.NTO instruction will
cause a virtual instruction exception."

With the concern being that it is possible for "implementation-specific
bounded time limit" to be infinite/never times out, and the kernel
enters a WRS where the reservation set is not required to be invalidated
for the condition we are waiting on to become true.

An option here would be to enforce in the spec that this time limit is
finite. If the original intention of the spec was to have it be finite,
then this would not be an issue. If the intention was to allow no time
limit, then this would probably have to be a new extension.

We are also able to change the kernel to not allow these conditions that
would break this interpretation of WRS. I found three instances in the
kernel that contain a condition that is not dependent on the wrs
reservation.

1.
# queued_spin_lock_slowpath() in kernel/locking/qspinlock.c
val = atomic_cond_read_relaxed(&lock->val,
			       (VAL != _Q_PENDING_VAL) || !cnt--);

The first condition will only become true if lock->val changes which
should invalidate the reservation. !cnt-- on the otherhand is a counter
of the number of loops that happen under-the-hood in
atomic_cond_read_relaxed. This seems like an abuse of the function and
could be factored out into a new bounded-iteration cond_read macro.

2.
# osq_lock() in kernel/locking/osq_lock.c
if (smp_cond_load_relaxed(&node->locked, VAL || need_resched() ||
			  vcpu_is_preempted(node_cpu(node->prev))))

VAL is the first condition and won't be a problem here since changes to
it will cause the reservation to become invalid. arm64 has hard-coded
vcpu_is_preempted to be false for the same exact reason that riscv would
want to (the wait wouldn't be woken up). There is a comment that
points this out in arch/arm64/include/asm/spinlock.h. riscv currently
uses the default implementation which returns false.

need_resched() should not be a problem since this condition only changes
when the hart recieves an IPI, so as long as the hart is able to receive
an IPI while in WRS it will be fine.

3.
# __arm_smmu_cmdq_poll_until_msi() in drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
smp_cond_load_relaxed(cmd, !VAL || (ret = queue_poll(&qp)));

arm driver, not relevant.



The only case that would cause a problem in the current implementation
of the kernel would be queued_spin_lock_slowpath() with the cnt check.
We are able to either change this definition, change the spec, or leave
it up to the vendor who would be hit by this issue to change it.

- Charlie

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Andrew Jones]

On Mon, Apr 22, 2024 at 06:36:45PM -0400, Charlie Jenkins wrote:
> On Sun, Apr 21, 2024 at 12:20:03PM +0200, Andrew Jones wrote:
> > On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
...
> > > What would be the purpose of a vendor implementing WRS.NTO (and putting
> > > it in the DT) that never terminates? The spec says "Then a subsequent
> > > WRS.NTO instruction would cause the hart to temporarily stall execution
> > > in a low- power state until a store occurs to the reservation set or an
> > > interrupt is observed." Why is this wording for WRS.NTO not sufficient
> > > to assume that an implementation of this instruction would eventually
> > > terminate?
> > >
> > 
> > We can invoke smp_cond_load_relaxed(addr, VAL || anything_we_want()). This
> > means we may not expect VAL ever to be written, which rules out "until a
> > store occurs". As for "an interrupt is observed", we don't know which one
> > to expect to arrive within a "reasonable" amount of time. We need to know
> > which one(s), since, while wrs.nto will terminate even when interrupts are
> > globally disabled, we still need to have the interrupt(s) we expect to be
> > locally enabled. And, the interrupts should arrive in a "reasonable"
> > amount of time since we want to poll anything_we_want() at a "reasonable"
> > frequency.
> > 
> > So, we need firmware to promise to enable exceptions if there aren't any
> > such interrupts. Or, we could require hardware descriptions to identify
> > which interrupt(s) would be good to have enabled before calling wrs.nto.
> > Maybe there's already some way to describe something like that?
> > 
> > Thanks,
> > drew
> 
> Ahh okay I am caught up now. So the wording we are looking at in the
> spec is:
> 
> "When executing in VS or VU mode, if the VTW bit is set in hstatus, the
> TW bit in mstatus is clear, and the WRS.NTO does not complete within an
> implementation-specific bounded time limit, the WRS.NTO instruction will
> cause a virtual instruction exception."

That's what the hypervisor should promise to do when there's no other
guarantee of wrs.nto terminating (but the hypervisor likely wants to
anyway since it wants guests to trap on wrs.nto in order to potentially
schedule the lock holding VCPU). The firmware of the host should likewise
promise to set mstatus.TW when there's no guarantee of wrs.nto
terminating, but that's likely _not_ something it normally would want to
do, so hopefully there will always be implementation-specific "other
reasons" which guarantee termination.

> 
> With the concern being that it is possible for "implementation-specific
> bounded time limit" to be infinite/never times out,

The implementation-defined short timeout cannot be infinite, but it only
applies to wrs.sto. While using wrs.sto would relieve the concern, it
cannot be configured to raise exceptions, which means it's not useful in
guests. If we want to use wrs.sto in hosts and wrs.nto in guests then we
need a paravirt channel which allows an "enlightened" guest to determine
that it is a guest and that the hypervisor has configured wrs.nto to
trap, which then indicates it's a good idea to patch wrs.sto to wrs.nto.
But, adding paravirt stuff should be avoided whenever possible since it
adds complexity we'd rather not maintain.

> and the kernel
> enters a WRS where the reservation set is not required to be invalidated
> for the condition we are waiting on to become true.
> 
> An option here would be to enforce in the spec that this time limit is
> finite. If the original intention of the spec was to have it be finite,
> then this would not be an issue. If the intention was to allow no time
> limit, then this would probably have to be a new extension.

wrs.nto has been specified to never timeout.
wrs.sto has been specified to never raise exceptions.

If we had an instruction which would timeout when mstatus.TW/hstatus.VTW
is clear and raise exceptions when set, then that's the one we'd choose.

> 
> We are also able to change the kernel to not allow these conditions that
> would break this interpretation of WRS. I found three instances in the
> kernel that contain a condition that is not dependent on the wrs
> reservation.
> 
> 1.
> # queued_spin_lock_slowpath() in kernel/locking/qspinlock.c
> val = atomic_cond_read_relaxed(&lock->val,
> 			       (VAL != _Q_PENDING_VAL) || !cnt--);
> 
> The first condition will only become true if lock->val changes which
> should invalidate the reservation. !cnt-- on the otherhand is a counter
> of the number of loops that happen under-the-hood in
> atomic_cond_read_relaxed. This seems like an abuse of the function and
> could be factored out into a new bounded-iteration cond_read macro.
> 
> 2.
> # osq_lock() in kernel/locking/osq_lock.c
> if (smp_cond_load_relaxed(&node->locked, VAL || need_resched() ||
> 			  vcpu_is_preempted(node_cpu(node->prev))))
> 
> VAL is the first condition and won't be a problem here since changes to
> it will cause the reservation to become invalid. arm64 has hard-coded
> vcpu_is_preempted to be false for the same exact reason that riscv would
> want to (the wait wouldn't be woken up). There is a comment that
> points this out in arch/arm64/include/asm/spinlock.h. riscv currently
> uses the default implementation which returns false.

The operative word is 'currently'. I plan to eventually get riscv's
vcpu_is_preempted() working since we already laid the groundwork by
adding a preempted flag to the SBI STA struct.

> 
> need_resched() should not be a problem since this condition only changes
> when the hart recieves an IPI, so as long as the hart is able to receive
> an IPI while in WRS it will be fine.
> 
> 3.
> # __arm_smmu_cmdq_poll_until_msi() in drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> smp_cond_load_relaxed(cmd, !VAL || (ret = queue_poll(&qp)));
> 
> arm driver, not relevant.
> 
> 
> 
> The only case that would cause a problem in the current implementation
> of the kernel would be queued_spin_lock_slowpath() with the cnt check.
> We are able to either change this definition, change the spec, or leave
> it up to the vendor who would be hit by this issue to change it.

We could attempt to document restrictions on the condition given to
smp_cond_load_relaxed() and then change the callers to honor those
restrictions, but that doesn't sound too easy. How will we remove
vcpu_is_preempted() on x86?

We should probably start the process of a new extension which has the
hybrid wrs.sto/nto instruction, but I'm reluctant to, since, in practice,
the current wrs.nto is probably fine. I don't really expect there to be
implementations that never terminate, even though I'd rather we document
that it's _required_ wrs.nto terminates, or that exceptions be raised.
Maybe I'm attempting to document it in the wrong place though. Maybe this
is more of a Documentation/arch/riscv/boot.rst type of thing.

Thanks,
drew

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Conor Dooley]

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

On Tue, Apr 23, 2024 at 10:46:01AM +0200, Andrew Jones wrote:
> Maybe I'm attempting to document it in the wrong place though. Maybe this
> is more of a Documentation/arch/riscv/boot.rst type of thing.

It might be good to document it there also, but I'd like to avoid being
unable to rely on what firmware tells us is supported because we have a
stricter requirement.

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

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Charlie Jenkins]

On Tue, Apr 23, 2024 at 10:46:01AM +0200, Andrew Jones wrote:
> On Mon, Apr 22, 2024 at 06:36:45PM -0400, Charlie Jenkins wrote:
> > On Sun, Apr 21, 2024 at 12:20:03PM +0200, Andrew Jones wrote:
> > > On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
> ...
> > > > What would be the purpose of a vendor implementing WRS.NTO (and putting
> > > > it in the DT) that never terminates? The spec says "Then a subsequent
> > > > WRS.NTO instruction would cause the hart to temporarily stall execution
> > > > in a low- power state until a store occurs to the reservation set or an
> > > > interrupt is observed." Why is this wording for WRS.NTO not sufficient
> > > > to assume that an implementation of this instruction would eventually
> > > > terminate?
> > > >
> > > 
> > > We can invoke smp_cond_load_relaxed(addr, VAL || anything_we_want()). This
> > > means we may not expect VAL ever to be written, which rules out "until a
> > > store occurs". As for "an interrupt is observed", we don't know which one
> > > to expect to arrive within a "reasonable" amount of time. We need to know
> > > which one(s), since, while wrs.nto will terminate even when interrupts are
> > > globally disabled, we still need to have the interrupt(s) we expect to be
> > > locally enabled. And, the interrupts should arrive in a "reasonable"
> > > amount of time since we want to poll anything_we_want() at a "reasonable"
> > > frequency.
> > > 
> > > So, we need firmware to promise to enable exceptions if there aren't any
> > > such interrupts. Or, we could require hardware descriptions to identify
> > > which interrupt(s) would be good to have enabled before calling wrs.nto.
> > > Maybe there's already some way to describe something like that?
> > > 
> > > Thanks,
> > > drew
> > 
> > Ahh okay I am caught up now. So the wording we are looking at in the
> > spec is:
> > 
> > "When executing in VS or VU mode, if the VTW bit is set in hstatus, the
> > TW bit in mstatus is clear, and the WRS.NTO does not complete within an
> > implementation-specific bounded time limit, the WRS.NTO instruction will
> > cause a virtual instruction exception."
> 
> That's what the hypervisor should promise to do when there's no other
> guarantee of wrs.nto terminating (but the hypervisor likely wants to
> anyway since it wants guests to trap on wrs.nto in order to potentially
> schedule the lock holding VCPU). The firmware of the host should likewise
> promise to set mstatus.TW when there's no guarantee of wrs.nto
> terminating, but that's likely _not_ something it normally would want to
> do, so hopefully there will always be implementation-specific "other
> reasons" which guarantee termination.
> 
> > 
> > With the concern being that it is possible for "implementation-specific
> > bounded time limit" to be infinite/never times out,
> 
> The implementation-defined short timeout cannot be infinite, but it only
> applies to wrs.sto. While using wrs.sto would relieve the concern, it
> cannot be configured to raise exceptions, which means it's not useful in
> guests. If we want to use wrs.sto in hosts and wrs.nto in guests then we
> need a paravirt channel which allows an "enlightened" guest to determine
> that it is a guest and that the hypervisor has configured wrs.nto to
> trap, which then indicates it's a good idea to patch wrs.sto to wrs.nto.
> But, adding paravirt stuff should be avoided whenever possible since it
> adds complexity we'd rather not maintain.
> 

That still wouldn't solve this issue, because the wrs.nto guest may still
never wakeup in the implementation-specific way?

> > and the kernel
> > enters a WRS where the reservation set is not required to be invalidated
> > for the condition we are waiting on to become true.
> > 
> > An option here would be to enforce in the spec that this time limit is
> > finite. If the original intention of the spec was to have it be finite,
> > then this would not be an issue. If the intention was to allow no time
> > limit, then this would probably have to be a new extension.
> 
> wrs.nto has been specified to never timeout.
> wrs.sto has been specified to never raise exceptions.
> 
> If we had an instruction which would timeout when mstatus.TW/hstatus.VTW
> is clear and raise exceptions when set, then that's the one we'd choose.
> 

Yes, this does seem like the ideal situtation.

> > 
> > We are also able to change the kernel to not allow these conditions that
> > would break this interpretation of WRS. I found three instances in the
> > kernel that contain a condition that is not dependent on the wrs
> > reservation.
> > 
> > 1.
> > # queued_spin_lock_slowpath() in kernel/locking/qspinlock.c
> > val = atomic_cond_read_relaxed(&lock->val,
> > 			       (VAL != _Q_PENDING_VAL) || !cnt--);
> > 
> > The first condition will only become true if lock->val changes which
> > should invalidate the reservation. !cnt-- on the otherhand is a counter
> > of the number of loops that happen under-the-hood in
> > atomic_cond_read_relaxed. This seems like an abuse of the function and
> > could be factored out into a new bounded-iteration cond_read macro.
> > 
> > 2.
> > # osq_lock() in kernel/locking/osq_lock.c
> > if (smp_cond_load_relaxed(&node->locked, VAL || need_resched() ||
> > 			  vcpu_is_preempted(node_cpu(node->prev))))
> > 
> > VAL is the first condition and won't be a problem here since changes to
> > it will cause the reservation to become invalid. arm64 has hard-coded
> > vcpu_is_preempted to be false for the same exact reason that riscv would
> > want to (the wait wouldn't be woken up). There is a comment that
> > points this out in arch/arm64/include/asm/spinlock.h. riscv currently
> > uses the default implementation which returns false.
> 
> The operative word is 'currently'. I plan to eventually get riscv's
> vcpu_is_preempted() working since we already laid the groundwork by
> adding a preempted flag to the SBI STA struct.
> 
> > 
> > need_resched() should not be a problem since this condition only changes
> > when the hart recieves an IPI, so as long as the hart is able to receive
> > an IPI while in WRS it will be fine.
> > 
> > 3.
> > # __arm_smmu_cmdq_poll_until_msi() in drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > smp_cond_load_relaxed(cmd, !VAL || (ret = queue_poll(&qp)));
> > 
> > arm driver, not relevant.
> > 
> > 
> > 
> > The only case that would cause a problem in the current implementation
> > of the kernel would be queued_spin_lock_slowpath() with the cnt check.
> > We are able to either change this definition, change the spec, or leave
> > it up to the vendor who would be hit by this issue to change it.
> 
> We could attempt to document restrictions on the condition given to
> smp_cond_load_relaxed() and then change the callers to honor those
> restrictions, but that doesn't sound too easy. How will we remove
> vcpu_is_preempted() on x86?

The solution here seems like it would be to not use wrs for riscv in
this case.  We really would need an alternate extension that allows wrs
in a guest to be guaranteed to trap into the host at some point. 

> 
> We should probably start the process of a new extension which has the
> hybrid wrs.sto/nto instruction, but I'm reluctant to, since, in practice,
> the current wrs.nto is probably fine. I don't really expect there to be
> implementations that never terminate, even though I'd rather we document
> that it's _required_ wrs.nto terminates, or that exceptions be raised.
> Maybe I'm attempting to document it in the wrong place though. Maybe this
> is more of a Documentation/arch/riscv/boot.rst type of thing.
> 

wrs.nto is most likely sufficient. A new extension will take a long
time. We could go ahead with wrs.nto, propose the extension, and when
the extension is ready switch over to it. In the meantime have this
behavior documented.

> Thanks,
> drew

- Charlie

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Charlie Jenkins]

On Tue, Apr 23, 2024 at 02:00:53PM -0400, Charlie Jenkins wrote:
> On Tue, Apr 23, 2024 at 10:46:01AM +0200, Andrew Jones wrote:
> > On Mon, Apr 22, 2024 at 06:36:45PM -0400, Charlie Jenkins wrote:
> > > On Sun, Apr 21, 2024 at 12:20:03PM +0200, Andrew Jones wrote:
> > > > On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
> > ...
> > > > > What would be the purpose of a vendor implementing WRS.NTO (and putting
> > > > > it in the DT) that never terminates? The spec says "Then a subsequent
> > > > > WRS.NTO instruction would cause the hart to temporarily stall execution
> > > > > in a low- power state until a store occurs to the reservation set or an
> > > > > interrupt is observed." Why is this wording for WRS.NTO not sufficient
> > > > > to assume that an implementation of this instruction would eventually
> > > > > terminate?
> > > > >
> > > > 
> > > > We can invoke smp_cond_load_relaxed(addr, VAL || anything_we_want()). This
> > > > means we may not expect VAL ever to be written, which rules out "until a
> > > > store occurs". As for "an interrupt is observed", we don't know which one
> > > > to expect to arrive within a "reasonable" amount of time. We need to know
> > > > which one(s), since, while wrs.nto will terminate even when interrupts are
> > > > globally disabled, we still need to have the interrupt(s) we expect to be
> > > > locally enabled. And, the interrupts should arrive in a "reasonable"
> > > > amount of time since we want to poll anything_we_want() at a "reasonable"
> > > > frequency.
> > > > 
> > > > So, we need firmware to promise to enable exceptions if there aren't any
> > > > such interrupts. Or, we could require hardware descriptions to identify
> > > > which interrupt(s) would be good to have enabled before calling wrs.nto.
> > > > Maybe there's already some way to describe something like that?
> > > > 
> > > > Thanks,
> > > > drew
> > > 
> > > Ahh okay I am caught up now. So the wording we are looking at in the
> > > spec is:
> > > 
> > > "When executing in VS or VU mode, if the VTW bit is set in hstatus, the
> > > TW bit in mstatus is clear, and the WRS.NTO does not complete within an
> > > implementation-specific bounded time limit, the WRS.NTO instruction will
> > > cause a virtual instruction exception."
> > 
> > That's what the hypervisor should promise to do when there's no other
> > guarantee of wrs.nto terminating (but the hypervisor likely wants to
> > anyway since it wants guests to trap on wrs.nto in order to potentially
> > schedule the lock holding VCPU). The firmware of the host should likewise
> > promise to set mstatus.TW when there's no guarantee of wrs.nto
> > terminating, but that's likely _not_ something it normally would want to
> > do, so hopefully there will always be implementation-specific "other
> > reasons" which guarantee termination.
> > 
> > > 
> > > With the concern being that it is possible for "implementation-specific
> > > bounded time limit" to be infinite/never times out,
> > 
> > The implementation-defined short timeout cannot be infinite, but it only
> > applies to wrs.sto. While using wrs.sto would relieve the concern, it
> > cannot be configured to raise exceptions, which means it's not useful in
> > guests. If we want to use wrs.sto in hosts and wrs.nto in guests then we
> > need a paravirt channel which allows an "enlightened" guest to determine
> > that it is a guest and that the hypervisor has configured wrs.nto to
> > trap, which then indicates it's a good idea to patch wrs.sto to wrs.nto.
> > But, adding paravirt stuff should be avoided whenever possible since it
> > adds complexity we'd rather not maintain.
> > 
> 
> That still wouldn't solve this issue, because the wrs.nto guest may still
> never wakeup in the implementation-specific way?
> 
> > > and the kernel
> > > enters a WRS where the reservation set is not required to be invalidated
> > > for the condition we are waiting on to become true.
> > > 
> > > An option here would be to enforce in the spec that this time limit is
> > > finite. If the original intention of the spec was to have it be finite,
> > > then this would not be an issue. If the intention was to allow no time
> > > limit, then this would probably have to be a new extension.
> > 
> > wrs.nto has been specified to never timeout.
> > wrs.sto has been specified to never raise exceptions.
> > 
> > If we had an instruction which would timeout when mstatus.TW/hstatus.VTW
> > is clear and raise exceptions when set, then that's the one we'd choose.
> > 
> 
> Yes, this does seem like the ideal situtation.
> 
> > > 
> > > We are also able to change the kernel to not allow these conditions that
> > > would break this interpretation of WRS. I found three instances in the
> > > kernel that contain a condition that is not dependent on the wrs
> > > reservation.
> > > 
> > > 1.
> > > # queued_spin_lock_slowpath() in kernel/locking/qspinlock.c
> > > val = atomic_cond_read_relaxed(&lock->val,
> > > 			       (VAL != _Q_PENDING_VAL) || !cnt--);
> > > 
> > > The first condition will only become true if lock->val changes which
> > > should invalidate the reservation. !cnt-- on the otherhand is a counter
> > > of the number of loops that happen under-the-hood in
> > > atomic_cond_read_relaxed. This seems like an abuse of the function and
> > > could be factored out into a new bounded-iteration cond_read macro.
> > > 
> > > 2.
> > > # osq_lock() in kernel/locking/osq_lock.c
> > > if (smp_cond_load_relaxed(&node->locked, VAL || need_resched() ||
> > > 			  vcpu_is_preempted(node_cpu(node->prev))))
> > > 
> > > VAL is the first condition and won't be a problem here since changes to
> > > it will cause the reservation to become invalid. arm64 has hard-coded
> > > vcpu_is_preempted to be false for the same exact reason that riscv would
> > > want to (the wait wouldn't be woken up). There is a comment that
> > > points this out in arch/arm64/include/asm/spinlock.h. riscv currently
> > > uses the default implementation which returns false.
> > 
> > The operative word is 'currently'. I plan to eventually get riscv's
> > vcpu_is_preempted() working since we already laid the groundwork by
> > adding a preempted flag to the SBI STA struct.
> > 
> > > 
> > > need_resched() should not be a problem since this condition only changes
> > > when the hart recieves an IPI, so as long as the hart is able to receive
> > > an IPI while in WRS it will be fine.
> > > 
> > > 3.
> > > # __arm_smmu_cmdq_poll_until_msi() in drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > > smp_cond_load_relaxed(cmd, !VAL || (ret = queue_poll(&qp)));
> > > 
> > > arm driver, not relevant.
> > > 
> > > 
> > > 
> > > The only case that would cause a problem in the current implementation
> > > of the kernel would be queued_spin_lock_slowpath() with the cnt check.
> > > We are able to either change this definition, change the spec, or leave
> > > it up to the vendor who would be hit by this issue to change it.
> > 
> > We could attempt to document restrictions on the condition given to
> > smp_cond_load_relaxed() and then change the callers to honor those
> > restrictions, but that doesn't sound too easy. How will we remove
> > vcpu_is_preempted() on x86?
> 
> The solution here seems like it would be to not use wrs for riscv in
> this case.  We really would need an alternate extension that allows wrs
> in a guest to be guaranteed to trap into the host at some point. 

Thinking about this a bit more, this is a performance penalty and not a
correctness issue. This line is an optimization that allows the lock
holder to jump the queue if the holder directly in front is a preempted
vcpu. Eventually the vcpu will be scheduled again and give up the lock.
So an implementation of WRS.NTO that does not have the
"implementation-specific bounded time limit" that the spec calls out for
WRS.NTO to raise a virtual instruction exception, would still function,
just slower.

I am not sure where the line we would draw here. Using WRS here would be
very beneficial for most implementations, but will not be optimally
efficient on some implementations of WRS in virtualization. We could
make a note that for optimal efficiency Linux recommends that a virtual
instruction exception is eventually thrown.

We also wouldn't need to mess with the cnt variable since that can only
increase during a loop by the thread that is doing the loop. It is an
optimization to break out early. Again this is purely performance. If
the implementation never breaks out without the lock variable being
changed, it will just wait until the variable is changed rather than
breaking out early.

- Charlie

> 
> > 
> > We should probably start the process of a new extension which has the
> > hybrid wrs.sto/nto instruction, but I'm reluctant to, since, in practice,
> > the current wrs.nto is probably fine. I don't really expect there to be
> > implementations that never terminate, even though I'd rather we document
> > that it's _required_ wrs.nto terminates, or that exceptions be raised.
> > Maybe I'm attempting to document it in the wrong place though. Maybe this
> > is more of a Documentation/arch/riscv/boot.rst type of thing.
> > 
> 
> wrs.nto is most likely sufficient. A new extension will take a long
> time. We could go ahead with wrs.nto, propose the extension, and when
> the extension is ready switch over to it. In the meantime have this
> behavior documented.
> 
> > Thanks,
> > drew
> 
> - Charlie
> 

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Andrew Jones]

On Tue, Apr 23, 2024 at 03:42:47PM -0400, Charlie Jenkins wrote:
> On Tue, Apr 23, 2024 at 02:00:53PM -0400, Charlie Jenkins wrote:
> > On Tue, Apr 23, 2024 at 10:46:01AM +0200, Andrew Jones wrote:
> > > On Mon, Apr 22, 2024 at 06:36:45PM -0400, Charlie Jenkins wrote:
> > > > On Sun, Apr 21, 2024 at 12:20:03PM +0200, Andrew Jones wrote:
> > > > > On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
> > > ...
> > > > > > What would be the purpose of a vendor implementing WRS.NTO (and putting
> > > > > > it in the DT) that never terminates? The spec says "Then a subsequent
> > > > > > WRS.NTO instruction would cause the hart to temporarily stall execution
> > > > > > in a low- power state until a store occurs to the reservation set or an
> > > > > > interrupt is observed." Why is this wording for WRS.NTO not sufficient
> > > > > > to assume that an implementation of this instruction would eventually
> > > > > > terminate?
> > > > > >
> > > > > 
> > > > > We can invoke smp_cond_load_relaxed(addr, VAL || anything_we_want()). This
> > > > > means we may not expect VAL ever to be written, which rules out "until a
> > > > > store occurs". As for "an interrupt is observed", we don't know which one
> > > > > to expect to arrive within a "reasonable" amount of time. We need to know
> > > > > which one(s), since, while wrs.nto will terminate even when interrupts are
> > > > > globally disabled, we still need to have the interrupt(s) we expect to be
> > > > > locally enabled. And, the interrupts should arrive in a "reasonable"
> > > > > amount of time since we want to poll anything_we_want() at a "reasonable"
> > > > > frequency.
> > > > > 
> > > > > So, we need firmware to promise to enable exceptions if there aren't any
> > > > > such interrupts. Or, we could require hardware descriptions to identify
> > > > > which interrupt(s) would be good to have enabled before calling wrs.nto.
> > > > > Maybe there's already some way to describe something like that?
> > > > > 
> > > > > Thanks,
> > > > > drew
> > > > 
> > > > Ahh okay I am caught up now. So the wording we are looking at in the
> > > > spec is:
> > > > 
> > > > "When executing in VS or VU mode, if the VTW bit is set in hstatus, the
> > > > TW bit in mstatus is clear, and the WRS.NTO does not complete within an
> > > > implementation-specific bounded time limit, the WRS.NTO instruction will
> > > > cause a virtual instruction exception."
> > > 
> > > That's what the hypervisor should promise to do when there's no other
> > > guarantee of wrs.nto terminating (but the hypervisor likely wants to
> > > anyway since it wants guests to trap on wrs.nto in order to potentially
> > > schedule the lock holding VCPU). The firmware of the host should likewise
> > > promise to set mstatus.TW when there's no guarantee of wrs.nto
> > > terminating, but that's likely _not_ something it normally would want to
> > > do, so hopefully there will always be implementation-specific "other
> > > reasons" which guarantee termination.
> > > 
> > > > 
> > > > With the concern being that it is possible for "implementation-specific
> > > > bounded time limit" to be infinite/never times out,
> > > 
> > > The implementation-defined short timeout cannot be infinite, but it only
> > > applies to wrs.sto. While using wrs.sto would relieve the concern, it
> > > cannot be configured to raise exceptions, which means it's not useful in
> > > guests. If we want to use wrs.sto in hosts and wrs.nto in guests then we
> > > need a paravirt channel which allows an "enlightened" guest to determine
> > > that it is a guest and that the hypervisor has configured wrs.nto to
> > > trap, which then indicates it's a good idea to patch wrs.sto to wrs.nto.
> > > But, adding paravirt stuff should be avoided whenever possible since it
> > > adds complexity we'd rather not maintain.
> > > 
> > 
> > That still wouldn't solve this issue, because the wrs.nto guest may still
> > never wakeup in the implementation-specific way?

The paravirt approach does solve it, because wrs.nto is specified to raise
exceptions after an implementation-specific bounded time limit when the
hypervisor sets hstatus.VTW.

> 
> Thinking about this a bit more, this is a performance penalty and not a
> correctness issue.

It's incorrect to have a design that is likely to result in bad
performance.

> This line is an optimization that allows the lock
> holder to jump the queue if the holder directly in front is a preempted
> vcpu. Eventually the vcpu will be scheduled again and give up the lock.
> So an implementation of WRS.NTO that does not have the
> "implementation-specific bounded time limit" that the spec calls out for
> WRS.NTO to raise a virtual instruction exception, would still function,
> just slower.

The problem isn't specific to virtualization. The problem is using wrs.nto
when it has not been configured to raise exceptions and there are not any
other guaranteed termination events (other than the reservation set
becoming invalid). While the paravirt solution is virtualization specific,
it works, because we would then only use wrs.nto when we know we can, and
otherwise use wrs.sto. But, as I said, I'd rather not have a paravirt
solution.

Thanks,
drew

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Christoph Müllner]

On Wed, Apr 24, 2024 at 9:34 AM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Tue, Apr 23, 2024 at 03:42:47PM -0400, Charlie Jenkins wrote:
> > On Tue, Apr 23, 2024 at 02:00:53PM -0400, Charlie Jenkins wrote:
> > > On Tue, Apr 23, 2024 at 10:46:01AM +0200, Andrew Jones wrote:
> > > > On Mon, Apr 22, 2024 at 06:36:45PM -0400, Charlie Jenkins wrote:
> > > > > On Sun, Apr 21, 2024 at 12:20:03PM +0200, Andrew Jones wrote:
> > > > > > On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
> > > > ...
> > > > > > > What would be the purpose of a vendor implementing WRS.NTO (and putting
> > > > > > > it in the DT) that never terminates? The spec says "Then a subsequent
> > > > > > > WRS.NTO instruction would cause the hart to temporarily stall execution
> > > > > > > in a low- power state until a store occurs to the reservation set or an
> > > > > > > interrupt is observed." Why is this wording for WRS.NTO not sufficient
> > > > > > > to assume that an implementation of this instruction would eventually
> > > > > > > terminate?
> > > > > > >
> > > > > >
> > > > > > We can invoke smp_cond_load_relaxed(addr, VAL || anything_we_want()). This
> > > > > > means we may not expect VAL ever to be written, which rules out "until a
> > > > > > store occurs". As for "an interrupt is observed", we don't know which one
> > > > > > to expect to arrive within a "reasonable" amount of time. We need to know
> > > > > > which one(s), since, while wrs.nto will terminate even when interrupts are
> > > > > > globally disabled, we still need to have the interrupt(s) we expect to be
> > > > > > locally enabled. And, the interrupts should arrive in a "reasonable"
> > > > > > amount of time since we want to poll anything_we_want() at a "reasonable"
> > > > > > frequency.
> > > > > >
> > > > > > So, we need firmware to promise to enable exceptions if there aren't any
> > > > > > such interrupts. Or, we could require hardware descriptions to identify
> > > > > > which interrupt(s) would be good to have enabled before calling wrs.nto.
> > > > > > Maybe there's already some way to describe something like that?
> > > > > >
> > > > > > Thanks,
> > > > > > drew
> > > > >
> > > > > Ahh okay I am caught up now. So the wording we are looking at in the
> > > > > spec is:
> > > > >
> > > > > "When executing in VS or VU mode, if the VTW bit is set in hstatus, the
> > > > > TW bit in mstatus is clear, and the WRS.NTO does not complete within an
> > > > > implementation-specific bounded time limit, the WRS.NTO instruction will
> > > > > cause a virtual instruction exception."
> > > >
> > > > That's what the hypervisor should promise to do when there's no other
> > > > guarantee of wrs.nto terminating (but the hypervisor likely wants to
> > > > anyway since it wants guests to trap on wrs.nto in order to potentially
> > > > schedule the lock holding VCPU). The firmware of the host should likewise
> > > > promise to set mstatus.TW when there's no guarantee of wrs.nto
> > > > terminating, but that's likely _not_ something it normally would want to
> > > > do, so hopefully there will always be implementation-specific "other
> > > > reasons" which guarantee termination.
> > > >
> > > > >
> > > > > With the concern being that it is possible for "implementation-specific
> > > > > bounded time limit" to be infinite/never times out,
> > > >
> > > > The implementation-defined short timeout cannot be infinite, but it only
> > > > applies to wrs.sto. While using wrs.sto would relieve the concern, it
> > > > cannot be configured to raise exceptions, which means it's not useful in
> > > > guests. If we want to use wrs.sto in hosts and wrs.nto in guests then we
> > > > need a paravirt channel which allows an "enlightened" guest to determine
> > > > that it is a guest and that the hypervisor has configured wrs.nto to
> > > > trap, which then indicates it's a good idea to patch wrs.sto to wrs.nto.
> > > > But, adding paravirt stuff should be avoided whenever possible since it
> > > > adds complexity we'd rather not maintain.
> > > >
> > >
> > > That still wouldn't solve this issue, because the wrs.nto guest may still
> > > never wakeup in the implementation-specific way?
>
> The paravirt approach does solve it, because wrs.nto is specified to raise
> exceptions after an implementation-specific bounded time limit when the
> hypervisor sets hstatus.VTW.
>
> >
> > Thinking about this a bit more, this is a performance penalty and not a
> > correctness issue.
>
> It's incorrect to have a design that is likely to result in bad
> performance.
>
> > This line is an optimization that allows the lock
> > holder to jump the queue if the holder directly in front is a preempted
> > vcpu. Eventually the vcpu will be scheduled again and give up the lock.
> > So an implementation of WRS.NTO that does not have the
> > "implementation-specific bounded time limit" that the spec calls out for
> > WRS.NTO to raise a virtual instruction exception, would still function,
> > just slower.
>
> The problem isn't specific to virtualization. The problem is using wrs.nto
> when it has not been configured to raise exceptions and there are not any
> other guaranteed termination events (other than the reservation set
> becoming invalid). While the paravirt solution is virtualization specific,
> it works, because we would then only use wrs.nto when we know we can, and
> otherwise use wrs.sto. But, as I said, I'd rather not have a paravirt
> solution.

Andrew, it would be great if you could summarize this finding to the
spec authors.
Maybe a non-normative text added to the spec (that raises awareness
for the issue
and provides a guideline to avoid it) could reduce the risk of triggering
the issue on real HW. This might be enough justification to use WRS.NTO.

If WRS.NTO is considered as not reliable enough to wake up and therefore causing
performance issues or CPU stalls if used for the spin lock optimization,
it might be also reasonable to use WRS.STO instead.
The cost of having too many wakeups seems much more acceptable than
a stalled CPU.

BR
Christoph

Re: [PATCH v2 2/6] dt-bindings: riscv: Add Zawrs ISA extension description

[Andrew Jones]

On Wed, Apr 24, 2024 at 11:23:00AM +0200, Christoph Müllner wrote:
> On Wed, Apr 24, 2024 at 9:34 AM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Tue, Apr 23, 2024 at 03:42:47PM -0400, Charlie Jenkins wrote:
> > > On Tue, Apr 23, 2024 at 02:00:53PM -0400, Charlie Jenkins wrote:
> > > > On Tue, Apr 23, 2024 at 10:46:01AM +0200, Andrew Jones wrote:
> > > > > On Mon, Apr 22, 2024 at 06:36:45PM -0400, Charlie Jenkins wrote:
> > > > > > On Sun, Apr 21, 2024 at 12:20:03PM +0200, Andrew Jones wrote:
> > > > > > > On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
> > > > > ...
> > > > > > > > What would be the purpose of a vendor implementing WRS.NTO (and putting
> > > > > > > > it in the DT) that never terminates? The spec says "Then a subsequent
> > > > > > > > WRS.NTO instruction would cause the hart to temporarily stall execution
> > > > > > > > in a low- power state until a store occurs to the reservation set or an
> > > > > > > > interrupt is observed." Why is this wording for WRS.NTO not sufficient
> > > > > > > > to assume that an implementation of this instruction would eventually
> > > > > > > > terminate?
> > > > > > > >
> > > > > > >
> > > > > > > We can invoke smp_cond_load_relaxed(addr, VAL || anything_we_want()). This
> > > > > > > means we may not expect VAL ever to be written, which rules out "until a
> > > > > > > store occurs". As for "an interrupt is observed", we don't know which one
> > > > > > > to expect to arrive within a "reasonable" amount of time. We need to know
> > > > > > > which one(s), since, while wrs.nto will terminate even when interrupts are
> > > > > > > globally disabled, we still need to have the interrupt(s) we expect to be
> > > > > > > locally enabled. And, the interrupts should arrive in a "reasonable"
> > > > > > > amount of time since we want to poll anything_we_want() at a "reasonable"
> > > > > > > frequency.
> > > > > > >
> > > > > > > So, we need firmware to promise to enable exceptions if there aren't any
> > > > > > > such interrupts. Or, we could require hardware descriptions to identify
> > > > > > > which interrupt(s) would be good to have enabled before calling wrs.nto.
> > > > > > > Maybe there's already some way to describe something like that?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > drew
> > > > > >
> > > > > > Ahh okay I am caught up now. So the wording we are looking at in the
> > > > > > spec is:
> > > > > >
> > > > > > "When executing in VS or VU mode, if the VTW bit is set in hstatus, the
> > > > > > TW bit in mstatus is clear, and the WRS.NTO does not complete within an
> > > > > > implementation-specific bounded time limit, the WRS.NTO instruction will
> > > > > > cause a virtual instruction exception."
> > > > >
> > > > > That's what the hypervisor should promise to do when there's no other
> > > > > guarantee of wrs.nto terminating (but the hypervisor likely wants to
> > > > > anyway since it wants guests to trap on wrs.nto in order to potentially
> > > > > schedule the lock holding VCPU). The firmware of the host should likewise
> > > > > promise to set mstatus.TW when there's no guarantee of wrs.nto
> > > > > terminating, but that's likely _not_ something it normally would want to
> > > > > do, so hopefully there will always be implementation-specific "other
> > > > > reasons" which guarantee termination.
> > > > >
> > > > > >
> > > > > > With the concern being that it is possible for "implementation-specific
> > > > > > bounded time limit" to be infinite/never times out,
> > > > >
> > > > > The implementation-defined short timeout cannot be infinite, but it only
> > > > > applies to wrs.sto. While using wrs.sto would relieve the concern, it
> > > > > cannot be configured to raise exceptions, which means it's not useful in
> > > > > guests. If we want to use wrs.sto in hosts and wrs.nto in guests then we
> > > > > need a paravirt channel which allows an "enlightened" guest to determine
> > > > > that it is a guest and that the hypervisor has configured wrs.nto to
> > > > > trap, which then indicates it's a good idea to patch wrs.sto to wrs.nto.
> > > > > But, adding paravirt stuff should be avoided whenever possible since it
> > > > > adds complexity we'd rather not maintain.
> > > > >
> > > >
> > > > That still wouldn't solve this issue, because the wrs.nto guest may still
> > > > never wakeup in the implementation-specific way?
> >
> > The paravirt approach does solve it, because wrs.nto is specified to raise
> > exceptions after an implementation-specific bounded time limit when the
> > hypervisor sets hstatus.VTW.
> >
> > >
> > > Thinking about this a bit more, this is a performance penalty and not a
> > > correctness issue.
> >
> > It's incorrect to have a design that is likely to result in bad
> > performance.
> >
> > > This line is an optimization that allows the lock
> > > holder to jump the queue if the holder directly in front is a preempted
> > > vcpu. Eventually the vcpu will be scheduled again and give up the lock.
> > > So an implementation of WRS.NTO that does not have the
> > > "implementation-specific bounded time limit" that the spec calls out for
> > > WRS.NTO to raise a virtual instruction exception, would still function,
> > > just slower.
> >
> > The problem isn't specific to virtualization. The problem is using wrs.nto
> > when it has not been configured to raise exceptions and there are not any
> > other guaranteed termination events (other than the reservation set
> > becoming invalid). While the paravirt solution is virtualization specific,
> > it works, because we would then only use wrs.nto when we know we can, and
> > otherwise use wrs.sto. But, as I said, I'd rather not have a paravirt
> > solution.
> 
> Andrew, it would be great if you could summarize this finding to the
> spec authors.
> Maybe a non-normative text added to the spec (that raises awareness
> for the issue

Sure, I'll write something up pointing out the concern with wrs.nto and
post it to a few RVI mailing lists.

> and provides a guideline to avoid it) could reduce the risk of triggering
> the issue on real HW. This might be enough justification to use WRS.NTO.
> 
> If WRS.NTO is considered as not reliable enough to wake up and therefore causing
> performance issues or CPU stalls if used for the spin lock optimization,
> it might be also reasonable to use WRS.STO instead.
> The cost of having too many wakeups seems much more acceptable than
> a stalled CPU.

wrs.sto is reasonable to use in all cases, since too many wakeups isn't
a concern. But, we can do better in the virtualization case with wrs.nto,
where the hypervisor can get involved, so, to avoid paravirt stuff, we'd
like to be able to always use wrs.nto.

Thanks,
drew

[PATCH v2 3/6] riscv: Add Zawrs support for spinlocks

[Andrew Jones]

From: Christoph Müllner <christoph.muellner@vrull.eu>

RISC-V code uses the generic ticket lock implementation, which calls
the macros smp_cond_load_relaxed() and smp_cond_load_acquire().
Introduce a RISC-V specific implementation of smp_cond_load_relaxed()
which applies WRS.NTO of the Zawrs extension in order to reduce power
consumption while waiting and allows hypervisors to enable guests to
trap while waiting. smp_cond_load_acquire() doesn't need a RISC-V
specific implementation as the generic implementation is based on
smp_cond_load_relaxed() and smp_acquire__after_ctrl_dep() sufficiently
provides the acquire semantics.

This implementation is heavily based on Arm's approach which is the
approach Andrea Parri also suggested.

The Zawrs specification can be found here:
https://github.com/riscv/riscv-zawrs/blob/main/zawrs.adoc

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
Co-developed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/Kconfig                | 13 ++++++++
 arch/riscv/include/asm/barrier.h  | 45 ++++++++++++++++++---------
 arch/riscv/include/asm/cmpxchg.h  | 51 +++++++++++++++++++++++++++++++
 arch/riscv/include/asm/hwcap.h    |  1 +
 arch/riscv/include/asm/insn-def.h |  2 ++
 arch/riscv/kernel/cpufeature.c    |  1 +
 6 files changed, 98 insertions(+), 15 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 7427d8088337..34bbe6b70546 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -578,6 +578,19 @@ config RISCV_ISA_V_PREEMPTIVE
 	  preemption. Enabling this config will result in higher memory
 	  consumption due to the allocation of per-task's kernel Vector context.
 
+config RISCV_ISA_ZAWRS
+	bool "Zawrs extension support for more efficient busy waiting"
+	depends on RISCV_ALTERNATIVE
+	default y
+	help
+	  The Zawrs extension defines instructions to be used in polling loops
+	  which allow a hart to enter a low-power state or to trap to the
+	  hypervisor while waiting on a store to a memory location. Enable the
+	  use of these instructions in the kernel when the Zawrs extension is
+	  detected at boot.
+
+	  If you don't know what to do here, say Y.
+
 config TOOLCHAIN_HAS_ZBB
 	bool
 	default y
diff --git a/arch/riscv/include/asm/barrier.h b/arch/riscv/include/asm/barrier.h
index 880b56d8480d..e1d9bf1deca6 100644
--- a/arch/riscv/include/asm/barrier.h
+++ b/arch/riscv/include/asm/barrier.h
@@ -11,6 +11,7 @@
 #define _ASM_RISCV_BARRIER_H
 
 #ifndef __ASSEMBLY__
+#include <asm/cmpxchg.h>
 #include <asm/fence.h>
 
 #define nop()		__asm__ __volatile__ ("nop")
@@ -28,21 +29,6 @@
 #define __smp_rmb()	RISCV_FENCE(r, r)
 #define __smp_wmb()	RISCV_FENCE(w, w)
 
-#define __smp_store_release(p, v)					\
-do {									\
-	compiletime_assert_atomic_type(*p);				\
-	RISCV_FENCE(rw, w);						\
-	WRITE_ONCE(*p, v);						\
-} while (0)
-
-#define __smp_load_acquire(p)						\
-({									\
-	typeof(*p) ___p1 = READ_ONCE(*p);				\
-	compiletime_assert_atomic_type(*p);				\
-	RISCV_FENCE(r, rw);						\
-	___p1;								\
-})
-
 /*
  * This is a very specific barrier: it's currently only used in two places in
  * the kernel, both in the scheduler.  See include/linux/spinlock.h for the two
@@ -70,6 +56,35 @@ do {									\
  */
 #define smp_mb__after_spinlock()	RISCV_FENCE(iorw, iorw)
 
+#define __smp_store_release(p, v)					\
+do {									\
+	compiletime_assert_atomic_type(*p);				\
+	RISCV_FENCE(rw, w);						\
+	WRITE_ONCE(*p, v);						\
+} while (0)
+
+#define __smp_load_acquire(p)						\
+({									\
+	typeof(*p) ___p1 = READ_ONCE(*p);				\
+	compiletime_assert_atomic_type(*p);				\
+	RISCV_FENCE(r, rw);						\
+	___p1;								\
+})
+
+#ifdef CONFIG_RISCV_ISA_ZAWRS
+#define smp_cond_load_relaxed(ptr, cond_expr) ({			\
+	typeof(ptr) __PTR = (ptr);					\
+	__unqual_scalar_typeof(*ptr) VAL;				\
+	for (;;) {							\
+		VAL = READ_ONCE(*__PTR);				\
+		if (cond_expr)						\
+			break;						\
+		__cmpwait_relaxed(ptr, VAL);				\
+	}								\
+	(typeof(*ptr))VAL;						\
+})
+#endif
+
 #include <asm-generic/barrier.h>
 
 #endif /* __ASSEMBLY__ */
diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index 2fee65cc8443..0926ac7f4ca6 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -8,7 +8,10 @@
 
 #include <linux/bug.h>
 
+#include <asm/alternative-macros.h>
 #include <asm/fence.h>
+#include <asm/hwcap.h>
+#include <asm/insn-def.h>
 
 #define __xchg_relaxed(ptr, new, size)					\
 ({									\
@@ -359,4 +362,52 @@
 	arch_cmpxchg_relaxed((ptr), (o), (n));				\
 })
 
+#ifdef CONFIG_RISCV_ISA_ZAWRS
+static __always_inline void __cmpwait(volatile void *ptr,
+				      unsigned long val,
+				      int size)
+{
+	unsigned long tmp;
+
+	asm goto(ALTERNATIVE("j %l[no_zawrs]", "nop",
+			     0, RISCV_ISA_EXT_ZAWRS, 1)
+		 : : : : no_zawrs);
+
+	switch (size) {
+	case 4:
+		asm volatile(
+		"	lr.w	%0, %1\n"
+		"	xor	%0, %0, %2\n"
+		"	bnez	%0, 1f\n"
+			ZAWRS_WRS_NTO "\n"
+		"1:"
+		: "=&r" (tmp), "+A" (*(u32 *)ptr)
+		: "r" (val));
+		break;
+#if __riscv_xlen == 64
+	case 8:
+		asm volatile(
+		"	lr.d	%0, %1\n"
+		"	xor	%0, %0, %2\n"
+		"	bnez	%0, 1f\n"
+			ZAWRS_WRS_NTO "\n"
+		"1:"
+		: "=&r" (tmp), "+A" (*(u64 *)ptr)
+		: "r" (val));
+		break;
+#endif
+	default:
+		BUILD_BUG();
+	}
+
+	return;
+
+no_zawrs:
+	asm volatile(RISCV_PAUSE : : : "memory");
+}
+
+#define __cmpwait_relaxed(ptr, val) \
+	__cmpwait((ptr), (unsigned long)(val), sizeof(*(ptr)))
+#endif
+
 #endif /* _ASM_RISCV_CMPXCHG_H */
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index e17d0078a651..5b358c3cf212 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -81,6 +81,7 @@
 #define RISCV_ISA_EXT_ZTSO		72
 #define RISCV_ISA_EXT_ZACAS		73
 #define RISCV_ISA_EXT_XANDESPMU		74
+#define RISCV_ISA_EXT_ZAWRS		75
 
 #define RISCV_ISA_EXT_XLINUXENVCFG	127
 
diff --git a/arch/riscv/include/asm/insn-def.h b/arch/riscv/include/asm/insn-def.h
index 64dffaa21bfa..9a913010cdd9 100644
--- a/arch/riscv/include/asm/insn-def.h
+++ b/arch/riscv/include/asm/insn-def.h
@@ -197,5 +197,7 @@
 	       RS1(base), SIMM12(4))
 
 #define RISCV_PAUSE	".4byte 0x100000f"
+#define ZAWRS_WRS_NTO	".4byte 0x00d00073"
+#define ZAWRS_WRS_STO	".4byte 0x01d00073"
 
 #endif /* __ASM_INSN_DEF_H */
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 3ed2359eae35..02de9eaa3f42 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -257,6 +257,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 	__RISCV_ISA_EXT_DATA(zihintpause, RISCV_ISA_EXT_ZIHINTPAUSE),
 	__RISCV_ISA_EXT_DATA(zihpm, RISCV_ISA_EXT_ZIHPM),
 	__RISCV_ISA_EXT_DATA(zacas, RISCV_ISA_EXT_ZACAS),
+	__RISCV_ISA_EXT_DATA(zawrs, RISCV_ISA_EXT_ZAWRS),
 	__RISCV_ISA_EXT_DATA(zfa, RISCV_ISA_EXT_ZFA),
 	__RISCV_ISA_EXT_DATA(zfh, RISCV_ISA_EXT_ZFH),
 	__RISCV_ISA_EXT_DATA(zfhmin, RISCV_ISA_EXT_ZFHMIN),
-- 
2.44.0

Re: [PATCH v2 3/6] riscv: Add Zawrs support for spinlocks

[Conor Dooley]

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

On Fri, Apr 19, 2024 at 03:53:25PM +0200, Andrew Jones wrote:

> +config RISCV_ISA_ZAWRS
> +	bool "Zawrs extension support for more efficient busy waiting"
> +	depends on RISCV_ALTERNATIVE
> +	default y
> +	help
> +	  The Zawrs extension defines instructions to be used in polling loops
> +	  which allow a hart to enter a low-power state or to trap to the
> +	  hypervisor while waiting on a store to a memory location. Enable the
> +	  use of these instructions in the kernel when the Zawrs extension is
> +	  detected at boot.

Ignoring the rest of the patch, and focusing on the bit relevant to our
other conversation, I think this description satisfies what I was trying
to do with the other options in terms of being clear about what exactly
it does.

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

Re: [PATCH v2 3/6] riscv: Add Zawrs support for spinlocks

[Andrea Parri]

On Fri, Apr 19, 2024 at 03:53:25PM +0200, Andrew Jones wrote:
> From: Christoph M??llner <christoph.muellner@vrull.eu>
> 
> RISC-V code uses the generic ticket lock implementation, which calls
> the macros smp_cond_load_relaxed() and smp_cond_load_acquire().
> Introduce a RISC-V specific implementation of smp_cond_load_relaxed()
> which applies WRS.NTO of the Zawrs extension in order to reduce power
> consumption while waiting and allows hypervisors to enable guests to
> trap while waiting. smp_cond_load_acquire() doesn't need a RISC-V
> specific implementation as the generic implementation is based on
> smp_cond_load_relaxed() and smp_acquire__after_ctrl_dep() sufficiently
> provides the acquire semantics.
> 
> This implementation is heavily based on Arm's approach which is the
> approach Andrea Parri also suggested.
> 
> The Zawrs specification can be found here:
> https://github.com/riscv/riscv-zawrs/blob/main/zawrs.adoc
> 
> Signed-off-by: Christoph M??llner <christoph.muellner@vrull.eu>
> Co-developed-by: Andrew Jones <ajones@ventanamicro.com>
> Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> ---
>  arch/riscv/Kconfig                | 13 ++++++++
>  arch/riscv/include/asm/barrier.h  | 45 ++++++++++++++++++---------
>  arch/riscv/include/asm/cmpxchg.h  | 51 +++++++++++++++++++++++++++++++
>  arch/riscv/include/asm/hwcap.h    |  1 +
>  arch/riscv/include/asm/insn-def.h |  2 ++
>  arch/riscv/kernel/cpufeature.c    |  1 +
>  6 files changed, 98 insertions(+), 15 deletions(-)

Doesn't apply to riscv/for-next (due to, AFAIU,

  https://lore.kernel.org/all/171275883330.18495.10110341843571163280.git-patchwork-notify@kernel.org/ ).

But other than that, this LGTM.  One nit below.


> -#define __smp_store_release(p, v)					\
> -do {									\
> -	compiletime_assert_atomic_type(*p);				\
> -	RISCV_FENCE(rw, w);						\
> -	WRITE_ONCE(*p, v);						\
> -} while (0)
> -
> -#define __smp_load_acquire(p)						\
> -({									\
> -	typeof(*p) ___p1 = READ_ONCE(*p);				\
> -	compiletime_assert_atomic_type(*p);				\
> -	RISCV_FENCE(r, rw);						\
> -	___p1;								\
> -})
> -
>  /*
>   * This is a very specific barrier: it's currently only used in two places in
>   * the kernel, both in the scheduler.  See include/linux/spinlock.h for the two
> @@ -70,6 +56,35 @@ do {									\
>   */
>  #define smp_mb__after_spinlock()	RISCV_FENCE(iorw, iorw)
>  
> +#define __smp_store_release(p, v)					\
> +do {									\
> +	compiletime_assert_atomic_type(*p);				\
> +	RISCV_FENCE(rw, w);						\
> +	WRITE_ONCE(*p, v);						\
> +} while (0)
> +
> +#define __smp_load_acquire(p)						\
> +({									\
> +	typeof(*p) ___p1 = READ_ONCE(*p);				\
> +	compiletime_assert_atomic_type(*p);				\
> +	RISCV_FENCE(r, rw);						\
> +	___p1;								\
> +})

Unrelated/unmotivated changes.

  Andrea

Re: [PATCH v2 3/6] riscv: Add Zawrs support for spinlocks

[Andrew Jones]

On Sun, Apr 21, 2024 at 11:16:47PM +0200, Andrea Parri wrote:
> On Fri, Apr 19, 2024 at 03:53:25PM +0200, Andrew Jones wrote:
> > From: Christoph M??llner <christoph.muellner@vrull.eu>
> > 
> > RISC-V code uses the generic ticket lock implementation, which calls
> > the macros smp_cond_load_relaxed() and smp_cond_load_acquire().
> > Introduce a RISC-V specific implementation of smp_cond_load_relaxed()
> > which applies WRS.NTO of the Zawrs extension in order to reduce power
> > consumption while waiting and allows hypervisors to enable guests to
> > trap while waiting. smp_cond_load_acquire() doesn't need a RISC-V
> > specific implementation as the generic implementation is based on
> > smp_cond_load_relaxed() and smp_acquire__after_ctrl_dep() sufficiently
> > provides the acquire semantics.
> > 
> > This implementation is heavily based on Arm's approach which is the
> > approach Andrea Parri also suggested.
> > 
> > The Zawrs specification can be found here:
> > https://github.com/riscv/riscv-zawrs/blob/main/zawrs.adoc
> > 
> > Signed-off-by: Christoph M??llner <christoph.muellner@vrull.eu>
> > Co-developed-by: Andrew Jones <ajones@ventanamicro.com>
> > Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
> > ---
> >  arch/riscv/Kconfig                | 13 ++++++++
> >  arch/riscv/include/asm/barrier.h  | 45 ++++++++++++++++++---------
> >  arch/riscv/include/asm/cmpxchg.h  | 51 +++++++++++++++++++++++++++++++
> >  arch/riscv/include/asm/hwcap.h    |  1 +
> >  arch/riscv/include/asm/insn-def.h |  2 ++
> >  arch/riscv/kernel/cpufeature.c    |  1 +
> >  6 files changed, 98 insertions(+), 15 deletions(-)
> 
> Doesn't apply to riscv/for-next (due to, AFAIU,
> 
>   https://lore.kernel.org/all/171275883330.18495.10110341843571163280.git-patchwork-notify@kernel.org/ ).

I based it on -rc1. We recently discussed what we should base on, but
I couldn't recall the final decision, so I fell back to the old approach.
I can rebase on for-next or the latest rc if that's the new, improved
approach.

> 
> But other than that, this LGTM.  One nit below.
> 
> 
> > -#define __smp_store_release(p, v)					\
> > -do {									\
> > -	compiletime_assert_atomic_type(*p);				\
> > -	RISCV_FENCE(rw, w);						\
> > -	WRITE_ONCE(*p, v);						\
> > -} while (0)
> > -
> > -#define __smp_load_acquire(p)						\
> > -({									\
> > -	typeof(*p) ___p1 = READ_ONCE(*p);				\
> > -	compiletime_assert_atomic_type(*p);				\
> > -	RISCV_FENCE(r, rw);						\
> > -	___p1;								\
> > -})
> > -
> >  /*
> >   * This is a very specific barrier: it's currently only used in two places in
> >   * the kernel, both in the scheduler.  See include/linux/spinlock.h for the two
> > @@ -70,6 +56,35 @@ do {									\
> >   */
> >  #define smp_mb__after_spinlock()	RISCV_FENCE(iorw, iorw)
> >  
> > +#define __smp_store_release(p, v)					\
> > +do {									\
> > +	compiletime_assert_atomic_type(*p);				\
> > +	RISCV_FENCE(rw, w);						\
> > +	WRITE_ONCE(*p, v);						\
> > +} while (0)
> > +
> > +#define __smp_load_acquire(p)						\
> > +({									\
> > +	typeof(*p) ___p1 = READ_ONCE(*p);				\
> > +	compiletime_assert_atomic_type(*p);				\
> > +	RISCV_FENCE(r, rw);						\
> > +	___p1;								\
> > +})
> 
> Unrelated/unmotivated changes.

The relation/motivation was to get the load/store macros in one part of
the file with the barrier macros in another. With this change we have

 __mb
 __rmb
 __wmb

 __smp_mb
 __smp_rmb
 __smp_wmb

 smp_mb__after_spinlock

 __smp_store_release
 __smp_load_acquire
 smp_cond_load_relaxed

Without the change, smp_mb__after_spinlock is either after all the
load/stores or in between them.

I didn't think the reorganization was worth its own patch, but I could
split it out (or just drop it).

Thanks,
drew

[PATCH v2 4/6] riscv: hwprobe: export Zawrs ISA extension

[Andrew Jones]

Export Zawrs ISA extension through hwprobe.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 Documentation/arch/riscv/hwprobe.rst  | 4 ++++
 arch/riscv/include/uapi/asm/hwprobe.h | 1 +
 arch/riscv/kernel/sys_hwprobe.c       | 1 +
 3 files changed, 6 insertions(+)

diff --git a/Documentation/arch/riscv/hwprobe.rst b/Documentation/arch/riscv/hwprobe.rst
index b2bcc9eed9aa..e072ce8285d8 100644
--- a/Documentation/arch/riscv/hwprobe.rst
+++ b/Documentation/arch/riscv/hwprobe.rst
@@ -188,6 +188,10 @@ The following keys are defined:
        manual starting from commit 95cf1f9 ("Add changes requested by Ved
        during signoff")
 
+  * :c:macro:`RISCV_HWPROBE_EXT_ZAWRS`: The Zawrs extension is supported as
+       ratified in commit 98918c844281 ("Merge pull request #1217 from
+       riscv/zawrs") of riscv-isa-manual.
+
 * :c:macro:`RISCV_HWPROBE_KEY_CPUPERF_0`: A bitmask that contains performance
   information about the selected set of processors.
 
diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
index 9f2a8e3ff204..a5fca3878a32 100644
--- a/arch/riscv/include/uapi/asm/hwprobe.h
+++ b/arch/riscv/include/uapi/asm/hwprobe.h
@@ -59,6 +59,7 @@ struct riscv_hwprobe {
 #define		RISCV_HWPROBE_EXT_ZTSO		(1ULL << 33)
 #define		RISCV_HWPROBE_EXT_ZACAS		(1ULL << 34)
 #define		RISCV_HWPROBE_EXT_ZICOND	(1ULL << 35)
+#define		RISCV_HWPROBE_EXT_ZAWRS		(1ULL << 36)
 #define RISCV_HWPROBE_KEY_CPUPERF_0	5
 #define		RISCV_HWPROBE_MISALIGNED_UNKNOWN	(0 << 0)
 #define		RISCV_HWPROBE_MISALIGNED_EMULATED	(1 << 0)
diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
index 8cae41a502dd..b86e3531a45a 100644
--- a/arch/riscv/kernel/sys_hwprobe.c
+++ b/arch/riscv/kernel/sys_hwprobe.c
@@ -111,6 +111,7 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
 		EXT_KEY(ZTSO);
 		EXT_KEY(ZACAS);
 		EXT_KEY(ZICOND);
+		EXT_KEY(ZAWRS);
 
 		if (has_vector()) {
 			EXT_KEY(ZVBB);
-- 
2.44.0

[PATCH v2 5/6] KVM: riscv: Support guest wrs.nto

[Andrew Jones]

When a guest traps on wrs.nto, call kvm_vcpu_on_spin() to attempt
to yield to the lock holding VCPU. Also extend the KVM ISA extension
ONE_REG interface to allow KVM userspace to detect and enable the
Zawrs extension for the Guest/VM.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/kvm_host.h |  1 +
 arch/riscv/include/uapi/asm/kvm.h |  1 +
 arch/riscv/kvm/vcpu.c             |  1 +
 arch/riscv/kvm/vcpu_insn.c        | 15 +++++++++++++++
 arch/riscv/kvm/vcpu_onereg.c      |  2 ++
 5 files changed, 20 insertions(+)

diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
index 484d04a92fa6..e27c56e44783 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -69,6 +69,7 @@ struct kvm_vcpu_stat {
 	struct kvm_vcpu_stat_generic generic;
 	u64 ecall_exit_stat;
 	u64 wfi_exit_stat;
+	u64 wrs_exit_stat;
 	u64 mmio_exit_user;
 	u64 mmio_exit_kernel;
 	u64 csr_exit_user;
diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index b1c503c2959c..89ea06bd07c2 100644
--- a/arch/riscv/include/uapi/asm/kvm.h
+++ b/arch/riscv/include/uapi/asm/kvm.h
@@ -167,6 +167,7 @@ enum KVM_RISCV_ISA_EXT_ID {
 	KVM_RISCV_ISA_EXT_ZFA,
 	KVM_RISCV_ISA_EXT_ZTSO,
 	KVM_RISCV_ISA_EXT_ZACAS,
+	KVM_RISCV_ISA_EXT_ZAWRS,
 	KVM_RISCV_ISA_EXT_MAX,
 };
 
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index b5ca9f2e98ac..abcdc78671e0 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -25,6 +25,7 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = {
 	KVM_GENERIC_VCPU_STATS(),
 	STATS_DESC_COUNTER(VCPU, ecall_exit_stat),
 	STATS_DESC_COUNTER(VCPU, wfi_exit_stat),
+	STATS_DESC_COUNTER(VCPU, wrs_exit_stat),
 	STATS_DESC_COUNTER(VCPU, mmio_exit_user),
 	STATS_DESC_COUNTER(VCPU, mmio_exit_kernel),
 	STATS_DESC_COUNTER(VCPU, csr_exit_user),
diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
index ee7215f4071f..97dec18e6989 100644
--- a/arch/riscv/kvm/vcpu_insn.c
+++ b/arch/riscv/kvm/vcpu_insn.c
@@ -16,6 +16,9 @@
 #define INSN_MASK_WFI		0xffffffff
 #define INSN_MATCH_WFI		0x10500073
 
+#define INSN_MASK_WRS		0xffffffff
+#define INSN_MATCH_WRS		0x00d00073
+
 #define INSN_MATCH_CSRRW	0x1073
 #define INSN_MASK_CSRRW		0x707f
 #define INSN_MATCH_CSRRS	0x2073
@@ -203,6 +206,13 @@ static int wfi_insn(struct kvm_vcpu *vcpu, struct kvm_run *run, ulong insn)
 	return KVM_INSN_CONTINUE_NEXT_SEPC;
 }
 
+static int wrs_insn(struct kvm_vcpu *vcpu, struct kvm_run *run, ulong insn)
+{
+	vcpu->stat.wrs_exit_stat++;
+	kvm_vcpu_on_spin(vcpu, vcpu->arch.guest_context.sstatus & SR_SPP);
+	return KVM_INSN_CONTINUE_NEXT_SEPC;
+}
+
 struct csr_func {
 	unsigned int base;
 	unsigned int count;
@@ -378,6 +388,11 @@ static const struct insn_func system_opcode_funcs[] = {
 		.match = INSN_MATCH_WFI,
 		.func  = wfi_insn,
 	},
+	{
+		.mask  = INSN_MASK_WRS,
+		.match = INSN_MATCH_WRS,
+		.func  = wrs_insn,
+	},
 };
 
 static int system_opcode_insn(struct kvm_vcpu *vcpu, struct kvm_run *run,
diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
index f4a6124d25c9..67c5794af3b6 100644
--- a/arch/riscv/kvm/vcpu_onereg.c
+++ b/arch/riscv/kvm/vcpu_onereg.c
@@ -41,6 +41,7 @@ static const unsigned long kvm_isa_ext_arr[] = {
 	KVM_ISA_EXT_ARR(SVNAPOT),
 	KVM_ISA_EXT_ARR(SVPBMT),
 	KVM_ISA_EXT_ARR(ZACAS),
+	KVM_ISA_EXT_ARR(ZAWRS),
 	KVM_ISA_EXT_ARR(ZBA),
 	KVM_ISA_EXT_ARR(ZBB),
 	KVM_ISA_EXT_ARR(ZBC),
@@ -120,6 +121,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext)
 	case KVM_RISCV_ISA_EXT_SVINVAL:
 	case KVM_RISCV_ISA_EXT_SVNAPOT:
 	case KVM_RISCV_ISA_EXT_ZACAS:
+	case KVM_RISCV_ISA_EXT_ZAWRS:
 	case KVM_RISCV_ISA_EXT_ZBA:
 	case KVM_RISCV_ISA_EXT_ZBB:
 	case KVM_RISCV_ISA_EXT_ZBC:
-- 
2.44.0

[PATCH v2 6/6] KVM: riscv: selftests: Add Zawrs extension to get-reg-list test

[Andrew Jones]

KVM RISC-V allows the Zawrs extension for the Guest/VM, so add it
to the get-reg-list test.

Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
 tools/testing/selftests/kvm/riscv/get-reg-list.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
index b882b7b9b785..8c4c27bd4b88 100644
--- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
+++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
@@ -48,6 +48,7 @@ bool filter_reg(__u64 reg)
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVNAPOT:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVPBMT:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZACAS:
+	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZAWRS:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZBA:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZBB:
 	case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZBC:
@@ -413,6 +414,7 @@ static const char *isa_ext_single_id_to_str(__u64 reg_off)
 		KVM_ISA_EXT_ARR(SVNAPOT),
 		KVM_ISA_EXT_ARR(SVPBMT),
 		KVM_ISA_EXT_ARR(ZACAS),
+		KVM_ISA_EXT_ARR(ZAWRS),
 		KVM_ISA_EXT_ARR(ZBA),
 		KVM_ISA_EXT_ARR(ZBB),
 		KVM_ISA_EXT_ARR(ZBC),
@@ -936,6 +938,7 @@ KVM_ISA_EXT_SIMPLE_CONFIG(svinval, SVINVAL);
 KVM_ISA_EXT_SIMPLE_CONFIG(svnapot, SVNAPOT);
 KVM_ISA_EXT_SIMPLE_CONFIG(svpbmt, SVPBMT);
 KVM_ISA_EXT_SIMPLE_CONFIG(zacas, ZACAS);
+KVM_ISA_EXT_SIMPLE_CONFIG(zawrs, ZAWRS);
 KVM_ISA_EXT_SIMPLE_CONFIG(zba, ZBA);
 KVM_ISA_EXT_SIMPLE_CONFIG(zbb, ZBB);
 KVM_ISA_EXT_SIMPLE_CONFIG(zbc, ZBC);
@@ -991,6 +994,7 @@ struct vcpu_reg_list *vcpu_configs[] = {
 	&config_svnapot,
 	&config_svpbmt,
 	&config_zacas,
+	&config_zawrs,
 	&config_zba,
 	&config_zbb,
 	&config_zbc,
-- 
2.44.0