riscv: Apply Zawrs when available
Subsystems (5)
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.
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. Currently, it's not expected that M-mode will configure and handle exceptions for timeouts (so it's expected that mstatus.TW=0), but 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. We then switch from wrs.sto to wrs.nto, add hwprobe support (since the instructions are also usable from usermode), and finally teach KVM 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 based on kvm/queue and 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-v1/
[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/2e712b19b7bb78634199bf262e6a75e09e1c87d2
Thanks,
drew
Andrew Jones (4):
riscv: Prefer wrs.nto over wrs.sto
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 + arch/riscv/Kconfig | 13 +++ arch/riscv/include/asm/barrier.h | 87 +++++++++++++++++++ arch/riscv/include/asm/hwcap.h | 1 + arch/riscv/include/asm/kvm_host.h | 1 + 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 + 13 files changed, 132 insertions(+)
Signature · 1 line
2.44.0
PATCH 1/5 riscv: Add Zawrs support for spinlocks
From: Christoph M?llner <[email protected]>
The current RISC-V code uses the generic ticket lock implementation, that calls the macros smp_cond_load_relaxed() and smp_cond_load_acquire(). Currently, RISC-V uses the generic implementation of these macros. This patch introduces a RISC-V specific implementation, of these macros, that peels off the first loop iteration and modifies the waiting loop such, that it is possible to use the WRS.STO instruction of the Zawrs ISA extension to stall the CPU.
The resulting implementation of smp_cond_load_*() will only work for 32-bit or 64-bit types for RV64 and 32-bit types for RV32. This is caused by the restrictions of the LR instruction (RISC-V only has LR.W and LR.D). Compiler assertions guard this new restriction.
This patch uses the existing RISC-V ISA extension framework to detect the presence of Zawrs at run-time.
If available a NOP instruction will be replaced by WRS.NTO or WRS.STO.
The whole mechanism is gated by Kconfig setting, which defaults to Y.
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 | 82 ++++++++++++++++++++++++++++++++ arch/riscv/include/asm/hwcap.h | 1 + arch/riscv/kernel/cpufeature.c | 1 + 4 files changed, 97 insertions(+)
arch/riscv/Kconfig +13 -0
@@ -569,6 +569,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 + Enable the use of the Zawrs (wait for reservation set) extension + when available. + + The Zawrs extension instructions (wrs.nto and wrs.sto) are used for + more efficient busy waiting. + + If you don't know what to do here, say Y. + config TOOLCHAIN_HAS_ZBB bool default y
arch/riscv/include/asm/barrier.h +82 -0
105 lines not on this page · raw message
arch/riscv/include/asm/hwcap.h +1 -0
@@ -80,6 +80,7 @@#define RISCV_ISA_EXT_ZFA 71#define RISCV_ISA_EXT_ZTSO 72#define RISCV_ISA_EXT_ZACAS 73 +#define RISCV_ISA_EXT_ZAWRS 74#define RISCV_ISA_EXT_XLINUXENVCFG 127
arch/riscv/kernel/cpufeature.c +1 -0
@@ -271,6 +271,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 1/5 riscv: Add Zawrs support for spinlocks
arch/riscv/Kconfig config RISCV_ISA_V_PREEMPTIVE · line 581
Quoted 10 lines
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 + Enable the use of the Zawrs (wait for reservation set) extension + when available. + + The Zawrs extension instructions (wrs.nto and wrs.sto) are used for + more efficient busy waiting.
Maybe mention that this is about _power_-efficiency? In the discussion of the previous iteration, I suggested [1]:
The Zawrs extension defines a pair of instructions to be used in polling loops that allows a core to enter a low-power state and wait on a store to a memory location.
(from the Zawrs spec -- I remain open to review other suggestiongs).
arch/riscv/include/asm/barrier.h line 24
Quoted 2 lines
arch/riscv/include/asm/barrier.h +2
-0+#define ZAWRS_WRS_NTO ".long 0x00d00073" +#define ZAWRS_WRS_STO ".long 0x01d00073"
In the discussion of the previous iteration, you observed [2]:
I'd prefer we use insn-def.h to define instructions, rather than scatter .long's around, but since this instruction doesn't have any inputs, then I guess it's not so important to use insn-def.h.
So that "preference" doesn't apply to the instructions at stake? Or is not "important"? No real objections to barrier.h, trying to understand the rationale.
arch/riscv/include/asm/barrier.h line 35
Quoted 11 lines
arch/riscv/include/asm/barrier.h +9
-0+#define ALT_WRS_NTO () \ + __asm__ __volatile__ (ALTERNATIVE( \ + "nop\n", ZAWRS_WRS_NTO "\n", \ + 0, RISCV_ISA_EXT_ZAWRS, CONFIG_RISCV_ISA_ZAWRS)) +#define ALT_WRS_STO () \ + __asm__ __volatile__ (ALTERNATIVE( \ + "nop\n", ZAWRS_WRS_STO "\n", \ + 0, RISCV_ISA_EXT_ZAWRS, CONFIG_RISCV_ISA_ZAWRS)) +#define RISCV_FENCE (p, s) \ __asm__ __volatile__ ("fence " #p "," #s : : : "memory")
FYI, this hunk/patch conflicts with Eric's changes [3].
Quoted 13 lines
arch/riscv/include/asm/barrier.h +13
-0+#define ___smp_load_reservedN (attr, ptr) \ +({ \ + typeof(*ptr) ___p1; \ + \ + __asm__ __volatile__ ("lr." attr " %[p], %[c]\n" \ + : [p]"=&r" (___p1), [c]"+A"(*ptr)); \ + ___p1; \ +}) + +#define __smp_load_reserved_relaxed (ptr) \ +({ \ + typeof(*ptr) ___p1; \ + \
arch/riscv/include/asm/barrier.h do { \ · line 84
Quoted 10 lines
arch/riscv/include/asm/barrier.h +10
-0+if (sizeof (*ptr) ==sizeof (int )) \ + ___p1 =___smp_load_reservedN ("w" , ptr); \ +else if (sizeof (*ptr) ==sizeof (long )) \ + ___p1 =___smp_load_reservedN ("d" , ptr); \ +else \ +compiletime_assert (0 , \ +"Need type compatible with LR/SC instructions for " \ + __stringify(ptr)); \ + ___p1; \ +})
In the discussion of the previous iteration, you observed [2]:
It's more common to use a switch for these things, [...] something like the macros in arch/riscv/include/asm/cmpxchg.h. Can we stick with that pattern?
Along the same lines (#codingstyle), notice that ___smp_load_reservedN() would become one of the first uses of the asmSymbolicName syntax in the arch/riscv/ directory.
So again, why not stick to the common style? something's wrong with it?
Quoted 23 lines
arch/riscv/include/asm/barrier.h +23
-0+#define smp_cond_load_relaxed (ptr, cond_expr) \ +({ \ + typeof(ptr) __PTR = (ptr); \ + __unqual_scalar_typeof(*ptr) VAL; \ + \ + VAL = READ_ONCE(*__PTR); \ + if (!cond_expr) { \ + for (;;) { \ + VAL = __smp_load_reserved_relaxed(__PTR); \ + if (cond_expr) \ + break; \ + ALT_WRS_STO(); \ + } \ + } \ + (typeof(*ptr))VAL; \ +}) + +#define smp_cond_load_acquire (ptr, cond_expr) \ +({ \ + typeof(ptr) __PTR = (ptr); \ + __unqual_scalar_typeof(*ptr) VAL; \ + \ + VAL = smp_load_acquire(__PTR); \
arch/riscv/include/asm/barrier.h do { \ · line 154
Quoted 10 lines
arch/riscv/include/asm/barrier.h +10
-0+if (!cond_expr) { \ +for (;;) { \ +VAL =__smp_load_reserved_acquire (__PTR); \ +if (cond_expr) \ +break ; \ +ALT_WRS_STO (); \ + } \ + } \ + (typeof(*ptr))VAL; \ +})
In the discussion of the previous iteration, you observed [2]:
I guess this peeling off of the first iteration is because it's expected that the load generated by READ_ONCE() is more efficient than lr.w/d? If we're worried about unnecessary use of lr.w/d, then shouldn't we look for a solution that doesn't issue those instructions when we don't have the Zawrs extension?
To which Palmer replied (apparently, agreeing with your remarks) [4]:
I haven't looked at the patch, but I'd expect we NOP out the whole LR/WRS sequence? I don't remember any reason to have the load reservation without the WRS, [...]
Unfortunately, this submission makes no mention to those comments and, more importantly, to the considerations/tradeoffs which have led you to submit different changes. In submitting-patches.rst's words,
Review comments or questions that do not lead to a code change should almost certainly bring about a comment or changelog entry so that the next reviewer better understands what is going on.
Andrea
P.S. BTW, not too far from the previous recommendation/paragraph is:
When sending a next version, [...] Notify people that commented on your patch about new versions by adding them to the patches CC list.
[1] https://lore.kernel.org/lkml/ZTE7eUyrb8+J+ORB at andrea
[2] https://lore.kernel.org/lkml/20230524-35efcabbbfd6a1ef83865fb4 at orel
[3] https://lore.kernel.org/lkml/20240217131206.3667544-1-ericchancf at google.com
[4] https://lore.kernel.org/lkml/mhng-d92f84d8-03db-4fb1-93c3-0d5bfbe7a796 at palmer-ri-x1c9a
PATCH 1/5 riscv: Add Zawrs support for spinlocks
Hi Andrea,
I actually just forgot to re-review the thread and make changes... I grabbed the patch and then focused on experimenting and testing it's impact on guests. I guess I should have started with applying the thread comments, because I ended up forgetting to return to them before posting...
Quoted 14 lines · Andrea Parri
arch/riscv/Kconfig config RISCV_ISA_V_PREEMPTIVE · line 581
Quoted 10 lines
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 + Enable the use of the Zawrs (wait for reservation set) extension + when available. + + The Zawrs extension instructions (wrs.nto and wrs.sto) are used for + more efficient busy waiting.Maybe mention that this is about _power_-efficiency? In the discussion of the previous iteration, I suggested [1]:
Quoted 5 lines
The Zawrs extension defines a pair of instructions to be used in polling loops that allows a core to enter a low-power state and wait on a store to a memory location.
(from the Zawrs spec -- I remain open to review other suggestiongs).
Sounds good to me. I'll make a change like that for v2.
Quoted 7 lines
arch/riscv/include/asm/barrier.h line 24
Quoted 4 lines
arch/riscv/include/asm/barrier.h +2
-0+#define ZAWRS_WRS_NTO ".long 0x00d00073" +#define ZAWRS_WRS_STO ".long 0x01d00073"In the discussion of the previous iteration, you observed [2]:
Quoted 7 lines
I'd prefer we use
insn-def.hto define instructions, rather than scatter .long's around, but since this instruction doesn't have any inputs, then I guess it's not so important to useinsn-def.h.So that "preference" doesn't apply to the instructions at stake? Or is not "important"? No real objections to
barrier.h, trying to understand the rationale.
Besides having simply forgotten to address my own comment, in this case I can live with the .long since we're at least not hard coding an operand register. However, since I prefer '.4byte', I'll change to that.
Quoted 15 lines
arch/riscv/include/asm/barrier.h line 35
Quoted 13 lines
arch/riscv/include/asm/barrier.h +9
-0+#define ALT_WRS_NTO () \ + __asm__ __volatile__ (ALTERNATIVE( \ + "nop\n", ZAWRS_WRS_NTO "\n", \ + 0, RISCV_ISA_EXT_ZAWRS, CONFIG_RISCV_ISA_ZAWRS)) +#define ALT_WRS_STO () \ + __asm__ __volatile__ (ALTERNATIVE( \ + "nop\n", ZAWRS_WRS_STO "\n", \ + 0, RISCV_ISA_EXT_ZAWRS, CONFIG_RISCV_ISA_ZAWRS)) +#define RISCV_FENCE (p, s) \ __asm__ __volatile__ ("fence " #p "," #s : : : "memory")FYI, this hunk/patch conflicts with Eric's changes [3].
Thanks, I had mentally noted that series as a possible conflict, but then forgot to try basing on it to see if it would be a problem.
Quoted 32 lines
Quoted 15 lines
arch/riscv/include/asm/barrier.h +13
-0+#define ___smp_load_reservedN (attr, ptr) \ +({ \ + typeof(*ptr) ___p1; \ + \ + __asm__ __volatile__ ("lr." attr " %[p], %[c]\n" \ + : [p]"=&r" (___p1), [c]"+A"(*ptr)); \ + ___p1; \ +}) + +#define __smp_load_reserved_relaxed (ptr) \ +({ \ + typeof(*ptr) ___p1; \ + \arch/riscv/include/asm/barrier.h do { \ · line 84
Quoted 10 lines
arch/riscv/include/asm/barrier.h +10
-0+if (sizeof (*ptr) ==sizeof (int )) \ + ___p1 =___smp_load_reservedN ("w" , ptr); \ +else if (sizeof (*ptr) ==sizeof (long )) \ + ___p1 =___smp_load_reservedN ("d" , ptr); \ +else \ +compiletime_assert (0 , \ +"Need type compatible with LR/SC instructions for " \ + __stringify(ptr)); \ + ___p1; \ +})In the discussion of the previous iteration, you observed [2]:
It's more common to use a switch for these things, [...] something like the macros in
arch/riscv/include/asm/cmpxchg.h. Can we stick with that pattern?
Quoted 5 lines
Along the same lines (#codingstyle), notice that
___smp_load_reservedN()would become one of the first uses of the asmSymbolicName syntax in thearch/riscv/directory.So again, why not stick to the common style? something's wrong with it?
For v2, I'll switch to the style that I recommended :-)
Quoted 44 lines
Quoted 25 lines
arch/riscv/include/asm/barrier.h +23
-0+#define smp_cond_load_relaxed (ptr, cond_expr) \ +({ \ + typeof(ptr) __PTR = (ptr); \ + __unqual_scalar_typeof(*ptr) VAL; \ + \ + VAL = READ_ONCE(*__PTR); \ + if (!cond_expr) { \ + for (;;) { \ + VAL = __smp_load_reserved_relaxed(__PTR); \ + if (cond_expr) \ + break; \ + ALT_WRS_STO(); \ + } \ + } \ + (typeof(*ptr))VAL; \ +}) + +#define smp_cond_load_acquire (ptr, cond_expr) \ +({ \ + typeof(ptr) __PTR = (ptr); \ + __unqual_scalar_typeof(*ptr) VAL; \ + \ + VAL = smp_load_acquire(__PTR); \arch/riscv/include/asm/barrier.h do { \ · line 154
Quoted 10 lines
arch/riscv/include/asm/barrier.h +10
-0+if (!cond_expr) { \ +for (;;) { \ +VAL =__smp_load_reserved_acquire (__PTR); \ +if (cond_expr) \ +break ; \ +ALT_WRS_STO (); \ + } \ + } \ + (typeof(*ptr))VAL; \ +})In the discussion of the previous iteration, you observed [2]:
I guess this peeling off of the first iteration is because it's expected that the load generated by
READ_ONCE()is more efficient than lr.w/d? If we're worried about unnecessary use of lr.w/d, then shouldn't we look for a solution that doesn't issue those instructions when we don't have the Zawrs extension?
Quoted 5 lines
To which Palmer replied (apparently, agreeing with your remarks) [4]:
I haven't looked at the patch, but I'd expect we NOP out the whole LR/WRS sequence? I don't remember any reason to have the load reservation without the WRS, [...]
For v2, I'll look closer at this to decide what to do.
Quoted 8 lines
Unfortunately, this submission makes no mention to those comments and, more importantly, to the considerations/tradeoffs which have led you to submit different changes. In submitting-patches.rst's words,
Review comments or questions that do not lead to a code change should almost certainly bring about a comment or changelog entry so that the next reviewer better understands what is going on.
Eh, there's nothing mysterious going on here. It was just me being forgetful... And, I even forgot to write a changelog entry explaining that I forgot :-)
Quoted 8 lines
Andrea
P.S. BTW, not too far from the previous recommendation/paragraph is:When sending a next version, [...] Notify people that commented on your patch about new versions by adding them to the patches CC list.
Yeah, had I returned to the old thread for the re-review, I would have seen your comments again and then remembered to CC you.
Thanks,
drew
Quoted 5 lines
[1] https://lore.kernel.org/lkml/ZTE7eUyrb8+J+ORB at andrea
[2] https://lore.kernel.org/lkml/20230524-35efcabbbfd6a1ef83865fb4 at orel
[3] https://lore.kernel.org/lkml/20240217131206.3667544-1-ericchancf at google.com
[4] https://lore.kernel.org/lkml/mhng-d92f84d8-03db-4fb1-93c3-0d5bfbe7a796 at palmer-ri-x1c9a
PATCH 2/5 riscv: Prefer wrs.nto over wrs.sto
In reply to Andrew JonesWhen running as a guest we'd like to trap to the host while waiting in order to give the hypervisor a chance to schedule the lock holding VCPU. Unlike wrs.sto, wrs.nto may be configured to raise an exception when its duration expires, so use it instead. Protect ourselves from unhandled exceptions with _ASM_EXTABLE in case the higher privileged level configures wrs.nto to raise exceptions, but then doesn't handle them.
--- arch/riscv/include/asm/barrier.h | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)
arch/riscv/include/asm/barrier.h +10 -5
@@ -13,6 +13,7 @@#ifndef __ASSEMBLY__#include <linux/compiler.h> #include <asm/alternative-macros.h> +#include <asm/asm-extable.h> #include <asm/hwcap.h> #include <asm/rwonce.h>
@@ -22,10 +23,14 @@#define ZAWRS_WRS_NTO ".long 0x00d00073"#define ZAWRS_WRS_STO ".long 0x01d00073" +#define __ALT_WRS_NTO \ + ALTERNATIVE("nop\n", ZAWRS_WRS_NTO "\n", \ + 0, RISCV_ISA_EXT_ZAWRS, CONFIG_RISCV_ISA_ZAWRS)#define ALT_WRS_NTO () \ - __asm__ __volatile__ (ALTERNATIVE( \ - "nop\n", ZAWRS_WRS_NTO "\n", \ - 0, RISCV_ISA_EXT_ZAWRS, CONFIG_RISCV_ISA_ZAWRS)) + __asm__ __volatile__ ( \ + "1: " __ALT_WRS_NTO "\n" \ + "2:\n" \ + _ASM_EXTABLE(1b, 2b))#define ALT_WRS_STO () \ __asm__ __volatile__ (ALTERNATIVE( \ "nop\n", ZAWRS_WRS_STO "\n", \
@@ -130,7 +135,7 @@ do { \VAL =__smp_load_reserved_relaxed (__PTR); \if (cond_expr) \break ; \ -ALT_WRS_STO (); \ +ALT_WRS_NTO (); \ } \ } \ (typeof(*ptr))VAL; \
@@ -147,7 +152,7 @@ do { \VAL =__smp_load_reserved_acquire (__PTR); \if (cond_expr) \break ; \ -ALT_WRS_STO (); \ +ALT_WRS_NTO (); \ } \ } \ (typeof(*ptr))VAL; \
Signature · 1 line
2.44.0
PATCH 3/5 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 + defined in version 1.0.1 of the RISC-V Wait-on-Reservation-Set (Zawrs) + extension specification. + * :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 4/5 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 5/5 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
cover riscv: Apply Zawrs when available
In reply to Andrew JonesQuoted 49 lines · 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.
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. Currently, it's not expected that M-mode will configure and handle exceptions for timeouts (so it's expected that mstatus.TW=0), but 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. We then switch from wrs.sto to wrs.nto, add hwprobe support (since the instructions are also usable from usermode), and finally teach KVM 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 based on kvm/queue and 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-v1/
[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/2e712b19b7bb78634199bf262e6a75e09e1c87d2Thanks,
drew
Quoted 8 lines
Andrew Jones (4):
riscv: Prefer wrs.nto over wrs.sto
riscv: hwprobe: export Zawrs ISA extension
KVM: riscv: Support guest wrs.nto
KVM: riscv: selftests: Add Zawrs extension to get-reg-list testChristoph M??llner (1):
riscv: Add Zawrs support for spinlocks
I see I also forgot to add the patch for the entry in Documentation/devicetree/bindings/riscv/extensions.yaml
Will do for v2.
Thanks,
drew