Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20230925133859.1735879-1-apatel@ventanamicro.com>
 <20230925133859.1735879-3-apatel@ventanamicro.com> <ZRHH25IyJJLWSolC@ghost> <CAK9=C2UoKxM+wknB4n8=okyXCCE6t0Vvz4jU_tBW6DMm6Vb3DA@mail.gmail.com>
In-Reply-To: <CAK9=C2UoKxM+wknB4n8=okyXCCE6t0Vvz4jU_tBW6DMm6Vb3DA@mail.gmail.com>
From:   Anup Patel <apatel@ventanamicro.com>
Date:   Tue, 26 Sep 2023 09:44:38 +0530
Message-ID: <CAK9=C2X9FpLTW4mDTNUWkoRLAXZonPzhrsOD5xrCfrqKSbaLhg@mail.gmail.com>
Subject: Re: [PATCH v2 2/9] RISC-V: Detect XVentanaCondOps from ISA string
To:     Charlie Jenkins <charlie@rivosinc.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Atish Patra <atishp@atishpatra.org>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Conor Dooley <conor@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Shuah Khan <shuah@kernel.org>,
        Andrew Jones <ajones@ventanamicro.com>,
        Mayuresh Chitale <mchitale@ventanamicro.com>,
        devicetree@vger.kernel.org, kvm@vger.kernel.org,
        kvm-riscv@lists.infradead.org, linux-riscv@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Sep 26, 2023 at 9:38=E2=80=AFAM Anup Patel <apatel@ventanamicro.com=
> wrote:
>
> On Mon, Sep 25, 2023 at 11:18=E2=80=AFPM Charlie Jenkins <charlie@rivosin=
c.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@ventanamicro.com>
> > > Reviewed-by: Andrew Jones <ajones@ventanamicro.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/cpufe=
ature.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[] =
=3D {
> > >       __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_XVENTANACON=
DOPS),
> > >  };
> > >
> > >  const size_t riscv_isa_ext_count =3D ARRAY_SIZE(riscv_isa_ext);
> > > --
> > > 2.34.1
> > >
> > >
> > > _______________________________________________
> > > linux-riscv mailing list
> > > linux-riscv@lists.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