| Message ID | 20231014033545.15220-2-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64 | expand |
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(); > }
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(); > > } >
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?
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
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?
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(); >>> }
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(); > >>> }
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(); >>>>> }
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(); }
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(-)