[PATCH 0/5] target/riscv: Enhanced ISA extension checks

Tsukasa OI posted 5 patches 1 week ago
target/riscv/cpu.c | 62 +++++++++++++++++++++++++++++++++-------------
1 file changed, 45 insertions(+), 17 deletions(-)
[PATCH 0/5] target/riscv: Enhanced ISA extension checks
Posted by Tsukasa OI 1 week ago
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
Re: [PATCH 0/5] target/riscv: Enhanced ISA extension checks
Posted by Alistair Francis 3 days, 15 hours ago
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
>
[PATCH v2 0/5] target/riscv: Enhanced ISA extension checks
Posted by Tsukasa OI 5 days, 14 hours ago
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
[PATCH v2 1/5] target/riscv: Fix coding style on "G" expansion
Posted by Tsukasa OI 5 days, 14 hours ago
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
Re: [PATCH v2 1/5] target/riscv: Fix coding style on "G" expansion
Posted by Alistair Francis 3 days, 17 hours ago
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
>
Re: [PATCH v2 1/5] target/riscv: Fix coding style on "G" expansion
Posted by Víctor Colombo 3 days, 23 hours ago
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>

[PATCH v2 2/5] target/riscv: Disable "G" by default
Posted by Tsukasa OI 5 days, 14 hours ago
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
Re: [PATCH v2 2/5] target/riscv: Disable "G" by default
Posted by Víctor Colombo 3 days, 23 hours ago
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>

[PATCH v2 3/5] target/riscv: Change "G" expansion
Posted by Tsukasa OI 5 days, 14 hours ago
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
Re: [PATCH v2 3/5] target/riscv: Change "G" expansion
Posted by Alistair Francis 3 days, 17 hours ago
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
>
[PATCH v2 4/5] target/riscv: FP extension requirements
Posted by Tsukasa OI 5 days, 14 hours ago
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
Re: [PATCH v2 4/5] target/riscv: FP extension requirements
Posted by Alistair Francis 3 days, 16 hours ago
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
>
Re: [PATCH v2 4/5] target/riscv: FP extension requirements
Posted by Weiwei Li 5 days, 3 hours ago
在 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;


Re: [PATCH v2 4/5] target/riscv: FP extension requirements
Posted by Tsukasa OI 5 days, 3 hours ago
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;
> 

Re: [PATCH v2 4/5] target/riscv: FP extension requirements
Posted by Weiwei Li 5 days, 2 hours ago
在 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;


[PATCH v2 5/5] target/riscv: Move/refactor ISA extension checks
Posted by Tsukasa OI 5 days, 14 hours ago
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
Re: [PATCH v2 5/5] target/riscv: Move/refactor ISA extension checks
Posted by Alistair Francis 3 days, 16 hours ago
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
>
Re: [PATCH v2 5/5] target/riscv: Move/refactor ISA extension checks
Posted by Weiwei Li 4 days, 14 hours ago
在 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);
>       }