| Message ID | 20230731084354.115015-16-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | plugins: Allow to read registers | expand |
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > These members will be used to help plugins to identify registers. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > target/arm/gdbstub.c | 46 +++++++++++++++++++++++++++--------------- > target/arm/gdbstub64.c | 42 +++++++++++++++++++++++++------------- > 2 files changed, 58 insertions(+), 30 deletions(-) > > diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c > index 100a6eed15..56d24028f6 100644 > --- a/target/arm/gdbstub.c > +++ b/target/arm/gdbstub.c > @@ -270,6 +270,7 @@ static void arm_gen_one_feature_sysreg(GString *s, > g_string_append_printf(s, " regnum=\"%d\"", regnum); > g_string_append_printf(s, " group=\"cp_regs\"/>"); > dyn_feature->data.cpregs.keys[dyn_feature->desc.num_regs] = ri_key; > + ((const char **)dyn_feature->desc.regs)[dyn_feature->desc.num_regs] = ri->name; > dyn_feature->desc.num_regs++; > } > > @@ -316,6 +317,8 @@ static GDBFeature *arm_gen_dynamic_sysreg_feature(CPUState *cs, int base_reg) > DynamicGDBFeatureInfo *dyn_feature = &cpu->dyn_sysreg_feature; > gsize num_regs = g_hash_table_size(cpu->cp_regs); > > + dyn_feature->desc.name = "org.qemu.gdb.arm.sys.regs"; > + dyn_feature->desc.regs = g_new(const char *, num_regs); AIUI this means we now have an array of register names which mirrors the names embedded in the XML. This smells like a few steps away from just abstracting the whole XML away from the targets and generating them inside gdbstub when we need them. As per my stalled attempt I referenced earlier. > 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\"?>"); > @@ -418,30 +421,34 @@ static int arm_gdb_set_m_systemreg(CPUARMState *env, uint8_t *buf, int reg) > } > > static GDBFeature *arm_gen_dynamic_m_systemreg_feature(CPUState *cs, > - int orig_base_reg) > + int base_reg) > { > ARMCPU *cpu = ARM_CPU(cs); > CPUARMState *env = &cpu->env; > GString *s = g_string_new(NULL); > - int base_reg = orig_base_reg; > - int i; > + const char **regs = g_new(const char *, ARRAY_SIZE(m_sysreg_def)); > + int i = 0; > + int j; > > 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.gnu.gdb.arm.m-system\">\n"); > > - for (i = 0; i < ARRAY_SIZE(m_sysreg_def); i++) { > - if (arm_feature(env, m_sysreg_def[i].feature)) { > + for (j = 0; j < ARRAY_SIZE(m_sysreg_def); j++) { > + if (arm_feature(env, m_sysreg_def[j].feature)) { > + regs[i] = m_sysreg_def[j].name; > g_string_append_printf(s, > "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", > - m_sysreg_def[i].name, base_reg++); > + m_sysreg_def[j].name, base_reg + i++); > } > } > > g_string_append_printf(s, "</feature>"); > + cpu->dyn_m_systemreg_feature.desc.name = "org.gnu.gdb.arm.m-system"; > 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; > + cpu->dyn_m_systemreg_feature.desc.regs = regs; > + cpu->dyn_m_systemreg_feature.desc.num_regs = i; > > return &cpu->dyn_m_systemreg_feature.desc; > } > @@ -462,30 +469,37 @@ static int arm_gdb_set_m_secextreg(CPUARMState *env, uint8_t *buf, int reg) > } > > static GDBFeature *arm_gen_dynamic_m_secextreg_feature(CPUState *cs, > - int orig_base_reg) > + int base_reg) > { > ARMCPU *cpu = ARM_CPU(cs); > GString *s = g_string_new(NULL); > - int base_reg = orig_base_reg; > - int i; > + const char **regs = g_new(const char *, ARRAY_SIZE(m_sysreg_def) * 2); > + int i = 0; > + int j; > > 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.gnu.gdb.arm.secext\">\n"); > > - for (i = 0; i < ARRAY_SIZE(m_sysreg_def); i++) { > + for (j = 0; j < ARRAY_SIZE(m_sysreg_def); j++) { > + regs[i] = g_strconcat(m_sysreg_def[j].name, "_ns", NULL); > g_string_append_printf(s, > - "<reg name=\"%s_ns\" bitsize=\"32\" regnum=\"%d\"/>\n", > - m_sysreg_def[i].name, base_reg++); > + "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", > + regs[i], base_reg + i); > + i++; > + regs[i] = g_strconcat(m_sysreg_def[j].name, "_s", NULL); > g_string_append_printf(s, > - "<reg name=\"%s_s\" bitsize=\"32\" regnum=\"%d\"/>\n", > - m_sysreg_def[i].name, base_reg++); > + "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", > + regs[i], base_reg + i); > + i++; > } > > g_string_append_printf(s, "</feature>"); > + cpu->dyn_m_secextreg_feature.desc.name = "org.gnu.gdb.arm.secext"; > 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; > + cpu->dyn_m_secextreg_feature.desc.regs = regs; > + cpu->dyn_m_secextreg_feature.desc.num_regs = i; > > return &cpu->dyn_m_secextreg_feature.desc; > } > diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c > index 20483ef9bc..c5ed7c0aa3 100644 > --- a/target/arm/gdbstub64.c > +++ b/target/arm/gdbstub64.c > @@ -316,15 +316,21 @@ static void output_vector_union_type(GString *s, int reg_width, > g_string_append(s, "</union>"); > } > > -GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int orig_base_reg) > +GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int base_reg) > { > ARMCPU *cpu = ARM_CPU(cs); > GString *s = g_string_new(NULL); > DynamicGDBFeatureInfo *info = &cpu->dyn_svereg_feature; > + const char **regs; > int reg_width = cpu->sve_max_vq * 128; > int pred_width = cpu->sve_max_vq * 16; > - int base_reg = orig_base_reg; > - int i; > + int i = 0; > + int j; > + > + info->desc.name = "org.gnu.gdb.aarch64.sve"; > + info->desc.num_regs = 32 + 16 + 4; > + regs = g_new(const char *, info->desc.num_regs); > + info->desc.regs = regs; > > g_string_printf(s, "<?xml version=\"1.0\"?>"); > g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"); > @@ -339,44 +345,52 @@ GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int orig_base_reg) > pred_width / 8); > > /* Define the vector registers. */ > - for (i = 0; i < 32; i++) { > + for (j = 0; j < 32; j++) { > + regs[i] = g_strdup_printf("z%d", j); > g_string_append_printf(s, > - "<reg name=\"z%d\" bitsize=\"%d\"" > + "<reg name=\"%s\" bitsize=\"%d\"" > " regnum=\"%d\" type=\"svev\"/>", > - i, reg_width, base_reg++); > + regs[i], reg_width, base_reg + i); > + i++; > } > > /* fpscr & status registers */ > + regs[i] = "fpsr"; > g_string_append_printf(s, "<reg name=\"fpsr\" bitsize=\"32\"" > " regnum=\"%d\" group=\"float\"" > - " type=\"int\"/>", base_reg++); > + " type=\"int\"/>", base_reg + i++); > + regs[i] = "fpcr"; > g_string_append_printf(s, "<reg name=\"fpcr\" bitsize=\"32\"" > " regnum=\"%d\" group=\"float\"" > - " type=\"int\"/>", base_reg++); > + " type=\"int\"/>", base_reg + i++); > > /* Define the predicate registers. */ > - for (i = 0; i < 16; i++) { > + for (j = 0; j < 16; j++) { > + regs[i] = g_strdup_printf("p%d", j); > g_string_append_printf(s, > - "<reg name=\"p%d\" bitsize=\"%d\"" > + "<reg name=\"%s\" bitsize=\"%d\"" > " regnum=\"%d\" type=\"svep\"/>", > - i, pred_width, base_reg++); > + regs[i], pred_width, base_reg + i); > + i++; > } > + regs[i] = "ffr"; > g_string_append_printf(s, > "<reg name=\"ffr\" bitsize=\"%d\"" > " regnum=\"%d\" group=\"vector\"" > " type=\"svep\"/>", > - pred_width, base_reg++); > + pred_width, base_reg + i++); > > /* Define the vector length pseudo-register. */ > + regs[i] = "vg"; > g_string_append_printf(s, > "<reg name=\"vg\" bitsize=\"64\"" > " regnum=\"%d\" type=\"int\"/>", > - base_reg++); > + base_reg + i++); > > g_string_append_printf(s, "</feature>"); > > info->desc.xmlname = "sve-registers.xml"; > info->desc.xml = g_string_free(s, false); > - info->desc.num_regs = base_reg - orig_base_reg; > + assert(info->desc.num_regs == i); > return &info->desc; > }
On 2023/08/14 23:56, Alex Bennée wrote: > > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > >> These members will be used to help plugins to identify registers. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> target/arm/gdbstub.c | 46 +++++++++++++++++++++++++++--------------- >> target/arm/gdbstub64.c | 42 +++++++++++++++++++++++++------------- >> 2 files changed, 58 insertions(+), 30 deletions(-) >> >> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c >> index 100a6eed15..56d24028f6 100644 >> --- a/target/arm/gdbstub.c >> +++ b/target/arm/gdbstub.c >> @@ -270,6 +270,7 @@ static void arm_gen_one_feature_sysreg(GString *s, >> g_string_append_printf(s, " regnum=\"%d\"", regnum); >> g_string_append_printf(s, " group=\"cp_regs\"/>"); >> dyn_feature->data.cpregs.keys[dyn_feature->desc.num_regs] = ri_key; >> + ((const char **)dyn_feature->desc.regs)[dyn_feature->desc.num_regs] = ri->name; >> dyn_feature->desc.num_regs++; >> } >> >> @@ -316,6 +317,8 @@ static GDBFeature *arm_gen_dynamic_sysreg_feature(CPUState *cs, int base_reg) >> DynamicGDBFeatureInfo *dyn_feature = &cpu->dyn_sysreg_feature; >> gsize num_regs = g_hash_table_size(cpu->cp_regs); >> >> + dyn_feature->desc.name = "org.qemu.gdb.arm.sys.regs"; >> + dyn_feature->desc.regs = g_new(const char *, num_regs); > > AIUI this means we now have an array of register names which mirrors the > names embedded in the XML. This smells like a few steps away from just > abstracting the whole XML away from the targets and generating them > inside gdbstub when we need them. As per my stalled attempt I referenced > earlier. The abstraction is strictly limited for identifiers. Most plugin should already have some knowledge of how registers are used. For example, a plugin that tracks stack frame for RISC-V should know sp is the stack pointer register. Similarly, a cycle simulator plugin should know how registers are used in a program. Only identifiers matter in such cases. I'm definitely *not* in favor of abstracting the whole XML for plugins. It will be too hard to maintain ABI compatibility when a new attribute emerges, for example. > > >> 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\"?>"); >> @@ -418,30 +421,34 @@ static int arm_gdb_set_m_systemreg(CPUARMState *env, uint8_t *buf, int reg) >> } >> >> static GDBFeature *arm_gen_dynamic_m_systemreg_feature(CPUState *cs, >> - int orig_base_reg) >> + int base_reg) >> { >> ARMCPU *cpu = ARM_CPU(cs); >> CPUARMState *env = &cpu->env; >> GString *s = g_string_new(NULL); >> - int base_reg = orig_base_reg; >> - int i; >> + const char **regs = g_new(const char *, ARRAY_SIZE(m_sysreg_def)); >> + int i = 0; >> + int j; >> >> 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.gnu.gdb.arm.m-system\">\n"); >> >> - for (i = 0; i < ARRAY_SIZE(m_sysreg_def); i++) { >> - if (arm_feature(env, m_sysreg_def[i].feature)) { >> + for (j = 0; j < ARRAY_SIZE(m_sysreg_def); j++) { >> + if (arm_feature(env, m_sysreg_def[j].feature)) { >> + regs[i] = m_sysreg_def[j].name; >> g_string_append_printf(s, >> "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", >> - m_sysreg_def[i].name, base_reg++); >> + m_sysreg_def[j].name, base_reg + i++); >> } >> } >> >> g_string_append_printf(s, "</feature>"); >> + cpu->dyn_m_systemreg_feature.desc.name = "org.gnu.gdb.arm.m-system"; >> 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; >> + cpu->dyn_m_systemreg_feature.desc.regs = regs; >> + cpu->dyn_m_systemreg_feature.desc.num_regs = i; >> >> return &cpu->dyn_m_systemreg_feature.desc; >> } >> @@ -462,30 +469,37 @@ static int arm_gdb_set_m_secextreg(CPUARMState *env, uint8_t *buf, int reg) >> } >> >> static GDBFeature *arm_gen_dynamic_m_secextreg_feature(CPUState *cs, >> - int orig_base_reg) >> + int base_reg) >> { >> ARMCPU *cpu = ARM_CPU(cs); >> GString *s = g_string_new(NULL); >> - int base_reg = orig_base_reg; >> - int i; >> + const char **regs = g_new(const char *, ARRAY_SIZE(m_sysreg_def) * 2); >> + int i = 0; >> + int j; >> >> 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.gnu.gdb.arm.secext\">\n"); >> >> - for (i = 0; i < ARRAY_SIZE(m_sysreg_def); i++) { >> + for (j = 0; j < ARRAY_SIZE(m_sysreg_def); j++) { >> + regs[i] = g_strconcat(m_sysreg_def[j].name, "_ns", NULL); >> g_string_append_printf(s, >> - "<reg name=\"%s_ns\" bitsize=\"32\" regnum=\"%d\"/>\n", >> - m_sysreg_def[i].name, base_reg++); >> + "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", >> + regs[i], base_reg + i); >> + i++; >> + regs[i] = g_strconcat(m_sysreg_def[j].name, "_s", NULL); >> g_string_append_printf(s, >> - "<reg name=\"%s_s\" bitsize=\"32\" regnum=\"%d\"/>\n", >> - m_sysreg_def[i].name, base_reg++); >> + "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", >> + regs[i], base_reg + i); >> + i++; >> } >> >> g_string_append_printf(s, "</feature>"); >> + cpu->dyn_m_secextreg_feature.desc.name = "org.gnu.gdb.arm.secext"; >> 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; >> + cpu->dyn_m_secextreg_feature.desc.regs = regs; >> + cpu->dyn_m_secextreg_feature.desc.num_regs = i; >> >> return &cpu->dyn_m_secextreg_feature.desc; >> } >> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c >> index 20483ef9bc..c5ed7c0aa3 100644 >> --- a/target/arm/gdbstub64.c >> +++ b/target/arm/gdbstub64.c >> @@ -316,15 +316,21 @@ static void output_vector_union_type(GString *s, int reg_width, >> g_string_append(s, "</union>"); >> } >> >> -GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int orig_base_reg) >> +GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int base_reg) >> { >> ARMCPU *cpu = ARM_CPU(cs); >> GString *s = g_string_new(NULL); >> DynamicGDBFeatureInfo *info = &cpu->dyn_svereg_feature; >> + const char **regs; >> int reg_width = cpu->sve_max_vq * 128; >> int pred_width = cpu->sve_max_vq * 16; >> - int base_reg = orig_base_reg; >> - int i; >> + int i = 0; >> + int j; >> + >> + info->desc.name = "org.gnu.gdb.aarch64.sve"; >> + info->desc.num_regs = 32 + 16 + 4; >> + regs = g_new(const char *, info->desc.num_regs); >> + info->desc.regs = regs; >> >> g_string_printf(s, "<?xml version=\"1.0\"?>"); >> g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"); >> @@ -339,44 +345,52 @@ GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int orig_base_reg) >> pred_width / 8); >> >> /* Define the vector registers. */ >> - for (i = 0; i < 32; i++) { >> + for (j = 0; j < 32; j++) { >> + regs[i] = g_strdup_printf("z%d", j); >> g_string_append_printf(s, >> - "<reg name=\"z%d\" bitsize=\"%d\"" >> + "<reg name=\"%s\" bitsize=\"%d\"" >> " regnum=\"%d\" type=\"svev\"/>", >> - i, reg_width, base_reg++); >> + regs[i], reg_width, base_reg + i); >> + i++; >> } >> >> /* fpscr & status registers */ >> + regs[i] = "fpsr"; >> g_string_append_printf(s, "<reg name=\"fpsr\" bitsize=\"32\"" >> " regnum=\"%d\" group=\"float\"" >> - " type=\"int\"/>", base_reg++); >> + " type=\"int\"/>", base_reg + i++); >> + regs[i] = "fpcr"; >> g_string_append_printf(s, "<reg name=\"fpcr\" bitsize=\"32\"" >> " regnum=\"%d\" group=\"float\"" >> - " type=\"int\"/>", base_reg++); >> + " type=\"int\"/>", base_reg + i++); >> >> /* Define the predicate registers. */ >> - for (i = 0; i < 16; i++) { >> + for (j = 0; j < 16; j++) { >> + regs[i] = g_strdup_printf("p%d", j); >> g_string_append_printf(s, >> - "<reg name=\"p%d\" bitsize=\"%d\"" >> + "<reg name=\"%s\" bitsize=\"%d\"" >> " regnum=\"%d\" type=\"svep\"/>", >> - i, pred_width, base_reg++); >> + regs[i], pred_width, base_reg + i); >> + i++; >> } >> + regs[i] = "ffr"; >> g_string_append_printf(s, >> "<reg name=\"ffr\" bitsize=\"%d\"" >> " regnum=\"%d\" group=\"vector\"" >> " type=\"svep\"/>", >> - pred_width, base_reg++); >> + pred_width, base_reg + i++); >> >> /* Define the vector length pseudo-register. */ >> + regs[i] = "vg"; >> g_string_append_printf(s, >> "<reg name=\"vg\" bitsize=\"64\"" >> " regnum=\"%d\" type=\"int\"/>", >> - base_reg++); >> + base_reg + i++); >> >> g_string_append_printf(s, "</feature>"); >> >> info->desc.xmlname = "sve-registers.xml"; >> info->desc.xml = g_string_free(s, false); >> - info->desc.num_regs = base_reg - orig_base_reg; >> + assert(info->desc.num_regs == i); >> return &info->desc; >> } > >
Akihiko Odaki <akihiko.odaki@daynix.com> writes: > On 2023/08/14 23:56, Alex Bennée wrote: >> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >> >>> These members will be used to help plugins to identify registers. >>> >>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>> --- >>> target/arm/gdbstub.c | 46 +++++++++++++++++++++++++++--------------- >>> target/arm/gdbstub64.c | 42 +++++++++++++++++++++++++------------- >>> 2 files changed, 58 insertions(+), 30 deletions(-) >>> >>> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c >>> index 100a6eed15..56d24028f6 100644 >>> --- a/target/arm/gdbstub.c >>> +++ b/target/arm/gdbstub.c >>> @@ -270,6 +270,7 @@ static void arm_gen_one_feature_sysreg(GString *s, >>> g_string_append_printf(s, " regnum=\"%d\"", regnum); >>> g_string_append_printf(s, " group=\"cp_regs\"/>"); >>> dyn_feature->data.cpregs.keys[dyn_feature->desc.num_regs] = ri_key; >>> + ((const char **)dyn_feature->desc.regs)[dyn_feature->desc.num_regs] = ri->name; >>> dyn_feature->desc.num_regs++; >>> } >>> @@ -316,6 +317,8 @@ static GDBFeature >>> *arm_gen_dynamic_sysreg_feature(CPUState *cs, int base_reg) >>> DynamicGDBFeatureInfo *dyn_feature = &cpu->dyn_sysreg_feature; >>> gsize num_regs = g_hash_table_size(cpu->cp_regs); >>> + dyn_feature->desc.name = "org.qemu.gdb.arm.sys.regs"; >>> + dyn_feature->desc.regs = g_new(const char *, num_regs); >> AIUI this means we now have an array of register names which mirrors >> the >> names embedded in the XML. This smells like a few steps away from just >> abstracting the whole XML away from the targets and generating them >> inside gdbstub when we need them. As per my stalled attempt I referenced >> earlier. > > The abstraction is strictly limited for identifiers. Most plugin > should already have some knowledge of how registers are used. For > example, a plugin that tracks stack frame for RISC-V should know sp is > the stack pointer register. Similarly, a cycle simulator plugin should > know how registers are used in a program. Only identifiers matter in > such cases. > > I'm definitely *not* in favor of abstracting the whole XML for > plugins. It will be too hard to maintain ABI compatibility when a new > attribute emerges, for example. No I agree the XML shouldn't go near the plugins. I was just looking to avoid having an XML builder for every target.
On 2023/08/17 0:03, Alex Bennée wrote: > > Akihiko Odaki <akihiko.odaki@daynix.com> writes: > >> On 2023/08/14 23:56, Alex Bennée wrote: >>> Akihiko Odaki <akihiko.odaki@daynix.com> writes: >>> >>>> These members will be used to help plugins to identify registers. >>>> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> --- >>>> target/arm/gdbstub.c | 46 +++++++++++++++++++++++++++--------------- >>>> target/arm/gdbstub64.c | 42 +++++++++++++++++++++++++------------- >>>> 2 files changed, 58 insertions(+), 30 deletions(-) >>>> >>>> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c >>>> index 100a6eed15..56d24028f6 100644 >>>> --- a/target/arm/gdbstub.c >>>> +++ b/target/arm/gdbstub.c >>>> @@ -270,6 +270,7 @@ static void arm_gen_one_feature_sysreg(GString *s, >>>> g_string_append_printf(s, " regnum=\"%d\"", regnum); >>>> g_string_append_printf(s, " group=\"cp_regs\"/>"); >>>> dyn_feature->data.cpregs.keys[dyn_feature->desc.num_regs] = ri_key; >>>> + ((const char **)dyn_feature->desc.regs)[dyn_feature->desc.num_regs] = ri->name; >>>> dyn_feature->desc.num_regs++; >>>> } >>>> @@ -316,6 +317,8 @@ static GDBFeature >>>> *arm_gen_dynamic_sysreg_feature(CPUState *cs, int base_reg) >>>> DynamicGDBFeatureInfo *dyn_feature = &cpu->dyn_sysreg_feature; >>>> gsize num_regs = g_hash_table_size(cpu->cp_regs); >>>> + dyn_feature->desc.name = "org.qemu.gdb.arm.sys.regs"; >>>> + dyn_feature->desc.regs = g_new(const char *, num_regs); >>> AIUI this means we now have an array of register names which mirrors >>> the >>> names embedded in the XML. This smells like a few steps away from just >>> abstracting the whole XML away from the targets and generating them >>> inside gdbstub when we need them. As per my stalled attempt I referenced >>> earlier. >> >> The abstraction is strictly limited for identifiers. Most plugin >> should already have some knowledge of how registers are used. For >> example, a plugin that tracks stack frame for RISC-V should know sp is >> the stack pointer register. Similarly, a cycle simulator plugin should >> know how registers are used in a program. Only identifiers matter in >> such cases. >> >> I'm definitely *not* in favor of abstracting the whole XML for >> plugins. It will be too hard to maintain ABI compatibility when a new >> attribute emerges, for example. > > No I agree the XML shouldn't go near the plugins. I was just looking to > avoid having an XML builder for every target. Oh, I see. It's done in v4 with patch "gdbstub: Introduce GDBFeatureBuilder". >
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c index 100a6eed15..56d24028f6 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -270,6 +270,7 @@ static void arm_gen_one_feature_sysreg(GString *s, g_string_append_printf(s, " regnum=\"%d\"", regnum); g_string_append_printf(s, " group=\"cp_regs\"/>"); dyn_feature->data.cpregs.keys[dyn_feature->desc.num_regs] = ri_key; + ((const char **)dyn_feature->desc.regs)[dyn_feature->desc.num_regs] = ri->name; dyn_feature->desc.num_regs++; } @@ -316,6 +317,8 @@ static GDBFeature *arm_gen_dynamic_sysreg_feature(CPUState *cs, int base_reg) DynamicGDBFeatureInfo *dyn_feature = &cpu->dyn_sysreg_feature; gsize num_regs = g_hash_table_size(cpu->cp_regs); + dyn_feature->desc.name = "org.qemu.gdb.arm.sys.regs"; + dyn_feature->desc.regs = g_new(const char *, num_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\"?>"); @@ -418,30 +421,34 @@ static int arm_gdb_set_m_systemreg(CPUARMState *env, uint8_t *buf, int reg) } static GDBFeature *arm_gen_dynamic_m_systemreg_feature(CPUState *cs, - int orig_base_reg) + int base_reg) { ARMCPU *cpu = ARM_CPU(cs); CPUARMState *env = &cpu->env; GString *s = g_string_new(NULL); - int base_reg = orig_base_reg; - int i; + const char **regs = g_new(const char *, ARRAY_SIZE(m_sysreg_def)); + int i = 0; + int j; 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.gnu.gdb.arm.m-system\">\n"); - for (i = 0; i < ARRAY_SIZE(m_sysreg_def); i++) { - if (arm_feature(env, m_sysreg_def[i].feature)) { + for (j = 0; j < ARRAY_SIZE(m_sysreg_def); j++) { + if (arm_feature(env, m_sysreg_def[j].feature)) { + regs[i] = m_sysreg_def[j].name; g_string_append_printf(s, "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", - m_sysreg_def[i].name, base_reg++); + m_sysreg_def[j].name, base_reg + i++); } } g_string_append_printf(s, "</feature>"); + cpu->dyn_m_systemreg_feature.desc.name = "org.gnu.gdb.arm.m-system"; 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; + cpu->dyn_m_systemreg_feature.desc.regs = regs; + cpu->dyn_m_systemreg_feature.desc.num_regs = i; return &cpu->dyn_m_systemreg_feature.desc; } @@ -462,30 +469,37 @@ static int arm_gdb_set_m_secextreg(CPUARMState *env, uint8_t *buf, int reg) } static GDBFeature *arm_gen_dynamic_m_secextreg_feature(CPUState *cs, - int orig_base_reg) + int base_reg) { ARMCPU *cpu = ARM_CPU(cs); GString *s = g_string_new(NULL); - int base_reg = orig_base_reg; - int i; + const char **regs = g_new(const char *, ARRAY_SIZE(m_sysreg_def) * 2); + int i = 0; + int j; 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.gnu.gdb.arm.secext\">\n"); - for (i = 0; i < ARRAY_SIZE(m_sysreg_def); i++) { + for (j = 0; j < ARRAY_SIZE(m_sysreg_def); j++) { + regs[i] = g_strconcat(m_sysreg_def[j].name, "_ns", NULL); g_string_append_printf(s, - "<reg name=\"%s_ns\" bitsize=\"32\" regnum=\"%d\"/>\n", - m_sysreg_def[i].name, base_reg++); + "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", + regs[i], base_reg + i); + i++; + regs[i] = g_strconcat(m_sysreg_def[j].name, "_s", NULL); g_string_append_printf(s, - "<reg name=\"%s_s\" bitsize=\"32\" regnum=\"%d\"/>\n", - m_sysreg_def[i].name, base_reg++); + "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", + regs[i], base_reg + i); + i++; } g_string_append_printf(s, "</feature>"); + cpu->dyn_m_secextreg_feature.desc.name = "org.gnu.gdb.arm.secext"; 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; + cpu->dyn_m_secextreg_feature.desc.regs = regs; + cpu->dyn_m_secextreg_feature.desc.num_regs = i; return &cpu->dyn_m_secextreg_feature.desc; } diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c index 20483ef9bc..c5ed7c0aa3 100644 --- a/target/arm/gdbstub64.c +++ b/target/arm/gdbstub64.c @@ -316,15 +316,21 @@ static void output_vector_union_type(GString *s, int reg_width, g_string_append(s, "</union>"); } -GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int orig_base_reg) +GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int base_reg) { ARMCPU *cpu = ARM_CPU(cs); GString *s = g_string_new(NULL); DynamicGDBFeatureInfo *info = &cpu->dyn_svereg_feature; + const char **regs; int reg_width = cpu->sve_max_vq * 128; int pred_width = cpu->sve_max_vq * 16; - int base_reg = orig_base_reg; - int i; + int i = 0; + int j; + + info->desc.name = "org.gnu.gdb.aarch64.sve"; + info->desc.num_regs = 32 + 16 + 4; + regs = g_new(const char *, info->desc.num_regs); + info->desc.regs = regs; g_string_printf(s, "<?xml version=\"1.0\"?>"); g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"); @@ -339,44 +345,52 @@ GDBFeature *arm_gen_dynamic_svereg_feature(CPUState *cs, int orig_base_reg) pred_width / 8); /* Define the vector registers. */ - for (i = 0; i < 32; i++) { + for (j = 0; j < 32; j++) { + regs[i] = g_strdup_printf("z%d", j); g_string_append_printf(s, - "<reg name=\"z%d\" bitsize=\"%d\"" + "<reg name=\"%s\" bitsize=\"%d\"" " regnum=\"%d\" type=\"svev\"/>", - i, reg_width, base_reg++); + regs[i], reg_width, base_reg + i); + i++; } /* fpscr & status registers */ + regs[i] = "fpsr"; g_string_append_printf(s, "<reg name=\"fpsr\" bitsize=\"32\"" " regnum=\"%d\" group=\"float\"" - " type=\"int\"/>", base_reg++); + " type=\"int\"/>", base_reg + i++); + regs[i] = "fpcr"; g_string_append_printf(s, "<reg name=\"fpcr\" bitsize=\"32\"" " regnum=\"%d\" group=\"float\"" - " type=\"int\"/>", base_reg++); + " type=\"int\"/>", base_reg + i++); /* Define the predicate registers. */ - for (i = 0; i < 16; i++) { + for (j = 0; j < 16; j++) { + regs[i] = g_strdup_printf("p%d", j); g_string_append_printf(s, - "<reg name=\"p%d\" bitsize=\"%d\"" + "<reg name=\"%s\" bitsize=\"%d\"" " regnum=\"%d\" type=\"svep\"/>", - i, pred_width, base_reg++); + regs[i], pred_width, base_reg + i); + i++; } + regs[i] = "ffr"; g_string_append_printf(s, "<reg name=\"ffr\" bitsize=\"%d\"" " regnum=\"%d\" group=\"vector\"" " type=\"svep\"/>", - pred_width, base_reg++); + pred_width, base_reg + i++); /* Define the vector length pseudo-register. */ + regs[i] = "vg"; g_string_append_printf(s, "<reg name=\"vg\" bitsize=\"64\"" " regnum=\"%d\" type=\"int\"/>", - base_reg++); + base_reg + i++); g_string_append_printf(s, "</feature>"); info->desc.xmlname = "sve-registers.xml"; info->desc.xml = g_string_free(s, false); - info->desc.num_regs = base_reg - orig_base_reg; + assert(info->desc.num_regs == i); return &info->desc; }
These members will be used to help plugins to identify registers. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- target/arm/gdbstub.c | 46 +++++++++++++++++++++++++++--------------- target/arm/gdbstub64.c | 42 +++++++++++++++++++++++++------------- 2 files changed, 58 insertions(+), 30 deletions(-)