* [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
* [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 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 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
* 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
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.