| Message ID | 20190807145939.1281-1-palmer@sifive.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for,4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings | expand |
On Wed, 7 Aug 2019, Palmer Dabbelt wrote: > The ISA strings we're providing from QEMU aren't actually legal RISC-V > ISA strings, as both the S and U extensions cannot exist as > single-letter extensions and must instead be multi-letter strings. > We're still using the ISA strings inside QEMU to track the availiable > extensions, so this patch just strips out the S and U extensions when > formatting ISA strings. > > This boots Linux on top of 4.1-rc3, which no longer has the U extension > in /proc/cpuinfo. > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > --- > This is another late one, but I'd like to target it for 4.1 as we're > providing illegal ISA strings and I don't want to bake that into a bunch > of other code. I'm unfamiliar with the underlying QEMU code beyond the patch posted here, but I can review the intention expressed in the patch description. The described intent is aligned with Section 22.6 and Table 22.1 of the RISC-V User-Level ISA Specification version 2.2: https://github.com/riscv/riscv-isa-manual/blob/master/release/riscv-spec-v2.2.pdf And on the Linux kernel side we've also recognized that our current parsing code is handling "s" and "u" incorrectly and that we'll need to fix it: https://lore.kernel.org/linux-riscv/alpine.DEB.2.21.9999.1908061818360.13971@viisi.sifive.com/ - Paul
On Wed, 7 Aug 2019 at 16:02, Palmer Dabbelt <palmer@sifive.com> wrote: > > The ISA strings we're providing from QEMU aren't actually legal RISC-V > ISA strings, as both the S and U extensions cannot exist as > single-letter extensions and must instead be multi-letter strings. > We're still using the ISA strings inside QEMU to track the availiable > extensions, so this patch just strips out the S and U extensions when > formatting ISA strings. > > This boots Linux on top of 4.1-rc3, which no longer has the U extension > in /proc/cpuinfo. > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > --- > This is another late one, but I'd like to target it for 4.1 as we're > providing illegal ISA strings and I don't want to bake that into a bunch > of other code. Sorry, you've missed the 4.1 train by about 24 hours. There will be no further changes to 4.1 unless they are absolute showstoppers (security bugs, bad data loss, etc), and this doesn't count, judging by the description. thanks -- PMM
On Wed, 07 Aug 2019 09:08:20 PDT (-0700), Peter Maydell wrote: > On Wed, 7 Aug 2019 at 16:02, Palmer Dabbelt <palmer@sifive.com> wrote: >> >> The ISA strings we're providing from QEMU aren't actually legal RISC-V >> ISA strings, as both the S and U extensions cannot exist as >> single-letter extensions and must instead be multi-letter strings. >> We're still using the ISA strings inside QEMU to track the availiable >> extensions, so this patch just strips out the S and U extensions when >> formatting ISA strings. >> >> This boots Linux on top of 4.1-rc3, which no longer has the U extension >> in /proc/cpuinfo. >> >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> --- >> This is another late one, but I'd like to target it for 4.1 as we're >> providing illegal ISA strings and I don't want to bake that into a bunch >> of other code. > > Sorry, you've missed the 4.1 train by about 24 hours. There > will be no further changes to 4.1 unless they are absolute > showstoppers (security bugs, bad data loss, etc), and this doesn't > count, judging by the description. OK, no problem.
On Wed, 7 Aug 2019 at 16:02, Palmer Dabbelt <palmer@sifive.com> wrote: > > The ISA strings we're providing from QEMU aren't actually legal RISC-V > ISA strings, as both the S and U extensions cannot exist as > single-letter extensions and must instead be multi-letter strings. > We're still using the ISA strings inside QEMU to track the availiable > extensions, so this patch just strips out the S and U extensions when > formatting ISA strings. > > This boots Linux on top of 4.1-rc3, which no longer has the U extension > in /proc/cpuinfo. > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > --- > This is another late one, but I'd like to target it for 4.1 as we're > providing illegal ISA strings and I don't want to bake that into a bunch > of other code. > --- > target/riscv/cpu.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index f8d07bd20ad7..4df14433d789 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -501,7 +501,22 @@ char *riscv_isa_string(RISCVCPU *cpu) > char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS); > for (i = 0; i < sizeof(riscv_exts); i++) { > if (cpu->env.misa & RV(riscv_exts[i])) { > - *p++ = qemu_tolower(riscv_exts[i]); > + char lower = qemu_tolower(riscv_exts[i]); > + switch (lower) { > + case 's': > + case 'u': > + /* > + * The 's' and 'u' extensions shouldn't be passed in the device > + * tree, but we still use them internally to track extension > + * sets. Here we just explicitly remove them when formatting > + * an ISA string. > + */ > + break; > + > + default: > + *p++ = qemu_tolower(riscv_exts[i]); *p++ = lower; ? thanks -- PMM
On Wed, 07 Aug 2019 09:41:17 PDT (-0700), Peter Maydell wrote: > On Wed, 7 Aug 2019 at 16:02, Palmer Dabbelt <palmer@sifive.com> wrote: >> >> The ISA strings we're providing from QEMU aren't actually legal RISC-V >> ISA strings, as both the S and U extensions cannot exist as >> single-letter extensions and must instead be multi-letter strings. >> We're still using the ISA strings inside QEMU to track the availiable >> extensions, so this patch just strips out the S and U extensions when >> formatting ISA strings. >> >> This boots Linux on top of 4.1-rc3, which no longer has the U extension >> in /proc/cpuinfo. >> >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> --- >> This is another late one, but I'd like to target it for 4.1 as we're >> providing illegal ISA strings and I don't want to bake that into a bunch >> of other code. >> --- >> target/riscv/cpu.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index f8d07bd20ad7..4df14433d789 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -501,7 +501,22 @@ char *riscv_isa_string(RISCVCPU *cpu) >> char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS); >> for (i = 0; i < sizeof(riscv_exts); i++) { >> if (cpu->env.misa & RV(riscv_exts[i])) { >> - *p++ = qemu_tolower(riscv_exts[i]); >> + char lower = qemu_tolower(riscv_exts[i]); >> + switch (lower) { >> + case 's': >> + case 'u': >> + /* >> + * The 's' and 'u' extensions shouldn't be passed in the device >> + * tree, but we still use them internally to track extension >> + * sets. Here we just explicitly remove them when formatting >> + * an ISA string. >> + */ >> + break; >> + >> + default: >> + *p++ = qemu_tolower(riscv_exts[i]); > > *p++ = lower; ? Yes, thanks -- that's why I pulled the variable out in the first place :) > > thanks > -- PMM
On Wed, Aug 7, 2019 at 8:00 AM Palmer Dabbelt <palmer@sifive.com> wrote: > > The ISA strings we're providing from QEMU aren't actually legal RISC-V > ISA strings, as both the S and U extensions cannot exist as > single-letter extensions and must instead be multi-letter strings. > We're still using the ISA strings inside QEMU to track the availiable s/availiable/available/g > extensions, so this patch just strips out the S and U extensions when > formatting ISA strings. Atish and I were talking about this and we concluded that S and U aren't extensions, but should be reported in the misa CSR. > > This boots Linux on top of 4.1-rc3, which no longer has the U extension > in /proc/cpuinfo. > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > --- > This is another late one, but I'd like to target it for 4.1 as we're > providing illegal ISA strings and I don't want to bake that into a bunch > of other code. > --- > target/riscv/cpu.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index f8d07bd20ad7..4df14433d789 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -501,7 +501,22 @@ char *riscv_isa_string(RISCVCPU *cpu) > char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS); > for (i = 0; i < sizeof(riscv_exts); i++) { > if (cpu->env.misa & RV(riscv_exts[i])) { > - *p++ = qemu_tolower(riscv_exts[i]); > + char lower = qemu_tolower(riscv_exts[i]); > + switch (lower) { > + case 's': > + case 'u': > + /* > + * The 's' and 'u' extensions shouldn't be passed in the device > + * tree, but we still use them internally to track extension > + * sets. Here we just explicitly remove them when formatting > + * an ISA string. This should be updated to note mention 's' and 'u' as extensions, but clarify that they are correctly include in the misa CSR. Alistair > + */ > + break; > + > + default: > + *p++ = qemu_tolower(riscv_exts[i]); > + break; > + } > } > } > *p = '\0'; > -- > 2.21.0 > >
On Wed, 07 Aug 2019 10:54:52 PDT (-0700), alistair23@gmail.com wrote: > On Wed, Aug 7, 2019 at 8:00 AM Palmer Dabbelt <palmer@sifive.com> wrote: >> >> The ISA strings we're providing from QEMU aren't actually legal RISC-V >> ISA strings, as both the S and U extensions cannot exist as >> single-letter extensions and must instead be multi-letter strings. >> We're still using the ISA strings inside QEMU to track the availiable > > s/availiable/available/g > >> extensions, so this patch just strips out the S and U extensions when >> formatting ISA strings. > > Atish and I were talking about this and we concluded that S and U > aren't extensions, but should be reported in the misa CSR. Andrew agrees. > >> >> This boots Linux on top of 4.1-rc3, which no longer has the U extension >> in /proc/cpuinfo. >> >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> --- >> This is another late one, but I'd like to target it for 4.1 as we're >> providing illegal ISA strings and I don't want to bake that into a bunch >> of other code. >> --- >> target/riscv/cpu.c | 17 ++++++++++++++++- >> 1 file changed, 16 insertions(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index f8d07bd20ad7..4df14433d789 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -501,7 +501,22 @@ char *riscv_isa_string(RISCVCPU *cpu) >> char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS); >> for (i = 0; i < sizeof(riscv_exts); i++) { >> if (cpu->env.misa & RV(riscv_exts[i])) { >> - *p++ = qemu_tolower(riscv_exts[i]); >> + char lower = qemu_tolower(riscv_exts[i]); >> + switch (lower) { >> + case 's': >> + case 'u': >> + /* >> + * The 's' and 'u' extensions shouldn't be passed in the device >> + * tree, but we still use them internally to track extension >> + * sets. Here we just explicitly remove them when formatting >> + * an ISA string. > > This should be updated to note mention 's' and 'u' as extensions, but > clarify that they are correctly include in the misa CSR. I'll send a v2 that cleans up the wording on the comment and commit message. > > Alistair > >> + */ >> + break; >> + >> + default: >> + *p++ = qemu_tolower(riscv_exts[i]); >> + break; >> + } >> } >> } >> *p = '\0'; >> -- >> 2.21.0 >> >>
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index f8d07bd20ad7..4df14433d789 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -501,7 +501,22 @@ char *riscv_isa_string(RISCVCPU *cpu) char *p = isa_str + snprintf(isa_str, maxlen, "rv%d", TARGET_LONG_BITS); for (i = 0; i < sizeof(riscv_exts); i++) { if (cpu->env.misa & RV(riscv_exts[i])) { - *p++ = qemu_tolower(riscv_exts[i]); + char lower = qemu_tolower(riscv_exts[i]); + switch (lower) { + case 's': + case 'u': + /* + * The 's' and 'u' extensions shouldn't be passed in the device + * tree, but we still use them internally to track extension + * sets. Here we just explicitly remove them when formatting + * an ISA string. + */ + break; + + default: + *p++ = qemu_tolower(riscv_exts[i]); + break; + } } } *p = '\0';
The ISA strings we're providing from QEMU aren't actually legal RISC-V ISA strings, as both the S and U extensions cannot exist as single-letter extensions and must instead be multi-letter strings. We're still using the ISA strings inside QEMU to track the availiable extensions, so this patch just strips out the S and U extensions when formatting ISA strings. This boots Linux on top of 4.1-rc3, which no longer has the U extension in /proc/cpuinfo. Signed-off-by: Palmer Dabbelt <palmer@sifive.com> --- This is another late one, but I'd like to target it for 4.1 as we're providing illegal ISA strings and I don't want to bake that into a bunch of other code. --- target/riscv/cpu.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)