On Sun, Mar 6, 2022 at 2:12 PM Atish Kumar Patra <ati...@rivosinc.com> wrote:
> > > On Sat, Mar 5, 2022 at 9:36 PM Frank Chang <frank.ch...@sifive.com> wrote: > >> On Sun, Mar 6, 2022 at 7:42 AM Atish Kumar Patra <ati...@rivosinc.com> >> wrote: >> >>> >>> >>> On Sat, Mar 5, 2022 at 10:05 AM Heiko Stuebner <he...@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.ch...@sifive.com> >>>> wrote: >>>> > > Atish Patra <ati...@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 <he...@sntech.de> >>>> > >> Signed-off-by: Atish Patra <ati...@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 >>>> > >> >>>> > >>>> > >>>> > >>>> >>>> >>>> >>>> >>>>
