* [PATCH 0/2] target/riscv: ISA string conversion fix and enhancement @ 2022-04-24 5:22 ` Tsukasa OI 0 siblings, 0 replies; 13+ messages in thread From: Tsukasa OI @ 2022-04-24 5:22 UTC (permalink / raw) To: Tsukasa OI, Alistair Francis, Frank Chang; +Cc: qemu-riscv, qemu-devel Hello, There is two issues related to RISC-V ISA extension string I want to be fixed before QEMU 7.1 release. Issue 1 (workaround in PATCH 1): Related: <https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg03726.html> Generating long ISA extension string is definately a good thing to merge. However, it includes two extensions with possibly invalid order: - Zhinx (IEEE754 binary16 arithmetic in GPR) - Zhinxmin (subset of Zhinx, conversion only) This is because: 1. Z* extensions are ordered with the second character by closely related extension category list (RISC-V ISA Manual draft: IMAFDQLCBKJTPV) 2. ... but it doesn't have the character "H" yet I raised this issue on RISC-V ISA Manual GitHub and being discussed: <https://github.com/riscv/riscv-isa-manual/issues/837> Considering software compatibility, "H" is likely placed after "V" (and "N"). I kept single-letter "H" based on this assumption. However, Zhinx and Zhinxmin extensions are not that important because it's incompatible with F and D. That's why I proposing to remove those from ISA extension string generation for now. If "H"-extension ordering is determined, we can safely add Zhinx* extensions again. Note that this patch does not remove extensions. It just disables putting Zhinx* extensions in a DeviceTree entry ("riscv,isa"). Of course, we can alternatively move Zhinx and Zhinxmin before "Svinval" but after "Zve64f", assuming "H" comes after "V". Let me know which might be better. Issue 2 (fixed in PATCH 2): Some operating systems does not correctly parse ISA extension string with version numbers and multi-letter extensions. On Linux, 5.18 is the first version to implement safe parser. However, old Linux kernels are still confused by ISA extension strings (generated by QEMU >= 7.1) containing multi-letter extensions. Much worse, those multi-letter extensions are enabled by default: 1. Zba 2. Zbb 3. Zbc 4. Zbs For instance, existence of "Zbc" can cause problems if we disable compressed instructions ("C" extension). As I searched through, I found this kind of issue on following OSes: - Linux (kernel version 5.17 or earlier) - FreeBSD (at least 14.0-CURRENT) - OpenBSD (at least current development version) I propose a new CPU option "short-isa-string" (default: false), which disables generating ISA extension string with multi-letter extensions. Example: qemu-system-riscv64 ... \ -cpu rv64,h=on,svnapot=on,svinval=on,short-isa-string=on \ ... Without "short-isa-string=on", QEMU generates DeviceTree with following ISA extension string: rv64imafdch_zba_zbb_zbc_zbs_svinval_svnapot With it, QEMU generates following ISA extension string: rv64imafdch Tsukasa OI (2): target/riscv: Tentatively remove Zhinx* from ISA extension string target/riscv: Add short-isa-string option target/riscv/cpu.c | 7 ++++--- target/riscv/cpu.h | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-) base-commit: 754f756cc4c6d9d14b7230c62b5bb20f9d655888 -- 2.32.0 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2] target/riscv: ISA string conversion fix and enhancement @ 2022-04-24 5:22 ` Tsukasa OI 0 siblings, 0 replies; 13+ messages in thread From: Tsukasa OI @ 2022-04-24 5:22 UTC (permalink / raw) To: Tsukasa OI, Alistair Francis, Frank Chang; +Cc: qemu-devel, qemu-riscv Hello, There is two issues related to RISC-V ISA extension string I want to be fixed before QEMU 7.1 release. Issue 1 (workaround in PATCH 1): Related: <https://lists.gnu.org/archive/html/qemu-devel/2022-03/msg03726.html> Generating long ISA extension string is definately a good thing to merge. However, it includes two extensions with possibly invalid order: - Zhinx (IEEE754 binary16 arithmetic in GPR) - Zhinxmin (subset of Zhinx, conversion only) This is because: 1. Z* extensions are ordered with the second character by closely related extension category list (RISC-V ISA Manual draft: IMAFDQLCBKJTPV) 2. ... but it doesn't have the character "H" yet I raised this issue on RISC-V ISA Manual GitHub and being discussed: <https://github.com/riscv/riscv-isa-manual/issues/837> Considering software compatibility, "H" is likely placed after "V" (and "N"). I kept single-letter "H" based on this assumption. However, Zhinx and Zhinxmin extensions are not that important because it's incompatible with F and D. That's why I proposing to remove those from ISA extension string generation for now. If "H"-extension ordering is determined, we can safely add Zhinx* extensions again. Note that this patch does not remove extensions. It just disables putting Zhinx* extensions in a DeviceTree entry ("riscv,isa"). Of course, we can alternatively move Zhinx and Zhinxmin before "Svinval" but after "Zve64f", assuming "H" comes after "V". Let me know which might be better. Issue 2 (fixed in PATCH 2): Some operating systems does not correctly parse ISA extension string with version numbers and multi-letter extensions. On Linux, 5.18 is the first version to implement safe parser. However, old Linux kernels are still confused by ISA extension strings (generated by QEMU >= 7.1) containing multi-letter extensions. Much worse, those multi-letter extensions are enabled by default: 1. Zba 2. Zbb 3. Zbc 4. Zbs For instance, existence of "Zbc" can cause problems if we disable compressed instructions ("C" extension). As I searched through, I found this kind of issue on following OSes: - Linux (kernel version 5.17 or earlier) - FreeBSD (at least 14.0-CURRENT) - OpenBSD (at least current development version) I propose a new CPU option "short-isa-string" (default: false), which disables generating ISA extension string with multi-letter extensions. Example: qemu-system-riscv64 ... \ -cpu rv64,h=on,svnapot=on,svinval=on,short-isa-string=on \ ... Without "short-isa-string=on", QEMU generates DeviceTree with following ISA extension string: rv64imafdch_zba_zbb_zbc_zbs_svinval_svnapot With it, QEMU generates following ISA extension string: rv64imafdch Tsukasa OI (2): target/riscv: Tentatively remove Zhinx* from ISA extension string target/riscv: Add short-isa-string option target/riscv/cpu.c | 7 ++++--- target/riscv/cpu.h | 2 ++ 2 files changed, 6 insertions(+), 3 deletions(-) base-commit: 754f756cc4c6d9d14b7230c62b5bb20f9d655888 -- 2.32.0 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] target/riscv: Tentatively remove Zhinx* from ISA extension string 2022-04-24 5:22 ` Tsukasa OI @ 2022-04-24 5:22 ` Tsukasa OI -1 siblings, 0 replies; 13+ messages in thread From: Tsukasa OI @ 2022-04-24 5:22 UTC (permalink / raw) To: Tsukasa OI, Alistair Francis, Frank Chang; +Cc: qemu-riscv, qemu-devel This commit disables ISA string conversion for Zhinx and Zhinxmin extensions for now. Because extension category ordering of "H" is not ratified, their ordering is likely invalid. Once "H"-extension ordering is determined, we can add Zhinx* again. Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> --- target/riscv/cpu.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 0c774056c5..c765f7ff00 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -954,8 +954,6 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len) ISA_EDATA_ENTRY(zfh, ext_zfh), ISA_EDATA_ENTRY(zfhmin, ext_zfhmin), ISA_EDATA_ENTRY(zfinx, ext_zfinx), - ISA_EDATA_ENTRY(zhinx, ext_zhinx), - ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin), ISA_EDATA_ENTRY(zdinx, ext_zdinx), ISA_EDATA_ENTRY(zba, ext_zba), ISA_EDATA_ENTRY(zbb, ext_zbb), -- 2.32.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/2] target/riscv: Tentatively remove Zhinx* from ISA extension string @ 2022-04-24 5:22 ` Tsukasa OI 0 siblings, 0 replies; 13+ messages in thread From: Tsukasa OI @ 2022-04-24 5:22 UTC (permalink / raw) To: Tsukasa OI, Alistair Francis, Frank Chang; +Cc: qemu-devel, qemu-riscv This commit disables ISA string conversion for Zhinx and Zhinxmin extensions for now. Because extension category ordering of "H" is not ratified, their ordering is likely invalid. Once "H"-extension ordering is determined, we can add Zhinx* again. Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> --- target/riscv/cpu.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index 0c774056c5..c765f7ff00 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -954,8 +954,6 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len) ISA_EDATA_ENTRY(zfh, ext_zfh), ISA_EDATA_ENTRY(zfhmin, ext_zfhmin), ISA_EDATA_ENTRY(zfinx, ext_zfinx), - ISA_EDATA_ENTRY(zhinx, ext_zhinx), - ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin), ISA_EDATA_ENTRY(zdinx, ext_zdinx), ISA_EDATA_ENTRY(zba, ext_zba), ISA_EDATA_ENTRY(zbb, ext_zbb), -- 2.32.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] target/riscv: Tentatively remove Zhinx* from ISA extension string 2022-04-24 5:22 ` Tsukasa OI @ 2022-04-27 23:58 ` Alistair Francis -1 siblings, 0 replies; 13+ messages in thread From: Alistair Francis @ 2022-04-27 23:58 UTC (permalink / raw) To: Tsukasa OI, liweiwei Cc: Frank Chang, open list:RISC-V, qemu-devel@nongnu.org Developers On Sun, Apr 24, 2022 at 3:22 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: > > This commit disables ISA string conversion for Zhinx and Zhinxmin > extensions for now. Because extension category ordering of "H" is not > ratified, their ordering is likely invalid. > > Once "H"-extension ordering is determined, we can add Zhinx* again. > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> Weiwei Li does this sound alright to you? Alistair > --- > target/riscv/cpu.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 0c774056c5..c765f7ff00 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -954,8 +954,6 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len) > ISA_EDATA_ENTRY(zfh, ext_zfh), > ISA_EDATA_ENTRY(zfhmin, ext_zfhmin), > ISA_EDATA_ENTRY(zfinx, ext_zfinx), > - ISA_EDATA_ENTRY(zhinx, ext_zhinx), > - ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin), > ISA_EDATA_ENTRY(zdinx, ext_zdinx), > ISA_EDATA_ENTRY(zba, ext_zba), > ISA_EDATA_ENTRY(zbb, ext_zbb), > -- > 2.32.0 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] target/riscv: Tentatively remove Zhinx* from ISA extension string @ 2022-04-27 23:58 ` Alistair Francis 0 siblings, 0 replies; 13+ messages in thread From: Alistair Francis @ 2022-04-27 23:58 UTC (permalink / raw) To: Tsukasa OI, liweiwei Cc: Frank Chang, qemu-devel@nongnu.org Developers, open list:RISC-V On Sun, Apr 24, 2022 at 3:22 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: > > This commit disables ISA string conversion for Zhinx and Zhinxmin > extensions for now. Because extension category ordering of "H" is not > ratified, their ordering is likely invalid. > > Once "H"-extension ordering is determined, we can add Zhinx* again. > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> Weiwei Li does this sound alright to you? Alistair > --- > target/riscv/cpu.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 0c774056c5..c765f7ff00 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -954,8 +954,6 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len) > ISA_EDATA_ENTRY(zfh, ext_zfh), > ISA_EDATA_ENTRY(zfhmin, ext_zfhmin), > ISA_EDATA_ENTRY(zfinx, ext_zfinx), > - ISA_EDATA_ENTRY(zhinx, ext_zhinx), > - ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin), > ISA_EDATA_ENTRY(zdinx, ext_zdinx), > ISA_EDATA_ENTRY(zba, ext_zba), > ISA_EDATA_ENTRY(zbb, ext_zbb), > -- > 2.32.0 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] target/riscv: Tentatively remove Zhinx* from ISA extension string 2022-04-27 23:58 ` Alistair Francis @ 2022-04-28 2:39 ` Weiwei Li -1 siblings, 0 replies; 13+ messages in thread From: Weiwei Li @ 2022-04-28 2:39 UTC (permalink / raw) To: Alistair Francis, Tsukasa OI Cc: Frank Chang, open list:RISC-V, qemu-devel@nongnu.org Developers 在 2022/4/28 上午7:58, Alistair Francis 写道: > On Sun, Apr 24, 2022 at 3:22 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: >> This commit disables ISA string conversion for Zhinx and Zhinxmin >> extensions for now. Because extension category ordering of "H" is not >> ratified, their ordering is likely invalid. >> >> Once "H"-extension ordering is determined, we can add Zhinx* again. >> >> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> > Weiwei Li does this sound alright to you? > > Alistair Even though the rule says: "The first letter following the 'Z' conventionally indicates the most closely related alphabetical extension category, IMAFDQLCBKJTPVH", zhinx* is not related to 'H' extension actually. I think the most closely related alphabetical extension is 'F' extension. Regards, Weiwei Li >> --- >> target/riscv/cpu.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 0c774056c5..c765f7ff00 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -954,8 +954,6 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len) >> ISA_EDATA_ENTRY(zfh, ext_zfh), >> ISA_EDATA_ENTRY(zfhmin, ext_zfhmin), >> ISA_EDATA_ENTRY(zfinx, ext_zfinx), >> - ISA_EDATA_ENTRY(zhinx, ext_zhinx), >> - ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin), >> ISA_EDATA_ENTRY(zdinx, ext_zdinx), >> ISA_EDATA_ENTRY(zba, ext_zba), >> ISA_EDATA_ENTRY(zbb, ext_zbb), >> -- >> 2.32.0 >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] target/riscv: Tentatively remove Zhinx* from ISA extension string @ 2022-04-28 2:39 ` Weiwei Li 0 siblings, 0 replies; 13+ messages in thread From: Weiwei Li @ 2022-04-28 2:39 UTC (permalink / raw) To: Alistair Francis, Tsukasa OI Cc: Frank Chang, qemu-devel@nongnu.org Developers, open list:RISC-V 在 2022/4/28 上午7:58, Alistair Francis 写道: > On Sun, Apr 24, 2022 at 3:22 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: >> This commit disables ISA string conversion for Zhinx and Zhinxmin >> extensions for now. Because extension category ordering of "H" is not >> ratified, their ordering is likely invalid. >> >> Once "H"-extension ordering is determined, we can add Zhinx* again. >> >> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> > Weiwei Li does this sound alright to you? > > Alistair Even though the rule says: "The first letter following the 'Z' conventionally indicates the most closely related alphabetical extension category, IMAFDQLCBKJTPVH", zhinx* is not related to 'H' extension actually. I think the most closely related alphabetical extension is 'F' extension. Regards, Weiwei Li >> --- >> target/riscv/cpu.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 0c774056c5..c765f7ff00 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -954,8 +954,6 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len) >> ISA_EDATA_ENTRY(zfh, ext_zfh), >> ISA_EDATA_ENTRY(zfhmin, ext_zfhmin), >> ISA_EDATA_ENTRY(zfinx, ext_zfinx), >> - ISA_EDATA_ENTRY(zhinx, ext_zhinx), >> - ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin), >> ISA_EDATA_ENTRY(zdinx, ext_zdinx), >> ISA_EDATA_ENTRY(zba, ext_zba), >> ISA_EDATA_ENTRY(zbb, ext_zbb), >> -- >> 2.32.0 >> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] target/riscv: Tentatively remove Zhinx* from ISA extension string 2022-04-28 2:39 ` Weiwei Li (?) @ 2022-05-10 11:25 ` Tsukasa OI -1 siblings, 0 replies; 13+ messages in thread From: Tsukasa OI @ 2022-05-10 11:25 UTC (permalink / raw) To: Weiwei Li, Alistair Francis Cc: Frank Chang, qemu-devel@nongnu.org Developers, open list:RISC-V On 2022/04/28 11:39, Weiwei Li wrote: > > 在 2022/4/28 上午7:58, Alistair Francis 写道: >> On Sun, Apr 24, 2022 at 3:22 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: >>> This commit disables ISA string conversion for Zhinx and Zhinxmin >>> extensions for now. Because extension category ordering of "H" is not >>> ratified, their ordering is likely invalid. >>> >>> Once "H"-extension ordering is determined, we can add Zhinx* again. >>> >>> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> >> Weiwei Li does this sound alright to you? >> >> Alistair > > Even though the rule says: "The first letter following the 'Z' conventionally indicates the most closely > > related alphabetical extension category, IMAFDQLCBKJTPVH", zhinx* is not related to 'H' extension actually. > > I think the most closely related alphabetical extension is 'F' extension. > > Regards, > > Weiwei Li I don't quite agree that. Although Zhinx* extensions are **functionally** related to 'F' (rather than 'H'), that's not how canonical ordering of Z* extensions works, at least this is not how canonical ordering is implemented in GNU/LLVM toolchains. Both toolchains orders Z* multi-letter extensions by second "character"- first manner. My interpretation is, Z* multi-letter extensions should be ordered by category character (the second one) first. Yes, it should have reflected the most closely related single-letter extension, but it didn't happend on Zhinx* extensions. Thanks, Tsukasa > >>> --- >>> target/riscv/cpu.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>> index 0c774056c5..c765f7ff00 100644 >>> --- a/target/riscv/cpu.c >>> +++ b/target/riscv/cpu.c >>> @@ -954,8 +954,6 @@ static void riscv_isa_string_ext(RISCVCPU *cpu, char **isa_str, int max_str_len) >>> ISA_EDATA_ENTRY(zfh, ext_zfh), >>> ISA_EDATA_ENTRY(zfhmin, ext_zfhmin), >>> ISA_EDATA_ENTRY(zfinx, ext_zfinx), >>> - ISA_EDATA_ENTRY(zhinx, ext_zhinx), >>> - ISA_EDATA_ENTRY(zhinxmin, ext_zhinxmin), >>> ISA_EDATA_ENTRY(zdinx, ext_zdinx), >>> ISA_EDATA_ENTRY(zba, ext_zba), >>> ISA_EDATA_ENTRY(zbb, ext_zbb), >>> -- >>> 2.32.0 >>> > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] target/riscv: Add short-isa-string option 2022-04-24 5:22 ` Tsukasa OI @ 2022-04-24 5:22 ` Tsukasa OI -1 siblings, 0 replies; 13+ messages in thread From: Tsukasa OI @ 2022-04-24 5:22 UTC (permalink / raw) To: Tsukasa OI, Alistair Francis, Frank Chang; +Cc: qemu-riscv, qemu-devel Because some operating systems don't correctly parse long ISA extension string, this commit adds short-isa-string boolean option to disable generating long ISA extension strings on Device Tree. Operating Systems which short-isa-string might be helpful: 1. Linux (5.17 or earlier) 2. FreeBSD (at least 14.0-CURRENT) 3. OpenBSD (at least current development version) Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> --- target/riscv/cpu.c | 5 ++++- target/riscv/cpu.h | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index c765f7ff00..9718cd0e7e 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -834,6 +834,8 @@ static Property riscv_cpu_properties[] = { DEFINE_PROP_BOOL("x-aia", RISCVCPU, cfg.aia, false), DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC), + + DEFINE_PROP_BOOL("short-isa-string", RISCVCPU, cfg.short_isa_string, false), DEFINE_PROP_END_OF_LIST(), }; @@ -989,7 +991,8 @@ char *riscv_isa_string(RISCVCPU *cpu) } } *p = '\0'; - riscv_isa_string_ext(cpu, &isa_str, maxlen); + if (!cpu->cfg.short_isa_string) + riscv_isa_string_ext(cpu, &isa_str, maxlen); return isa_str; } diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 34c22d5d3b..5b7fe32218 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -408,6 +408,8 @@ struct RISCVCPUConfig { bool aia; bool debug; uint64_t resetvec; + + bool short_isa_string; }; typedef struct RISCVCPUConfig RISCVCPUConfig; -- 2.32.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] target/riscv: Add short-isa-string option @ 2022-04-24 5:22 ` Tsukasa OI 0 siblings, 0 replies; 13+ messages in thread From: Tsukasa OI @ 2022-04-24 5:22 UTC (permalink / raw) To: Tsukasa OI, Alistair Francis, Frank Chang; +Cc: qemu-devel, qemu-riscv Because some operating systems don't correctly parse long ISA extension string, this commit adds short-isa-string boolean option to disable generating long ISA extension strings on Device Tree. Operating Systems which short-isa-string might be helpful: 1. Linux (5.17 or earlier) 2. FreeBSD (at least 14.0-CURRENT) 3. OpenBSD (at least current development version) Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> --- target/riscv/cpu.c | 5 ++++- target/riscv/cpu.h | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c index c765f7ff00..9718cd0e7e 100644 --- a/target/riscv/cpu.c +++ b/target/riscv/cpu.c @@ -834,6 +834,8 @@ static Property riscv_cpu_properties[] = { DEFINE_PROP_BOOL("x-aia", RISCVCPU, cfg.aia, false), DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC), + + DEFINE_PROP_BOOL("short-isa-string", RISCVCPU, cfg.short_isa_string, false), DEFINE_PROP_END_OF_LIST(), }; @@ -989,7 +991,8 @@ char *riscv_isa_string(RISCVCPU *cpu) } } *p = '\0'; - riscv_isa_string_ext(cpu, &isa_str, maxlen); + if (!cpu->cfg.short_isa_string) + riscv_isa_string_ext(cpu, &isa_str, maxlen); return isa_str; } diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 34c22d5d3b..5b7fe32218 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -408,6 +408,8 @@ struct RISCVCPUConfig { bool aia; bool debug; uint64_t resetvec; + + bool short_isa_string; }; typedef struct RISCVCPUConfig RISCVCPUConfig; -- 2.32.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] target/riscv: Add short-isa-string option 2022-04-24 5:22 ` Tsukasa OI (?) @ 2022-05-09 9:51 ` Alistair Francis 2022-05-10 11:20 ` Tsukasa OI -1 siblings, 1 reply; 13+ messages in thread From: Alistair Francis @ 2022-05-09 9:51 UTC (permalink / raw) To: Tsukasa OI Cc: Frank Chang, qemu-devel@nongnu.org Developers, open list:RISC-V On Sun, Apr 24, 2022 at 7:22 AM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: > > Because some operating systems don't correctly parse long ISA extension > string, this commit adds short-isa-string boolean option to disable > generating long ISA extension strings on Device Tree. > > Operating Systems which short-isa-string might be helpful: > > 1. Linux (5.17 or earlier) > 2. FreeBSD (at least 14.0-CURRENT) > 3. OpenBSD (at least current development version) > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> > --- > target/riscv/cpu.c | 5 ++++- > target/riscv/cpu.h | 2 ++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index c765f7ff00..9718cd0e7e 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -834,6 +834,8 @@ static Property riscv_cpu_properties[] = { > DEFINE_PROP_BOOL("x-aia", RISCVCPU, cfg.aia, false), > > DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC), > + > + DEFINE_PROP_BOOL("short-isa-string", RISCVCPU, cfg.short_isa_string, false), > DEFINE_PROP_END_OF_LIST(), > }; > > @@ -989,7 +991,8 @@ char *riscv_isa_string(RISCVCPU *cpu) > } > } > *p = '\0'; > - riscv_isa_string_ext(cpu, &isa_str, maxlen); > + if (!cpu->cfg.short_isa_string) > + riscv_isa_string_ext(cpu, &isa_str, maxlen); I don't love this, the long strings are part of the ISA, it seems strange to add an option to disable them. Can you provide more details on what this breaks? Alistair > return isa_str; > } > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 34c22d5d3b..5b7fe32218 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -408,6 +408,8 @@ struct RISCVCPUConfig { > bool aia; > bool debug; > uint64_t resetvec; > + > + bool short_isa_string; > }; > > typedef struct RISCVCPUConfig RISCVCPUConfig; > -- > 2.32.0 > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] target/riscv: Add short-isa-string option 2022-05-09 9:51 ` Alistair Francis @ 2022-05-10 11:20 ` Tsukasa OI 0 siblings, 0 replies; 13+ messages in thread From: Tsukasa OI @ 2022-05-10 11:20 UTC (permalink / raw) To: Alistair Francis Cc: Frank Chang, qemu-devel@nongnu.org Developers, open list:RISC-V On 2022/05/09 18:51, Alistair Francis wrote: > On Sun, Apr 24, 2022 at 7:22 AM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: >> >> Because some operating systems don't correctly parse long ISA extension >> string, this commit adds short-isa-string boolean option to disable >> generating long ISA extension strings on Device Tree. >> >> Operating Systems which short-isa-string might be helpful: >> >> 1. Linux (5.17 or earlier) >> 2. FreeBSD (at least 14.0-CURRENT) >> 3. OpenBSD (at least current development version) >> >> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> >> --- >> target/riscv/cpu.c | 5 ++++- >> target/riscv/cpu.h | 2 ++ >> 2 files changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index c765f7ff00..9718cd0e7e 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -834,6 +834,8 @@ static Property riscv_cpu_properties[] = { >> DEFINE_PROP_BOOL("x-aia", RISCVCPU, cfg.aia, false), >> >> DEFINE_PROP_UINT64("resetvec", RISCVCPU, cfg.resetvec, DEFAULT_RSTVEC), >> + >> + DEFINE_PROP_BOOL("short-isa-string", RISCVCPU, cfg.short_isa_string, false), >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> @@ -989,7 +991,8 @@ char *riscv_isa_string(RISCVCPU *cpu) >> } >> } >> *p = '\0'; >> - riscv_isa_string_ext(cpu, &isa_str, maxlen); >> + if (!cpu->cfg.short_isa_string) >> + riscv_isa_string_ext(cpu, &isa_str, maxlen); > > I don't love this, the long strings are part of the ISA, it seems > strange to add an option to disable them. > > Can you provide more details on what this breaks? > > Alistair I don't like it either but I think this is necessary for (at least) a few years (as a workaround). Images for testing: <https://a4lg.com/downloads/archives/tmp/2022-05-10/qemu-issue-reproduction-20220510.tar.xz> Use latest (development version of) QEMU to reproduce. - Linux 5.15 (FPU support enabled) - Busybox 1.35.0 (use of FPU disabled, -march=rv64imac -mabi=lp64) Config 1. `-cpu rv64,g=on,f=on,d=on,zfinx=off,zdinx=off' This is generic RV64. ISA string is "rv64imafdch_zba_zbb_zbc_zbs". With this ISA, it works. ...Actually, it misunderstands Zbc extension as `Z', `B' and `C' extensions (which might cause problems on other configurations) in Linux 5.15 but... not now. Config 2. `-cpu rv64,g=off,f=off,d=off,zfinx=on,zdinx=on' This is generic RV64 but with floating point using GPRs (Zfinx and Zdinx). ISA string is "rv64imach_zfinx_zdinx_zba_zbb_zbc_zbs". OK, this is the problem. If you try to run userland (Busybox-based), it crashes on __fstate_restore function in kernel. [ 0.619174] Oops - illegal instruction [#1] [ 0.619544] Modules linked in: [ 0.619913] CPU: 0 PID: 1 Comm: init Not tainted 5.15.0 #47 [ 0.620594] Hardware name: riscv-virtio,qemu (DT) [ 0.621142] epc : __fstate_restore+0x12/0x8c [ 0.621858] ra : start_thread+0x28/0x5a [ 0.623463] epc : ffffffff80005332 ra : ffffffff80003352 sp : ffffffd00060bc90 [ 0.624291] gp : ffffffff812e6e38 tp : ffffffe001630000 t0 : 0000000000000000 [ 0.625194] t1 : 0000000000006000 t2 : 0000000000000000 s0 : ffffffd00060bcc0 [ 0.626448] s1 : ffffffd00060bee0 a0 : ffffffe001630900 a1 : 000000000001054c [ 0.627431] a2 : 0000000000000900 a3 : 0000000000000000 a4 : 0000000000000000 [ 0.627983] a5 : 0000000000002020 a6 : 000000000000000c a7 : 0000000000000000 [ 0.629473] s2 : 0000003ff4473e10 s3 : 000000000001054c s4 : 0000003ff4473ff2 [ 0.630798] s5 : 0000003ffffffff8 s6 : 000000000001054c s7 : 0000000000040000 [ 0.631623] s8 : 0000003ff4473e38 s9 : 0000003ff4473e38 s10: ffffffe002083600 [ 0.632310] s11: ffffffe002070000 t3 : 000000000000000e t4 : 0000000000000000 [ 0.633080] t5 : 0000000000000180 t6 : 0000000000040000 [ 0.633648] status: 0000000200000120 badaddr: 0000000000053007 cause: 0000000000000002 [ 0.635025] [<ffffffff80005332>] __fstate_restore+0x12/0x8c [ 0.635771] [<ffffffff8017eb1a>] load_elf_binary+0xe16/0xe4a [ 0.636149] [<ffffffff8012c97a>] bprm_execve+0x1e4/0x468 [ 0.636603] [<ffffffff8012d646>] kernel_execve+0xdc/0x142 [ 0.636943] [<ffffffff80709158>] run_init_process+0x90/0x9e [ 0.637493] [<ffffffff807136a2>] kernel_init+0x72/0x104 [ 0.638390] [<ffffffff80003008>] ret_from_exception+0x0/0xc [ 0.639513] ---[ end trace e4dec1a155401c43 ]--- [ 0.640489] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b Apparently, it crashes as follows: 1. Linux (-5.17) misunderstands `Zfinx' and `Zdinx' extensions as I, F, D, N, X and Z single-letter extensions and thinks FPU with dedicated registers is there. 2. Because of that, the kernel tries to initialize FP registers from memory using `fld' instruction but this is a part of `D' extension, not `Zdinx'. 3. Illegal instruction trap is generated and the kernel panics. As you can see, many operating systems currently in use still don't correctly understand long ISA strings: >> 1. Linux (5.17 or earlier) >> 2. FreeBSD (at least 14.0-CURRENT) >> 3. OpenBSD (at least current development version) ...and it affects in-kernel behavior directly! That means, we still need something to prevent multi-letter extension names from appearing in "riscv,isa" DeviceTree ISA string. That's the purpose of this option. I am preparing for PATCH v2 (which "moves" Zhinx*, instead of removing) so please wait for it (this commit will be unchanged but will reflect your comment). Tsukasa > >> return isa_str; >> } >> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 34c22d5d3b..5b7fe32218 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -408,6 +408,8 @@ struct RISCVCPUConfig { >> bool aia; >> bool debug; >> uint64_t resetvec; >> + >> + bool short_isa_string; >> }; >> >> typedef struct RISCVCPUConfig RISCVCPUConfig; >> -- >> 2.32.0 >> > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-05-10 11:27 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-24 5:22 [PATCH 0/2] target/riscv: ISA string conversion fix and enhancement Tsukasa OI 2022-04-24 5:22 ` Tsukasa OI 2022-04-24 5:22 ` [PATCH 1/2] target/riscv: Tentatively remove Zhinx* from ISA extension string Tsukasa OI 2022-04-24 5:22 ` Tsukasa OI 2022-04-27 23:58 ` Alistair Francis 2022-04-27 23:58 ` Alistair Francis 2022-04-28 2:39 ` Weiwei Li 2022-04-28 2:39 ` Weiwei Li 2022-05-10 11:25 ` Tsukasa OI 2022-04-24 5:22 ` [PATCH 2/2] target/riscv: Add short-isa-string option Tsukasa OI 2022-04-24 5:22 ` Tsukasa OI 2022-05-09 9:51 ` Alistair Francis 2022-05-10 11:20 ` Tsukasa OI
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.