Re: [PATCH] riscv: Add riscv_force_qspinlock for early_param

[Alexandre Ghiti]

Hi Guo,

First sorry I did not follow up on this patch.

On 22/12/2024 05:03, Guo Ren wrote:
> On Mon, Dec 16, 2024 at 5:15 PM Alexandre Ghiti <alex@xxxxxxxx> wrote:
> > Hi Guo,
> >
> > On 14/12/2024 05:35, guoren@xxxxxxxxxx wrote:
> > > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > >
> > > When CONFIG_RISCV_COMBO_SPINLOCKS is enabled, permit qspinlock
> > > force enabled. See the Kconfig entry for RISCV_COMBO_SPINLOCKS.
> > >
> > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Guo Ren <guoren@xxxxxxxxxx>
> > > ---
> > >    Documentation/admin-guide/kernel-parameters.txt |  5 +++++
> > >    arch/riscv/kernel/setup.c                       | 13 ++++++++++++-
> > >    2 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > > index 3872bc6ec49d..43d0df2922b2 100644
> > > --- a/Documentation/admin-guide/kernel-parameters.txt
> > > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > > @@ -5887,6 +5887,11 @@
> > >                        [KNL] Disable ring 3 MONITOR/MWAIT feature on supported
> > >                        CPUs.
> > >
> > > +     riscv_force_qspinlock [RISCV, EARLY]
> > > +                     When CONFIG_RISCV_COMBO_SPINLOCKS is enabled, permit
> > > +                     qspinlock force enabled. See the Kconfig entry for
> > > +                     RISCV_COMBO_SPINLOCKS.
> > > +
> > >        riscv_isa_fallback [RISCV,EARLY]
> > >                        When CONFIG_RISCV_ISA_FALLBACK is not enabled, permit
> > >                        falling back to detecting extension support by parsing
> > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
> > > index 45010e71df86..74b13bc64c9c 100644
> > > --- a/arch/riscv/kernel/setup.c
> > > +++ b/arch/riscv/kernel/setup.c
> > > @@ -247,6 +247,15 @@ static void __init parse_dtb(void)
> > >    #if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
> > >    DEFINE_STATIC_KEY_TRUE(qspinlock_key);
> > >    EXPORT_SYMBOL(qspinlock_key);
> > > +
> > > +static bool force_qspinlock;
> > > +
> > > +static int __init riscv_force_qspinlock(char *p)
> > > +{
> > > +     force_qspinlock = true;
> > > +     return 0;
> > > +}
> > > +early_param("riscv_force_qspinlock", riscv_force_qspinlock);
> > >    #endif
> > >
> > >    static void __init riscv_spinlock_init(void)
> > > @@ -267,7 +276,9 @@ static void __init riscv_spinlock_init(void)
> > >                using_ext = "using Ziccrse";
> > >        }
> > >    #if defined(CONFIG_RISCV_COMBO_SPINLOCKS)
> > > -     else {
> > > +     else if (force_qspinlock) {
> > > +             using_ext = "force";
> > > +     } else {
> > >                static_branch_disable(&qspinlock_key);
> > >                pr_info("Ticket spinlock: enabled\n");
> > >                return;
> >
> > What's the use case for this early param? To me that implies that a
> > platform may have another extension which would allow the usage of
> We want to use it for sg2042 & th1520. No new extension was
> introduced, and some micro-architecture could give LR/SC
> implementation a forward progress guarantee more than the spec
> required.


But then those platforms support Ziccrse which is enough to use 
qspinlocks right?

Thanks,

Alex


> > qspinlock, so why not listing it in riscv_spinlock_init() instead?
> List all platforms in riscv_spinlock_init is noisy, maybe give a
> qspinlock param in cmdline, and they could put it in their boot args.
>
> > Thanks,
> >
> > Alex