On Mon, Sep 11, 2023 at 8:53 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> wrote:
>
> On Mon, Sep 11, 2023 at 11:36:27AM +0800, Guo Ren wrote:
> > On Mon, Sep 11, 2023 at 3:45 AM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > >
> > > 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@xxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Sun, Sep 10, 2023 at 04:28:54AM -0400, guoren@xxxxxxxxxx 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
>
> > KVM can't use any fairness lock, so forcing it using a Test-Set lock
> > or paravirt qspinlock is the right way. KVM is not a vendor platform.
>
> My point is that KVM should be telling the guest what additional features
> it is capable of using, rather than the kernel making some assumptions
> based on$vendorid etc that are invalid when the kernel is running as a
> KVM guest.
> If the mvendorid etc related assumptions were dropped, the kernel would
> then default away from your qspinlock & there'd not be a need to
> special-case KVM AFAICT.
>
> > > 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.
> > I prefer forcing T-HEAD processors using qspinlock, but if all people
> > thought it must be in the ACPI or DT, I would compromise. Then, I
> > would delete the qspinlock cmdline param patch and move it into DT.
> >
> > By the way, what's the kind of DT format? How about:
>
> I added the new "riscv,isa-extensions" property in part to make
> communicating vendor extensions like this easier. Please try to use
> that. "qspinlock" is software configuration though, the vendor extension
> should focus on the guarantee of strong forward progress, since that is
> the non-standard aspect of your IP.
The qspinlock contains three paths:
- Native qspinlock, this is your strong forward progress.
- virt_spin_lock, for KVM guest when paravirt qspinlock disabled.
https://lore.kernel.org/linux-riscv/20230910082911.3378782-9-guoren@xxxxxxxxxx/
- paravirt qspinlock, for KVM guest
So, we need a software configuration here, "riscv,isa-extensions" is
all about vendor extension.
>
> A commandline property may still be desirable, to control the locking
> method used, since the DT should be a description of the hardware, not
> for configuring software policy in your operating system.
Okay, I would keep the cmdline property.
>
> Thanks,
> Conor.
>
> > cpus {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > + qspinlock;
> > cpu0: cpu@0 {
> > compatible = "sifive,bullet0", "riscv";
> > device_type = "cpu";
> > i-cache-block-size = <64>;
> > i-cache-sets = <128>;
> >
> > --
> > Best Regards
> > Guo Ren
--
Best Regards
Guo Ren
