diff mbox series

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

Message ID 20240109171848.32237-2-rbradford@rivosinc.com (mailing list archive)
State New, archived
Headers show
Series target/riscv: Add support for 'B' extension | expand

Commit Message

Rob Bradford Jan. 9, 2024, 5:07 p.m. UTC 
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(-)

Comments

Daniel Henrique Barboza Jan. 10, 2024, 6:18 p.m. UTC | #1 
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>



>   };
>   
>   /*
Andrew Jones Jan. 11, 2024, 1:07 p.m. UTC | #2 
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
> 
>
Andrew Jones Jan. 11, 2024, 1:14 p.m. UTC | #3 
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
Andrew Jones Jan. 11, 2024, 1:15 p.m. UTC | #4 
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>
Rob Bradford Jan. 11, 2024, 3:17 p.m. UTC | #5 
+ 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
Andrew Jones Jan. 12, 2024, 4:08 p.m. UTC | #6 
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
Rob Bradford Jan. 12, 2024, 4:54 p.m. UTC | #7 
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
Ved Shanbhogue Jan. 13, 2024, 12:28 a.m. UTC | #8 
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
diff mbox series

Patch

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)
 };
 
 /*