diff mbox series

[13/29] target/riscv: Use GDBFeature for dynamic XML

Message ID 20231103195956.1998255-14-alex.bennee@linaro.org
State Superseded
Headers show
Series gdbstub and plugin read register and windows support | expand

Commit Message

Alex Bennée Nov. 3, 2023, 7:59 p.m. UTC 
From: Akihiko Odaki <akihiko.odaki@daynix.com>

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>
Message-Id: <20231025093128.33116-7-akihiko.odaki@daynix.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/riscv/cpu.h     |  5 +--
 target/riscv/cpu.c     |  4 +--
 target/riscv/gdbstub.c | 79 +++++++++++++++++++-----------------------
 3 files changed, 40 insertions(+), 48 deletions(-)

Comments

Alex Bennée Nov. 6, 2023, 9:32 a.m. UTC | #1 
Alex Bennée <alex.bennee@linaro.org> writes:

> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>
> 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>
> Message-Id: <20231025093128.33116-7-akihiko.odaki@daynix.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

I bisected the following failure:

 ./qemu-riscv64 -g 1234 ./tests/tcg/riscv64-linux-user/sha512

and:

 gdb-multiarch ./tests/tcg/riscv64-linux-user/sha512 -ex "target remote localhost:1234" -x ../../tests/tcg/multiarch/gdbstub/registers.py

gives:

warning: Architecture rejected target-supplied description
Ignoring packet error, continuing...
Ignoring packet error, continuing...
Ignoring packet error, continuing...
Ignoring packet error, continuing...

> ---
>  target/riscv/cpu.h     |  5 +--
>  target/riscv/cpu.c     |  4 +--
>  target/riscv/gdbstub.c | 79 +++++++++++++++++++-----------------------
>  3 files changed, 40 insertions(+), 48 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index f8ffa5ee38..73ec1d3b79 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -24,6 +24,7 @@
>  #include "hw/registerfields.h"
>  #include "hw/qdev-properties.h"
>  #include "exec/cpu-defs.h"
> +#include "exec/gdbstub.h"
>  #include "qemu/cpu-float.h"
>  #include "qom/object.h"
>  #include "qemu/int128.h"
> @@ -395,8 +396,8 @@ struct ArchCPU {
>  
>      CPURISCVState env;
>  
> -    char *dyn_csr_xml;
> -    char *dyn_vreg_xml;
> +    GDBFeature dyn_csr_feature;
> +    GDBFeature dyn_vreg_feature;
>  
>      /* Configuration Settings */
>      RISCVCPUConfig cfg;
> diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
> index ac4a6c7eec..5200fba9b9 100644
> --- a/target/riscv/cpu.c
> +++ b/target/riscv/cpu.c
> @@ -1421,9 +1421,9 @@ static const char *riscv_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
>      RISCVCPU *cpu = RISCV_CPU(cs);
>  
>      if (strcmp(xmlname, "riscv-csr.xml") == 0) {
> -        return cpu->dyn_csr_xml;
> +        return cpu->dyn_csr_feature.xml;
>      } else if (strcmp(xmlname, "riscv-vector.xml") == 0) {
> -        return cpu->dyn_vreg_xml;
> +        return cpu->dyn_vreg_feature.xml;
>      }
>  
>      return NULL;
> diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
> index 524bede865..a3ac0212d1 100644
> --- a/target/riscv/gdbstub.c
> +++ b/target/riscv/gdbstub.c
> @@ -212,12 +212,13 @@ static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
>      return 0;
>  }
>  
> -static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
> +static GDBFeature *riscv_gen_dynamic_csr_feature(CPUState *cs, int base_reg)
>  {
>      RISCVCPU *cpu = RISCV_CPU(cs);
>      CPURISCVState *env = &cpu->env;
> -    GString *s = g_string_new(NULL);
> +    GDBFeatureBuilder builder;
>      riscv_csr_predicate_fn predicate;
> +    const char *name;
>      int bitsize = 16 << env->misa_mxl_max;
>      int i;
>  
> @@ -230,9 +231,9 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
>          bitsize = 64;
>      }
>  
> -    g_string_printf(s, "<?xml version=\"1.0\"?>");
> -    g_string_append_printf(s, "<!DOCTYPE feature SYSTEM \"gdb-target.dtd\">");
> -    g_string_append_printf(s, "<feature name=\"org.gnu.gdb.riscv.csr\">");
> +    gdb_feature_builder_init(&builder, &cpu->dyn_csr_feature,
> +                             "org.gnu.gdb.riscv.csr", "riscv-csr.xml",
> +                             base_reg);
>  
>      for (i = 0; i < CSR_TABLE_SIZE; i++) {
>          if (env->priv_ver < csr_ops[i].min_priv_ver) {
> @@ -240,72 +241,64 @@ static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
>          }
>          predicate = csr_ops[i].predicate;
>          if (predicate && (predicate(env, i) == RISCV_EXCP_NONE)) {
> -            if (csr_ops[i].name) {
> -                g_string_append_printf(s, "<reg name=\"%s\"", csr_ops[i].name);
> -            } else {
> -                g_string_append_printf(s, "<reg name=\"csr%03x\"", i);
> +            g_autofree char *dynamic_name = NULL;
> +            name = csr_ops[i].name;
> +            if (!name) {
> +                dynamic_name = g_strdup_printf("csr%03x", i);
> +                name = dynamic_name;
>              }
> -            g_string_append_printf(s, " bitsize=\"%d\"", bitsize);
> -            g_string_append_printf(s, " regnum=\"%d\"/>", base_reg + i);
> +
> +            gdb_feature_builder_append_reg(&builder, name, bitsize, i,
> +                                           "int", NULL);
>          }
>      }
>  
> -    g_string_append_printf(s, "</feature>");
> -
> -    cpu->dyn_csr_xml = g_string_free(s, false);
> +    gdb_feature_builder_end(&builder);
>  
>  #if !defined(CONFIG_USER_ONLY)
>      env->debugger = false;
>  #endif
>  
> -    return CSR_TABLE_SIZE;
> +    return &cpu->dyn_csr_feature;
>  }
>  
> -static int ricsv_gen_dynamic_vector_xml(CPUState *cs, int base_reg)
> +static GDBFeature *ricsv_gen_dynamic_vector_feature(CPUState *cs, int base_reg)
>  {
>      RISCVCPU *cpu = RISCV_CPU(cs);
> -    GString *s = g_string_new(NULL);
> -    g_autoptr(GString) ts = g_string_new("");
> +    GDBFeatureBuilder builder;
>      int reg_width = cpu->cfg.vlen;
> -    int num_regs = 0;
>      int i;
>  
> -    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.riscv.vector\">");
> +    gdb_feature_builder_init(&builder, &cpu->dyn_vreg_feature,
> +                             "org.gnu.gdb.riscv.vector", "riscv-vector.xml",
> +                             base_reg);
>  
>      /* First define types and totals in a whole VL */
>      for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
>          int count = reg_width / vec_lanes[i].size;
> -        g_string_printf(ts, "%s", vec_lanes[i].id);
> -        g_string_append_printf(s,
> -                               "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>",
> -                               ts->str, vec_lanes[i].gdb_type, count);
> +        gdb_feature_builder_append_tag(
> +            &builder, "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>",
> +            vec_lanes[i].id, vec_lanes[i].gdb_type, count);
>      }
>  
>      /* Define unions */
> -    g_string_append_printf(s, "<union id=\"riscv_vector\">");
> +    gdb_feature_builder_append_tag(&builder, "<union id=\"riscv_vector\">");
>      for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
> -        g_string_append_printf(s, "<field name=\"%c\" type=\"%s\"/>",
> -                               vec_lanes[i].suffix,
> -                               vec_lanes[i].id);
> +        gdb_feature_builder_append_tag(&builder,
> +                                       "<field name=\"%c\" type=\"%s\"/>",
> +                                       vec_lanes[i].suffix, vec_lanes[i].id);
>      }
> -    g_string_append(s, "</union>");
> +    gdb_feature_builder_append_tag(&builder, "</union>");
>  
>      /* Define vector registers */
>      for (i = 0; i < 32; i++) {
> -        g_string_append_printf(s,
> -                               "<reg name=\"v%d\" bitsize=\"%d\""
> -                               " regnum=\"%d\" group=\"vector\""
> -                               " type=\"riscv_vector\"/>",
> -                               i, reg_width, base_reg++);
> -        num_regs++;
> +        gdb_feature_builder_append_reg(&builder, g_strdup_printf("v%d", i),
> +                                       reg_width, i, "riscv_vector", "vector");
>      }
>  
> -    g_string_append_printf(s, "</feature>");
> +    gdb_feature_builder_end(&builder);
>  
> -    cpu->dyn_vreg_xml = g_string_free(s, false);
> -    return num_regs;
> +    return &cpu->dyn_vreg_feature;
>  }
>  
>  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
> @@ -320,10 +313,9 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>                                   32, "riscv-32bit-fpu.xml", 0);
>      }
>      if (env->misa_ext & RVV) {
> -        int base_reg = cs->gdb_num_regs;
>          gdb_register_coprocessor(cs, riscv_gdb_get_vector,
>                                   riscv_gdb_set_vector,
> -                                 ricsv_gen_dynamic_vector_xml(cs, base_reg),
> +                                 ricsv_gen_dynamic_vector_feature(cs, cs->gdb_num_regs)->num_regs,
>                                   "riscv-vector.xml", 0);
>      }
>      switch (env->misa_mxl_max) {
> @@ -343,9 +335,8 @@ void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
>      }
>  
>      if (cpu->cfg.ext_icsr) {
> -        int base_reg = cs->gdb_num_regs;
>          gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
> -                                 riscv_gen_dynamic_csr_xml(cs, base_reg),
> +                                 riscv_gen_dynamic_csr_feature(cs, cs->gdb_num_regs)->num_regs,
>                                   "riscv-csr.xml", 0);
>      }
>  }
Alex Bennée Nov. 6, 2023, 3:35 p.m. UTC | #2 
Alex Bennée <alex.bennee@linaro.org> writes:

> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>>
>> 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>
>> Message-Id: <20231025093128.33116-7-akihiko.odaki@daynix.com>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> I bisected the following failure:
>
>  ./qemu-riscv64 -g 1234 ./tests/tcg/riscv64-linux-user/sha512
>
> and:
>
>  gdb-multiarch ./tests/tcg/riscv64-linux-user/sha512 -ex "target remote localhost:1234" -x ../../tests/tcg/multiarch/gdbstub/registers.py
>
> gives:
>
> warning: Architecture rejected target-supplied description
> Ignoring packet error, continuing...
> Ignoring packet error, continuing...
> Ignoring packet error, continuing...
> Ignoring packet error, continuing...

I don't know if the discontinuous register numbering could be the
problem:

  <feature name="org.gnu.gdb.riscv.csr">
    <reg name="fflags" bitsize="64" type="int" regnum="67"/>
    <reg name="frm" bitsize="64" type="int" regnum="68"/>
    <reg name="fcsr" bitsize="64" type="int" regnum="69"/>
    <reg name="cycle" bitsize="64" type="int" regnum="3138"/>
    <reg name="time" bitsize="64" type="int" regnum="3139"/>
    <reg name="instret" bitsize="64" type="int" regnum="3140"/>
  </feature>

?
diff mbox series

Patch

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index f8ffa5ee38..73ec1d3b79 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -24,6 +24,7 @@ 
 #include "hw/registerfields.h"
 #include "hw/qdev-properties.h"
 #include "exec/cpu-defs.h"
+#include "exec/gdbstub.h"
 #include "qemu/cpu-float.h"
 #include "qom/object.h"
 #include "qemu/int128.h"
@@ -395,8 +396,8 @@  struct ArchCPU {
 
     CPURISCVState env;
 
-    char *dyn_csr_xml;
-    char *dyn_vreg_xml;
+    GDBFeature dyn_csr_feature;
+    GDBFeature dyn_vreg_feature;
 
     /* Configuration Settings */
     RISCVCPUConfig cfg;
diff --git a/target/riscv/cpu.c b/target/riscv/cpu.c
index ac4a6c7eec..5200fba9b9 100644
--- a/target/riscv/cpu.c
+++ b/target/riscv/cpu.c
@@ -1421,9 +1421,9 @@  static const char *riscv_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
     RISCVCPU *cpu = RISCV_CPU(cs);
 
     if (strcmp(xmlname, "riscv-csr.xml") == 0) {
-        return cpu->dyn_csr_xml;
+        return cpu->dyn_csr_feature.xml;
     } else if (strcmp(xmlname, "riscv-vector.xml") == 0) {
-        return cpu->dyn_vreg_xml;
+        return cpu->dyn_vreg_feature.xml;
     }
 
     return NULL;
diff --git a/target/riscv/gdbstub.c b/target/riscv/gdbstub.c
index 524bede865..a3ac0212d1 100644
--- a/target/riscv/gdbstub.c
+++ b/target/riscv/gdbstub.c
@@ -212,12 +212,13 @@  static int riscv_gdb_set_virtual(CPURISCVState *cs, uint8_t *mem_buf, int n)
     return 0;
 }
 
-static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
+static GDBFeature *riscv_gen_dynamic_csr_feature(CPUState *cs, int base_reg)
 {
     RISCVCPU *cpu = RISCV_CPU(cs);
     CPURISCVState *env = &cpu->env;
-    GString *s = g_string_new(NULL);
+    GDBFeatureBuilder builder;
     riscv_csr_predicate_fn predicate;
+    const char *name;
     int bitsize = 16 << env->misa_mxl_max;
     int i;
 
@@ -230,9 +231,9 @@  static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
         bitsize = 64;
     }
 
-    g_string_printf(s, "<?xml version=\"1.0\"?>");
-    g_string_append_printf(s, "<!DOCTYPE feature SYSTEM \"gdb-target.dtd\">");
-    g_string_append_printf(s, "<feature name=\"org.gnu.gdb.riscv.csr\">");
+    gdb_feature_builder_init(&builder, &cpu->dyn_csr_feature,
+                             "org.gnu.gdb.riscv.csr", "riscv-csr.xml",
+                             base_reg);
 
     for (i = 0; i < CSR_TABLE_SIZE; i++) {
         if (env->priv_ver < csr_ops[i].min_priv_ver) {
@@ -240,72 +241,64 @@  static int riscv_gen_dynamic_csr_xml(CPUState *cs, int base_reg)
         }
         predicate = csr_ops[i].predicate;
         if (predicate && (predicate(env, i) == RISCV_EXCP_NONE)) {
-            if (csr_ops[i].name) {
-                g_string_append_printf(s, "<reg name=\"%s\"", csr_ops[i].name);
-            } else {
-                g_string_append_printf(s, "<reg name=\"csr%03x\"", i);
+            g_autofree char *dynamic_name = NULL;
+            name = csr_ops[i].name;
+            if (!name) {
+                dynamic_name = g_strdup_printf("csr%03x", i);
+                name = dynamic_name;
             }
-            g_string_append_printf(s, " bitsize=\"%d\"", bitsize);
-            g_string_append_printf(s, " regnum=\"%d\"/>", base_reg + i);
+
+            gdb_feature_builder_append_reg(&builder, name, bitsize, i,
+                                           "int", NULL);
         }
     }
 
-    g_string_append_printf(s, "</feature>");
-
-    cpu->dyn_csr_xml = g_string_free(s, false);
+    gdb_feature_builder_end(&builder);
 
 #if !defined(CONFIG_USER_ONLY)
     env->debugger = false;
 #endif
 
-    return CSR_TABLE_SIZE;
+    return &cpu->dyn_csr_feature;
 }
 
-static int ricsv_gen_dynamic_vector_xml(CPUState *cs, int base_reg)
+static GDBFeature *ricsv_gen_dynamic_vector_feature(CPUState *cs, int base_reg)
 {
     RISCVCPU *cpu = RISCV_CPU(cs);
-    GString *s = g_string_new(NULL);
-    g_autoptr(GString) ts = g_string_new("");
+    GDBFeatureBuilder builder;
     int reg_width = cpu->cfg.vlen;
-    int num_regs = 0;
     int i;
 
-    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.riscv.vector\">");
+    gdb_feature_builder_init(&builder, &cpu->dyn_vreg_feature,
+                             "org.gnu.gdb.riscv.vector", "riscv-vector.xml",
+                             base_reg);
 
     /* First define types and totals in a whole VL */
     for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
         int count = reg_width / vec_lanes[i].size;
-        g_string_printf(ts, "%s", vec_lanes[i].id);
-        g_string_append_printf(s,
-                               "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>",
-                               ts->str, vec_lanes[i].gdb_type, count);
+        gdb_feature_builder_append_tag(
+            &builder, "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>",
+            vec_lanes[i].id, vec_lanes[i].gdb_type, count);
     }
 
     /* Define unions */
-    g_string_append_printf(s, "<union id=\"riscv_vector\">");
+    gdb_feature_builder_append_tag(&builder, "<union id=\"riscv_vector\">");
     for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
-        g_string_append_printf(s, "<field name=\"%c\" type=\"%s\"/>",
-                               vec_lanes[i].suffix,
-                               vec_lanes[i].id);
+        gdb_feature_builder_append_tag(&builder,
+                                       "<field name=\"%c\" type=\"%s\"/>",
+                                       vec_lanes[i].suffix, vec_lanes[i].id);
     }
-    g_string_append(s, "</union>");
+    gdb_feature_builder_append_tag(&builder, "</union>");
 
     /* Define vector registers */
     for (i = 0; i < 32; i++) {
-        g_string_append_printf(s,
-                               "<reg name=\"v%d\" bitsize=\"%d\""
-                               " regnum=\"%d\" group=\"vector\""
-                               " type=\"riscv_vector\"/>",
-                               i, reg_width, base_reg++);
-        num_regs++;
+        gdb_feature_builder_append_reg(&builder, g_strdup_printf("v%d", i),
+                                       reg_width, i, "riscv_vector", "vector");
     }
 
-    g_string_append_printf(s, "</feature>");
+    gdb_feature_builder_end(&builder);
 
-    cpu->dyn_vreg_xml = g_string_free(s, false);
-    return num_regs;
+    return &cpu->dyn_vreg_feature;
 }
 
 void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
@@ -320,10 +313,9 @@  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
                                  32, "riscv-32bit-fpu.xml", 0);
     }
     if (env->misa_ext & RVV) {
-        int base_reg = cs->gdb_num_regs;
         gdb_register_coprocessor(cs, riscv_gdb_get_vector,
                                  riscv_gdb_set_vector,
-                                 ricsv_gen_dynamic_vector_xml(cs, base_reg),
+                                 ricsv_gen_dynamic_vector_feature(cs, cs->gdb_num_regs)->num_regs,
                                  "riscv-vector.xml", 0);
     }
     switch (env->misa_mxl_max) {
@@ -343,9 +335,8 @@  void riscv_cpu_register_gdb_regs_for_features(CPUState *cs)
     }
 
     if (cpu->cfg.ext_icsr) {
-        int base_reg = cs->gdb_num_regs;
         gdb_register_coprocessor(cs, riscv_gdb_get_csr, riscv_gdb_set_csr,
-                                 riscv_gen_dynamic_csr_xml(cs, base_reg),
+                                 riscv_gen_dynamic_csr_feature(cs, cs->gdb_num_regs)->num_regs,
                                  "riscv-csr.xml", 0);
     }
 }