[PATCH v6 00/13] Zacas/Zabha support and qspinlocks

[PATCH v6 00/13] Zacas/Zabha support and qspinlocks

[Alexandre Ghiti]

This implements [cmp]xchgXX() macros using Zacas and Zabha extensions
and finally uses those newly introduced macros to add support for
qspinlocks: note that this implementation of qspinlocks satisfies the
forward progress guarantee.

It also uses Ziccrse to provide the qspinlock implementation.

Thanks to Guo and Leonardo for their work!

v5: https://lore.kernel.org/linux-riscv/20240818063538.6651-1-alexghiti@rivosinc.com/
v4: https://lore.kernel.org/linux-riscv/20240731072405.197046-1-alexghiti@rivosinc.com/
v3: https://lore.kernel.org/linux-riscv/20240717061957.140712-1-alexghiti@rivosinc.com/
v2: https://lore.kernel.org/linux-riscv/20240626130347.520750-1-alexghiti@rivosinc.com/
v1: https://lore.kernel.org/linux-riscv/20240528151052.313031-1-alexghiti@rivosinc.com/

Changes in v6:
- Rebase on 6.12-rc5
- Fix comment about macros (Drew)
- Add RB from Drew

Changes in v5:
- Remove useless include in cpufeature.h and add required ones (Drew)
- Add RB from Drew
- Add AB from Conor and Peter
- use macros to help readability of arch_cmpxchg_XXX() (Drew)
- restore the build_bug() for size > 8 (Drew)
- Update Ziccrse riscv profile spec version commit hash (Conor)

Changes in v4:
- rename sc_sfx into sc_cas_sfx in _arch_cmpxchg (Drew)
- cmpxchg() depends on 64BIT (Drew)
- rename xX register into tX (Drew)
- cas operations require the old value in rd, make this assignment more explicit
  as it seems to confuse people (Drew, Andrea)
- Fix ticket/queued configs build errors (Andrea)
- riscv_spinlock_init() is only needed for combo spinlocks but implement it
  anyway to inform of the type of spinlocks used (Andrea)
- Add RB from Guo
- Add NONPORTABLE to RISCV_QUEUED_SPINLOCKS (Samuel)
- Add a link to Guo's qspinlocks results on the sophgo platform
- Reorder ZICCRSE (Samuel)
- Use riscv_has_extention_unlikely() instead of direct asm goto, which is way
  cleaner and fixes the llvm 16 bug
- add dependency on RISCV_ALTERNATIVES in kconfig
- Rebase on top of 6.11, add patches to fix header circular dependency and
  to fix build_bug()

Changes in v3:
- Fix patch 4 to restrict the optimization to fully ordered AMO (Andrea)
- Move RISCV_ISA_EXT_ZABHA definition to patch 4 (Andrea)
- !Zacas at build time => no CAS from Zabha too (Andrea)
- drop patch 7 "riscv: Improve amoswap.X use in xchg()" (Andrea)
- Switch lr/sc and cas order (Guo)
- Combo spinlocks do not depend on Zabha
- Add a Kconfig for ticket/queued/combo (Guo)
- Use Ziccrse (Guo)

Changes in v2:
- Add patch for Zabha dtbinding (Conor)
- Fix cmpxchg128() build warnings missed in v1
- Make arch_cmpxchg128() fully ordered
- Improve Kconfig help texts for both extensions (Conor)
- Fix Makefile dependencies by requiring TOOLCHAIN_HAS_XXX (Nathan)
- Fix compilation errors when the toolchain does not support the
  extensions (Nathan)
- Fix C23 warnings about label at the end of coumpound statements (Nathan)
- Fix Zabha and !Zacas configurations (Andrea)
- Add COMBO spinlocks (Guo)
- Improve amocas fully ordered operations by using .aqrl semantics and
  removing the fence rw, rw (Andrea)
- Rebase on top "riscv: Fix fully ordered LR/SC xchg[8|16]() implementations"
- Add ARCH_WEAK_RELEASE_ACQUIRE (Andrea)
- Remove the extension version in march for LLVM since it is only required
  for experimental extensions (Nathan)
- Fix cmpxchg128() implementation by adding both registers of a pair
  in the list of input/output operands

Alexandre Ghiti (11):
  riscv: Move cpufeature.h macros into their own header
  riscv: Do not fail to build on byte/halfword operations with Zawrs
  riscv: Implement cmpxchg32/64() using Zacas
  dt-bindings: riscv: Add Zabha ISA extension description
  riscv: Implement cmpxchg8/16() using Zabha
  riscv: Improve zacas fully-ordered cmpxchg()
  riscv: Implement arch_cmpxchg128() using Zacas
  riscv: Implement xchg8/16() using Zabha
  riscv: Add ISA extension parsing for Ziccrse
  dt-bindings: riscv: Add Ziccrse ISA extension description
  riscv: Add qspinlock support

Guo Ren (2):
  asm-generic: ticket-lock: Reuse arch_spinlock_t of qspinlock
  asm-generic: ticket-lock: Add separate ticket-lock.h

 .../devicetree/bindings/riscv/extensions.yaml |  12 +
 .../locking/queued-spinlocks/arch-support.txt |   2 +-
 arch/riscv/Kconfig                            |  69 +++++
 arch/riscv/Makefile                           |   6 +
 arch/riscv/include/asm/Kbuild                 |   4 +-
 arch/riscv/include/asm/cmpxchg.h              | 286 +++++++++++++-----
 arch/riscv/include/asm/cpufeature-macros.h    |  66 ++++
 arch/riscv/include/asm/cpufeature.h           |  61 +---
 arch/riscv/include/asm/hwcap.h                |   2 +
 arch/riscv/include/asm/spinlock.h             |  47 +++
 arch/riscv/kernel/cpufeature.c                |   2 +
 arch/riscv/kernel/setup.c                     |  37 +++
 include/asm-generic/qspinlock.h               |   2 +
 include/asm-generic/spinlock.h                |  87 +-----
 include/asm-generic/spinlock_types.h          |  12 +-
 include/asm-generic/ticket_spinlock.h         | 105 +++++++
 16 files changed, 567 insertions(+), 233 deletions(-)
 create mode 100644 arch/riscv/include/asm/cpufeature-macros.h
 create mode 100644 arch/riscv/include/asm/spinlock.h
 create mode 100644 include/asm-generic/ticket_spinlock.h

-- 
2.39.2

[PATCH v6 01/13] riscv: Move cpufeature.h macros into their own header

[Alexandre Ghiti]

asm/cmpxchg.h will soon need riscv_has_extension_unlikely() macros and
then needs to include asm/cpufeature.h which introduces a lot of header
circular dependencies.

So move the riscv_has_extension_XXX() macros into their own header which
prevents such circular dependencies by including a restricted number of
headers.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/cpufeature-macros.h | 66 ++++++++++++++++++++++
 arch/riscv/include/asm/cpufeature.h        | 61 ++------------------
 2 files changed, 70 insertions(+), 57 deletions(-)
 create mode 100644 arch/riscv/include/asm/cpufeature-macros.h

diff --git a/arch/riscv/include/asm/cpufeature-macros.h b/arch/riscv/include/asm/cpufeature-macros.h
new file mode 100644
index 000000000000..a8103edbf51f
--- /dev/null
+++ b/arch/riscv/include/asm/cpufeature-macros.h
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright 2022-2024 Rivos, Inc
+ */
+
+#ifndef _ASM_CPUFEATURE_MACROS_H
+#define _ASM_CPUFEATURE_MACROS_H
+
+#include <asm/hwcap.h>
+#include <asm/alternative-macros.h>
+
+#define STANDARD_EXT		0
+
+bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned int bit);
+#define riscv_isa_extension_available(isa_bitmap, ext)	\
+	__riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext)
+
+static __always_inline bool __riscv_has_extension_likely(const unsigned long vendor,
+							 const unsigned long ext)
+{
+	asm goto(ALTERNATIVE("j	%l[l_no]", "nop", %[vendor], %[ext], 1)
+	:
+	: [vendor] "i" (vendor), [ext] "i" (ext)
+	:
+	: l_no);
+
+	return true;
+l_no:
+	return false;
+}
+
+static __always_inline bool __riscv_has_extension_unlikely(const unsigned long vendor,
+							   const unsigned long ext)
+{
+	asm goto(ALTERNATIVE("nop", "j	%l[l_yes]", %[vendor], %[ext], 1)
+	:
+	: [vendor] "i" (vendor), [ext] "i" (ext)
+	:
+	: l_yes);
+
+	return false;
+l_yes:
+	return true;
+}
+
+static __always_inline bool riscv_has_extension_unlikely(const unsigned long ext)
+{
+	compiletime_assert(ext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX");
+
+	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))
+		return __riscv_has_extension_unlikely(STANDARD_EXT, ext);
+
+	return __riscv_isa_extension_available(NULL, ext);
+}
+
+static __always_inline bool riscv_has_extension_likely(const unsigned long ext)
+{
+	compiletime_assert(ext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX");
+
+	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))
+		return __riscv_has_extension_likely(STANDARD_EXT, ext);
+
+	return __riscv_isa_extension_available(NULL, ext);
+}
+
+#endif /* _ASM_CPUFEATURE_MACROS_H */
diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
index 45f9c1171a48..87ed88fc950d 100644
--- a/arch/riscv/include/asm/cpufeature.h
+++ b/arch/riscv/include/asm/cpufeature.h
@@ -8,9 +8,11 @@
 
 #include <linux/bitmap.h>
 #include <linux/jump_label.h>
+#include <linux/kconfig.h>
+#include <linux/percpu-defs.h>
+#include <linux/threads.h>
 #include <asm/hwcap.h>
-#include <asm/alternative-macros.h>
-#include <asm/errno.h>
+#include <asm/cpufeature-macros.h>
 
 /*
  * These are probed via a device_initcall(), via either the SBI or directly
@@ -103,61 +105,6 @@ extern const size_t riscv_isa_ext_count;
 extern bool riscv_isa_fallback;
 
 unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
-
-#define STANDARD_EXT		0
-
-bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, unsigned int bit);
-#define riscv_isa_extension_available(isa_bitmap, ext)	\
-	__riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext)
-
-static __always_inline bool __riscv_has_extension_likely(const unsigned long vendor,
-							 const unsigned long ext)
-{
-	asm goto(ALTERNATIVE("j	%l[l_no]", "nop", %[vendor], %[ext], 1)
-	:
-	: [vendor] "i" (vendor), [ext] "i" (ext)
-	:
-	: l_no);
-
-	return true;
-l_no:
-	return false;
-}
-
-static __always_inline bool __riscv_has_extension_unlikely(const unsigned long vendor,
-							   const unsigned long ext)
-{
-	asm goto(ALTERNATIVE("nop", "j	%l[l_yes]", %[vendor], %[ext], 1)
-	:
-	: [vendor] "i" (vendor), [ext] "i" (ext)
-	:
-	: l_yes);
-
-	return false;
-l_yes:
-	return true;
-}
-
-static __always_inline bool riscv_has_extension_unlikely(const unsigned long ext)
-{
-	compiletime_assert(ext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX");
-
-	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))
-		return __riscv_has_extension_unlikely(STANDARD_EXT, ext);
-
-	return __riscv_isa_extension_available(NULL, ext);
-}
-
-static __always_inline bool riscv_has_extension_likely(const unsigned long ext)
-{
-	compiletime_assert(ext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX");
-
-	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE))
-		return __riscv_has_extension_likely(STANDARD_EXT, ext);
-
-	return __riscv_isa_extension_available(NULL, ext);
-}
-
 static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext)
 {
 	compiletime_assert(ext < RISCV_ISA_EXT_MAX, "ext must be < RISCV_ISA_EXT_MAX");
-- 
2.39.2

[PATCH v6 02/13] riscv: Do not fail to build on byte/halfword operations with Zawrs

[Alexandre Ghiti]

riscv does not have lr instructions on byte and halfword but the
qspinlock implementation actually uses such atomics provided by the
Zabha extension, so those sizes are legitimate.

Then instead of failing to build, just fallback to the !Zawrs path.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/cmpxchg.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index ebbce134917c..ac1d7df898ef 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -245,6 +245,11 @@ static __always_inline void __cmpwait(volatile void *ptr,
 		 : : : : no_zawrs);
 
 	switch (size) {
+	case 1:
+		fallthrough;
+	case 2:
+		/* RISC-V doesn't have lr instructions on byte and half-word. */
+		goto no_zawrs;
 	case 4:
 		asm volatile(
 		"	lr.w	%0, %1\n"
-- 
2.39.2

[PATCH v6 03/13] riscv: Implement cmpxchg32/64() using Zacas

[Alexandre Ghiti]

This adds runtime support for Zacas in cmpxchg operations.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/Kconfig               | 16 +++++++++++
 arch/riscv/Makefile              |  3 ++
 arch/riscv/include/asm/cmpxchg.h | 48 +++++++++++++++++++++-----------
 3 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 62545946ecf4..3542efe3088b 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -632,6 +632,22 @@ config RISCV_ISA_ZAWRS
 	  use of these instructions in the kernel when the Zawrs extension is
 	  detected at boot.
 
+config TOOLCHAIN_HAS_ZACAS
+	bool
+	default y
+	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zacas)
+	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zacas)
+	depends on AS_HAS_OPTION_ARCH
+
+config RISCV_ISA_ZACAS
+	bool "Zacas extension support for atomic CAS"
+	depends on TOOLCHAIN_HAS_ZACAS
+	depends on RISCV_ALTERNATIVE
+	default y
+	help
+	  Enable the use of the Zacas ISA-extension to implement kernel atomic
+	  cmpxchg operations when it is detected at boot.
+
 	  If you don't know what to do here, say Y.
 
 config TOOLCHAIN_HAS_ZBB
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index d469db9f46f4..3700a1574413 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -82,6 +82,9 @@ else
 riscv-march-$(CONFIG_TOOLCHAIN_NEEDS_EXPLICIT_ZICSR_ZIFENCEI) := $(riscv-march-y)_zicsr_zifencei
 endif
 
+# Check if the toolchain supports Zacas
+riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZACAS) := $(riscv-march-y)_zacas
+
 # Remove F,D,V from isa string for all. Keep extensions between "fd" and "v" by
 # matching non-v and non-multi-letter extensions out with the filter ([^v_]*)
 KBUILD_CFLAGS += -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)fd([^v_]*)v?/\1\2/')
diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index ac1d7df898ef..39c1daf39f6a 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -12,6 +12,7 @@
 #include <asm/fence.h>
 #include <asm/hwcap.h>
 #include <asm/insn-def.h>
+#include <asm/cpufeature-macros.h>
 
 #define __arch_xchg_masked(sc_sfx, prepend, append, r, p, n)		\
 ({									\
@@ -137,24 +138,37 @@
 	r = (__typeof__(*(p)))((__retx & __mask) >> __s);		\
 })
 
-#define __arch_cmpxchg(lr_sfx, sc_sfx, prepend, append, r, p, co, o, n)	\
+#define __arch_cmpxchg(lr_sfx, sc_cas_sfx, prepend, append, r, p, co, o, n)	\
 ({									\
-	register unsigned int __rc;					\
+	if (IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) &&			\
+	    riscv_has_extension_unlikely(RISCV_ISA_EXT_ZACAS)) {	\
+		r = o;							\
 									\
-	__asm__ __volatile__ (						\
-		prepend							\
-		"0:	lr" lr_sfx " %0, %2\n"				\
-		"	bne  %0, %z3, 1f\n"				\
-		"	sc" sc_sfx " %1, %z4, %2\n"			\
-		"	bnez %1, 0b\n"					\
-		append							\
-		"1:\n"							\
-		: "=&r" (r), "=&r" (__rc), "+A" (*(p))			\
-		: "rJ" (co o), "rJ" (n)					\
-		: "memory");						\
+		__asm__ __volatile__ (					\
+			prepend						\
+			"	amocas" sc_cas_sfx " %0, %z2, %1\n"	\
+			append						\
+			: "+&r" (r), "+A" (*(p))			\
+			: "rJ" (n)					\
+			: "memory");					\
+	} else {							\
+		register unsigned int __rc;				\
+									\
+		__asm__ __volatile__ (					\
+			prepend						\
+			"0:	lr" lr_sfx " %0, %2\n"			\
+			"	bne  %0, %z3, 1f\n"			\
+			"	sc" sc_cas_sfx " %1, %z4, %2\n"		\
+			"	bnez %1, 0b\n"				\
+			append						\
+			"1:\n"						\
+			: "=&r" (r), "=&r" (__rc), "+A" (*(p))		\
+			: "rJ" (co o), "rJ" (n)				\
+			: "memory");					\
+	}								\
 })
 
-#define _arch_cmpxchg(ptr, old, new, sc_sfx, prepend, append)		\
+#define _arch_cmpxchg(ptr, old, new, sc_cas_sfx, prepend, append)	\
 ({									\
 	__typeof__(ptr) __ptr = (ptr);					\
 	__typeof__(*(__ptr)) __old = (old);				\
@@ -164,15 +178,15 @@
 	switch (sizeof(*__ptr)) {					\
 	case 1:								\
 	case 2:								\
-		__arch_cmpxchg_masked(sc_sfx, prepend, append,		\
+		__arch_cmpxchg_masked(sc_cas_sfx, prepend, append,	\
 					__ret, __ptr, __old, __new);	\
 		break;							\
 	case 4:								\
-		__arch_cmpxchg(".w", ".w" sc_sfx, prepend, append,	\
+		__arch_cmpxchg(".w", ".w" sc_cas_sfx, prepend, append,	\
 				__ret, __ptr, (long), __old, __new);	\
 		break;							\
 	case 8:								\
-		__arch_cmpxchg(".d", ".d" sc_sfx, prepend, append,	\
+		__arch_cmpxchg(".d", ".d" sc_cas_sfx, prepend, append,	\
 				__ret, __ptr, /**/, __old, __new);	\
 		break;							\
 	default:							\
-- 
2.39.2

[PATCH v6 04/13] dt-bindings: riscv: Add Zabha ISA extension description

[Alexandre Ghiti]

Add description for the Zabha ISA extension which was ratified in April
2024.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Guo Ren <guoren@kernel.org>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 Documentation/devicetree/bindings/riscv/extensions.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
index 2cf2026cff57..db062107823b 100644
--- a/Documentation/devicetree/bindings/riscv/extensions.yaml
+++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
@@ -178,6 +178,12 @@ properties:
             as ratified at commit 4a69197e5617 ("Update to ratified state") of
             riscv-svvptc.
 
+        - const: zabha
+          description: |
+            The Zabha extension for Byte and Halfword Atomic Memory Operations
+            as ratified at commit 49f49c842ff9 ("Update to Rafified state") of
+            riscv-zabha.
+
         - const: zacas
           description: |
             The Zacas extension for Atomic Compare-and-Swap (CAS) instructions
-- 
2.39.2

[PATCH v6 05/13] riscv: Implement cmpxchg8/16() using Zabha

[Alexandre Ghiti]

This adds runtime support for Zabha in cmpxchg8/16() operations.

Note that in the absence of Zacas support in the toolchain, CAS
instructions from Zabha won't be used.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/Kconfig               | 18 ++++++++
 arch/riscv/Makefile              |  3 ++
 arch/riscv/include/asm/cmpxchg.h | 78 ++++++++++++++++++++------------
 arch/riscv/include/asm/hwcap.h   |  1 +
 arch/riscv/kernel/cpufeature.c   |  1 +
 5 files changed, 72 insertions(+), 29 deletions(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 3542efe3088b..668be90a42e4 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -632,6 +632,24 @@ config RISCV_ISA_ZAWRS
 	  use of these instructions in the kernel when the Zawrs extension is
 	  detected at boot.
 
+config TOOLCHAIN_HAS_ZABHA
+	bool
+	default y
+	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zabha)
+	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zabha)
+	depends on AS_HAS_OPTION_ARCH
+
+config RISCV_ISA_ZABHA
+	bool "Zabha extension support for atomic byte/halfword operations"
+	depends on TOOLCHAIN_HAS_ZABHA
+	depends on RISCV_ALTERNATIVE
+	default y
+	help
+	  Enable the use of the Zabha ISA-extension to implement kernel
+	  byte/halfword atomic memory operations when it is detected at boot.
+
+	  If you don't know what to do here, say Y.
+
 config TOOLCHAIN_HAS_ZACAS
 	bool
 	default y
diff --git a/arch/riscv/Makefile b/arch/riscv/Makefile
index 3700a1574413..9fe1ee740dda 100644
--- a/arch/riscv/Makefile
+++ b/arch/riscv/Makefile
@@ -85,6 +85,9 @@ endif
 # Check if the toolchain supports Zacas
 riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZACAS) := $(riscv-march-y)_zacas
 
+# Check if the toolchain supports Zabha
+riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZABHA) := $(riscv-march-y)_zabha
+
 # Remove F,D,V from isa string for all. Keep extensions between "fd" and "v" by
 # matching non-v and non-multi-letter extensions out with the filter ([^v_]*)
 KBUILD_CFLAGS += -march=$(shell echo $(riscv-march-y) | sed -E 's/(rv32ima|rv64ima)fd([^v_]*)v?/\1\2/')
diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index 39c1daf39f6a..1f4cd12e4664 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -108,34 +108,49 @@
  * indicated by comparing RETURN with OLD.
  */
 
-#define __arch_cmpxchg_masked(sc_sfx, prepend, append, r, p, o, n)	\
-({									\
-	u32 *__ptr32b = (u32 *)((ulong)(p) & ~0x3);			\
-	ulong __s = ((ulong)(p) & (0x4 - sizeof(*p))) * BITS_PER_BYTE;	\
-	ulong __mask = GENMASK(((sizeof(*p)) * BITS_PER_BYTE) - 1, 0)	\
-			<< __s;						\
-	ulong __newx = (ulong)(n) << __s;				\
-	ulong __oldx = (ulong)(o) << __s;				\
-	ulong __retx;							\
-	ulong __rc;							\
-									\
-	__asm__ __volatile__ (						\
-		prepend							\
-		"0:	lr.w %0, %2\n"					\
-		"	and  %1, %0, %z5\n"				\
-		"	bne  %1, %z3, 1f\n"				\
-		"	and  %1, %0, %z6\n"				\
-		"	or   %1, %1, %z4\n"				\
-		"	sc.w" sc_sfx " %1, %1, %2\n"			\
-		"	bnez %1, 0b\n"					\
-		append							\
-		"1:\n"							\
-		: "=&r" (__retx), "=&r" (__rc), "+A" (*(__ptr32b))	\
-		: "rJ" ((long)__oldx), "rJ" (__newx),			\
-		  "rJ" (__mask), "rJ" (~__mask)				\
-		: "memory");						\
-									\
-	r = (__typeof__(*(p)))((__retx & __mask) >> __s);		\
+#define __arch_cmpxchg_masked(sc_sfx, cas_sfx, prepend, append, r, p, o, n)	\
+({										\
+	if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) &&				\
+	    IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) &&				\
+	    riscv_has_extension_unlikely(RISCV_ISA_EXT_ZABHA) &&		\
+	    riscv_has_extension_unlikely(RISCV_ISA_EXT_ZACAS)) {		\
+		r = o;								\
+										\
+		__asm__ __volatile__ (						\
+			prepend							\
+			"	amocas" cas_sfx " %0, %z2, %1\n"		\
+			append							\
+			: "+&r" (r), "+A" (*(p))				\
+			: "rJ" (n)						\
+			: "memory");						\
+	} else {								\
+		u32 *__ptr32b = (u32 *)((ulong)(p) & ~0x3);			\
+		ulong __s = ((ulong)(p) & (0x4 - sizeof(*p))) * BITS_PER_BYTE;	\
+		ulong __mask = GENMASK(((sizeof(*p)) * BITS_PER_BYTE) - 1, 0)	\
+			       << __s;						\
+		ulong __newx = (ulong)(n) << __s;				\
+		ulong __oldx = (ulong)(o) << __s;				\
+		ulong __retx;							\
+		ulong __rc;							\
+										\
+		__asm__ __volatile__ (						\
+			prepend							\
+			"0:	lr.w %0, %2\n"					\
+			"	and  %1, %0, %z5\n"				\
+			"	bne  %1, %z3, 1f\n"				\
+			"	and  %1, %0, %z6\n"				\
+			"	or   %1, %1, %z4\n"				\
+			"	sc.w" sc_sfx " %1, %1, %2\n"			\
+			"	bnez %1, 0b\n"					\
+			append							\
+			"1:\n"							\
+			: "=&r" (__retx), "=&r" (__rc), "+A" (*(__ptr32b))	\
+			: "rJ" ((long)__oldx), "rJ" (__newx),			\
+			  "rJ" (__mask), "rJ" (~__mask)				\
+			: "memory");						\
+										\
+		r = (__typeof__(*(p)))((__retx & __mask) >> __s);		\
+	}									\
 })
 
 #define __arch_cmpxchg(lr_sfx, sc_cas_sfx, prepend, append, r, p, co, o, n)	\
@@ -177,8 +192,13 @@
 									\
 	switch (sizeof(*__ptr)) {					\
 	case 1:								\
+		__arch_cmpxchg_masked(sc_cas_sfx, ".b" sc_cas_sfx,	\
+					prepend, append,		\
+					__ret, __ptr, __old, __new);    \
+		break;							\
 	case 2:								\
-		__arch_cmpxchg_masked(sc_cas_sfx, prepend, append,	\
+		__arch_cmpxchg_masked(sc_cas_sfx, ".h" sc_cas_sfx,	\
+					prepend, append,		\
 					__ret, __ptr, __old, __new);	\
 		break;							\
 	case 4:								\
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 46d9de54179e..74bcb0e2bd1f 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -93,6 +93,7 @@
 #define RISCV_ISA_EXT_ZCMOP		84
 #define RISCV_ISA_EXT_ZAWRS		85
 #define RISCV_ISA_EXT_SVVPTC		86
+#define RISCV_ISA_EXT_ZABHA		87
 
 #define RISCV_ISA_EXT_XLINUXENVCFG	127
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 3a8eeaa9310c..5e743d8d34f5 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -322,6 +322,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(zimop, RISCV_ISA_EXT_ZIMOP),
+	__RISCV_ISA_EXT_DATA(zabha, RISCV_ISA_EXT_ZABHA),
 	__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),
-- 
2.39.2

[PATCH v6 06/13] riscv: Improve zacas fully-ordered cmpxchg()

[Alexandre Ghiti]

The current fully-ordered cmpxchgXX() implementation results in:

  amocas.X.rl     a5,a4,(s1)
  fence           rw,rw

This provides enough sync but we can actually use the following better
mapping instead:

  amocas.X.aqrl   a5,a4,(s1)

Suggested-by: Andrea Parri <andrea@rivosinc.com>
Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/cmpxchg.h | 92 ++++++++++++++++++++++----------
 1 file changed, 64 insertions(+), 28 deletions(-)

diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index 1f4cd12e4664..052418aba11a 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -107,8 +107,10 @@
  * store NEW in MEM.  Return the initial value in MEM.  Success is
  * indicated by comparing RETURN with OLD.
  */
-
-#define __arch_cmpxchg_masked(sc_sfx, cas_sfx, prepend, append, r, p, o, n)	\
+#define __arch_cmpxchg_masked(sc_sfx, cas_sfx,					\
+			      sc_prepend, sc_append,				\
+			      cas_prepend, cas_append,				\
+			      r, p, o, n)					\
 ({										\
 	if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) &&				\
 	    IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) &&				\
@@ -117,9 +119,9 @@
 		r = o;								\
 										\
 		__asm__ __volatile__ (						\
-			prepend							\
+			cas_prepend							\
 			"	amocas" cas_sfx " %0, %z2, %1\n"		\
-			append							\
+			cas_append							\
 			: "+&r" (r), "+A" (*(p))				\
 			: "rJ" (n)						\
 			: "memory");						\
@@ -134,7 +136,7 @@
 		ulong __rc;							\
 										\
 		__asm__ __volatile__ (						\
-			prepend							\
+			sc_prepend							\
 			"0:	lr.w %0, %2\n"					\
 			"	and  %1, %0, %z5\n"				\
 			"	bne  %1, %z3, 1f\n"				\
@@ -142,7 +144,7 @@
 			"	or   %1, %1, %z4\n"				\
 			"	sc.w" sc_sfx " %1, %1, %2\n"			\
 			"	bnez %1, 0b\n"					\
-			append							\
+			sc_append							\
 			"1:\n"							\
 			: "=&r" (__retx), "=&r" (__rc), "+A" (*(__ptr32b))	\
 			: "rJ" ((long)__oldx), "rJ" (__newx),			\
@@ -153,16 +155,19 @@
 	}									\
 })
 
-#define __arch_cmpxchg(lr_sfx, sc_cas_sfx, prepend, append, r, p, co, o, n)	\
+#define __arch_cmpxchg(lr_sfx, sc_sfx, cas_sfx,				\
+		       sc_prepend, sc_append,				\
+		       cas_prepend, cas_append,				\
+		       r, p, co, o, n)					\
 ({									\
 	if (IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) &&			\
 	    riscv_has_extension_unlikely(RISCV_ISA_EXT_ZACAS)) {	\
 		r = o;							\
 									\
 		__asm__ __volatile__ (					\
-			prepend						\
-			"	amocas" sc_cas_sfx " %0, %z2, %1\n"	\
-			append						\
+			cas_prepend					\
+			"	amocas" cas_sfx " %0, %z2, %1\n"	\
+			cas_append					\
 			: "+&r" (r), "+A" (*(p))			\
 			: "rJ" (n)					\
 			: "memory");					\
@@ -170,12 +175,12 @@
 		register unsigned int __rc;				\
 									\
 		__asm__ __volatile__ (					\
-			prepend						\
+			sc_prepend					\
 			"0:	lr" lr_sfx " %0, %2\n"			\
 			"	bne  %0, %z3, 1f\n"			\
-			"	sc" sc_cas_sfx " %1, %z4, %2\n"		\
+			"	sc" sc_sfx " %1, %z4, %2\n"		\
 			"	bnez %1, 0b\n"				\
-			append						\
+			sc_append					\
 			"1:\n"						\
 			: "=&r" (r), "=&r" (__rc), "+A" (*(p))		\
 			: "rJ" (co o), "rJ" (n)				\
@@ -183,7 +188,9 @@
 	}								\
 })
 
-#define _arch_cmpxchg(ptr, old, new, sc_cas_sfx, prepend, append)	\
+#define _arch_cmpxchg(ptr, old, new, sc_sfx, cas_sfx,			\
+		      sc_prepend, sc_append,				\
+		      cas_prepend, cas_append)				\
 ({									\
 	__typeof__(ptr) __ptr = (ptr);					\
 	__typeof__(*(__ptr)) __old = (old);				\
@@ -192,22 +199,28 @@
 									\
 	switch (sizeof(*__ptr)) {					\
 	case 1:								\
-		__arch_cmpxchg_masked(sc_cas_sfx, ".b" sc_cas_sfx,	\
-					prepend, append,		\
-					__ret, __ptr, __old, __new);    \
+		__arch_cmpxchg_masked(sc_sfx, ".b" cas_sfx,		\
+				      sc_prepend, sc_append,		\
+				      cas_prepend, cas_append,		\
+				      __ret, __ptr, __old, __new);	\
 		break;							\
 	case 2:								\
-		__arch_cmpxchg_masked(sc_cas_sfx, ".h" sc_cas_sfx,	\
-					prepend, append,		\
-					__ret, __ptr, __old, __new);	\
+		__arch_cmpxchg_masked(sc_sfx, ".h" cas_sfx,		\
+				      sc_prepend, sc_append,		\
+				      cas_prepend, cas_append,		\
+				      __ret, __ptr, __old, __new);	\
 		break;							\
 	case 4:								\
-		__arch_cmpxchg(".w", ".w" sc_cas_sfx, prepend, append,	\
-				__ret, __ptr, (long), __old, __new);	\
+		__arch_cmpxchg(".w", ".w" sc_sfx, ".w" cas_sfx,		\
+			       sc_prepend, sc_append,			\
+			       cas_prepend, cas_append,			\
+			       __ret, __ptr, (long), __old, __new);	\
 		break;							\
 	case 8:								\
-		__arch_cmpxchg(".d", ".d" sc_cas_sfx, prepend, append,	\
-				__ret, __ptr, /**/, __old, __new);	\
+		__arch_cmpxchg(".d", ".d" sc_sfx, ".d" cas_sfx,		\
+			       sc_prepend, sc_append,			\
+			       cas_prepend, cas_append,			\
+			       __ret, __ptr, /**/, __old, __new);	\
 		break;							\
 	default:							\
 		BUILD_BUG();						\
@@ -215,17 +228,40 @@
 	(__typeof__(*(__ptr)))__ret;					\
 })
 
+/*
+ * These macros are here to improve the readability of the arch_cmpxchg_XXX()
+ * macros.
+ */
+#define SC_SFX(x)	x
+#define CAS_SFX(x)	x
+#define SC_PREPEND(x)	x
+#define SC_APPEND(x)	x
+#define CAS_PREPEND(x)	x
+#define CAS_APPEND(x)	x
+
 #define arch_cmpxchg_relaxed(ptr, o, n)					\
-	_arch_cmpxchg((ptr), (o), (n), "", "", "")
+	_arch_cmpxchg((ptr), (o), (n),					\
+		      SC_SFX(""), CAS_SFX(""),				\
+		      SC_PREPEND(""), SC_APPEND(""),			\
+		      CAS_PREPEND(""), CAS_APPEND(""))
 
 #define arch_cmpxchg_acquire(ptr, o, n)					\
-	_arch_cmpxchg((ptr), (o), (n), "", "", RISCV_ACQUIRE_BARRIER)
+	_arch_cmpxchg((ptr), (o), (n),					\
+		      SC_SFX(""), CAS_SFX(""),				\
+		      SC_PREPEND(""), SC_APPEND(RISCV_ACQUIRE_BARRIER),	\
+		      CAS_PREPEND(""), CAS_APPEND(RISCV_ACQUIRE_BARRIER))
 
 #define arch_cmpxchg_release(ptr, o, n)					\
-	_arch_cmpxchg((ptr), (o), (n), "", RISCV_RELEASE_BARRIER, "")
+	_arch_cmpxchg((ptr), (o), (n),					\
+		      SC_SFX(""), CAS_SFX(""),				\
+		      SC_PREPEND(RISCV_RELEASE_BARRIER), SC_APPEND(""),	\
+		      CAS_PREPEND(RISCV_RELEASE_BARRIER), CAS_APPEND(""))
 
 #define arch_cmpxchg(ptr, o, n)						\
-	_arch_cmpxchg((ptr), (o), (n), ".rl", "", "	fence rw, rw\n")
+	_arch_cmpxchg((ptr), (o), (n),					\
+		      SC_SFX(".rl"), CAS_SFX(".aqrl"),			\
+		      SC_PREPEND(""), SC_APPEND(RISCV_FULL_BARRIER),	\
+		      CAS_PREPEND(""), CAS_APPEND(""))
 
 #define arch_cmpxchg_local(ptr, o, n)					\
 	arch_cmpxchg_relaxed((ptr), (o), (n))
-- 
2.39.2

[PATCH v6 07/13] riscv: Implement arch_cmpxchg128() using Zacas

[Alexandre Ghiti]

Now that Zacas is supported in the kernel, let's use the double word
atomic version of amocas to improve the SLUB allocator.

Note that we have to select fixed registers, otherwise gcc fails to pick
even registers and then produces a reserved encoding which fails to
assemble.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/Kconfig               |  1 +
 arch/riscv/include/asm/cmpxchg.h | 38 ++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 668be90a42e4..093ee6537331 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -115,6 +115,7 @@ config RISCV
 	select GENERIC_VDSO_TIME_NS if HAVE_GENERIC_VDSO
 	select HARDIRQS_SW_RESEND
 	select HAS_IOPORT if MMU
+	select HAVE_ALIGNED_STRUCT_PAGE
 	select HAVE_ARCH_AUDITSYSCALL
 	select HAVE_ARCH_HUGE_VMALLOC if HAVE_ARCH_HUGE_VMAP
 	select HAVE_ARCH_HUGE_VMAP if MMU && 64BIT
diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index 052418aba11a..f95929f538b2 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -296,6 +296,44 @@
 	arch_cmpxchg_release((ptr), (o), (n));				\
 })
 
+#if defined(CONFIG_64BIT) && defined(CONFIG_RISCV_ISA_ZACAS)
+
+#define system_has_cmpxchg128()        riscv_has_extension_unlikely(RISCV_ISA_EXT_ZACAS)
+
+union __u128_halves {
+	u128 full;
+	struct {
+		u64 low, high;
+	};
+};
+
+#define __arch_cmpxchg128(p, o, n, cas_sfx)					\
+({										\
+	__typeof__(*(p)) __o = (o);                                             \
+	union __u128_halves __hn = { .full = (n) };				\
+	union __u128_halves __ho = { .full = (__o) };				\
+	register unsigned long t1 asm ("t1") = __hn.low;			\
+	register unsigned long t2 asm ("t2") = __hn.high;			\
+	register unsigned long t3 asm ("t3") = __ho.low;			\
+	register unsigned long t4 asm ("t4") = __ho.high;			\
+										\
+	__asm__ __volatile__ (							\
+		 "       amocas.q" cas_sfx " %0, %z3, %2"			\
+		 : "+&r" (t3), "+&r" (t4), "+A" (*(p))				\
+		 : "rJ" (t1), "rJ" (t2)						\
+		 : "memory");							\
+										\
+		 ((u128)t4 << 64) | t3;						\
+})
+
+#define arch_cmpxchg128(ptr, o, n)						\
+	__arch_cmpxchg128((ptr), (o), (n), ".aqrl")
+
+#define arch_cmpxchg128_local(ptr, o, n)					\
+	__arch_cmpxchg128((ptr), (o), (n), "")
+
+#endif /* CONFIG_64BIT && CONFIG_RISCV_ISA_ZACAS */
+
 #ifdef CONFIG_RISCV_ISA_ZAWRS
 /*
  * Despite wrs.nto being "WRS-with-no-timeout", in the absence of changes to
-- 
2.39.2

[PATCH v6 08/13] riscv: Implement xchg8/16() using Zabha

[Alexandre Ghiti]

This adds runtime support for Zabha in xchg8/16() operations.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/cmpxchg.h | 65 ++++++++++++++++++++------------
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h
index f95929f538b2..4cadc56220fe 100644
--- a/arch/riscv/include/asm/cmpxchg.h
+++ b/arch/riscv/include/asm/cmpxchg.h
@@ -14,29 +14,41 @@
 #include <asm/insn-def.h>
 #include <asm/cpufeature-macros.h>
 
-#define __arch_xchg_masked(sc_sfx, prepend, append, r, p, n)		\
-({									\
-	u32 *__ptr32b = (u32 *)((ulong)(p) & ~0x3);			\
-	ulong __s = ((ulong)(p) & (0x4 - sizeof(*p))) * BITS_PER_BYTE;	\
-	ulong __mask = GENMASK(((sizeof(*p)) * BITS_PER_BYTE) - 1, 0)	\
-			<< __s;						\
-	ulong __newx = (ulong)(n) << __s;				\
-	ulong __retx;							\
-	ulong __rc;							\
-									\
-	__asm__ __volatile__ (						\
-	       prepend							\
-	       "0:	lr.w %0, %2\n"					\
-	       "	and  %1, %0, %z4\n"				\
-	       "	or   %1, %1, %z3\n"				\
-	       "	sc.w" sc_sfx " %1, %1, %2\n"			\
-	       "	bnez %1, 0b\n"					\
-	       append							\
-	       : "=&r" (__retx), "=&r" (__rc), "+A" (*(__ptr32b))	\
-	       : "rJ" (__newx), "rJ" (~__mask)				\
-	       : "memory");						\
-									\
-	r = (__typeof__(*(p)))((__retx & __mask) >> __s);		\
+#define __arch_xchg_masked(sc_sfx, swap_sfx, prepend, sc_append,		\
+			   swap_append, r, p, n)				\
+({										\
+	if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) &&				\
+	    riscv_has_extension_unlikely(RISCV_ISA_EXT_ZABHA)) {		\
+		__asm__ __volatile__ (						\
+			prepend							\
+			"	amoswap" swap_sfx " %0, %z2, %1\n"		\
+			swap_append						\
+			: "=&r" (r), "+A" (*(p))				\
+			: "rJ" (n)						\
+			: "memory");						\
+	} else {								\
+		u32 *__ptr32b = (u32 *)((ulong)(p) & ~0x3);			\
+		ulong __s = ((ulong)(p) & (0x4 - sizeof(*p))) * BITS_PER_BYTE;	\
+		ulong __mask = GENMASK(((sizeof(*p)) * BITS_PER_BYTE) - 1, 0)	\
+				<< __s;						\
+		ulong __newx = (ulong)(n) << __s;				\
+		ulong __retx;							\
+		ulong __rc;							\
+										\
+		__asm__ __volatile__ (						\
+		       prepend							\
+		       "0:	lr.w %0, %2\n"					\
+		       "	and  %1, %0, %z4\n"				\
+		       "	or   %1, %1, %z3\n"				\
+		       "	sc.w" sc_sfx " %1, %1, %2\n"			\
+		       "	bnez %1, 0b\n"					\
+		       sc_append						\
+		       : "=&r" (__retx), "=&r" (__rc), "+A" (*(__ptr32b))	\
+		       : "rJ" (__newx), "rJ" (~__mask)				\
+		       : "memory");						\
+										\
+		r = (__typeof__(*(p)))((__retx & __mask) >> __s);		\
+	}									\
 })
 
 #define __arch_xchg(sfx, prepend, append, r, p, n)			\
@@ -59,8 +71,13 @@
 									\
 	switch (sizeof(*__ptr)) {					\
 	case 1:								\
+		__arch_xchg_masked(sc_sfx, ".b" swap_sfx,		\
+				   prepend, sc_append, swap_append,	\
+				   __ret, __ptr, __new);		\
+		break;							\
 	case 2:								\
-		__arch_xchg_masked(sc_sfx, prepend, sc_append,		\
+		__arch_xchg_masked(sc_sfx, ".h" swap_sfx,		\
+				   prepend, sc_append, swap_append,	\
 				   __ret, __ptr, __new);		\
 		break;							\
 	case 4:								\
-- 
2.39.2

[PATCH v6 09/13] asm-generic: ticket-lock: Reuse arch_spinlock_t of qspinlock

[Alexandre Ghiti]

From: Guo Ren <guoren@linux.alibaba.com>

The arch_spinlock_t of qspinlock has contained the atomic_t val, which
satisfies the ticket-lock requirement. Thus, unify the arch_spinlock_t
into qspinlock_types.h. This is the preparation for the next combo
spinlock.

Reviewed-by: Leonardo Bras <leobras@redhat.com>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Link: https://lore.kernel.org/linux-riscv/CAK8P3a2rnz9mQqhN6-e0CGUUv9rntRELFdxt_weiD7FxH7fkfQ@mail.gmail.com/
Signed-off-by: Guo Ren <guoren@kernel.org>
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 include/asm-generic/spinlock.h       | 14 +++++++-------
 include/asm-generic/spinlock_types.h | 12 ++----------
 2 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/include/asm-generic/spinlock.h b/include/asm-generic/spinlock.h
index 90803a826ba0..4773334ee638 100644
--- a/include/asm-generic/spinlock.h
+++ b/include/asm-generic/spinlock.h
@@ -32,7 +32,7 @@
 
 static __always_inline void arch_spin_lock(arch_spinlock_t *lock)
 {
-	u32 val = atomic_fetch_add(1<<16, lock);
+	u32 val = atomic_fetch_add(1<<16, &lock->val);
 	u16 ticket = val >> 16;
 
 	if (ticket == (u16)val)
@@ -46,31 +46,31 @@ static __always_inline void arch_spin_lock(arch_spinlock_t *lock)
 	 * have no outstanding writes due to the atomic_fetch_add() the extra
 	 * orderings are free.
 	 */
-	atomic_cond_read_acquire(lock, ticket == (u16)VAL);
+	atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
 	smp_mb();
 }
 
 static __always_inline bool arch_spin_trylock(arch_spinlock_t *lock)
 {
-	u32 old = atomic_read(lock);
+	u32 old = atomic_read(&lock->val);
 
 	if ((old >> 16) != (old & 0xffff))
 		return false;
 
-	return atomic_try_cmpxchg(lock, &old, old + (1<<16)); /* SC, for RCsc */
+	return atomic_try_cmpxchg(&lock->val, &old, old + (1<<16)); /* SC, for RCsc */
 }
 
 static __always_inline void arch_spin_unlock(arch_spinlock_t *lock)
 {
 	u16 *ptr = (u16 *)lock + IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
-	u32 val = atomic_read(lock);
+	u32 val = atomic_read(&lock->val);
 
 	smp_store_release(ptr, (u16)val + 1);
 }
 
 static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
 {
-	u32 val = lock.counter;
+	u32 val = lock.val.counter;
 
 	return ((val >> 16) == (val & 0xffff));
 }
@@ -84,7 +84,7 @@ static __always_inline int arch_spin_is_locked(arch_spinlock_t *lock)
 
 static __always_inline int arch_spin_is_contended(arch_spinlock_t *lock)
 {
-	u32 val = atomic_read(lock);
+	u32 val = atomic_read(&lock->val);
 
 	return (s16)((val >> 16) - (val & 0xffff)) > 1;
 }
diff --git a/include/asm-generic/spinlock_types.h b/include/asm-generic/spinlock_types.h
index 8962bb730945..f534aa5de394 100644
--- a/include/asm-generic/spinlock_types.h
+++ b/include/asm-generic/spinlock_types.h
@@ -3,15 +3,7 @@
 #ifndef __ASM_GENERIC_SPINLOCK_TYPES_H
 #define __ASM_GENERIC_SPINLOCK_TYPES_H
 
-#include <linux/types.h>
-typedef atomic_t arch_spinlock_t;
-
-/*
- * qrwlock_types depends on arch_spinlock_t, so we must typedef that before the
- * include.
- */
-#include <asm/qrwlock_types.h>
-
-#define __ARCH_SPIN_LOCK_UNLOCKED	ATOMIC_INIT(0)
+#include <asm-generic/qspinlock_types.h>
+#include <asm-generic/qrwlock_types.h>
 
 #endif /* __ASM_GENERIC_SPINLOCK_TYPES_H */
-- 
2.39.2

[PATCH v6 10/13] asm-generic: ticket-lock: Add separate ticket-lock.h

[Alexandre Ghiti]

From: Guo Ren <guoren@linux.alibaba.com>

Add a separate ticket-lock.h to include multiple spinlock versions and
select one at compile time or runtime.

Reviewed-by: Leonardo Bras <leobras@redhat.com>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Link: https://lore.kernel.org/linux-riscv/CAK8P3a2rnz9mQqhN6-e0CGUUv9rntRELFdxt_weiD7FxH7fkfQ@mail.gmail.com/
Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
Signed-off-by: Guo Ren <guoren@kernel.org>
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 include/asm-generic/spinlock.h        |  87 +---------------------
 include/asm-generic/ticket_spinlock.h | 103 ++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 86 deletions(-)
 create mode 100644 include/asm-generic/ticket_spinlock.h

diff --git a/include/asm-generic/spinlock.h b/include/asm-generic/spinlock.h
index 4773334ee638..970590baf61b 100644
--- a/include/asm-generic/spinlock.h
+++ b/include/asm-generic/spinlock.h
@@ -1,94 +1,9 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
-/*
- * 'Generic' ticket-lock implementation.
- *
- * It relies on atomic_fetch_add() having well defined forward progress
- * guarantees under contention. If your architecture cannot provide this, stick
- * to a test-and-set lock.
- *
- * It also relies on atomic_fetch_add() being safe vs smp_store_release() on a
- * sub-word of the value. This is generally true for anything LL/SC although
- * you'd be hard pressed to find anything useful in architecture specifications
- * about this. If your architecture cannot do this you might be better off with
- * a test-and-set.
- *
- * It further assumes atomic_*_release() + atomic_*_acquire() is RCpc and hence
- * uses atomic_fetch_add() which is RCsc to create an RCsc hot path, along with
- * a full fence after the spin to upgrade the otherwise-RCpc
- * atomic_cond_read_acquire().
- *
- * The implementation uses smp_cond_load_acquire() to spin, so if the
- * architecture has WFE like instructions to sleep instead of poll for word
- * modifications be sure to implement that (see ARM64 for example).
- *
- */
-
 #ifndef __ASM_GENERIC_SPINLOCK_H
 #define __ASM_GENERIC_SPINLOCK_H
 
-#include <linux/atomic.h>
-#include <asm-generic/spinlock_types.h>
-
-static __always_inline void arch_spin_lock(arch_spinlock_t *lock)
-{
-	u32 val = atomic_fetch_add(1<<16, &lock->val);
-	u16 ticket = val >> 16;
-
-	if (ticket == (u16)val)
-		return;
-
-	/*
-	 * atomic_cond_read_acquire() is RCpc, but rather than defining a
-	 * custom cond_read_rcsc() here we just emit a full fence.  We only
-	 * need the prior reads before subsequent writes ordering from
-	 * smb_mb(), but as atomic_cond_read_acquire() just emits reads and we
-	 * have no outstanding writes due to the atomic_fetch_add() the extra
-	 * orderings are free.
-	 */
-	atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
-	smp_mb();
-}
-
-static __always_inline bool arch_spin_trylock(arch_spinlock_t *lock)
-{
-	u32 old = atomic_read(&lock->val);
-
-	if ((old >> 16) != (old & 0xffff))
-		return false;
-
-	return atomic_try_cmpxchg(&lock->val, &old, old + (1<<16)); /* SC, for RCsc */
-}
-
-static __always_inline void arch_spin_unlock(arch_spinlock_t *lock)
-{
-	u16 *ptr = (u16 *)lock + IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
-	u32 val = atomic_read(&lock->val);
-
-	smp_store_release(ptr, (u16)val + 1);
-}
-
-static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
-{
-	u32 val = lock.val.counter;
-
-	return ((val >> 16) == (val & 0xffff));
-}
-
-static __always_inline int arch_spin_is_locked(arch_spinlock_t *lock)
-{
-	arch_spinlock_t val = READ_ONCE(*lock);
-
-	return !arch_spin_value_unlocked(val);
-}
-
-static __always_inline int arch_spin_is_contended(arch_spinlock_t *lock)
-{
-	u32 val = atomic_read(&lock->val);
-
-	return (s16)((val >> 16) - (val & 0xffff)) > 1;
-}
-
+#include <asm-generic/ticket_spinlock.h>
 #include <asm/qrwlock.h>
 
 #endif /* __ASM_GENERIC_SPINLOCK_H */
diff --git a/include/asm-generic/ticket_spinlock.h b/include/asm-generic/ticket_spinlock.h
new file mode 100644
index 000000000000..cfcff22b37b3
--- /dev/null
+++ b/include/asm-generic/ticket_spinlock.h
@@ -0,0 +1,103 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+/*
+ * 'Generic' ticket-lock implementation.
+ *
+ * It relies on atomic_fetch_add() having well defined forward progress
+ * guarantees under contention. If your architecture cannot provide this, stick
+ * to a test-and-set lock.
+ *
+ * It also relies on atomic_fetch_add() being safe vs smp_store_release() on a
+ * sub-word of the value. This is generally true for anything LL/SC although
+ * you'd be hard pressed to find anything useful in architecture specifications
+ * about this. If your architecture cannot do this you might be better off with
+ * a test-and-set.
+ *
+ * It further assumes atomic_*_release() + atomic_*_acquire() is RCpc and hence
+ * uses atomic_fetch_add() which is RCsc to create an RCsc hot path, along with
+ * a full fence after the spin to upgrade the otherwise-RCpc
+ * atomic_cond_read_acquire().
+ *
+ * The implementation uses smp_cond_load_acquire() to spin, so if the
+ * architecture has WFE like instructions to sleep instead of poll for word
+ * modifications be sure to implement that (see ARM64 for example).
+ *
+ */
+
+#ifndef __ASM_GENERIC_TICKET_SPINLOCK_H
+#define __ASM_GENERIC_TICKET_SPINLOCK_H
+
+#include <linux/atomic.h>
+#include <asm-generic/spinlock_types.h>
+
+static __always_inline void ticket_spin_lock(arch_spinlock_t *lock)
+{
+	u32 val = atomic_fetch_add(1<<16, &lock->val);
+	u16 ticket = val >> 16;
+
+	if (ticket == (u16)val)
+		return;
+
+	/*
+	 * atomic_cond_read_acquire() is RCpc, but rather than defining a
+	 * custom cond_read_rcsc() here we just emit a full fence.  We only
+	 * need the prior reads before subsequent writes ordering from
+	 * smb_mb(), but as atomic_cond_read_acquire() just emits reads and we
+	 * have no outstanding writes due to the atomic_fetch_add() the extra
+	 * orderings are free.
+	 */
+	atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
+	smp_mb();
+}
+
+static __always_inline bool ticket_spin_trylock(arch_spinlock_t *lock)
+{
+	u32 old = atomic_read(&lock->val);
+
+	if ((old >> 16) != (old & 0xffff))
+		return false;
+
+	return atomic_try_cmpxchg(&lock->val, &old, old + (1<<16)); /* SC, for RCsc */
+}
+
+static __always_inline void ticket_spin_unlock(arch_spinlock_t *lock)
+{
+	u16 *ptr = (u16 *)lock + IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
+	u32 val = atomic_read(&lock->val);
+
+	smp_store_release(ptr, (u16)val + 1);
+}
+
+static __always_inline int ticket_spin_value_unlocked(arch_spinlock_t lock)
+{
+	u32 val = lock.val.counter;
+
+	return ((val >> 16) == (val & 0xffff));
+}
+
+static __always_inline int ticket_spin_is_locked(arch_spinlock_t *lock)
+{
+	arch_spinlock_t val = READ_ONCE(*lock);
+
+	return !ticket_spin_value_unlocked(val);
+}
+
+static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
+{
+	u32 val = atomic_read(&lock->val);
+
+	return (s16)((val >> 16) - (val & 0xffff)) > 1;
+}
+
+/*
+ * Remapping spinlock architecture specific functions to the corresponding
+ * ticket spinlock functions.
+ */
+#define arch_spin_is_locked(l)		ticket_spin_is_locked(l)
+#define arch_spin_is_contended(l)	ticket_spin_is_contended(l)
+#define arch_spin_value_unlocked(l)	ticket_spin_value_unlocked(l)
+#define arch_spin_lock(l)		ticket_spin_lock(l)
+#define arch_spin_trylock(l)		ticket_spin_trylock(l)
+#define arch_spin_unlock(l)		ticket_spin_unlock(l)
+
+#endif /* __ASM_GENERIC_TICKET_SPINLOCK_H */
-- 
2.39.2

[PATCH v6 11/13] riscv: Add ISA extension parsing for Ziccrse

[Alexandre Ghiti]

Add support to parse the Ziccrse string in the riscv,isa string.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 arch/riscv/include/asm/hwcap.h | 1 +
 arch/riscv/kernel/cpufeature.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 74bcb0e2bd1f..0aa3c3f5e682 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -94,6 +94,7 @@
 #define RISCV_ISA_EXT_ZAWRS		85
 #define RISCV_ISA_EXT_SVVPTC		86
 #define RISCV_ISA_EXT_ZABHA		87
+#define RISCV_ISA_EXT_ZICCRSE		88
 
 #define RISCV_ISA_EXT_XLINUXENVCFG	127
 
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 5e743d8d34f5..5f453a039ec9 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -314,6 +314,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 					  riscv_ext_zicbom_validate),
 	__RISCV_ISA_EXT_SUPERSET_VALIDATE(zicboz, RISCV_ISA_EXT_ZICBOZ, riscv_xlinuxenvcfg_exts,
 					  riscv_ext_zicboz_validate),
+	__RISCV_ISA_EXT_DATA(ziccrse, RISCV_ISA_EXT_ZICCRSE),
 	__RISCV_ISA_EXT_DATA(zicntr, RISCV_ISA_EXT_ZICNTR),
 	__RISCV_ISA_EXT_DATA(zicond, RISCV_ISA_EXT_ZICOND),
 	__RISCV_ISA_EXT_DATA(zicsr, RISCV_ISA_EXT_ZICSR),
-- 
2.39.2

[PATCH v6 12/13] dt-bindings: riscv: Add Ziccrse ISA extension description

[Alexandre Ghiti]

Add description for the Ziccrse ISA extension which was ratified in
the riscv profiles specification v1.0.

Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Reviewed-by: Guo Ren <guoren@kernel.org>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 Documentation/devicetree/bindings/riscv/extensions.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
index db062107823b..35a9ad1d7e63 100644
--- a/Documentation/devicetree/bindings/riscv/extensions.yaml
+++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
@@ -296,6 +296,12 @@ properties:
             in commit 64074bc ("Update version numbers for Zfh/Zfinx") of
             riscv-isa-manual.
 
+        - const: ziccrse
+          description:
+            The standard Ziccrse extension which provides forward progress
+            guarantee on LR/SC sequences, as ratified in commit b1d806605f87
+            ("Updated to ratified state.") of the riscv profiles specification.
+
         - const: zk
           description:
             The standard Zk Standard Scalar cryptography extension as ratified
-- 
2.39.2

[PATCH v6 13/13] riscv: Add qspinlock support

[Alexandre Ghiti]

In order to produce a generic kernel, a user can select
CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
spinlock implementation if Zabha or Ziccrse are not present.

Note that we can't use alternatives here because the discovery of
extensions is done too late and we need to start with the qspinlock
implementation because the ticket spinlock implementation would pollute
the spinlock value, so let's use static keys.

This is largely based on Guo's work and Leonardo reviews at [1].

Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
Signed-off-by: Guo Ren <guoren@kernel.org>
Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
---
 .../locking/queued-spinlocks/arch-support.txt |  2 +-
 arch/riscv/Kconfig                            | 34 ++++++++++++++
 arch/riscv/include/asm/Kbuild                 |  4 +-
 arch/riscv/include/asm/spinlock.h             | 47 +++++++++++++++++++
 arch/riscv/kernel/setup.c                     | 37 +++++++++++++++
 include/asm-generic/qspinlock.h               |  2 +
 include/asm-generic/ticket_spinlock.h         |  2 +
 7 files changed, 126 insertions(+), 2 deletions(-)
 create mode 100644 arch/riscv/include/asm/spinlock.h

diff --git a/Documentation/features/locking/queued-spinlocks/arch-support.txt b/Documentation/features/locking/queued-spinlocks/arch-support.txt
index 22f2990392ff..cf26042480e2 100644
--- a/Documentation/features/locking/queued-spinlocks/arch-support.txt
+++ b/Documentation/features/locking/queued-spinlocks/arch-support.txt
@@ -20,7 +20,7 @@
     |    openrisc: |  ok  |
     |      parisc: | TODO |
     |     powerpc: |  ok  |
-    |       riscv: | TODO |
+    |       riscv: |  ok  |
     |        s390: | TODO |
     |          sh: | TODO |
     |       sparc: |  ok  |
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 093ee6537331..f5698ecc5ccc 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -82,6 +82,7 @@ config RISCV
 	select ARCH_WANT_OPTIMIZE_HUGETLB_VMEMMAP
 	select ARCH_WANTS_NO_INSTR
 	select ARCH_WANTS_THP_SWAP if HAVE_ARCH_TRANSPARENT_HUGEPAGE
+	select ARCH_WEAK_RELEASE_ACQUIRE if ARCH_USE_QUEUED_SPINLOCKS
 	select BINFMT_FLAT_NO_DATA_START_OFFSET if !MMU
 	select BUILDTIME_TABLE_SORT if MMU
 	select CLINT_TIMER if RISCV_M_MODE
@@ -507,6 +508,39 @@ config NODES_SHIFT
 	  Specify the maximum number of NUMA Nodes available on the target
 	  system.  Increases memory reserved to accommodate various tables.
 
+choice
+	prompt "RISC-V spinlock type"
+	default RISCV_COMBO_SPINLOCKS
+
+config RISCV_TICKET_SPINLOCKS
+	bool "Using ticket spinlock"
+
+config RISCV_QUEUED_SPINLOCKS
+	bool "Using queued spinlock"
+	depends on SMP && MMU && NONPORTABLE
+	select ARCH_USE_QUEUED_SPINLOCKS
+	help
+	  The queued spinlock implementation requires the forward progress
+	  guarantee of cmpxchg()/xchg() atomic operations: CAS with Zabha or
+	  LR/SC with Ziccrse provide such guarantee.
+
+	  Select this if and only if Zabha or Ziccrse is available on your
+	  platform, RISCV_QUEUED_SPINLOCKS must not be selected for platforms
+	  without one of those extensions.
+
+	  If unsure, select RISCV_COMBO_SPINLOCKS, which will use qspinlocks
+	  when supported and otherwise ticket spinlocks.
+
+config RISCV_COMBO_SPINLOCKS
+	bool "Using combo spinlock"
+	depends on SMP && MMU
+	select ARCH_USE_QUEUED_SPINLOCKS
+	help
+	  Embed both queued spinlock and ticket lock so that the spinlock
+	  implementation can be chosen at runtime.
+
+endchoice
+
 config RISCV_ALTERNATIVE
 	bool
 	depends on !XIP_KERNEL
diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild
index 1461af12da6e..de13d5a234f8 100644
--- a/arch/riscv/include/asm/Kbuild
+++ b/arch/riscv/include/asm/Kbuild
@@ -6,10 +6,12 @@ generic-y += early_ioremap.h
 generic-y += flat.h
 generic-y += kvm_para.h
 generic-y += mmzone.h
+generic-y += mcs_spinlock.h
 generic-y += parport.h
-generic-y += spinlock.h
 generic-y += spinlock_types.h
+generic-y += ticket_spinlock.h
 generic-y += qrwlock.h
 generic-y += qrwlock_types.h
+generic-y += qspinlock.h
 generic-y += user.h
 generic-y += vmlinux.lds.h
diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h
new file mode 100644
index 000000000000..e5121b89acea
--- /dev/null
+++ b/arch/riscv/include/asm/spinlock.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ASM_RISCV_SPINLOCK_H
+#define __ASM_RISCV_SPINLOCK_H
+
+#ifdef CONFIG_RISCV_COMBO_SPINLOCKS
+#define _Q_PENDING_LOOPS	(1 << 9)
+
+#define __no_arch_spinlock_redefine
+#include <asm/ticket_spinlock.h>
+#include <asm/qspinlock.h>
+#include <asm/jump_label.h>
+
+/*
+ * TODO: Use an alternative instead of a static key when we are able to parse
+ * the extensions string earlier in the boot process.
+ */
+DECLARE_STATIC_KEY_TRUE(qspinlock_key);
+
+#define SPINLOCK_BASE_DECLARE(op, type, type_lock)			\
+static __always_inline type arch_spin_##op(type_lock lock)		\
+{									\
+	if (static_branch_unlikely(&qspinlock_key))			\
+		return queued_spin_##op(lock);				\
+	return ticket_spin_##op(lock);					\
+}
+
+SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
+SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
+SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
+SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
+SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
+SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
+
+#elif defined(CONFIG_RISCV_QUEUED_SPINLOCKS)
+
+#include <asm/qspinlock.h>
+
+#else
+
+#include <asm/ticket_spinlock.h>
+
+#endif
+
+#include <asm/qrwlock.h>
+
+#endif /* __ASM_RISCV_SPINLOCK_H */
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index a2cde65b69e9..438e4f6ad2ad 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -244,6 +244,42 @@ static void __init parse_dtb(void)
 #endif
 }
 
+#if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
+DEFINE_STATIC_KEY_TRUE(qspinlock_key);
+EXPORT_SYMBOL(qspinlock_key);
+#endif
+
+static void __init riscv_spinlock_init(void)
+{
+	char *using_ext = NULL;
+
+	if (IS_ENABLED(CONFIG_RISCV_TICKET_SPINLOCKS)) {
+		pr_info("Ticket spinlock: enabled\n");
+		return;
+	}
+
+	if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) &&
+	    IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) &&
+	    riscv_isa_extension_available(NULL, ZABHA) &&
+	    riscv_isa_extension_available(NULL, ZACAS)) {
+		using_ext = "using Zabha";
+	} else if (riscv_isa_extension_available(NULL, ZICCRSE)) {
+		using_ext = "using Ziccrse";
+	}
+#if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
+	else {
+		static_branch_disable(&qspinlock_key);
+		pr_info("Ticket spinlock: enabled\n");
+		return;
+	}
+#endif
+
+	if (!using_ext)
+		pr_err("Queued spinlock without Zabha or Ziccrse");
+	else
+		pr_info("Queued spinlock %s: enabled\n", using_ext);
+}
+
 extern void __init init_rt_signal_env(void);
 
 void __init setup_arch(char **cmdline_p)
@@ -297,6 +333,7 @@ void __init setup_arch(char **cmdline_p)
 	riscv_set_dma_cache_alignment();
 
 	riscv_user_isa_enable();
+	riscv_spinlock_init();
 }
 
 bool arch_cpu_is_hotpluggable(int cpu)
diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
index 0655aa5b57b2..bf47cca2c375 100644
--- a/include/asm-generic/qspinlock.h
+++ b/include/asm-generic/qspinlock.h
@@ -136,6 +136,7 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
 }
 #endif
 
+#ifndef __no_arch_spinlock_redefine
 /*
  * Remapping spinlock architecture specific functions to the corresponding
  * queued spinlock functions.
@@ -146,5 +147,6 @@ static __always_inline bool virt_spin_lock(struct qspinlock *lock)
 #define arch_spin_lock(l)		queued_spin_lock(l)
 #define arch_spin_trylock(l)		queued_spin_trylock(l)
 #define arch_spin_unlock(l)		queued_spin_unlock(l)
+#endif
 
 #endif /* __ASM_GENERIC_QSPINLOCK_H */
diff --git a/include/asm-generic/ticket_spinlock.h b/include/asm-generic/ticket_spinlock.h
index cfcff22b37b3..325779970d8a 100644
--- a/include/asm-generic/ticket_spinlock.h
+++ b/include/asm-generic/ticket_spinlock.h
@@ -89,6 +89,7 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
 	return (s16)((val >> 16) - (val & 0xffff)) > 1;
 }
 
+#ifndef __no_arch_spinlock_redefine
 /*
  * Remapping spinlock architecture specific functions to the corresponding
  * ticket spinlock functions.
@@ -99,5 +100,6 @@ static __always_inline int ticket_spin_is_contended(arch_spinlock_t *lock)
 #define arch_spin_lock(l)		ticket_spin_lock(l)
 #define arch_spin_trylock(l)		ticket_spin_trylock(l)
 #define arch_spin_unlock(l)		ticket_spin_unlock(l)
+#endif
 
 #endif /* __ASM_GENERIC_TICKET_SPINLOCK_H */
-- 
2.39.2

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[kernel test robot]

Hi Alexandre,

kernel test robot noticed the following build warnings:

[auto build test WARNING on arnd-asm-generic/master]
[also build test WARNING on robh/for-next tip/locking/core linus/master v6.12-rc6]
[cannot apply to next-20241101]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexandre-Ghiti/riscv-Move-cpufeature-h-macros-into-their-own-header/20241103-230614
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git master
patch link:    https://lore.kernel.org/r/20241103145153.105097-14-alexghiti%40rivosinc.com
patch subject: [PATCH v6 13/13] riscv: Add qspinlock support
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411041609.gxjI2dsw-lkp@intel.com/

includecheck warnings: (new ones prefixed by >>)
>> arch/riscv/include/asm/spinlock.h: asm/ticket_spinlock.h is included more than once.
>> arch/riscv/include/asm/spinlock.h: asm/qspinlock.h is included more than once.

vim +10 arch/riscv/include/asm/spinlock.h

     8	
     9	#define __no_arch_spinlock_redefine
  > 10	#include <asm/ticket_spinlock.h>
    11	#include <asm/qspinlock.h>
    12	#include <asm/jump_label.h>
    13	
    14	/*
    15	 * TODO: Use an alternative instead of a static key when we are able to parse
    16	 * the extensions string earlier in the boot process.
    17	 */
    18	DECLARE_STATIC_KEY_TRUE(qspinlock_key);
    19	
    20	#define SPINLOCK_BASE_DECLARE(op, type, type_lock)			\
    21	static __always_inline type arch_spin_##op(type_lock lock)		\
    22	{									\
    23		if (static_branch_unlikely(&qspinlock_key))			\
    24			return queued_spin_##op(lock);				\
    25		return ticket_spin_##op(lock);					\
    26	}
    27	
    28	SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
    29	SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
    30	SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
    31	SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
    32	SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
    33	SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
    34	
    35	#elif defined(CONFIG_RISCV_QUEUED_SPINLOCKS)
    36	
    37	#include <asm/qspinlock.h>
    38	
    39	#else
    40	
  > 41	#include <asm/ticket_spinlock.h>
    42	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Alexandre Ghiti]

On Mon, Nov 4, 2024 at 10:05 AM kernel test robot <lkp@intel.com> wrote:
>
> Hi Alexandre,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on arnd-asm-generic/master]
> [also build test WARNING on robh/for-next tip/locking/core linus/master v6.12-rc6]
> [cannot apply to next-20241101]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url:    https://github.com/intel-lab-lkp/linux/commits/Alexandre-Ghiti/riscv-Move-cpufeature-h-macros-into-their-own-header/20241103-230614
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git master
> patch link:    https://lore.kernel.org/r/20241103145153.105097-14-alexghiti%40rivosinc.com
> patch subject: [PATCH v6 13/13] riscv: Add qspinlock support
> compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202411041609.gxjI2dsw-lkp@intel.com/
>
> includecheck warnings: (new ones prefixed by >>)
> >> arch/riscv/include/asm/spinlock.h: asm/ticket_spinlock.h is included more than once.
> >> arch/riscv/include/asm/spinlock.h: asm/qspinlock.h is included more than once.

Yes but that's in a #ifdef/#elif#else clause so nothing to do here!

>
> vim +10 arch/riscv/include/asm/spinlock.h
>
>      8
>      9  #define __no_arch_spinlock_redefine
>   > 10  #include <asm/ticket_spinlock.h>
>     11  #include <asm/qspinlock.h>
>     12  #include <asm/jump_label.h>
>     13
>     14  /*
>     15   * TODO: Use an alternative instead of a static key when we are able to parse
>     16   * the extensions string earlier in the boot process.
>     17   */
>     18  DECLARE_STATIC_KEY_TRUE(qspinlock_key);
>     19
>     20  #define SPINLOCK_BASE_DECLARE(op, type, type_lock)                      \
>     21  static __always_inline type arch_spin_##op(type_lock lock)              \
>     22  {                                                                       \
>     23          if (static_branch_unlikely(&qspinlock_key))                     \
>     24                  return queued_spin_##op(lock);                          \
>     25          return ticket_spin_##op(lock);                                  \
>     26  }
>     27
>     28  SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
>     29  SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
>     30  SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
>     31  SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
>     32  SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
>     33  SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
>     34
>     35  #elif defined(CONFIG_RISCV_QUEUED_SPINLOCKS)
>     36
>     37  #include <asm/qspinlock.h>
>     38
>     39  #else
>     40
>   > 41  #include <asm/ticket_spinlock.h>
>     42
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Philip Li]

On Mon, Nov 04, 2024 at 10:09:07AM +0100, Alexandre Ghiti wrote:
> On Mon, Nov 4, 2024 at 10:05 AM kernel test robot <lkp@intel.com> wrote:
> >
> > Hi Alexandre,
> >
> > kernel test robot noticed the following build warnings:
> >
> > [auto build test WARNING on arnd-asm-generic/master]
> > [also build test WARNING on robh/for-next tip/locking/core linus/master v6.12-rc6]
> > [cannot apply to next-20241101]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> >
> > url:    https://github.com/intel-lab-lkp/linux/commits/Alexandre-Ghiti/riscv-Move-cpufeature-h-macros-into-their-own-header/20241103-230614
> > base:   https://git.kernel.org/pub/scm/linux/kernel/git/arnd/asm-generic.git master
> > patch link:    https://lore.kernel.org/r/20241103145153.105097-14-alexghiti%40rivosinc.com
> > patch subject: [PATCH v6 13/13] riscv: Add qspinlock support
> > compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
> >
> > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > the same patch/commit), kindly add following tags
> > | Reported-by: kernel test robot <lkp@intel.com>
> > | Closes: https://lore.kernel.org/oe-kbuild-all/202411041609.gxjI2dsw-lkp@intel.com/
> >
> > includecheck warnings: (new ones prefixed by >>)
> > >> arch/riscv/include/asm/spinlock.h: asm/ticket_spinlock.h is included more than once.
> > >> arch/riscv/include/asm/spinlock.h: asm/qspinlock.h is included more than once.
> 
> Yes but that's in a #ifdef/#elif#else clause so nothing to do here!

Thanks for the info, we will fix the bot. Sorry for the false positive report.

> 
> >
> > vim +10 arch/riscv/include/asm/spinlock.h
> >
> >      8
> >      9  #define __no_arch_spinlock_redefine
> >   > 10  #include <asm/ticket_spinlock.h>
> >     11  #include <asm/qspinlock.h>
> >     12  #include <asm/jump_label.h>
> >     13
> >     14  /*
> >     15   * TODO: Use an alternative instead of a static key when we are able to parse
> >     16   * the extensions string earlier in the boot process.
> >     17   */
> >     18  DECLARE_STATIC_KEY_TRUE(qspinlock_key);
> >     19
> >     20  #define SPINLOCK_BASE_DECLARE(op, type, type_lock)                      \
> >     21  static __always_inline type arch_spin_##op(type_lock lock)              \
> >     22  {                                                                       \
> >     23          if (static_branch_unlikely(&qspinlock_key))                     \
> >     24                  return queued_spin_##op(lock);                          \
> >     25          return ticket_spin_##op(lock);                                  \
> >     26  }
> >     27
> >     28  SPINLOCK_BASE_DECLARE(lock, void, arch_spinlock_t *)
> >     29  SPINLOCK_BASE_DECLARE(unlock, void, arch_spinlock_t *)
> >     30  SPINLOCK_BASE_DECLARE(is_locked, int, arch_spinlock_t *)
> >     31  SPINLOCK_BASE_DECLARE(is_contended, int, arch_spinlock_t *)
> >     32  SPINLOCK_BASE_DECLARE(trylock, bool, arch_spinlock_t *)
> >     33  SPINLOCK_BASE_DECLARE(value_unlocked, int, arch_spinlock_t)
> >     34
> >     35  #elif defined(CONFIG_RISCV_QUEUED_SPINLOCKS)
> >     36
> >     37  #include <asm/qspinlock.h>
> >     38
> >     39  #else
> >     40
> >   > 41  #include <asm/ticket_spinlock.h>
> >     42
> >
> > --
> > 0-DAY CI Kernel Test Service
> > https://github.com/intel/lkp-tests/wiki
> 

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Will Deacon]

On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> In order to produce a generic kernel, a user can select
> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> spinlock implementation if Zabha or Ziccrse are not present.
> 
> Note that we can't use alternatives here because the discovery of
> extensions is done too late and we need to start with the qspinlock
> implementation because the ticket spinlock implementation would pollute
> the spinlock value, so let's use static keys.

I think the static key toggling takes a mutex (jump_label_lock()) which
can take a spinlock (lock->wait_lock) internally, so I don't grok how
this works:

> +static void __init riscv_spinlock_init(void)
> +{
> +	char *using_ext = NULL;
> +
> +	if (IS_ENABLED(CONFIG_RISCV_TICKET_SPINLOCKS)) {
> +		pr_info("Ticket spinlock: enabled\n");
> +		return;
> +	}
> +
> +	if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) &&
> +	    IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) &&
> +	    riscv_isa_extension_available(NULL, ZABHA) &&
> +	    riscv_isa_extension_available(NULL, ZACAS)) {
> +		using_ext = "using Zabha";
> +	} else if (riscv_isa_extension_available(NULL, ZICCRSE)) {
> +		using_ext = "using Ziccrse";
> +	}
> +#if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
> +	else {
> +		static_branch_disable(&qspinlock_key);
> +		pr_info("Ticket spinlock: enabled\n");
> +		return;
> +	}
> +#endif

i.e. we've potentially already used the qspinlock at this point.

Will

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Guo Ren]

On Tue, Nov 12, 2024 at 12:43 AM Will Deacon <will@kernel.org> wrote:
>
> On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > In order to produce a generic kernel, a user can select
> > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > spinlock implementation if Zabha or Ziccrse are not present.
> >
> > Note that we can't use alternatives here because the discovery of
> > extensions is done too late and we need to start with the qspinlock
> > implementation because the ticket spinlock implementation would pollute
> > the spinlock value, so let's use static keys.
>
> I think the static key toggling takes a mutex (jump_label_lock()) which
> can take a spinlock (lock->wait_lock) internally, so I don't grok how
> this works:
>
> > +static void __init riscv_spinlock_init(void)
> > +{
> > +     char *using_ext = NULL;
> > +
> > +     if (IS_ENABLED(CONFIG_RISCV_TICKET_SPINLOCKS)) {
> > +             pr_info("Ticket spinlock: enabled\n");
> > +             return;
> > +     }
> > +
> > +     if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) &&
> > +         IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) &&
> > +         riscv_isa_extension_available(NULL, ZABHA) &&
> > +         riscv_isa_extension_available(NULL, ZACAS)) {
> > +             using_ext = "using Zabha";
> > +     } else if (riscv_isa_extension_available(NULL, ZICCRSE)) {
> > +             using_ext = "using Ziccrse";
> > +     }
> > +#if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
> > +     else {
> > +             static_branch_disable(&qspinlock_key);
> > +             pr_info("Ticket spinlock: enabled\n");
> > +             return;
> > +     }
> > +#endif
>
> i.e. we've potentially already used the qspinlock at this point.
Yes, I've used qspinlock here. But riscv_spinlock_init is called with
irq_disabled and smp_off. That means this qspinlock only performs a
test-set lock behavior by qspinlock fast-path.

The qspinlock is a clean implementation. After qspin_unlock, the lock
value remains at zero, but the ticket lock makes the value dirty. So
we use Qspinlock at first or change it to ticket-lock before irq & smp
up.

>
> Will



-- 
Best Regards
 Guo Ren

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Will Deacon]

On Tue, Nov 12, 2024 at 09:49:15AM +0800, Guo Ren wrote:
> On Tue, Nov 12, 2024 at 12:43 AM Will Deacon <will@kernel.org> wrote:
> >
> > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > In order to produce a generic kernel, a user can select
> > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > spinlock implementation if Zabha or Ziccrse are not present.
> > >
> > > Note that we can't use alternatives here because the discovery of
> > > extensions is done too late and we need to start with the qspinlock
> > > implementation because the ticket spinlock implementation would pollute
> > > the spinlock value, so let's use static keys.
> >
> > I think the static key toggling takes a mutex (jump_label_lock()) which
> > can take a spinlock (lock->wait_lock) internally, so I don't grok how
> > this works:
> >
> > > +static void __init riscv_spinlock_init(void)
> > > +{
> > > +     char *using_ext = NULL;
> > > +
> > > +     if (IS_ENABLED(CONFIG_RISCV_TICKET_SPINLOCKS)) {
> > > +             pr_info("Ticket spinlock: enabled\n");
> > > +             return;
> > > +     }
> > > +
> > > +     if (IS_ENABLED(CONFIG_RISCV_ISA_ZABHA) &&
> > > +         IS_ENABLED(CONFIG_RISCV_ISA_ZACAS) &&
> > > +         riscv_isa_extension_available(NULL, ZABHA) &&
> > > +         riscv_isa_extension_available(NULL, ZACAS)) {
> > > +             using_ext = "using Zabha";
> > > +     } else if (riscv_isa_extension_available(NULL, ZICCRSE)) {
> > > +             using_ext = "using Ziccrse";
> > > +     }
> > > +#if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
> > > +     else {
> > > +             static_branch_disable(&qspinlock_key);
> > > +             pr_info("Ticket spinlock: enabled\n");
> > > +             return;
> > > +     }
> > > +#endif
> >
> > i.e. we've potentially already used the qspinlock at this point.
> Yes, I've used qspinlock here. But riscv_spinlock_init is called with
> irq_disabled and smp_off. That means this qspinlock only performs a
> test-set lock behavior by qspinlock fast-path.

That's... horrendous.

Will

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Conor Dooley]

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

On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> In order to produce a generic kernel, a user can select
> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> spinlock implementation if Zabha or Ziccrse are not present.
> 
> Note that we can't use alternatives here because the discovery of
> extensions is done too late and we need to start with the qspinlock
> implementation because the ticket spinlock implementation would pollute
> the spinlock value, so let's use static keys.
> 
> This is largely based on Guo's work and Leonardo reviews at [1].
> 
> Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> Signed-off-by: Guo Ren <guoren@kernel.org>
> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>

This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
breaks boot on polarfire soc. It dies before outputting anything to the
console. My .config has:

# CONFIG_RISCV_TICKET_SPINLOCKS is not set
# CONFIG_RISCV_QUEUED_SPINLOCKS is not set
CONFIG_RISCV_COMBO_SPINLOCKS=y

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

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Will Deacon]

On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > In order to produce a generic kernel, a user can select
> > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > spinlock implementation if Zabha or Ziccrse are not present.
> > 
> > Note that we can't use alternatives here because the discovery of
> > extensions is done too late and we need to start with the qspinlock
> > implementation because the ticket spinlock implementation would pollute
> > the spinlock value, so let's use static keys.
> > 
> > This is largely based on Guo's work and Leonardo reviews at [1].
> > 
> > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > Signed-off-by: Guo Ren <guoren@kernel.org>
> > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> 
> This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> breaks boot on polarfire soc. It dies before outputting anything to the
> console. My .config has:
> 
> # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> CONFIG_RISCV_COMBO_SPINLOCKS=y

I pointed out some of the fragility during review:

https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/

so I'm kinda surprised it got merged tbh :/

Will

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Conor Dooley]

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

On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > In order to produce a generic kernel, a user can select
> > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > spinlock implementation if Zabha or Ziccrse are not present.
> > > 
> > > Note that we can't use alternatives here because the discovery of
> > > extensions is done too late and we need to start with the qspinlock
> > > implementation because the ticket spinlock implementation would pollute
> > > the spinlock value, so let's use static keys.
> > > 
> > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > 
> > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > 
> > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > breaks boot on polarfire soc. It dies before outputting anything to the
> > console. My .config has:
> > 
> > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > CONFIG_RISCV_COMBO_SPINLOCKS=y
> 
> I pointed out some of the fragility during review:
> 
> https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> 
> so I'm kinda surprised it got merged tbh :/

Maybe it could be reverted rather than having a broken boot with the
default settings in -rc1.

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

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Alexandre Ghiti]

On 28/11/2024 15:14, Conor Dooley wrote:
> On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
>> On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
>>> On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
>>>> In order to produce a generic kernel, a user can select
>>>> CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
>>>> spinlock implementation if Zabha or Ziccrse are not present.
>>>>
>>>> Note that we can't use alternatives here because the discovery of
>>>> extensions is done too late and we need to start with the qspinlock
>>>> implementation because the ticket spinlock implementation would pollute
>>>> the spinlock value, so let's use static keys.
>>>>
>>>> This is largely based on Guo's work and Leonardo reviews at [1].
>>>>
>>>> Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
>>>> Signed-off-by: Guo Ren <guoren@kernel.org>
>>>> Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
>>> This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
>>> breaks boot on polarfire soc. It dies before outputting anything to the
>>> console. My .config has:
>>>
>>> # CONFIG_RISCV_TICKET_SPINLOCKS is not set
>>> # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
>>> CONFIG_RISCV_COMBO_SPINLOCKS=y
>> I pointed out some of the fragility during review:
>>
>> https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
>>
>> so I'm kinda surprised it got merged tbh :/
> Maybe it could be reverted rather than having a broken boot with the
> default settings in -rc1.


No need to rush before we know what's happening,I guess you bisected to 
this commit right?

I don't have this soc, so can you provide $stval/$sepc/$scause, a 
config, a kernel, anything?

Does the polarfire soc provide Ziccrse?


>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2807 bytes --]

On Thu, Nov 28, 2024 at 03:50:09PM +0100, Alexandre Ghiti wrote:
> On 28/11/2024 15:14, Conor Dooley wrote:
> > On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> > > On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > > > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > > > In order to produce a generic kernel, a user can select
> > > > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > > spinlock implementation if Zabha or Ziccrse are not present.
> > > > > 
> > > > > Note that we can't use alternatives here because the discovery of
> > > > > extensions is done too late and we need to start with the qspinlock
> > > > > implementation because the ticket spinlock implementation would pollute
> > > > > the spinlock value, so let's use static keys.
> > > > > 
> > > > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > > > 
> > > > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > > > breaks boot on polarfire soc. It dies before outputting anything to the
> > > > console. My .config has:
> > > > 
> > > > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > > > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > > > CONFIG_RISCV_COMBO_SPINLOCKS=y
> > > I pointed out some of the fragility during review:
> > > 
> > > https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> > > 
> > > so I'm kinda surprised it got merged tbh :/
> > Maybe it could be reverted rather than having a broken boot with the
> > default settings in -rc1.
> 
> 
> No need to rush before we know what's happening,I guess you bisected to this
> commit right?

The symptom is a failure to boot, without any console output, of course
I bisected it before blaming something specific. But I don't think it is
"rushing" as having -rc1 broken with an option's default is a massive pain
in the arse when it comes to testing.

> I don't have this soc, so can you provide $stval/$sepc/$scause, a config, a
> kernel, anything?

I don't have the former cos it died immediately on boot. config is
attached. It reproduces in QEMU so you don't need any hardware.

> Does the polarfire soc provide Ziccrse?

I don't think that is relevant because ziccrse is not listed in the dts,
so the kernel should not be assuming that LR/SC has a forward progress
guarantee. It's not even getting as far as riscv_spinlock_init() given
several things before that should be emitting logs, so it doesn't even
get to make any decisions about Ziccrse.

[-- Attachment #1.2: qspinlocks_config --]
[-- Type: text/plain, Size: 4678 bytes --]

CONFIG_SYSVIPC=y
CONFIG_POSIX_MQUEUE=y
CONFIG_GENERIC_IRQ_DEBUGFS=y
CONFIG_NO_HZ_IDLE=y
CONFIG_HIGH_RES_TIMERS=y
CONFIG_BPF_SYSCALL=y
CONFIG_PREEMPT_RT=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y
CONFIG_CGROUPS=y
CONFIG_CGROUP_SCHED=y
CONFIG_CFS_BANDWIDTH=y
CONFIG_CGROUP_BPF=y
CONFIG_NAMESPACES=y
CONFIG_USER_NS=y
CONFIG_CHECKPOINT_RESTORE=y
CONFIG_BLK_DEV_INITRD=y
CONFIG_EXPERT=y
# CONFIG_SYSFS_SYSCALL is not set
CONFIG_PROFILING=y
CONFIG_KEXEC=y
CONFIG_KEXEC_FILE=y
CONFIG_ARCH_MICROCHIP=y
CONFIG_ARCH_RENESAS=y
CONFIG_ARCH_SOPHGO=y
CONFIG_SOC_STARFIVE=y
CONFIG_ARCH_THEAD=y
CONFIG_ARCH_VIRT=y
CONFIG_NONPORTABLE=y
CONFIG_SMP=y
CONFIG_RANDOMIZE_BASE=y
CONFIG_CMDLINE="earlycon keep_bootcon reboot=cold"
CONFIG_HIBERNATION=y
CONFIG_CPU_FREQ=y
CONFIG_VIRTUALIZATION=y
CONFIG_KVM=m
CONFIG_ACPI=y
CONFIG_JUMP_LABEL=y
CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y
CONFIG_SPARSEMEM_MANUAL=y
CONFIG_NET=y
CONFIG_PACKET=y
CONFIG_IP_MULTICAST=y
CONFIG_IP_ADVANCED_ROUTER=y
CONFIG_IP_PNP=y
CONFIG_IP_PNP_DHCP=y
CONFIG_IP_PNP_BOOTP=y
CONFIG_IP_PNP_RARP=y
CONFIG_NETLINK_DIAG=y
CONFIG_NET_9P=y
CONFIG_NET_9P_VIRTIO=y
CONFIG_PCI=y
CONFIG_PCIEPORTBUS=y
CONFIG_PCI_HOST_GENERIC=y
CONFIG_PCIE_XILINX=y
CONFIG_PCIE_MICROCHIP_HOST=y
CONFIG_DEVTMPFS=y
CONFIG_DEVTMPFS_MOUNT=y
CONFIG_FW_LOADER_USER_HELPER=y
CONFIG_DEBUG_DRIVER=y
CONFIG_AX45MP_L2_CACHE=y
CONFIG_SIFIVE_CCACHE=y
CONFIG_EFI_ZBOOT=y
CONFIG_MTD=y
CONFIG_MTD_CMDLINE_PARTS=y
CONFIG_MTD_CFI=y
CONFIG_MTD_JEDECPROBE=y
CONFIG_MTD_SPI_NAND=y
CONFIG_MTD_SPI_NOR=y
# CONFIG_MTD_SPI_NOR_USE_4K_SECTORS is not set
CONFIG_OF_OVERLAY=y
CONFIG_ZRAM=y
CONFIG_ZRAM_MEMORY_TRACKING=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_VIRTIO_BLK=y
CONFIG_BLK_DEV_NVME=y
CONFIG_BLK_DEV_SD=y
CONFIG_BLK_DEV_SR=y
CONFIG_SCSI_VIRTIO=y
CONFIG_ATA=y
CONFIG_SATA_AHCI=y
CONFIG_SATA_AHCI_PLATFORM=y
CONFIG_NETDEVICES=y
CONFIG_VIRTIO_NET=y
CONFIG_MACB=y
CONFIG_E1000E=y
CONFIG_R8169=y
CONFIG_MICROSEMI_PHY=y
CONFIG_VITESSE_PHY=y
CONFIG_INPUT_MOUSEDEV=y
CONFIG_INPUT_JOYDEV=m
CONFIG_INPUT_JOYSTICK=y
CONFIG_JOYSTICK_SENSEHAT=m
CONFIG_SERIAL_8250=y
CONFIG_SERIAL_8250_CONSOLE=y
CONFIG_SERIAL_OF_PLATFORM=y
CONFIG_VIRTIO_CONSOLE=y
CONFIG_HW_RANDOM=y
CONFIG_HW_RANDOM_VIRTIO=y
CONFIG_HW_RANDOM_POLARFIRE_SOC=y
CONFIG_I2C=y
CONFIG_I2C_CHARDEV=y
CONFIG_I2C_MICROCHIP_CORE=y
CONFIG_SPI=y
CONFIG_SPI_MICROCHIP_CORE=y
CONFIG_SPI_MICROCHIP_CORE_QSPI=y
CONFIG_SPI_SIFIVE=y
# CONFIG_PTP_1588_CLOCK is not set
CONFIG_GPIO_SYSFS=y
CONFIG_GPIO_POLARFIRE_SOC=y
CONFIG_GPIO_SIFIVE=y
CONFIG_AUXDISPLAY=y
CONFIG_FB=y
CONFIG_FRAMEBUFFER_CONSOLE=y
CONFIG_SOUND=y
CONFIG_SND=y
CONFIG_SND_SOC=y
CONFIG_SND_SOC_MAX9867=y
CONFIG_USB=y
CONFIG_USB_XHCI_HCD=y
CONFIG_USB_XHCI_PLATFORM=y
CONFIG_USB_EHCI_HCD=y
CONFIG_USB_EHCI_HCD_PLATFORM=y
CONFIG_USB_OHCI_HCD=y
CONFIG_USB_OHCI_HCD_PLATFORM=y
CONFIG_USB_STORAGE=y
CONFIG_USB_UAS=y
CONFIG_USB_MUSB_HDRC=y
CONFIG_USB_MUSB_POLARFIRE_SOC=y
CONFIG_USB_INVENTRA_DMA=y
CONFIG_NOP_USB_XCEIV=y
CONFIG_MMC=y
CONFIG_MMC_SDHCI=y
CONFIG_MMC_SDHCI_PLTFM=y
CONFIG_MMC_SDHCI_CADENCE=y
CONFIG_MMC_SPI=y
CONFIG_NEW_LEDS=y
CONFIG_LEDS_CLASS=y
CONFIG_LEDS_GPIO=y
CONFIG_LEDS_PWM=y
CONFIG_RTC_CLASS=y
CONFIG_RTC_DRV_PCF2123=y
CONFIG_DMADEVICES=y
CONFIG_SF_PDMA=y
CONFIG_VIRTIO_PCI=y
CONFIG_VIRTIO_BALLOON=y
CONFIG_VIRTIO_INPUT=y
CONFIG_VIRTIO_MMIO=y
CONFIG_MAILBOX=y
CONFIG_POLARFIRE_SOC_MAILBOX=y
CONFIG_RPMSG_CHAR=y
CONFIG_RPMSG_CTRL=y
CONFIG_RPMSG_VIRTIO=y
CONFIG_POLARFIRE_SOC_SYS_CTRL=y
CONFIG_IIO=y
CONFIG_ADXL345_SPI=y
CONFIG_MCP320X=y
CONFIG_MCP3564=y
CONFIG_PAC1934=y
CONFIG_SD_ADC_MODULATOR=y
CONFIG_HTS221=m
CONFIG_IIO_ST_LSM6DSX=m
CONFIG_IIO_ST_MAGN_3AXIS=m
CONFIG_IIO_ST_PRESS=m
CONFIG_PWM=y
CONFIG_PWM_DEBUG=y
CONFIG_PWM_MICROCHIP_CORE=y
CONFIG_LIBNVDIMM=y
CONFIG_FPGA=y
CONFIG_FPGA_BRIDGE=y
CONFIG_FPGA_REGION=y
CONFIG_FPGA_MGR_MICROCHIP_SPI=y
CONFIG_EXT4_FS=y
CONFIG_EXT4_FS_POSIX_ACL=y
CONFIG_MSDOS_FS=y
CONFIG_VFAT_FS=y
CONFIG_TMPFS=y
CONFIG_TMPFS_POSIX_ACL=y
CONFIG_HUGETLBFS=y
CONFIG_NFS_FS=y
CONFIG_NFS_V3_ACL=y
CONFIG_NFS_V4=y
CONFIG_NFS_V4_1=y
CONFIG_NFS_V4_2=y
CONFIG_NFS_V4_1_MIGRATION=y
CONFIG_ROOT_NFS=y
CONFIG_9P_FS=y
CONFIG_NLS_CODEPAGE_437=y
CONFIG_NLS_ISO8859_1=m
CONFIG_CRYPTO_DEFLATE=y
CONFIG_CRYPTO_ZSTD=y
CONFIG_CRYPTO_USER_API_HASH=y
CONFIG_CRYPTO_DEV_VIRTIO=y
CONFIG_PRINTK_TIME=y
CONFIG_DYNAMIC_DEBUG=y
CONFIG_MAGIC_SYSRQ=y
CONFIG_DEBUG_FS=y
CONFIG_DEBUG_PAGEALLOC=y
CONFIG_SCHED_STACK_END_CHECK=y
CONFIG_DEBUG_VM=y
CONFIG_DEBUG_VM_PGFLAGS=y
CONFIG_DEBUG_MEMORY_INIT=y
CONFIG_DEBUG_PER_CPU_MAPS=y
CONFIG_SOFTLOCKUP_DETECTOR=y
CONFIG_WQ_WATCHDOG=y
CONFIG_PROVE_LOCKING=y
CONFIG_DEBUG_ATOMIC_SLEEP=y
CONFIG_DEBUG_LIST=y
CONFIG_DEBUG_PLIST=y
CONFIG_DEBUG_SG=y
# CONFIG_RCU_TRACE is not set
CONFIG_RCU_EQS_DEBUG=y
CONFIG_SAMPLES=y
CONFIG_MEMTEST=y

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

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Guo Ren]

On Fri, Nov 29, 2024 at 12:19 AM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Nov 28, 2024 at 03:50:09PM +0100, Alexandre Ghiti wrote:
> > On 28/11/2024 15:14, Conor Dooley wrote:
> > > On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> > > > On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > > > > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > > > > In order to produce a generic kernel, a user can select
> > > > > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > > > spinlock implementation if Zabha or Ziccrse are not present.
> > > > > >
> > > > > > Note that we can't use alternatives here because the discovery of
> > > > > > extensions is done too late and we need to start with the qspinlock
> > > > > > implementation because the ticket spinlock implementation would pollute
> > > > > > the spinlock value, so let's use static keys.
> > > > > >
> > > > > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > > > >
> > > > > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > > > > breaks boot on polarfire soc. It dies before outputting anything to the
> > > > > console. My .config has:
> > > > >
> > > > > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > > > > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > > > > CONFIG_RISCV_COMBO_SPINLOCKS=y
> > > > I pointed out some of the fragility during review:
> > > >
> > > > https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> > > >
> > > > so I'm kinda surprised it got merged tbh :/
> > > Maybe it could be reverted rather than having a broken boot with the
> > > default settings in -rc1.
> >
> >
> > No need to rush before we know what's happening,I guess you bisected to this
> > commit right?
>
> The symptom is a failure to boot, without any console output, of course
> I bisected it before blaming something specific. But I don't think it is
> "rushing" as having -rc1 broken with an option's default is a massive pain
> in the arse when it comes to testing.
>
> > I don't have this soc, so can you provide $stval/$sepc/$scause, a config, a
> > kernel, anything?
>
> I don't have the former cos it died immediately on boot. config is
> attached. It reproduces in QEMU so you don't need any hardware.
If QEMU could reproduce, could you provide a dmesg by the below method?

Qemu cmd append: -s -S
ref: https://qemu-project.gitlab.io/qemu/system/gdb.html

Connect gdb and in console:
1. file vmlinux
2. source ./Documentation/admin-guide/kdump/gdbmacros.txt
3. dmesg

Then, we could get the kernel's early boot logs from memory.

>
> > Does the polarfire soc provide Ziccrse?
>
> I don't think that is relevant because ziccrse is not listed in the dts,
> so the kernel should not be assuming that LR/SC has a forward progress
> guarantee. It's not even getting as far as riscv_spinlock_init() given
> several things before that should be emitting logs, so it doesn't even
> get to make any decisions about Ziccrse.



-- 
Best Regards
 Guo Ren

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Guo Ren]

On Fri, Nov 29, 2024 at 8:55 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Fri, Nov 29, 2024 at 12:19 AM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Nov 28, 2024 at 03:50:09PM +0100, Alexandre Ghiti wrote:
> > > On 28/11/2024 15:14, Conor Dooley wrote:
> > > > On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> > > > > On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > > > > > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > > > > > In order to produce a generic kernel, a user can select
> > > > > > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > > > > spinlock implementation if Zabha or Ziccrse are not present.
> > > > > > >
> > > > > > > Note that we can't use alternatives here because the discovery of
> > > > > > > extensions is done too late and we need to start with the qspinlock
> > > > > > > implementation because the ticket spinlock implementation would pollute
> > > > > > > the spinlock value, so let's use static keys.
> > > > > > >
> > > > > > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > > > > >
> > > > > > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > > > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > > > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > > > > > breaks boot on polarfire soc. It dies before outputting anything to the
> > > > > > console. My .config has:
> > > > > >
> > > > > > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > > > > > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > > > > > CONFIG_RISCV_COMBO_SPINLOCKS=y
> > > > > I pointed out some of the fragility during review:
> > > > >
> > > > > https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> > > > >
> > > > > so I'm kinda surprised it got merged tbh :/
> > > > Maybe it could be reverted rather than having a broken boot with the
> > > > default settings in -rc1.
> > >
> > >
> > > No need to rush before we know what's happening,I guess you bisected to this
> > > commit right?
> >
> > The symptom is a failure to boot, without any console output, of course
> > I bisected it before blaming something specific. But I don't think it is
> > "rushing" as having -rc1 broken with an option's default is a massive pain
> > in the arse when it comes to testing.
> >
> > > I don't have this soc, so can you provide $stval/$sepc/$scause, a config, a
> > > kernel, anything?
> >
> > I don't have the former cos it died immediately on boot. config is
> > attached. It reproduces in QEMU so you don't need any hardware.
> If QEMU could reproduce, could you provide a dmesg by the below method?
>
> Qemu cmd append: -s -S
> ref: https://qemu-project.gitlab.io/qemu/system/gdb.html
>
> Connect gdb and in console:
> 1. file vmlinux
> 2. source ./Documentation/admin-guide/kdump/gdbmacros.txt
> 3. dmesg
>
> Then, we could get the kernel's early boot logs from memory.
I've reproduced it on qemu, thx for the config.

Reading symbols from ../build-rv64lp64/vmlinux...
(gdb) tar rem:1234
Remote debugging using :1234
ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
/home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
49              atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
(gdb) bt
#0  ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
/home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
#1  arch_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
/home/guoren/source/kernel/linux/arch/riscv/include/asm/spinlock.h:28
#2  do_raw_spin_lock (lock=lock@entry=0xffffffff81b9a5b8 <text_mutex>)
at /home/guoren/source/kernel/linux/kernel/locking/spinlock_debug.c:116
#3  0xffffffff80b2ea0e in __raw_spin_lock_irqsave
(lock=0xffffffff81b9a5b8 <text_mutex>) at
/home/guoren/source/kernel/linux/include/linux/spinlock_api_smp.h:111
#4  _raw_spin_lock_irqsave (lock=lock@entry=0xffffffff81b9a5b8
<text_mutex>) at
/home/guoren/source/kernel/linux/kernel/locking/spinlock.c:162
#5  0xffffffff80b27c54 in rt_mutex_slowtrylock
(lock=0xffffffff81b9a5b8 <text_mutex>) at
/home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1393
#6  0xffffffff80b295ea in rt_mutex_try_acquire
(lock=0xffffffff81b9a5b8 <text_mutex>) at
/home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:319
#7  __rt_mutex_lock (state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
/home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1805
#8  __mutex_lock_common (ip=18446744071562135170, nest_lock=0x0,
subclass=0, state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
/home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:518
#9  mutex_lock_nested (lock=0xffffffff81b9a5b8 <text_mutex>,
subclass=subclass@entry=0) at
/home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:529
#10 0xffffffff80010682 in arch_jump_label_transform_queue
(entry=entry@entry=0xffffffff8158da28, type=<optimized out>) at
/home/guoren/source/kernel/linux/arch/riscv/kernel/jump_label.c:39
#11 0xffffffff801d86b2 in __jump_label_update
(key=key@entry=0xffffffff81a1abb0 <qspinlock_key>,
entry=0xffffffff8158da28, stop=stop@entry=0xffffffff815a5e68
<__tracepoint_ptr_initcall_finish>, init=init@entry=true)
    at /home/guoren/source/kernel/linux/kernel/jump_label.c:513
#12 0xffffffff801d890c in jump_label_update
(key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
/home/guoren/source/kernel/linux/kernel/jump_label.c:920
#13 0xffffffff801d8be8 in static_key_disable_cpuslocked
(key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
/home/guoren/source/kernel/linux/kernel/jump_label.c:240
#14 0xffffffff801d8c04 in static_key_disable
(key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
/home/guoren/source/kernel/linux/kernel/jump_label.c:248
#15 0xffffffff80c04a1a in riscv_spinlock_init () at
/home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:271
#16 setup_arch (cmdline_p=cmdline_p@entry=0xffffffff81a03e88) at
/home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:336
#17 0xffffffff80c007a2 in start_kernel () at
/home/guoren/source/kernel/linux/init/main.c:922
#18 0xffffffff80001164 in _start_kernel ()
Backtrace stopped: frame did not save the PC
(gdb) p /x lock
$1 = 0xffffffff81b9a5b8
(gdb) p /x *lock
$2 = {{val = {counter = 0x20000}, {locked = 0x0, pending = 0x0},
{locked_pending = 0x0, tail = 0x2}}}

>
> >
> > > Does the polarfire soc provide Ziccrse?
> >
> > I don't think that is relevant because ziccrse is not listed in the dts,
> > so the kernel should not be assuming that LR/SC has a forward progress
> > guarantee. It's not even getting as far as riscv_spinlock_init() given
> > several things before that should be emitting logs, so it doesn't even
> > get to make any decisions about Ziccrse.
>
>
>
> --
> Best Regards
>  Guo Ren



-- 
Best Regards
 Guo Ren

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Guo Ren]

Hi Conor & Alexandre,

On Fri, Nov 29, 2024 at 10:58 AM Guo Ren <guoren@kernel.org> wrote:
>
> On Fri, Nov 29, 2024 at 8:55 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Fri, Nov 29, 2024 at 12:19 AM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > On Thu, Nov 28, 2024 at 03:50:09PM +0100, Alexandre Ghiti wrote:
> > > > On 28/11/2024 15:14, Conor Dooley wrote:
> > > > > On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> > > > > > On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > > > > > > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > > > > > > In order to produce a generic kernel, a user can select
> > > > > > > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > > > > > spinlock implementation if Zabha or Ziccrse are not present.
> > > > > > > >
> > > > > > > > Note that we can't use alternatives here because the discovery of
> > > > > > > > extensions is done too late and we need to start with the qspinlock
> > > > > > > > implementation because the ticket spinlock implementation would pollute
> > > > > > > > the spinlock value, so let's use static keys.
> > > > > > > >
> > > > > > > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > > > > > >
> > > > > > > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > > > > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > > > > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > > > > > > breaks boot on polarfire soc. It dies before outputting anything to the
> > > > > > > console. My .config has:
> > > > > > >
> > > > > > > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > > > > > > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > > > > > > CONFIG_RISCV_COMBO_SPINLOCKS=y
> > > > > > I pointed out some of the fragility during review:
> > > > > >
> > > > > > https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> > > > > >
> > > > > > so I'm kinda surprised it got merged tbh :/
> > > > > Maybe it could be reverted rather than having a broken boot with the
> > > > > default settings in -rc1.
> > > >
> > > >
> > > > No need to rush before we know what's happening,I guess you bisected to this
> > > > commit right?
> > >
> > > The symptom is a failure to boot, without any console output, of course
> > > I bisected it before blaming something specific. But I don't think it is
> > > "rushing" as having -rc1 broken with an option's default is a massive pain
> > > in the arse when it comes to testing.
> > >
> > > > I don't have this soc, so can you provide $stval/$sepc/$scause, a config, a
> > > > kernel, anything?
> > >
> > > I don't have the former cos it died immediately on boot. config is
> > > attached. It reproduces in QEMU so you don't need any hardware.
> > If QEMU could reproduce, could you provide a dmesg by the below method?
> >
> > Qemu cmd append: -s -S
> > ref: https://qemu-project.gitlab.io/qemu/system/gdb.html
> >
> > Connect gdb and in console:
> > 1. file vmlinux
> > 2. source ./Documentation/admin-guide/kdump/gdbmacros.txt
> > 3. dmesg
> >
> > Then, we could get the kernel's early boot logs from memory.
> I've reproduced it on qemu, thx for the config.
>
> Reading symbols from ../build-rv64lp64/vmlinux...
> (gdb) tar rem:1234
> Remote debugging using :1234
> ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> 49              atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
> (gdb) bt
> #0  ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> #1  arch_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> /home/guoren/source/kernel/linux/arch/riscv/include/asm/spinlock.h:28
> #2  do_raw_spin_lock (lock=lock@entry=0xffffffff81b9a5b8 <text_mutex>)
> at /home/guoren/source/kernel/linux/kernel/locking/spinlock_debug.c:116
> #3  0xffffffff80b2ea0e in __raw_spin_lock_irqsave
> (lock=0xffffffff81b9a5b8 <text_mutex>) at
> /home/guoren/source/kernel/linux/include/linux/spinlock_api_smp.h:111
> #4  _raw_spin_lock_irqsave (lock=lock@entry=0xffffffff81b9a5b8
> <text_mutex>) at
> /home/guoren/source/kernel/linux/kernel/locking/spinlock.c:162
> #5  0xffffffff80b27c54 in rt_mutex_slowtrylock
> (lock=0xffffffff81b9a5b8 <text_mutex>) at
> /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1393
> #6  0xffffffff80b295ea in rt_mutex_try_acquire
> (lock=0xffffffff81b9a5b8 <text_mutex>) at
> /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:319
> #7  __rt_mutex_lock (state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1805
> #8  __mutex_lock_common (ip=18446744071562135170, nest_lock=0x0,
> subclass=0, state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:518
> #9  mutex_lock_nested (lock=0xffffffff81b9a5b8 <text_mutex>,
> subclass=subclass@entry=0) at
> /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:529
> #10 0xffffffff80010682 in arch_jump_label_transform_queue
> (entry=entry@entry=0xffffffff8158da28, type=<optimized out>) at
> /home/guoren/source/kernel/linux/arch/riscv/kernel/jump_label.c:39
> #11 0xffffffff801d86b2 in __jump_label_update
> (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>,
> entry=0xffffffff8158da28, stop=stop@entry=0xffffffff815a5e68
> <__tracepoint_ptr_initcall_finish>, init=init@entry=true)
>     at /home/guoren/source/kernel/linux/kernel/jump_label.c:513
> #12 0xffffffff801d890c in jump_label_update
> (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> /home/guoren/source/kernel/linux/kernel/jump_label.c:920
> #13 0xffffffff801d8be8 in static_key_disable_cpuslocked
> (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> /home/guoren/source/kernel/linux/kernel/jump_label.c:240
> #14 0xffffffff801d8c04 in static_key_disable
> (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> /home/guoren/source/kernel/linux/kernel/jump_label.c:248
> #15 0xffffffff80c04a1a in riscv_spinlock_init () at
> /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:271
> #16 setup_arch (cmdline_p=cmdline_p@entry=0xffffffff81a03e88) at
> /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:336
> #17 0xffffffff80c007a2 in start_kernel () at
> /home/guoren/source/kernel/linux/init/main.c:922
> #18 0xffffffff80001164 in _start_kernel ()
> Backtrace stopped: frame did not save the PC
> (gdb) p /x lock
> $1 = 0xffffffff81b9a5b8
> (gdb) p /x *lock
> $2 = {{val = {counter = 0x20000}, {locked = 0x0, pending = 0x0},
> {locked_pending = 0x0, tail = 0x2}}}

I have for you here a fast fixup for reference. (PS: I'm digging into
the root cause mentioned by Will Deacon.)

diff --git a/arch/riscv/include/asm/text-patching.h
b/arch/riscv/include/asm/text-patching.h
index 7228e266b9a1..0439609f1cff 100644
--- a/arch/riscv/include/asm/text-patching.h
+++ b/arch/riscv/include/asm/text-patching.h
@@ -12,5 +12,6 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len);
 int patch_text(void *addr, u32 *insns, size_t len);

 extern int riscv_patch_in_stop_machine;
+extern int riscv_patch_in_spinlock_init;

 #endif /* _ASM_RISCV_PATCH_H */
diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
index 6eee6f736f68..d9a5a5c1933d 100644
--- a/arch/riscv/kernel/jump_label.c
+++ b/arch/riscv/kernel/jump_label.c
@@ -36,9 +36,11 @@ bool arch_jump_label_transform_queue(struct
jump_entry *entry,
                insn = RISCV_INSN_NOP;
        }

-       mutex_lock(&text_mutex);
+       if (!riscv_patch_in_spinlock_init)
+               mutex_lock(&text_mutex);
        patch_insn_write(addr, &insn, sizeof(insn));
-       mutex_unlock(&text_mutex);
+       if (!riscv_patch_in_spinlock_init)
+               mutex_unlock(&text_mutex);

        return true;
 }
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index db13c9ddf9e3..ab009cf855c2 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -24,6 +24,7 @@ struct patch_insn {
 };

 int riscv_patch_in_stop_machine = false;
+int riscv_patch_in_spinlock_init = false;

 #ifdef CONFIG_MMU

@@ -131,7 +132,7 @@ static int __patch_insn_write(void *addr, const
void *insn, size_t len)
         * safe but triggers a lockdep failure, so just elide it for that
         * specific case.
         */
-       if (!riscv_patch_in_stop_machine)
+       if (!riscv_patch_in_stop_machine && !riscv_patch_in_spinlock_init)
                lockdep_assert_held(&text_mutex);

        preempt_disable();
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 016b48fcd6f2..87ddf1702be4 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -268,7 +268,9 @@ static void __init riscv_spinlock_init(void)
        }
 #if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
        else {
+               riscv_patch_in_spinlock_init = 1;
                static_branch_disable(&qspinlock_key);
+               riscv_patch_in_spinlock_init = 0;
                pr_info("Ticket spinlock: enabled\n");
                return;
        }



-- 
Best Regards
 Guo Ren

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Alexandre Ghiti]

Hi everyone,

On Fri, Nov 29, 2024 at 7:28 AM Guo Ren <guoren@kernel.org> wrote:
>
> Hi Conor & Alexandre,
>
> On Fri, Nov 29, 2024 at 10:58 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > On Fri, Nov 29, 2024 at 8:55 AM Guo Ren <guoren@kernel.org> wrote:
> > >
> > > On Fri, Nov 29, 2024 at 12:19 AM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Thu, Nov 28, 2024 at 03:50:09PM +0100, Alexandre Ghiti wrote:
> > > > > On 28/11/2024 15:14, Conor Dooley wrote:
> > > > > > On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> > > > > > > On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > > > > > > > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > > > > > > > In order to produce a generic kernel, a user can select
> > > > > > > > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > > > > > > spinlock implementation if Zabha or Ziccrse are not present.
> > > > > > > > >
> > > > > > > > > Note that we can't use alternatives here because the discovery of
> > > > > > > > > extensions is done too late and we need to start with the qspinlock
> > > > > > > > > implementation because the ticket spinlock implementation would pollute
> > > > > > > > > the spinlock value, so let's use static keys.
> > > > > > > > >
> > > > > > > > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > > > > > > >
> > > > > > > > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > > > > > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > > > > > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > > > > > > > breaks boot on polarfire soc. It dies before outputting anything to the
> > > > > > > > console. My .config has:
> > > > > > > >
> > > > > > > > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > > > > > > > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > > > > > > > CONFIG_RISCV_COMBO_SPINLOCKS=y
> > > > > > > I pointed out some of the fragility during review:
> > > > > > >
> > > > > > > https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> > > > > > >
> > > > > > > so I'm kinda surprised it got merged tbh :/
> > > > > > Maybe it could be reverted rather than having a broken boot with the
> > > > > > default settings in -rc1.
> > > > >
> > > > >
> > > > > No need to rush before we know what's happening,I guess you bisected to this
> > > > > commit right?
> > > >
> > > > The symptom is a failure to boot, without any console output, of course
> > > > I bisected it before blaming something specific. But I don't think it is
> > > > "rushing" as having -rc1 broken with an option's default is a massive pain
> > > > in the arse when it comes to testing.
> > > >
> > > > > I don't have this soc, so can you provide $stval/$sepc/$scause, a config, a
> > > > > kernel, anything?
> > > >
> > > > I don't have the former cos it died immediately on boot. config is
> > > > attached. It reproduces in QEMU so you don't need any hardware.
> > > If QEMU could reproduce, could you provide a dmesg by the below method?
> > >
> > > Qemu cmd append: -s -S
> > > ref: https://qemu-project.gitlab.io/qemu/system/gdb.html
> > >
> > > Connect gdb and in console:
> > > 1. file vmlinux
> > > 2. source ./Documentation/admin-guide/kdump/gdbmacros.txt
> > > 3. dmesg
> > >
> > > Then, we could get the kernel's early boot logs from memory.
> > I've reproduced it on qemu, thx for the config.
> >
> > Reading symbols from ../build-rv64lp64/vmlinux...
> > (gdb) tar rem:1234
> > Remote debugging using :1234
> > ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > 49              atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
> > (gdb) bt
> > #0  ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > #1  arch_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > /home/guoren/source/kernel/linux/arch/riscv/include/asm/spinlock.h:28
> > #2  do_raw_spin_lock (lock=lock@entry=0xffffffff81b9a5b8 <text_mutex>)
> > at /home/guoren/source/kernel/linux/kernel/locking/spinlock_debug.c:116
> > #3  0xffffffff80b2ea0e in __raw_spin_lock_irqsave
> > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > /home/guoren/source/kernel/linux/include/linux/spinlock_api_smp.h:111
> > #4  _raw_spin_lock_irqsave (lock=lock@entry=0xffffffff81b9a5b8
> > <text_mutex>) at
> > /home/guoren/source/kernel/linux/kernel/locking/spinlock.c:162
> > #5  0xffffffff80b27c54 in rt_mutex_slowtrylock
> > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1393
> > #6  0xffffffff80b295ea in rt_mutex_try_acquire
> > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:319
> > #7  __rt_mutex_lock (state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1805
> > #8  __mutex_lock_common (ip=18446744071562135170, nest_lock=0x0,
> > subclass=0, state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:518
> > #9  mutex_lock_nested (lock=0xffffffff81b9a5b8 <text_mutex>,
> > subclass=subclass@entry=0) at
> > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:529
> > #10 0xffffffff80010682 in arch_jump_label_transform_queue
> > (entry=entry@entry=0xffffffff8158da28, type=<optimized out>) at
> > /home/guoren/source/kernel/linux/arch/riscv/kernel/jump_label.c:39
> > #11 0xffffffff801d86b2 in __jump_label_update
> > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>,
> > entry=0xffffffff8158da28, stop=stop@entry=0xffffffff815a5e68
> > <__tracepoint_ptr_initcall_finish>, init=init@entry=true)
> >     at /home/guoren/source/kernel/linux/kernel/jump_label.c:513
> > #12 0xffffffff801d890c in jump_label_update
> > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > /home/guoren/source/kernel/linux/kernel/jump_label.c:920
> > #13 0xffffffff801d8be8 in static_key_disable_cpuslocked
> > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > /home/guoren/source/kernel/linux/kernel/jump_label.c:240
> > #14 0xffffffff801d8c04 in static_key_disable
> > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > /home/guoren/source/kernel/linux/kernel/jump_label.c:248
> > #15 0xffffffff80c04a1a in riscv_spinlock_init () at
> > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:271
> > #16 setup_arch (cmdline_p=cmdline_p@entry=0xffffffff81a03e88) at
> > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:336
> > #17 0xffffffff80c007a2 in start_kernel () at
> > /home/guoren/source/kernel/linux/init/main.c:922
> > #18 0xffffffff80001164 in _start_kernel ()
> > Backtrace stopped: frame did not save the PC
> > (gdb) p /x lock
> > $1 = 0xffffffff81b9a5b8
> > (gdb) p /x *lock
> > $2 = {{val = {counter = 0x20000}, {locked = 0x0, pending = 0x0},
> > {locked_pending = 0x0, tail = 0x2}}}
>
> I have for you here a fast fixup for reference. (PS: I'm digging into
> the root cause mentioned by Will Deacon.)
>
> diff --git a/arch/riscv/include/asm/text-patching.h
> b/arch/riscv/include/asm/text-patching.h
> index 7228e266b9a1..0439609f1cff 100644
> --- a/arch/riscv/include/asm/text-patching.h
> +++ b/arch/riscv/include/asm/text-patching.h
> @@ -12,5 +12,6 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len);
>  int patch_text(void *addr, u32 *insns, size_t len);
>
>  extern int riscv_patch_in_stop_machine;
> +extern int riscv_patch_in_spinlock_init;
>
>  #endif /* _ASM_RISCV_PATCH_H */
> diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
> index 6eee6f736f68..d9a5a5c1933d 100644
> --- a/arch/riscv/kernel/jump_label.c
> +++ b/arch/riscv/kernel/jump_label.c
> @@ -36,9 +36,11 @@ bool arch_jump_label_transform_queue(struct
> jump_entry *entry,
>                 insn = RISCV_INSN_NOP;
>         }
>
> -       mutex_lock(&text_mutex);
> +       if (!riscv_patch_in_spinlock_init)
> +               mutex_lock(&text_mutex);
>         patch_insn_write(addr, &insn, sizeof(insn));
> -       mutex_unlock(&text_mutex);
> +       if (!riscv_patch_in_spinlock_init)
> +               mutex_unlock(&text_mutex);
>
>         return true;
>  }
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> index db13c9ddf9e3..ab009cf855c2 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -24,6 +24,7 @@ struct patch_insn {
>  };
>
>  int riscv_patch_in_stop_machine = false;
> +int riscv_patch_in_spinlock_init = false;
>
>  #ifdef CONFIG_MMU
>
> @@ -131,7 +132,7 @@ static int __patch_insn_write(void *addr, const
> void *insn, size_t len)
>          * safe but triggers a lockdep failure, so just elide it for that
>          * specific case.
>          */
> -       if (!riscv_patch_in_stop_machine)
> +       if (!riscv_patch_in_stop_machine && !riscv_patch_in_spinlock_init)
>                 lockdep_assert_held(&text_mutex);
>
>         preempt_disable();
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 016b48fcd6f2..87ddf1702be4 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -268,7 +268,9 @@ static void __init riscv_spinlock_init(void)
>         }
>  #if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
>         else {
> +               riscv_patch_in_spinlock_init = 1;
>                 static_branch_disable(&qspinlock_key);
> +               riscv_patch_in_spinlock_init = 0;
>                 pr_info("Ticket spinlock: enabled\n");
>                 return;
>         }
>
>
>
> --
> Best Regards
>  Guo Ren

Thanks Guo for looking into this.

Your solution is not very pretty but I don't have anything better :/
Unless introducing a static_branch_XXX_nolock() API? I gave it a try
and it fixes the issue, but not sure this will be accepted.

The thing is the usage of static branches is temporary, we'll use
alternatives when I finish working on getting the extensions very
early from the ACPI tables (I have a poc that works, just needs some
cleaning).

So let's say that I make this early extension parsing my priority for
6.14, can we live with Guo's hack in this release? Or should we revert
this commit?

Thanks,

Alex

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Conor Dooley]

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

On Fri, Nov 29, 2024 at 11:31:44AM +0100, Alexandre Ghiti wrote:
> Hi everyone,
> 
> On Fri, Nov 29, 2024 at 7:28 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > Hi Conor & Alexandre,
> >
> > On Fri, Nov 29, 2024 at 10:58 AM Guo Ren <guoren@kernel.org> wrote:
> > >
> > > On Fri, Nov 29, 2024 at 8:55 AM Guo Ren <guoren@kernel.org> wrote:
> > > >
> > > > On Fri, Nov 29, 2024 at 12:19 AM Conor Dooley <conor@kernel.org> wrote:
> > > > >
> > > > > On Thu, Nov 28, 2024 at 03:50:09PM +0100, Alexandre Ghiti wrote:
> > > > > > On 28/11/2024 15:14, Conor Dooley wrote:
> > > > > > > On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> > > > > > > > On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > > > > > > > > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > > > > > > > > In order to produce a generic kernel, a user can select
> > > > > > > > > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > > > > > > > spinlock implementation if Zabha or Ziccrse are not present.
> > > > > > > > > >
> > > > > > > > > > Note that we can't use alternatives here because the discovery of
> > > > > > > > > > extensions is done too late and we need to start with the qspinlock
> > > > > > > > > > implementation because the ticket spinlock implementation would pollute
> > > > > > > > > > the spinlock value, so let's use static keys.
> > > > > > > > > >
> > > > > > > > > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > > > > > > > >
> > > > > > > > > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > > > > > > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > > > > > > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > > > > > > > > breaks boot on polarfire soc. It dies before outputting anything to the
> > > > > > > > > console. My .config has:
> > > > > > > > >
> > > > > > > > > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > > > > > > > > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > > > > > > > > CONFIG_RISCV_COMBO_SPINLOCKS=y
> > > > > > > > I pointed out some of the fragility during review:
> > > > > > > >
> > > > > > > > https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> > > > > > > >
> > > > > > > > so I'm kinda surprised it got merged tbh :/
> > > > > > > Maybe it could be reverted rather than having a broken boot with the
> > > > > > > default settings in -rc1.
> > > > > >
> > > > > >
> > > > > > No need to rush before we know what's happening,I guess you bisected to this
> > > > > > commit right?
> > > > >
> > > > > The symptom is a failure to boot, without any console output, of course
> > > > > I bisected it before blaming something specific. But I don't think it is
> > > > > "rushing" as having -rc1 broken with an option's default is a massive pain
> > > > > in the arse when it comes to testing.
> > > > >
> > > > > > I don't have this soc, so can you provide $stval/$sepc/$scause, a config, a
> > > > > > kernel, anything?
> > > > >
> > > > > I don't have the former cos it died immediately on boot. config is
> > > > > attached. It reproduces in QEMU so you don't need any hardware.
> > > > If QEMU could reproduce, could you provide a dmesg by the below method?
> > > >
> > > > Qemu cmd append: -s -S
> > > > ref: https://qemu-project.gitlab.io/qemu/system/gdb.html
> > > >
> > > > Connect gdb and in console:
> > > > 1. file vmlinux
> > > > 2. source ./Documentation/admin-guide/kdump/gdbmacros.txt
> > > > 3. dmesg
> > > >
> > > > Then, we could get the kernel's early boot logs from memory.
> > > I've reproduced it on qemu, thx for the config.
> > >
> > > Reading symbols from ../build-rv64lp64/vmlinux...
> > > (gdb) tar rem:1234
> > > Remote debugging using :1234
> > > ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > > 49              atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
> > > (gdb) bt
> > > #0  ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > > #1  arch_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/arch/riscv/include/asm/spinlock.h:28
> > > #2  do_raw_spin_lock (lock=lock@entry=0xffffffff81b9a5b8 <text_mutex>)
> > > at /home/guoren/source/kernel/linux/kernel/locking/spinlock_debug.c:116
> > > #3  0xffffffff80b2ea0e in __raw_spin_lock_irqsave
> > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/include/linux/spinlock_api_smp.h:111
> > > #4  _raw_spin_lock_irqsave (lock=lock@entry=0xffffffff81b9a5b8
> > > <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/spinlock.c:162
> > > #5  0xffffffff80b27c54 in rt_mutex_slowtrylock
> > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1393
> > > #6  0xffffffff80b295ea in rt_mutex_try_acquire
> > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:319
> > > #7  __rt_mutex_lock (state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1805
> > > #8  __mutex_lock_common (ip=18446744071562135170, nest_lock=0x0,
> > > subclass=0, state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:518
> > > #9  mutex_lock_nested (lock=0xffffffff81b9a5b8 <text_mutex>,
> > > subclass=subclass@entry=0) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:529
> > > #10 0xffffffff80010682 in arch_jump_label_transform_queue
> > > (entry=entry@entry=0xffffffff8158da28, type=<optimized out>) at
> > > /home/guoren/source/kernel/linux/arch/riscv/kernel/jump_label.c:39
> > > #11 0xffffffff801d86b2 in __jump_label_update
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>,
> > > entry=0xffffffff8158da28, stop=stop@entry=0xffffffff815a5e68
> > > <__tracepoint_ptr_initcall_finish>, init=init@entry=true)
> > >     at /home/guoren/source/kernel/linux/kernel/jump_label.c:513
> > > #12 0xffffffff801d890c in jump_label_update
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > /home/guoren/source/kernel/linux/kernel/jump_label.c:920
> > > #13 0xffffffff801d8be8 in static_key_disable_cpuslocked
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > /home/guoren/source/kernel/linux/kernel/jump_label.c:240
> > > #14 0xffffffff801d8c04 in static_key_disable
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > /home/guoren/source/kernel/linux/kernel/jump_label.c:248
> > > #15 0xffffffff80c04a1a in riscv_spinlock_init () at
> > > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:271
> > > #16 setup_arch (cmdline_p=cmdline_p@entry=0xffffffff81a03e88) at
> > > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:336
> > > #17 0xffffffff80c007a2 in start_kernel () at
> > > /home/guoren/source/kernel/linux/init/main.c:922
> > > #18 0xffffffff80001164 in _start_kernel ()
> > > Backtrace stopped: frame did not save the PC
> > > (gdb) p /x lock
> > > $1 = 0xffffffff81b9a5b8
> > > (gdb) p /x *lock
> > > $2 = {{val = {counter = 0x20000}, {locked = 0x0, pending = 0x0},
> > > {locked_pending = 0x0, tail = 0x2}}}
> >
> > I have for you here a fast fixup for reference. (PS: I'm digging into
> > the root cause mentioned by Will Deacon.)
> >
> > diff --git a/arch/riscv/include/asm/text-patching.h
> > b/arch/riscv/include/asm/text-patching.h
> > index 7228e266b9a1..0439609f1cff 100644
> > --- a/arch/riscv/include/asm/text-patching.h
> > +++ b/arch/riscv/include/asm/text-patching.h
> > @@ -12,5 +12,6 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len);
> >  int patch_text(void *addr, u32 *insns, size_t len);
> >
> >  extern int riscv_patch_in_stop_machine;
> > +extern int riscv_patch_in_spinlock_init;
> >
> >  #endif /* _ASM_RISCV_PATCH_H */
> > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
> > index 6eee6f736f68..d9a5a5c1933d 100644
> > --- a/arch/riscv/kernel/jump_label.c
> > +++ b/arch/riscv/kernel/jump_label.c
> > @@ -36,9 +36,11 @@ bool arch_jump_label_transform_queue(struct
> > jump_entry *entry,
> >                 insn = RISCV_INSN_NOP;
> >         }
> >
> > -       mutex_lock(&text_mutex);
> > +       if (!riscv_patch_in_spinlock_init)
> > +               mutex_lock(&text_mutex);
> >         patch_insn_write(addr, &insn, sizeof(insn));
> > -       mutex_unlock(&text_mutex);
> > +       if (!riscv_patch_in_spinlock_init)
> > +               mutex_unlock(&text_mutex);
> >
> >         return true;
> >  }
> > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> > index db13c9ddf9e3..ab009cf855c2 100644
> > --- a/arch/riscv/kernel/patch.c
> > +++ b/arch/riscv/kernel/patch.c
> > @@ -24,6 +24,7 @@ struct patch_insn {
> >  };
> >
> >  int riscv_patch_in_stop_machine = false;
> > +int riscv_patch_in_spinlock_init = false;
> >
> >  #ifdef CONFIG_MMU
> >
> > @@ -131,7 +132,7 @@ static int __patch_insn_write(void *addr, const
> > void *insn, size_t len)
> >          * safe but triggers a lockdep failure, so just elide it for that
> >          * specific case.
> >          */
> > -       if (!riscv_patch_in_stop_machine)
> > +       if (!riscv_patch_in_stop_machine && !riscv_patch_in_spinlock_init)
> >                 lockdep_assert_held(&text_mutex);
> >
> >         preempt_disable();
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 016b48fcd6f2..87ddf1702be4 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -268,7 +268,9 @@ static void __init riscv_spinlock_init(void)
> >         }
> >  #if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
> >         else {
> > +               riscv_patch_in_spinlock_init = 1;
> >                 static_branch_disable(&qspinlock_key);
> > +               riscv_patch_in_spinlock_init = 0;
> >                 pr_info("Ticket spinlock: enabled\n");
> >                 return;
> >         }
> >
> >
> >
> > --
> > Best Regards
> >  Guo Ren
> 
> Thanks Guo for looking into this.
> 
> Your solution is not very pretty but I don't have anything better :/
> Unless introducing a static_branch_XXX_nolock() API? I gave it a try
> and it fixes the issue, but not sure this will be accepted.
> 
> The thing is the usage of static branches is temporary, we'll use
> alternatives when I finish working on getting the extensions very
> early from the ACPI tables (I have a poc that works, just needs some
> cleaning).
> 
> So let's say that I make this early extension parsing my priority for
> 6.14, can we live with Guo's hack in this release? Or should we revert
> this commit?

I tried this diff, and it doesn't actually fix the problem - either in
QEMU or in hardware. I'll do some more poking.

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

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Conor Dooley]

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

On Fri, Nov 29, 2024 at 11:18:24AM +0000, Conor Dooley wrote:
> On Fri, Nov 29, 2024 at 11:31:44AM +0100, Alexandre Ghiti wrote:
> > Hi everyone,
> > 
> > On Fri, Nov 29, 2024 at 7:28 AM Guo Ren <guoren@kernel.org> wrote:
> > >
> > > Hi Conor & Alexandre,
> > >
> > > On Fri, Nov 29, 2024 at 10:58 AM Guo Ren <guoren@kernel.org> wrote:
> > > >
> > > > On Fri, Nov 29, 2024 at 8:55 AM Guo Ren <guoren@kernel.org> wrote:
> > > > >
> > > > > On Fri, Nov 29, 2024 at 12:19 AM Conor Dooley <conor@kernel.org> wrote:
> > > > > >
> > > > > > On Thu, Nov 28, 2024 at 03:50:09PM +0100, Alexandre Ghiti wrote:
> > > > > > > On 28/11/2024 15:14, Conor Dooley wrote:
> > > > > > > > On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> > > > > > > > > On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > > > > > > > > > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > > > > > > > > > In order to produce a generic kernel, a user can select
> > > > > > > > > > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > > > > > > > > spinlock implementation if Zabha or Ziccrse are not present.
> > > > > > > > > > >
> > > > > > > > > > > Note that we can't use alternatives here because the discovery of
> > > > > > > > > > > extensions is done too late and we need to start with the qspinlock
> > > > > > > > > > > implementation because the ticket spinlock implementation would pollute
> > > > > > > > > > > the spinlock value, so let's use static keys.
> > > > > > > > > > >
> > > > > > > > > > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > > > > > > > > >
> > > > > > > > > > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > > > > > > > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > > > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > > > > > > > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > > > > > > > > > breaks boot on polarfire soc. It dies before outputting anything to the
> > > > > > > > > > console. My .config has:
> > > > > > > > > >
> > > > > > > > > > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > > > > > > > > > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > > > > > > > > > CONFIG_RISCV_COMBO_SPINLOCKS=y
> > > > > > > > > I pointed out some of the fragility during review:
> > > > > > > > >
> > > > > > > > > https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> > > > > > > > >
> > > > > > > > > so I'm kinda surprised it got merged tbh :/
> > > > > > > > Maybe it could be reverted rather than having a broken boot with the
> > > > > > > > default settings in -rc1.
> > > > > > >
> > > > > > >
> > > > > > > No need to rush before we know what's happening,I guess you bisected to this
> > > > > > > commit right?
> > > > > >
> > > > > > The symptom is a failure to boot, without any console output, of course
> > > > > > I bisected it before blaming something specific. But I don't think it is
> > > > > > "rushing" as having -rc1 broken with an option's default is a massive pain
> > > > > > in the arse when it comes to testing.
> > > > > >
> > > > > > > I don't have this soc, so can you provide $stval/$sepc/$scause, a config, a
> > > > > > > kernel, anything?
> > > > > >
> > > > > > I don't have the former cos it died immediately on boot. config is
> > > > > > attached. It reproduces in QEMU so you don't need any hardware.
> > > > > If QEMU could reproduce, could you provide a dmesg by the below method?
> > > > >
> > > > > Qemu cmd append: -s -S
> > > > > ref: https://qemu-project.gitlab.io/qemu/system/gdb.html
> > > > >
> > > > > Connect gdb and in console:
> > > > > 1. file vmlinux
> > > > > 2. source ./Documentation/admin-guide/kdump/gdbmacros.txt
> > > > > 3. dmesg
> > > > >
> > > > > Then, we could get the kernel's early boot logs from memory.
> > > > I've reproduced it on qemu, thx for the config.
> > > >
> > > > Reading symbols from ../build-rv64lp64/vmlinux...
> > > > (gdb) tar rem:1234
> > > > Remote debugging using :1234
> > > > ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > > > 49              atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
> > > > (gdb) bt
> > > > #0  ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > > > #1  arch_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/arch/riscv/include/asm/spinlock.h:28
> > > > #2  do_raw_spin_lock (lock=lock@entry=0xffffffff81b9a5b8 <text_mutex>)
> > > > at /home/guoren/source/kernel/linux/kernel/locking/spinlock_debug.c:116
> > > > #3  0xffffffff80b2ea0e in __raw_spin_lock_irqsave
> > > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/include/linux/spinlock_api_smp.h:111
> > > > #4  _raw_spin_lock_irqsave (lock=lock@entry=0xffffffff81b9a5b8
> > > > <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/spinlock.c:162
> > > > #5  0xffffffff80b27c54 in rt_mutex_slowtrylock
> > > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1393
> > > > #6  0xffffffff80b295ea in rt_mutex_try_acquire
> > > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:319
> > > > #7  __rt_mutex_lock (state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1805
> > > > #8  __mutex_lock_common (ip=18446744071562135170, nest_lock=0x0,
> > > > subclass=0, state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:518
> > > > #9  mutex_lock_nested (lock=0xffffffff81b9a5b8 <text_mutex>,
> > > > subclass=subclass@entry=0) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:529
> > > > #10 0xffffffff80010682 in arch_jump_label_transform_queue
> > > > (entry=entry@entry=0xffffffff8158da28, type=<optimized out>) at
> > > > /home/guoren/source/kernel/linux/arch/riscv/kernel/jump_label.c:39
> > > > #11 0xffffffff801d86b2 in __jump_label_update
> > > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>,
> > > > entry=0xffffffff8158da28, stop=stop@entry=0xffffffff815a5e68
> > > > <__tracepoint_ptr_initcall_finish>, init=init@entry=true)
> > > >     at /home/guoren/source/kernel/linux/kernel/jump_label.c:513
> > > > #12 0xffffffff801d890c in jump_label_update
> > > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > > /home/guoren/source/kernel/linux/kernel/jump_label.c:920
> > > > #13 0xffffffff801d8be8 in static_key_disable_cpuslocked
> > > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > > /home/guoren/source/kernel/linux/kernel/jump_label.c:240
> > > > #14 0xffffffff801d8c04 in static_key_disable
> > > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > > /home/guoren/source/kernel/linux/kernel/jump_label.c:248
> > > > #15 0xffffffff80c04a1a in riscv_spinlock_init () at
> > > > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:271
> > > > #16 setup_arch (cmdline_p=cmdline_p@entry=0xffffffff81a03e88) at
> > > > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:336
> > > > #17 0xffffffff80c007a2 in start_kernel () at
> > > > /home/guoren/source/kernel/linux/init/main.c:922
> > > > #18 0xffffffff80001164 in _start_kernel ()
> > > > Backtrace stopped: frame did not save the PC
> > > > (gdb) p /x lock
> > > > $1 = 0xffffffff81b9a5b8
> > > > (gdb) p /x *lock
> > > > $2 = {{val = {counter = 0x20000}, {locked = 0x0, pending = 0x0},
> > > > {locked_pending = 0x0, tail = 0x2}}}
> > >
> > > I have for you here a fast fixup for reference. (PS: I'm digging into
> > > the root cause mentioned by Will Deacon.)
> > >
> > > diff --git a/arch/riscv/include/asm/text-patching.h
> > > b/arch/riscv/include/asm/text-patching.h
> > > index 7228e266b9a1..0439609f1cff 100644
> > > --- a/arch/riscv/include/asm/text-patching.h
> > > +++ b/arch/riscv/include/asm/text-patching.h
> > > @@ -12,5 +12,6 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len);
> > >  int patch_text(void *addr, u32 *insns, size_t len);
> > >
> > >  extern int riscv_patch_in_stop_machine;
> > > +extern int riscv_patch_in_spinlock_init;
> > >
> > >  #endif /* _ASM_RISCV_PATCH_H */
> > > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
> > > index 6eee6f736f68..d9a5a5c1933d 100644
> > > --- a/arch/riscv/kernel/jump_label.c
> > > +++ b/arch/riscv/kernel/jump_label.c
> > > @@ -36,9 +36,11 @@ bool arch_jump_label_transform_queue(struct
> > > jump_entry *entry,
> > >                 insn = RISCV_INSN_NOP;
> > >         }
> > >
> > > -       mutex_lock(&text_mutex);
> > > +       if (!riscv_patch_in_spinlock_init)
> > > +               mutex_lock(&text_mutex);
> > >         patch_insn_write(addr, &insn, sizeof(insn));
> > > -       mutex_unlock(&text_mutex);
> > > +       if (!riscv_patch_in_spinlock_init)
> > > +               mutex_unlock(&text_mutex);
> > >
> > >         return true;
> > >  }
> > > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> > > index db13c9ddf9e3..ab009cf855c2 100644
> > > --- a/arch/riscv/kernel/patch.c
> > > +++ b/arch/riscv/kernel/patch.c
> > > @@ -24,6 +24,7 @@ struct patch_insn {
> > >  };
> > >
> > >  int riscv_patch_in_stop_machine = false;
> > > +int riscv_patch_in_spinlock_init = false;
> > >
> > >  #ifdef CONFIG_MMU
> > >
> > > @@ -131,7 +132,7 @@ static int __patch_insn_write(void *addr, const
> > > void *insn, size_t len)
> > >          * safe but triggers a lockdep failure, so just elide it for that
> > >          * specific case.
> > >          */
> > > -       if (!riscv_patch_in_stop_machine)
> > > +       if (!riscv_patch_in_stop_machine && !riscv_patch_in_spinlock_init)
> > >                 lockdep_assert_held(&text_mutex);
> > >
> > >         preempt_disable();
> > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > index 016b48fcd6f2..87ddf1702be4 100644
> > > --- a/arch/riscv/kernel/setup.c
> > > +++ b/arch/riscv/kernel/setup.c
> > > @@ -268,7 +268,9 @@ static void __init riscv_spinlock_init(void)
> > >         }
> > >  #if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
> > >         else {
> > > +               riscv_patch_in_spinlock_init = 1;
> > >                 static_branch_disable(&qspinlock_key);
> > > +               riscv_patch_in_spinlock_init = 0;
> > >                 pr_info("Ticket spinlock: enabled\n");
> > >                 return;
> > >         }
> > >
> > >
> > >
> > > --
> > > Best Regards
> > >  Guo Ren
> > 
> > Thanks Guo for looking into this.
> > 
> > Your solution is not very pretty but I don't have anything better :/
> > Unless introducing a static_branch_XXX_nolock() API? I gave it a try
> > and it fixes the issue, but not sure this will be accepted.
> > 
> > The thing is the usage of static branches is temporary, we'll use
> > alternatives when I finish working on getting the extensions very
> > early from the ACPI tables (I have a poc that works, just needs some
> > cleaning).
> > 
> > So let's say that I make this early extension parsing my priority for
> > 6.14, can we live with Guo's hack in this release? Or should we revert
> > this commit?
> 
> I tried this diff, and it doesn't actually fix the problem - either in
> QEMU or in hardware. I'll do some more poking.

Looks like it might have been my fault, typo while hand-applying the
diff pasted above cos it was corrupted :/ Rebuilding..


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

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Conor Dooley]

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

On Fri, Nov 29, 2024 at 11:43:44AM +0000, Conor Dooley wrote:
> On Fri, Nov 29, 2024 at 11:18:24AM +0000, Conor Dooley wrote:
> > 
> > I tried this diff, and it doesn't actually fix the problem - either in
> > QEMU or in hardware. I'll do some more poking.
> 
> Looks like it might have been my fault, typo while hand-applying the
> diff pasted above cos it was corrupted :/ Rebuilding..


sans-typo, the diff does make it boot, my bad.


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

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Guo Ren]

On Fri, Nov 29, 2024 at 6:32 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> Hi everyone,
>
> On Fri, Nov 29, 2024 at 7:28 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > Hi Conor & Alexandre,
> >
> > On Fri, Nov 29, 2024 at 10:58 AM Guo Ren <guoren@kernel.org> wrote:
> > >
> > > On Fri, Nov 29, 2024 at 8:55 AM Guo Ren <guoren@kernel.org> wrote:
> > > >
> > > > On Fri, Nov 29, 2024 at 12:19 AM Conor Dooley <conor@kernel.org> wrote:
> > > > >
> > > > > On Thu, Nov 28, 2024 at 03:50:09PM +0100, Alexandre Ghiti wrote:
> > > > > > On 28/11/2024 15:14, Conor Dooley wrote:
> > > > > > > On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> > > > > > > > On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > > > > > > > > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > > > > > > > > In order to produce a generic kernel, a user can select
> > > > > > > > > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > > > > > > > spinlock implementation if Zabha or Ziccrse are not present.
> > > > > > > > > >
> > > > > > > > > > Note that we can't use alternatives here because the discovery of
> > > > > > > > > > extensions is done too late and we need to start with the qspinlock
> > > > > > > > > > implementation because the ticket spinlock implementation would pollute
> > > > > > > > > > the spinlock value, so let's use static keys.
> > > > > > > > > >
> > > > > > > > > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > > > > > > > >
> > > > > > > > > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > > > > > > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > > > > > > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > > > > > > > > breaks boot on polarfire soc. It dies before outputting anything to the
> > > > > > > > > console. My .config has:
> > > > > > > > >
> > > > > > > > > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > > > > > > > > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > > > > > > > > CONFIG_RISCV_COMBO_SPINLOCKS=y
> > > > > > > > I pointed out some of the fragility during review:
> > > > > > > >
> > > > > > > > https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> > > > > > > >
> > > > > > > > so I'm kinda surprised it got merged tbh :/
> > > > > > > Maybe it could be reverted rather than having a broken boot with the
> > > > > > > default settings in -rc1.
> > > > > >
> > > > > >
> > > > > > No need to rush before we know what's happening,I guess you bisected to this
> > > > > > commit right?
> > > > >
> > > > > The symptom is a failure to boot, without any console output, of course
> > > > > I bisected it before blaming something specific. But I don't think it is
> > > > > "rushing" as having -rc1 broken with an option's default is a massive pain
> > > > > in the arse when it comes to testing.
> > > > >
> > > > > > I don't have this soc, so can you provide $stval/$sepc/$scause, a config, a
> > > > > > kernel, anything?
> > > > >
> > > > > I don't have the former cos it died immediately on boot. config is
> > > > > attached. It reproduces in QEMU so you don't need any hardware.
> > > > If QEMU could reproduce, could you provide a dmesg by the below method?
> > > >
> > > > Qemu cmd append: -s -S
> > > > ref: https://qemu-project.gitlab.io/qemu/system/gdb.html
> > > >
> > > > Connect gdb and in console:
> > > > 1. file vmlinux
> > > > 2. source ./Documentation/admin-guide/kdump/gdbmacros.txt
> > > > 3. dmesg
> > > >
> > > > Then, we could get the kernel's early boot logs from memory.
> > > I've reproduced it on qemu, thx for the config.
> > >
> > > Reading symbols from ../build-rv64lp64/vmlinux...
> > > (gdb) tar rem:1234
> > > Remote debugging using :1234
> > > ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > > 49              atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
> > > (gdb) bt
> > > #0  ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > > #1  arch_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/arch/riscv/include/asm/spinlock.h:28
> > > #2  do_raw_spin_lock (lock=lock@entry=0xffffffff81b9a5b8 <text_mutex>)
> > > at /home/guoren/source/kernel/linux/kernel/locking/spinlock_debug.c:116
> > > #3  0xffffffff80b2ea0e in __raw_spin_lock_irqsave
> > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/include/linux/spinlock_api_smp.h:111
> > > #4  _raw_spin_lock_irqsave (lock=lock@entry=0xffffffff81b9a5b8
> > > <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/spinlock.c:162
> > > #5  0xffffffff80b27c54 in rt_mutex_slowtrylock
> > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1393
> > > #6  0xffffffff80b295ea in rt_mutex_try_acquire
> > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:319
> > > #7  __rt_mutex_lock (state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1805
> > > #8  __mutex_lock_common (ip=18446744071562135170, nest_lock=0x0,
> > > subclass=0, state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:518
> > > #9  mutex_lock_nested (lock=0xffffffff81b9a5b8 <text_mutex>,
> > > subclass=subclass@entry=0) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:529
> > > #10 0xffffffff80010682 in arch_jump_label_transform_queue
> > > (entry=entry@entry=0xffffffff8158da28, type=<optimized out>) at
> > > /home/guoren/source/kernel/linux/arch/riscv/kernel/jump_label.c:39
> > > #11 0xffffffff801d86b2 in __jump_label_update
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>,
> > > entry=0xffffffff8158da28, stop=stop@entry=0xffffffff815a5e68
> > > <__tracepoint_ptr_initcall_finish>, init=init@entry=true)
> > >     at /home/guoren/source/kernel/linux/kernel/jump_label.c:513
> > > #12 0xffffffff801d890c in jump_label_update
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > /home/guoren/source/kernel/linux/kernel/jump_label.c:920
> > > #13 0xffffffff801d8be8 in static_key_disable_cpuslocked
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > /home/guoren/source/kernel/linux/kernel/jump_label.c:240
> > > #14 0xffffffff801d8c04 in static_key_disable
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > /home/guoren/source/kernel/linux/kernel/jump_label.c:248
> > > #15 0xffffffff80c04a1a in riscv_spinlock_init () at
> > > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:271
> > > #16 setup_arch (cmdline_p=cmdline_p@entry=0xffffffff81a03e88) at
> > > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:336
> > > #17 0xffffffff80c007a2 in start_kernel () at
> > > /home/guoren/source/kernel/linux/init/main.c:922
> > > #18 0xffffffff80001164 in _start_kernel ()
> > > Backtrace stopped: frame did not save the PC
> > > (gdb) p /x lock
> > > $1 = 0xffffffff81b9a5b8
> > > (gdb) p /x *lock
> > > $2 = {{val = {counter = 0x20000}, {locked = 0x0, pending = 0x0},
> > > {locked_pending = 0x0, tail = 0x2}}}
> >
> > I have for you here a fast fixup for reference. (PS: I'm digging into
> > the root cause mentioned by Will Deacon.)
> >
> > diff --git a/arch/riscv/include/asm/text-patching.h
> > b/arch/riscv/include/asm/text-patching.h
> > index 7228e266b9a1..0439609f1cff 100644
> > --- a/arch/riscv/include/asm/text-patching.h
> > +++ b/arch/riscv/include/asm/text-patching.h
> > @@ -12,5 +12,6 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len);
> >  int patch_text(void *addr, u32 *insns, size_t len);
> >
> >  extern int riscv_patch_in_stop_machine;
> > +extern int riscv_patch_in_spinlock_init;
> >
> >  #endif /* _ASM_RISCV_PATCH_H */
> > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
> > index 6eee6f736f68..d9a5a5c1933d 100644
> > --- a/arch/riscv/kernel/jump_label.c
> > +++ b/arch/riscv/kernel/jump_label.c
> > @@ -36,9 +36,11 @@ bool arch_jump_label_transform_queue(struct
> > jump_entry *entry,
> >                 insn = RISCV_INSN_NOP;
> >         }
> >
> > -       mutex_lock(&text_mutex);
> > +       if (!riscv_patch_in_spinlock_init)
> > +               mutex_lock(&text_mutex);
> >         patch_insn_write(addr, &insn, sizeof(insn));
> > -       mutex_unlock(&text_mutex);
> > +       if (!riscv_patch_in_spinlock_init)
> > +               mutex_unlock(&text_mutex);
> >
> >         return true;
> >  }
> > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> > index db13c9ddf9e3..ab009cf855c2 100644
> > --- a/arch/riscv/kernel/patch.c
> > +++ b/arch/riscv/kernel/patch.c
> > @@ -24,6 +24,7 @@ struct patch_insn {
> >  };
> >
> >  int riscv_patch_in_stop_machine = false;
> > +int riscv_patch_in_spinlock_init = false;
> >
> >  #ifdef CONFIG_MMU
> >
> > @@ -131,7 +132,7 @@ static int __patch_insn_write(void *addr, const
> > void *insn, size_t len)
> >          * safe but triggers a lockdep failure, so just elide it for that
> >          * specific case.
> >          */
> > -       if (!riscv_patch_in_stop_machine)
> > +       if (!riscv_patch_in_stop_machine && !riscv_patch_in_spinlock_init)
> >                 lockdep_assert_held(&text_mutex);
> >
> >         preempt_disable();
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 016b48fcd6f2..87ddf1702be4 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -268,7 +268,9 @@ static void __init riscv_spinlock_init(void)
> >         }
> >  #if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
> >         else {
> > +               riscv_patch_in_spinlock_init = 1;
> >                 static_branch_disable(&qspinlock_key);
> > +               riscv_patch_in_spinlock_init = 0;
> >                 pr_info("Ticket spinlock: enabled\n");
> >                 return;
> >         }
> >
> >
> >
> > --
> > Best Regards
> >  Guo Ren
>
> Thanks Guo for looking into this.
>
> Your solution is not very pretty but I don't have anything better :/
> Unless introducing a static_branch_XXX_nolock() API? I gave it a try
> and it fixes the issue, but not sure this will be accepted.
>
> The thing is the usage of static branches is temporary, we'll use
> alternatives when I finish working on getting the extensions very
> early from the ACPI tables (I have a poc that works, just needs some
> cleaning).
>
> So let's say that I make this early extension parsing my priority for
> 6.14, can we live with Guo's hack in this release? Or should we revert
> this commit?
I almost get the root cause. Please give me a while.

>
> Thanks,
>
> Alex




--
Best Regards
 Guo Ren

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Guo Ren]

Hi Alexandre & Conor

On Fri, Nov 29, 2024 at 8:50 PM Guo Ren <guoren@kernel.org> wrote:
>
> On Fri, Nov 29, 2024 at 6:32 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
> >
> > Hi everyone,
> >
> > On Fri, Nov 29, 2024 at 7:28 AM Guo Ren <guoren@kernel.org> wrote:
> > >
> > > Hi Conor & Alexandre,
> > >
> > > On Fri, Nov 29, 2024 at 10:58 AM Guo Ren <guoren@kernel.org> wrote:
> > > >
> > > > On Fri, Nov 29, 2024 at 8:55 AM Guo Ren <guoren@kernel.org> wrote:
> > > > >
> > > > > On Fri, Nov 29, 2024 at 12:19 AM Conor Dooley <conor@kernel.org> wrote:
> > > > > >
> > > > > > On Thu, Nov 28, 2024 at 03:50:09PM +0100, Alexandre Ghiti wrote:
> > > > > > > On 28/11/2024 15:14, Conor Dooley wrote:
> > > > > > > > On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> > > > > > > > > On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > > > > > > > > > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > > > > > > > > > In order to produce a generic kernel, a user can select
> > > > > > > > > > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > > > > > > > > spinlock implementation if Zabha or Ziccrse are not present.
> > > > > > > > > > >
> > > > > > > > > > > Note that we can't use alternatives here because the discovery of
> > > > > > > > > > > extensions is done too late and we need to start with the qspinlock
> > > > > > > > > > > implementation because the ticket spinlock implementation would pollute
> > > > > > > > > > > the spinlock value, so let's use static keys.
> > > > > > > > > > >
> > > > > > > > > > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > > > > > > > > >
> > > > > > > > > > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > > > > > > > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > > > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > > > > > > > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > > > > > > > > > breaks boot on polarfire soc. It dies before outputting anything to the
> > > > > > > > > > console. My .config has:
> > > > > > > > > >
> > > > > > > > > > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > > > > > > > > > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > > > > > > > > > CONFIG_RISCV_COMBO_SPINLOCKS=y
> > > > > > > > > I pointed out some of the fragility during review:
> > > > > > > > >
> > > > > > > > > https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> > > > > > > > >
> > > > > > > > > so I'm kinda surprised it got merged tbh :/
> > > > > > > > Maybe it could be reverted rather than having a broken boot with the
> > > > > > > > default settings in -rc1.
> > > > > > >
> > > > > > >
> > > > > > > No need to rush before we know what's happening,I guess you bisected to this
> > > > > > > commit right?
> > > > > >
> > > > > > The symptom is a failure to boot, without any console output, of course
> > > > > > I bisected it before blaming something specific. But I don't think it is
> > > > > > "rushing" as having -rc1 broken with an option's default is a massive pain
> > > > > > in the arse when it comes to testing.
> > > > > >
> > > > > > > I don't have this soc, so can you provide $stval/$sepc/$scause, a config, a
> > > > > > > kernel, anything?
> > > > > >
> > > > > > I don't have the former cos it died immediately on boot. config is
> > > > > > attached. It reproduces in QEMU so you don't need any hardware.
> > > > > If QEMU could reproduce, could you provide a dmesg by the below method?
> > > > >
> > > > > Qemu cmd append: -s -S
> > > > > ref: https://qemu-project.gitlab.io/qemu/system/gdb.html
> > > > >
> > > > > Connect gdb and in console:
> > > > > 1. file vmlinux
> > > > > 2. source ./Documentation/admin-guide/kdump/gdbmacros.txt
> > > > > 3. dmesg
> > > > >
> > > > > Then, we could get the kernel's early boot logs from memory.
> > > > I've reproduced it on qemu, thx for the config.
> > > >
> > > > Reading symbols from ../build-rv64lp64/vmlinux...
> > > > (gdb) tar rem:1234
> > > > Remote debugging using :1234
> > > > ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > > > 49              atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
> > > > (gdb) bt
> > > > #0  ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > > > #1  arch_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/arch/riscv/include/asm/spinlock.h:28
> > > > #2  do_raw_spin_lock (lock=lock@entry=0xffffffff81b9a5b8 <text_mutex>)
> > > > at /home/guoren/source/kernel/linux/kernel/locking/spinlock_debug.c:116
> > > > #3  0xffffffff80b2ea0e in __raw_spin_lock_irqsave
> > > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/include/linux/spinlock_api_smp.h:111
> > > > #4  _raw_spin_lock_irqsave (lock=lock@entry=0xffffffff81b9a5b8
> > > > <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/spinlock.c:162
> > > > #5  0xffffffff80b27c54 in rt_mutex_slowtrylock
> > > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1393
> > > > #6  0xffffffff80b295ea in rt_mutex_try_acquire
> > > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:319
> > > > #7  __rt_mutex_lock (state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1805
> > > > #8  __mutex_lock_common (ip=18446744071562135170, nest_lock=0x0,
> > > > subclass=0, state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:518
> > > > #9  mutex_lock_nested (lock=0xffffffff81b9a5b8 <text_mutex>,
> > > > subclass=subclass@entry=0) at
> > > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:529
> > > > #10 0xffffffff80010682 in arch_jump_label_transform_queue
> > > > (entry=entry@entry=0xffffffff8158da28, type=<optimized out>) at
> > > > /home/guoren/source/kernel/linux/arch/riscv/kernel/jump_label.c:39
> > > > #11 0xffffffff801d86b2 in __jump_label_update
> > > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>,
> > > > entry=0xffffffff8158da28, stop=stop@entry=0xffffffff815a5e68
> > > > <__tracepoint_ptr_initcall_finish>, init=init@entry=true)
> > > >     at /home/guoren/source/kernel/linux/kernel/jump_label.c:513
> > > > #12 0xffffffff801d890c in jump_label_update
> > > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > > /home/guoren/source/kernel/linux/kernel/jump_label.c:920
> > > > #13 0xffffffff801d8be8 in static_key_disable_cpuslocked
> > > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > > /home/guoren/source/kernel/linux/kernel/jump_label.c:240
> > > > #14 0xffffffff801d8c04 in static_key_disable
> > > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > > /home/guoren/source/kernel/linux/kernel/jump_label.c:248
> > > > #15 0xffffffff80c04a1a in riscv_spinlock_init () at
> > > > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:271
> > > > #16 setup_arch (cmdline_p=cmdline_p@entry=0xffffffff81a03e88) at
> > > > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:336
> > > > #17 0xffffffff80c007a2 in start_kernel () at
> > > > /home/guoren/source/kernel/linux/init/main.c:922
> > > > #18 0xffffffff80001164 in _start_kernel ()
> > > > Backtrace stopped: frame did not save the PC
> > > > (gdb) p /x lock
> > > > $1 = 0xffffffff81b9a5b8
> > > > (gdb) p /x *lock
> > > > $2 = {{val = {counter = 0x20000}, {locked = 0x0, pending = 0x0},
> > > > {locked_pending = 0x0, tail = 0x2}}}
> > >
> > > I have for you here a fast fixup for reference. (PS: I'm digging into
> > > the root cause mentioned by Will Deacon.)
> > >
> > > diff --git a/arch/riscv/include/asm/text-patching.h
> > > b/arch/riscv/include/asm/text-patching.h
> > > index 7228e266b9a1..0439609f1cff 100644
> > > --- a/arch/riscv/include/asm/text-patching.h
> > > +++ b/arch/riscv/include/asm/text-patching.h
> > > @@ -12,5 +12,6 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len);
> > >  int patch_text(void *addr, u32 *insns, size_t len);
> > >
> > >  extern int riscv_patch_in_stop_machine;
> > > +extern int riscv_patch_in_spinlock_init;
> > >
> > >  #endif /* _ASM_RISCV_PATCH_H */
> > > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
> > > index 6eee6f736f68..d9a5a5c1933d 100644
> > > --- a/arch/riscv/kernel/jump_label.c
> > > +++ b/arch/riscv/kernel/jump_label.c
> > > @@ -36,9 +36,11 @@ bool arch_jump_label_transform_queue(struct
> > > jump_entry *entry,
> > >                 insn = RISCV_INSN_NOP;
> > >         }
> > >
> > > -       mutex_lock(&text_mutex);
> > > +       if (!riscv_patch_in_spinlock_init)
> > > +               mutex_lock(&text_mutex);
> > >         patch_insn_write(addr, &insn, sizeof(insn));
> > > -       mutex_unlock(&text_mutex);
> > > +       if (!riscv_patch_in_spinlock_init)
> > > +               mutex_unlock(&text_mutex);
> > >
> > >         return true;
> > >  }
> > > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> > > index db13c9ddf9e3..ab009cf855c2 100644
> > > --- a/arch/riscv/kernel/patch.c
> > > +++ b/arch/riscv/kernel/patch.c
> > > @@ -24,6 +24,7 @@ struct patch_insn {
> > >  };
> > >
> > >  int riscv_patch_in_stop_machine = false;
> > > +int riscv_patch_in_spinlock_init = false;
> > >
> > >  #ifdef CONFIG_MMU
> > >
> > > @@ -131,7 +132,7 @@ static int __patch_insn_write(void *addr, const
> > > void *insn, size_t len)
> > >          * safe but triggers a lockdep failure, so just elide it for that
> > >          * specific case.
> > >          */
> > > -       if (!riscv_patch_in_stop_machine)
> > > +       if (!riscv_patch_in_stop_machine && !riscv_patch_in_spinlock_init)
> > >                 lockdep_assert_held(&text_mutex);
> > >
> > >         preempt_disable();
> > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > index 016b48fcd6f2..87ddf1702be4 100644
> > > --- a/arch/riscv/kernel/setup.c
> > > +++ b/arch/riscv/kernel/setup.c
> > > @@ -268,7 +268,9 @@ static void __init riscv_spinlock_init(void)
> > >         }
> > >  #if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
> > >         else {
> > > +               riscv_patch_in_spinlock_init = 1;
> > >                 static_branch_disable(&qspinlock_key);
> > > +               riscv_patch_in_spinlock_init = 0;
> > >                 pr_info("Ticket spinlock: enabled\n");
> > >                 return;
> > >         }
> > >
> > >
> > >
> > > --
> > > Best Regards
> > >  Guo Ren
> >
> > Thanks Guo for looking into this.
> >
> > Your solution is not very pretty but I don't have anything better :/
> > Unless introducing a static_branch_XXX_nolock() API? I gave it a try
> > and it fixes the issue, but not sure this will be accepted.
> >
> > The thing is the usage of static branches is temporary, we'll use
> > alternatives when I finish working on getting the extensions very
> > early from the ACPI tables (I have a poc that works, just needs some
> > cleaning).
> >
> > So let's say that I make this early extension parsing my priority for
> > 6.14, can we live with Guo's hack in this release? Or should we revert
> > this commit?
> I almost get the root cause. Please give me a while.
Here is the root cause (CONFIG_RT_MUTEXES=y):

When CONFIG_RT_MUTEXES=y, rt_mutex_try_acquire would change from
rt_mutex_cmpxchg_acquire to rt_mutex_slowtrylock.

rt_mutex_slowtrylock()
        *raw_spin_lock_irqsave(&lock->wait_lock, flags);*
        ret = __rt_mutex_slowtrylock(lock);
        *raw_spin_unlock_irqrestore(&lock->wait_lock, flags);*

Because queued_spin_#ops to ticket_#ops is changed one by one by
jump_label, spinlock usage would cause a deadlock during the change.

That means in arch/riscv/kernel/jump_label.c:
arch_jump_label_transform_queue() ->
mutex_lock(&text_mutex); -> raw_spin_lock  -> queued_spin_lock
                                         | -> raw_spin_unlock ->
queued_spin_unlock
patch_insn_write -> change the raw_spin_lock to ticket_lock
mutex_unlock(&text_mutex);
...
arch_jump_label_transform_queue() ->
mutex_lock(&text_mutex); -> raw_spin_lock -> ticket_lock
                                         | -> raw_spin_unlock; ->
queued_spin_unlock // *cause the problem*
patch_insn_write  ->  change the raw_spin_unlock to ticket_unlock
mutex_unlock(&text_mutex);
...
arch_jump_label_transform_queue() ->
mutex_lock(&text_mutex); -> raw_spin_lock -> ticket_lock // *deadlock*
                                         | -> raw_spin_unlock -> ticket_unlock
patch_insn_write -> change other raw_spin_#op -> ticket_#op
mutex_unlock(&text_mutex);

So, the solution is to disable mutex usage of
arch_jump_label_transform_queue() during spinlock_init, just like we
have done for stop_machine.

Ps:
The plan of improvement (remove the jump_label):
Use the Alternative to improve the performance of combo_spinlock's
ticket_lock (there is no branch jump for ticket_lock) and reduce its
code size.

--
Best Regards
 Guo Ren

Re: [PATCH v6 13/13] riscv: Add qspinlock support

[Guo Ren]

On Fri, Nov 29, 2024 at 6:32 PM Alexandre Ghiti <alexghiti@rivosinc.com> wrote:
>
> Hi everyone,
>
> On Fri, Nov 29, 2024 at 7:28 AM Guo Ren <guoren@kernel.org> wrote:
> >
> > Hi Conor & Alexandre,
> >
> > On Fri, Nov 29, 2024 at 10:58 AM Guo Ren <guoren@kernel.org> wrote:
> > >
> > > On Fri, Nov 29, 2024 at 8:55 AM Guo Ren <guoren@kernel.org> wrote:
> > > >
> > > > On Fri, Nov 29, 2024 at 12:19 AM Conor Dooley <conor@kernel.org> wrote:
> > > > >
> > > > > On Thu, Nov 28, 2024 at 03:50:09PM +0100, Alexandre Ghiti wrote:
> > > > > > On 28/11/2024 15:14, Conor Dooley wrote:
> > > > > > > On Thu, Nov 28, 2024 at 01:41:36PM +0000, Will Deacon wrote:
> > > > > > > > On Thu, Nov 28, 2024 at 12:56:55PM +0000, Conor Dooley wrote:
> > > > > > > > > On Sun, Nov 03, 2024 at 03:51:53PM +0100, Alexandre Ghiti wrote:
> > > > > > > > > > In order to produce a generic kernel, a user can select
> > > > > > > > > > CONFIG_COMBO_SPINLOCKS which will fallback at runtime to the ticket
> > > > > > > > > > spinlock implementation if Zabha or Ziccrse are not present.
> > > > > > > > > >
> > > > > > > > > > Note that we can't use alternatives here because the discovery of
> > > > > > > > > > extensions is done too late and we need to start with the qspinlock
> > > > > > > > > > implementation because the ticket spinlock implementation would pollute
> > > > > > > > > > the spinlock value, so let's use static keys.
> > > > > > > > > >
> > > > > > > > > > This is largely based on Guo's work and Leonardo reviews at [1].
> > > > > > > > > >
> > > > > > > > > > Link: https://lore.kernel.org/linux-riscv/20231225125847.2778638-1-guoren@kernel.org/ [1]
> > > > > > > > > > Signed-off-by: Guo Ren <guoren@kernel.org>
> > > > > > > > > > Signed-off-by: Alexandre Ghiti <alexghiti@rivosinc.com>
> > > > > > > > > This patch (now commit ab83647fadae2 ("riscv: Add qspinlock support"))
> > > > > > > > > breaks boot on polarfire soc. It dies before outputting anything to the
> > > > > > > > > console. My .config has:
> > > > > > > > >
> > > > > > > > > # CONFIG_RISCV_TICKET_SPINLOCKS is not set
> > > > > > > > > # CONFIG_RISCV_QUEUED_SPINLOCKS is not set
> > > > > > > > > CONFIG_RISCV_COMBO_SPINLOCKS=y
> > > > > > > > I pointed out some of the fragility during review:
> > > > > > > >
> > > > > > > > https://lore.kernel.org/all/20241111164259.GA20042@willie-the-truck/
> > > > > > > >
> > > > > > > > so I'm kinda surprised it got merged tbh :/
> > > > > > > Maybe it could be reverted rather than having a broken boot with the
> > > > > > > default settings in -rc1.
> > > > > >
> > > > > >
> > > > > > No need to rush before we know what's happening,I guess you bisected to this
> > > > > > commit right?
> > > > >
> > > > > The symptom is a failure to boot, without any console output, of course
> > > > > I bisected it before blaming something specific. But I don't think it is
> > > > > "rushing" as having -rc1 broken with an option's default is a massive pain
> > > > > in the arse when it comes to testing.
> > > > >
> > > > > > I don't have this soc, so can you provide $stval/$sepc/$scause, a config, a
> > > > > > kernel, anything?
> > > > >
> > > > > I don't have the former cos it died immediately on boot. config is
> > > > > attached. It reproduces in QEMU so you don't need any hardware.
> > > > If QEMU could reproduce, could you provide a dmesg by the below method?
> > > >
> > > > Qemu cmd append: -s -S
> > > > ref: https://qemu-project.gitlab.io/qemu/system/gdb.html
> > > >
> > > > Connect gdb and in console:
> > > > 1. file vmlinux
> > > > 2. source ./Documentation/admin-guide/kdump/gdbmacros.txt
> > > > 3. dmesg
> > > >
> > > > Then, we could get the kernel's early boot logs from memory.
> > > I've reproduced it on qemu, thx for the config.
> > >
> > > Reading symbols from ../build-rv64lp64/vmlinux...
> > > (gdb) tar rem:1234
> > > Remote debugging using :1234
> > > ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > > 49              atomic_cond_read_acquire(&lock->val, ticket == (u16)VAL);
> > > (gdb) bt
> > > #0  ticket_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/include/asm-generic/ticket_spinlock.h:49
> > > #1  arch_spin_lock (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/arch/riscv/include/asm/spinlock.h:28
> > > #2  do_raw_spin_lock (lock=lock@entry=0xffffffff81b9a5b8 <text_mutex>)
> > > at /home/guoren/source/kernel/linux/kernel/locking/spinlock_debug.c:116
> > > #3  0xffffffff80b2ea0e in __raw_spin_lock_irqsave
> > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/include/linux/spinlock_api_smp.h:111
> > > #4  _raw_spin_lock_irqsave (lock=lock@entry=0xffffffff81b9a5b8
> > > <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/spinlock.c:162
> > > #5  0xffffffff80b27c54 in rt_mutex_slowtrylock
> > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1393
> > > #6  0xffffffff80b295ea in rt_mutex_try_acquire
> > > (lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:319
> > > #7  __rt_mutex_lock (state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex.c:1805
> > > #8  __mutex_lock_common (ip=18446744071562135170, nest_lock=0x0,
> > > subclass=0, state=2, lock=0xffffffff81b9a5b8 <text_mutex>) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:518
> > > #9  mutex_lock_nested (lock=0xffffffff81b9a5b8 <text_mutex>,
> > > subclass=subclass@entry=0) at
> > > /home/guoren/source/kernel/linux/kernel/locking/rtmutex_api.c:529
> > > #10 0xffffffff80010682 in arch_jump_label_transform_queue
> > > (entry=entry@entry=0xffffffff8158da28, type=<optimized out>) at
> > > /home/guoren/source/kernel/linux/arch/riscv/kernel/jump_label.c:39
> > > #11 0xffffffff801d86b2 in __jump_label_update
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>,
> > > entry=0xffffffff8158da28, stop=stop@entry=0xffffffff815a5e68
> > > <__tracepoint_ptr_initcall_finish>, init=init@entry=true)
> > >     at /home/guoren/source/kernel/linux/kernel/jump_label.c:513
> > > #12 0xffffffff801d890c in jump_label_update
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > /home/guoren/source/kernel/linux/kernel/jump_label.c:920
> > > #13 0xffffffff801d8be8 in static_key_disable_cpuslocked
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > /home/guoren/source/kernel/linux/kernel/jump_label.c:240
> > > #14 0xffffffff801d8c04 in static_key_disable
> > > (key=key@entry=0xffffffff81a1abb0 <qspinlock_key>) at
> > > /home/guoren/source/kernel/linux/kernel/jump_label.c:248
> > > #15 0xffffffff80c04a1a in riscv_spinlock_init () at
> > > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:271
> > > #16 setup_arch (cmdline_p=cmdline_p@entry=0xffffffff81a03e88) at
> > > /home/guoren/source/kernel/linux/arch/riscv/kernel/setup.c:336
> > > #17 0xffffffff80c007a2 in start_kernel () at
> > > /home/guoren/source/kernel/linux/init/main.c:922
> > > #18 0xffffffff80001164 in _start_kernel ()
> > > Backtrace stopped: frame did not save the PC
> > > (gdb) p /x lock
> > > $1 = 0xffffffff81b9a5b8
> > > (gdb) p /x *lock
> > > $2 = {{val = {counter = 0x20000}, {locked = 0x0, pending = 0x0},
> > > {locked_pending = 0x0, tail = 0x2}}}
> >
> > I have for you here a fast fixup for reference. (PS: I'm digging into
> > the root cause mentioned by Will Deacon.)
> >
> > diff --git a/arch/riscv/include/asm/text-patching.h
> > b/arch/riscv/include/asm/text-patching.h
> > index 7228e266b9a1..0439609f1cff 100644
> > --- a/arch/riscv/include/asm/text-patching.h
> > +++ b/arch/riscv/include/asm/text-patching.h
> > @@ -12,5 +12,6 @@ int patch_text_set_nosync(void *addr, u8 c, size_t len);
> >  int patch_text(void *addr, u32 *insns, size_t len);
> >
> >  extern int riscv_patch_in_stop_machine;
> > +extern int riscv_patch_in_spinlock_init;
> >
> >  #endif /* _ASM_RISCV_PATCH_H */
> > diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
> > index 6eee6f736f68..d9a5a5c1933d 100644
> > --- a/arch/riscv/kernel/jump_label.c
> > +++ b/arch/riscv/kernel/jump_label.c
> > @@ -36,9 +36,11 @@ bool arch_jump_label_transform_queue(struct
> > jump_entry *entry,
> >                 insn = RISCV_INSN_NOP;
> >         }
> >
> > -       mutex_lock(&text_mutex);
> > +       if (!riscv_patch_in_spinlock_init)
> > +               mutex_lock(&text_mutex);
> >         patch_insn_write(addr, &insn, sizeof(insn));
> > -       mutex_unlock(&text_mutex);
> > +       if (!riscv_patch_in_spinlock_init)
> > +               mutex_unlock(&text_mutex);
> >
> >         return true;
> >  }
> > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> > index db13c9ddf9e3..ab009cf855c2 100644
> > --- a/arch/riscv/kernel/patch.c
> > +++ b/arch/riscv/kernel/patch.c
> > @@ -24,6 +24,7 @@ struct patch_insn {
> >  };
> >
> >  int riscv_patch_in_stop_machine = false;
> > +int riscv_patch_in_spinlock_init = false;
> >
> >  #ifdef CONFIG_MMU
> >
> > @@ -131,7 +132,7 @@ static int __patch_insn_write(void *addr, const
> > void *insn, size_t len)
> >          * safe but triggers a lockdep failure, so just elide it for that
> >          * specific case.
> >          */
> > -       if (!riscv_patch_in_stop_machine)
> > +       if (!riscv_patch_in_stop_machine && !riscv_patch_in_spinlock_init)
> >                 lockdep_assert_held(&text_mutex);
> >
> >         preempt_disable();
> > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > index 016b48fcd6f2..87ddf1702be4 100644
> > --- a/arch/riscv/kernel/setup.c
> > +++ b/arch/riscv/kernel/setup.c
> > @@ -268,7 +268,9 @@ static void __init riscv_spinlock_init(void)
> >         }
> >  #if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
> >         else {
> > +               riscv_patch_in_spinlock_init = 1;
> >                 static_branch_disable(&qspinlock_key);
> > +               riscv_patch_in_spinlock_init = 0;
> >                 pr_info("Ticket spinlock: enabled\n");
> >                 return;
> >         }
> >
> >
> >
> > --
> > Best Regards
> >  Guo Ren
>
> Thanks Guo for looking into this.
>
> Your solution is not very pretty but I don't have anything better :/
Here is another solution (Only one file modified, maybe better):

diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
index 6eee6f736f68..654ed159c830 100644
--- a/arch/riscv/kernel/jump_label.c
+++ b/arch/riscv/kernel/jump_label.c
@@ -36,9 +36,15 @@ bool arch_jump_label_transform_queue(struct
jump_entry *entry,
                insn = RISCV_INSN_NOP;
        }

-       mutex_lock(&text_mutex);
-       patch_insn_write(addr, &insn, sizeof(insn));
-       mutex_unlock(&text_mutex);
+       if (early_boot_irqs_disabled) {
+               riscv_patch_in_stop_machine = 1;
+               patch_insn_write(addr, &insn, sizeof(insn));
+               riscv_patch_in_stop_machine = 0;
+       } else {
+               mutex_lock(&text_mutex);
+               patch_insn_write(addr, &insn, sizeof(insn));
+               mutex_unlock(&text_mutex);
+       }

        return true;
 }

> Unless introducing a static_branch_XXX_nolock() API? I gave it a try
> and it fixes the issue, but not sure this will be accepted.
>
> The thing is the usage of static branches is temporary, we'll use
> alternatives when I finish working on getting the extensions very
The "alternatives" also need patch codes one by one, which means it
will meet the same problem as the jump_label.

So, you will still provide a patch like the one above for the
alternative implementation.

> early from the ACPI tables (I have a poc that works, just needs some
> cleaning).
>
> So let's say that I make this early extension parsing my priority for
> 6.14, can we live with Guo's hack in this release? Or should we revert
> this commit?
>
> Thanks,
>
> Alex
--
Best Regards
 Guo Ren

Re: [PATCH v6 00/13] Zacas/Zabha support and qspinlocks

[Andrea Parri]

> Alexandre Ghiti (11):
>   riscv: Move cpufeature.h macros into their own header
>   riscv: Do not fail to build on byte/halfword operations with Zawrs
>   riscv: Implement cmpxchg32/64() using Zacas
>   dt-bindings: riscv: Add Zabha ISA extension description
>   riscv: Implement cmpxchg8/16() using Zabha
>   riscv: Improve zacas fully-ordered cmpxchg()
>   riscv: Implement arch_cmpxchg128() using Zacas
>   riscv: Implement xchg8/16() using Zabha
>   riscv: Add ISA extension parsing for Ziccrse
>   dt-bindings: riscv: Add Ziccrse ISA extension description
>   riscv: Add qspinlock support
> 
> Guo Ren (2):
>   asm-generic: ticket-lock: Reuse arch_spinlock_t of qspinlock
>   asm-generic: ticket-lock: Add separate ticket-lock.h

For the series,

Reviewed-by: Andrea Parri <parri.andrea@gmail.com>

  Andrea

Re: [PATCH v6 00/13] Zacas/Zabha support and qspinlocks

[patchwork-bot+linux-riscv]

Hello:

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

On Sun,  3 Nov 2024 15:51:40 +0100 you wrote:
> This implements [cmp]xchgXX() macros using Zacas and Zabha extensions
> and finally uses those newly introduced macros to add support for
> qspinlocks: note that this implementation of qspinlocks satisfies the
> forward progress guarantee.
> 
> It also uses Ziccrse to provide the qspinlock implementation.
> 
> [...]

Here is the summary with links:
  - [v6,01/13] riscv: Move cpufeature.h macros into their own header
    https://git.kernel.org/riscv/c/010e12aa4925
  - [v6,02/13] riscv: Do not fail to build on byte/halfword operations with Zawrs
    https://git.kernel.org/riscv/c/af042c457db0
  - [v6,03/13] riscv: Implement cmpxchg32/64() using Zacas
    https://git.kernel.org/riscv/c/38acdee32d23
  - [v6,04/13] dt-bindings: riscv: Add Zabha ISA extension description
    https://git.kernel.org/riscv/c/51624ddcf59d
  - [v6,05/13] riscv: Implement cmpxchg8/16() using Zabha
    https://git.kernel.org/riscv/c/1658ef4314b3
  - [v6,06/13] riscv: Improve zacas fully-ordered cmpxchg()
    https://git.kernel.org/riscv/c/6116e22ef33a
  - [v6,07/13] riscv: Implement arch_cmpxchg128() using Zacas
    https://git.kernel.org/riscv/c/f7bd2be7663c
  - [v6,08/13] riscv: Implement xchg8/16() using Zabha
    https://git.kernel.org/riscv/c/97ddab7fbea8
  - [v6,09/13] asm-generic: ticket-lock: Reuse arch_spinlock_t of qspinlock
    https://git.kernel.org/riscv/c/cbe82e140bb7
  - [v6,10/13] asm-generic: ticket-lock: Add separate ticket-lock.h
    https://git.kernel.org/riscv/c/22c33321e260
  - [v6,11/13] riscv: Add ISA extension parsing for Ziccrse
    https://git.kernel.org/riscv/c/2d36fe89d872
  - [v6,12/13] dt-bindings: riscv: Add Ziccrse ISA extension description
    https://git.kernel.org/riscv/c/447b2afbcde1
  - [v6,13/13] riscv: Add qspinlock support
    https://git.kernel.org/riscv/c/ab83647fadae

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