On Sun, Sep 10, 2023 at 04:29:05AM -0400, guoren@xxxxxxxxxx wrote: > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > > Using static_call to switch between: > native_queued_spin_lock_slowpath() __pv_queued_spin_lock_slowpath() > native_queued_spin_unlock() __pv_queued_spin_unlock() > > Finish the pv_wait implementation, but pv_kick needs the SBI > definition of the next patches. > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > Signed-off-by: Guo Ren <guoren@xxxxxxxxxx> > --- > arch/riscv/include/asm/Kbuild | 1 - > arch/riscv/include/asm/qspinlock.h | 35 +++++++++++++ > arch/riscv/include/asm/qspinlock_paravirt.h | 29 +++++++++++ > arch/riscv/include/asm/spinlock.h | 2 +- > arch/riscv/kernel/qspinlock_paravirt.c | 57 +++++++++++++++++++++ > arch/riscv/kernel/setup.c | 4 ++ > 6 files changed, 126 insertions(+), 2 deletions(-) > create mode 100644 arch/riscv/include/asm/qspinlock.h > create mode 100644 arch/riscv/include/asm/qspinlock_paravirt.h > create mode 100644 arch/riscv/kernel/qspinlock_paravirt.c > > diff --git a/arch/riscv/include/asm/Kbuild b/arch/riscv/include/asm/Kbuild > index a0dc85e4a754..b89cb3b73c13 100644 > --- a/arch/riscv/include/asm/Kbuild > +++ b/arch/riscv/include/asm/Kbuild > @@ -7,6 +7,5 @@ generic-y += parport.h > generic-y += spinlock_types.h > generic-y += qrwlock.h > generic-y += qrwlock_types.h > -generic-y += qspinlock.h > generic-y += user.h > generic-y += vmlinux.lds.h > diff --git a/arch/riscv/include/asm/qspinlock.h b/arch/riscv/include/asm/qspinlock.h > new file mode 100644 > index 000000000000..7d4f416c908c > --- /dev/null > +++ b/arch/riscv/include/asm/qspinlock.h > @@ -0,0 +1,35 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c), 2023 Alibaba Cloud > + * Authors: > + * Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > + */ > + > +#ifndef _ASM_RISCV_QSPINLOCK_H > +#define _ASM_RISCV_QSPINLOCK_H > + > +#ifdef CONFIG_PARAVIRT_SPINLOCKS > +#include <asm/qspinlock_paravirt.h> > + > +/* How long a lock should spin before we consider blocking */ > +#define SPIN_THRESHOLD (1 << 15) > + > +void native_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val); > +void __pv_init_lock_hash(void); > +void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val); > + > +static inline void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val) > +{ > + static_call(pv_queued_spin_lock_slowpath)(lock, val); > +} > + > +#define queued_spin_unlock queued_spin_unlock > +static inline void queued_spin_unlock(struct qspinlock *lock) > +{ > + static_call(pv_queued_spin_unlock)(lock); > +} > +#endif /* CONFIG_PARAVIRT_SPINLOCKS */ > + > +#include <asm-generic/qspinlock.h> > + > +#endif /* _ASM_RISCV_QSPINLOCK_H */ > diff --git a/arch/riscv/include/asm/qspinlock_paravirt.h b/arch/riscv/include/asm/qspinlock_paravirt.h > new file mode 100644 > index 000000000000..9681e851f69d > --- /dev/null > +++ b/arch/riscv/include/asm/qspinlock_paravirt.h > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (c), 2023 Alibaba Cloud > + * Authors: > + * Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > + */ > + > +#ifndef _ASM_RISCV_QSPINLOCK_PARAVIRT_H > +#define _ASM_RISCV_QSPINLOCK_PARAVIRT_H > + > +void pv_wait(u8 *ptr, u8 val); > +void pv_kick(int cpu); > + > +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); > +DECLARE_STATIC_CALL(pv_queued_spin_unlock, dummy_queued_spin_unlock); > + > +void __init pv_qspinlock_init(void); > + > +static inline bool pv_is_native_spin_unlock(void) > +{ > + return false; > +} > + > +void __pv_queued_spin_unlock(struct qspinlock *lock); > + > +#endif /* _ASM_RISCV_QSPINLOCK_PARAVIRT_H */ > diff --git a/arch/riscv/include/asm/spinlock.h b/arch/riscv/include/asm/spinlock.h > index 6b38d6616f14..ed4253f491fe 100644 > --- a/arch/riscv/include/asm/spinlock.h > +++ b/arch/riscv/include/asm/spinlock.h > @@ -39,7 +39,7 @@ static inline bool virt_spin_lock(struct qspinlock *lock) > #undef arch_spin_trylock > #undef arch_spin_unlock > > -#include <asm-generic/qspinlock.h> > +#include <asm/qspinlock.h> > #include <linux/jump_label.h> > > #undef arch_spin_is_locked > diff --git a/arch/riscv/kernel/qspinlock_paravirt.c b/arch/riscv/kernel/qspinlock_paravirt.c > new file mode 100644 > index 000000000000..85ff5a3ec234 > --- /dev/null > +++ b/arch/riscv/kernel/qspinlock_paravirt.c > @@ -0,0 +1,57 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c), 2023 Alibaba Cloud > + * Authors: > + * Guo Ren <guoren@xxxxxxxxxxxxxxxxx> > + */ > + > +#include <linux/static_call.h> > +#include <asm/qspinlock_paravirt.h> > +#include <asm/sbi.h> > + > +void pv_kick(int cpu) > +{ > + return; > +} > + > +void pv_wait(u8 *ptr, u8 val) > +{ > + unsigned long flags; > + > + if (in_nmi()) > + return; > + > + local_irq_save(flags); > + if (READ_ONCE(*ptr) != val) > + goto out; > + > + /* wait_for_interrupt(); */ > +out: > + local_irq_restore(flags); > +} > + > +static void native_queued_spin_unlock(struct qspinlock *lock) > +{ > + smp_store_release(&lock->locked, 0); > +} > + > +DEFINE_STATIC_CALL(pv_queued_spin_lock_slowpath, native_queued_spin_lock_slowpath); > +EXPORT_STATIC_CALL(pv_queued_spin_lock_slowpath); > + > +DEFINE_STATIC_CALL(pv_queued_spin_unlock, native_queued_spin_unlock); > +EXPORT_STATIC_CALL(pv_queued_spin_unlock); > + > +void __init pv_qspinlock_init(void) > +{ > + if (num_possible_cpus() == 1) > + return; > + > + if(sbi_get_firmware_id() != SBI_EXT_BASE_IMPL_ID_KVM) Checks like this seem to be very common on this patchset. For someone not much familiar with this, it can be hard to understand. I mean, on patch 8/17 you introduce those IDs, which look to be incremental ( ID == N includes stuff from ID < N ), but I am not sure as I couln't find much documentation on that. Then above you test for the id being different than SBI_EXT_BASE_IMPL_ID_KVM, but if they are actually incremental and a new version lands, the new version will also return early because it passes the test. I am no sure if above is right, but it's all I could understand without documentation. Well, my point is: this seems hard to understand & review, so it would be nice to have a macro like this to be used instead: #define sbi_fw_implements_kvm() \ (sbi_get_firmware_id() >= SBI_EXT_BASE_IMPL_ID_KVM) if(!sbi_fw_implements_kvm()) return; What do you think? Other than that, LGTM. Thanks! Leo > + return; > + > + pr_info("PV qspinlocks enabled\n"); > + __pv_init_lock_hash(); > + > + static_call_update(pv_queued_spin_lock_slowpath, __pv_queued_spin_lock_slowpath); > + static_call_update(pv_queued_spin_unlock, __pv_queued_spin_unlock); > +} > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index c57d15b05160..88690751f2ee 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -321,6 +321,10 @@ static void __init riscv_spinlock_init(void) > #ifdef CONFIG_QUEUED_SPINLOCKS > virt_spin_lock_init(); > #endif > + > +#ifdef CONFIG_PARAVIRT_SPINLOCKS > + pv_qspinlock_init(); > +#endif > } > > extern void __init init_rt_signal_env(void); > -- > 2.36.1 >