Re: [PATCH v2 2/9] RISC-V: Detect XVentanaCondOps from ISA string

[Charlie Jenkins]

On Tue, Sep 26, 2023 at 09:44:38AM +0530, Anup Patel wrote:
> On Tue, Sep 26, 2023 at 9:38 AM Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote:
> >
> > On Mon, Sep 25, 2023 at 11:18 PM Charlie Jenkins <charlie@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > > > Reviewed-by: Andrew Jones <ajones@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxxxxxxxxxxx
> > > > 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.
> >

I was conflating hwprobe with hwcap when I was thinking about this.
However, I think it might still be beneficial to split out the vendor
extensions. It is possible for vendors to implement each other's
extensions but I don't expect that to be the average case. Because I do
not expect this to be the average case, riscv_isa_ext becomes needlessly
large as it has to contain the extensions of every vendor.

> > >
> > > 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@xxxxxxxxxxxx/
> > >
> >
> > Regards,
> > Anup
> 
> Regards,
> Anup

- Charlie