[v2] gdbstub and TCG plugin improvements

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

Posted by Akihiko Odaki 2 years, 3 months ago

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

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

Posted by Daniel Henrique Barboza 2 years, 3 months ago


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

Posted by Akihiko Odaki 2 years, 3 months ago

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).