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:

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

<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro