[PATCH V10 07/19] riscv: qspinlock: errata: Introduce ERRATA_THEAD_QSPINLOCK
Palmer Dabbelt
palmer at rivosinc.com
Wed Sep 13 11:54:21 PDT 2023
On Sun, 06 Aug 2023 22:23:34 PDT (-0700), sorear at fastmail.com wrote:
> On Wed, Aug 2, 2023, at 12:46 PM, guoren at kernel.org wrote:
>> From: Guo Ren <guoren at 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 at linux.alibaba.com>
>> Signed-off-by: Guo Ren <guoren at 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
>
>> +
>> 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 at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
More information about the linux-riscv
mailing list