Re: [RFC PATCH 07/24] target/arm: Use GDBFeature for dynamic XML

["Alex Bennée" <alex.bennee@linaro.org>]

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

> In preparation for a change to use GDBFeature as a parameter of
> gdb_register_coprocessor(), convert the internal representation of
> dynamic feature from plain XML to GDBFeature.

FWIW one of the aims I had with my stalled rewrite of the register API
was to move all this XML generation into common code:

  https://github.com/qemu/qemu/compare/master...stsquad:qemu:introspection/registers#diff-f6409265629976beb19cc9b8d96889b67c006a265586615f491e7d59dd83dc44R68

to avoid each of the targets having to mess with constructing their own
XML and just concentrate of the semantics of each register type.

>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  target/arm/cpu.h       | 20 +++++------
>  target/arm/internals.h |  2 +-
>  target/arm/gdbstub.c   | 80 +++++++++++++++++++++++-------------------
>  target/arm/gdbstub64.c | 11 +++---
>  4 files changed, 60 insertions(+), 53 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 88e5accda6..d6c2378d05 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -136,23 +136,21 @@ enum {
>   */
>  
>  /**
> - * DynamicGDBXMLInfo:
> - * @desc: Contains the XML descriptions.
> - * @num: Number of the registers in this XML seen by GDB.
> + * DynamicGDBFeatureInfo:
> + * @desc: Contains the feature descriptions.
>   * @data: A union with data specific to the set of registers
>   *    @cpregs_keys: Array that contains the corresponding Key of
>   *                  a given cpreg with the same order of the cpreg
>   *                  in the XML description.
>   */
> -typedef struct DynamicGDBXMLInfo {
> -    char *desc;
> -    int num;
> +typedef struct DynamicGDBFeatureInfo {
> +    GDBFeature desc;
>      union {
>          struct {
>              uint32_t *keys;
>          } cpregs;
>      } data;
> -} DynamicGDBXMLInfo;
> +} DynamicGDBFeatureInfo;
>  
>  /* CPU state for each instance of a generic timer (in cp15 c14) */
>  typedef struct ARMGenericTimer {
> @@ -881,10 +879,10 @@ struct ArchCPU {
>      uint64_t *cpreg_vmstate_values;
>      int32_t cpreg_vmstate_array_len;
>  
> -    DynamicGDBXMLInfo dyn_sysreg_xml;
> -    DynamicGDBXMLInfo dyn_svereg_xml;
> -    DynamicGDBXMLInfo dyn_m_systemreg_xml;
> -    DynamicGDBXMLInfo dyn_m_secextreg_xml;
> +    DynamicGDBFeatureInfo dyn_sysreg_feature;
> +    DynamicGDBFeatureInfo dyn_svereg_feature;
> +    DynamicGDBFeatureInfo dyn_m_systemreg_feature;
> +    DynamicGDBFeatureInfo dyn_m_secextreg_feature;
>  
>      /* Timers used by the generic (architected) timer */
>      QEMUTimer *gt_timer[NUM_GTIMERS];
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 0f01bc32a8..8421a755af 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -1388,7 +1388,7 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
>  }
>  
>  #ifdef TARGET_AARCH64
> -int arm_gen_dynamic_svereg_xml(CPUState *cpu, int base_reg);
> +GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cpu, int base_reg);
>  int aarch64_gdb_get_sve_reg(CPUARMState *env, GByteArray *buf, int reg);
>  int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg);
>  int aarch64_gdb_get_fpu_reg(CPUARMState *env, GByteArray *buf, int reg);
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index f421c5d041..cd35bac013 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -25,11 +25,11 @@
>  #include "internals.h"
>  #include "cpregs.h"
>  
> -typedef struct RegisterSysregXmlParam {
> +typedef struct RegisterSysregFeatureParam {
>      CPUState *cs;
>      GString *s;
>      int n;
> -} RegisterSysregXmlParam;
> +} RegisterSysregFeatureParam;
>  
>  /* Old gdb always expect FPA registers.  Newer (xml-aware) gdb only expect
>     whatever the target description contains.  Due to a historical mishap
> @@ -243,7 +243,7 @@ static int arm_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg)
>      const ARMCPRegInfo *ri;
>      uint32_t key;
>  
> -    key = cpu->dyn_sysreg_xml.data.cpregs.keys[reg];
> +    key = cpu->dyn_sysreg_feature.data.cpregs.keys[reg];
>      ri = get_arm_cp_reginfo(cpu->cp_regs, key);
>      if (ri) {
>          if (cpreg_field_is_64bit(ri)) {
> @@ -260,7 +260,8 @@ static int arm_gdb_set_sysreg(CPUARMState *env, uint8_t *buf, int reg)
>      return 0;
>  }
>  
> -static void arm_gen_one_xml_sysreg_tag(GString *s, DynamicGDBXMLInfo *dyn_xml,
> +static void arm_gen_one_feature_sysreg(GString *s,
> +                                       DynamicGDBFeatureInfo *dyn_feature,
>                                         ARMCPRegInfo *ri, uint32_t ri_key,
>                                         int bitsize, int regnum)
>  {
> @@ -268,25 +269,25 @@ static void arm_gen_one_xml_sysreg_tag(GString *s, DynamicGDBXMLInfo *dyn_xml,
>      g_string_append_printf(s, " bitsize=\"%d\"", bitsize);
>      g_string_append_printf(s, " regnum=\"%d\"", regnum);
>      g_string_append_printf(s, " group=\"cp_regs\"/>");
> -    dyn_xml->data.cpregs.keys[dyn_xml->num] = ri_key;
> -    dyn_xml->num++;
> +    dyn_feature->data.cpregs.keys[dyn_feature->desc.num_regs] = ri_key;
> +    dyn_feature->desc.num_regs++;
>  }
>  
> -static void arm_register_sysreg_for_xml(gpointer key, gpointer value,
> -                                        gpointer p)
> +static void arm_register_sysreg_for_feature(gpointer key, gpointer value,
> +                                            gpointer p)
>  {
>      uint32_t ri_key = (uintptr_t)key;
>      ARMCPRegInfo *ri = value;
> -    RegisterSysregXmlParam *param = (RegisterSysregXmlParam *)p;
> +    RegisterSysregFeatureParam *param = (RegisterSysregFeatureParam *)p;
>      GString *s = param->s;
>      ARMCPU *cpu = ARM_CPU(param->cs);
>      CPUARMState *env = &cpu->env;
> -    DynamicGDBXMLInfo *dyn_xml = &cpu->dyn_sysreg_xml;
> +    DynamicGDBFeatureInfo *dyn_feature = &cpu->dyn_sysreg_feature;
>  
>      if (!(ri->type & (ARM_CP_NO_RAW | ARM_CP_NO_GDB))) {
>          if (arm_feature(env, ARM_FEATURE_AARCH64)) {
>              if (ri->state == ARM_CP_STATE_AA64) {
> -                arm_gen_one_xml_sysreg_tag(s , dyn_xml, ri, ri_key, 64,
> +                arm_gen_one_feature_sysreg(s , dyn_feature, ri, ri_key, 64,
>                                             param->n++);
>              }
>          } else {
> @@ -296,10 +297,10 @@ static void arm_register_sysreg_for_xml(gpointer key, gpointer value,
>                      return;
>                  }
>                  if (ri->type & ARM_CP_64BIT) {
> -                    arm_gen_one_xml_sysreg_tag(s , dyn_xml, ri, ri_key, 64,
> +                    arm_gen_one_feature_sysreg(s , dyn_feature, ri, ri_key, 64,
>                                                 param->n++);
>                  } else {
> -                    arm_gen_one_xml_sysreg_tag(s , dyn_xml, ri, ri_key, 32,
> +                    arm_gen_one_feature_sysreg(s , dyn_feature, ri, ri_key, 32,
>                                                 param->n++);
>                  }
>              }
> @@ -307,21 +308,24 @@ static void arm_register_sysreg_for_xml(gpointer key, gpointer value,
>      }
>  }
>  
> -static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg)
> +static GDBFeature *arm_gen_dynamic_sysreg_feature(CPUState *cs, int base_reg)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      GString *s = g_string_new(NULL);
> -    RegisterSysregXmlParam param = {cs, s, base_reg};
> +    RegisterSysregFeatureParam param = {cs, s, base_reg};
> +    DynamicGDBFeatureInfo *dyn_feature = &cpu->dyn_sysreg_feature;
> +    gsize num_regs = g_hash_table_size(cpu->cp_regs);
>  
> -    cpu->dyn_sysreg_xml.num = 0;
> -    cpu->dyn_sysreg_xml.data.cpregs.keys = g_new(uint32_t, g_hash_table_size(cpu->cp_regs));
> +    dyn_feature->desc.num_regs = 0;
> +    dyn_feature->data.cpregs.keys = g_new(uint32_t, num_regs);
>      g_string_printf(s, "<?xml version=\"1.0\"?>");
>      g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">");
>      g_string_append_printf(s, "<feature name=\"org.qemu.gdb.arm.sys.regs\">");
> -    g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_xml, &param);
> +    g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_feature, &param);
>      g_string_append_printf(s, "</feature>");
> -    cpu->dyn_sysreg_xml.desc = g_string_free(s, false);
> -    return cpu->dyn_sysreg_xml.num;
> +    dyn_feature->desc.xmlname = "system-registers.xml";
> +    dyn_feature->desc.xml = g_string_free(s, false);
> +    return &dyn_feature->desc;
>  }
>  
>  #ifdef CONFIG_TCG
> @@ -413,7 +417,8 @@ static int arm_gdb_set_m_systemreg(CPUARMState *env, uint8_t *buf, int reg)
>      return 0; /* TODO */
>  }
>  
> -static int arm_gen_dynamic_m_systemreg_xml(CPUState *cs, int orig_base_reg)
> +static GDBFeature *arm_gen_dynamic_m_systemreg_feature(CPUState *cs,
> +                                                       int orig_base_reg)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      CPUARMState *env = &cpu->env;
> @@ -434,10 +439,11 @@ static int arm_gen_dynamic_m_systemreg_xml(CPUState *cs, int orig_base_reg)
>      }
>  
>      g_string_append_printf(s, "</feature>");
> -    cpu->dyn_m_systemreg_xml.desc = g_string_free(s, false);
> -    cpu->dyn_m_systemreg_xml.num = base_reg - orig_base_reg;
> +    cpu->dyn_m_systemreg_feature.desc.xmlname = "arm-m-system.xml";
> +    cpu->dyn_m_systemreg_feature.desc.xml = g_string_free(s, false);
> +    cpu->dyn_m_systemreg_feature.desc.num_regs = base_reg - orig_base_reg;
>  
> -    return cpu->dyn_m_systemreg_xml.num;
> +    return &cpu->dyn_m_systemreg_feature.desc;
>  }
>  
>  #ifndef CONFIG_USER_ONLY
> @@ -455,7 +461,8 @@ static int arm_gdb_set_m_secextreg(CPUARMState *env, uint8_t *buf, int reg)
>      return 0; /* TODO */
>  }
>  
> -static int arm_gen_dynamic_m_secextreg_xml(CPUState *cs, int orig_base_reg)
> +static GDBFeature *arm_gen_dynamic_m_secextreg_feature(CPUState *cs,
> +                                                       int orig_base_reg)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      GString *s = g_string_new(NULL);
> @@ -476,10 +483,11 @@ static int arm_gen_dynamic_m_secextreg_xml(CPUState *cs, int orig_base_reg)
>      }
>  
>      g_string_append_printf(s, "</feature>");
> -    cpu->dyn_m_secextreg_xml.desc = g_string_free(s, false);
> -    cpu->dyn_m_secextreg_xml.num = base_reg - orig_base_reg;
> +    cpu->dyn_m_secextreg_feature.desc.xmlname = "arm-m-secext.xml";
> +    cpu->dyn_m_secextreg_feature.desc.xml = g_string_free(s, false);
> +    cpu->dyn_m_secextreg_feature.desc.num_regs = base_reg - orig_base_reg;
>  
> -    return cpu->dyn_m_secextreg_xml.num;
> +    return &cpu->dyn_m_secextreg_feature.desc;
>  }
>  #endif
>  #endif /* CONFIG_TCG */
> @@ -489,14 +497,14 @@ const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
>      ARMCPU *cpu = ARM_CPU(cs);
>  
>      if (strcmp(xmlname, "system-registers.xml") == 0) {
> -        return cpu->dyn_sysreg_xml.desc;
> +        return cpu->dyn_sysreg_feature.desc.xml;
>      } else if (strcmp(xmlname, "sve-registers.xml") == 0) {
> -        return cpu->dyn_svereg_xml.desc;
> +        return cpu->dyn_svereg_feature.desc.xml;
>      } else if (strcmp(xmlname, "arm-m-system.xml") == 0) {
> -        return cpu->dyn_m_systemreg_xml.desc;
> +        return cpu->dyn_m_systemreg_feature.desc.xml;
>  #ifndef CONFIG_USER_ONLY
>      } else if (strcmp(xmlname, "arm-m-secext.xml") == 0) {
> -        return cpu->dyn_m_secextreg_xml.desc;
> +        return cpu->dyn_m_secextreg_feature.desc.xml;
>  #endif
>      }
>      return NULL;
> @@ -514,7 +522,7 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
>           */
>  #ifdef TARGET_AARCH64
>          if (isar_feature_aa64_sve(&cpu->isar)) {
> -            int nreg = arm_gen_dynamic_svereg_xml(cs, cs->gdb_num_regs);
> +            int nreg = arm_gen_dynamic_svereg_feature(cs, cs->gdb_num_regs)->num_regs;
>              gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
>                                       aarch64_gdb_set_sve_reg, nreg,
>                                       "sve-registers.xml", 0);
> @@ -560,20 +568,20 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
>                                   1, "arm-m-profile-mve.xml", 0);
>      }
>      gdb_register_coprocessor(cs, arm_gdb_get_sysreg, arm_gdb_set_sysreg,
> -                             arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs),
> +                             arm_gen_dynamic_sysreg_feature(cs, cs->gdb_num_regs)->num_regs,
>                               "system-registers.xml", 0);
>  
>  #ifdef CONFIG_TCG
>      if (arm_feature(env, ARM_FEATURE_M) && tcg_enabled()) {
>          gdb_register_coprocessor(cs,
>              arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg,
> -            arm_gen_dynamic_m_systemreg_xml(cs, cs->gdb_num_regs),
> +            arm_gen_dynamic_m_systemreg_feature(cs, cs->gdb_num_regs)->num_regs,
>              "arm-m-system.xml", 0);
>  #ifndef CONFIG_USER_ONLY
>          if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
>              gdb_register_coprocessor(cs,
>                  arm_gdb_get_m_secextreg, arm_gdb_set_m_secextreg,
> -                arm_gen_dynamic_m_secextreg_xml(cs, cs->gdb_num_regs),
> +                arm_gen_dynamic_m_secextreg_feature(cs, cs->gdb_num_regs)->num_regs,
>                  "arm-m-secext.xml", 0);
>          }
>  #endif
> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> index d7b79a6589..20483ef9bc 100644
> --- a/target/arm/gdbstub64.c
> +++ b/target/arm/gdbstub64.c
> @@ -316,11 +316,11 @@ static void output_vector_union_type(GString *s, int reg_width,
>      g_string_append(s, "</union>");
>  }
>  
> -int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
> +GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int orig_base_reg)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
>      GString *s = g_string_new(NULL);
> -    DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
> +    DynamicGDBFeatureInfo *info = &cpu->dyn_svereg_feature;
>      int reg_width = cpu->sve_max_vq * 128;
>      int pred_width = cpu->sve_max_vq * 16;
>      int base_reg = orig_base_reg;
> @@ -375,7 +375,8 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
>  
>      g_string_append_printf(s, "</feature>");
>  
> -    info->desc = g_string_free(s, false);
> -    info->num = base_reg - orig_base_reg;
> -    return info->num;
> +    info->desc.xmlname = "sve-registers.xml";
> +    info->desc.xml = g_string_free(s, false);
> +    info->desc.num_regs = base_reg - orig_base_reg;
> +    return &info->desc;
>  }

Otherwise:

Acked-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro