| Message ID | 20220805155405.1504081-2-mail@conchuod.ie |
|---|---|
| State | New |
| Headers | show |
| Series | QEMU: Fix RISC-V virt & spike machines' dtbs | expand |
On Sat, Aug 6, 2022 at 2:08 AM Conor Dooley <mail@conchuod.ie> wrote: > > From: Palmer Dabbelt <palmer@sifive.com> > > The ISA strings we're providing from QEMU aren't actually legal RISC-V > ISA strings, as both S and U 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 just strip > out the S and U extensions when formatting ISA strings. > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > [Conor: rebased on 7.1.0-rc1 & slightly tweaked the commit message] > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > target/riscv/cpu.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index ac6f82ebd0..95fdc03b3d 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1122,7 +1122,23 @@ 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_single_letter_exts) - 1; i++) { > if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) { > - *p++ = qemu_tolower(riscv_single_letter_exts[i]); riscv_single_letter_exts doesn't contain S or U, is this patch still required? Alistair > + char lower = qemu_tolower(riscv_single_letter_exts[i]); > + switch (lower) { > + case 's': > + case 'u': > + /* > + * The 's' and 'u' letters shouldn't show up in ISA strings as > + * they're not extensions, but they should show up in MISA. > + * Since we use these letters interally as a pseudo ISA string > + * to set MISA it's easier to just strip them out when > + * formatting the ISA string. > + */ > + break; > + > + default: > + *p++ = lower; > + break; > + } > } > } > *p = '\0'; > -- > 2.37.1 > >
On 07/08/2022 23:53, Alistair Francis wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Sat, Aug 6, 2022 at 2:08 AM Conor Dooley <mail@conchuod.ie> wrote: >> >> From: Palmer Dabbelt <palmer@sifive.com> >> >> The ISA strings we're providing from QEMU aren't actually legal RISC-V >> ISA strings, as both S and U 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 just strip >> out the S and U extensions when formatting ISA strings. >> >> Signed-off-by: Palmer Dabbelt <palmer@sifive.com> >> [Conor: rebased on 7.1.0-rc1 & slightly tweaked the commit message] >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> target/riscv/cpu.c | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index ac6f82ebd0..95fdc03b3d 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -1122,7 +1122,23 @@ 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_single_letter_exts) - 1; i++) { >> if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) { >> - *p++ = qemu_tolower(riscv_single_letter_exts[i]); > > riscv_single_letter_exts doesn't contain S or U, is this patch still required? Seemingly, yes. This is what ends up in the dtb: /home/rob/riscv-virt.dtb: cpu@0: riscv,isa:0: 'rv64imafdcsuh' is not one of ['rv64imac', 'rv64imafdc'] From schema: /home/rob/proj/git/linux-dt/Documentation/devicetree/bindings/riscv/cpus.yaml With Palmer's patch applied, the dtb's isa string becomes rv64imafdch_zicsr_zifencei_zba_zbb_zbc_zbs while in n /proc/cpuinfo it is rv64imafdch The short_isa_string flag (I think that's it's name) is not set for the dtb creation. meant to note this under the --- for this patch but obviously I forgot. Thanks, Conor. > > Alistair > >> + char lower = qemu_tolower(riscv_single_letter_exts[i]); >> + switch (lower) { >> + case 's': >> + case 'u': >> + /* >> + * The 's' and 'u' letters shouldn't show up in ISA strings as >> + * they're not extensions, but they should show up in MISA. >> + * Since we use these letters interally as a pseudo ISA string >> + * to set MISA it's easier to just strip them out when >> + * formatting the ISA string. >> + */ >> + break; >> + >> + default: >> + *p++ = lower; >> + break; >> + } >> } >> } >> *p = '\0'; >> -- >> 2.37.1 >> >>
On 2022/08/06 0:54, Conor Dooley wrote: > From: Palmer Dabbelt <palmer@sifive.com> > > The ISA strings we're providing from QEMU aren't actually legal RISC-V > ISA strings, as both S and U 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 just strip > out the S and U extensions when formatting ISA strings. > > Signed-off-by: Palmer Dabbelt <palmer@sifive.com> > [Conor: rebased on 7.1.0-rc1 & slightly tweaked the commit message] > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > target/riscv/cpu.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index ac6f82ebd0..95fdc03b3d 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -1122,7 +1122,23 @@ 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_single_letter_exts) - 1; i++) { > if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) { > - *p++ = qemu_tolower(riscv_single_letter_exts[i]); > + char lower = qemu_tolower(riscv_single_letter_exts[i]); > + switch (lower) { > + case 's': > + case 'u': > + /* > + * The 's' and 'u' letters shouldn't show up in ISA strings as > + * they're not extensions, but they should show up in MISA. > + * Since we use these letters interally as a pseudo ISA string > + * to set MISA it's easier to just strip them out when > + * formatting the ISA string. > + */ > + break; > + > + default: > + *p++ = lower; > + break; > + } > } > } > *p = '\0'; I agree with Alistair. **I** removed 'S' and 'U' from the ISA string and it should be working in the latest development branch (I tested). I tested it on master and QEMU 7.1-rc1 (tag: v7.1-rc1). Example: /opt/riscv/bin/qemu-system-riscv64 -machine virt -nographic -cpu rv64 -smp 1 -kernel images/linux.bin -initrd images/busybox.cpio.gz -append 'console=hvc0 earlycon=sbi' -bios images/opensbi-fw_jump.elf -gdb tcp::9000 Replacing -machine virt with -machine virt,dumpdtb=sample.dtb dumps the binary DeviceTree as sample.dtb and generated CPU-related parts like... cpu@0 { phandle = <0x01>; device_type = "cpu"; reg = <0x00>; status = "okay"; compatible = "riscv"; riscv,isa = "rv64imafdch_zicsr_zifencei_zba_zbb_zbc_zbs"; mmu-type = "riscv,sv48"; interrupt-controller { #interrupt-cells = <0x01>; interrupt-controller; compatible = "riscv,cpu-intc"; phandle = <0x02>; }; }; Besides, this function alone generates the ISA string for DTB and there's no way such ISA strings with invalid 'S' and 'U' can be generated. It's definitely a behavior of QEMU 7.0 or before. Please. Please make sure that you are testing the right version of QEMU. Thanks, Tsukasa
On 08/08/2022 16:03, Tsukasa OI wrote: > I agree with Alistair. **I** removed 'S' and 'U' from the ISA string > and it should be working in the latest development branch (I tested). Yeah, I saw what you as I looked at the commit log while trying to understand why there were invalid strings appearing when the code looked like it was impossible. Certainly I found it very very odd. I didn't just revive a 2 year old patch without taking a look at the code. > Besides, this function alone generates the ISA string for DTB and > there's no way such ISA strings with invalid 'S' and 'U' can be > generated. It's definitely a behavior of QEMU 7.0 or before. Hmm, it would seem that you're right - I have retested on a fresh clone. I did checkout v7.1.0-rc1 before running the first build that I saw the invalid string on as I'd been on some hacked up & fossilised version prior to that. Perhaps some build artifacts were not correctly removed, consider me quite confused! I do recall the configure script saying something about my build directory when I kicked it off, so it is likely down to that. Unfortunately my bash history is out of order so I will not be able to replicate the conditions, having many terminals open does have it's downsides. > Please. Please make sure that you are testing the right version of QEMU. Heh. Please, please give me some allowance for reasonably believing I was in fact on the latest qemu/master after checking it out and building! I guess this patch can then be safely ignored :) Glad to have cleared this up as I was rather confused by what I saw. Thanks Tsukasa/Alistair.
On Tue, Aug 9, 2022 at 2:14 AM <Conor.Dooley@microchip.com> wrote: > > On 08/08/2022 16:03, Tsukasa OI wrote: > > I agree with Alistair. **I** removed 'S' and 'U' from the ISA string > > and it should be working in the latest development branch (I tested). > > Yeah, I saw what you as I looked at the commit log while trying to > understand why there were invalid strings appearing when the code > looked like it was impossible. Certainly I found it very very odd. > I didn't just revive a 2 year old patch without taking a look at > the code. > > > > Besides, this function alone generates the ISA string for DTB and > > there's no way such ISA strings with invalid 'S' and 'U' can be > > generated. It's definitely a behavior of QEMU 7.0 or before. > > Hmm, it would seem that you're right - I have retested on a fresh > clone. I did checkout v7.1.0-rc1 before running the first build that > I saw the invalid string on as I'd been on some hacked up & fossilised > version prior to that. Perhaps some build artifacts were not correctly > removed, consider me quite confused! I do recall the configure script > saying something about my build directory when I kicked it off, so > it is likely down to that. > > Unfortunately my bash history is out of order so I will not be able to > replicate the conditions, having many terminals open does have it's > downsides. > > > Please. Please make sure that you are testing the right version of QEMU. > > Heh. Please, please give me some allowance for reasonably believing I > was in fact on the latest qemu/master after checking it out and building! No worries! Glad we cleared that up > > I guess this patch can then be safely ignored :) > Glad to have cleared this up as I was rather confused by what I saw. Great! Do you mind resending the series then with this patch dropped? It just makes it easier for me to track and manage Alistair > Thanks Tsukasa/Alistair. >
On 08/08/2022 21:51, Alistair Francis wrote: > On Tue, Aug 9, 2022 at 2:14 AM <Conor.Dooley@microchip.com> wrote: >> I guess this patch can then be safely ignored :) >> Glad to have cleared this up as I was rather confused by what I saw. > > Great! Do you mind resending the series then with this patch dropped? > It just makes it easier for me to track and manage Aye, willdo.
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index ac6f82ebd0..95fdc03b3d 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -1122,7 +1122,23 @@ 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_single_letter_exts) - 1; i++) { if (cpu->env.misa_ext & RV(riscv_single_letter_exts[i])) { - *p++ = qemu_tolower(riscv_single_letter_exts[i]); + char lower = qemu_tolower(riscv_single_letter_exts[i]); + switch (lower) { + case 's': + case 'u': + /* + * The 's' and 'u' letters shouldn't show up in ISA strings as + * they're not extensions, but they should show up in MISA. + * Since we use these letters interally as a pseudo ISA string + * to set MISA it's easier to just strip them out when + * formatting the ISA string. + */ + break; + + default: + *p++ = lower; + break; + } } } *p = '\0';