Akihiko Odaki <akihiko.od...@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.od...@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, ¶m); > + g_hash_table_foreach(cpu->cp_regs, arm_register_sysreg_for_feature, > ¶m); > 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.ben...@linaro.org> -- Alex Bennée Virtualisation Tech Lead @ Linaro
