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

Message

Tsukasa OI May 13, 2022, 9:45 a.m. UTC

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

Comments

Alistair Francis May 17, 2022, 2:17 a.m. UTC | #1

On Fri, May 13, 2022 at 7:46 PM Tsukasa OI <[e-mail]> 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
>