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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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