Re: [PATCH v3 01/15] hw/riscv: Expand the is 32-bit check to support more CPUs

[Alistair Francis]

On Tue, Dec 15, 2020 at 1:25 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 12/15/20 10:44 AM, Alistair Francis wrote:
> > On Tue, Dec 15, 2020 at 1:26 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >> On Tue, Dec 15, 2020 at 4:34 AM Alistair Francis
> >> <alistair.francis@wdc.com> wrote:
> >>>
> >>> Currently the riscv_is_32_bit() function only supports the generic rv32
> >>> CPUs. Extend the function to support the SiFive and LowRISC CPUs as
> >>> well.
> >>>
> >>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >>> ---
> >>>  hw/riscv/boot.c | 12 +++++++++++-
> >>>  1 file changed, 11 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> >>> index d62f3dc758..3c70ac75d7 100644
> >>> --- a/hw/riscv/boot.c
> >>> +++ b/hw/riscv/boot.c
> >>> @@ -41,7 +41,17 @@
> >>>
> >>>  bool riscv_is_32_bit(MachineState *machine)
> >>>  {
> >>> -    if (!strncmp(machine->cpu_type, "rv32", 4)) {
> >>> +    /*
> >>> +     * To determine if the CPU is 32-bit we need to check a few 
> >>> different CPUs.
> >>> +     *
> >>> +     * If the CPU starts with rv32
> >>> +     * If the CPU is a sifive 3 seriries CPU (E31, U34)
> >>> +     * If it's the Ibex CPU
> >>> +     */
> >>> +    if (!strncmp(machine->cpu_type, "rv32", 4) ||
> >>> +        (!strncmp(machine->cpu_type, "sifive", 6) &&
> >>> +            machine->cpu_type[8] == '3') ||
> >>> +        !strncmp(machine->cpu_type, "lowrisc-ibex", 12)) {
> >>
> >> This does not look like a scalable way. With more and more CPU added
> >> in the future, this may end up with a huge list of strncmps..
> >
> > Any better ideas?
>
> It doesn't appear as if you need to query this before you've instantiated the
> cpu.  The first dynamic use that I see is create_fdt, which happens after that
> point.
>
> Verified like so:
>
> $ gdb --args ./qemu-system-riscv64 -M virt
> (gdb) b rv64_base_cpu_init
> Breakpoint 1 at 0x548390: file ../qemu/target/riscv/cpu.c, line 156.
> (gdb) b riscv_is_32_bit
> Breakpoint 2 at 0x490dd0: file ../qemu/hw/riscv/boot.c, line 37.
> (gdb) run
> ...
> Thread 1 "qemu-system-ris" hit Breakpoint 1, rv64_base_cpu_init
>
> >From the machine, you can find the boot cpu, and use the riscv_cpu_is_32bit
> helper on that.
>
> Also, just noticed that the two functions spell "32bit" differently.  :-)

Thanks for the help Richard.

I have added a commit in v4 that changes the function like you
mention. The rebased ended up being a little tricky so I added the
commit to the end instead.

Alistair

>
>
> r~