Re: [PATCH V10 07/19] riscv: qspinlock: errata: Introduce ERRATA_THEAD_QSPINLOCK

On Sun, 06 Aug 2023 22:23:34 PDT (-0700), sorear@fastmail.com wrote:

On Wed, Aug 2, 2023, at 12:46 PM, guoren@kernel.org wrote:

quoted

From: Guo Ren <redacted>

According to qspinlock requirements, RISC-V gives out a weak LR/SC
forward progress guarantee which does not satisfy qspinlock. But
many vendors could produce stronger forward guarantee LR/SC to
ensure the xchg_tail could be finished in time on any kind of
hart. T-HEAD is the vendor which implements strong forward
guarantee LR/SC instruction pairs, so enable qspinlock for T-HEAD
with errata help.

T-HEAD early version of processors has the merge buffer delay
problem, so we need ERRATA_WRITEONCE to support qspinlock.

Signed-off-by: Guo Ren <redacted>
Signed-off-by: Guo Ren <guoren@kernel.org>
---
 arch/riscv/Kconfig.errata              | 13 +++++++++++++
 arch/riscv/errata/thead/errata.c       | 24 ++++++++++++++++++++++++
 arch/riscv/include/asm/errata_list.h   | 20 ++++++++++++++++++++
 arch/riscv/include/asm/vendorid_list.h |  3 ++-
 arch/riscv/kernel/cpufeature.c         |  3 ++-
 5 files changed, 61 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/Kconfig.errata b/arch/riscv/Kconfig.errata
index 4745a5c57e7c..eb43677b13cc 100644
--- a/arch/riscv/Kconfig.errata
+++ b/arch/riscv/Kconfig.errata

@@ -96,4 +96,17 @@ config ERRATA_THEAD_WRITE_ONCE

 	  If you don't know what to do here, say "Y".

+config ERRATA_THEAD_QSPINLOCK
+	bool "Apply T-Head queued spinlock errata"
+	depends on ERRATA_THEAD
+	default y
+	help
+	  The T-HEAD C9xx processors implement strong fwd guarantee LR/SC to
+	  match the xchg_tail requirement of qspinlock.
+
+	  This will apply the QSPINLOCK errata to handle the non-standard
+	  behavior via using qspinlock instead of ticket_lock.
+
+	  If you don't know what to do here, say "Y".

If this is to be applied, I would like to see a detailed explanation somewhere,
preferably with citations, of:

(a) The memory model requirements for qspinlock
(b) Why, with arguments, RISC-V does not architecturally meet (a)
(c) Why, with arguments, T-HEAD C9xx meets (a)
(d) Why at least one other architecture which defines ARCH_USE_QUEUED_SPINLOCKS
    meets (a)

I agree.

Just having a magic fence that makes qspinlocks stop livelocking on some 
processors is going to lead to a mess -- I'd argue this means those 
processors just don't provide the forward progress guarantee, but we'd 
really need something written down about what this new custom 
instruction aliasing as a fence does.

As far as I can tell, the RISC-V guarantees concerning constrained LR/SC loops
(livelock freedom but no starvation freedom) are exactly the same as those in
Armv8 (as of 0487F.c) for equivalent loops, and xchg_tail compiles to a
constrained LR/SC loop with guaranteed eventual success (with -O1).  Clearly you
disagree; I would like to see your perspective.

It sounds to me like this processor might be quite broken: if it's 
permanently holding stores in a buffer we're going to have more issues 
than just qspinlock, pretty much anything concurrent is going to have 
issues -- and that's not just in the kernel, there's concurrent 
userspace code as well.

-s

quoted

+
 endmenu # "CPU errata selection"

diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
index 881729746d2e..d560dc45c0e7 100644
--- a/arch/riscv/errata/thead/errata.c
+++ b/arch/riscv/errata/thead/errata.c

@@ -86,6 +86,27 @@ static bool errata_probe_write_once(unsigned int stage,
 	return false;
 }

+static bool errata_probe_qspinlock(unsigned int stage,
+				   unsigned long arch_id, unsigned long impid)
+{
+	if (!IS_ENABLED(CONFIG_ERRATA_THEAD_QSPINLOCK))
+		return false;
+
+	/*
+	 * The queued_spinlock torture would get in livelock without
+	 * ERRATA_THEAD_WRITE_ONCE fixup for the early versions of T-HEAD
+	 * processors.
+	 */
+	if (arch_id == 0 && impid == 0 &&
+	    !IS_ENABLED(CONFIG_ERRATA_THEAD_WRITE_ONCE))
+		return false;
+
+	if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
+		return true;
+
+	return false;
+}
+
 static u32 thead_errata_probe(unsigned int stage,
 			      unsigned long archid, unsigned long impid)
 {

@@ -103,6 +124,9 @@ static u32 thead_errata_probe(unsigned int stage,
 	if (errata_probe_write_once(stage, archid, impid))
 		cpu_req_errata |= BIT(ERRATA_THEAD_WRITE_ONCE);

+	if (errata_probe_qspinlock(stage, archid, impid))
+		cpu_req_errata |= BIT(ERRATA_THEAD_QSPINLOCK);
+
 	return cpu_req_errata;
 }

diff --git a/arch/riscv/include/asm/errata_list.h

b/arch/riscv/include/asm/errata_list.h
index fbb2b8d39321..a696d18d1b0d 100644

--- a/arch/riscv/include/asm/errata_list.h
+++ b/arch/riscv/include/asm/errata_list.h

@@ -141,6 +141,26 @@ asm volatile(ALTERNATIVE(						\
 	: "=r" (__ovl) :						\
 	: "memory")

+static __always_inline bool
+riscv_has_errata_thead_qspinlock(void)
+{
+	if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) {
+		asm_volatile_goto(
+		ALTERNATIVE(
+		"j	%l[l_no]", "nop",
+		THEAD_VENDOR_ID,
+		ERRATA_THEAD_QSPINLOCK,
+		CONFIG_ERRATA_THEAD_QSPINLOCK)
+		: : : : l_no);
+	} else {
+		goto l_no;
+	}
+
+	return true;
+l_no:
+	return false;
+}
+
 #endif /* __ASSEMBLY__ */

 #endif

diff --git a/arch/riscv/include/asm/vendorid_list.h

b/arch/riscv/include/asm/vendorid_list.h
index 73078cfe4029..1f1d03877f5f 100644

--- a/arch/riscv/include/asm/vendorid_list.h
+++ b/arch/riscv/include/asm/vendorid_list.h

@@ -19,7 +19,8 @@
 #define	ERRATA_THEAD_CMO 1
 #define	ERRATA_THEAD_PMU 2
 #define	ERRATA_THEAD_WRITE_ONCE 3
-#define	ERRATA_THEAD_NUMBER 4
+#define	ERRATA_THEAD_QSPINLOCK 4
+#define	ERRATA_THEAD_NUMBER 5
 #endif

 #endif

diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index f8dbbe1bbd34..d9694fe40a9a 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c

@@ -342,7 +342,8 @@ void __init riscv_fill_hwcap(void)
 		 * spinlock value, the only way is to change from queued_spinlock to
 		 * ticket_spinlock, but can not be vice.
 		 */
-		if (!force_qspinlock) {
+		if (!force_qspinlock &&
+		    !riscv_has_errata_thead_qspinlock()) {
 			set_bit(RISCV_ISA_EXT_XTICKETLOCK, isainfo->isa);
 		}
 #endif
--

2.36.1


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