[Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

[Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

[Palmer Dabbelt]

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]);
+                break;
+            }
         }
     }
     *p = '\0';
-- 
2.21.0

Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

[Paul Walmsley]

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

Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

[Palmer Dabbelt]

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.

Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

[Palmer Dabbelt]

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

Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

[Alistair Francis]

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

Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings

[Palmer Dabbelt]

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