Re: [PATCH RFC v4 2/4] RISC-V: paravirt: Add pvqspinlock frontend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] riscv: out-of-line static_call in queued_spin_unlock() degrades performance
- [High] riscv: missing headers in qspinlock_paravirt cause build failures
--

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

RISC-V: paravirt: Add pvqspinlock frontend

Add a virtualization-friendly unfair qspinlock frontend that halts the
virtual CPU instead of spinning.

> diff --git a/arch/riscv/include/asm/qspinlock.h b/arch/riscv/include/asm/qspinlock.h
> new file mode 100644
> index 0000000000000..330b714edc447
> --- /dev/null
> +++ b/arch/riscv/include/asm/qspinlock.h

[ ... ]

> +#define queued_spin_unlock	queued_spin_unlock
> +static inline void queued_spin_unlock(struct qspinlock *lock)
> +{
> +	static_call(pv_queued_spin_unlock)(lock);
> +}

[Severity: High]
Does replacing the inline smp_store_release() with a static_call() regress
native bare-metal performance?

Because RISC-V does not yet support inline static calls, this change replaces
the fast path native smp_store_release() instruction with an out-of-line
function call or indirect branch across the kernel for every spin_unlock
operation when CONFIG_PARAVIRT_SPINLOCKS is enabled.

> diff --git a/arch/riscv/include/asm/qspinlock_paravirt.h b/arch/riscv/include/asm/qspinlock_paravirt.h
> new file mode 100644
> index 0000000000000..7261cd9b13b2e
> --- /dev/null
> +++ b/arch/riscv/include/asm/qspinlock_paravirt.h

[ ... ]

> +void dummy_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
> +void dummy_queued_spin_unlock(struct qspinlock *lock);
> +
> +DECLARE_STATIC_CALL(pv_queued_spin_lock_slowpath, dummy_queued_spin_lock_slowpath);

[Severity: High]
Can this cause a build failure due to an undefined macro?

DECLARE_STATIC_CALL requires <linux/static_call_types.h>, which does not
appear to be included in this file.

> +DECLARE_STATIC_CALL(pv_queued_spin_unlock, dummy_queued_spin_unlock);

[ ... ]

> diff --git a/arch/riscv/kernel/qspinlock_paravirt.c b/arch/riscv/kernel/qspinlock_paravirt.c
> new file mode 100644
> index 0000000000000..c534447437fec
> --- /dev/null
> +++ b/arch/riscv/kernel/qspinlock_paravirt.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2026 GUO Ren (XuanTie) <guoren@kernel.org>
> + */
> +
> +#include <linux/static_call.h>
> +#include <asm/qspinlock_paravirt.h>
> +#include <asm/sbi.h>
> +
> +void pv_kick(int cpu)
> +{
> +	sbi_ecall(SBI_EXT_PVLOCK, SBI_EXT_PVLOCK_KICK_CPU,
> +		  cpuid_to_hartid_map(cpu), 0, 0, 0, 0, 0);

[Severity: High]
Will this cause an implicit declaration error when compiling?

The cpuid_to_hartid_map() function requires <asm/smp.h>, which is missing
from the includes in this file.

> +}

[ ... ]

> +static void native_queued_spin_unlock(struct qspinlock *lock)
> +{
> +	/*
> +	 * unlock() needs release semantics:
> +	 */
> +	smp_store_release(&lock->locked, 0);
> +}

[Severity: High]
By forcing this into an out-of-line static function, does this prevent the
compiler from inlining the native fast path entirely?

> +
> +DEFINE_STATIC_CALL(pv_queued_spin_lock_slowpath, native_queued_spin_lock_slowpath);

[Severity: High]
Will this result in an undeclared identifier error for
native_queued_spin_lock_slowpath?

The native_queued_spin_lock_slowpath symbol requires <asm/qspinlock.h>,
which is missing from the includes in this file.

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