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

[Akihiko Odaki]

On 2023/07/31 22:44, Philippe Mathieu-Daudé wrote:
> On 31/7/23 10:43, Akihiko Odaki wrote:
> > 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.
> >
> > 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;
>
> Since now DynamicGDBFeatureInfo.desc contains xmlname, we can replace
> all these by a generic 'DynamicGDBFeatureInfo *dyn_features' and have
> arm_gdb_get_dynamic_xml() looking up the xmlname.

In patch 12, cpu-specific gdb_get_dynamic_xml() function is removed, and 
gdbstub instead looks up an internal list it holds.

> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> > @@ -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;