qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 17/29] gdbstub: Simplify XML lookup


From: Frédéric Pétrot
Subject: Re: [PATCH 17/29] gdbstub: Simplify XML lookup
Date: Tue, 7 Nov 2023 09:46:21 +0100
User-agent: Mozilla Thunderbird

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,                            Pr. Grenoble INP-Ensimag/TIMA |
| Mob/Pho: +33 6 74 57 99 65/+33 4 76 57 48 70      Ad augusta  per angusta |
| http://tima.univ-grenoble-alpes.fr frederic.petrot@univ-grenoble-alpes.fr |
+---------------------------------------------------------------------------+



reply via email to

[Prev in Thread] Current Thread [Next in Thread]