target/riscv/cpu.c | 62 +++++++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 17 deletions(-)
Hello, This is another patchset for RISC-V ISA extension / feature handling. Aside from coding style fix / refactoring patch (PATCH 1 and 5), this patchset contains two changes: 1. "G" extension handling 1.A. "G" extension expansion (PATCH 3) On ISA version 20190608 or later, "G" expands to "IMAFD_Zicsr_Zifencei", not just "IMAFD" (this is because "Zicsr" and "Zifencei" extensions are splitted from "I"). Because both "Zicsr" and "Zifencei" are enabled by default, it should be safe to change "G" extension expansion. 1.B. Disable "G" by default (PATCH 2) This seems quite odd but I have a good reason. While "G" is enabled by default, all "I", "M", "A", "F", "D", "Zicsr" and "Zifencei" are also enabled by default, making default "G" expansion useless. There's more. If we want to change detailed configuration of a RV32/RV64 CPU and want to disable some, for example, "F" and "D", we must also disable "G". This is obviously bloat. This doesn't work: -cpu rv64,f=off,d=off This does work (but bloat): -cpu rv64,g=off,f=off,d=off Disabling "G" suppresses such problem and mostly harmless, too. 2. Floating point arithmetic consistency (PATCH 4) With -cpu option, we can configure details of RISC-V CPU emulated by QEMU. However, we can set some invalid combinations that would make FP arithmetic effectively unusable (and invalid on RISC-V ISA specification). - F requires Zicsr - Zfinx requires Zicsr - Zfh/Zfhmin require F - D requires F - V requires D Reproducing this kind of problem requires manually disabling certain extensions (because all "Zicsr", "F" and "D" are enabled by default). So, I consider that just making an error message (when unsatisfied) should be enough, not implying related extensions like "zk*" properties. Note that this checking only works on any, rv32 and rv64. On other CPUs (for example, sifive-u54), it sets nonzero `misa' value on corresponding `set_misa' call (c.f. in rv64_sifive_u_cpu_init in target/riscv/cpu.c) and consistency checks are skipped (because nonzero `misa' value is set prior to RISC-V CPU realization process). I think we generally use generic "rv32" or "rv64" on heavy customizing so I don't think this is not a big problem. Still, we could fix this later (e.g. by setting properties on CPU init function or by checking some consistency problems even if previously-set `misa' is nonzero). Tsukasa OI (5): target/riscv: Fix "G" extension expansion typing target/riscv: Disable "G" by default target/riscv: Change "G" expansion target/riscv: FP extension requirements target/riscv: Move/refactor ISA extension checks target/riscv/cpu.c | 62 +++++++++++++++++++++++++++++++++------------- 1 file changed, 45 insertions(+), 17 deletions(-) base-commit: 178bacb66d98d9ee7a702b9f2a4dfcd88b72a9ab -- 2.34.1
On Fri, May 13, 2022 at 7:46 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: > > Hello, > > This is another patchset for RISC-V ISA extension / feature handling. > > Aside from coding style fix / refactoring patch (PATCH 1 and 5), this > patchset contains two changes: > > > > 1. "G" extension handling > > 1.A. "G" extension expansion (PATCH 3) > > On ISA version 20190608 or later, "G" expands to "IMAFD_Zicsr_Zifencei", > not just "IMAFD" (this is because "Zicsr" and "Zifencei" extensions are > splitted from "I"). Because both "Zicsr" and "Zifencei" are enabled by > default, it should be safe to change "G" extension expansion. > > 1.B. Disable "G" by default (PATCH 2) > > This seems quite odd but I have a good reason. While "G" is enabled by > default, all "I", "M", "A", "F", "D", "Zicsr" and "Zifencei" are also > enabled by default, making default "G" expansion useless. > > There's more. If we want to change detailed configuration of a RV32/RV64 > CPU and want to disable some, for example, "F" and "D", we must also > disable "G". This is obviously bloat. > > This doesn't work: > -cpu rv64,f=off,d=off > > This does work (but bloat): > -cpu rv64,g=off,f=off,d=off > > Disabling "G" suppresses such problem and mostly harmless, too. > > > > 2. Floating point arithmetic consistency (PATCH 4) > > With -cpu option, we can configure details of RISC-V CPU emulated by QEMU. > However, we can set some invalid combinations that would make FP arithmetic > effectively unusable (and invalid on RISC-V ISA specification). > > - F requires Zicsr > - Zfinx requires Zicsr > - Zfh/Zfhmin require F > - D requires F > - V requires D > > Reproducing this kind of problem requires manually disabling certain > extensions (because all "Zicsr", "F" and "D" are enabled by default). So, > I consider that just making an error message (when unsatisfied) should be > enough, not implying related extensions like "zk*" properties. > > > Note that this checking only works on any, rv32 and rv64. On other CPUs > (for example, sifive-u54), it sets nonzero `misa' value on corresponding > `set_misa' call (c.f. in rv64_sifive_u_cpu_init in target/riscv/cpu.c) and > consistency checks are skipped (because nonzero `misa' value is set prior > to RISC-V CPU realization process). > > I think we generally use generic "rv32" or "rv64" on heavy customizing so I > don't think this is not a big problem. Still, we could fix this later > (e.g. by setting properties on CPU init function or by checking some > consistency problems even if previously-set `misa' is nonzero). > > > > > Tsukasa OI (5): > target/riscv: Fix "G" extension expansion typing > target/riscv: Disable "G" by default > target/riscv: Change "G" expansion > target/riscv: FP extension requirements > target/riscv: Move/refactor ISA extension checks Thanks! Applied to riscv-to-apply.next Alistair > > target/riscv/cpu.c | 62 +++++++++++++++++++++++++++++++++------------- > 1 file changed, 45 insertions(+), 17 deletions(-) > > > base-commit: 178bacb66d98d9ee7a702b9f2a4dfcd88b72a9ab > -- > 2.34.1 >
c.f. <https://lists.gnu.org/archive/html/qemu-riscv/2022-05/msg00221.html> I was obviously drunk when finalizing PATCH v1. [BUGS in PATCH v1 (fixed in v2)] PATCH 1: My English was (or, "is"?) broken (commit subject and message is rewritten) PATCH 4: Zfinx requirement test were in the wrong place PATCH 5: Zfinx/F exclusivity test was completely wrong I meant Zfinx&&F but when finalizing my patchset, I somehow changed this place to Zfinx&&!F. Tsukasa OI (5): target/riscv: Fix coding style on "G" expansion target/riscv: Disable "G" by default target/riscv: Change "G" expansion target/riscv: FP extension requirements target/riscv: Move/refactor ISA extension checks target/riscv/cpu.c | 63 +++++++++++++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 17 deletions(-) base-commit: 178bacb66d98d9ee7a702b9f2a4dfcd88b72a9ab -- 2.34.1
Because ext_? members are boolean variables, operator `&&' should be
used instead of `&'.
Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
target/riscv/cpu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ccacdee215..00bf26ec8b 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -596,8 +596,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
return;
}
- if (cpu->cfg.ext_g && !(cpu->cfg.ext_i & cpu->cfg.ext_m &
- cpu->cfg.ext_a & cpu->cfg.ext_f &
+ if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
+ cpu->cfg.ext_a && cpu->cfg.ext_f &&
cpu->cfg.ext_d)) {
warn_report("Setting G will also set IMAFD");
cpu->cfg.ext_i = true;
--
2.34.1
On Sun, May 15, 2022 at 12:56 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: > > Because ext_? members are boolean variables, operator `&&' should be > used instead of `&'. > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/cpu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index ccacdee215..00bf26ec8b 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -596,8 +596,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > return; > } > > - if (cpu->cfg.ext_g && !(cpu->cfg.ext_i & cpu->cfg.ext_m & > - cpu->cfg.ext_a & cpu->cfg.ext_f & > + if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m && > + cpu->cfg.ext_a && cpu->cfg.ext_f && > cpu->cfg.ext_d)) { > warn_report("Setting G will also set IMAFD"); > cpu->cfg.ext_i = true; > -- > 2.34.1 >
On 14/05/2022 23:56, Tsukasa OI wrote: > [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de e-mail suspeito entre imediatamente em contato com o DTI. > > Because ext_? members are boolean variables, operator `&&' should be > used instead of `&'. > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> > --- > target/riscv/cpu.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index ccacdee215..00bf26ec8b 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -596,8 +596,8 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > return; > } > > - if (cpu->cfg.ext_g && !(cpu->cfg.ext_i & cpu->cfg.ext_m & > - cpu->cfg.ext_a & cpu->cfg.ext_f & > + if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m && > + cpu->cfg.ext_a && cpu->cfg.ext_f && > cpu->cfg.ext_d)) { > warn_report("Setting G will also set IMAFD"); > cpu->cfg.ext_i = true; > -- > 2.34.1 > > Sorry, looks like I mistakenly reviewed v1 earlier Reviewed-by: Víctor Colombo <victor.colombo@eldorado.org.br> -- Víctor Cora Colombo Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
Because "G" virtual extension expands to "IMAFD", we cannot separately
disable extensions like "F" or "D" without disabling "G". Because all
"IMAFD" are enabled by default, it's harmless to disable "G" by default.
Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
target/riscv/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 00bf26ec8b..3ea68d5cd7 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -812,7 +812,7 @@ static Property riscv_cpu_properties[] = {
/* Defaults for standard extensions */
DEFINE_PROP_BOOL("i", RISCVCPU, cfg.ext_i, true),
DEFINE_PROP_BOOL("e", RISCVCPU, cfg.ext_e, false),
- DEFINE_PROP_BOOL("g", RISCVCPU, cfg.ext_g, true),
+ DEFINE_PROP_BOOL("g", RISCVCPU, cfg.ext_g, false),
DEFINE_PROP_BOOL("m", RISCVCPU, cfg.ext_m, true),
DEFINE_PROP_BOOL("a", RISCVCPU, cfg.ext_a, true),
DEFINE_PROP_BOOL("f", RISCVCPU, cfg.ext_f, true),
--
2.34.1
On 14/05/2022 23:56, Tsukasa OI wrote: > Because "G" virtual extension expands to "IMAFD", we cannot separately > disable extensions like "F" or "D" without disabling "G". Because all > "IMAFD" are enabled by default, it's harmless to disable "G" by default. > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> > --- > target/riscv/cpu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 00bf26ec8b..3ea68d5cd7 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -812,7 +812,7 @@ static Property riscv_cpu_properties[] = { > /* Defaults for standard extensions */ > DEFINE_PROP_BOOL("i", RISCVCPU, cfg.ext_i, true), > DEFINE_PROP_BOOL("e", RISCVCPU, cfg.ext_e, false), > - DEFINE_PROP_BOOL("g", RISCVCPU, cfg.ext_g, true), > + DEFINE_PROP_BOOL("g", RISCVCPU, cfg.ext_g, false), > DEFINE_PROP_BOOL("m", RISCVCPU, cfg.ext_m, true), > DEFINE_PROP_BOOL("a", RISCVCPU, cfg.ext_a, true), > DEFINE_PROP_BOOL("f", RISCVCPU, cfg.ext_f, true), > -- > 2.34.1 > > I think the logic looks ok, and (with my limited understanding of the code) I agree on the reasoning for the changes in patches 2 and 3. Just some clarification needed: where is the value of 'g' checked? can the behavior change in this patch cause a situation where IMAFD_Zicsr_Zifencei is set but 'g' is not, or does patch 3 guarantee that in this case 'g' will be set? Thanks! -- Víctor Cora Colombo Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
On 2022/05/17 3:04, Víctor Colombo wrote: > On 14/05/2022 23:56, Tsukasa OI wrote: >> Because "G" virtual extension expands to "IMAFD", we cannot separately >> disable extensions like "F" or "D" without disabling "G". Because all >> "IMAFD" are enabled by default, it's harmless to disable "G" by default. >> >> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> >> --- >> target/riscv/cpu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 00bf26ec8b..3ea68d5cd7 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -812,7 +812,7 @@ static Property riscv_cpu_properties[] = { >> /* Defaults for standard extensions */ >> DEFINE_PROP_BOOL("i", RISCVCPU, cfg.ext_i, true), >> DEFINE_PROP_BOOL("e", RISCVCPU, cfg.ext_e, false), >> - DEFINE_PROP_BOOL("g", RISCVCPU, cfg.ext_g, true), >> + DEFINE_PROP_BOOL("g", RISCVCPU, cfg.ext_g, false), >> DEFINE_PROP_BOOL("m", RISCVCPU, cfg.ext_m, true), >> DEFINE_PROP_BOOL("a", RISCVCPU, cfg.ext_a, true), >> DEFINE_PROP_BOOL("f", RISCVCPU, cfg.ext_f, true), >> -- >> 2.34.1 >> >> > > I think the logic looks ok, and (with my limited understanding of the > code) I agree on the reasoning for the changes in patches 2 and 3. > Just some clarification needed: where is the value of 'g' checked? > can the behavior change in this patch cause a situation where > IMAFD_Zicsr_Zifencei is set but 'g' is not, or does patch 3 > guarantee that in this case 'g' will be set? > > > Thanks! > Probably too late to answer but on Alistair's riscv-to-apply.next tree... target/riscv/cpu.c (19f13a9) line 599-611: if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m && cpu->cfg.ext_a && cpu->cfg.ext_f && cpu->cfg.ext_d && cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) { warn_report("Setting G will also set IMAFD_Zicsr_Zifencei"); cpu->cfg.ext_i = true; cpu->cfg.ext_m = true; cpu->cfg.ext_a = true; cpu->cfg.ext_f = true; cpu->cfg.ext_d = true; cpu->cfg.ext_icsr = true; cpu->cfg.ext_ifencei = true; } This is the only place where "G" (ext_g) is read. Here, if G is enabled and not all IMAFD_Zicsr_Zifencei are enabled, it enables them with a warning message. So, even if "G" is disabled alone, it's harmless. Note that IMAFD_Zicsr_Zifencei are enabled by default. Thanks, Tsukasa
On 24/05/2022 06:07, Tsukasa OI wrote: > On 2022/05/17 3:04, Víctor Colombo wrote: >> On 14/05/2022 23:56, Tsukasa OI wrote: >>> Because "G" virtual extension expands to "IMAFD", we cannot separately >>> disable extensions like "F" or "D" without disabling "G". Because all >>> "IMAFD" are enabled by default, it's harmless to disable "G" by default. >>> >>> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> >>> --- >>> target/riscv/cpu.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>> index 00bf26ec8b..3ea68d5cd7 100644 >>> --- a/target/riscv/cpu.c >>> +++ b/target/riscv/cpu.c >>> @@ -812,7 +812,7 @@ static Property riscv_cpu_properties[] = { >>> /* Defaults for standard extensions */ >>> DEFINE_PROP_BOOL("i", RISCVCPU, cfg.ext_i, true), >>> DEFINE_PROP_BOOL("e", RISCVCPU, cfg.ext_e, false), >>> - DEFINE_PROP_BOOL("g", RISCVCPU, cfg.ext_g, true), >>> + DEFINE_PROP_BOOL("g", RISCVCPU, cfg.ext_g, false), >>> DEFINE_PROP_BOOL("m", RISCVCPU, cfg.ext_m, true), >>> DEFINE_PROP_BOOL("a", RISCVCPU, cfg.ext_a, true), >>> DEFINE_PROP_BOOL("f", RISCVCPU, cfg.ext_f, true), >>> -- >>> 2.34.1 >>> >>> >> >> I think the logic looks ok, and (with my limited understanding of the >> code) I agree on the reasoning for the changes in patches 2 and 3. >> Just some clarification needed: where is the value of 'g' checked? >> can the behavior change in this patch cause a situation where >> IMAFD_Zicsr_Zifencei is set but 'g' is not, or does patch 3 >> guarantee that in this case 'g' will be set? >> >> >> Thanks! >> > > Probably too late to answer but on Alistair's riscv-to-apply.next tree... > > target/riscv/cpu.c (19f13a9) line 599-611: > if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m && > cpu->cfg.ext_a && cpu->cfg.ext_f && > cpu->cfg.ext_d && > cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) { > warn_report("Setting G will also set IMAFD_Zicsr_Zifencei"); > cpu->cfg.ext_i = true; > cpu->cfg.ext_m = true; > cpu->cfg.ext_a = true; > cpu->cfg.ext_f = true; > cpu->cfg.ext_d = true; > cpu->cfg.ext_icsr = true; > cpu->cfg.ext_ifencei = true; > } > > This is the only place where "G" (ext_g) is read. Here, if G is enabled > and not all IMAFD_Zicsr_Zifencei are enabled, it enables them with a > warning message. > > So, even if "G" is disabled alone, it's harmless. Note that > IMAFD_Zicsr_Zifencei are enabled by default. > > Thanks, > Tsukasa Hello! Thank you very much for the clarification, it was helpful. Still getting used to the riscv code base in QEMU Best regards, -- Víctor Cora Colombo Instituto de Pesquisas ELDORADO Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
On ISA version 20190608 or later, "G" expands to "IMAFD_Zicsr_Zifencei".
Both "Zicsr" and "Zifencei" are enabled by default and "G" is supposed to
be (virtually) enabled as well, it should be safe to change its expansion.
Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
target/riscv/cpu.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 3ea68d5cd7..0854ca9103 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -598,13 +598,16 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m &&
cpu->cfg.ext_a && cpu->cfg.ext_f &&
- cpu->cfg.ext_d)) {
- warn_report("Setting G will also set IMAFD");
+ cpu->cfg.ext_d &&
+ cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) {
+ warn_report("Setting G will also set IMAFD_Zicsr_Zifencei");
cpu->cfg.ext_i = true;
cpu->cfg.ext_m = true;
cpu->cfg.ext_a = true;
cpu->cfg.ext_f = true;
cpu->cfg.ext_d = true;
+ cpu->cfg.ext_icsr = true;
+ cpu->cfg.ext_ifencei = true;
}
if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx ||
--
2.34.1
On Sun, May 15, 2022 at 12:56 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: > > On ISA version 20190608 or later, "G" expands to "IMAFD_Zicsr_Zifencei". > Both "Zicsr" and "Zifencei" are enabled by default and "G" is supposed to > be (virtually) enabled as well, it should be safe to change its expansion. > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/cpu.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 3ea68d5cd7..0854ca9103 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -598,13 +598,16 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > > if (cpu->cfg.ext_g && !(cpu->cfg.ext_i && cpu->cfg.ext_m && > cpu->cfg.ext_a && cpu->cfg.ext_f && > - cpu->cfg.ext_d)) { > - warn_report("Setting G will also set IMAFD"); > + cpu->cfg.ext_d && > + cpu->cfg.ext_icsr && cpu->cfg.ext_ifencei)) { > + warn_report("Setting G will also set IMAFD_Zicsr_Zifencei"); > cpu->cfg.ext_i = true; > cpu->cfg.ext_m = true; > cpu->cfg.ext_a = true; > cpu->cfg.ext_f = true; > cpu->cfg.ext_d = true; > + cpu->cfg.ext_icsr = true; > + cpu->cfg.ext_ifencei = true; > } > > if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx || > -- > 2.34.1 >
QEMU allowed inconsistent configurations that made floating point
arithmetic effectively unusable.
This commit adds certain checks for consistent FP arithmetic:
- F requires Zicsr
- Zfinx requires Zicsr
- Zfh/Zfhmin require F
- D requires F
- V requires D
Because F/D/Zicsr are enabled by default (and an error will not occur unless
we manually disable one or more of prerequisites), this commit just enforces
the user to give consistent combinations.
Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
target/riscv/cpu.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index 0854ca9103..f910a41407 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -610,11 +610,36 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
cpu->cfg.ext_ifencei = true;
}
+ if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) {
+ error_setg(errp, "F extension requires Zicsr");
+ return;
+ }
+
+ if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) {
+ error_setg(errp, "Zfh/Zfhmin extensions require F extension");
+ return;
+ }
+
+ if (cpu->cfg.ext_d && !cpu->cfg.ext_f) {
+ error_setg(errp, "D extension requires F extension");
+ return;
+ }
+
+ if (cpu->cfg.ext_v && !cpu->cfg.ext_d) {
+ error_setg(errp, "V extension requires D extension");
+ return;
+ }
+
if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx ||
cpu->cfg.ext_zhinxmin) {
cpu->cfg.ext_zfinx = true;
}
+ if (cpu->cfg.ext_zfinx && !cpu->cfg.ext_icsr) {
+ error_setg(errp, "Zfinx extension requires Zicsr");
+ return;
+ }
+
if (cpu->cfg.ext_zk) {
cpu->cfg.ext_zkn = true;
cpu->cfg.ext_zkr = true;
--
2.34.1
On Sun, May 15, 2022 at 12:56 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: > > QEMU allowed inconsistent configurations that made floating point > arithmetic effectively unusable. > > This commit adds certain checks for consistent FP arithmetic: > > - F requires Zicsr > - Zfinx requires Zicsr > - Zfh/Zfhmin require F > - D requires F > - V requires D > > Because F/D/Zicsr are enabled by default (and an error will not occur unless > we manually disable one or more of prerequisites), this commit just enforces > the user to give consistent combinations. > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/cpu.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 0854ca9103..f910a41407 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -610,11 +610,36 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > cpu->cfg.ext_ifencei = true; > } > > + if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) { > + error_setg(errp, "F extension requires Zicsr"); > + return; > + } > + > + if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) { > + error_setg(errp, "Zfh/Zfhmin extensions require F extension"); > + return; > + } > + > + if (cpu->cfg.ext_d && !cpu->cfg.ext_f) { > + error_setg(errp, "D extension requires F extension"); > + return; > + } > + > + if (cpu->cfg.ext_v && !cpu->cfg.ext_d) { > + error_setg(errp, "V extension requires D extension"); > + return; > + } > + > if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx || > cpu->cfg.ext_zhinxmin) { > cpu->cfg.ext_zfinx = true; > } > > + if (cpu->cfg.ext_zfinx && !cpu->cfg.ext_icsr) { > + error_setg(errp, "Zfinx extension requires Zicsr"); > + return; > + } > + > if (cpu->cfg.ext_zk) { > cpu->cfg.ext_zkn = true; > cpu->cfg.ext_zkr = true; > -- > 2.34.1 >
在 2022/5/15 上午10:56, Tsukasa OI 写道: > QEMU allowed inconsistent configurations that made floating point > arithmetic effectively unusable. > > This commit adds certain checks for consistent FP arithmetic: > > - F requires Zicsr > - Zfinx requires Zicsr > - Zfh/Zfhmin require F > - D requires F > - V requires D Why 'V requires D'? I know partial vector instructions require D, However, I think they just like c.fsd instruction requires D, not 'C requires D' or 'D requires C'. Is there any rvv spec change I don't know? Regards. Weiwei Li > > Because F/D/Zicsr are enabled by default (and an error will not occur unless > we manually disable one or more of prerequisites), this commit just enforces > the user to give consistent combinations. > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> > --- > target/riscv/cpu.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index 0854ca9103..f910a41407 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -610,11 +610,36 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > cpu->cfg.ext_ifencei = true; > } > > + if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) { > + error_setg(errp, "F extension requires Zicsr"); > + return; > + } > + > + if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) { > + error_setg(errp, "Zfh/Zfhmin extensions require F extension"); > + return; > + } > + > + if (cpu->cfg.ext_d && !cpu->cfg.ext_f) { > + error_setg(errp, "D extension requires F extension"); > + return; > + } > + > + if (cpu->cfg.ext_v && !cpu->cfg.ext_d) { > + error_setg(errp, "V extension requires D extension"); > + return; > + } > + > if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx || > cpu->cfg.ext_zhinxmin) { > cpu->cfg.ext_zfinx = true; > } > > + if (cpu->cfg.ext_zfinx && !cpu->cfg.ext_icsr) { > + error_setg(errp, "Zfinx extension requires Zicsr"); > + return; > + } > + > if (cpu->cfg.ext_zk) { > cpu->cfg.ext_zkn = true; > cpu->cfg.ext_zkr = true;
On 2022/05/15 23:37, Weiwei Li wrote: > > 在 2022/5/15 上午10:56, Tsukasa OI 写道: >> QEMU allowed inconsistent configurations that made floating point >> arithmetic effectively unusable. >> >> This commit adds certain checks for consistent FP arithmetic: >> >> - F requires Zicsr >> - Zfinx requires Zicsr >> - Zfh/Zfhmin require F >> - D requires F >> - V requires D > > Why 'V requires D'? I know partial vector instructions require D, However, I think they just like c.fsd > > instruction requires D, not 'C requires D' or 'D requires C'. Is there any rvv spec change I don't know? I thought it was crystal-clear. RISC-V "V" Vector Extension Version 1.0 (riscv-v-spec-1.0.pdf) 18.3. V: Vector Extension for Application Processors Page 94: > The V extension requires the scalar processor implements the F and D > extensions, and implements all vector floating-point instructions > (Section Vector Floating-Point Instructions) for floating-point operands > with EEW=32 or EEW=64 (including widening instructions and conversions > between FP32 and FP64). c.f. <https://github.com/riscv/riscv-v-spec/releases/tag/v1.0> Thanks, Tsukasa > > Regards. > > Weiwei Li > >> >> Because F/D/Zicsr are enabled by default (and an error will not occur unless >> we manually disable one or more of prerequisites), this commit just enforces >> the user to give consistent combinations. >> >> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> >> --- >> target/riscv/cpu.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >> index 0854ca9103..f910a41407 100644 >> --- a/target/riscv/cpu.c >> +++ b/target/riscv/cpu.c >> @@ -610,11 +610,36 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) >> cpu->cfg.ext_ifencei = true; >> } >> + if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) { >> + error_setg(errp, "F extension requires Zicsr"); >> + return; >> + } >> + >> + if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) { >> + error_setg(errp, "Zfh/Zfhmin extensions require F extension"); >> + return; >> + } >> + >> + if (cpu->cfg.ext_d && !cpu->cfg.ext_f) { >> + error_setg(errp, "D extension requires F extension"); >> + return; >> + } >> + >> + if (cpu->cfg.ext_v && !cpu->cfg.ext_d) { >> + error_setg(errp, "V extension requires D extension"); >> + return; >> + } >> + >> if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx || >> cpu->cfg.ext_zhinxmin) { >> cpu->cfg.ext_zfinx = true; >> } >> + if (cpu->cfg.ext_zfinx && !cpu->cfg.ext_icsr) { >> + error_setg(errp, "Zfinx extension requires Zicsr"); >> + return; >> + } >> + >> if (cpu->cfg.ext_zk) { >> cpu->cfg.ext_zkn = true; >> cpu->cfg.ext_zkr = true; >
在 2022/5/15 下午10:45, Tsukasa OI 写道: > On 2022/05/15 23:37, Weiwei Li wrote: >> 在 2022/5/15 上午10:56, Tsukasa OI 写道: >>> QEMU allowed inconsistent configurations that made floating point >>> arithmetic effectively unusable. >>> >>> This commit adds certain checks for consistent FP arithmetic: >>> >>> - F requires Zicsr >>> - Zfinx requires Zicsr >>> - Zfh/Zfhmin require F >>> - D requires F >>> - V requires D >> Why 'V requires D'? I know partial vector instructions require D, However, I think they just like c.fsd >> >> instruction requires D, not 'C requires D' or 'D requires C'. Is there any rvv spec change I don't know? > I thought it was crystal-clear. > > RISC-V "V" Vector Extension Version 1.0 (riscv-v-spec-1.0.pdf) > 18.3. V: Vector Extension for Application Processors > Page 94: > >> The V extension requires the scalar processor implements the F and D >> extensions, and implements all vector floating-point instructions >> (Section Vector Floating-Point Instructions) for floating-point operands >> with EEW=32 or EEW=64 (including widening instructions and conversions >> between FP32 and FP64). > c.f. <https://github.com/riscv/riscv-v-spec/releases/tag/v1.0> > > Thanks, > > Tsukasa OK. Thanks. Regards, Weiwei Li > >> Regards. >> >> Weiwei Li >> >>> Because F/D/Zicsr are enabled by default (and an error will not occur unless >>> we manually disable one or more of prerequisites), this commit just enforces >>> the user to give consistent combinations. >>> >>> Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> >>> --- >>> target/riscv/cpu.c | 25 +++++++++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c >>> index 0854ca9103..f910a41407 100644 >>> --- a/target/riscv/cpu.c >>> +++ b/target/riscv/cpu.c >>> @@ -610,11 +610,36 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) >>> cpu->cfg.ext_ifencei = true; >>> } >>> + if (cpu->cfg.ext_f && !cpu->cfg.ext_icsr) { >>> + error_setg(errp, "F extension requires Zicsr"); >>> + return; >>> + } >>> + >>> + if ((cpu->cfg.ext_zfh || cpu->cfg.ext_zfhmin) && !cpu->cfg.ext_f) { >>> + error_setg(errp, "Zfh/Zfhmin extensions require F extension"); >>> + return; >>> + } >>> + >>> + if (cpu->cfg.ext_d && !cpu->cfg.ext_f) { >>> + error_setg(errp, "D extension requires F extension"); >>> + return; >>> + } >>> + >>> + if (cpu->cfg.ext_v && !cpu->cfg.ext_d) { >>> + error_setg(errp, "V extension requires D extension"); >>> + return; >>> + } >>> + >>> if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx || >>> cpu->cfg.ext_zhinxmin) { >>> cpu->cfg.ext_zfinx = true; >>> } >>> + if (cpu->cfg.ext_zfinx && !cpu->cfg.ext_icsr) { >>> + error_setg(errp, "Zfinx extension requires Zicsr"); >>> + return; >>> + } >>> + >>> if (cpu->cfg.ext_zk) { >>> cpu->cfg.ext_zkn = true; >>> cpu->cfg.ext_zkr = true;
We should separate "check" and "configure" steps as possible.
This commit separates both steps except vector/Zfinx-related checks.
Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com>
---
target/riscv/cpu.c | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index f910a41407..5ab246bf63 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -630,14 +630,27 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
return;
}
+ if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
+ error_setg(errp, "Zve32f/Zve64f extensions require F extension");
+ return;
+ }
+
+ /* Set the ISA extensions, checks should have happened above */
if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx ||
cpu->cfg.ext_zhinxmin) {
cpu->cfg.ext_zfinx = true;
}
- if (cpu->cfg.ext_zfinx && !cpu->cfg.ext_icsr) {
- error_setg(errp, "Zfinx extension requires Zicsr");
- return;
+ if (cpu->cfg.ext_zfinx) {
+ if (!cpu->cfg.ext_icsr) {
+ error_setg(errp, "Zfinx extension requires Zicsr");
+ return;
+ }
+ if (cpu->cfg.ext_f) {
+ error_setg(errp,
+ "Zfinx cannot be supported together with F extension");
+ return;
+ }
}
if (cpu->cfg.ext_zk) {
@@ -663,7 +676,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
cpu->cfg.ext_zksh = true;
}
- /* Set the ISA extensions, checks should have happened above */
if (cpu->cfg.ext_i) {
ext |= RVI;
}
@@ -734,20 +746,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp)
}
set_vext_version(env, vext_version);
}
- if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) {
- error_setg(errp, "Zve32f/Zve64f extension depends upon RVF.");
- return;
- }
if (cpu->cfg.ext_j) {
ext |= RVJ;
}
- if (cpu->cfg.ext_zfinx && ((ext & (RVF | RVD)) || cpu->cfg.ext_zfh ||
- cpu->cfg.ext_zfhmin)) {
- error_setg(errp,
- "'Zfinx' cannot be supported together with 'F', 'D', 'Zfh',"
- " 'Zfhmin'");
- return;
- }
set_misa(env, env->misa_mxl, ext);
}
--
2.34.1
On Sun, May 15, 2022 at 12:56 PM Tsukasa OI <research_trasio@irq.a4lg.com> wrote: > > We should separate "check" and "configure" steps as possible. > This commit separates both steps except vector/Zfinx-related checks. > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> Alistair > --- > target/riscv/cpu.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index f910a41407..5ab246bf63 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -630,14 +630,27 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > return; > } > > + if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) { > + error_setg(errp, "Zve32f/Zve64f extensions require F extension"); > + return; > + } > + > + /* Set the ISA extensions, checks should have happened above */ > if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx || > cpu->cfg.ext_zhinxmin) { > cpu->cfg.ext_zfinx = true; > } > > - if (cpu->cfg.ext_zfinx && !cpu->cfg.ext_icsr) { > - error_setg(errp, "Zfinx extension requires Zicsr"); > - return; > + if (cpu->cfg.ext_zfinx) { > + if (!cpu->cfg.ext_icsr) { > + error_setg(errp, "Zfinx extension requires Zicsr"); > + return; > + } > + if (cpu->cfg.ext_f) { > + error_setg(errp, > + "Zfinx cannot be supported together with F extension"); > + return; > + } > } > > if (cpu->cfg.ext_zk) { > @@ -663,7 +676,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > cpu->cfg.ext_zksh = true; > } > > - /* Set the ISA extensions, checks should have happened above */ > if (cpu->cfg.ext_i) { > ext |= RVI; > } > @@ -734,20 +746,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > } > set_vext_version(env, vext_version); > } > - if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) { > - error_setg(errp, "Zve32f/Zve64f extension depends upon RVF."); > - return; > - } > if (cpu->cfg.ext_j) { > ext |= RVJ; > } > - if (cpu->cfg.ext_zfinx && ((ext & (RVF | RVD)) || cpu->cfg.ext_zfh || > - cpu->cfg.ext_zfhmin)) { > - error_setg(errp, > - "'Zfinx' cannot be supported together with 'F', 'D', 'Zfh'," > - " 'Zfhmin'"); > - return; > - } > > set_misa(env, env->misa_mxl, ext); > } > -- > 2.34.1 >
在 2022/5/15 上午10:56, Tsukasa OI 写道: > We should separate "check" and "configure" steps as possible. > This commit separates both steps except vector/Zfinx-related checks. > > Signed-off-by: Tsukasa OI <research_trasio@irq.a4lg.com> > --- > target/riscv/cpu.c | 31 ++++++++++++++++--------------- > 1 file changed, 16 insertions(+), 15 deletions(-) > > diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c > index f910a41407..5ab246bf63 100644 > --- a/target/riscv/cpu.c > +++ b/target/riscv/cpu.c > @@ -630,14 +630,27 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > return; > } > > + if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) { > + error_setg(errp, "Zve32f/Zve64f extensions require F extension"); > + return; > + } > + > + /* Set the ISA extensions, checks should have happened above */ > if (cpu->cfg.ext_zdinx || cpu->cfg.ext_zhinx || > cpu->cfg.ext_zhinxmin) { > cpu->cfg.ext_zfinx = true; > } > > - if (cpu->cfg.ext_zfinx && !cpu->cfg.ext_icsr) { > - error_setg(errp, "Zfinx extension requires Zicsr"); > - return; > + if (cpu->cfg.ext_zfinx) { > + if (!cpu->cfg.ext_icsr) { > + error_setg(errp, "Zfinx extension requires Zicsr"); > + return; > + } > + if (cpu->cfg.ext_f) { > + error_setg(errp, > + "Zfinx cannot be supported together with F extension"); > + return; > + } > } I think these checks for non-single-letter extensions are better to move out of the 'if (env->misa_ext == 0)) { ...}', since they are enabled directly by cfg property, such as we can set cpu option to sifive-u34 with zfinx=true. This may not be a proper way to set cpu option, However it's truly a legal command option, but configure an illegal supported ISA which enable both f and zfinx. Regards, Weiwei Li > > if (cpu->cfg.ext_zk) { > @@ -663,7 +676,6 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > cpu->cfg.ext_zksh = true; > } > > - /* Set the ISA extensions, checks should have happened above */ > if (cpu->cfg.ext_i) { > ext |= RVI; > } > @@ -734,20 +746,9 @@ static void riscv_cpu_realize(DeviceState *dev, Error **errp) > } > set_vext_version(env, vext_version); > } > - if ((cpu->cfg.ext_zve32f || cpu->cfg.ext_zve64f) && !cpu->cfg.ext_f) { > - error_setg(errp, "Zve32f/Zve64f extension depends upon RVF."); > - return; > - } > if (cpu->cfg.ext_j) { > ext |= RVJ; > } > - if (cpu->cfg.ext_zfinx && ((ext & (RVF | RVD)) || cpu->cfg.ext_zfh || > - cpu->cfg.ext_zfhmin)) { > - error_setg(errp, > - "'Zfinx' cannot be supported together with 'F', 'D', 'Zfh'," > - " 'Zfhmin'"); > - return; > - } > > set_misa(env, env->misa_mxl, ext); > }
© 2016 - 2024 Red Hat, Inc.