[PATCH v2 1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64

Akihiko Odaki posted 3 patches 1 year, 1 month ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Bin Meng <bin.meng@windriver.com>, Weiwei Li <liweiwei@iscas.ac.cn>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>
There is a newer version of this series
[PATCH v2 1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64
Posted by Akihiko Odaki 1 year, 1 month ago
TARGET_RISCV64 does not have riscv-32bit-cpu.xml so it shouldn't accept
MXL_RV32.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 target/riscv/tcg/tcg-cpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index a28918ab30..e0cbc56320 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -161,10 +161,11 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
     case MXL_RV128:
         cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
         break;
-#endif
+#elif defined(TARGET_RISCV32)
     case MXL_RV32:
         cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
         break;
+#endif
     default:
         g_assert_not_reached();
     }
-- 
2.42.0
Re: [PATCH v2 1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64
Posted by Daniel Henrique Barboza 1 year, 1 month ago

On 10/14/23 00:35, Akihiko Odaki wrote:
> TARGET_RISCV64 does not have riscv-32bit-cpu.xml so it shouldn't accept
> MXL_RV32.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---

Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>


>   target/riscv/tcg/tcg-cpu.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index a28918ab30..e0cbc56320 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -161,10 +161,11 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>       case MXL_RV128:
>           cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
>           break;
> -#endif
> +#elif defined(TARGET_RISCV32)
>       case MXL_RV32:
>           cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
>           break;
> +#endif
>       default:
>           g_assert_not_reached();
>       }
Re: [PATCH v2 1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64
Posted by Alistair Francis 1 year, 1 month ago
On Sun, Oct 15, 2023 at 4:05 AM Daniel Henrique Barboza
<dbarboza@ventanamicro.com> wrote:
>
>
>
> On 10/14/23 00:35, Akihiko Odaki wrote:
> > TARGET_RISCV64 does not have riscv-32bit-cpu.xml so it shouldn't accept
> > MXL_RV32.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
>
> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>
>
> >   target/riscv/tcg/tcg-cpu.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> > index a28918ab30..e0cbc56320 100644
> > --- a/target/riscv/tcg/tcg-cpu.c
> > +++ b/target/riscv/tcg/tcg-cpu.c
> > @@ -161,10 +161,11 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
> >       case MXL_RV128:
> >           cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
> >           break;
> > -#endif
> > +#elif defined(TARGET_RISCV32)
> >       case MXL_RV32:
> >           cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
> >           break;
> > +#endif

This isn't the right fix. The idea is that riscv64-softmmu can run
32-bit CPUs, so we instead should include riscv-32bit-cpu.xml

Alistair

> >       default:
> >           g_assert_not_reached();
> >       }
>
Re: [PATCH v2 1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64
Posted by LIU Zhiwei 1 year, 1 month ago
On 2023/10/16 9:51, Alistair Francis wrote:
> On Sun, Oct 15, 2023 at 4:05 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>> On 10/14/23 00:35, Akihiko Odaki wrote:
>>> TARGET_RISCV64 does not have riscv-32bit-cpu.xml so it shouldn't accept
>>> MXL_RV32.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>
>>
>>>    target/riscv/tcg/tcg-cpu.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>>> index a28918ab30..e0cbc56320 100644
>>> --- a/target/riscv/tcg/tcg-cpu.c
>>> +++ b/target/riscv/tcg/tcg-cpu.c
>>> @@ -161,10 +161,11 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>>>        case MXL_RV128:
>>>            cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
>>>            break;
>>> -#endif
>>> +#elif defined(TARGET_RISCV32)
>>>        case MXL_RV32:
>>>            cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
>>>            break;
>>> +#endif
> This isn't the right fix. The idea is that riscv64-softmmu can run
> 32-bit CPUs, so we instead should include riscv-32bit-cpu.xml

Agree. I'd like to go on the work. The question is that we don't have 
64-bit OpenSBI which supports booting 32-bit Linux.
So even we have implemented the SXLEN 32bit, we may not have the 
software to test it.

Do you support the SXL upstreaming with no testing?

Thanks,
Zhiwei

>
> Alistair
>
>>>        default:
>>>            g_assert_not_reached();
>>>        }

Re: [PATCH v2 1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64
Posted by Alistair Francis 1 year, 1 month ago
On Tue, Oct 17, 2023 at 12:14 PM LIU Zhiwei
<zhiwei_liu@linux.alibaba.com> wrote:
>
>
> On 2023/10/16 9:51, Alistair Francis wrote:
> > On Sun, Oct 15, 2023 at 4:05 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >>
> >> On 10/14/23 00:35, Akihiko Odaki wrote:
> >>> TARGET_RISCV64 does not have riscv-32bit-cpu.xml so it shouldn't accept
> >>> MXL_RV32.
> >>>
> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>> ---
> >> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >>
> >>
> >>>    target/riscv/tcg/tcg-cpu.c | 3 ++-
> >>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> >>> index a28918ab30..e0cbc56320 100644
> >>> --- a/target/riscv/tcg/tcg-cpu.c
> >>> +++ b/target/riscv/tcg/tcg-cpu.c
> >>> @@ -161,10 +161,11 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
> >>>        case MXL_RV128:
> >>>            cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
> >>>            break;
> >>> -#endif
> >>> +#elif defined(TARGET_RISCV32)
> >>>        case MXL_RV32:
> >>>            cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
> >>>            break;
> >>> +#endif
> > This isn't the right fix. The idea is that riscv64-softmmu can run
> > 32-bit CPUs, so we instead should include riscv-32bit-cpu.xml
>
> Agree. I'd like to go on the work. The question is that we don't have
> 64-bit OpenSBI which supports booting 32-bit Linux.
> So even we have implemented the SXLEN 32bit, we may not have the
> software to test it.

Ah! So I was first talking around just a full 32-bit CPU.

So for example:
    qemu-system-riscv64 -machine opentitan

So we are using qemu-system-riscv64 to run a 32-bit only CPU.

Then we can think about SXLEN

>
> Do you support the SXL upstreaming with no testing?

No, it should be tested

Alistair

>
> Thanks,
> Zhiwei
>
> >
> > Alistair
> >
> >>>        default:
> >>>            g_assert_not_reached();
> >>>        }
Re: [PATCH v2 1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64
Posted by LIU Zhiwei 1 year, 1 month ago
On 2023/10/17 11:32, Alistair Francis wrote:
> On Tue, Oct 17, 2023 at 12:14 PM LIU Zhiwei
> <zhiwei_liu@linux.alibaba.com> wrote:
>>
>> On 2023/10/16 9:51, Alistair Francis wrote:
>>> On Sun, Oct 15, 2023 at 4:05 AM Daniel Henrique Barboza
>>> <dbarboza@ventanamicro.com> wrote:
>>>>
>>>> On 10/14/23 00:35, Akihiko Odaki wrote:
>>>>> TARGET_RISCV64 does not have riscv-32bit-cpu.xml so it shouldn't accept
>>>>> MXL_RV32.
>>>>>
>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>> ---
>>>> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>>
>>>>
>>>>>     target/riscv/tcg/tcg-cpu.c | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>>>>> index a28918ab30..e0cbc56320 100644
>>>>> --- a/target/riscv/tcg/tcg-cpu.c
>>>>> +++ b/target/riscv/tcg/tcg-cpu.c
>>>>> @@ -161,10 +161,11 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>>>>>         case MXL_RV128:
>>>>>             cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
>>>>>             break;
>>>>> -#endif
>>>>> +#elif defined(TARGET_RISCV32)
>>>>>         case MXL_RV32:
>>>>>             cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
>>>>>             break;
>>>>> +#endif
>>> This isn't the right fix. The idea is that riscv64-softmmu can run
>>> 32-bit CPUs, so we instead should include riscv-32bit-cpu.xml
>> Agree. I'd like to go on the work. The question is that we don't have
>> 64-bit OpenSBI which supports booting 32-bit Linux.
>> So even we have implemented the SXLEN 32bit, we may not have the
>> software to test it.
> Ah! So I was first talking around just a full 32-bit CPU.
>
> So for example:
>      qemu-system-riscv64 -machine opentitan
>
> So we are using qemu-system-riscv64 to run a 32-bit only CPU.

Yes, if the 32-bit only cpu only has M mode, it can work now.

I have tried this for Xuantie E series cpu, for example the e902,  in 
the wild tree.

>
> Then we can think about SXLEN

Agree, maybe we can expose these cpus now.

Thanks,
Zhiwei

>
>> Do you support the SXL upstreaming with no testing?
> No, it should be tested
>
> Alistair
>
>> Thanks,
>> Zhiwei
>>
>>> Alistair
>>>
>>>>>         default:
>>>>>             g_assert_not_reached();
>>>>>         }

Re: [PATCH v2 1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64
Posted by Akihiko Odaki 1 year, 1 month ago

On 2023/10/16 10:51, Alistair Francis wrote:
> On Sun, Oct 15, 2023 at 4:05 AM Daniel Henrique Barboza
> <dbarboza@ventanamicro.com> wrote:
>>
>>
>>
>> On 10/14/23 00:35, Akihiko Odaki wrote:
>>> TARGET_RISCV64 does not have riscv-32bit-cpu.xml so it shouldn't accept
>>> MXL_RV32.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>
>> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>
>>
>>>    target/riscv/tcg/tcg-cpu.c | 3 ++-
>>>    1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>>> index a28918ab30..e0cbc56320 100644
>>> --- a/target/riscv/tcg/tcg-cpu.c
>>> +++ b/target/riscv/tcg/tcg-cpu.c
>>> @@ -161,10 +161,11 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>>>        case MXL_RV128:
>>>            cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
>>>            break;
>>> -#endif
>>> +#elif defined(TARGET_RISCV32)
>>>        case MXL_RV32:
>>>            cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
>>>            break;
>>> +#endif
> 
> This isn't the right fix. The idea is that riscv64-softmmu can run
> 32-bit CPUs, so we instead should include riscv-32bit-cpu.xml

In that case I can continue working on the previous version of this 
series, but is it really true? I see no 32-bit CPUs enabled for 
riscv64-softmmu. Is there a plan to enable them for riscv64-softmmu?

Re: [PATCH v2 1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64
Posted by Alistair Francis 1 year, 1 month ago
On Mon, Oct 16, 2023 at 1:22 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
>
>
> On 2023/10/16 10:51, Alistair Francis wrote:
> > On Sun, Oct 15, 2023 at 4:05 AM Daniel Henrique Barboza
> > <dbarboza@ventanamicro.com> wrote:
> >>
> >>
> >>
> >> On 10/14/23 00:35, Akihiko Odaki wrote:
> >>> TARGET_RISCV64 does not have riscv-32bit-cpu.xml so it shouldn't accept
> >>> MXL_RV32.
> >>>
> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>> ---
> >>
> >> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> >>
> >>
> >>>    target/riscv/tcg/tcg-cpu.c | 3 ++-
> >>>    1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> >>> index a28918ab30..e0cbc56320 100644
> >>> --- a/target/riscv/tcg/tcg-cpu.c
> >>> +++ b/target/riscv/tcg/tcg-cpu.c
> >>> @@ -161,10 +161,11 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
> >>>        case MXL_RV128:
> >>>            cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
> >>>            break;
> >>> -#endif
> >>> +#elif defined(TARGET_RISCV32)
> >>>        case MXL_RV32:
> >>>            cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
> >>>            break;
> >>> +#endif
> >
> > This isn't the right fix. The idea is that riscv64-softmmu can run
> > 32-bit CPUs, so we instead should include riscv-32bit-cpu.xml
>
> In that case I can continue working on the previous version of this
> series, but is it really true? I see no 32-bit CPUs enabled for
> riscv64-softmmu. Is there a plan to enable them for riscv64-softmmu?

Yeah....

So last time I tried the 32-bit CPUs didn't work correctly. I didn't
figure out what the issue was, but the *idea* is to eventually enable
32-bit CPUs in the 64-bit builds.

Alistair
Re: [PATCH v2 1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64
Posted by Akihiko Odaki 1 year, 1 month ago
On 2023/10/16 13:07, Alistair Francis wrote:
> On Mon, Oct 16, 2023 at 1:22 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>>
>>
>> On 2023/10/16 10:51, Alistair Francis wrote:
>>> On Sun, Oct 15, 2023 at 4:05 AM Daniel Henrique Barboza
>>> <dbarboza@ventanamicro.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/14/23 00:35, Akihiko Odaki wrote:
>>>>> TARGET_RISCV64 does not have riscv-32bit-cpu.xml so it shouldn't accept
>>>>> MXL_RV32.
>>>>>
>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>> ---
>>>>
>>>> Reviewed-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
>>>>
>>>>
>>>>>     target/riscv/tcg/tcg-cpu.c | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>>>>> index a28918ab30..e0cbc56320 100644
>>>>> --- a/target/riscv/tcg/tcg-cpu.c
>>>>> +++ b/target/riscv/tcg/tcg-cpu.c
>>>>> @@ -161,10 +161,11 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>>>>>         case MXL_RV128:
>>>>>             cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
>>>>>             break;
>>>>> -#endif
>>>>> +#elif defined(TARGET_RISCV32)
>>>>>         case MXL_RV32:
>>>>>             cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
>>>>>             break;
>>>>> +#endif
>>>
>>> This isn't the right fix. The idea is that riscv64-softmmu can run
>>> 32-bit CPUs, so we instead should include riscv-32bit-cpu.xml
>>
>> In that case I can continue working on the previous version of this
>> series, but is it really true? I see no 32-bit CPUs enabled for
>> riscv64-softmmu. Is there a plan to enable them for riscv64-softmmu?
> 
> Yeah....
> 
> So last time I tried the 32-bit CPUs didn't work correctly. I didn't
> figure out what the issue was, but the *idea* is to eventually enable
> 32-bit CPUs in the 64-bit builds.

Ok, then I'll push the previous version forward.

Daniel, you are concerned that moving misa_mxl_max to class to match 
with gdb_core_xml_file will result in an extra casts when fetching it 
and data when initializing the class.

I think the extra cast is fine since no fetch of misa_mxl_max happens in 
a hot path. Requiring data when initializing the class should also be 
fine since the proposed patch uses the class_data member of TypeInfo, 
which is currently free. Does it make sense?