Akihiko Odaki <akihiko.od...@daynix.com> writes:
> On 2023/08/14 20:59, Alex Bennée wrote:
>> Akihiko Odaki <akihiko.od...@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.od...@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
