[v4] target/riscv: Add isa extenstion strings to the device tree

The Linux kernel parses the ISA extensions from "riscv,isa" DT
property. It used to parse only the single letter base extensions
until now. A generic ISA extension parsing framework was proposed[1]
recently that can parse multi-letter ISA extensions as well.

Generate the extended ISA string by appending  the available ISA extensions
to the "riscv,isa" string if it is enabled so that kernel can process it.

[1] https://lkml.org/lkml/2022/2/15/263

Reviewed-by: Anup Patel <anup@brainfault.org>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Suggested-by: Heiko Stubner <heiko@sntech.de>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---

Changes from v3->v4:
1. Fixed the order of the extension names.
2. Added all the available ISA extensions in Qemu.

Changes from v2->v3:
1. Used g_strconcat to replace snprintf & a max isa string length as
suggested by Anup.
2. I have not included the Tested-by Tag from Heiko because the
implementation changed from v2 to v3.

Changes from v1->v2:
1. Improved the code redability by using arrays instead of individual check
---
 target/riscv/cpu.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Atish Patra <atishp@rivosinc.com> 於 2022年3月9日 週三 上午8:53寫道：

> The Linux kernel parses the ISA extensions from "riscv,isa" DT
> property. It used to parse only the single letter base extensions
> until now. A generic ISA extension parsing framework was proposed[1]
> recently that can parse multi-letter ISA extensions as well.
>
> Generate the extended ISA string by appending  the available ISA extensions
> to the "riscv,isa" string if it is enabled so that kernel can process it.
>
> [1] https://lkml.org/lkml/2022/2/15/263
>
> Reviewed-by: Anup Patel <anup@brainfault.org>
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> Suggested-by: Heiko Stubner <heiko@sntech.de>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
>
> Changes from v3->v4:
> 1. Fixed the order of the extension names.
> 2. Added all the available ISA extensions in Qemu.
>
> Changes from v2->v3:
> 1. Used g_strconcat to replace snprintf & a max isa string length as
> suggested by Anup.
> 2. I have not included the Tested-by Tag from Heiko because the
> implementation changed from v2 to v3.
>
> Changes from v1->v2:
> 1. Improved the code redability by using arrays instead of individual check
> ---
>  target/riscv/cpu.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index ddda4906ffb7..2521a6f31f9f 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -34,6 +34,12 @@
>
>  /* RISC-V CPU definitions */
>
> +/* This includes the null terminated character '\0' */
> +struct isa_ext_data {
> +        const char *name;
> +        bool enabled;
> +};
> +
>  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
>
>  const char * const riscv_int_regnames[] = {
> @@ -898,6 +904,42 @@ static void riscv_cpu_class_init(ObjectClass *c, void
> *data)
>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>
> +#define ISA_EDATA_ENTRY(name, prop) {#name, cpu->cfg.prop}
> +
> +static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int
> max_str_len)
> +{
> +    char *old = *isa_str;
> +    char *new = *isa_str;
> +    int i;
> +    struct isa_ext_data isa_edata_arr[] = {
> +        ISA_EDATA_ENTRY(svinval, ext_svinval),
> +        ISA_EDATA_ENTRY(svnapot, ext_svnapot),
> +        ISA_EDATA_ENTRY(svpbmt, ext_svpbmt),
> +        ISA_EDATA_ENTRY(zba, ext_zba),
> +        ISA_EDATA_ENTRY(zbb, ext_zbb),
> +        ISA_EDATA_ENTRY(zbc, ext_zbc),
> +        ISA_EDATA_ENTRY(zbs, ext_zbs),
> +        ISA_EDATA_ENTRY(zdinx, ext_zdinx),
> +        ISA_EDATA_ENTRY(zfh, ext_zfhmin),
> +        ISA_EDATA_ENTRY(zfhmin, ext_zfhmin),
> +        ISA_EDATA_ENTRY(zfinx, ext_zfinx),
> +        ISA_EDATA_ENTRY(zhinx, ext_zhinx),
> +        ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin),
> +        ISA_EDATA_ENTRY(zve32f, ext_zve32f),
> +        ISA_EDATA_ENTRY(zve64f, ext_zve64f),
> +    };
>

Hi Atish,

According to RISC-V Unpriviledge spec, Section 28.6:
https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L85
<https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L199>

"The first letter following the “Z” conventionally indicates the most
closely
related alphabetical extension category, IMAFDQLCBKJTPV.
For the “Zam” extension for misaligned atomics,
for example, the letter “a” indicates the extension is related to the “A”
standard extension.
If multiple “Z” extensions are named, they should be ordered first by
category,
then alphabetically within a category—for example, “Zicsr Zifencei Zam”."

So I think the order of "Z" extensions should be:
zfh, zfhmin, zfinx, zdinx, zba, zbb, zbc, zbs, zve32f, zve64f, zhinx,
zhinxmin

Also, I think "Zifencei" and "Zicsr" should also be covered as well,
and all extensions should follow the order defined in Table 28.11:
https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L141

"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."

So the overall order would be:
zicsr, zifencei, zfh, zfhmin, zfinx, zdinx, zba, zbb, zbc, zbs, zve32f,
zve64f, zhinx, zhinxmin, svinval, svnapot, svpbmt,

Regards,
Frank Chang


> +
> +    for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> +        if (isa_edata_arr[i].enabled) {
> +            new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
> +            g_free(old);
> +            old = new;
> +        }
> +    }
> +
> +    *isa_str = new;
> +}
> +
>  char *riscv_isa_string(RISCVCPU *cpu)
>  {
>      int i;
> @@ -910,6 +952,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>          }
>      }
>      *p = '\0';
> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>      return isa_str;
>  }
>
> --
> 2.30.2
>
>
>

On Wed, Mar 9, 2022 at 5:47 AM Frank Chang <frank.chang@sifive.com> wrote:
>
> Atish Patra <atishp@rivosinc.com> 於 2022年3月9日 週三 上午8:53寫道：
>>
>> The Linux kernel parses the ISA extensions from "riscv,isa" DT
>> property. It used to parse only the single letter base extensions
>> until now. A generic ISA extension parsing framework was proposed[1]
>> recently that can parse multi-letter ISA extensions as well.
>>
>> Generate the extended ISA string by appending  the available ISA extensions
>> to the "riscv,isa" string if it is enabled so that kernel can process it.
>>
>> [1] https://lkml.org/lkml/2022/2/15/263
>>
>> Reviewed-by: Anup Patel <anup@brainfault.org>
>> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
>> Suggested-by: Heiko Stubner <heiko@sntech.de>
>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> ---
>>
>> Changes from v3->v4:
>> 1. Fixed the order of the extension names.
>> 2. Added all the available ISA extensions in Qemu.
>>
>> Changes from v2->v3:
>> 1. Used g_strconcat to replace snprintf & a max isa string length as
>> suggested by Anup.
>> 2. I have not included the Tested-by Tag from Heiko because the
>> implementation changed from v2 to v3.
>>
>> Changes from v1->v2:
>> 1. Improved the code redability by using arrays instead of individual check
>> ---
>>  target/riscv/cpu.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 43 insertions(+)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index ddda4906ffb7..2521a6f31f9f 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -34,6 +34,12 @@
>>
>>  /* RISC-V CPU definitions */
>>
>> +/* This includes the null terminated character '\0' */
>> +struct isa_ext_data {
>> +        const char *name;
>> +        bool enabled;
>> +};
>> +
>>  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
>>
>>  const char * const riscv_int_regnames[] = {
>> @@ -898,6 +904,42 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>>      device_class_set_props(dc, riscv_cpu_properties);
>>  }
>>
>> +#define ISA_EDATA_ENTRY(name, prop) {#name, cpu->cfg.prop}
>> +
>> +static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len)
>> +{
>> +    char *old = *isa_str;
>> +    char *new = *isa_str;
>> +    int i;
>> +    struct isa_ext_data isa_edata_arr[] = {
>> +        ISA_EDATA_ENTRY(svinval, ext_svinval),
>> +        ISA_EDATA_ENTRY(svnapot, ext_svnapot),
>> +        ISA_EDATA_ENTRY(svpbmt, ext_svpbmt),
>> +        ISA_EDATA_ENTRY(zba, ext_zba),
>> +        ISA_EDATA_ENTRY(zbb, ext_zbb),
>> +        ISA_EDATA_ENTRY(zbc, ext_zbc),
>> +        ISA_EDATA_ENTRY(zbs, ext_zbs),
>> +        ISA_EDATA_ENTRY(zdinx, ext_zdinx),
>> +        ISA_EDATA_ENTRY(zfh, ext_zfhmin),
>> +        ISA_EDATA_ENTRY(zfhmin, ext_zfhmin),
>> +        ISA_EDATA_ENTRY(zfinx, ext_zfinx),
>> +        ISA_EDATA_ENTRY(zhinx, ext_zhinx),
>> +        ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin),
>> +        ISA_EDATA_ENTRY(zve32f, ext_zve32f),
>> +        ISA_EDATA_ENTRY(zve64f, ext_zve64f),
>> +    };
>
>
> Hi Atish,
>
> According to RISC-V Unpriviledge spec, Section 28.6:
> https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L85
>
> "The first letter following the “Z” conventionally indicates the most closely
> related alphabetical extension category, IMAFDQLCBKJTPV.
> For the “Zam” extension for misaligned atomics,
> for example, the letter “a” indicates the extension is related to the “A” standard extension.
> If multiple “Z” extensions are named, they should be ordered first by category,
> then alphabetically within a category—for example, “Zicsr Zifencei Zam”."
>

Yes. Sorry I missed that part. Will fix it.

> So I think the order of "Z" extensions should be:
> zfh, zfhmin, zfinx, zdinx, zba, zbb, zbc, zbs, zve32f, zve64f, zhinx, zhinxmin
>
> Also, I think "Zifencei" and "Zicsr" should also be covered as well,
> and all extensions should follow the order defined in Table 28.11:
> https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L141
>

I thought about that earlier as well. Zifencei & Zicsr was already
part of the ISA and carved out as an extension later.
Qemu/Any supervisor support that by default and won't work without
that. We can't possibly disable those and boot anything.

Do you think there is any benefit adding it ?


> "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."
>
> So the overall order would be:
> zicsr, zifencei, zfh, zfhmin, zfinx, zdinx, zba, zbb, zbc, zbs, zve32f, zve64f, zhinx, zhinxmin, svinval, svnapot, svpbmt,
>
> Regards,
> Frank Chang
>
>>
>> +
>> +    for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
>> +        if (isa_edata_arr[i].enabled) {
>> +            new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
>> +            g_free(old);
>> +            old = new;
>> +        }
>> +    }
>> +
>> +    *isa_str = new;
>> +}
>> +
>>  char *riscv_isa_string(RISCVCPU *cpu)
>>  {
>>      int i;
>> @@ -910,6 +952,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>>          }
>>      }
>>      *p = '\0';
>> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>>      return isa_str;
>>  }
>>
>> --
>> 2.30.2
>>
>>

On Fri, Mar 11, 2022 at 2:42 AM Atish Kumar Patra <atishp@rivosinc.com>
wrote:

> On Wed, Mar 9, 2022 at 5:47 AM Frank Chang <frank.chang@sifive.com> wrote:
> >
> > Atish Patra <atishp@rivosinc.com> 於 2022年3月9日 週三 上午8:53寫道：
> >>
> >> The Linux kernel parses the ISA extensions from "riscv,isa" DT
> >> property. It used to parse only the single letter base extensions
> >> until now. A generic ISA extension parsing framework was proposed[1]
> >> recently that can parse multi-letter ISA extensions as well.
> >>
> >> Generate the extended ISA string by appending  the available ISA
> extensions
> >> to the "riscv,isa" string if it is enabled so that kernel can process
> it.
> >>
> >> [1] https://lkml.org/lkml/2022/2/15/263
> >>
> >> Reviewed-by: Anup Patel <anup@brainfault.org>
> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> >> Suggested-by: Heiko Stubner <heiko@sntech.de>
> >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >> ---
> >>
> >> Changes from v3->v4:
> >> 1. Fixed the order of the extension names.
> >> 2. Added all the available ISA extensions in Qemu.
> >>
> >> Changes from v2->v3:
> >> 1. Used g_strconcat to replace snprintf & a max isa string length as
> >> suggested by Anup.
> >> 2. I have not included the Tested-by Tag from Heiko because the
> >> implementation changed from v2 to v3.
> >>
> >> Changes from v1->v2:
> >> 1. Improved the code redability by using arrays instead of individual
> check
> >> ---
> >>  target/riscv/cpu.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 43 insertions(+)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index ddda4906ffb7..2521a6f31f9f 100644
> >> --- a/target/riscv/cpu.c
> >> +++ b/target/riscv/cpu.c
> >> @@ -34,6 +34,12 @@
> >>
> >>  /* RISC-V CPU definitions */
> >>
> >> +/* This includes the null terminated character '\0' */
> >> +struct isa_ext_data {
> >> +        const char *name;
> >> +        bool enabled;
> >> +};
> >> +
> >>  static const char riscv_exts[26] = "IEMAFDQCLBJTPVNSUHKORWXYZG";
> >>
> >>  const char * const riscv_int_regnames[] = {
> >> @@ -898,6 +904,42 @@ static void riscv_cpu_class_init(ObjectClass *c,
> void *data)
> >>      device_class_set_props(dc, riscv_cpu_properties);
> >>  }
> >>
> >> +#define ISA_EDATA_ENTRY(name, prop) {#name, cpu->cfg.prop}
> >> +
> >> +static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int
> max_str_len)
> >> +{
> >> +    char *old = *isa_str;
> >> +    char *new = *isa_str;
> >> +    int i;
> >> +    struct isa_ext_data isa_edata_arr[] = {
> >> +        ISA_EDATA_ENTRY(svinval, ext_svinval),
> >> +        ISA_EDATA_ENTRY(svnapot, ext_svnapot),
> >> +        ISA_EDATA_ENTRY(svpbmt, ext_svpbmt),
> >> +        ISA_EDATA_ENTRY(zba, ext_zba),
> >> +        ISA_EDATA_ENTRY(zbb, ext_zbb),
> >> +        ISA_EDATA_ENTRY(zbc, ext_zbc),
> >> +        ISA_EDATA_ENTRY(zbs, ext_zbs),
> >> +        ISA_EDATA_ENTRY(zdinx, ext_zdinx),
> >> +        ISA_EDATA_ENTRY(zfh, ext_zfhmin),
> >> +        ISA_EDATA_ENTRY(zfhmin, ext_zfhmin),
> >> +        ISA_EDATA_ENTRY(zfinx, ext_zfinx),
> >> +        ISA_EDATA_ENTRY(zhinx, ext_zhinx),
> >> +        ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin),
> >> +        ISA_EDATA_ENTRY(zve32f, ext_zve32f),
> >> +        ISA_EDATA_ENTRY(zve64f, ext_zve64f),
> >> +    };
> >
> >
> > Hi Atish,
> >
> > According to RISC-V Unpriviledge spec, Section 28.6:
> > https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L85
> >
> > "The first letter following the “Z” conventionally indicates the most
> closely
> > related alphabetical extension category, IMAFDQLCBKJTPV.
> > For the “Zam” extension for misaligned atomics,
> > for example, the letter “a” indicates the extension is related to the
> “A” standard extension.
> > If multiple “Z” extensions are named, they should be ordered first by
> category,
> > then alphabetically within a category—for example, “Zicsr Zifencei Zam”."
> >
>
> Yes. Sorry I missed that part. Will fix it.
>
> > So I think the order of "Z" extensions should be:
> > zfh, zfhmin, zfinx, zdinx, zba, zbb, zbc, zbs, zve32f, zve64f, zhinx,
> zhinxmin
> >
> > Also, I think "Zifencei" and "Zicsr" should also be covered as well,
> > and all extensions should follow the order defined in Table 28.11:
> >
> https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L141
> >
>
> I thought about that earlier as well. Zifencei & Zicsr was already
> part of the ISA and carved out as an extension later.
> Qemu/Any supervisor support that by default and won't work without
> that. We can't possibly disable those and boot anything.
>
> Do you think there is any benefit adding it ?
>

I believe Zifencei & Zicsr are mostly supported by default.
They do appear in our internal hardware-generated DTS.
So... it's fine to either add them or not.
I don't see any penalty (as well as the benefit) to add them.

Regards,
Frank Chang


>
>
> > "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."
> >
> > So the overall order would be:
> > zicsr, zifencei, zfh, zfhmin, zfinx, zdinx, zba, zbb, zbc, zbs, zve32f,
> zve64f, zhinx, zhinxmin, svinval, svnapot, svpbmt,
> >
> > Regards,
> > Frank Chang
> >
> >>
> >> +
> >> +    for (i = 0; i < ARRAY_SIZE(isa_edata_arr); i++) {
> >> +        if (isa_edata_arr[i].enabled) {
> >> +            new = g_strconcat(old, "_", isa_edata_arr[i].name, NULL);
> >> +            g_free(old);
> >> +            old = new;
> >> +        }
> >> +    }
> >> +
> >> +    *isa_str = new;
> >> +}
> >> +
> >>  char *riscv_isa_string(RISCVCPU *cpu)
> >>  {
> >>      int i;
> >> @@ -910,6 +952,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
> >>          }
> >>      }
> >>      *p = '\0';
> >> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
> >>      return isa_str;
> >>  }
> >>
> >> --
> >> 2.30.2
> >>
> >>
>