QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings
@ 2019-08-07 14:59 Palmer Dabbelt
  2019-08-07 15:27 ` Paul Walmsley
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2019-08-07 14:59 UTC (permalink / raw)
  To: qemu-riscv, Alistair Francis
  Cc: Atish Patra, Palmer Dabbelt, qemu-devel, Paul Walmsley

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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings
  2019-08-07 14:59 [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings Palmer Dabbelt
@ 2019-08-07 15:27 ` Paul Walmsley
  2019-08-07 16:08 ` Peter Maydell
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 8+ messages in thread
From: Paul Walmsley @ 2019-08-07 15:27 UTC (permalink / raw)
  To: Palmer Dabbelt; +Cc: Alistair Francis, qemu-riscv, qemu-devel, Atish Patra

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings
  2019-08-07 14:59 [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings Palmer Dabbelt
  2019-08-07 15:27 ` Paul Walmsley
@ 2019-08-07 16:08 ` Peter Maydell
  2019-08-07 16:20   ` Palmer Dabbelt
  2019-08-07 16:41 ` Peter Maydell
  2019-08-07 17:54 ` Alistair Francis
  3 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2019-08-07 16:08 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Alistair Francis, Paul Walmsley, open list:RISC-V,
	QEMU Developers, Atish Patra

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings
  2019-08-07 16:08 ` Peter Maydell
@ 2019-08-07 16:20   ` Palmer Dabbelt
  0 siblings, 0 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2019-08-07 16:20 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Alistair Francis, Paul Walmsley, qemu-riscv, qemu-devel, Atish Patra

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.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings
  2019-08-07 14:59 [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings Palmer Dabbelt
  2019-08-07 15:27 ` Paul Walmsley
  2019-08-07 16:08 ` Peter Maydell
@ 2019-08-07 16:41 ` Peter Maydell
  2019-08-07 17:25   ` Palmer Dabbelt
  2019-08-07 17:54 ` Alistair Francis
  3 siblings, 1 reply; 8+ messages in thread
From: Peter Maydell @ 2019-08-07 16:41 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Alistair Francis, Paul Walmsley, open list:RISC-V,
	QEMU Developers, Atish Patra

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings
  2019-08-07 16:41 ` Peter Maydell
@ 2019-08-07 17:25   ` Palmer Dabbelt
  0 siblings, 0 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2019-08-07 17:25 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Alistair Francis, Paul Walmsley, qemu-riscv, qemu-devel, Atish Patra

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings
  2019-08-07 14:59 [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings Palmer Dabbelt
                   ` (2 preceding siblings ...)
  2019-08-07 16:41 ` Peter Maydell
@ 2019-08-07 17:54 ` Alistair Francis
  2019-08-13 22:54   ` Palmer Dabbelt
  3 siblings, 1 reply; 8+ messages in thread
From: Alistair Francis @ 2019-08-07 17:54 UTC (permalink / raw)
  To: Palmer Dabbelt
  Cc: Alistair Francis, Paul Walmsley, open list:RISC-V,
	qemu-devel@nongnu.org Developers, Atish Patra

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings
  2019-08-07 17:54 ` Alistair Francis
@ 2019-08-13 22:54   ` Palmer Dabbelt
  0 siblings, 0 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2019-08-13 22:54 UTC (permalink / raw)
  To: alistair23
  Cc: Alistair Francis, Paul Walmsley, qemu-riscv, qemu-devel, Atish Patra

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-07 14:59 [Qemu-devel] [PATCH for 4.1] RISC-V: Ignore the S and U extensions when formatting ISA strings Palmer Dabbelt
2019-08-07 15:27 ` Paul Walmsley
2019-08-07 16:08 ` Peter Maydell
2019-08-07 16:20   ` Palmer Dabbelt
2019-08-07 16:41 ` Peter Maydell
2019-08-07 17:25   ` Palmer Dabbelt
2019-08-07 17:54 ` Alistair Francis
2019-08-13 22:54   ` Palmer Dabbelt

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org qemu-devel@archiver.kernel.org
	public-inbox-index qemu-devel


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox