Message-ID: <CAK9=C2X9FpLTW4mDTNUWkoRLAXZonPzhrsOD5xrCfrqKSbaLhg@mail.gmail.com>
Date:   Tue, 26 Sep 2023 09:44:38 +0530
From:   Anup Patel <apatel@...tanamicro.com>
To:     Charlie Jenkins <charlie@...osinc.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Atish Patra <atishp@...shpatra.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Conor Dooley <conor@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Shuah Khan <shuah@...nel.org>,
        Andrew Jones <ajones@...tanamicro.com>,
        Mayuresh Chitale <mchitale@...tanamicro.com>,
        devicetree@...r.kernel.org, kvm@...r.kernel.org,
        kvm-riscv@...ts.infradead.org, linux-riscv@...ts.infradead.org,
        linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v2 2/9] RISC-V: Detect XVentanaCondOps from ISA string

On Tue, Sep 26, 2023 at 9:38 AM Anup Patel <apatel@...tanamicro.com> wrote:
>
> On Mon, Sep 25, 2023 at 11:18 PM Charlie Jenkins <charlie@...osinc.com> wrote:
> >
> > On Mon, Sep 25, 2023 at 07:08:52PM +0530, Anup Patel wrote:
> > > The Veyron-V1 CPU supports custom conditional arithmetic and
> > > conditional-select/move operations referred to as XVentanaCondOps
> > > extension. In fact, QEMU RISC-V also has support for emulating
> > > XVentanaCondOps extension.
> > >
> > > Let us detect XVentanaCondOps extension from ISA string available
> > > through DT or ACPI.
> > >
> > > Signed-off-by: Anup Patel <apatel@...tanamicro.com>
> > > Reviewed-by: Andrew Jones <ajones@...tanamicro.com>
> > > ---
> > >  arch/riscv/include/asm/hwcap.h | 1 +
> > >  arch/riscv/kernel/cpufeature.c | 1 +
> > >  2 files changed, 2 insertions(+)
> > >
> > > diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> > > index 0f520f7d058a..b7efe9e2fa89 100644
> > > --- a/arch/riscv/include/asm/hwcap.h
> > > +++ b/arch/riscv/include/asm/hwcap.h
> > > @@ -59,6 +59,7 @@
> > >  #define RISCV_ISA_EXT_ZIFENCEI               41
> > >  #define RISCV_ISA_EXT_ZIHPM          42
> > >  #define RISCV_ISA_EXT_SMSTATEEN              43
> > > +#define RISCV_ISA_EXT_XVENTANACONDOPS        44
> > >
> > >  #define RISCV_ISA_EXT_MAX            64
> > >
> > > diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> > > index 3755a8c2a9de..3a31d34fe709 100644
> > > --- a/arch/riscv/kernel/cpufeature.c
> > > +++ b/arch/riscv/kernel/cpufeature.c
> > > @@ -182,6 +182,7 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
> > >       __RISCV_ISA_EXT_DATA(svinval, RISCV_ISA_EXT_SVINVAL),
> > >       __RISCV_ISA_EXT_DATA(svnapot, RISCV_ISA_EXT_SVNAPOT),
> > >       __RISCV_ISA_EXT_DATA(svpbmt, RISCV_ISA_EXT_SVPBMT),
> > > +     __RISCV_ISA_EXT_DATA(xventanacondops, RISCV_ISA_EXT_XVENTANACONDOPS),
> > >  };
> > >
> > >  const size_t riscv_isa_ext_count = ARRAY_SIZE(riscv_isa_ext);
> > > --
> > > 2.34.1
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@...ts.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-riscv
> >
> > I worry about storing vendor extensions in this file. Because vendor
> > extensions are not standardized, they can only be expected to have the
> > desired behavior on hardware with the appropriate vendor id. A couple
>
> Assuming that a vendor extension is only available on hardware with
> appropriate vendor id is not correct because:
> 1) vendor A can allow vendor B to implement a custom extension
>     defined by vendor B

Typo correction: "vendor A can allow vendor B to implement a custom
extension defined by vendor A"

> 2) vendor A and vendor B can jointly develop a RISC-V CPU where
>     both vendors integrate their custom extensions.
>
> It is best to identify a vendor extension independently with a
> "X<vendor_name><extension_name>" string to keep it simple
> and scalable.
>
> Along these lines, each T-Head custom extension should have a
> "XThead<xyz>" name associated with it.
>
> > months ago I sent a patch to address this by handling vector extensions
> > independently for each vendor [1]. I dropped the patch because it
> > relied upon Heiko's T-Head vector extension support that he stopped
> > working on. However, I can revive this patch so you can build off of it.
>
> At least, the conditional operations don't need a hwprobe interface
> because an application is either compiled with or without conditional
> operations. In other words, effective use of conditional operation is
> only possible if compiler generates these instructions based on
> code patterns.
>
> >
> > This scheme has the added benefit that vendors do not have to worry
> > about conficting extensions, and the kernel does not have to act as a
> > key registry for vendors.
>
> How can vendor extensions conflict if they all follow the
> "X<vendor_name><extension_name>" naming scheme ?
>
> >
> > What are your thoughts?
> >
> > - Charlie
> >
> > [1] https://lore.kernel.org/lkml/20230705-thead_vendor_extensions-v1-2-ad6915349c4d@rivosinc.com/
> >
>
> Regards,
> Anup

Regards,
Anup

Powered by blists - more mailing lists