diff mbox series

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

Message ID 3b6338af937d0d3aa0858ba1a4ad0fd9e759be66.1607967113.git.alistair.francis@wdc.com (mailing list archive)
State New, archived
Headers show
Series RISC-V: Start to remove xlen preprocess | expand

Commit Message

Alistair Francis Dec. 14, 2020, 8:33 p.m. UTC
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(-)

Comments

Bin Meng Dec. 15, 2020, 9:26 a.m. UTC | #1
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..

>          return true;
>      } else {
>          return false;

Regards,
Bin
Alistair Francis Dec. 15, 2020, 4:44 p.m. UTC | #2
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 should handle all SiFive CPUs, besides that we don't have that many CPUs.

Alistair

>
> >          return true;
> >      } else {
> >          return false;
>
> Regards,
> Bin
Richard Henderson Dec. 15, 2020, 9:25 p.m. UTC | #3
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.  :-)


r~
Alistair Francis Dec. 16, 2020, 6:16 p.m. UTC | #4
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~
diff mbox series

Patch

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)) {
         return true;
     } else {
         return false;