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

[Anup Patel]

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