1. Patchew
  2. QEMU
  3. View series

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

Atish Patra posted 1 patch 2 years, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20220222223830.2319856-1-atishp@rivosinc.com
Test checkpatch passed
Maintainers: Alistair Francis <alistair.francis@wdc.com>, Palmer Dabbelt <palmer@dabbelt.com>, Bin Meng <bin.meng@windriver.com>
There is a newer version of this series
target/riscv/cpu.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
[PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Atish Patra 2 years, 2 months ago 
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

Suggested-by: Heiko Stubner <heiko@sntech.de>
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
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 | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index b0a40b83e7a8..2c7ff6ef555a 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[] = {
@@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     device_class_set_props(dc, riscv_cpu_properties);
 }
 
+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[] = {
+        { "svpbmt", cpu->cfg.ext_svpbmt   },
+        { "svinval", cpu->cfg.ext_svinval },
+        { "svnapot", cpu->cfg.ext_svnapot },
+    };
+
+    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;
@@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
         }
     }
     *p = '\0';
+    riscv_isa_string_ext(cpu, &isa_str, maxlen);
     return isa_str;
 }
 
-- 
2.30.2
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Anup Patel 2 years, 2 months ago 
On Wed, Feb 23, 2022 at 4:09 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> 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
>
> Suggested-by: Heiko Stubner <heiko@sntech.de>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
> 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 | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>
> +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[] = {
> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
> +        { "svinval", cpu->cfg.ext_svinval },
> +        { "svnapot", cpu->cfg.ext_svnapot },
> +    };
> +
> +    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;
> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>          }
>      }
>      *p = '\0';
> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>      return isa_str;
>  }
>
> --
> 2.30.2
>
>
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Frank Chang 2 years, 2 months ago 
Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：

> 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
>
> Suggested-by: Heiko Stubner <heiko@sntech.de>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> ---
> 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 | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c, void
> *data)
>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>
> +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[] = {
> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
> +        { "svinval", cpu->cfg.ext_svinval },
> +        { "svnapot", cpu->cfg.ext_svnapot },
>

We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs... etc.
Do you mind adding them as well?

Also, I think the order of ISA strings should be alphabetical as described:
https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96

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;
> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>          }
>      }
>      *p = '\0';
> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>      return isa_str;
>  }
>
> --
> 2.30.2
>
>
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Atish Patra 2 years, 1 month ago 
On Fri, Feb 25, 2022 at 11:46 PM Frank Chang <frank.chang@sifive.com> wrote:
>
>
>
> Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
>>
>> 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
>>
>> Suggested-by: Heiko Stubner <heiko@sntech.de>
>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> ---
>> 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 | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
>> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>>      device_class_set_props(dc, riscv_cpu_properties);
>>  }
>>
>> +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[] = {
>> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
>> +        { "svinval", cpu->cfg.ext_svinval },
>> +        { "svnapot", cpu->cfg.ext_svnapot },
>
>
> We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs... etc.
> Do you mind adding them as well?
>

Do we really need it ? Does any OS actually parse it from the device tree ?
AFAIK, Linux kernel doesn't use them. As the device tree is intended
to keep the information useful
for supervisor software, I prefer to avoid bloating if possible.

> Also, I think the order of ISA strings should be alphabetical as described:
> https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
>

Ahh yes. I will order them in alphabetical order and leave a big
comment for future reference as well.

> 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;
>> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>>          }
>>      }
>>      *p = '\0';
>> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>>      return isa_str;
>>  }
>>
>> --
>> 2.30.2
>>


-- 
Regards,
Atish
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Heiko Stuebner 2 years, 1 month ago 
Hi,

Am Donnerstag, 3. März 2022, 19:58:38 CET schrieb Atish Patra:
> On Fri, Feb 25, 2022 at 11:46 PM Frank Chang <frank.chang@sifive.com> wrote:
> > Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
> >>
> >> 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
> >>
> >> Suggested-by: Heiko Stubner <heiko@sntech.de>
> >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >> ---
> >> 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 | 29 +++++++++++++++++++++++++++++
> >>  1 file changed, 29 insertions(+)
> >>
> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
> >> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
> >>      device_class_set_props(dc, riscv_cpu_properties);
> >>  }
> >>
> >> +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[] = {
> >> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
> >> +        { "svinval", cpu->cfg.ext_svinval },
> >> +        { "svnapot", cpu->cfg.ext_svnapot },
> >
> >
> > We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs... etc.
> > Do you mind adding them as well?
> >
> 
> Do we really need it ? Does any OS actually parse it from the device tree ?
> AFAIK, Linux kernel doesn't use them. As the device tree is intended
> to keep the information useful
> for supervisor software,

That actually isn't true ;-) .

The devicetree is intended to _describe_ the hardware present in full
and has really nothing to do with what the userspace needs.
So the argument "Linux doesn't need this" is never true when talking
about devicetrees ;-) .

On the other hand the devicetree user doesn't need to parse everything
from DT. So adding code to parse things only really is needed if you
need that information.

So if some part of the kernel needs to know about those additional
extensions, the array entries for them can also be added in a later patch.


Heiko

> > Also, I think the order of ISA strings should be alphabetical as described:
> > https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
> >
> 
> Ahh yes. I will order them in alphabetical order and leave a big
> comment for future reference as well.
> 
> > 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;
> >> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
> >>          }
> >>      }
> >>      *p = '\0';
> >> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
> >>      return isa_str;
> >>  }
> >>
> >> --
> >> 2.30.2
> >>
> 
> 
>
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Atish Kumar Patra 2 years, 1 month ago 
On Sat, Mar 5, 2022 at 10:05 AM Heiko Stuebner <heiko@sntech.de> wrote:

> Hi,
>
> Am Donnerstag, 3. März 2022, 19:58:38 CET schrieb Atish Patra:
> > On Fri, Feb 25, 2022 at 11:46 PM Frank Chang <frank.chang@sifive.com>
> wrote:
> > > Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
> > >>
> > >> 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
> > >>
> > >> Suggested-by: Heiko Stubner <heiko@sntech.de>
> > >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> > >> ---
> > >> 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 | 29 +++++++++++++++++++++++++++++
> > >>  1 file changed, 29 insertions(+)
> > >>
> > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> > >> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
> > >> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c,
> void *data)
> > >>      device_class_set_props(dc, riscv_cpu_properties);
> > >>  }
> > >>
> > >> +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[] = {
> > >> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
> > >> +        { "svinval", cpu->cfg.ext_svinval },
> > >> +        { "svnapot", cpu->cfg.ext_svnapot },
> > >
> > >
> > > We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs...
> etc.
> > > Do you mind adding them as well?
> > >
> >
> > Do we really need it ? Does any OS actually parse it from the device
> tree ?
> > AFAIK, Linux kernel doesn't use them. As the device tree is intended
> > to keep the information useful
> > for supervisor software,
>
> That actually isn't true ;-) .
>
> The devicetree is intended to _describe_ the hardware present in full
> and has really nothing to do with what the userspace needs.
> So the argument "Linux doesn't need this" is never true when talking
> about devicetrees ;-) .
>

Yes. I didn’t mean that way. I was simply asking if these extensions
currently in use. I just mentioned Linux as an example.

The larger point I was trying to make if we should add all the supported
extensions when they are added to Qemu or on a need basis.

I don’t feel strongly either way. So I am okay with the former approach if
that’s what everyone prefers!

>
> On the other hand the devicetree user doesn't need to parse everything
> from DT. So adding code to parse things only really is needed if you
> need that information.
>

Agreed.


> So if some part of the kernel needs to know about those additional
> extensions, the array entries for them can also be added in a later patch.
>

Yes. That was the idea in isa extension framework series where the
extension specific array entries will only be added when support for that
extension is enabled.

>
>
> Heiko
>
> > > Also, I think the order of ISA strings should be alphabetical as
> described:
> > >
> https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
> > >
> >
> > Ahh yes. I will order them in alphabetical order and leave a big
> > comment for future reference as well.
> >
> > > 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;
> > >> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
> > >>          }
> > >>      }
> > >>      *p = '\0';
> > >> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
> > >>      return isa_str;
> > >>  }
> > >>
> > >> --
> > >> 2.30.2
> > >>
> >
> >
> >
>
>
>
>
>
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Frank Chang 2 years, 1 month ago 
On Sun, Mar 6, 2022 at 7:42 AM Atish Kumar Patra <atishp@rivosinc.com>
wrote:

>
>
> On Sat, Mar 5, 2022 at 10:05 AM Heiko Stuebner <heiko@sntech.de> wrote:
>
>> Hi,
>>
>> Am Donnerstag, 3. März 2022, 19:58:38 CET schrieb Atish Patra:
>> > On Fri, Feb 25, 2022 at 11:46 PM Frank Chang <frank.chang@sifive.com>
>> wrote:
>> > > Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
>> > >>
>> > >> 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
>> > >>
>> > >> Suggested-by: Heiko Stubner <heiko@sntech.de>
>> > >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> > >> ---
>> > >> 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 | 29 +++++++++++++++++++++++++++++
>> > >>  1 file changed, 29 insertions(+)
>> > >>
>> > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> > >> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
>> > >> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass
>> *c, void *data)
>> > >>      device_class_set_props(dc, riscv_cpu_properties);
>> > >>  }
>> > >>
>> > >> +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[] = {
>> > >> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
>> > >> +        { "svinval", cpu->cfg.ext_svinval },
>> > >> +        { "svnapot", cpu->cfg.ext_svnapot },
>> > >
>> > >
>> > > We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs...
>> etc.
>> > > Do you mind adding them as well?
>> > >
>> >
>> > Do we really need it ? Does any OS actually parse it from the device
>> tree ?
>> > AFAIK, Linux kernel doesn't use them. As the device tree is intended
>> > to keep the information useful
>> > for supervisor software,
>>
>> That actually isn't true ;-) .
>>
>> The devicetree is intended to _describe_ the hardware present in full
>> and has really nothing to do with what the userspace needs.
>> So the argument "Linux doesn't need this" is never true when talking
>> about devicetrees ;-) .
>>
>
> Yes. I didn’t mean that way. I was simply asking if these extensions
> currently in use. I just mentioned Linux as an example.
>
> The larger point I was trying to make if we should add all the supported
> extensions when they are added to Qemu or on a need basis.
>
> I don’t feel strongly either way. So I am okay with the former approach if
> that’s what everyone prefers!
>

Linux Kernel itself might not,
but userspace applications may query /proc/cpuinfo to get core's
capabilities, e.g. for those vector applications.
It really depends on how the application is written.

I still think adding all the enabled extensions into the isa string would
be a proper solution, no matter they are used or not.
However, we can leave that beyond this patch.

Regards,
Frank Chang


>
>> On the other hand the devicetree user doesn't need to parse everything
>> from DT. So adding code to parse things only really is needed if you
>> need that information.
>>
>
> Agreed.
>
>
>> So if some part of the kernel needs to know about those additional
>> extensions, the array entries for them can also be added in a later patch.
>>
>
> Yes. That was the idea in isa extension framework series where the
> extension specific array entries will only be added when support for that
> extension is enabled.
>
>>
>>
>> Heiko
>>
>> > > Also, I think the order of ISA strings should be alphabetical as
>> described:
>> > >
>> https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
>> > >
>> >
>> > Ahh yes. I will order them in alphabetical order and leave a big
>> > comment for future reference as well.
>> >
>> > > 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;
>> > >> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>> > >>          }
>> > >>      }
>> > >>      *p = '\0';
>> > >> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>> > >>      return isa_str;
>> > >>  }
>> > >>
>> > >> --
>> > >> 2.30.2
>> > >>
>> >
>> >
>> >
>>
>>
>>
>>
>>
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Atish Kumar Patra 2 years, 1 month ago 
On Sat, Mar 5, 2022 at 9:36 PM Frank Chang <frank.chang@sifive.com> wrote:

> On Sun, Mar 6, 2022 at 7:42 AM Atish Kumar Patra <atishp@rivosinc.com>
> wrote:
>
>>
>>
>> On Sat, Mar 5, 2022 at 10:05 AM Heiko Stuebner <heiko@sntech.de> wrote:
>>
>>> Hi,
>>>
>>> Am Donnerstag, 3. März 2022, 19:58:38 CET schrieb Atish Patra:
>>> > On Fri, Feb 25, 2022 at 11:46 PM Frank Chang <frank.chang@sifive.com>
>>> wrote:
>>> > > Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
>>> > >>
>>> > >> 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
>>> > >>
>>> > >> Suggested-by: Heiko Stubner <heiko@sntech.de>
>>> > >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>> > >> ---
>>> > >> 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 | 29 +++++++++++++++++++++++++++++
>>> > >>  1 file changed, 29 insertions(+)
>>> > >>
>>> > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>> > >> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
>>> > >> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass
>>> *c, void *data)
>>> > >>      device_class_set_props(dc, riscv_cpu_properties);
>>> > >>  }
>>> > >>
>>> > >> +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[] = {
>>> > >> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
>>> > >> +        { "svinval", cpu->cfg.ext_svinval },
>>> > >> +        { "svnapot", cpu->cfg.ext_svnapot },
>>> > >
>>> > >
>>> > > We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs...
>>> etc.
>>> > > Do you mind adding them as well?
>>> > >
>>> >
>>> > Do we really need it ? Does any OS actually parse it from the device
>>> tree ?
>>> > AFAIK, Linux kernel doesn't use them. As the device tree is intended
>>> > to keep the information useful
>>> > for supervisor software,
>>>
>>> That actually isn't true ;-) .
>>>
>>> The devicetree is intended to _describe_ the hardware present in full
>>> and has really nothing to do with what the userspace needs.
>>> So the argument "Linux doesn't need this" is never true when talking
>>> about devicetrees ;-) .
>>>
>>
>> Yes. I didn’t mean that way. I was simply asking if these extensions
>> currently in use. I just mentioned Linux as an example.
>>
>> The larger point I was trying to make if we should add all the supported
>> extensions when they are added to Qemu or on a need basis.
>>
>> I don’t feel strongly either way. So I am okay with the former approach
>> if that’s what everyone prefers!
>>
>
> Linux Kernel itself might not,
> but userspace applications may query /proc/cpuinfo to get core's
> capabilities, e.g. for those vector applications.
> It really depends on how the application is written.
>
> I still think adding all the enabled extensions into the isa string would
> be a proper solution, no matter they are used or not.
> However, we can leave that beyond this patch.
>


Fair enough. I will update the patch to include all the extension available.


> Regards,
> Frank Chang
>
>
>>
>>> On the other hand the devicetree user doesn't need to parse everything
>>> from DT. So adding code to parse things only really is needed if you
>>> need that information.
>>>
>>
>> Agreed.
>>
>>
>>> So if some part of the kernel needs to know about those additional
>>> extensions, the array entries for them can also be added in a later
>>> patch.
>>>
>>
>> Yes. That was the idea in isa extension framework series where the
>> extension specific array entries will only be added when support for that
>> extension is enabled.
>>
>>>
>>>
>>> Heiko
>>>
>>> > > Also, I think the order of ISA strings should be alphabetical as
>>> described:
>>> > >
>>> https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
>>> > >
>>> >
>>> > Ahh yes. I will order them in alphabetical order and leave a big
>>> > comment for future reference as well.
>>> >
>>> > > 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;
>>> > >> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>>> > >>          }
>>> > >>      }
>>> > >>      *p = '\0';
>>> > >> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>>> > >>      return isa_str;
>>> > >>  }
>>> > >>
>>> > >> --
>>> > >> 2.30.2
>>> > >>
>>> >
>>> >
>>> >
>>>
>>>
>>>
>>>
>>>
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Frank Chang 2 years, 1 month ago 
On Sun, Mar 6, 2022 at 2:12 PM Atish Kumar Patra <atishp@rivosinc.com>
wrote:

>
>
> On Sat, Mar 5, 2022 at 9:36 PM Frank Chang <frank.chang@sifive.com> wrote:
>
>> On Sun, Mar 6, 2022 at 7:42 AM Atish Kumar Patra <atishp@rivosinc.com>
>> wrote:
>>
>>>
>>>
>>> On Sat, Mar 5, 2022 at 10:05 AM Heiko Stuebner <heiko@sntech.de> wrote:
>>>
>>>> Hi,
>>>>
>>>> Am Donnerstag, 3. März 2022, 19:58:38 CET schrieb Atish Patra:
>>>> > On Fri, Feb 25, 2022 at 11:46 PM Frank Chang <frank.chang@sifive.com>
>>>> wrote:
>>>> > > Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
>>>> > >>
>>>> > >> 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
>>>> > >>
>>>> > >> Suggested-by: Heiko Stubner <heiko@sntech.de>
>>>> > >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>>> > >> ---
>>>> > >> 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 | 29 +++++++++++++++++++++++++++++
>>>> > >>  1 file changed, 29 insertions(+)
>>>> > >>
>>>> > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>>> > >> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
>>>> > >> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass
>>>> *c, void *data)
>>>> > >>      device_class_set_props(dc, riscv_cpu_properties);
>>>> > >>  }
>>>> > >>
>>>> > >> +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[] = {
>>>> > >> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
>>>> > >> +        { "svinval", cpu->cfg.ext_svinval },
>>>> > >> +        { "svnapot", cpu->cfg.ext_svnapot },
>>>> > >
>>>> > >
>>>> > > We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs...
>>>> etc.
>>>> > > Do you mind adding them as well?
>>>> > >
>>>> >
>>>> > Do we really need it ? Does any OS actually parse it from the device
>>>> tree ?
>>>> > AFAIK, Linux kernel doesn't use them. As the device tree is intended
>>>> > to keep the information useful
>>>> > for supervisor software,
>>>>
>>>> That actually isn't true ;-) .
>>>>
>>>> The devicetree is intended to _describe_ the hardware present in full
>>>> and has really nothing to do with what the userspace needs.
>>>> So the argument "Linux doesn't need this" is never true when talking
>>>> about devicetrees ;-) .
>>>>
>>>
>>> Yes. I didn’t mean that way. I was simply asking if these extensions
>>> currently in use. I just mentioned Linux as an example.
>>>
>>> The larger point I was trying to make if we should add all the supported
>>> extensions when they are added to Qemu or on a need basis.
>>>
>>> I don’t feel strongly either way. So I am okay with the former approach
>>> if that’s what everyone prefers!
>>>
>>
>> Linux Kernel itself might not,
>> but userspace applications may query /proc/cpuinfo to get core's
>> capabilities, e.g. for those vector applications.
>> It really depends on how the application is written.
>>
>> I still think adding all the enabled extensions into the isa string would
>> be a proper solution, no matter they are used or not.
>> However, we can leave that beyond this patch.
>>
>
>
> Fair enough. I will update the patch to include all the extension
> available.
>

Thanks, that would be great.

BTW, I think current QEMU RISC-V isa string includes both "s" and "u":
https://github.com/qemu/qemu/blob/master/target/riscv/cpu.c#L37
But in fact, they should not be presented in the DTS isa string.
Do you think we should exclude them as well?

Regards,
Frank Chang


>
>> Regards,
>> Frank Chang
>>
>>
>>>
>>>> On the other hand the devicetree user doesn't need to parse everything
>>>> from DT. So adding code to parse things only really is needed if you
>>>> need that information.
>>>>
>>>
>>> Agreed.
>>>
>>>
>>>> So if some part of the kernel needs to know about those additional
>>>> extensions, the array entries for them can also be added in a later
>>>> patch.
>>>>
>>>
>>> Yes. That was the idea in isa extension framework series where the
>>> extension specific array entries will only be added when support for that
>>> extension is enabled.
>>>
>>>>
>>>>
>>>> Heiko
>>>>
>>>> > > Also, I think the order of ISA strings should be alphabetical as
>>>> described:
>>>> > >
>>>> https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
>>>> > >
>>>> >
>>>> > Ahh yes. I will order them in alphabetical order and leave a big
>>>> > comment for future reference as well.
>>>> >
>>>> > > 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;
>>>> > >> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>>>> > >>          }
>>>> > >>      }
>>>> > >>      *p = '\0';
>>>> > >> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>>>> > >>      return isa_str;
>>>> > >>  }
>>>> > >>
>>>> > >> --
>>>> > >> 2.30.2
>>>> > >>
>>>> >
>>>> >
>>>> >
>>>>
>>>>
>>>>
>>>>
>>>>
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Atish Patra 2 years, 1 month ago 
On Sat, Mar 5, 2022 at 10:43 PM Frank Chang <frank.chang@sifive.com> wrote:
>
> On Sun, Mar 6, 2022 at 2:12 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>>
>>
>>
>> On Sat, Mar 5, 2022 at 9:36 PM Frank Chang <frank.chang@sifive.com> wrote:
>>>
>>> On Sun, Mar 6, 2022 at 7:42 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>>>>
>>>>
>>>>
>>>> On Sat, Mar 5, 2022 at 10:05 AM Heiko Stuebner <heiko@sntech.de> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> Am Donnerstag, 3. März 2022, 19:58:38 CET schrieb Atish Patra:
>>>>> > On Fri, Feb 25, 2022 at 11:46 PM Frank Chang <frank.chang@sifive.com> wrote:
>>>>> > > Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
>>>>> > >>
>>>>> > >> 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
>>>>> > >>
>>>>> > >> Suggested-by: Heiko Stubner <heiko@sntech.de>
>>>>> > >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>>>> > >> ---
>>>>> > >> 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 | 29 +++++++++++++++++++++++++++++
>>>>> > >>  1 file changed, 29 insertions(+)
>>>>> > >>
>>>>> > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>>>> > >> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
>>>>> > >> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>>>>> > >>      device_class_set_props(dc, riscv_cpu_properties);
>>>>> > >>  }
>>>>> > >>
>>>>> > >> +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[] = {
>>>>> > >> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
>>>>> > >> +        { "svinval", cpu->cfg.ext_svinval },
>>>>> > >> +        { "svnapot", cpu->cfg.ext_svnapot },
>>>>> > >
>>>>> > >
>>>>> > > We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs... etc.
>>>>> > > Do you mind adding them as well?
>>>>> > >
>>>>> >
>>>>> > Do we really need it ? Does any OS actually parse it from the device tree ?
>>>>> > AFAIK, Linux kernel doesn't use them. As the device tree is intended
>>>>> > to keep the information useful
>>>>> > for supervisor software,
>>>>>
>>>>> That actually isn't true ;-) .
>>>>>
>>>>> The devicetree is intended to _describe_ the hardware present in full
>>>>> and has really nothing to do with what the userspace needs.
>>>>> So the argument "Linux doesn't need this" is never true when talking
>>>>> about devicetrees ;-) .
>>>>
>>>>
>>>> Yes. I didn’t mean that way. I was simply asking if these extensions currently in use. I just mentioned Linux as an example.
>>>>
>>>> The larger point I was trying to make if we should add all the supported extensions when they are added to Qemu or on a need basis.
>>>>
>>>> I don’t feel strongly either way. So I am okay with the former approach if that’s what everyone prefers!
>>>
>>>
>>> Linux Kernel itself might not,
>>> but userspace applications may query /proc/cpuinfo to get core's capabilities, e.g. for those vector applications.
>>> It really depends on how the application is written.
>>>
>>> I still think adding all the enabled extensions into the isa string would be a proper solution, no matter they are used or not.
>>> However, we can leave that beyond this patch.
>>
>>
>>
>> Fair enough. I will update the patch to include all the extension available.
>
>
> Thanks, that would be great.
>
> BTW, I think current QEMU RISC-V isa string includes both "s" and "u":
> https://github.com/qemu/qemu/blob/master/target/riscv/cpu.c#L37
> But in fact, they should not be presented in the DTS isa string.
> Do you think we should exclude them as well?
>

There is a patch in the mailing list fixing that issue

https://lists.nongnu.org/archive/html/qemu-riscv/2022-02/msg00099.html

> Regards,
> Frank Chang
>
>>
>>>
>>> Regards,
>>> Frank Chang
>>>
>>>>>
>>>>>
>>>>> On the other hand the devicetree user doesn't need to parse everything
>>>>> from DT. So adding code to parse things only really is needed if you
>>>>> need that information.
>>>>
>>>>
>>>> Agreed.
>>>>
>>>>>
>>>>> So if some part of the kernel needs to know about those additional
>>>>> extensions, the array entries for them can also be added in a later patch.
>>>>
>>>>
>>>> Yes. That was the idea in isa extension framework series where the extension specific array entries will only be added when support for that extension is enabled.
>>>>>
>>>>>
>>>>>
>>>>> Heiko
>>>>>
>>>>> > > Also, I think the order of ISA strings should be alphabetical as described:
>>>>> > > https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
>>>>> > >
>>>>> >
>>>>> > Ahh yes. I will order them in alphabetical order and leave a big
>>>>> > comment for future reference as well.
>>>>> >
>>>>> > > 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;
>>>>> > >> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>>>>> > >>          }
>>>>> > >>      }
>>>>> > >>      *p = '\0';
>>>>> > >> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>>>>> > >>      return isa_str;
>>>>> > >>  }
>>>>> > >>
>>>>> > >> --
>>>>> > >> 2.30.2
>>>>> > >>
>>>>> >
>>>>> >
>>>>> >
>>>>>
>>>>>
>>>>>
>>>>>


-- 
Regards,
Atish
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Atish Patra 2 years, 1 month ago 
On Tue, Mar 8, 2022 at 2:53 PM Atish Patra <atishp@atishpatra.org> wrote:
>
> On Sat, Mar 5, 2022 at 10:43 PM Frank Chang <frank.chang@sifive.com> wrote:
> >
> > On Sun, Mar 6, 2022 at 2:12 PM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >>
> >>
> >>
> >> On Sat, Mar 5, 2022 at 9:36 PM Frank Chang <frank.chang@sifive.com> wrote:
> >>>
> >>> On Sun, Mar 6, 2022 at 7:42 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On Sat, Mar 5, 2022 at 10:05 AM Heiko Stuebner <heiko@sntech.de> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> Am Donnerstag, 3. März 2022, 19:58:38 CET schrieb Atish Patra:
> >>>>> > On Fri, Feb 25, 2022 at 11:46 PM Frank Chang <frank.chang@sifive.com> wrote:
> >>>>> > > Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
> >>>>> > >>
> >>>>> > >> 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
> >>>>> > >>
> >>>>> > >> Suggested-by: Heiko Stubner <heiko@sntech.de>
> >>>>> > >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
> >>>>> > >> ---
> >>>>> > >> 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 | 29 +++++++++++++++++++++++++++++
> >>>>> > >>  1 file changed, 29 insertions(+)
> >>>>> > >>
> >>>>> > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> >>>>> > >> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
> >>>>> > >> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
> >>>>> > >>      device_class_set_props(dc, riscv_cpu_properties);
> >>>>> > >>  }
> >>>>> > >>
> >>>>> > >> +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[] = {
> >>>>> > >> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
> >>>>> > >> +        { "svinval", cpu->cfg.ext_svinval },
> >>>>> > >> +        { "svnapot", cpu->cfg.ext_svnapot },
> >>>>> > >
> >>>>> > >
> >>>>> > > We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs... etc.
> >>>>> > > Do you mind adding them as well?
> >>>>> > >
> >>>>> >
> >>>>> > Do we really need it ? Does any OS actually parse it from the device tree ?
> >>>>> > AFAIK, Linux kernel doesn't use them. As the device tree is intended
> >>>>> > to keep the information useful
> >>>>> > for supervisor software,
> >>>>>
> >>>>> That actually isn't true ;-) .
> >>>>>
> >>>>> The devicetree is intended to _describe_ the hardware present in full
> >>>>> and has really nothing to do with what the userspace needs.
> >>>>> So the argument "Linux doesn't need this" is never true when talking
> >>>>> about devicetrees ;-) .
> >>>>
> >>>>
> >>>> Yes. I didn’t mean that way. I was simply asking if these extensions currently in use. I just mentioned Linux as an example.
> >>>>
> >>>> The larger point I was trying to make if we should add all the supported extensions when they are added to Qemu or on a need basis.
> >>>>
> >>>> I don’t feel strongly either way. So I am okay with the former approach if that’s what everyone prefers!
> >>>
> >>>
> >>> Linux Kernel itself might not,
> >>> but userspace applications may query /proc/cpuinfo to get core's capabilities, e.g. for those vector applications.
> >>> It really depends on how the application is written.
> >>>
> >>> I still think adding all the enabled extensions into the isa string would be a proper solution, no matter they are used or not.
> >>> However, we can leave that beyond this patch.
> >>
> >>
> >>
> >> Fair enough. I will update the patch to include all the extension available.
> >
> >
> > Thanks, that would be great.
> >
> > BTW, I think current QEMU RISC-V isa string includes both "s" and "u":
> > https://github.com/qemu/qemu/blob/master/target/riscv/cpu.c#L37
> > But in fact, they should not be presented in the DTS isa string.
> > Do you think we should exclude them as well?
> >
>
> There is a patch in the mailing list fixing that issue
>
> https://lists.nongnu.org/archive/html/qemu-riscv/2022-02/msg00099.html
>

I just realized that it was only sent to qemu-riscv@nongnu.org not qemu-devel.

@Tsukasa OI : Can you please send it again to both qemu-riscv &
qemu-devel so that it is accessible to the broader qemu community.

> > Regards,
> > Frank Chang
> >
> >>
> >>>
> >>> Regards,
> >>> Frank Chang
> >>>
> >>>>>
> >>>>>
> >>>>> On the other hand the devicetree user doesn't need to parse everything
> >>>>> from DT. So adding code to parse things only really is needed if you
> >>>>> need that information.
> >>>>
> >>>>
> >>>> Agreed.
> >>>>
> >>>>>
> >>>>> So if some part of the kernel needs to know about those additional
> >>>>> extensions, the array entries for them can also be added in a later patch.
> >>>>
> >>>>
> >>>> Yes. That was the idea in isa extension framework series where the extension specific array entries will only be added when support for that extension is enabled.
> >>>>>
> >>>>>
> >>>>>
> >>>>> Heiko
> >>>>>
> >>>>> > > Also, I think the order of ISA strings should be alphabetical as described:
> >>>>> > > https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
> >>>>> > >
> >>>>> >
> >>>>> > Ahh yes. I will order them in alphabetical order and leave a big
> >>>>> > comment for future reference as well.
> >>>>> >
> >>>>> > > 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;
> >>>>> > >> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
> >>>>> > >>          }
> >>>>> > >>      }
> >>>>> > >>      *p = '\0';
> >>>>> > >> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
> >>>>> > >>      return isa_str;
> >>>>> > >>  }
> >>>>> > >>
> >>>>> > >> --
> >>>>> > >> 2.30.2
> >>>>> > >>
> >>>>> >
> >>>>> >
> >>>>> >
> >>>>>
> >>>>>
> >>>>>
> >>>>>
>
>
> --
> Regards,
> Atish



-- 
Regards,
Atish
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Anup Patel 2 years, 1 month ago 
On Sun, Mar 6, 2022 at 11:06 AM Frank Chang <frank.chang@sifive.com> wrote:
>
> On Sun, Mar 6, 2022 at 7:42 AM Atish Kumar Patra <atishp@rivosinc.com> wrote:
>>
>>
>>
>> On Sat, Mar 5, 2022 at 10:05 AM Heiko Stuebner <heiko@sntech.de> wrote:
>>>
>>> Hi,
>>>
>>> Am Donnerstag, 3. März 2022, 19:58:38 CET schrieb Atish Patra:
>>> > On Fri, Feb 25, 2022 at 11:46 PM Frank Chang <frank.chang@sifive.com> wrote:
>>> > > Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
>>> > >>
>>> > >> 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
>>> > >>
>>> > >> Suggested-by: Heiko Stubner <heiko@sntech.de>
>>> > >> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>> > >> ---
>>> > >> 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 | 29 +++++++++++++++++++++++++++++
>>> > >>  1 file changed, 29 insertions(+)
>>> > >>
>>> > >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>> > >> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
>>> > >> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>>> > >>      device_class_set_props(dc, riscv_cpu_properties);
>>> > >>  }
>>> > >>
>>> > >> +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[] = {
>>> > >> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
>>> > >> +        { "svinval", cpu->cfg.ext_svinval },
>>> > >> +        { "svnapot", cpu->cfg.ext_svnapot },
>>> > >
>>> > >
>>> > > We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs... etc.
>>> > > Do you mind adding them as well?
>>> > >
>>> >
>>> > Do we really need it ? Does any OS actually parse it from the device tree ?
>>> > AFAIK, Linux kernel doesn't use them. As the device tree is intended
>>> > to keep the information useful
>>> > for supervisor software,
>>>
>>> That actually isn't true ;-) .
>>>
>>> The devicetree is intended to _describe_ the hardware present in full
>>> and has really nothing to do with what the userspace needs.
>>> So the argument "Linux doesn't need this" is never true when talking
>>> about devicetrees ;-) .
>>
>>
>> Yes. I didn’t mean that way. I was simply asking if these extensions currently in use. I just mentioned Linux as an example.
>>
>> The larger point I was trying to make if we should add all the supported extensions when they are added to Qemu or on a need basis.
>>
>> I don’t feel strongly either way. So I am okay with the former approach if that’s what everyone prefers!
>
>
> Linux Kernel itself might not,
> but userspace applications may query /proc/cpuinfo to get core's capabilities, e.g. for those vector applications.
> It really depends on how the application is written.
>
> I still think adding all the enabled extensions into the isa string would be a proper solution, no matter they are used or not.
> However, we can leave that beyond this patch.

I agree. Adding all enabled extensions in ISA string is aligned with
what this patch is doing so no harm in updating this patch.

Regards,
Anup

>
> Regards,
> Frank Chang
>
>>>
>>>
>>> On the other hand the devicetree user doesn't need to parse everything
>>> from DT. So adding code to parse things only really is needed if you
>>> need that information.
>>
>>
>> Agreed.
>>
>>>
>>> So if some part of the kernel needs to know about those additional
>>> extensions, the array entries for them can also be added in a later patch.
>>
>>
>> Yes. That was the idea in isa extension framework series where the extension specific array entries will only be added when support for that extension is enabled.
>>>
>>>
>>>
>>> Heiko
>>>
>>> > > Also, I think the order of ISA strings should be alphabetical as described:
>>> > > https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
>>> > >
>>> >
>>> > Ahh yes. I will order them in alphabetical order and leave a big
>>> > comment for future reference as well.
>>> >
>>> > > 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;
>>> > >> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>>> > >>          }
>>> > >>      }
>>> > >>      *p = '\0';
>>> > >> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>>> > >>      return isa_str;
>>> > >>  }
>>> > >>
>>> > >> --
>>> > >> 2.30.2
>>> > >>
>>> >
>>> >
>>> >
>>>
>>>
>>>
>>>
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Frank Chang 2 years, 1 month ago 
Typo in patch title:
s/extenstion/extension/g

Regards,
Frank Chang

On Sat, Feb 26, 2022 at 3:45 PM Frank Chang <frank.chang@sifive.com> wrote:

>
>
> Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
>
>> 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
>>
>> Suggested-by: Heiko Stubner <heiko@sntech.de>
>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>> ---
>> 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 | 29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
>> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c,
>> void *data)
>>      device_class_set_props(dc, riscv_cpu_properties);
>>  }
>>
>> +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[] = {
>> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
>> +        { "svinval", cpu->cfg.ext_svinval },
>> +        { "svnapot", cpu->cfg.ext_svnapot },
>>
>
> We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs... etc.
> Do you mind adding them as well?
>
> Also, I think the order of ISA strings should be alphabetical as described:
> https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
>
> 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;
>> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>>          }
>>      }
>>      *p = '\0';
>> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>>      return isa_str;
>>  }
>>
>> --
>> 2.30.2
>>
>>
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Atish Patra 2 years, 1 month ago 
On Sat, Mar 5, 2022 at 10:47 PM Frank Chang <frank.chang@sifive.com> wrote:
>
> Typo in patch title:
> s/extenstion/extension/g
>

Thanks for catching it. Will fix it.

> Regards,
> Frank Chang
>
> On Sat, Feb 26, 2022 at 3:45 PM Frank Chang <frank.chang@sifive.com> wrote:
>>
>>
>>
>> Atish Patra <atishp@rivosinc.com> 於 2022年2月23日 週三 上午6:39寫道：
>>>
>>> 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
>>>
>>> Suggested-by: Heiko Stubner <heiko@sntech.de>
>>> Signed-off-by: Atish Patra <atishp@rivosinc.com>
>>> ---
>>> 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 | 29 +++++++++++++++++++++++++++++
>>>  1 file changed, 29 insertions(+)
>>>
>>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>>> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
>>> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>>>      device_class_set_props(dc, riscv_cpu_properties);
>>>  }
>>>
>>> +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[] = {
>>> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
>>> +        { "svinval", cpu->cfg.ext_svinval },
>>> +        { "svnapot", cpu->cfg.ext_svnapot },
>>
>>
>> We still have other sub-extensions, e.g. Zfh, Zba, Zbb, Zbc, Zbs... etc.
>> Do you mind adding them as well?
>>
>> Also, I think the order of ISA strings should be alphabetical as described:
>> https://github.com/riscv/riscv-isa-manual/blob/master/src/naming.tex#L96
>>
>> 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;
>>> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>>>          }
>>>      }
>>>      *p = '\0';
>>> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>>>      return isa_str;
>>>  }
>>>
>>> --
>>> 2.30.2
>>>


-- 
Regards,
Atish
Re: [PATCH v3] target/riscv: Add isa extenstion strings to the device tree
Posted by Alistair Francis 2 years, 2 months ago 
On Wed, Feb 23, 2022 at 8:39 AM Atish Patra <atishp@rivosinc.com> wrote:
>
> 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
>
> Suggested-by: Heiko Stubner <heiko@sntech.de>
> Signed-off-by: Atish Patra <atishp@rivosinc.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> 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 | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
>
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index b0a40b83e7a8..2c7ff6ef555a 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[] = {
> @@ -881,6 +887,28 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>      device_class_set_props(dc, riscv_cpu_properties);
>  }
>
> +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[] = {
> +        { "svpbmt", cpu->cfg.ext_svpbmt   },
> +        { "svinval", cpu->cfg.ext_svinval },
> +        { "svnapot", cpu->cfg.ext_svnapot },
> +    };
> +
> +    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;
> @@ -893,6 +921,7 @@ char *riscv_isa_string(RISCVCPU *cpu)
>          }
>      }
>      *p = '\0';
> +    riscv_isa_string_ext(cpu, &isa_str, maxlen);
>      return isa_str;
>  }
>
> --
> 2.30.2
>
>

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.