Re: [PATCH RFC v4 4/4] RISC-V: paravirt: Support nopvspin to disable PARAVIRT_SPINLOCKS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] build: unresolved virt_spin_lock_key when PARAVIRT_SPINLOCKS is disabled
- [High] locking/pv: inverted nopvspin semantics disables TAS fallback on unaccelerated VMs
- [High] locking/pv: skipping ZICCRSE extension check exposes guest to LR/SC livelock
- [Low] locking/pv: casting atomic_t to raw pointer in smp_cond_load_relaxed
- [Low] locking/pv: static_branch_likely pessimizes native spinlock fastpath
--

commit 51ef25d6df1335ba98379605c62c0034ca0b9dfe
Author: GUO Ren (XuanTie) <guoren@kernel.org>

RISC-V: paravirt: Support nopvspin to disable PARAVIRT_SPINLOCKS

VM guests should fall back to a Test-and-Set spinlock when
PARAVIRT_SPINLOCKS is disabled, because fair locks suffer from severe
lock-holder preemption issues. The virt_spin_lock_key shortcuts
queued_spin_lock_slowpath(), allowing virt_spin_lock() to hijack it.

> diff --git a/arch/riscv/include/asm/qspinlock.h b/arch/riscv/include/asm/qspinlock.h
> index 330b714edc447..9c7108baa40a1 100644
> --- a/arch/riscv/include/asm/qspinlock.h
> +++ b/arch/riscv/include/asm/qspinlock.h
[ ... ]
> @@ -29,5 +31,27 @@ static inline void queued_spin_unlock(struct qspinlock *lock)
>  #endif /* CONFIG_PARAVIRT_SPINLOCKS */
>  
>  #include <asm-generic/qspinlock.h>
> +#include <asm/jump_label.h>
> +
> +/*
> + * KVM guests fall back to a Test-and-Set spinlock because fair locks suffer
> + * from severe lock-holder-preemption issues. When virt_spin_lock_key is
> + * enabled, virt_spin_lock() shortcuts queued_spin_lock_slowpath() and hijacks
> + * the lock acquisition.
> + */
> +DECLARE_STATIC_KEY_FALSE(virt_spin_lock_key);

[Severity: High]
Is it possible for the kernel to fail to link due to an undefined symbol here?

virt_spin_lock_key is unconditionally declared and used by the inline function
rv_virt_spin_lock(). However, its definition resides in
arch/riscv/kernel/qspinlock_paravirt.c, which is only compiled when
CONFIG_PARAVIRT_SPINLOCKS is enabled. Does this break the build when
CONFIG_QUEUED_SPINLOCKS=y but CONFIG_PARAVIRT_SPINLOCKS=n?

> +
> +#define virt_spin_lock rv_virt_spin_lock
> +static inline bool rv_virt_spin_lock(struct qspinlock *lock)
> +{
> +	if (!static_branch_likely(&virt_spin_lock_key))
> +		return false;

[Severity: Low]
Does using likely() here force the compiler to treat the bare-metal default
path (when the key is disabled) as out-of-line?

Since virt_spin_lock_key is false by default on bare-metal systems, wouldn't
static_branch_likely() require an unconditional jump in the native queued
spinlock fastpath, degrading performance? Should this be
static_branch_unlikely() instead?

> +
> +	do {
> +		smp_cond_load_relaxed((s32 *)&lock->val, VAL == 0);

[Severity: Low]
Is it safe to cast &lock->val (an atomic_t) to a raw pointer like (s32 *)?

This bypasses the atomic_t abstraction. Should atomic_cond_read_relaxed()
be used here instead?

> +	} while (atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL) != 0);
> +
> +	return true;
> +}
>  
>  #endif /* _ASM_RISCV_QSPINLOCK_H */
> diff --git a/arch/riscv/kernel/qspinlock_paravirt.c b/arch/riscv/kernel/qspinlock_paravirt.c
> index 04b13994e971c..28c6c78d7e84c 100644
> --- a/arch/riscv/kernel/qspinlock_paravirt.c
> +++ b/arch/riscv/kernel/qspinlock_paravirt.c
[ ... ]
> @@ -58,6 +60,12 @@ bool __init pv_qspinlock_init(void)
>  	if (!sbi_probe_extension(SBI_EXT_PVLOCK))
>  		return false;
>  
> +	if (nopvspin) {
> +		static_branch_enable(&virt_spin_lock_key);
> +		pr_info("virt_spin_lock enabled by nopvspin\n");
> +		return true;
> +	}

[Severity: High]
Do the nopvspin semantics here invert the expected behavior?

If an unaccelerated VM boots without SBI_EXT_PVLOCK, pv_qspinlock_init()
returns early before enabling virt_spin_lock_key, defeating the TAS fallback.

Furthermore, if nopvspin is passed, it statically enables the key, whereas on
x86, nopvspin explicitly disables PV optimizations and forces native queued
spinlocks.

[Severity: High]
Can this early return expose guests to LR/SC livelocks?

If a VM uses nopvspin, returning true here causes riscv_spinlock_init() in
arch/riscv/kernel/setup.c to completely bypass the hardware extension checks
like ZABHA, ZACAS, or ZICCRSE.

Because rv_virt_spin_lock() relies on an LR/SC atomic_cmpxchg loop, will it
livelock under heavy contention if the underlying hardware lacks the ZICCRSE
extension, which provides forward progress guarantees?

> +
>  	pr_info("PV qspinlocks enabled\n");
>  	__pv_init_lock_hash();

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-pvqspinlock-v4-0-409a22aed6ef@kernel.org?part=4