diff mbox series

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

Message ID 20230925133859.1735879-3-apatel@ventanamicro.com (mailing list archive)
State New
Headers show
Series KVM RISC-V Conditional Operations | expand

Commit Message

Anup Patel Sept. 25, 2023, 1:38 p.m. UTC 
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(+)

Comments

Charlie Jenkins Sept. 25, 2023, 5:48 p.m. UTC | #1 
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/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@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
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.

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.

What are your thoughts?

- Charlie

[1] https://lore.kernel.org/lkml/20230705-thead_vendor_extensions-v1-2-ad6915349c4d@rivosinc.com/
Charlie Jenkins Sept. 25, 2023, 6:12 p.m. UTC | #2 
On Mon, Sep 25, 2023 at 10:48:11AM -0700, Charlie Jenkins 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/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@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
> 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.
> 
> 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.
> 
> What are your thoughts?
> 
> - Charlie
> 
> [1] https://lore.kernel.org/lkml/20230705-thead_vendor_extensions-v1-2-ad6915349c4d@rivosinc.com/
> 

I guess I don't need to revive the patch, you could just take the code
and update it for xventanacondops.

- Charlie
Anup Patel Sept. 26, 2023, 4:08 a.m. UTC | #3 
On Mon, Sep 25, 2023 at 11:18 PM Charlie Jenkins <charlie@rivosinc.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/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@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
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
Anup Patel Sept. 26, 2023, 4:14 a.m. UTC | #4 
On Tue, Sep 26, 2023 at 9:38 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Mon, Sep 25, 2023 at 11:18 PM Charlie Jenkins <charlie@rivosinc.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/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@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
Charlie Jenkins Sept. 27, 2023, 2:13 a.m. UTC | #5 
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@ventanamicro.com> wrote:
> >
> > On Mon, Sep 25, 2023 at 11:18 PM Charlie Jenkins <charlie@rivosinc.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/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@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.
> >

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@rivosinc.com/
> > >
> >
> > Regards,
> > Anup
> 
> Regards,
> Anup

- Charlie
diff mbox series

Patch

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);