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

From: "Stefan O'Rear" <sorear@fastmail.com>
To: guoren <guoren@kernel.org>,
	paul.walmsley@sifive.com, "Anup Patel" <anup@brainfault.org>,
	peterz@infradead.org, mingo@redhat.com, will@kernel.org,
	"Palmer Dabbelt" <palmer@rivosinc.com>,
	longman@redhat.com, boqun.feng@gmail.com, tglx@linutronix.de,
	paulmck@kernel.org, rostedt@goodmis.org, rdunlap@infradead.org,
	catalin.marinas@arm.com,
	"Conor Dooley" <conor.dooley@microchip.com>,
	xiaoguang.xing@sophgo.com, "Björn Töpel" <bjorn@rivosinc.com>,
	alexghiti@rivosinc.com, "Kees Cook" <keescook@chromium.org>,
	greentime.hu@sifive.com, "Andrew Jones" <ajones@ventanamicro.com>,
	jszhang@kernel.org, wefu@redhat.com, wuwei2016@iscas.ac.cn
Cc: linux-arch@vger.kernel.org, linux-riscv@lists.infradead.org,
	linux-doc@vger.kernel.org, kvm@vger.kernel.org,
	virtualization@lists.linux-foundation.org,
	linux-csky@vger.kernel.org, "Guo Ren" <guoren@linux.alibaba.com>
Subject: Re: [PATCH V10 07/19] riscv: qspinlock: errata: Introduce ERRATA_THEAD_QSPINLOCK
Date: Mon, 07 Aug 2023 01:23:34 -0400	[thread overview]
Message-ID: <ae320af5-6cca-4689-aa66-9d0193713d40@app.fastmail.com> (raw)
In-Reply-To: <20230802164701.192791-8-guoren@kernel.org>

On Wed, Aug 2, 2023, at 12:46 PM, guoren@kernel.org wrote:
> From: Guo Ren <guoren@linux.alibaba.com>
>
> 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 <guoren@linux.alibaba.com>
> 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)

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.

-s

> +
>  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