Re: [PATCH 17/29] gdbstub: Simplify XML lookup
Le 07/11/2023 à 11:31, Alex Bennée a écrit : Frédéric Pétrot 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.ben...@linaro.org> Date: Mon, 6 Nov 2023 18:50:50 + Subject: [PATCH 00/22] Maintainer updates for 8.2 (gdbstub, tests, plugins) pre-PR From: =?UTF-8?q?Alex=20Benn=C3=A9e?= 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 ! -- +---+ | 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.pet...@univ-grenoble-alpes.fr | +---+
Re: [PATCH 17/29] gdbstub: Simplify XML lookup
Frédéric Pétrot 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.ben...@linaro.org> Date: Mon, 6 Nov 2023 18:50:50 + Subject: [PATCH 00/22] Maintainer updates for 8.2 (gdbstub, tests, plugins) pre-PR From: =?UTF-8?q?Alex=20Benn=C3=A9e?= 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. -- Alex Bennée Virtualisation Tech Lead @ Linaro
Re: [PATCH 17/29] gdbstub: Simplify XML lookup
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
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
Reviewed-by: Alex Bennée
Message-Id: <20231025093128.33116-11-akihiko.od...@daynix.com>
Signed-off-by: Alex Bennée
---
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("%s",
cc->gdb_arch_name(cpu)));
}
-g_ptr_array_add(
-xml,
-g_markup_printf_escaped("",
-cc->gdb_core_xml_file));
-if (cpu->gdb_regs) {
-for (guint i = 0; i < cpu->gdb_regs->len; i++) {
-r = _array_index(cpu->gdb_regs, GDBRegisterState, i);
-g_ptr_array_add(
-xml,
-g_markup_printf_escaped("",
-r->feature->xmlname));
-}
+for (guint i = 0; i < cpu->gdb_regs->len; i++) {
+r = _array_index(cpu->gdb_regs, GDBRegisterState, i);
+g_ptr_array_add(
+xml,
+g_markup_printf_escaped("",
+r->feature->xmlname));
}
g_ptr_array_add(xml, g_strdup(""));
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 = _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);
}
-
[PATCH 17/29] gdbstub: Simplify XML lookup
From: Akihiko Odaki
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
Reviewed-by: Alex Bennée
Message-Id: <20231025093128.33116-11-akihiko.od...@daynix.com>
Signed-off-by: Alex Bennée
---
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("%s",
cc->gdb_arch_name(cpu)));
}
-g_ptr_array_add(
-xml,
-g_markup_printf_escaped("",
-cc->gdb_core_xml_file));
-if (cpu->gdb_regs) {
-for (guint i = 0; i < cpu->gdb_regs->len; i++) {
-r = _array_index(cpu->gdb_regs, GDBRegisterState, i);
-g_ptr_array_add(
-xml,
-g_markup_printf_escaped("",
-r->feature->xmlname));
-}
+for (guint i = 0; i < cpu->gdb_regs->len; i++) {
+r = _array_index(cpu->gdb_regs, GDBRegisterState, i);
+g_ptr_array_add(
+xml,
+g_markup_printf_escaped("",
+r->feature->xmlname));
}
g_ptr_array_add(xml, g_strdup(""));
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 = _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 = _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 = _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);
}
