Re: [RFC PATCH 15/24] target/arm: Fill new members of GDBFeature

[Alex Bennée]

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;
>  }


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro