[PATCH v2 0/3] gdbstub and TCG plugin improvements

[PATCH v2 0/3] gdbstub and TCG plugin improvements

[Akihiko Odaki]

This series extracts fixes and refactorings that can be applied
independently from "[PATCH v9 00/23] plugins: Allow to read registers".

The patch "target/riscv: Move MISA limits to class" was replaced with
patch "target/riscv: Move misa_mxl_max to class" since I found instances
may have different misa_ext_mask.

V1 -> V2:
  Added patch "target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64".
  Added patch "target/riscv: Initialize gdb_core_xml_file only once".
  Dropped patch "target/riscv: Remove misa_mxl validation".
  Dropped patch "target/riscv: Move misa_mxl_max to class".
  Dropped patch "target/riscv: Validate misa_mxl_max only once".

Akihiko Odaki (3):
  target/riscv: Do not allow MXL_RV32 for TARGET_RISCV64
  target/riscv: Initialize gdb_core_xml_file only once
  plugins: Remove an extra parameter

 accel/tcg/plugin-gen.c     | 9 +++------
 target/riscv/cpu.c         | 5 +++++
 target/riscv/tcg/tcg-cpu.c | 7 ++-----
 3 files changed, 10 insertions(+), 11 deletions(-)

-- 
2.42.0

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

[Akihiko Odaki]

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

[PATCH v2 2/3] target/riscv: Initialize gdb_core_xml_file only once

[Akihiko Odaki]

gdb_core_xml_file was assigned each time a CPU is instantiated before
this change.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 target/riscv/cpu.c         | 5 +++++
 target/riscv/tcg/tcg-cpu.c | 4 ----
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ac4a6c7eec..a811215150 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1575,6 +1575,11 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
     cc->get_pc = riscv_cpu_get_pc;
     cc->gdb_read_register = riscv_cpu_gdb_read_register;
     cc->gdb_write_register = riscv_cpu_gdb_write_register;
+#ifdef TARGET_RISCV64
+    cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
+#elif defined(TARGET_RISCV32)
+    cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
+#endif
     cc->gdb_num_core_regs = 33;
     cc->gdb_stop_before_watchpoint = true;
     cc->disas_set_info = riscv_cpu_disas_set_info;
diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
index e0cbc56320..626fb2acea 100644
--- a/target/riscv/tcg/tcg-cpu.c
+++ b/target/riscv/tcg/tcg-cpu.c
@@ -150,8 +150,6 @@ static void riscv_cpu_validate_misa_priv(CPURISCVState *env, Error **errp)
 
 static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
 {
-    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
-    CPUClass *cc = CPU_CLASS(mcc);
     CPURISCVState *env = &cpu->env;
 
     /* Validate that MISA_MXL is set properly. */
@@ -159,11 +157,9 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
 #ifdef TARGET_RISCV64
     case MXL_RV64:
     case MXL_RV128:
-        cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
         break;
 #elif defined(TARGET_RISCV32)
     case MXL_RV32:
-        cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
         break;
 #endif
     default:
-- 
2.42.0

[PATCH v2 3/3] plugins: Remove an extra parameter

[Akihiko Odaki]

copy_call() has an unused parameter so remove it.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 accel/tcg/plugin-gen.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/accel/tcg/plugin-gen.c b/accel/tcg/plugin-gen.c
index 39b3c9351f..78b331b251 100644
--- a/accel/tcg/plugin-gen.c
+++ b/accel/tcg/plugin-gen.c
@@ -327,8 +327,7 @@ static TCGOp *copy_st_ptr(TCGOp **begin_op, TCGOp *op)
     return op;
 }
 
-static TCGOp *copy_call(TCGOp **begin_op, TCGOp *op, void *empty_func,
-                        void *func, int *cb_idx)
+static TCGOp *copy_call(TCGOp **begin_op, TCGOp *op, void *func, int *cb_idx)
 {
     TCGOp *old_op;
     int func_idx;
@@ -372,8 +371,7 @@ static TCGOp *append_udata_cb(const struct qemu_plugin_dyn_cb *cb,
     }
 
     /* call */
-    op = copy_call(&begin_op, op, HELPER(plugin_vcpu_udata_cb),
-                   cb->f.vcpu_udata, cb_idx);
+    op = copy_call(&begin_op, op, cb->f.vcpu_udata, cb_idx);
 
     return op;
 }
@@ -420,8 +418,7 @@ static TCGOp *append_mem_cb(const struct qemu_plugin_dyn_cb *cb,
 
     if (type == PLUGIN_GEN_CB_MEM) {
         /* call */
-        op = copy_call(&begin_op, op, HELPER(plugin_vcpu_mem_cb),
-                       cb->f.vcpu_udata, cb_idx);
+        op = copy_call(&begin_op, op, cb->f.vcpu_udata, cb_idx);
     }
 
     return op;
-- 
2.42.0

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

[Daniel Henrique Barboza]

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 2/3] target/riscv: Initialize gdb_core_xml_file only once

[Daniel Henrique Barboza]

On 10/14/23 00:35, Akihiko Odaki wrote:
> gdb_core_xml_file was assigned each time a CPU is instantiated before
> this change.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   target/riscv/cpu.c         | 5 +++++
>   target/riscv/tcg/tcg-cpu.c | 4 ----
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index ac4a6c7eec..a811215150 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1575,6 +1575,11 @@ static void riscv_cpu_class_init(ObjectClass *c, void *data)
>       cc->get_pc = riscv_cpu_get_pc;
>       cc->gdb_read_register = riscv_cpu_gdb_read_register;
>       cc->gdb_write_register = riscv_cpu_gdb_write_register;
> +#ifdef TARGET_RISCV64
> +    cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
> +#elif defined(TARGET_RISCV32)
> +    cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
> +#endif
>       cc->gdb_num_core_regs = 33;
>       cc->gdb_stop_before_watchpoint = true;
>       cc->disas_set_info = riscv_cpu_disas_set_info;
> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
> index e0cbc56320..626fb2acea 100644
> --- a/target/riscv/tcg/tcg-cpu.c
> +++ b/target/riscv/tcg/tcg-cpu.c
> @@ -150,8 +150,6 @@ static void riscv_cpu_validate_misa_priv(CPURISCVState *env, Error **errp)
>   
>   static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>   {
> -    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
> -    CPUClass *cc = CPU_CLASS(mcc);
>       CPURISCVState *env = &cpu->env;
>   
>       /* Validate that MISA_MXL is set properly. */
> @@ -159,11 +157,9 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>   #ifdef TARGET_RISCV64
>       case MXL_RV64:
>       case MXL_RV128:
> -        cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
>           break;
>   #elif defined(TARGET_RISCV32)
>       case MXL_RV32:
> -        cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
>           break;


Hmmm the issue here is that, in patch 1, you added an "elif defined(TARGET_RISCV32)"
based on an assumption that you changed here since there's no more gdb_core files being
set.

My suggestion is to use patch 1 from v1, where you removed the misa_mxl_max == misa_mxl
check at the end of this function. And then in this patch you can remove this function
altogether since you're assigning gdb_core in riscv_cpu_class_init() and the function will
be left doing nothing of note.


Thanks,

Daniel

>   #endif
>       default:

Re: [PATCH v2 2/3] target/riscv: Initialize gdb_core_xml_file only once

[Akihiko Odaki]

On 2023/10/15 3:19, Daniel Henrique Barboza wrote:
> 
> 
> On 10/14/23 00:35, Akihiko Odaki wrote:
>> gdb_core_xml_file was assigned each time a CPU is instantiated before
>> this change.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   target/riscv/cpu.c         | 5 +++++
>>   target/riscv/tcg/tcg-cpu.c | 4 ----
>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
>> index ac4a6c7eec..a811215150 100644
>> --- a/target/riscv/cpu.c
>> +++ b/target/riscv/cpu.c
>> @@ -1575,6 +1575,11 @@ static void riscv_cpu_class_init(ObjectClass 
>> *c, void *data)
>>       cc->get_pc = riscv_cpu_get_pc;
>>       cc->gdb_read_register = riscv_cpu_gdb_read_register;
>>       cc->gdb_write_register = riscv_cpu_gdb_write_register;
>> +#ifdef TARGET_RISCV64
>> +    cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
>> +#elif defined(TARGET_RISCV32)
>> +    cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
>> +#endif
>>       cc->gdb_num_core_regs = 33;
>>       cc->gdb_stop_before_watchpoint = true;
>>       cc->disas_set_info = riscv_cpu_disas_set_info;
>> diff --git a/target/riscv/tcg/tcg-cpu.c b/target/riscv/tcg/tcg-cpu.c
>> index e0cbc56320..626fb2acea 100644
>> --- a/target/riscv/tcg/tcg-cpu.c
>> +++ b/target/riscv/tcg/tcg-cpu.c
>> @@ -150,8 +150,6 @@ static void 
>> riscv_cpu_validate_misa_priv(CPURISCVState *env, Error **errp)
>>   static void riscv_cpu_validate_misa_mxl(RISCVCPU *cpu, Error **errp)
>>   {
>> -    RISCVCPUClass *mcc = RISCV_CPU_GET_CLASS(cpu);
>> -    CPUClass *cc = CPU_CLASS(mcc);
>>       CPURISCVState *env = &cpu->env;
>>       /* Validate that MISA_MXL is set properly. */
>> @@ -159,11 +157,9 @@ static void riscv_cpu_validate_misa_mxl(RISCVCPU 
>> *cpu, Error **errp)
>>   #ifdef TARGET_RISCV64
>>       case MXL_RV64:
>>       case MXL_RV128:
>> -        cc->gdb_core_xml_file = "riscv-64bit-cpu.xml";
>>           break;
>>   #elif defined(TARGET_RISCV32)
>>       case MXL_RV32:
>> -        cc->gdb_core_xml_file = "riscv-32bit-cpu.xml";
>>           break;
> 
> 
> Hmmm the issue here is that, in patch 1, you added an "elif 
> defined(TARGET_RISCV32)"
> based on an assumption that you changed here since there's no more 
> gdb_core files being
> set.

It's opposite. Patch 1 added "elif defined(TARGET_RISCV32)" because 
configs/targets/riscv64-linux-user.mak and 
configs/targets/riscv64-softmmu.mak do not list riscv-32bit-cpu.xml and 
referencing it from TARGET_RISCV64 results in an error.

Since patch 1 now ensures TARGET_RISCV64 will never have MXL_RV32 as 
misa_mxl_max, patch 2 can safely assume TARGET_RISCV64 always has 
riscv-64bit-cpu.xml as gdb_core_xml_file.

> 
> My suggestion is to use patch 1 from v1, where you removed the 
> misa_mxl_max == misa_mxl
> check at the end of this function. And then in this patch you can remove 
> this function
> altogether since you're assigning gdb_core in riscv_cpu_class_init() and 
> the function will
> be left doing nothing of note.

Assigning gdb_core_xml_file is more like a side-effect of 
riscv_cpu_validate_misa_mxl(), and the main purpose of this function is 
to validate misa_mxl[_max]. I think it's still a good idea to validate 
misa_mxl_max in particular. Specifying MXL_RV32 as misa_mxl_max for 
TARGET_RISCV64 or specifying MXL_RV64/MXL_RV128 for TARGET_RISCV32 will 
not work because of the incompatible gdb_core_xml_file (and probably 
other reasons).

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

[Alistair Francis]

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

[Akihiko Odaki]

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

[Alistair Francis]

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

[Akihiko Odaki]

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?

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

[LIU Zhiwei]

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

[Alistair Francis]

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

[LIU Zhiwei]

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();
>>>>>         }