Re: [PATCH V11 00/17] riscv: Add Native/Paravirt qspinlock support

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2861 bytes --]

On Sun, Sep 10, 2023 at 05:49:13PM +0800, Guo Ren wrote:
> On Sun, Sep 10, 2023 at 5:32 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Sun, Sep 10, 2023 at 05:16:46PM +0800, Guo Ren wrote:
> > > On Sun, Sep 10, 2023 at 4:58 PM Conor Dooley <conor@kernel.org> wrote:
> > > >
> > > > On Sun, Sep 10, 2023 at 04:28:54AM -0400, guoren@kernel.org wrote:
> > > >
> > > > > Changlog:
> > > > > V11:
> > > > >  - Based on Leonardo Bras's cmpxchg_small patches v5.
> > > > >  - Based on Guo Ren's Optimize arch_spin_value_unlocked patch v3.
> > > > >  - Remove abusing alternative framework and use jump_label instead.
> > > >
> > > > btw, I didn't say that using alternatives was the problem, it was
> > > > abusing the errata framework to perform feature detection that I had
> > > > a problem with. That's not changed in v11.
> > > I've removed errata feature detection. The only related patches are:
> > >  - riscv: qspinlock: errata: Add ERRATA_THEAD_WRITE_ONCE fixup
> > >  - riscv: qspinlock: errata: Enable qspinlock for T-HEAD processors
> > >
> > > Which one is your concern? Could you reply on the exact patch thread? Thx.
> >
> > riscv: qspinlock: errata: Enable qspinlock for T-HEAD processors
> >
> > Please go back and re-read the comments I left on v11 about using the
> > errata code for feature detection.
> >
> > > > A stronger forward progress guarantee is not an erratum, AFAICT.
> >
> > > Sorry, there is no erratum of "stronger forward progress guarantee" in the V11.
> >
> > "riscv: qspinlock: errata: Enable qspinlock for T-HEAD processors" still
> > uses the errata framework to detect the presence of the stronger forward
> > progress guarantee in v11.
> Oh, thx for pointing it out. I could replace it with this:
> 
> diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> index 88690751f2ee..4be92766d3e3 100644
> --- a/arch/riscv/kernel/setup.c
> +++ b/arch/riscv/kernel/setup.c
> @@ -310,7 +310,8 @@ static void __init riscv_spinlock_init(void)
>  {
>  #ifdef CONFIG_RISCV_COMBO_SPINLOCKS
>         if (!enable_qspinlock_key &&
> -           (sbi_get_firmware_id() != SBI_EXT_BASE_IMPL_ID_KVM)) {
> +           (sbi_get_firmware_id() != SBI_EXT_BASE_IMPL_ID_KVM) &&
> +           (sbi_get_mvendorid() != THEAD_VENDOR_ID)) {
>                 static_branch_disable(&combo_qspinlock_key);
>                 pr_info("Ticket spinlock: enabled\n");
>         } else {

As I said on v11, I am opposed to feature probing using mvendorid & Co,
partially due to the exact sort of check here to see if the kernel is
running as a KVM guest. IMO, whether a platform has this stronger
guarantee needs to be communicated by firmware, using ACPI or DT.
I made some comments on v11, referring similar discussion about the
thead vector stuff. Please go take a look at that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]