[PATCH 0/3] target/riscv: Add support for 'B' extension

[PATCH 0/3] target/riscv: Add support for 'B' extension

[Rob Bradford]

Add support for the new (fast track) 'B' extension [1] this extension
uses the misa.B bit to indicate that the Zba, Zbb and Zbs extensions are
present.

Since this extension is not yet frozen it is exposed via the 'x-b' cpu
option. The validation logic is based on the new approach taken for the
'G' extension. [2]

The specification handles backward compatability: The misa.B bit may be
set if Zba, Zbb and Zbs are present but in order to not break existing
systems the bit is not required to be set if they are present. As such
even though Zba, Zbb and Zbs default to on in QEMU this extension is not
enabled by default in any cpu other than the 'max' variant.

Cheers,

Rob

[1] - https://github.com/riscv/riscv-b
[2] - https://patchew.org/QEMU/20231218125334.37184-1-dbarboza@ventanamicro.com/20231218125334.37184-16-dbarboza@ventanamicro.com/

Rob Bradford (3):
  target/riscv: Add infrastructure for 'B' MISA extension
  target/riscv: Add step to validate 'B' extension
  target/riscv: Enable 'B' extension on max CPU type

 target/riscv/cpu.c         |  5 +++--
 target/riscv/cpu.h         |  1 +
 target/riscv/tcg/tcg-cpu.c | 37 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 40 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH 1/3] target/riscv: Add infrastructure for 'B' MISA extension

[Rob Bradford]

Add the infrastructure for the 'B' extension which is the union of the
Zba, Zbb and Zbs instructions.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
---
 target/riscv/cpu.c         | 5 +++--
 target/riscv/cpu.h         | 1 +
 target/riscv/tcg/tcg-cpu.c | 1 +
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b07a76ef6b..22f8e527ff 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -38,9 +38,9 @@
 #include "tcg/tcg.h"
 
 /* RISC-V CPU definitions */
-static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
+static const char riscv_single_letter_exts[] = "IEMAFDQCBPVH";
 const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
-                              RVC, RVS, RVU, RVH, RVJ, RVG, 0};
+                              RVC, RVS, RVU, RVH, RVJ, RVG, RVB, 0};
 
 /*
  * From vector_helper.c
@@ -1251,6 +1251,7 @@ static const MISAExtInfo misa_ext_info_arr[] = {
     MISA_EXT_INFO(RVJ, "x-j", "Dynamic translated languages"),
     MISA_EXT_INFO(RVV, "v", "Vector operations"),
     MISA_EXT_INFO(RVG, "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
+    MISA_EXT_INFO(RVB, "x-b", "Bit manipulation (Zba_Zbb_Zbs)")
 };
 
 static int riscv_validate_misa_info_idx(uint32_t bit)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 2725528bb5..756a345513 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -69,6 +69,7 @@ typedef struct CPUArchState CPURISCVState;
 #define RVH RV('H')
 #define RVJ RV('J')
 #define RVG RV('G')
+#define RVB RV('B')
 
 extern const uint32_t misa_bits[];
 const char *riscv_get_misa_ext_name(uint32_t bit);
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index 8a35683a34..fda54671d5 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -791,6 +791,7 @@ static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
     MISA_CFG(RVJ, false),
     MISA_CFG(RVV, false),
     MISA_CFG(RVG, false),
+    MISA_CFG(RVB, false)
 };
 
 /*
-- 
2.43.0

Re: [PATCH 1/3] target/riscv: Add infrastructure for 'B' MISA extension

[Daniel Henrique Barboza]

On 1/9/24 14:07, Rob Bradford wrote:
> Add the infrastructure for the 'B' extension which is the union of the
> Zba, Zbb and Zbs instructions.
> 
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>   target/riscv/cpu.c         | 5 +++--
>   target/riscv/cpu.h         | 1 +
>   target/riscv/tcg/tcg-cpu.c | 1 +
>   3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b07a76ef6b..22f8e527ff 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -38,9 +38,9 @@
>   #include "tcg/tcg.h"
>   
>   /* RISC-V CPU definitions */
> -static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
> +static const char riscv_single_letter_exts[] = "IEMAFDQCBPVH";
>   const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
> -                              RVC, RVS, RVU, RVH, RVJ, RVG, 0};
> +                              RVC, RVS, RVU, RVH, RVJ, RVG, RVB, 0};
>   
>   /*
>    * From vector_helper.c
> @@ -1251,6 +1251,7 @@ static const MISAExtInfo misa_ext_info_arr[] = {
>       MISA_EXT_INFO(RVJ, "x-j", "Dynamic translated languages"),
>       MISA_EXT_INFO(RVV, "v", "Vector operations"),
>       MISA_EXT_INFO(RVG, "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
> +    MISA_EXT_INFO(RVB, "x-b", "Bit manipulation (Zba_Zbb_Zbs)")
>   };
>   
>   static int riscv_validate_misa_info_idx(uint32_t bit)
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 2725528bb5..756a345513 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -69,6 +69,7 @@ typedef struct CPUArchState CPURISCVState;
>   #define RVH RV('H')
>   #define RVJ RV('J')
>   #define RVG RV('G')
> +#define RVB RV('B')
>   
>   extern const uint32_t misa_bits[];
>   const char *riscv_get_misa_ext_name(uint32_t bit);
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 8a35683a34..fda54671d5 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -791,6 +791,7 @@ static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>       MISA_CFG(RVJ, false),
>       MISA_CFG(RVV, false),
>       MISA_CFG(RVG, false),
> +    MISA_CFG(RVB, false)

Please add a comma at the end:


> +    MISA_CFG(RVB, false),

This way, when a new bit is added, the change is limited to the new entry.


With this nit fixed:


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>



>   };
>   
>   /*

Re: [PATCH 1/3] target/riscv: Add infrastructure for 'B' MISA extension

[Andrew Jones]

On Tue, Jan 09, 2024 at 05:07:35PM +0000, Rob Bradford wrote:
> Add the infrastructure for the 'B' extension which is the union of the
> Zba, Zbb and Zbs instructions.
> 
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>  target/riscv/cpu.c         | 5 +++--
>  target/riscv/cpu.h         | 1 +
>  target/riscv/tcg/tcg-cpu.c | 1 +
>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b07a76ef6b..22f8e527ff 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -38,9 +38,9 @@
>  #include "tcg/tcg.h"
>  
>  /* RISC-V CPU definitions */
> -static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
> +static const char riscv_single_letter_exts[] = "IEMAFDQCBPVH";

Is there a corresponding proposed change to table 29.1 of the nonpriv spec
which states B comes after C and before P? If so, can you provide a link
to it? Otherwise, how do we know that?

Thanks,
drew

>  const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
> -                              RVC, RVS, RVU, RVH, RVJ, RVG, 0};
> +                              RVC, RVS, RVU, RVH, RVJ, RVG, RVB, 0};
>  
>  /*
>   * From vector_helper.c
> @@ -1251,6 +1251,7 @@ static const MISAExtInfo misa_ext_info_arr[] = {
>      MISA_EXT_INFO(RVJ, "x-j", "Dynamic translated languages"),
>      MISA_EXT_INFO(RVV, "v", "Vector operations"),
>      MISA_EXT_INFO(RVG, "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
> +    MISA_EXT_INFO(RVB, "x-b", "Bit manipulation (Zba_Zbb_Zbs)")
>  };
>  
>  static int riscv_validate_misa_info_idx(uint32_t bit)
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 2725528bb5..756a345513 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -69,6 +69,7 @@ typedef struct CPUArchState CPURISCVState;
>  #define RVH RV('H')
>  #define RVJ RV('J')
>  #define RVG RV('G')
> +#define RVB RV('B')
>  
>  extern const uint32_t misa_bits[];
>  const char *riscv_get_misa_ext_name(uint32_t bit);
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 8a35683a34..fda54671d5 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -791,6 +791,7 @@ static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>      MISA_CFG(RVJ, false),
>      MISA_CFG(RVV, false),
>      MISA_CFG(RVG, false),
> +    MISA_CFG(RVB, false)
>  };
>  
>  /*
> -- 
> 2.43.0
> 
> 

Re: Re: [PATCH 1/3] target/riscv: Add infrastructure for 'B' MISA extension

[Andrew Jones]

On Thu, Jan 11, 2024 at 02:07:34PM +0100, Andrew Jones wrote:
> On Tue, Jan 09, 2024 at 05:07:35PM +0000, Rob Bradford wrote:
> > Add the infrastructure for the 'B' extension which is the union of the
> > Zba, Zbb and Zbs instructions.
> > 
> > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > ---
> >  target/riscv/cpu.c         | 5 +++--
> >  target/riscv/cpu.h         | 1 +
> >  target/riscv/tcg/tcg-cpu.c | 1 +
> >  3 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > index b07a76ef6b..22f8e527ff 100644
> > --- a/target/riscv/cpu.c
> > +++ b/target/riscv/cpu.c
> > @@ -38,9 +38,9 @@
> >  #include "tcg/tcg.h"
> >  
> >  /* RISC-V CPU definitions */
> > -static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
> > +static const char riscv_single_letter_exts[] = "IEMAFDQCBPVH";
> 
> Is there a corresponding proposed change to table 29.1 of the nonpriv spec
> which states B comes after C and before P? If so, can you provide a link
> to it? Otherwise, how do we know that?

Oh, I see. The unpriv spec B chapter comes after the C chapter (and before
J, P, ...). I still wonder if we'll have a 29.1 table update with the
ratification of this extension though.

Thanks,
drew

Re: Re: [PATCH 1/3] target/riscv: Add infrastructure for 'B' MISA extension

[Rob Bradford]

+ Ved

On Thu, 2024-01-11 at 14:14 +0100, Andrew Jones wrote:
> On Thu, Jan 11, 2024 at 02:07:34PM +0100, Andrew Jones wrote:
> > On Tue, Jan 09, 2024 at 05:07:35PM +0000, Rob Bradford wrote:
> > > Add the infrastructure for the 'B' extension which is the union
> > > of the
> > > Zba, Zbb and Zbs instructions.
> > > 
> > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > ---
> > >  target/riscv/cpu.c         | 5 +++--
> > >  target/riscv/cpu.h         | 1 +
> > >  target/riscv/tcg/tcg-cpu.c | 1 +
> > >  3 files changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > index b07a76ef6b..22f8e527ff 100644
> > > --- a/target/riscv/cpu.c
> > > +++ b/target/riscv/cpu.c
> > > @@ -38,9 +38,9 @@
> > >  #include "tcg/tcg.h"
> > >  
> > >  /* RISC-V CPU definitions */
> > > -static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
> > > +static const char riscv_single_letter_exts[] = "IEMAFDQCBPVH";
> > 
> > Is there a corresponding proposed change to table 29.1 of the
> > nonpriv spec
> > which states B comes after C and before P? If so, can you provide a
> > link
> > to it? Otherwise, how do we know that?
> 
> Oh, I see. The unpriv spec B chapter comes after the C chapter (and
> before
> J, P, ...). I still wonder if we'll have a 29.1 table update with the
> ratification of this extension though.
> 
> 

I agree it's a bit confusing - but the order is established by the
table in the unprivileged spec and the table explanation also makes
this clear.

"""
Table 27.1: Standard ISA extension names. The table also defines the
canonical order in which
extension names must appear in the name string, with top-to-bottom in
table indicating first-to-last
in the name string, e.g., RV32IMACV is legal, whereas RV32IMAVC is not.
"""

The proposed B specification does not make any remarks about the
ordering in the ISA definition string. [1] I would worry there would be
a lot of software churn if this ordering were to be changed.

Cheers,

Rob

> Thanks,
> drew

[1] - https://github.com/riscv/riscv-b

Re: Re: Re: [PATCH 1/3] target/riscv: Add infrastructure for 'B' MISA extension

[Andrew Jones]

On Thu, Jan 11, 2024 at 03:17:25PM +0000, Rob Bradford wrote:
> + Ved
> 
> On Thu, 2024-01-11 at 14:14 +0100, Andrew Jones wrote:
> > On Thu, Jan 11, 2024 at 02:07:34PM +0100, Andrew Jones wrote:
> > > On Tue, Jan 09, 2024 at 05:07:35PM +0000, Rob Bradford wrote:
> > > > Add the infrastructure for the 'B' extension which is the union
> > > > of the
> > > > Zba, Zbb and Zbs instructions.
> > > > 
> > > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > > ---
> > > >  target/riscv/cpu.c         | 5 +++--
> > > >  target/riscv/cpu.h         | 1 +
> > > >  target/riscv/tcg/tcg-cpu.c | 1 +
> > > >  3 files changed, 5 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > index b07a76ef6b..22f8e527ff 100644
> > > > --- a/target/riscv/cpu.c
> > > > +++ b/target/riscv/cpu.c
> > > > @@ -38,9 +38,9 @@
> > > >  #include "tcg/tcg.h"
> > > >  
> > > >  /* RISC-V CPU definitions */
> > > > -static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
> > > > +static const char riscv_single_letter_exts[] = "IEMAFDQCBPVH";
> > > 
> > > Is there a corresponding proposed change to table 29.1 of the
> > > nonpriv spec
> > > which states B comes after C and before P? If so, can you provide a
> > > link
> > > to it? Otherwise, how do we know that?
> > 
> > Oh, I see. The unpriv spec B chapter comes after the C chapter (and
> > before
> > J, P, ...). I still wonder if we'll have a 29.1 table update with the
> > ratification of this extension though.
> > 
> > 
> 
> I agree it's a bit confusing - but the order is established by the
> table in the unprivileged spec and the table explanation also makes
> this clear.
> 
> """
> Table 27.1: Standard ISA extension names. The table also defines the
> canonical order in which
> extension names must appear in the name string, with top-to-bottom in
> table indicating first-to-last
> in the name string, e.g., RV32IMACV is legal, whereas RV32IMAVC is not.
> """

Yes, this is the table I was referring to when I referenced "table 29.1 of
the nonpriv spec". Since there's a chance I was looking at too old a spec
I've now gone straight to the source,

https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc

but I still don't see B there. Do you see B in the table you're looking
at?

> 
> The proposed B specification does not make any remarks about the
> ordering in the ISA definition string. [1] I would worry there would be
> a lot of software churn if this ordering were to be changed.

The ordering shouldn't change, but I can't see where it's documented
(beyond the B chapter coming after the C chapter).

Thanks,
drew

Re: Re: Re: [PATCH 1/3] target/riscv: Add infrastructure for 'B' MISA extension

[Rob Bradford]

On Fri, 2024-01-12 at 17:08 +0100, Andrew Jones wrote:
> On Thu, Jan 11, 2024 at 03:17:25PM +0000, Rob Bradford wrote:
> > + Ved
> > 
> > On Thu, 2024-01-11 at 14:14 +0100, Andrew Jones wrote:
> > > On Thu, Jan 11, 2024 at 02:07:34PM +0100, Andrew Jones wrote:
> > > > On Tue, Jan 09, 2024 at 05:07:35PM +0000, Rob Bradford wrote:
> > > > > Add the infrastructure for the 'B' extension which is the
> > > > > union
> > > > > of the
> > > > > Zba, Zbb and Zbs instructions.
> > > > > 
> > > > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > > > ---
> > > > >  target/riscv/cpu.c         | 5 +++--
> > > > >  target/riscv/cpu.h         | 1 +
> > > > >  target/riscv/tcg/tcg-cpu.c | 1 +
> > > > >  3 files changed, 5 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > > > > index b07a76ef6b..22f8e527ff 100644
> > > > > --- a/target/riscv/cpu.c
> > > > > +++ b/target/riscv/cpu.c
> > > > > @@ -38,9 +38,9 @@
> > > > >  #include "tcg/tcg.h"
> > > > >  
> > > > >  /* RISC-V CPU definitions */
> > > > > -static const char riscv_single_letter_exts[] =
> > > > > "IEMAFDQCPVH";
> > > > > +static const char riscv_single_letter_exts[] =
> > > > > "IEMAFDQCBPVH";
> > > > 
> > > > Is there a corresponding proposed change to table 29.1 of the
> > > > nonpriv spec
> > > > which states B comes after C and before P? If so, can you
> > > > provide a
> > > > link
> > > > to it? Otherwise, how do we know that?
> > > 
> > > Oh, I see. The unpriv spec B chapter comes after the C chapter
> > > (and
> > > before
> > > J, P, ...). I still wonder if we'll have a 29.1 table update with
> > > the
> > > ratification of this extension though.
> > > 
> > > 
> > 
> > I agree it's a bit confusing - but the order is established by the
> > table in the unprivileged spec and the table explanation also makes
> > this clear.
> > 
> > """
> > Table 27.1: Standard ISA extension names. The table also defines
> > the
> > canonical order in which
> > extension names must appear in the name string, with top-to-bottom
> > in
> > table indicating first-to-last
> > in the name string, e.g., RV32IMACV is legal, whereas RV32IMAVC is
> > not.
> > """
> 
> Yes, this is the table I was referring to when I referenced "table
> 29.1 of
> the nonpriv spec". Since there's a chance I was looking at too old a
> spec
> I've now gone straight to the source,
> 
> https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc
> 
> but I still don't see B there. Do you see B in the table you're
> looking
> at?
> 
> > 
> > The proposed B specification does not make any remarks about the
> > ordering in the ISA definition string. [1] I would worry there
> > would be
> > a lot of software churn if this ordering were to be changed.
> 
> The ordering shouldn't change, but I can't see where it's documented
> (beyond the B chapter coming after the C chapter).

I'm using table 27.1 in the published PDF which is the PDF in this
release: 
https://github.com/riscv/riscv-isa-manual/releases/tag/Ratified-IMAFDQC
It looks like it was removed in this commit (which is a set of
backports):

https://github.com/riscv/riscv-isa-manual/commit/6ecd735338241583d53396b7065eab7c9ace68aa

Cheers,

Rob
> 
> Thanks,
> drew

Re: Re: Re: [PATCH 1/3] target/riscv: Add infrastructure for 'B' MISA extension

[Ved Shanbhogue]

Rob Bradford wrote:
>I'm using table 27.1 in the published PDF which is the PDF in this
>release:
>https://github.com/riscv/riscv-isa-manual/releases/tag/Ratified-IMAFDQC
>It looks like it was removed in this commit (which is a set of
>backports):
>

We would retain the previously documented canonical order with B
between C and P and that table updated on ratification.

regards
ved

Re: [PATCH 1/3] target/riscv: Add infrastructure for 'B' MISA extension

[Andrew Jones]

On Tue, Jan 09, 2024 at 05:07:35PM +0000, Rob Bradford wrote:
> Add the infrastructure for the 'B' extension which is the union of the
> Zba, Zbb and Zbs instructions.
> 
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>  target/riscv/cpu.c         | 5 +++--
>  target/riscv/cpu.h         | 1 +
>  target/riscv/tcg/tcg-cpu.c | 1 +
>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b07a76ef6b..22f8e527ff 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -38,9 +38,9 @@
>  #include "tcg/tcg.h"
>  
>  /* RISC-V CPU definitions */
> -static const char riscv_single_letter_exts[] = "IEMAFDQCPVH";
> +static const char riscv_single_letter_exts[] = "IEMAFDQCBPVH";
>  const uint32_t misa_bits[] = {RVI, RVE, RVM, RVA, RVF, RVD, RVV,
> -                              RVC, RVS, RVU, RVH, RVJ, RVG, 0};
> +                              RVC, RVS, RVU, RVH, RVJ, RVG, RVB, 0};
>  
>  /*
>   * From vector_helper.c
> @@ -1251,6 +1251,7 @@ static const MISAExtInfo misa_ext_info_arr[] = {
>      MISA_EXT_INFO(RVJ, "x-j", "Dynamic translated languages"),
>      MISA_EXT_INFO(RVV, "v", "Vector operations"),
>      MISA_EXT_INFO(RVG, "g", "General purpose (IMAFD_Zicsr_Zifencei)"),
> +    MISA_EXT_INFO(RVB, "x-b", "Bit manipulation (Zba_Zbb_Zbs)")
>  };
>  
>  static int riscv_validate_misa_info_idx(uint32_t bit)
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 2725528bb5..756a345513 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -69,6 +69,7 @@ typedef struct CPUArchState CPURISCVState;
>  #define RVH RV('H')
>  #define RVJ RV('J')
>  #define RVG RV('G')
> +#define RVB RV('B')
>  
>  extern const uint32_t misa_bits[];
>  const char *riscv_get_misa_ext_name(uint32_t bit);
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index 8a35683a34..fda54671d5 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -791,6 +791,7 @@ static const RISCVCPUMisaExtConfig misa_ext_cfgs[] = {
>      MISA_CFG(RVJ, false),
>      MISA_CFG(RVV, false),
>      MISA_CFG(RVG, false),
> +    MISA_CFG(RVB, false)
>  };
>  
>  /*
> -- 
> 2.43.0
> 
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH 2/3] target/riscv: Add step to validate 'B' extension

[Rob Bradford]

If the B extension is enabled warn if the user has disabled any of the
required extensions that are part of the 'B' extension. Conversely
enable the extensions that make up the 'B' extension if it is enabled.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
---
 target/riscv/tcg/tcg-cpu.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index fda54671d5..f10871d352 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -273,6 +273,35 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
     }
 }
 
+static void riscv_cpu_validate_b(RISCVCPU *cpu)
+{
+    const char *warn_msg = "RVB mandates disabled extension %s";
+
+    if (!cpu->cfg.ext_zba) {
+        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zba))) {
+            cpu->cfg.ext_zba = true;
+        } else {
+            warn_report(warn_msg, "zba");
+        }
+    }
+
+    if (!cpu->cfg.ext_zbb) {
+        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zbb))) {
+            cpu->cfg.ext_zbb = true;
+        } else {
+            warn_report(warn_msg, "zbb");
+        }
+    }
+
+    if (!cpu->cfg.ext_zbs) {
+        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zbs))) {
+            cpu->cfg.ext_zbs = true;
+        } else {
+            warn_report(warn_msg, "zbs");
+        }
+    }
+}
+
 /*
  * Check consistency between chosen extensions while setting
  * cpu->cfg accordingly.
@@ -309,6 +338,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
         env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
     }
 
+    if (riscv_has_ext(env, RVB)) {
+        riscv_cpu_validate_b(cpu);
+    }
+
     if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
         error_setg(errp,
                    "I and E extensions are incompatible");
-- 
2.43.0

Re: [PATCH 2/3] target/riscv: Add step to validate 'B' extension

[Daniel Henrique Barboza]

On 1/9/24 14:07, Rob Bradford wrote:
> If the B extension is enabled warn if the user has disabled any of the
> required extensions that are part of the 'B' extension. Conversely
> enable the extensions that make up the 'B' extension if it is enabled.
> 
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---

This patch doesn't apply cleanly on current master. Which is normal, since we just
had a RISC-V PR merged.

I'm afraid you'll need to resend the series rebased on top of master (since it's newer
than Alistair's riscv-to-apply.next now).


>   target/riscv/tcg/tcg-cpu.c | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 

I fixed the conflicts and applied the patch and it works for me. So:


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>



> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index fda54671d5..f10871d352 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -273,6 +273,35 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>       }
>   }
>   
> +static void riscv_cpu_validate_b(RISCVCPU *cpu)
> +{
> +    const char *warn_msg = "RVB mandates disabled extension %s";
> +
> +    if (!cpu->cfg.ext_zba) {
> +        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zba))) {
> +            cpu->cfg.ext_zba = true;
> +        } else {
> +            warn_report(warn_msg, "zba");
> +        }
> +    }
> +
> +    if (!cpu->cfg.ext_zbb) {
> +        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zbb))) {
> +            cpu->cfg.ext_zbb = true;
> +        } else {
> +            warn_report(warn_msg, "zbb");
> +        }
> +    }
> +
> +    if (!cpu->cfg.ext_zbs) {
> +        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zbs))) {
> +            cpu->cfg.ext_zbs = true;
> +        } else {
> +            warn_report(warn_msg, "zbs");
> +        }
> +    }
> +}
> +
>   /*
>    * Check consistency between chosen extensions while setting
>    * cpu->cfg accordingly.
> @@ -309,6 +338,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>           env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>       }
>   
> +    if (riscv_has_ext(env, RVB)) {
> +        riscv_cpu_validate_b(cpu);
> +    }
> +
>       if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
>           error_setg(errp,
>                      "I and E extensions are incompatible");

Re: [PATCH 2/3] target/riscv: Add step to validate 'B' extension

[Andrew Jones]

On Tue, Jan 09, 2024 at 05:07:36PM +0000, Rob Bradford wrote:
> If the B extension is enabled warn if the user has disabled any of the
> required extensions that are part of the 'B' extension. Conversely
> enable the extensions that make up the 'B' extension if it is enabled.
> 
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>  target/riscv/tcg/tcg-cpu.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index fda54671d5..f10871d352 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -273,6 +273,35 @@ static void riscv_cpu_disable_priv_spec_isa_exts(RISCVCPU *cpu)
>      }
>  }
>  
> +static void riscv_cpu_validate_b(RISCVCPU *cpu)
> +{
> +    const char *warn_msg = "RVB mandates disabled extension %s";
> +
> +    if (!cpu->cfg.ext_zba) {
> +        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zba))) {
> +            cpu->cfg.ext_zba = true;
> +        } else {
> +            warn_report(warn_msg, "zba");
> +        }
> +    }
> +
> +    if (!cpu->cfg.ext_zbb) {
> +        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zbb))) {
> +            cpu->cfg.ext_zbb = true;
> +        } else {
> +            warn_report(warn_msg, "zbb");
> +        }
> +    }
> +
> +    if (!cpu->cfg.ext_zbs) {
> +        if (!cpu_cfg_ext_is_user_set(CPU_CFG_OFFSET(ext_zbs))) {
> +            cpu->cfg.ext_zbs = true;
> +        } else {
> +            warn_report(warn_msg, "zbs");
> +        }
> +    }
> +}
> +
>  /*
>   * Check consistency between chosen extensions while setting
>   * cpu->cfg accordingly.
> @@ -309,6 +338,10 @@ void riscv_cpu_validate_set_extensions(RISCVCPU *cpu, Error **errp)
>          env->misa_ext_mask |= RVI | RVM | RVA | RVF | RVD;
>      }
>  
> +    if (riscv_has_ext(env, RVB)) {
> +        riscv_cpu_validate_b(cpu);
> +    }
> +
>      if (riscv_has_ext(env, RVI) && riscv_has_ext(env, RVE)) {
>          error_setg(errp,
>                     "I and E extensions are incompatible");
> -- 
> 2.43.0
> 
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH 3/3] target/riscv: Enable 'B' extension on max CPU type

[Rob Bradford]

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
---
 target/riscv/tcg/tcg-cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index f10871d352..9705daec93 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -999,7 +999,8 @@ static void riscv_init_max_cpu_extensions(Object *obj)
     const RISCVCPUMultiExtConfig *prop;
 
     /* Enable RVG, RVJ and RVV that are disabled by default */
-    riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV);
+    riscv_cpu_set_misa(env, env->misa_mxl,
+                       env->misa_ext | RVG | RVJ | RVV | RVB);
 
     for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
         isa_ext_update_enabled(cpu, prop->offset, true);
-- 
2.43.0

Re: [PATCH 3/3] target/riscv: Enable 'B' extension on max CPU type

[Daniel Henrique Barboza]

On 1/9/24 14:07, Rob Bradford wrote:
> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> ---
>   target/riscv/tcg/tcg-cpu.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index f10871d352..9705daec93 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -999,7 +999,8 @@ static void riscv_init_max_cpu_extensions(Object *obj)
>       const RISCVCPUMultiExtConfig *prop;
>   
>       /* Enable RVG, RVJ and RVV that are disabled by default */
> -    riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV);
> +    riscv_cpu_set_misa(env, env->misa_mxl,
> +                       env->misa_ext | RVG | RVJ | RVV | RVB);

I'm aware that we decided a while ago the 'max' CPU could only have non-vendor and
non-experimental extensions enabled. RVB is experimental, so in theory we shouldn't
enable it.

But RVB is an alias for zba, zbb and zbs, extensions that the 'max' CPU is already
enabling. In this case I think it's sensible to enable RVB here since it would just
reflect stuff that it's already happening.


Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>




>   
>       for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
>           isa_ext_update_enabled(cpu, prop->offset, true);

Re: [PATCH 3/3] target/riscv: Enable 'B' extension on max CPU type

[Daniel Henrique Barboza]

Rob,

Given that you'll need to resend the patches due to the conflict in patch 2,
I think it would be nice to mention in this commit message that we're ok with
enabling RVB in the 'max' CPU, even though RVB per se is experimental, because
it's just an alias for extensions that the CPU already uses.

I'm asking this because future Daniel will forget why we're adding an experimental
extension in the 'max' CPU, and this info in the commit msg will spare him from
searching the ML archives to discover why :)



Thanks,

Daniel

On 1/10/24 15:32, Daniel Henrique Barboza wrote:
> 
> 
> On 1/9/24 14:07, Rob Bradford wrote:
>> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
>> ---
>>   target/riscv/tcg/tcg-cpu.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>> index f10871d352..9705daec93 100644
>> --- a/target/riscv/tcg/tcg-cpu.c
>> +++ b/target/riscv/tcg/tcg-cpu.c
>> @@ -999,7 +999,8 @@ static void riscv_init_max_cpu_extensions(Object *obj)
>>       const RISCVCPUMultiExtConfig *prop;
>>       /* Enable RVG, RVJ and RVV that are disabled by default */
>> -    riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV);
>> +    riscv_cpu_set_misa(env, env->misa_mxl,
>> +                       env->misa_ext | RVG | RVJ | RVV | RVB);
> 
> I'm aware that we decided a while ago the 'max' CPU could only have non-vendor and
> non-experimental extensions enabled. RVB is experimental, so in theory we shouldn't
> enable it.
> 
> But RVB is an alias for zba, zbb and zbs, extensions that the 'max' CPU is already
> enabling. In this case I think it's sensible to enable RVB here since it would just
> reflect stuff that it's already happening.
> 
> 
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> 
> 
> 
> 
>>       for (prop = riscv_cpu_extensions; prop && prop->name; prop++) {
>>           isa_ext_update_enabled(cpu, prop->offset, true);

Re: Re: [PATCH 3/3] target/riscv: Enable 'B' extension on max CPU type

[Andrew Jones]

On Wed, Jan 10, 2024 at 03:32:21PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 1/9/24 14:07, Rob Bradford wrote:
> > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > ---
> >   target/riscv/tcg/tcg-cpu.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > index f10871d352..9705daec93 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -999,7 +999,8 @@ static void riscv_init_max_cpu_extensions(Object *obj)
> >       const RISCVCPUMultiExtConfig *prop;
> >       /* Enable RVG, RVJ and RVV that are disabled by default */
> > -    riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV);
> > +    riscv_cpu_set_misa(env, env->misa_mxl,
> > +                       env->misa_ext | RVG | RVJ | RVV | RVB);
> 
> I'm aware that we decided a while ago the 'max' CPU could only have non-vendor and
> non-experimental extensions enabled. RVB is experimental, so in theory we shouldn't
> enable it.
> 
> But RVB is an alias for zba, zbb and zbs, extensions that the 'max' CPU is already
> enabling. In this case I think it's sensible to enable RVB here since it would just
> reflect stuff that it's already happening.

It's also setting the B bit in misa, which, until this spec is at least
frozen, is a reserved bit and reserved bits "must return zero when read".

I don't want to stand in the way of progress and it seems 99.9% likely
that the spec will be frozen and ratified, but, if we want to stick to
our policies (which we should document), then even the 'max' cpu type
should require x-b be added to the command line if it wants the B bit
set in misa.

Thanks,
drew

Re: [PATCH 3/3] target/riscv: Enable 'B' extension on max CPU type

[Daniel Henrique Barboza]

On 1/11/24 10:02, Andrew Jones wrote:
> On Wed, Jan 10, 2024 at 03:32:21PM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 1/9/24 14:07, Rob Bradford wrote:
>>> Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
>>> ---
>>>    target/riscv/tcg/tcg-cpu.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>>> index f10871d352..9705daec93 100644
>>> --- a/target/riscv/tcg/tcg-cpu.c
>>> +++ b/target/riscv/tcg/tcg-cpu.c
>>> @@ -999,7 +999,8 @@ static void riscv_init_max_cpu_extensions(Object *obj)
>>>        const RISCVCPUMultiExtConfig *prop;
>>>        /* Enable RVG, RVJ and RVV that are disabled by default */
>>> -    riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG | RVJ | RVV);
>>> +    riscv_cpu_set_misa(env, env->misa_mxl,
>>> +                       env->misa_ext | RVG | RVJ | RVV | RVB);
>>
>> I'm aware that we decided a while ago the 'max' CPU could only have non-vendor and
>> non-experimental extensions enabled. RVB is experimental, so in theory we shouldn't
>> enable it.
>>
>> But RVB is an alias for zba, zbb and zbs, extensions that the 'max' CPU is already
>> enabling. In this case I think it's sensible to enable RVB here since it would just													
>> reflect stuff that it's already happening.
> 
> It's also setting the B bit in misa, which, until this spec is at least
> frozen, is a reserved bit and reserved bits "must return zero when read".

This is a side effect that I wasn't aware of.

Rob, given that the 'max' CPU already has the zb* extensions enabled, is there any
other gain in enabling RVB in this CPU? If there isn't I'd rather leave this one
out for now.


Thanks,

Daniel


> 
> I don't want to stand in the way of progress and it seems 99.9% likely
> that the spec will be frozen and ratified, but, if we want to stick to
> our policies (which we should document), then even the 'max' cpu type
> should require x-b be added to the command line if it wants the B bit
> set in misa.
> 
> Thanks,
> drew

Re: [PATCH 3/3] target/riscv: Enable 'B' extension on max CPU type

[Rob Bradford]

On Thu, 2024-01-11 at 11:53 -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 1/11/24 10:02, Andrew Jones wrote:
> > On Wed, Jan 10, 2024 at 03:32:21PM -0300, Daniel Henrique Barboza
> > wrote:
> > > 
> > > 
> > > On 1/9/24 14:07, Rob Bradford wrote:
> > > > Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
> > > > ---
> > > >    target/riscv/tcg/tcg-cpu.c | 3 ++-
> > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-
> > > > cpu.c
> > > > index f10871d352..9705daec93 100644
> > > > --- a/target/riscv/tcg/tcg-cpu.c
> > > > +++ b/target/riscv/tcg/tcg-cpu.c
> > > > @@ -999,7 +999,8 @@ static void
> > > > riscv_init_max_cpu_extensions(Object *obj)
> > > >        const RISCVCPUMultiExtConfig *prop;
> > > >        /* Enable RVG, RVJ and RVV that are disabled by default
> > > > */
> > > > -    riscv_cpu_set_misa(env, env->misa_mxl, env->misa_ext | RVG
> > > > | RVJ | RVV);
> > > > +    riscv_cpu_set_misa(env, env->misa_mxl,
> > > > +                       env->misa_ext | RVG | RVJ | RVV | RVB);
> > > 
> > > I'm aware that we decided a while ago the 'max' CPU could only
> > > have non-vendor and
> > > non-experimental extensions enabled. RVB is experimental, so in
> > > theory we shouldn't
> > > enable it.
> > > 
> > > But RVB is an alias for zba, zbb and zbs, extensions that the
> > > 'max' CPU is already
> > > enabling. In this case I think it's sensible to enable RVB here
> > > since it would
> > > just                                                             
> > >                                         
> > > reflect stuff that it's already happening.
> > 
> > It's also setting the B bit in misa, which, until this spec is at
> > least
> > frozen, is a reserved bit and reserved bits "must return zero when
> > read".
> 
> This is a side effect that I wasn't aware of.
> 
> Rob, given that the 'max' CPU already has the zb* extensions enabled,
> is there any
> other gain in enabling RVB in this CPU? If there isn't I'd rather
> leave this one
> out for now.
> 

It seems completely reasonable to me to drop it for now.

Thanks for all the feedback,

Rob

> 
> Thanks,
> 
> Daniel
> 
> 
> > 
> > I don't want to stand in the way of progress and it seems 99.9%
> > likely
> > that the spec will be frozen and ratified, but, if we want to stick
> > to
> > our policies (which we should document), then even the 'max' cpu
> > type
> > should require x-b be added to the command line if it wants the B
> > bit
> > set in misa.
> > 
> > Thanks,
> > drew