[17/29] gdbstub: Simplify XML lookup

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

Now we know all instances of GDBFeature that is used in CPU so we can
traverse them to find XML. This removes the need for a CPU-specific
lookup function for dynamic XMLs.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20231025093128.33116-11-akihiko.odaki@daynix.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/exec/gdbstub.h |   6 +++
 gdbstub/gdbstub.c      | 118 +++++++++++++++++++++--------------------
 hw/core/cpu-common.c   |   5 +-
 3 files changed, 69 insertions(+), 60 deletions(-)

Hello Alex and Akihiko,

this patch introduces a regression for riscv.
When connecting to gdb, gdb issues the infamous "Architecture rejected
target-supplied description" warning.
As this patch touches "common" QEMU files (not riscv ones that I am
a bit more familiar with and am able to test), I will probably not
be able to come out with a proper patch, so I leave it to you guys.

Fixing this might also fix an other issue with selecting registers on
QEMU command line when using Alex "new" execlog: for riscv there is no
match on the general purpose registers that are given in
gdb-xml/riscv-64bit-cpu.xml.

And by the way thanks for this very useful feature!
Frédéric

Le 03/11/2023 à 20:59, Alex Bennée a écrit :
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> Now we know all instances of GDBFeature that is used in CPU so we can
> traverse them to find XML. This removes the need for a CPU-specific
> lookup function for dynamic XMLs.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20231025093128.33116-11-akihiko.odaki@daynix.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/exec/gdbstub.h |   6 +++
>   gdbstub/gdbstub.c      | 118 +++++++++++++++++++++--------------------
>   hw/core/cpu-common.c   |   5 +-
>   3 files changed, 69 insertions(+), 60 deletions(-)
> 
> diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
> index bcaab1bc75..82a8afa237 100644
> --- a/include/exec/gdbstub.h
> +++ b/include/exec/gdbstub.h
> @@ -27,6 +27,12 @@ typedef struct GDBFeatureBuilder {
>   typedef int (*gdb_get_reg_cb)(CPUState *cpu, GByteArray *buf, int reg);
>   typedef int (*gdb_set_reg_cb)(CPUState *cpu, uint8_t *buf, int reg);
>   
> +/**
> + * gdb_init_cpu(): Initialize the CPU for gdbstub.
> + * @cpu: The CPU to be initialized.
> + */
> +void gdb_init_cpu(CPUState *cpu);
> +
>   /**
>    * gdb_register_coprocessor() - register a supplemental set of registers
>    * @cpu - the CPU associated with registers
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 4809c90c4a..5ecd1f23e6 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -352,6 +352,7 @@ static const char *get_feature_xml(const char *p, const char **newp,
>   {
>       CPUState *cpu = gdb_get_first_cpu_in_process(process);
>       CPUClass *cc = CPU_GET_CLASS(cpu);
> +    GDBRegisterState *r;
>       size_t len;
>   
>       /*
> @@ -365,7 +366,6 @@ static const char *get_feature_xml(const char *p, const char **newp,
>       /* Is it the main target xml? */
>       if (strncmp(p, "target.xml", len) == 0) {
>           if (!process->target_xml) {
> -            GDBRegisterState *r;
>               g_autoptr(GPtrArray) xml = g_ptr_array_new_with_free_func(g_free);
>   
>               g_ptr_array_add(
> @@ -380,18 +380,12 @@ static const char *get_feature_xml(const char *p, const char **newp,
>                       g_markup_printf_escaped("<architecture>%s</architecture>",
>                                               cc->gdb_arch_name(cpu)));
>               }
> -            g_ptr_array_add(
> -                xml,
> -                g_markup_printf_escaped("<xi:include href=\"%s\"/>",
> -                                        cc->gdb_core_xml_file));
> -            if (cpu->gdb_regs) {
> -                for (guint i = 0; i < cpu->gdb_regs->len; i++) {
> -                    r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> -                    g_ptr_array_add(
> -                        xml,
> -                        g_markup_printf_escaped("<xi:include href=\"%s\"/>",
> -                                                r->feature->xmlname));
> -                }
> +            for (guint i = 0; i < cpu->gdb_regs->len; i++) {
> +                r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> +                g_ptr_array_add(
> +                    xml,
> +                    g_markup_printf_escaped("<xi:include href=\"%s\"/>",
> +                                            r->feature->xmlname));
>               }
>               g_ptr_array_add(xml, g_strdup("</target>"));
>               g_ptr_array_add(xml, NULL);
> @@ -400,20 +394,11 @@ static const char *get_feature_xml(const char *p, const char **newp,
>           }
>           return process->target_xml;
>       }
> -    /* Is it dynamically generated by the target? */
> -    if (cc->gdb_get_dynamic_xml) {
> -        g_autofree char *xmlname = g_strndup(p, len);
> -        const char *xml = cc->gdb_get_dynamic_xml(cpu, xmlname);
> -        if (xml) {
> -            return xml;
> -        }
> -    }
> -    /* Is it one of the encoded gdb-xml/ files? */
> -    for (int i = 0; gdb_static_features[i].xmlname; i++) {
> -        const char *name = gdb_static_features[i].xmlname;
> -        if ((strncmp(name, p, len) == 0) &&
> -            strlen(name) == len) {
> -            return gdb_static_features[i].xml;
> +    /* Is it one of the features? */
> +    for (guint i = 0; i < cpu->gdb_regs->len; i++) {
> +        r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> +        if (strncmp(p, r->feature->xmlname, len) == 0) {
> +            return r->feature->xml;
>           }
>       }
>   
> @@ -508,12 +493,10 @@ static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg)
>           return cc->gdb_read_register(cpu, buf, reg);
>       }
>   
> -    if (cpu->gdb_regs) {
> -        for (guint i = 0; i < cpu->gdb_regs->len; i++) {
> -            r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> -            if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
> -                return r->get_reg(cpu, buf, reg - r->base_reg);
> -            }
> +    for (guint i = 0; i < cpu->gdb_regs->len; i++) {
> +        r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> +        if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
> +            return r->get_reg(cpu, buf, reg - r->base_reg);
>           }
>       }
>       return 0;
> @@ -528,51 +511,70 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg)
>           return cc->gdb_write_register(cpu, mem_buf, reg);
>       }
>   
> -    if (cpu->gdb_regs) {
> -        for (guint i = 0; i < cpu->gdb_regs->len; i++) {
> -            r =  &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> -            if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
> -                return r->set_reg(cpu, mem_buf, reg - r->base_reg);
> -            }
> +    for (guint i = 0; i < cpu->gdb_regs->len; i++) {
> +        r =  &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> +        if (r->base_reg <= reg && reg < r->base_reg + r->feature->num_regs) {
> +            return r->set_reg(cpu, mem_buf, reg - r->base_reg);
>           }
>       }
>       return 0;
>   }
>   
> +static void gdb_register_feature(CPUState *cpu, int base_reg,
> +                                 gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
> +                                 const GDBFeature *feature)
> +{
> +    GDBRegisterState s = {
> +        .base_reg = base_reg,
> +        .get_reg = get_reg,
> +        .set_reg = set_reg,
> +        .feature = feature
> +    };
> +
> +    g_array_append_val(cpu->gdb_regs, s);
> +}
> +
> +void gdb_init_cpu(CPUState *cpu)
> +{
> +    CPUClass *cc = CPU_GET_CLASS(cpu);
> +    const GDBFeature *feature;
> +
> +    cpu->gdb_regs = g_array_new(false, false, sizeof(GDBRegisterState));
> +
> +    if (cc->gdb_core_xml_file) {
> +        feature = gdb_find_static_feature(cc->gdb_core_xml_file);
> +        gdb_register_feature(cpu, 0,
> +                             cc->gdb_read_register, cc->gdb_write_register,
> +                             feature);
> +    }
> +
> +    cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
> +}
> +
>   void gdb_register_coprocessor(CPUState *cpu,
>                                 gdb_get_reg_cb get_reg, gdb_set_reg_cb set_reg,
>                                 const GDBFeature *feature, int g_pos)
>   {
>       GDBRegisterState *s;
>       guint i;
> +    int base_reg = cpu->gdb_num_regs;
>   
> -    if (cpu->gdb_regs) {
> -        for (i = 0; i < cpu->gdb_regs->len; i++) {
> -            /* Check for duplicates.  */
> -            s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> -            if (s->feature == feature) {
> -                return;
> -            }
> +    for (i = 0; i < cpu->gdb_regs->len; i++) {
> +        /* Check for duplicates.  */
> +        s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> +        if (s->feature == feature) {
> +            return;
>           }
> -    } else {
> -        cpu->gdb_regs = g_array_new(false, false, sizeof(GDBRegisterState));
> -        i = 0;
>       }
>   
> -    g_array_set_size(cpu->gdb_regs, i + 1);
> -    s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i);
> -    s->base_reg = cpu->gdb_num_regs;
> -    s->get_reg = get_reg;
> -    s->set_reg = set_reg;
> -    s->feature = feature;
> +    gdb_register_feature(cpu, base_reg, get_reg, set_reg, feature);
>   
>       /* Add to end of list.  */
>       cpu->gdb_num_regs += feature->num_regs;
>       if (g_pos) {
> -        if (g_pos != s->base_reg) {
> +        if (g_pos != base_reg) {
>               error_report("Error: Bad gdb register numbering for '%s', "
> -                         "expected %d got %d", feature->xml,
> -                         g_pos, s->base_reg);
> +                         "expected %d got %d", feature->xml, g_pos, base_reg);
>           } else {
>               cpu->gdb_num_g_regs = cpu->gdb_num_regs;
>           }
> diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c
> index bab8942c30..2a2a6eb3eb 100644
> --- a/hw/core/cpu-common.c
> +++ b/hw/core/cpu-common.c
> @@ -27,6 +27,7 @@
>   #include "qemu/main-loop.h"
>   #include "exec/log.h"
>   #include "exec/cpu-common.h"
> +#include "exec/gdbstub.h"
>   #include "qemu/error-report.h"
>   #include "qemu/qemu-print.h"
>   #include "sysemu/tcg.h"
> @@ -223,11 +224,10 @@ static void cpu_common_unrealizefn(DeviceState *dev)
>   static void cpu_common_initfn(Object *obj)
>   {
>       CPUState *cpu = CPU(obj);
> -    CPUClass *cc = CPU_GET_CLASS(obj);
>   
> +    gdb_init_cpu(cpu);
>       cpu->cpu_index = UNASSIGNED_CPU_INDEX;
>       cpu->cluster_index = UNASSIGNED_CLUSTER_INDEX;
> -    cpu->gdb_num_regs = cpu->gdb_num_g_regs = cc->gdb_num_core_regs;
>       /* user-mode doesn't have configurable SMP topology */
>       /* the default value is changed by qemu_init_vcpu() for system-mode */
>       cpu->nr_cores = 1;
> @@ -247,6 +247,7 @@ static void cpu_common_finalize(Object *obj)
>   {
>       CPUState *cpu = CPU(obj);
>   
> +    g_array_free(cpu->gdb_regs, TRUE);
>       qemu_lockcnt_destroy(&cpu->in_ioctl_lock);
>       qemu_mutex_destroy(&cpu->work_mutex);
>   }

Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> writes:

> Hello Alex and Akihiko,
>
> this patch introduces a regression for riscv.
> When connecting to gdb, gdb issues the infamous "Architecture rejected
> target-supplied description" warning.

I tracked it down to 13/29 when bisecting which I dropped from:

  Message-Id: <20231106185112.2755262-1-alex.bennee@linaro.org>
  Date: Mon,  6 Nov 2023 18:50:50 +0000
  Subject: [PATCH 00/22] Maintainer updates for 8.2 (gdbstub, tests, plugins) pre-PR
  From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>

So if you can check that series doesn't regress RiscV (it passes the
tests) then we can at least get some of the stuff merged during freeze.

Le 07/11/2023 à 11:31, Alex Bennée a écrit :
> Frédéric Pétrot <frederic.petrot@univ-grenoble-alpes.fr> writes:
> 
>> Hello Alex and Akihiko,
>>
>> this patch introduces a regression for riscv.
>> When connecting to gdb, gdb issues the infamous "Architecture rejected
>> target-supplied description" warning.
> 
> I tracked it down to 13/29 when bisecting which I dropped from:
> 
>    Message-Id: <20231106185112.2755262-1-alex.bennee@linaro.org>
>    Date: Mon,  6 Nov 2023 18:50:50 +0000
>    Subject: [PATCH 00/22] Maintainer updates for 8.2 (gdbstub, tests, plugins) pre-PR
>    From: =?UTF-8?q?Alex=20Benn=C3=A9e?= <alex.bennee@linaro.org>
> 
> So if you can check that series doesn't regress RiscV (it passes the
> tests) then we can at least get some of the stuff merged during freeze.

   I do confirm that this fixes the issue with gdb, thanks !