Re: [RFC PATCH 06/24] hw/core/cpu: Replace gdb_core_xml_file with gdb_core_feature

[Alex Bennée]

Akihiko Odaki <akihiko.odaki@daynix.com> writes:

> On 2023/08/14 20:59, Alex Bennée wrote:
>> Akihiko Odaki <akihiko.odaki@daynix.com> writes:
>> 
>>> This is a tree-wide change to replace gdb_core_xml_file, the path to
>>> GDB XML file with gdb_core_feature, the pointer to GDBFeature. This
>>> also replaces the values assigned to gdb_num_core_regs with the
>>> num_regs member of GDBFeature where applicable to remove magic numbers.
>>>
>>> A following change will utilize additional information provided by
>>> GDBFeature to simplify XML file lookup.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>   include/hw/core/cpu.h   | 5 +++--
>>>   target/s390x/cpu.h      | 2 --
>>>   gdbstub/gdbstub.c       | 6 +++---
>>>   target/arm/cpu.c        | 4 ++--
>>>   target/arm/cpu64.c      | 4 ++--
>>>   target/arm/tcg/cpu32.c  | 3 ++-
>>>   target/avr/cpu.c        | 4 ++--
>>>   target/hexagon/cpu.c    | 2 +-
>>>   target/i386/cpu.c       | 7 +++----
>>>   target/loongarch/cpu.c  | 4 ++--
>>>   target/m68k/cpu.c       | 7 ++++---
>>>   target/microblaze/cpu.c | 4 ++--
>>>   target/ppc/cpu_init.c   | 4 ++--
>>>   target/riscv/cpu.c      | 7 ++++---
>>>   target/rx/cpu.c         | 4 ++--
>>>   target/s390x/cpu.c      | 4 ++--
>>>   16 files changed, 36 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
>>> index fdcbe87352..84219c1885 100644
>>> --- a/include/hw/core/cpu.h
>>> +++ b/include/hw/core/cpu.h
>>> @@ -23,6 +23,7 @@
>>>   #include "hw/qdev-core.h"
>>>   #include "disas/dis-asm.h"
>>>   #include "exec/cpu-common.h"
>>> +#include "exec/gdbstub.h"
>>>   #include "exec/hwaddr.h"
>>>   #include "exec/memattrs.h"
>>>   #include "qapi/qapi-types-run-state.h"
>>> @@ -127,7 +128,7 @@ struct SysemuCPUOps;
>>>    *       breakpoint.  Used by AVR to handle a gdb mis-feature with
>>>    *       its Harvard architecture split code and data.
>>>    * @gdb_num_core_regs: Number of core registers accessible to GDB.
>> It seems redundant to have this when gdb_core_features already
>> encapsulates this, especially since...
>> 
>>> - * @gdb_core_xml_file: File name for core registers GDB XML description.
>>> + * @gdb_core_feature: GDB core feature description.
>>>    * @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to 
>>> stop
>>>    *           before the insn which triggers a watchpoint rather than 
>>> after it.
>>>    * @gdb_arch_name: Optional callback that returns the architecture name 
>>> known
>>> @@ -163,7 +164,7 @@ struct CPUClass {
>>>       int (*gdb_write_register)(CPUState *cpu, uint8_t *buf, int reg);
>>>       vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr);
>>>   -    const char *gdb_core_xml_file;
>>> +    const GDBFeature *gdb_core_feature;
>>>       gchar * (*gdb_arch_name)(CPUState *cpu);
>>>       const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char 
>>> *xmlname);
>>>   
>> <snip>
>>> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
>>> index d71a162070..a206ab6b1b 100644
>>> --- a/target/arm/cpu.c
>>> +++ b/target/arm/cpu.c
>>> @@ -2353,7 +2353,6 @@ static void arm_cpu_class_init(ObjectClass *oc, void 
>>> *data)
>>>   #ifndef CONFIG_USER_ONLY
>>>       cc->sysemu_ops = &arm_sysemu_ops;
>>>   #endif
>>> -    cc->gdb_num_core_regs = 26;
>>>       cc->gdb_arch_name = arm_gdb_arch_name;
>>>       cc->gdb_get_dynamic_xml = arm_gdb_get_dynamic_xml;
>>>       cc->gdb_stop_before_watchpoint = true;
>>> @@ -2378,7 +2377,8 @@ static void cpu_register_class_init(ObjectClass *oc, 
>>> void *data)
>>>       CPUClass *cc = CPU_CLASS(acc);
>>>         acc->info = data;
>>> -    cc->gdb_core_xml_file = "arm-core.xml";
>>> +    cc->gdb_core_feature = gdb_find_static_feature("arm-core.xml");
>>> +    cc->gdb_num_core_regs = cc->gdb_core_feature->num_regs;
>> You are doing assignments like this. I think something like this in
>> gdbstub:
>> modified   gdbstub/gdbstub.c
>> @@ -440,7 +440,7 @@ int gdb_read_register(CPUState *cpu, GByteArray *buf, 
>> int reg, bool has_xml)
>>       CPUArchState *env = cpu->env_ptr;
>>       GDBRegisterState *r;
>>   -    if (reg < cc->gdb_num_core_regs) {
>> +    if (reg < cc->gdb_core_feature->num_regs) {
>>           return cc->gdb_read_register(cpu, buf, reg, has_xml);
>>       }
>>   @@ -459,7 +459,7 @@ static int gdb_write_register(CPUState *cpu,
>> uint8_t *mem_buf, int reg,
>>       CPUArchState *env = cpu->env_ptr;
>>       GDBRegisterState *r;
>>   -    if (reg < cc->gdb_num_core_regs) {
>> +    if (reg < cc->gdb_core_feature->num_regs) {
>>           return cc->gdb_write_register(cpu, mem_buf, reg, has_xml);
>>       }
>> makes most of the uses go away. Some of the other arches might need
>> target specific tweaks.
>
> The problem is how to deal with the target specific tweaks. ppc
> requires gdb_num_core_regs to have some value greater than
> cc->gdb_core_feature->num_regs for compatibility with legacy GDB.
> Other architectures simply do not have XMLs. Simply replacing
> cc->gdb_num_core_regs with cc->gdb_core_feature->num_regs will break
> those architectures.

How about:

  int core_regs = cc->gdb_core_feature ? cc->gdb_core_feature->num_regs
  : cc->gdb_num_core_regs

And document the field as for legacy gdb use only?

>
>> <snip>
>> 


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro