riscv: Apply Zawrs when available
Subsystems (6)
v2 cover riscv: Apply Zawrs when available
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 at 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(-)
Signature · 1 line
2.44.0
PATCH v2 1/6 riscv: Provide a definition for 'pause'
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.
--- 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(-)
arch/riscv/Kconfig +0 -7
@@ -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
arch/riscv/Makefile +0 -3
@@ -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/')
arch/riscv/include/asm/insn-def.h +2 -0
@@ -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 */
arch/riscv/include/asm/vdso/processor.h +2 -6
@@ -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 (); }
Signature · 1 line
2.44.0
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
In reply to Andrew JonesAdd description for the Zawrs (Wait-on-Reservation-Set) ISA extension which was ratified in commit 98918c844281 of riscv-isa-manual.
--- .../devicetree/bindings/riscv/extensions.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)
Documentation/devicetree/bindings/riscv/extensions.yaml +12 -0
@@ -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
Signature · 1 line
2.44.0
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 13 lines · Andrew Jones
Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension which was ratified in commit
98918c844281of riscv-isa-manual.Signed-off-by Andrew Jones <[email protected]>--- .../devicetree/bindings/riscv/extensions.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)Documentation/devicetree/bindings/riscv/extensions.yaml +0
-0
Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 185
Quoted 9 lines
Documentation/devicetree/bindings/riscv/extensions.yaml +6
-0@@ -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...
Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 190
Quoted 6 lines
Linux assumes that WRS.NTO will
Documentation/devicetree/bindings/riscv/extensions.yaml +5
-0+ 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20240419/7cc99d58/attachment.sig>
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 38 lines · Conor Dooley
Quoted 13 lines · Andrew Jones
Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension which was ratified in commit
98918c844281of riscv-isa-manual.Signed-off-by Andrew Jones <[email protected]>--- .../devicetree/bindings/riscv/extensions.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)Documentation/devicetree/bindings/riscv/extensions.yaml +0
-0Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 185
Quoted 9 lines
Documentation/devicetree/bindings/riscv/extensions.yaml +6
-0@@ -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...
Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 190
Quoted 6 lines
Linux assumes that WRS.NTO will
Documentation/devicetree/bindings/riscv/extensions.yaml +5
-0+ 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.
Quoted 5 lines
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
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 45 lines · Andrew Jones
Quoted 38 lines · Conor Dooley
Quoted 13 lines · Andrew Jones
Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension which was ratified in commit
98918c844281of riscv-isa-manual.Signed-off-by Andrew Jones <[email protected]>--- .../devicetree/bindings/riscv/extensions.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)Documentation/devicetree/bindings/riscv/extensions.yaml +0
-0Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 185
Quoted 9 lines
Documentation/devicetree/bindings/riscv/extensions.yaml +6
-0@@ -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...
Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 190
Quoted 6 lines
Linux assumes that WRS.NTO will
Documentation/devicetree/bindings/riscv/extensions.yaml +5
-0+ 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.
Quoted 5 lines
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.
Quoted 5 lines
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20240419/1541caf5/attachment.sig>
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 52 lines · Conor Dooley
Quoted 45 lines · Andrew Jones
Quoted 38 lines · Conor Dooley
Quoted 13 lines · Andrew Jones
Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension which was ratified in commit
98918c844281of riscv-isa-manual.Signed-off-by Andrew Jones <[email protected]>--- .../devicetree/bindings/riscv/extensions.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)Documentation/devicetree/bindings/riscv/extensions.yaml +0
-0Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 185
Quoted 9 lines
Documentation/devicetree/bindings/riscv/extensions.yaml +6
-0@@ -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...
Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 190
Quoted 6 lines
Linux assumes that WRS.NTO will
Documentation/devicetree/bindings/riscv/extensions.yaml +5
-0+ 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.
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 59 lines · Charlie Jenkins
Quoted 52 lines · Conor Dooley
Quoted 45 lines · Andrew Jones
Quoted 38 lines · Conor Dooley
Quoted 13 lines · Andrew Jones
Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension which was ratified in commit
98918c844281of riscv-isa-manual.Signed-off-by Andrew Jones <[email protected]>--- .../devicetree/bindings/riscv/extensions.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)Documentation/devicetree/bindings/riscv/extensions.yaml +0
-0Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 185
Quoted 9 lines
Documentation/devicetree/bindings/riscv/extensions.yaml +6
-0@@ -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...
Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 190
Quoted 6 lines
Linux assumes that WRS.NTO will
Documentation/devicetree/bindings/riscv/extensions.yaml +5
-0+ 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.
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 8 lines
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
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 79 lines · Andrew Jones
Quoted 59 lines · Charlie Jenkins
Quoted 52 lines · Conor Dooley
Quoted 45 lines · Andrew Jones
Quoted 38 lines · Conor Dooley
Quoted 13 lines · Andrew Jones
Add description for the Zawrs (Wait-on-Reservation-Set) ISA extension which was ratified in commit
98918c844281of riscv-isa-manual.Signed-off-by Andrew Jones <[email protected]>--- .../devicetree/bindings/riscv/extensions.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)Documentation/devicetree/bindings/riscv/extensions.yaml +0
-0Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 185
Quoted 9 lines
Documentation/devicetree/bindings/riscv/extensions.yaml +6
-0@@ -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...
Documentation/devicetree/bindings/riscv/extensions.yaml properties: · line 190
Quoted 6 lines
Linux assumes that WRS.NTO will
Documentation/devicetree/bindings/riscv/extensions.yaml +5
-0+ 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.
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 5 lines
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.
Quoted 8 lines
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 pollanything_we_want()at a "reasonable" frequency.
Quoted 7 lines
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
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 2 lines · Charlie Jenkins
Quoted 1 line · Andrew Jones
On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
...
Quoted 27 lines
Quoted 19 lines
Quoted 8 lines
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 pollanything_we_want()at a "reasonable" frequency.Quoted 7 lines
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
Quoted 7 lines
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.
Quoted 3 lines
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.
Quoted 8 lines
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.
Quoted 20 lines
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()inkernel/locking/qspinlock.cval = atomic_cond_read_relaxed(&lock->val,(VAL != _Q_PENDING_VAL) || !cnt--);The first condition will only become true if
lock->valchanges which should invalidate the reservation. !cnt-- on the otherhand is a counter of the number of loops that happen under-the-hood inatomic_cond_read_relaxed. This seems like an abuse of the function and could be factored out into a new bounded-iterationcond_readmacro.2.
#osq_lock()inkernel/locking/osq_lock.c
if (smp_cond_load_relaxed(&node->locked, VAL ||need_resched()||
Quoted 8 lines
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_preemptedto 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 inarch/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.
Quoted 9 lines
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()indrivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.csmp_cond_load_relaxed(cmd, !VAL || (ret = queue_poll(&qp)));
Quoted 8 lines
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
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 2 lines · Andrew Jones
Maybe I'm attempting to document it in the wrong place though. Maybe this is more of a
Documentation/arch/riscv/boot.rsttype 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20240423/a1265cf9/attachment.sig>
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
In reply to Andrew JonesQuoted 52 lines · Andrew Jones
Quoted 2 lines · Charlie Jenkins
Quoted 1 line · Andrew Jones
On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
...
Quoted 27 lines
Quoted 19 lines
Quoted 8 lines
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 pollanything_we_want()at a "reasonable" frequency.Quoted 7 lines
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,
drewQuoted 7 lines
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.
Quoted 3 lines
With the concern being that it is possible for "implementation-specific bounded time limit" to be infinite/never times out,
Quoted 10 lines
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?
Quoted 9 lines
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 51 lines
Quoted 20 lines
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()inkernel/locking/qspinlock.cval = atomic_cond_read_relaxed(&lock->val,(VAL != _Q_PENDING_VAL) || !cnt--);The first condition will only become true if
lock->valchanges which should invalidate the reservation. !cnt-- on the otherhand is a counter of the number of loops that happen under-the-hood inatomic_cond_read_relaxed. This seems like an abuse of the function and could be factored out into a new bounded-iterationcond_readmacro.2.
#osq_lock()inkernel/locking/osq_lock.c
if (smp_cond_load_relaxed(&node->locked, VAL ||need_resched()||Quoted 8 lines
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_preemptedto 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 inarch/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.Quoted 9 lines
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()indrivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.csmp_cond_load_relaxed(cmd, !VAL || (ret = queue_poll(&qp)));Quoted 8 lines
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.
Quoted 4 lines
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 removevcpu_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.
Quoted 9 lines
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.rsttype 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.
Quoted 2 lines
Thanks,
drew
- Charlie
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 136 lines · Charlie Jenkins
Quoted 52 lines · Andrew Jones
Quoted 2 lines · Charlie Jenkins
Quoted 1 line · Andrew Jones
On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
...
Quoted 27 lines
Quoted 19 lines
Quoted 8 lines
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 pollanything_we_want()at a "reasonable" frequency.Quoted 7 lines
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,
drewQuoted 7 lines
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.
Quoted 3 lines
With the concern being that it is possible for "implementation-specific bounded time limit" to be infinite/never times out,
Quoted 10 lines
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?
Quoted 9 lines
Quoted 8 lines
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.
Quoted 6 lines
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.
Quoted 51 lines
Quoted 20 lines
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()inkernel/locking/qspinlock.cval = atomic_cond_read_relaxed(&lock->val,(VAL != _Q_PENDING_VAL) || !cnt--);The first condition will only become true if
lock->valchanges which should invalidate the reservation. !cnt-- on the otherhand is a counter of the number of loops that happen under-the-hood inatomic_cond_read_relaxed. This seems like an abuse of the function and could be factored out into a new bounded-iterationcond_readmacro.2.
#osq_lock()inkernel/locking/osq_lock.c
if (smp_cond_load_relaxed(&node->locked, VAL ||need_resched()||Quoted 8 lines
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_preemptedto 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 inarch/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.Quoted 17 lines
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()indrivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.csmp_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.
Quoted 8 lines
Quoted 4 lines
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 removevcpu_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
Quoted 21 lines
Quoted 10 lines
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.rsttype 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.
Quoted 2 lines
Thanks,
drew- Charlie
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 65 lines · Charlie Jenkins
Quoted 64 lines · Charlie Jenkins
Quoted 62 lines · Andrew Jones
Quoted 2 lines · Charlie Jenkins
Quoted 1 line · Andrew Jones
On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
...
Quoted 27 lines
Quoted 19 lines
Quoted 8 lines
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 pollanything_we_want()at a "reasonable" frequency.Quoted 7 lines
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,
drewQuoted 7 lines
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.
Quoted 3 lines
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.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 7 lines
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
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 89 lines · On Wed, Apr 24, 2024 at 9:34?AM Andrew Jones <[email protected]> wrote:
Quoted 65 lines · Charlie Jenkins
Quoted 64 lines · Charlie Jenkins
Quoted 62 lines · Andrew Jones
Quoted 2 lines · Charlie Jenkins
Quoted 1 line · Andrew Jones
On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
...
Quoted 27 lines
Quoted 19 lines
Quoted 8 lines
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 pollanything_we_want()at a "reasonable" frequency.Quoted 7 lines
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,
drewQuoted 7 lines
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.
Quoted 3 lines
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.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 7 lines
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.
Quoted 7 lines
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
PATCH v2 2/6 dt-bindings: riscv: Add Zawrs ISA extension description
Quoted 98 lines · Christoph M?llner
Quoted 89 lines · On Wed, Apr 24, 2024 at 9:34?AM Andrew Jones <[email protected]> wrote:
Quoted 65 lines · Charlie Jenkins
Quoted 64 lines · Charlie Jenkins
Quoted 62 lines · Andrew Jones
Quoted 2 lines · Charlie Jenkins
Quoted 1 line · Andrew Jones
On Fri, Apr 19, 2024 at 12:40:01PM -0400, Charlie Jenkins wrote:
...
Quoted 27 lines
Quoted 19 lines
Quoted 8 lines
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 pollanything_we_want()at a "reasonable" frequency.Quoted 7 lines
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,
drewQuoted 7 lines
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.
Quoted 3 lines
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.
Quoted 2 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 7 lines
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.Quoted 7 lines
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.
Quoted 4 lines
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.
Quoted 8 lines
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
In reply to Andrew JonesFrom: Christoph M?llner <[email protected]>
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
--- 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(-)
arch/riscv/Kconfig +13 -0
@@ -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
arch/riscv/include/asm/barrier.h +30 -15
63 lines not on this page · raw message
arch/riscv/include/asm/cmpxchg.h +51 -0
62 lines not on this page · raw message
arch/riscv/include/asm/hwcap.h +1 -0
@@ -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
arch/riscv/include/asm/insn-def.h +2 -0
@@ -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 */
arch/riscv/kernel/cpufeature.c +1 -0
@@ -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),
Signature · 1 line
2.44.0
PATCH v2 3/6 riscv: Add Zawrs support for spinlocks
arch/riscv/Kconfig config RISCV_ISA_V_PREEMPTIVE · line 590
Quoted 10 lines · Andrew Jones
arch/riscv/Kconfig +10
-0+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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20240419/555241cd/attachment.sig>
PATCH v2 3/6 riscv: Add Zawrs support for spinlocks
In reply to Andrew JonesQuoted 21 lines · Andrew Jones
From: Christoph M??llner <[email protected]>
RISC-V code uses the generic ticket lock implementation, which calls the macros
smp_cond_load_relaxed()andsmp_cond_load_acquire(). Introduce a RISC-V specific implementation ofsmp_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 onsmp_cond_load_relaxed()andsmp_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.adocSigned-off-by Christoph M??llner <[email protected]>Co-developed-by Andrew Jones <[email protected]>Signed-off-by Andrew Jones <[email protected]>
Quoted 8 lines
--- 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 at kernel.org/ ).
But other than that, this LGTM. One nit below.
Quoted 26 lines
arch/riscv/include/asm/barrier.h +4
-15-#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); \
arch/riscv/include/asm/barrier.h do { \ · line 72
Quoted 10 lines
arch/riscv/include/asm/barrier.h +10
-0+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
PATCH v2 3/6 riscv: Add Zawrs support for spinlocks
Quoted 31 lines · Andrea Parri
Quoted 21 lines · Andrew Jones
From: Christoph M??llner <[email protected]>
RISC-V code uses the generic ticket lock implementation, which calls the macros
smp_cond_load_relaxed()andsmp_cond_load_acquire(). Introduce a RISC-V specific implementation ofsmp_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 onsmp_cond_load_relaxed()andsmp_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.adocSigned-off-by Christoph M??llner <[email protected]>Co-developed-by Andrew Jones <[email protected]>Signed-off-by Andrew Jones <[email protected]>Quoted 8 lines
--- 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(-)
Quoted 3 lines
Doesn't apply to riscv/for-next (due to, AFAIU,
https://lore.kernel.org/all/171275883330.18495.10110341843571163280.git-patchwork-notify at 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.
Quoted 26 lines
But other than that, this LGTM. One nit below.
Quoted 22 lines
arch/riscv/include/asm/barrier.h +0
-15-#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)
Quoted 16 lines
Quoted 4 lines
arch/riscv/include/asm/barrier.h +4
-0+#define __smp_store_release (p, v) \ +do { \ + compiletime_assert_atomic_type(*p); \ + RISCV_FENCE(rw, w); \arch/riscv/include/asm/barrier.h do { \ · line 72
Quoted 10 lines
arch/riscv/include/asm/barrier.h +10
-0+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
In reply to Andrew JonesExport Zawrs ISA extension through hwprobe.
--- 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(+)
Documentation/arch/riscv/hwprobe.rst +4 -0
@@ -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.
arch/riscv/include/uapi/asm/hwprobe.h +1 -0
@@ -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)
arch/riscv/kernel/sys_hwprobe.c +1 -0
@@ -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 );
Signature · 1 line
2.44.0
PATCH v2 5/6 KVM: riscv: Support guest wrs.nto
In reply to Andrew JonesWhen 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.
--- 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(+)
arch/riscv/include/asm/kvm_host.h +1 -0
@@ -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;
arch/riscv/include/uapi/asm/kvm.h +1 -0
@@ -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, };
arch/riscv/kvm/vcpu.c +1 -0
@@ -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),
arch/riscv/kvm/vcpu_insn.c +15 -0
@@ -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,
arch/riscv/kvm/vcpu_onereg.c +2 -0
@@ -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 :
Signature · 1 line
2.44.0
PATCH v2 6/6 KVM: riscv: selftests: Add Zawrs extension to get-reg-list test
In reply to Andrew JonesKVM RISC-V allows the Zawrs extension for the Guest/VM, so add it to the get-reg-list test.
--- tools/testing/selftests/kvm/riscv/get-reg-list.c | 4 ++++ 1 file changed, 4 insertions(+)
tools/testing/selftests/kvm/riscv/get-reg-list.c +4 -0
@@ -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,
Signature · 1 line
2.44.0