* [PATCH 1/5] riscv: Add Zawrs support for spinlocks
2024-03-15 13:40 [PATCH 0/5] riscv: Apply Zawrs when available Andrew Jones
@ 2024-03-15 13:40 ` Andrew Jones
2024-03-16 11:36 ` Andrea Parri
2024-03-15 13:40 ` [PATCH 2/5] riscv: Prefer wrs.nto over wrs.sto Andrew Jones
` (4 subsequent siblings)
5 siblings, 1 reply; 9+ messages in thread
From: Andrew Jones @ 2024-03-15 13:40 UTC (permalink / raw)
To: linux-riscv, kvm-riscv
Cc: paul.walmsley, palmer, aou, conor.dooley, anup, atishp,
christoph.muellner, heiko, charlie, David.Laight, Heiko Stuebner
From: Christoph Müllner <christoph.muellner@vrull.eu>
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
Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
[rebase, update to review comments]
Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
[rebase, move ALT_WRS* to barrier.h]
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
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(+)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index e3142ce531a0..2c296113aeb1 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -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
diff --git a/arch/riscv/include/asm/barrier.h b/arch/riscv/include/asm/barrier.h
index 110752594228..93b3f572d643 100644
--- a/arch/riscv/include/asm/barrier.h
+++ b/arch/riscv/include/asm/barrier.h
@@ -11,11 +11,26 @@
#define _ASM_RISCV_BARRIER_H
#ifndef __ASSEMBLY__
+#include <linux/compiler.h>
+#include <asm/alternative-macros.h>
+#include <asm/hwcap.h>
+#include <asm/rwonce.h>
#define nop() __asm__ __volatile__ ("nop")
#define __nops(n) ".rept " #n "\nnop\n.endr\n"
#define nops(n) __asm__ __volatile__ (__nops(n))
+#define ZAWRS_WRS_NTO ".long 0x00d00073"
+#define ZAWRS_WRS_STO ".long 0x01d00073"
+#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")
@@ -44,6 +59,39 @@ do { \
___p1; \
})
+#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; \
+ \
+ 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; \
+})
+
+#define __smp_load_reserved_acquire(ptr) \
+({ \
+ typeof(*ptr) ___p1; \
+ \
+ ___p1 = __smp_load_reserved_relaxed(ptr); \
+ 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
@@ -71,6 +119,40 @@ do { \
*/
#define smp_mb__after_spinlock() RISCV_FENCE(iorw,iorw)
+#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); \
+ if (!cond_expr) { \
+ for (;;) { \
+ VAL = __smp_load_reserved_acquire(__PTR); \
+ if (cond_expr) \
+ break; \
+ ALT_WRS_STO(); \
+ } \
+ } \
+ (typeof(*ptr))VAL; \
+})
+
#include <asm-generic/barrier.h>
#endif /* __ASSEMBLY__ */
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 1f2d2599c655..eac7214a4bd0 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -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
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 79a5a35fab96..0e3c79094b07 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -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),
--
2.44.0
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 1/5] riscv: Add Zawrs support for spinlocks
2024-03-15 13:40 ` [PATCH 1/5] riscv: Add Zawrs support for spinlocks Andrew Jones
@ 2024-03-16 11:36 ` Andrea Parri
2024-03-16 17:17 ` Andrew Jones
0 siblings, 1 reply; 9+ messages in thread
From: Andrea Parri @ 2024-03-16 11:36 UTC (permalink / raw)
To: Andrew Jones
Cc: linux-riscv, kvm-riscv, paul.walmsley, palmer, aou, conor.dooley,
anup, atishp, christoph.muellner, heiko, charlie, David.Laight,
Heiko Stuebner
> +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).
> +#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.
> +#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].
> +#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; \
> + \
> + 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?
> +#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); \
> + 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@andrea
[2] https://lore.kernel.org/lkml/20230524-35efcabbbfd6a1ef83865fb4@orel
[3] https://lore.kernel.org/lkml/20240217131206.3667544-1-ericchancf@google.com
[4] https://lore.kernel.org/lkml/mhng-d92f84d8-03db-4fb1-93c3-0d5bfbe7a796@palmer-ri-x1c9a
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 1/5] riscv: Add Zawrs support for spinlocks
2024-03-16 11:36 ` Andrea Parri
@ 2024-03-16 17:17 ` Andrew Jones
0 siblings, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2024-03-16 17:17 UTC (permalink / raw)
To: Andrea Parri
Cc: linux-riscv, kvm-riscv, paul.walmsley, palmer, aou, conor.dooley,
anup, atishp, christoph.muellner, heiko, charlie, David.Laight,
Heiko Stuebner
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...
On Sat, Mar 16, 2024 at 12:36:23PM +0100, Andrea Parri wrote:
> > +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).
Sounds good to me. I'll make a change like that for v2.
>
>
> > +#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.
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.
>
>
> > +#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.
>
>
> > +#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; \
> > + \
> > + 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?
For v2, I'll switch to the style that I recommended :-)
>
>
> > +#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); \
> > + 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, [...]
For v2, I'll look closer at this to decide what to do.
>
> 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 :-)
>
> 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
>
> [1] https://lore.kernel.org/lkml/ZTE7eUyrb8+J+ORB@andrea
> [2] https://lore.kernel.org/lkml/20230524-35efcabbbfd6a1ef83865fb4@orel
> [3] https://lore.kernel.org/lkml/20240217131206.3667544-1-ericchancf@google.com
> [4] https://lore.kernel.org/lkml/mhng-d92f84d8-03db-4fb1-93c3-0d5bfbe7a796@palmer-ri-x1c9a
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/5] riscv: Prefer wrs.nto over wrs.sto
2024-03-15 13:40 [PATCH 0/5] riscv: Apply Zawrs when available Andrew Jones
2024-03-15 13:40 ` [PATCH 1/5] riscv: Add Zawrs support for spinlocks Andrew Jones
@ 2024-03-15 13:40 ` Andrew Jones
2024-03-15 13:40 ` [PATCH 3/5] riscv: hwprobe: export Zawrs ISA extension Andrew Jones
` (3 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2024-03-15 13:40 UTC (permalink / raw)
To: linux-riscv, kvm-riscv
Cc: paul.walmsley, palmer, aou, conor.dooley, anup, atishp,
christoph.muellner, heiko, charlie, David.Laight
When 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.
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
arch/riscv/include/asm/barrier.h | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/riscv/include/asm/barrier.h b/arch/riscv/include/asm/barrier.h
index 93b3f572d643..441b9eb4b0ef 100644
--- a/arch/riscv/include/asm/barrier.h
+++ b/arch/riscv/include/asm/barrier.h
@@ -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; \
--
2.44.0
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 3/5] riscv: hwprobe: export Zawrs ISA extension
2024-03-15 13:40 [PATCH 0/5] riscv: Apply Zawrs when available Andrew Jones
2024-03-15 13:40 ` [PATCH 1/5] riscv: Add Zawrs support for spinlocks Andrew Jones
2024-03-15 13:40 ` [PATCH 2/5] riscv: Prefer wrs.nto over wrs.sto Andrew Jones
@ 2024-03-15 13:40 ` Andrew Jones
2024-03-15 13:40 ` [PATCH 4/5] KVM: riscv: Support guest wrs.nto Andrew Jones
` (2 subsequent siblings)
5 siblings, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2024-03-15 13:40 UTC (permalink / raw)
To: linux-riscv, kvm-riscv
Cc: paul.walmsley, palmer, aou, conor.dooley, anup, atishp,
christoph.muellner, heiko, charlie, David.Laight
Export Zawrs ISA extension through hwprobe.
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
Documentation/arch/riscv/hwprobe.rst | 4 ++++
arch/riscv/include/uapi/asm/hwprobe.h | 1 +
arch/riscv/kernel/sys_hwprobe.c | 1 +
3 files changed, 6 insertions(+)
diff --git a/Documentation/arch/riscv/hwprobe.rst b/Documentation/arch/riscv/hwprobe.rst
index b2bcc9eed9aa..290afe9b5753 100644
--- a/Documentation/arch/riscv/hwprobe.rst
+++ b/Documentation/arch/riscv/hwprobe.rst
@@ -188,6 +188,10 @@ The following keys are defined:
manual starting from commit 95cf1f9 ("Add changes requested by Ved
during signoff")
+ * :c:macro:`RISCV_HWPROBE_EXT_ZAWRS`: The Zawrs extension is supported as
+ 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.
diff --git a/arch/riscv/include/uapi/asm/hwprobe.h b/arch/riscv/include/uapi/asm/hwprobe.h
index 9f2a8e3ff204..a5fca3878a32 100644
--- a/arch/riscv/include/uapi/asm/hwprobe.h
+++ b/arch/riscv/include/uapi/asm/hwprobe.h
@@ -59,6 +59,7 @@ struct riscv_hwprobe {
#define RISCV_HWPROBE_EXT_ZTSO (1ULL << 33)
#define RISCV_HWPROBE_EXT_ZACAS (1ULL << 34)
#define RISCV_HWPROBE_EXT_ZICOND (1ULL << 35)
+#define RISCV_HWPROBE_EXT_ZAWRS (1ULL << 36)
#define RISCV_HWPROBE_KEY_CPUPERF_0 5
#define RISCV_HWPROBE_MISALIGNED_UNKNOWN (0 << 0)
#define RISCV_HWPROBE_MISALIGNED_EMULATED (1 << 0)
diff --git a/arch/riscv/kernel/sys_hwprobe.c b/arch/riscv/kernel/sys_hwprobe.c
index a7c56b41efd2..cc443480fd00 100644
--- a/arch/riscv/kernel/sys_hwprobe.c
+++ b/arch/riscv/kernel/sys_hwprobe.c
@@ -111,6 +111,7 @@ static void hwprobe_isa_ext0(struct riscv_hwprobe *pair,
EXT_KEY(ZTSO);
EXT_KEY(ZACAS);
EXT_KEY(ZICOND);
+ EXT_KEY(ZAWRS);
if (has_vector()) {
EXT_KEY(ZVBB);
--
2.44.0
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 4/5] KVM: riscv: Support guest wrs.nto
2024-03-15 13:40 [PATCH 0/5] riscv: Apply Zawrs when available Andrew Jones
` (2 preceding siblings ...)
2024-03-15 13:40 ` [PATCH 3/5] riscv: hwprobe: export Zawrs ISA extension Andrew Jones
@ 2024-03-15 13:40 ` Andrew Jones
2024-03-15 13:40 ` [PATCH 5/5] KVM: riscv: selftests: Add Zawrs extension to get-reg-list test Andrew Jones
2024-03-18 15:31 ` [PATCH 0/5] riscv: Apply Zawrs when available Andrew Jones
5 siblings, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2024-03-15 13:40 UTC (permalink / raw)
To: linux-riscv, kvm-riscv
Cc: paul.walmsley, palmer, aou, conor.dooley, anup, atishp,
christoph.muellner, heiko, charlie, David.Laight
When a guest traps on wrs.nto, call kvm_vcpu_on_spin() to attempt
to yield to the lock holding VCPU. Also extend the KVM ISA extension
ONE_REG interface to allow KVM userspace to detect and enable the
Zawrs extension for the Guest/VM.
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
arch/riscv/include/asm/kvm_host.h | 1 +
arch/riscv/include/uapi/asm/kvm.h | 1 +
arch/riscv/kvm/vcpu.c | 1 +
arch/riscv/kvm/vcpu_insn.c | 15 +++++++++++++++
arch/riscv/kvm/vcpu_onereg.c | 2 ++
5 files changed, 20 insertions(+)
diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
index 484d04a92fa6..e27c56e44783 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -69,6 +69,7 @@ struct kvm_vcpu_stat {
struct kvm_vcpu_stat_generic generic;
u64 ecall_exit_stat;
u64 wfi_exit_stat;
+ u64 wrs_exit_stat;
u64 mmio_exit_user;
u64 mmio_exit_kernel;
u64 csr_exit_user;
diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index b1c503c2959c..89ea06bd07c2 100644
--- a/arch/riscv/include/uapi/asm/kvm.h
+++ b/arch/riscv/include/uapi/asm/kvm.h
@@ -167,6 +167,7 @@ enum KVM_RISCV_ISA_EXT_ID {
KVM_RISCV_ISA_EXT_ZFA,
KVM_RISCV_ISA_EXT_ZTSO,
KVM_RISCV_ISA_EXT_ZACAS,
+ KVM_RISCV_ISA_EXT_ZAWRS,
KVM_RISCV_ISA_EXT_MAX,
};
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index b5ca9f2e98ac..abcdc78671e0 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -25,6 +25,7 @@ const struct _kvm_stats_desc kvm_vcpu_stats_desc[] = {
KVM_GENERIC_VCPU_STATS(),
STATS_DESC_COUNTER(VCPU, ecall_exit_stat),
STATS_DESC_COUNTER(VCPU, wfi_exit_stat),
+ STATS_DESC_COUNTER(VCPU, wrs_exit_stat),
STATS_DESC_COUNTER(VCPU, mmio_exit_user),
STATS_DESC_COUNTER(VCPU, mmio_exit_kernel),
STATS_DESC_COUNTER(VCPU, csr_exit_user),
diff --git a/arch/riscv/kvm/vcpu_insn.c b/arch/riscv/kvm/vcpu_insn.c
index ee7215f4071f..97dec18e6989 100644
--- a/arch/riscv/kvm/vcpu_insn.c
+++ b/arch/riscv/kvm/vcpu_insn.c
@@ -16,6 +16,9 @@
#define INSN_MASK_WFI 0xffffffff
#define INSN_MATCH_WFI 0x10500073
+#define INSN_MASK_WRS 0xffffffff
+#define INSN_MATCH_WRS 0x00d00073
+
#define INSN_MATCH_CSRRW 0x1073
#define INSN_MASK_CSRRW 0x707f
#define INSN_MATCH_CSRRS 0x2073
@@ -203,6 +206,13 @@ static int wfi_insn(struct kvm_vcpu *vcpu, struct kvm_run *run, ulong insn)
return KVM_INSN_CONTINUE_NEXT_SEPC;
}
+static int wrs_insn(struct kvm_vcpu *vcpu, struct kvm_run *run, ulong insn)
+{
+ vcpu->stat.wrs_exit_stat++;
+ kvm_vcpu_on_spin(vcpu, vcpu->arch.guest_context.sstatus & SR_SPP);
+ return KVM_INSN_CONTINUE_NEXT_SEPC;
+}
+
struct csr_func {
unsigned int base;
unsigned int count;
@@ -378,6 +388,11 @@ static const struct insn_func system_opcode_funcs[] = {
.match = INSN_MATCH_WFI,
.func = wfi_insn,
},
+ {
+ .mask = INSN_MASK_WRS,
+ .match = INSN_MATCH_WRS,
+ .func = wrs_insn,
+ },
};
static int system_opcode_insn(struct kvm_vcpu *vcpu, struct kvm_run *run,
diff --git a/arch/riscv/kvm/vcpu_onereg.c b/arch/riscv/kvm/vcpu_onereg.c
index f4a6124d25c9..67c5794af3b6 100644
--- a/arch/riscv/kvm/vcpu_onereg.c
+++ b/arch/riscv/kvm/vcpu_onereg.c
@@ -41,6 +41,7 @@ static const unsigned long kvm_isa_ext_arr[] = {
KVM_ISA_EXT_ARR(SVNAPOT),
KVM_ISA_EXT_ARR(SVPBMT),
KVM_ISA_EXT_ARR(ZACAS),
+ KVM_ISA_EXT_ARR(ZAWRS),
KVM_ISA_EXT_ARR(ZBA),
KVM_ISA_EXT_ARR(ZBB),
KVM_ISA_EXT_ARR(ZBC),
@@ -120,6 +121,7 @@ static bool kvm_riscv_vcpu_isa_disable_allowed(unsigned long ext)
case KVM_RISCV_ISA_EXT_SVINVAL:
case KVM_RISCV_ISA_EXT_SVNAPOT:
case KVM_RISCV_ISA_EXT_ZACAS:
+ case KVM_RISCV_ISA_EXT_ZAWRS:
case KVM_RISCV_ISA_EXT_ZBA:
case KVM_RISCV_ISA_EXT_ZBB:
case KVM_RISCV_ISA_EXT_ZBC:
--
2.44.0
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 5/5] KVM: riscv: selftests: Add Zawrs extension to get-reg-list test
2024-03-15 13:40 [PATCH 0/5] riscv: Apply Zawrs when available Andrew Jones
` (3 preceding siblings ...)
2024-03-15 13:40 ` [PATCH 4/5] KVM: riscv: Support guest wrs.nto Andrew Jones
@ 2024-03-15 13:40 ` Andrew Jones
2024-03-18 15:31 ` [PATCH 0/5] riscv: Apply Zawrs when available Andrew Jones
5 siblings, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2024-03-15 13:40 UTC (permalink / raw)
To: linux-riscv, kvm-riscv
Cc: paul.walmsley, palmer, aou, conor.dooley, anup, atishp,
christoph.muellner, heiko, charlie, David.Laight
KVM RISC-V allows the Zawrs extension for the Guest/VM, so add it
to the get-reg-list test.
Signed-off-by: Andrew Jones <ajones@ventanamicro.com>
---
tools/testing/selftests/kvm/riscv/get-reg-list.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/tools/testing/selftests/kvm/riscv/get-reg-list.c b/tools/testing/selftests/kvm/riscv/get-reg-list.c
index b882b7b9b785..8c4c27bd4b88 100644
--- a/tools/testing/selftests/kvm/riscv/get-reg-list.c
+++ b/tools/testing/selftests/kvm/riscv/get-reg-list.c
@@ -48,6 +48,7 @@ bool filter_reg(__u64 reg)
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVNAPOT:
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_SVPBMT:
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZACAS:
+ case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZAWRS:
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZBA:
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZBB:
case KVM_REG_RISCV_ISA_EXT | KVM_REG_RISCV_ISA_SINGLE | KVM_RISCV_ISA_EXT_ZBC:
@@ -413,6 +414,7 @@ static const char *isa_ext_single_id_to_str(__u64 reg_off)
KVM_ISA_EXT_ARR(SVNAPOT),
KVM_ISA_EXT_ARR(SVPBMT),
KVM_ISA_EXT_ARR(ZACAS),
+ KVM_ISA_EXT_ARR(ZAWRS),
KVM_ISA_EXT_ARR(ZBA),
KVM_ISA_EXT_ARR(ZBB),
KVM_ISA_EXT_ARR(ZBC),
@@ -936,6 +938,7 @@ KVM_ISA_EXT_SIMPLE_CONFIG(svinval, SVINVAL);
KVM_ISA_EXT_SIMPLE_CONFIG(svnapot, SVNAPOT);
KVM_ISA_EXT_SIMPLE_CONFIG(svpbmt, SVPBMT);
KVM_ISA_EXT_SIMPLE_CONFIG(zacas, ZACAS);
+KVM_ISA_EXT_SIMPLE_CONFIG(zawrs, ZAWRS);
KVM_ISA_EXT_SIMPLE_CONFIG(zba, ZBA);
KVM_ISA_EXT_SIMPLE_CONFIG(zbb, ZBB);
KVM_ISA_EXT_SIMPLE_CONFIG(zbc, ZBC);
@@ -991,6 +994,7 @@ struct vcpu_reg_list *vcpu_configs[] = {
&config_svnapot,
&config_svpbmt,
&config_zacas,
+ &config_zawrs,
&config_zba,
&config_zbb,
&config_zbc,
--
2.44.0
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 0/5] riscv: Apply Zawrs when available
2024-03-15 13:40 [PATCH 0/5] riscv: Apply Zawrs when available Andrew Jones
` (4 preceding siblings ...)
2024-03-15 13:40 ` [PATCH 5/5] KVM: riscv: selftests: Add Zawrs extension to get-reg-list test Andrew Jones
@ 2024-03-18 15:31 ` Andrew Jones
5 siblings, 0 replies; 9+ messages in thread
From: Andrew Jones @ 2024-03-18 15:31 UTC (permalink / raw)
To: linux-riscv, kvm-riscv
Cc: paul.walmsley, palmer, aou, conor.dooley, anup, atishp,
christoph.muellner, heiko, charlie, David.Laight
On Fri, Mar 15, 2024 at 02:40:10PM +0100, Andrew Jones wrote:
> 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@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
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 9+ messages in thread