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

[Atish Patra]

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