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