* [PATCH 01/14] target/arm: Normalize aarch64 gdbstub get/set function names
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-14 18:55 ` Fabiano Rosas
2023-02-14 16:30 ` [PATCH 02/14] target/arm: Unexport arm_gen_dynamic_sysreg_xml Richard Henderson
` (12 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Make the form of the function names between fp and sve the same:
- arm_gdb_*_svereg -> aarch64_gdb_*_sve_reg.
- aarch64_fpu_gdb_*_reg -> aarch64_gdb_*_fpu_reg.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/internals.h | 8 ++++----
target/arm/gdbstub.c | 9 +++++----
target/arm/gdbstub64.c | 8 ++++----
3 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index e1e018da46..69b5e7ba73 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1340,10 +1340,10 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
}
#ifdef TARGET_AARCH64
-int arm_gdb_get_svereg(CPUARMState *env, GByteArray *buf, int reg);
-int arm_gdb_set_svereg(CPUARMState *env, uint8_t *buf, int reg);
-int aarch64_fpu_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg);
-int aarch64_fpu_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg);
+int aarch64_gdb_get_sve_reg(CPUARMState *env, GByteArray *buf, int reg);
+int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg);
+int aarch64_gdb_get_fpu_reg(CPUARMState *env, GByteArray *buf, int reg);
+int aarch64_gdb_set_fpu_reg(CPUARMState *env, uint8_t *buf, int reg);
void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
void arm_cpu_sme_finalize(ARMCPU *cpu, Error **errp);
void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 2f806512d0..cf1c01e3cf 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -466,12 +466,13 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
*/
#ifdef TARGET_AARCH64
if (isar_feature_aa64_sve(&cpu->isar)) {
- gdb_register_coprocessor(cs, arm_gdb_get_svereg, arm_gdb_set_svereg,
- arm_gen_dynamic_svereg_xml(cs, cs->gdb_num_regs),
+ int nreg = arm_gen_dynamic_svereg_xml(cs, cs->gdb_num_regs);
+ gdb_register_coprocessor(cs, aarch64_gdb_get_sve_reg,
+ aarch64_gdb_set_sve_reg, nreg,
"sve-registers.xml", 0);
} else {
- gdb_register_coprocessor(cs, aarch64_fpu_gdb_get_reg,
- aarch64_fpu_gdb_set_reg,
+ gdb_register_coprocessor(cs, aarch64_gdb_get_fpu_reg,
+ aarch64_gdb_set_fpu_reg,
34, "aarch64-fpu.xml", 0);
}
#endif
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 07a6746944..c598cb0375 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -72,7 +72,7 @@ int aarch64_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
return 0;
}
-int aarch64_fpu_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
+int aarch64_gdb_get_fpu_reg(CPUARMState *env, GByteArray *buf, int reg)
{
switch (reg) {
case 0 ... 31:
@@ -92,7 +92,7 @@ int aarch64_fpu_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
}
}
-int aarch64_fpu_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)
+int aarch64_gdb_set_fpu_reg(CPUARMState *env, uint8_t *buf, int reg)
{
switch (reg) {
case 0 ... 31:
@@ -116,7 +116,7 @@ int aarch64_fpu_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)
}
}
-int arm_gdb_get_svereg(CPUARMState *env, GByteArray *buf, int reg)
+int aarch64_gdb_get_sve_reg(CPUARMState *env, GByteArray *buf, int reg)
{
ARMCPU *cpu = env_archcpu(env);
@@ -164,7 +164,7 @@ int arm_gdb_get_svereg(CPUARMState *env, GByteArray *buf, int reg)
return 0;
}
-int arm_gdb_set_svereg(CPUARMState *env, uint8_t *buf, int reg)
+int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg)
{
ARMCPU *cpu = env_archcpu(env);
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 02/14] target/arm: Unexport arm_gen_dynamic_sysreg_xml
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
2023-02-14 16:30 ` [PATCH 01/14] target/arm: Normalize aarch64 gdbstub get/set function names Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-14 18:57 ` Fabiano Rosas
2023-02-14 16:30 ` [PATCH 03/14] target/arm: Move arm_gen_dynamic_svereg_xml to gdbstub64.c Richard Henderson
` (11 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
This function is not used outside gdbstub.c.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/cpu.h | 1 -
target/arm/gdbstub.c | 2 +-
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 7bc97fece9..b2c49b3605 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1114,7 +1114,6 @@ int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
* Helpers to dynamically generates XML descriptions of the sysregs
* and SVE registers. Returns the number of registers in each set.
*/
-int arm_gen_dynamic_sysreg_xml(CPUState *cpu, int base_reg);
int arm_gen_dynamic_svereg_xml(CPUState *cpu, int base_reg);
/* Returns the dynamically generated XML for the gdb stub.
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index cf1c01e3cf..52581e9784 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -305,7 +305,7 @@ static void arm_register_sysreg_for_xml(gpointer key, gpointer value,
}
}
-int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg)
+static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg)
{
ARMCPU *cpu = ARM_CPU(cs);
GString *s = g_string_new(NULL);
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 03/14] target/arm: Move arm_gen_dynamic_svereg_xml to gdbstub64.c
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
2023-02-14 16:30 ` [PATCH 01/14] target/arm: Normalize aarch64 gdbstub get/set function names Richard Henderson
2023-02-14 16:30 ` [PATCH 02/14] target/arm: Unexport arm_gen_dynamic_sysreg_xml Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-14 19:08 ` Fabiano Rosas
2023-02-14 16:30 ` [PATCH 04/14] target/arm: Split out output_vector_union_type Richard Henderson
` (10 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
The function is only used for aarch64, so move it to the
file that has the other aarch64 gdbstub stuff. Move the
declaration to internals.h.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/cpu.h | 6 ---
target/arm/internals.h | 1 +
target/arm/gdbstub.c | 120 -----------------------------------------
target/arm/gdbstub64.c | 118 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 119 insertions(+), 126 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b2c49b3605..c9f768f945 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1110,12 +1110,6 @@ hwaddr arm_cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr addr,
int arm_cpu_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
-/*
- * Helpers to dynamically generates XML descriptions of the sysregs
- * and SVE registers. Returns the number of registers in each set.
- */
-int arm_gen_dynamic_svereg_xml(CPUState *cpu, int base_reg);
-
/* Returns the dynamically generated XML for the gdb stub.
* Returns a pointer to the XML contents for the specified XML file or NULL
* if the XML name doesn't match the predefined one.
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 69b5e7ba73..c98482561e 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1340,6 +1340,7 @@ static inline uint64_t pmu_counter_mask(CPUARMState *env)
}
#ifdef TARGET_AARCH64
+int arm_gen_dynamic_svereg_xml(CPUState *cpu, int base_reg);
int aarch64_gdb_get_sve_reg(CPUARMState *env, GByteArray *buf, int reg);
int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg);
int aarch64_gdb_get_fpu_reg(CPUARMState *env, GByteArray *buf, int reg);
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 52581e9784..bf8aff7824 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -322,126 +322,6 @@ static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg)
return cpu->dyn_sysreg_xml.num;
}
-struct TypeSize {
- const char *gdb_type;
- int size;
- const char sz, suffix;
-};
-
-static const struct TypeSize vec_lanes[] = {
- /* quads */
- { "uint128", 128, 'q', 'u' },
- { "int128", 128, 'q', 's' },
- /* 64 bit */
- { "ieee_double", 64, 'd', 'f' },
- { "uint64", 64, 'd', 'u' },
- { "int64", 64, 'd', 's' },
- /* 32 bit */
- { "ieee_single", 32, 's', 'f' },
- { "uint32", 32, 's', 'u' },
- { "int32", 32, 's', 's' },
- /* 16 bit */
- { "ieee_half", 16, 'h', 'f' },
- { "uint16", 16, 'h', 'u' },
- { "int16", 16, 'h', 's' },
- /* bytes */
- { "uint8", 8, 'b', 'u' },
- { "int8", 8, 'b', 's' },
-};
-
-
-int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
-{
- ARMCPU *cpu = ARM_CPU(cs);
- GString *s = g_string_new(NULL);
- DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
- g_autoptr(GString) ts = g_string_new("");
- int i, j, bits, reg_width = (cpu->sve_max_vq * 128);
- info->num = 0;
- 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.aarch64.sve\">");
-
- /* 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, "svev%c%c", vec_lanes[i].sz, vec_lanes[i].suffix);
- g_string_append_printf(s,
- "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>",
- ts->str, vec_lanes[i].gdb_type, count);
- }
- /*
- * Now define a union for each size group containing unsigned and
- * signed and potentially float versions of each size from 128 to
- * 8 bits.
- */
- for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
- const char suf[] = { 'q', 'd', 's', 'h', 'b' };
- g_string_append_printf(s, "<union id=\"svevn%c\">", suf[i]);
- for (j = 0; j < ARRAY_SIZE(vec_lanes); j++) {
- if (vec_lanes[j].size == bits) {
- g_string_append_printf(s, "<field name=\"%c\" type=\"svev%c%c\"/>",
- vec_lanes[j].suffix,
- vec_lanes[j].sz, vec_lanes[j].suffix);
- }
- }
- g_string_append(s, "</union>");
- }
- /* And now the final union of unions */
- g_string_append(s, "<union id=\"svev\">");
- for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
- const char suf[] = { 'q', 'd', 's', 'h', 'b' };
- g_string_append_printf(s, "<field name=\"%c\" type=\"svevn%c\"/>",
- suf[i], suf[i]);
- }
- g_string_append(s, "</union>");
-
- /* Finally the sve prefix type */
- g_string_append_printf(s,
- "<vector id=\"svep\" type=\"uint8\" count=\"%d\"/>",
- reg_width / 8);
-
- /* Then define each register in parts for each vq */
- for (i = 0; i < 32; i++) {
- g_string_append_printf(s,
- "<reg name=\"z%d\" bitsize=\"%d\""
- " regnum=\"%d\" type=\"svev\"/>",
- i, reg_width, base_reg++);
- info->num++;
- }
- /* fpscr & status registers */
- g_string_append_printf(s, "<reg name=\"fpsr\" bitsize=\"32\""
- " regnum=\"%d\" group=\"float\""
- " type=\"int\"/>", base_reg++);
- g_string_append_printf(s, "<reg name=\"fpcr\" bitsize=\"32\""
- " regnum=\"%d\" group=\"float\""
- " type=\"int\"/>", base_reg++);
- info->num += 2;
-
- for (i = 0; i < 16; i++) {
- g_string_append_printf(s,
- "<reg name=\"p%d\" bitsize=\"%d\""
- " regnum=\"%d\" type=\"svep\"/>",
- i, cpu->sve_max_vq * 16, base_reg++);
- info->num++;
- }
- g_string_append_printf(s,
- "<reg name=\"ffr\" bitsize=\"%d\""
- " regnum=\"%d\" group=\"vector\""
- " type=\"svep\"/>",
- cpu->sve_max_vq * 16, base_reg++);
- g_string_append_printf(s,
- "<reg name=\"vg\" bitsize=\"64\""
- " regnum=\"%d\" type=\"int\"/>",
- base_reg++);
- info->num += 2;
- g_string_append_printf(s, "</feature>");
- cpu->dyn_svereg_xml.desc = g_string_free(s, false);
-
- return cpu->dyn_svereg_xml.num;
-}
-
-
const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
{
ARMCPU *cpu = ARM_CPU(cs);
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index c598cb0375..59fb5465d5 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -209,3 +209,121 @@ int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg)
return 0;
}
+
+struct TypeSize {
+ const char *gdb_type;
+ short size;
+ char sz, suffix;
+};
+
+static const struct TypeSize vec_lanes[] = {
+ /* quads */
+ { "uint128", 128, 'q', 'u' },
+ { "int128", 128, 'q', 's' },
+ /* 64 bit */
+ { "ieee_double", 64, 'd', 'f' },
+ { "uint64", 64, 'd', 'u' },
+ { "int64", 64, 'd', 's' },
+ /* 32 bit */
+ { "ieee_single", 32, 's', 'f' },
+ { "uint32", 32, 's', 'u' },
+ { "int32", 32, 's', 's' },
+ /* 16 bit */
+ { "ieee_half", 16, 'h', 'f' },
+ { "uint16", 16, 'h', 'u' },
+ { "int16", 16, 'h', 's' },
+ /* bytes */
+ { "uint8", 8, 'b', 'u' },
+ { "int8", 8, 'b', 's' },
+};
+
+int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
+{
+ ARMCPU *cpu = ARM_CPU(cs);
+ GString *s = g_string_new(NULL);
+ DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
+ g_autoptr(GString) ts = g_string_new("");
+ int i, j, bits, reg_width = (cpu->sve_max_vq * 128);
+ info->num = 0;
+ 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.aarch64.sve\">");
+
+ /* 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, "svev%c%c", vec_lanes[i].sz, vec_lanes[i].suffix);
+ g_string_append_printf(s,
+ "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>",
+ ts->str, vec_lanes[i].gdb_type, count);
+ }
+ /*
+ * Now define a union for each size group containing unsigned and
+ * signed and potentially float versions of each size from 128 to
+ * 8 bits.
+ */
+ for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
+ const char suf[] = { 'q', 'd', 's', 'h', 'b' };
+ g_string_append_printf(s, "<union id=\"svevn%c\">", suf[i]);
+ for (j = 0; j < ARRAY_SIZE(vec_lanes); j++) {
+ if (vec_lanes[j].size == bits) {
+ g_string_append_printf(s, "<field name=\"%c\" type=\"svev%c%c\"/>",
+ vec_lanes[j].suffix,
+ vec_lanes[j].sz, vec_lanes[j].suffix);
+ }
+ }
+ g_string_append(s, "</union>");
+ }
+ /* And now the final union of unions */
+ g_string_append(s, "<union id=\"svev\">");
+ for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
+ const char suf[] = { 'q', 'd', 's', 'h', 'b' };
+ g_string_append_printf(s, "<field name=\"%c\" type=\"svevn%c\"/>",
+ suf[i], suf[i]);
+ }
+ g_string_append(s, "</union>");
+
+ /* Finally the sve prefix type */
+ g_string_append_printf(s,
+ "<vector id=\"svep\" type=\"uint8\" count=\"%d\"/>",
+ reg_width / 8);
+
+ /* Then define each register in parts for each vq */
+ for (i = 0; i < 32; i++) {
+ g_string_append_printf(s,
+ "<reg name=\"z%d\" bitsize=\"%d\""
+ " regnum=\"%d\" type=\"svev\"/>",
+ i, reg_width, base_reg++);
+ info->num++;
+ }
+ /* fpscr & status registers */
+ g_string_append_printf(s, "<reg name=\"fpsr\" bitsize=\"32\""
+ " regnum=\"%d\" group=\"float\""
+ " type=\"int\"/>", base_reg++);
+ g_string_append_printf(s, "<reg name=\"fpcr\" bitsize=\"32\""
+ " regnum=\"%d\" group=\"float\""
+ " type=\"int\"/>", base_reg++);
+ info->num += 2;
+
+ for (i = 0; i < 16; i++) {
+ g_string_append_printf(s,
+ "<reg name=\"p%d\" bitsize=\"%d\""
+ " regnum=\"%d\" type=\"svep\"/>",
+ i, cpu->sve_max_vq * 16, base_reg++);
+ info->num++;
+ }
+ g_string_append_printf(s,
+ "<reg name=\"ffr\" bitsize=\"%d\""
+ " regnum=\"%d\" group=\"vector\""
+ " type=\"svep\"/>",
+ cpu->sve_max_vq * 16, base_reg++);
+ g_string_append_printf(s,
+ "<reg name=\"vg\" bitsize=\"64\""
+ " regnum=\"%d\" type=\"int\"/>",
+ base_reg++);
+ info->num += 2;
+ g_string_append_printf(s, "</feature>");
+ info->desc = g_string_free(s, false);
+
+ return info->num;
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 04/14] target/arm: Split out output_vector_union_type
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (2 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 03/14] target/arm: Move arm_gen_dynamic_svereg_xml to gdbstub64.c Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-14 19:35 ` Fabiano Rosas
2023-02-20 16:07 ` Peter Maydell
2023-02-14 16:30 ` [PATCH 05/14] target/arm: Simplify register counting in arm_gen_dynamic_svereg_xml Richard Henderson
` (9 subsequent siblings)
13 siblings, 2 replies; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Create a subroutine for creating the union of unions
of the various type sizes that a vector may contain.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/gdbstub64.c | 83 +++++++++++++++++++++++-------------------
1 file changed, 45 insertions(+), 38 deletions(-)
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 59fb5465d5..811833d8de 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -210,44 +210,39 @@ int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg)
return 0;
}
-struct TypeSize {
- const char *gdb_type;
- short size;
- char sz, suffix;
-};
-
-static const struct TypeSize vec_lanes[] = {
- /* quads */
- { "uint128", 128, 'q', 'u' },
- { "int128", 128, 'q', 's' },
- /* 64 bit */
- { "ieee_double", 64, 'd', 'f' },
- { "uint64", 64, 'd', 'u' },
- { "int64", 64, 'd', 's' },
- /* 32 bit */
- { "ieee_single", 32, 's', 'f' },
- { "uint32", 32, 's', 'u' },
- { "int32", 32, 's', 's' },
- /* 16 bit */
- { "ieee_half", 16, 'h', 'f' },
- { "uint16", 16, 'h', 'u' },
- { "int16", 16, 'h', 's' },
- /* bytes */
- { "uint8", 8, 'b', 'u' },
- { "int8", 8, 'b', 's' },
-};
-
-int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
+static void output_vector_union_type(GString *s, int reg_width)
{
- ARMCPU *cpu = ARM_CPU(cs);
- GString *s = g_string_new(NULL);
- DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
+ struct TypeSize {
+ const char *gdb_type;
+ short size;
+ char sz, suffix;
+ };
+
+ static const struct TypeSize vec_lanes[] = {
+ /* quads */
+ { "uint128", 128, 'q', 'u' },
+ { "int128", 128, 'q', 's' },
+ /* 64 bit */
+ { "ieee_double", 64, 'd', 'f' },
+ { "uint64", 64, 'd', 'u' },
+ { "int64", 64, 'd', 's' },
+ /* 32 bit */
+ { "ieee_single", 32, 's', 'f' },
+ { "uint32", 32, 's', 'u' },
+ { "int32", 32, 's', 's' },
+ /* 16 bit */
+ { "ieee_half", 16, 'h', 'f' },
+ { "uint16", 16, 'h', 'u' },
+ { "int16", 16, 'h', 's' },
+ /* bytes */
+ { "uint8", 8, 'b', 'u' },
+ { "int8", 8, 'b', 's' },
+ };
+
+ static const char suf[] = { 'q', 'd', 's', 'h', 'b' };
+
g_autoptr(GString) ts = g_string_new("");
- int i, j, bits, reg_width = (cpu->sve_max_vq * 128);
- info->num = 0;
- 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.aarch64.sve\">");
+ int i, j, bits;
/* First define types and totals in a whole VL */
for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
@@ -263,7 +258,6 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
* 8 bits.
*/
for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
- const char suf[] = { 'q', 'd', 's', 'h', 'b' };
g_string_append_printf(s, "<union id=\"svevn%c\">", suf[i]);
for (j = 0; j < ARRAY_SIZE(vec_lanes); j++) {
if (vec_lanes[j].size == bits) {
@@ -277,11 +271,24 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
/* And now the final union of unions */
g_string_append(s, "<union id=\"svev\">");
for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
- const char suf[] = { 'q', 'd', 's', 'h', 'b' };
g_string_append_printf(s, "<field name=\"%c\" type=\"svevn%c\"/>",
suf[i], suf[i]);
}
g_string_append(s, "</union>");
+}
+
+int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
+{
+ ARMCPU *cpu = ARM_CPU(cs);
+ GString *s = g_string_new(NULL);
+ DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
+ int i, reg_width = (cpu->sve_max_vq * 128);
+ info->num = 0;
+ 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.aarch64.sve\">");
+
+ output_vector_union_type(s, reg_width);
/* Finally the sve prefix type */
g_string_append_printf(s,
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 05/14] target/arm: Simplify register counting in arm_gen_dynamic_svereg_xml
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (3 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 04/14] target/arm: Split out output_vector_union_type Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-14 19:42 ` Fabiano Rosas
2023-02-14 16:30 ` [PATCH 06/14] target/arm: Hoist pred_width " Richard Henderson
` (8 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Rather than increment base_reg and num, compute num
from the change to base_reg at the end. Clean up some
nearby comments.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/gdbstub64.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 811833d8de..8d174ff6e0 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -277,32 +277,35 @@ static void output_vector_union_type(GString *s, int reg_width)
g_string_append(s, "</union>");
}
-int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
+int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
{
ARMCPU *cpu = ARM_CPU(cs);
GString *s = g_string_new(NULL);
DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
- int i, reg_width = (cpu->sve_max_vq * 128);
- info->num = 0;
+ int reg_width = cpu->sve_max_vq * 128;
+ int base_reg = orig_base_reg;
+ 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.aarch64.sve\">");
+ /* Create the vector union type. */
output_vector_union_type(s, reg_width);
- /* Finally the sve prefix type */
+ /* Create the predicate vector type. */
g_string_append_printf(s,
"<vector id=\"svep\" type=\"uint8\" count=\"%d\"/>",
reg_width / 8);
- /* Then define each register in parts for each vq */
+ /* Define the vector registers. */
for (i = 0; i < 32; i++) {
g_string_append_printf(s,
"<reg name=\"z%d\" bitsize=\"%d\""
" regnum=\"%d\" type=\"svev\"/>",
i, reg_width, base_reg++);
- info->num++;
}
+
/* fpscr & status registers */
g_string_append_printf(s, "<reg name=\"fpsr\" bitsize=\"32\""
" regnum=\"%d\" group=\"float\""
@@ -310,8 +313,8 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
g_string_append_printf(s, "<reg name=\"fpcr\" bitsize=\"32\""
" regnum=\"%d\" group=\"float\""
" type=\"int\"/>", base_reg++);
- info->num += 2;
+ /* Define the predicate registers. */
for (i = 0; i < 16; i++) {
g_string_append_printf(s,
"<reg name=\"p%d\" bitsize=\"%d\""
@@ -324,13 +327,16 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
" regnum=\"%d\" group=\"vector\""
" type=\"svep\"/>",
cpu->sve_max_vq * 16, base_reg++);
+
+ /* Define the vector length pseudo-register. */
g_string_append_printf(s,
"<reg name=\"vg\" bitsize=\"64\""
" regnum=\"%d\" type=\"int\"/>",
base_reg++);
- info->num += 2;
- g_string_append_printf(s, "</feature>");
- info->desc = g_string_free(s, false);
+ g_string_append_printf(s, "</feature>");
+
+ info->desc = g_string_free(s, false);
+ info->num = base_reg - orig_base_reg;
return info->num;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 05/14] target/arm: Simplify register counting in arm_gen_dynamic_svereg_xml
2023-02-14 16:30 ` [PATCH 05/14] target/arm: Simplify register counting in arm_gen_dynamic_svereg_xml Richard Henderson
@ 2023-02-14 19:42 ` Fabiano Rosas
2023-02-14 22:56 ` Richard Henderson
0 siblings, 1 reply; 35+ messages in thread
From: Fabiano Rosas @ 2023-02-14 19:42 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: qemu-arm
Richard Henderson <richard.henderson@linaro.org> writes:
> Rather than increment base_reg and num, compute num
> from the change to base_reg at the end. Clean up some
> nearby comments.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/gdbstub64.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> index 811833d8de..8d174ff6e0 100644
> --- a/target/arm/gdbstub64.c
> +++ b/target/arm/gdbstub64.c
> @@ -277,32 +277,35 @@ static void output_vector_union_type(GString *s, int reg_width)
> g_string_append(s, "</union>");
> }
>
> -int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
> +int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
> {
> ARMCPU *cpu = ARM_CPU(cs);
> GString *s = g_string_new(NULL);
> DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
> - int i, reg_width = (cpu->sve_max_vq * 128);
> - info->num = 0;
> + int reg_width = cpu->sve_max_vq * 128;
> + int base_reg = orig_base_reg;
> + 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.aarch64.sve\">");
>
> + /* Create the vector union type. */
> output_vector_union_type(s, reg_width);
>
> - /* Finally the sve prefix type */
> + /* Create the predicate vector type. */
> g_string_append_printf(s,
> "<vector id=\"svep\" type=\"uint8\" count=\"%d\"/>",
> reg_width / 8);
>
> - /* Then define each register in parts for each vq */
> + /* Define the vector registers. */
> for (i = 0; i < 32; i++) {
> g_string_append_printf(s,
> "<reg name=\"z%d\" bitsize=\"%d\""
> " regnum=\"%d\" type=\"svev\"/>",
> i, reg_width, base_reg++);
> - info->num++;
> }
> +
> /* fpscr & status registers */
> g_string_append_printf(s, "<reg name=\"fpsr\" bitsize=\"32\""
> " regnum=\"%d\" group=\"float\""
> @@ -310,8 +313,8 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
> g_string_append_printf(s, "<reg name=\"fpcr\" bitsize=\"32\""
> " regnum=\"%d\" group=\"float\""
> " type=\"int\"/>", base_reg++);
> - info->num += 2;
>
> + /* Define the predicate registers. */
> for (i = 0; i < 16; i++) {
There's a info->num++; at the end of this loop.
> g_string_append_printf(s,
> "<reg name=\"p%d\" bitsize=\"%d\""
> @@ -324,13 +327,16 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
> " regnum=\"%d\" group=\"vector\""
> " type=\"svep\"/>",
> cpu->sve_max_vq * 16, base_reg++);
> +
> + /* Define the vector length pseudo-register. */
> g_string_append_printf(s,
> "<reg name=\"vg\" bitsize=\"64\""
> " regnum=\"%d\" type=\"int\"/>",
> base_reg++);
> - info->num += 2;
> - g_string_append_printf(s, "</feature>");
> - info->desc = g_string_free(s, false);
>
> + g_string_append_printf(s, "</feature>");
> +
> + info->desc = g_string_free(s, false);
> + info->num = base_reg - orig_base_reg;
> return info->num;
> }
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 05/14] target/arm: Simplify register counting in arm_gen_dynamic_svereg_xml
2023-02-14 19:42 ` Fabiano Rosas
@ 2023-02-14 22:56 ` Richard Henderson
0 siblings, 0 replies; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 22:56 UTC (permalink / raw)
To: Fabiano Rosas, qemu-devel; +Cc: qemu-arm
On 2/14/23 09:42, Fabiano Rosas wrote:
>> @@ -310,8 +313,8 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int base_reg)
>> g_string_append_printf(s, "<reg name=\"fpcr\" bitsize=\"32\""
>> " regnum=\"%d\" group=\"float\""
>> " type=\"int\"/>", base_reg++);
>> - info->num += 2;
>>
>> + /* Define the predicate registers. */
>> for (i = 0; i < 16; i++) {
>
> There's a info->num++; at the end of this loop.
Good catch, thanks.
r~
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 06/14] target/arm: Hoist pred_width in arm_gen_dynamic_svereg_xml
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (4 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 05/14] target/arm: Simplify register counting in arm_gen_dynamic_svereg_xml Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-14 19:44 ` Fabiano Rosas
2023-02-14 16:30 ` [PATCH 07/14] target/arm: Fix svep width " Richard Henderson
` (7 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/gdbstub64.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 8d174ff6e0..02a0256c5c 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -283,6 +283,7 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
GString *s = g_string_new(NULL);
DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
int reg_width = cpu->sve_max_vq * 128;
+ int pred_width = cpu->sve_max_vq * 16;
int base_reg = orig_base_reg;
int i;
@@ -319,14 +320,14 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
g_string_append_printf(s,
"<reg name=\"p%d\" bitsize=\"%d\""
" regnum=\"%d\" type=\"svep\"/>",
- i, cpu->sve_max_vq * 16, base_reg++);
+ i, pred_width, base_reg++);
info->num++;
}
g_string_append_printf(s,
"<reg name=\"ffr\" bitsize=\"%d\""
" regnum=\"%d\" group=\"vector\""
" type=\"svep\"/>",
- cpu->sve_max_vq * 16, base_reg++);
+ pred_width, base_reg++);
/* Define the vector length pseudo-register. */
g_string_append_printf(s,
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 06/14] target/arm: Hoist pred_width in arm_gen_dynamic_svereg_xml
2023-02-14 16:30 ` [PATCH 06/14] target/arm: Hoist pred_width " Richard Henderson
@ 2023-02-14 19:44 ` Fabiano Rosas
0 siblings, 0 replies; 35+ messages in thread
From: Fabiano Rosas @ 2023-02-14 19:44 UTC (permalink / raw)
To: Richard Henderson, qemu-devel; +Cc: qemu-arm
Richard Henderson <richard.henderson@linaro.org> writes:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/gdbstub64.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
> index 8d174ff6e0..02a0256c5c 100644
> --- a/target/arm/gdbstub64.c
> +++ b/target/arm/gdbstub64.c
> @@ -283,6 +283,7 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
> GString *s = g_string_new(NULL);
> DynamicGDBXMLInfo *info = &cpu->dyn_svereg_xml;
> int reg_width = cpu->sve_max_vq * 128;
> + int pred_width = cpu->sve_max_vq * 16;
> int base_reg = orig_base_reg;
> int i;
>
> @@ -319,14 +320,14 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
> g_string_append_printf(s,
> "<reg name=\"p%d\" bitsize=\"%d\""
> " regnum=\"%d\" type=\"svep\"/>",
> - i, cpu->sve_max_vq * 16, base_reg++);
> + i, pred_width, base_reg++);
> info->num++;
> }
> g_string_append_printf(s,
> "<reg name=\"ffr\" bitsize=\"%d\""
> " regnum=\"%d\" group=\"vector\""
> " type=\"svep\"/>",
> - cpu->sve_max_vq * 16, base_reg++);
> + pred_width, base_reg++);
>
> /* Define the vector length pseudo-register. */
> g_string_append_printf(s,
Reviewed-by: Fabiano Rosas <farosas@suse.de>
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 07/14] target/arm: Fix svep width in arm_gen_dynamic_svereg_xml
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (5 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 06/14] target/arm: Hoist pred_width " Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-20 16:18 ` Peter Maydell
2023-02-14 16:30 ` [PATCH 08/14] target/arm: Add name argument to output_vector_union_type Richard Henderson
` (6 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Define svep based on the size of the predicates,
not the primary vector registers.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/gdbstub64.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 02a0256c5c..ec61211949 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -297,7 +297,7 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
/* Create the predicate vector type. */
g_string_append_printf(s,
"<vector id=\"svep\" type=\"uint8\" count=\"%d\"/>",
- reg_width / 8);
+ pred_width / 8);
/* Define the vector registers. */
for (i = 0; i < 32; i++) {
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 08/14] target/arm: Add name argument to output_vector_union_type
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (6 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 07/14] target/arm: Fix svep width " Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-20 16:20 ` Peter Maydell
2023-02-14 16:30 ` [PATCH 09/14] target/arm: Simplify iteration over bit widths Richard Henderson
` (5 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
This will make the function usable between SVE and SME.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/gdbstub64.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index ec61211949..166cb288cd 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -210,7 +210,8 @@ int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg)
return 0;
}
-static void output_vector_union_type(GString *s, int reg_width)
+static void output_vector_union_type(GString *s, int reg_width,
+ const char *name)
{
struct TypeSize {
const char *gdb_type;
@@ -240,39 +241,38 @@ static void output_vector_union_type(GString *s, int reg_width)
};
static const char suf[] = { 'q', 'd', 's', 'h', 'b' };
-
- g_autoptr(GString) ts = g_string_new("");
int i, j, bits;
/* 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, "svev%c%c", vec_lanes[i].sz, vec_lanes[i].suffix);
g_string_append_printf(s,
- "<vector id=\"%s\" type=\"%s\" count=\"%d\"/>",
- ts->str, vec_lanes[i].gdb_type, count);
+ "<vector id=\"%s%c%c\" type=\"%s\" count=\"%d\"/>",
+ name, vec_lanes[i].sz, vec_lanes[i].suffix,
+ vec_lanes[i].gdb_type, reg_width / vec_lanes[i].size);
}
+
/*
* Now define a union for each size group containing unsigned and
* signed and potentially float versions of each size from 128 to
* 8 bits.
*/
for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
- g_string_append_printf(s, "<union id=\"svevn%c\">", suf[i]);
+ g_string_append_printf(s, "<union id=\"%sn%c\">", name, suf[i]);
for (j = 0; j < ARRAY_SIZE(vec_lanes); j++) {
if (vec_lanes[j].size == bits) {
- g_string_append_printf(s, "<field name=\"%c\" type=\"svev%c%c\"/>",
- vec_lanes[j].suffix,
+ g_string_append_printf(s, "<field name=\"%c\" type=\"%s%c%c\"/>",
+ vec_lanes[j].suffix, name,
vec_lanes[j].sz, vec_lanes[j].suffix);
}
}
g_string_append(s, "</union>");
}
+
/* And now the final union of unions */
- g_string_append(s, "<union id=\"svev\">");
+ g_string_append_printf(s, "<union id=\"%s\">", name);
for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
- g_string_append_printf(s, "<field name=\"%c\" type=\"svevn%c\"/>",
- suf[i], suf[i]);
+ g_string_append_printf(s, "<field name=\"%c\" type=\"%sn%c\"/>",
+ suf[i], name, suf[i]);
}
g_string_append(s, "</union>");
}
@@ -292,7 +292,7 @@ int arm_gen_dynamic_svereg_xml(CPUState *cs, int orig_base_reg)
g_string_append_printf(s, "<feature name=\"org.gnu.gdb.aarch64.sve\">");
/* Create the vector union type. */
- output_vector_union_type(s, reg_width);
+ output_vector_union_type(s, reg_width, "svev");
/* Create the predicate vector type. */
g_string_append_printf(s,
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 09/14] target/arm: Simplify iteration over bit widths
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (7 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 08/14] target/arm: Add name argument to output_vector_union_type Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-20 16:24 ` Peter Maydell
2023-02-14 16:30 ` [PATCH 10/14] target/arm: Create pauth_ptr_mask Richard Henderson
` (4 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Order suf[] by the log8 of the width.
Use ARRAY_SIZE instead of hard-coding 128.
This changes the order of the union definitions,
but retains the order of the union-of-union members.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/gdbstub64.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index 166cb288cd..a6a8e7eb40 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -240,8 +240,8 @@ static void output_vector_union_type(GString *s, int reg_width,
{ "int8", 8, 'b', 's' },
};
- static const char suf[] = { 'q', 'd', 's', 'h', 'b' };
- int i, j, bits;
+ static const char suf[] = { 'b', 'h', 's', 'd', 'q' };
+ int i, j;
/* First define types and totals in a whole VL */
for (i = 0; i < ARRAY_SIZE(vec_lanes); i++) {
@@ -256,7 +256,9 @@ static void output_vector_union_type(GString *s, int reg_width,
* signed and potentially float versions of each size from 128 to
* 8 bits.
*/
- for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
+ for (i = 0; i < ARRAY_SIZE(suf); i++) {
+ int bits = 8 << i;
+
g_string_append_printf(s, "<union id=\"%sn%c\">", name, suf[i]);
for (j = 0; j < ARRAY_SIZE(vec_lanes); j++) {
if (vec_lanes[j].size == bits) {
@@ -270,7 +272,7 @@ static void output_vector_union_type(GString *s, int reg_width,
/* And now the final union of unions */
g_string_append_printf(s, "<union id=\"%s\">", name);
- for (bits = 128, i = 0; bits >= 8; bits /= 2, i++) {
+ for (i = ARRAY_SIZE(suf) - 1; i >= 0; i--) {
g_string_append_printf(s, "<field name=\"%c\" type=\"%sn%c\"/>",
suf[i], name, suf[i]);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 10/14] target/arm: Create pauth_ptr_mask
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (8 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 09/14] target/arm: Simplify iteration over bit widths Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-20 16:39 ` Peter Maydell
2023-02-14 16:30 ` [PATCH 11/14] target/arm: Implement gdbstub pauth extension Richard Henderson
` (3 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm
Keep the logic for pauth within pauth_helper.c, and expose
a helper function for use with the gdbstub pac extension.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/internals.h | 10 ++++++++++
target/arm/pauth_helper.c | 26 ++++++++++++++++++++++----
2 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index c98482561e..bb3983645d 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1364,6 +1364,16 @@ int exception_target_el(CPUARMState *env);
bool arm_singlestep_active(CPUARMState *env);
bool arm_generate_debug_exceptions(CPUARMState *env);
+/**
+ * pauth_ptr_mask:
+ * @env: cpu context
+ * @ptr: selects between TTBR0 and TTBR1
+ * @data: selects between TBI and TBID
+ *
+ * Return a mask of the bits of @ptr that contain the authentication code.
+ */
+uint64_t pauth_ptr_mask(CPUARMState *env, uint64_t ptr, bool data);
+
/* Add the cpreg definitions for debug related system registers */
void define_debug_regs(ARMCPU *cpu);
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index d0483bf051..20f347332d 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -339,14 +339,32 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier,
return pac | ext | ptr;
}
-static uint64_t pauth_original_ptr(uint64_t ptr, ARMVAParameters param)
+static uint64_t pauth_ptr_mask_internal(ARMVAParameters param)
{
- /* Note that bit 55 is used whether or not the regime has 2 ranges. */
- uint64_t extfield = sextract64(ptr, 55, 1);
int bot_pac_bit = 64 - param.tsz;
int top_pac_bit = 64 - 8 * param.tbi;
- return deposit64(ptr, bot_pac_bit, top_pac_bit - bot_pac_bit, extfield);
+ return MAKE_64BIT_MASK(bot_pac_bit, top_pac_bit - bot_pac_bit);
+}
+
+static uint64_t pauth_original_ptr(uint64_t ptr, ARMVAParameters param)
+{
+ uint64_t mask = pauth_ptr_mask_internal(param);
+
+ /* Note that bit 55 is used whether or not the regime has 2 ranges. */
+ if (extract64(ptr, 55, 1)) {
+ return ptr | mask;
+ } else {
+ return ptr & ~mask;
+ }
+}
+
+uint64_t pauth_ptr_mask(CPUARMState *env, uint64_t ptr, bool data)
+{
+ ARMMMUIdx mmu_idx = arm_stage1_mmu_idx(env);
+ ARMVAParameters param = aa64_va_parameters(env, ptr, mmu_idx, data);
+
+ return pauth_ptr_mask_internal(param);
}
static uint64_t pauth_auth(CPUARMState *env, uint64_t ptr, uint64_t modifier,
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 11/14] target/arm: Implement gdbstub pauth extension
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (9 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 10/14] target/arm: Create pauth_ptr_mask Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-20 16:58 ` Peter Maydell
2023-02-14 16:30 ` [PATCH 12/14] target/arm: Export arm_v7m_mrs_control Richard Henderson
` (2 subsequent siblings)
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, Luis Machado, Thiago Jung Bauermann
The extension is primarily defined by the Linux kernel NT_ARM_PAC_MASK
ptrace register set.
The original gdb feature consists of two masks, data and code, which are
used to mask out the authentication code within a pointer. Following
discussion with Luis Machado, add two more masks in order to support
pointers within the high half of the address space (i.e. TTBR1 vs TTBR0).
Cc: Luis Machado <luis.machado@arm.com>
Cc: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1105
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
configs/targets/aarch64-linux-user.mak | 2 +-
configs/targets/aarch64-softmmu.mak | 2 +-
configs/targets/aarch64_be-linux-user.mak | 2 +-
target/arm/internals.h | 2 ++
target/arm/gdbstub.c | 5 ++++
target/arm/gdbstub64.c | 29 +++++++++++++++++++++++
gdb-xml/aarch64-pauth.xml | 15 ++++++++++++
7 files changed, 54 insertions(+), 3 deletions(-)
create mode 100644 gdb-xml/aarch64-pauth.xml
diff --git a/configs/targets/aarch64-linux-user.mak b/configs/targets/aarch64-linux-user.mak
index db552f1839..ba8bc5fe3f 100644
--- a/configs/targets/aarch64-linux-user.mak
+++ b/configs/targets/aarch64-linux-user.mak
@@ -1,6 +1,6 @@
TARGET_ARCH=aarch64
TARGET_BASE_ARCH=arm
-TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml
+TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/aarch64-pauth.xml
TARGET_HAS_BFLT=y
CONFIG_SEMIHOSTING=y
CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
diff --git a/configs/targets/aarch64-softmmu.mak b/configs/targets/aarch64-softmmu.mak
index d489e6da83..b4338e9568 100644
--- a/configs/targets/aarch64-softmmu.mak
+++ b/configs/targets/aarch64-softmmu.mak
@@ -1,5 +1,5 @@
TARGET_ARCH=aarch64
TARGET_BASE_ARCH=arm
TARGET_SUPPORTS_MTTCG=y
-TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-vfp-sysregs.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml gdb-xml/arm-m-profile-mve.xml
+TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/arm-core.xml gdb-xml/arm-vfp.xml gdb-xml/arm-vfp3.xml gdb-xml/arm-vfp-sysregs.xml gdb-xml/arm-neon.xml gdb-xml/arm-m-profile.xml gdb-xml/arm-m-profile-mve.xml gdb-xml/aarch64-pauth.xml
TARGET_NEED_FDT=y
diff --git a/configs/targets/aarch64_be-linux-user.mak b/configs/targets/aarch64_be-linux-user.mak
index dc78044fb1..acb5620cdb 100644
--- a/configs/targets/aarch64_be-linux-user.mak
+++ b/configs/targets/aarch64_be-linux-user.mak
@@ -1,7 +1,7 @@
TARGET_ARCH=aarch64
TARGET_BASE_ARCH=arm
TARGET_BIG_ENDIAN=y
-TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml
+TARGET_XML_FILES= gdb-xml/aarch64-core.xml gdb-xml/aarch64-fpu.xml gdb-xml/aarch64-pauth.xml
TARGET_HAS_BFLT=y
CONFIG_SEMIHOSTING=y
CONFIG_ARM_COMPATIBLE_SEMIHOSTING=y
diff --git a/target/arm/internals.h b/target/arm/internals.h
index bb3983645d..4b60355a7e 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1345,6 +1345,8 @@ int aarch64_gdb_get_sve_reg(CPUARMState *env, GByteArray *buf, int reg);
int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg);
int aarch64_gdb_get_fpu_reg(CPUARMState *env, GByteArray *buf, int reg);
int aarch64_gdb_set_fpu_reg(CPUARMState *env, uint8_t *buf, int reg);
+int aarch64_gdb_get_pauth_reg(CPUARMState *env, GByteArray *buf, int reg);
+int aarch64_gdb_set_pauth_reg(CPUARMState *env, uint8_t *buf, int reg);
void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp);
void arm_cpu_sme_finalize(ARMCPU *cpu, Error **errp);
void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index bf8aff7824..062c8d447a 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -355,6 +355,11 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
aarch64_gdb_set_fpu_reg,
34, "aarch64-fpu.xml", 0);
}
+ if (isar_feature_aa64_pauth(&cpu->isar)) {
+ gdb_register_coprocessor(cs, aarch64_gdb_get_pauth_reg,
+ aarch64_gdb_set_pauth_reg,
+ 4, "aarch64-pauth.xml", 0);
+ }
#endif
} else {
if (arm_feature(env, ARM_FEATURE_NEON)) {
diff --git a/target/arm/gdbstub64.c b/target/arm/gdbstub64.c
index a6a8e7eb40..465d7fb196 100644
--- a/target/arm/gdbstub64.c
+++ b/target/arm/gdbstub64.c
@@ -210,6 +210,35 @@ int aarch64_gdb_set_sve_reg(CPUARMState *env, uint8_t *buf, int reg)
return 0;
}
+int aarch64_gdb_get_pauth_reg(CPUARMState *env, GByteArray *buf, int reg)
+{
+ switch (reg) {
+ case 0: /* pauth_dmask */
+ case 1: /* pauth_cmask */
+ case 2: /* pauth_dmask_high */
+ case 3: /* pauth_cmask_high */
+ /*
+ * Note that older versions of this feature only contained
+ * pauth_{d,c}mask, for use with Linux user processes, and
+ * thus exclusively in the low half of the address space.
+ *
+ * To support system mode, and to debug kernels, two new regs
+ * were added to cover the high half of the address space.
+ * For the purpose of pauth_ptr_mask, we can use any well-formed
+ * address within the address space half -- here, 0 and -2.
+ */
+ return gdb_get_reg64(buf, pauth_ptr_mask(env, -(reg & 2), ~reg & 1));
+ default:
+ return 0;
+ }
+}
+
+int aarch64_gdb_set_pauth_reg(CPUARMState *env, uint8_t *buf, int reg)
+{
+ /* All pseudo registers are read-only. */
+ return 0;
+}
+
static void output_vector_union_type(GString *s, int reg_width,
const char *name)
{
diff --git a/gdb-xml/aarch64-pauth.xml b/gdb-xml/aarch64-pauth.xml
new file mode 100644
index 0000000000..24af5f903c
--- /dev/null
+++ b/gdb-xml/aarch64-pauth.xml
@@ -0,0 +1,15 @@
+<?xml version="1.0"?>
+<!-- Copyright (C) 2018-2022 Free Software Foundation, Inc.
+
+ Copying and distribution of this file, with or without modification,
+ are permitted in any medium without royalty provided the copyright
+ notice and this notice are preserved. -->
+
+<!DOCTYPE feature SYSTEM "gdb-target.dtd">
+<feature name="org.gnu.gdb.aarch64.pauth">
+ <reg name="pauth_dmask" bitsize="64"/>
+ <reg name="pauth_cmask" bitsize="64"/>
+ <reg name="pauth_dmask_high" bitsize="64"/>
+ <reg name="pauth_cmask_high" bitsize="64"/>
+</feature>
+
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 11/14] target/arm: Implement gdbstub pauth extension
2023-02-14 16:30 ` [PATCH 11/14] target/arm: Implement gdbstub pauth extension Richard Henderson
@ 2023-02-20 16:58 ` Peter Maydell
0 siblings, 0 replies; 35+ messages in thread
From: Peter Maydell @ 2023-02-20 16:58 UTC (permalink / raw)
To: Richard Henderson
Cc: qemu-devel, qemu-arm, Luis Machado, Thiago Jung Bauermann
On Tue, 14 Feb 2023 at 16:32, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The extension is primarily defined by the Linux kernel NT_ARM_PAC_MASK
> ptrace register set.
>
> The original gdb feature consists of two masks, data and code, which are
> used to mask out the authentication code within a pointer. Following
> discussion with Luis Machado, add two more masks in order to support
> pointers within the high half of the address space (i.e. TTBR1 vs TTBR0).
>
> +int aarch64_gdb_get_pauth_reg(CPUARMState *env, GByteArray *buf, int reg)
> +{
> + switch (reg) {
> + case 0: /* pauth_dmask */
> + case 1: /* pauth_cmask */
> + case 2: /* pauth_dmask_high */
> + case 3: /* pauth_cmask_high */
> + /*
> + * Note that older versions of this feature only contained
> + * pauth_{d,c}mask, for use with Linux user processes, and
> + * thus exclusively in the low half of the address space.
> + *
> + * To support system mode, and to debug kernels, two new regs
> + * were added to cover the high half of the address space.
> + * For the purpose of pauth_ptr_mask, we can use any well-formed
> + * address within the address space half -- here, 0 and -2.
> + */
> + return gdb_get_reg64(buf, pauth_ptr_mask(env, -(reg & 2), ~reg & 1));
This seems pretty confusing to me. Is there a clearer way
we could write this? Pulling out a
bool is_data = !(reg & 1);
would help, for instance.
> + default:
> + return 0;
> + }
thanks
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 12/14] target/arm: Export arm_v7m_mrs_control
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (10 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 11/14] target/arm: Implement gdbstub pauth extension Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-20 16:50 ` Peter Maydell
2023-02-14 16:30 ` [PATCH 13/14] target/arm: Export arm_v7m_get_sp_ptr Richard Henderson
2023-02-14 16:30 ` [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb Richard Henderson
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, David Reiss
From: David Reiss <dreiss@meta.com>
Allow the function to be used outside of m_helper.c.
Rename with an "arm_" prefix.
Signed-off-by: David Reiss <dreiss@meta.com>
[rth: Split out of a larger patch]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/internals.h | 3 +++
target/arm/m_helper.c | 6 +++---
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 4b60355a7e..127a425961 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1353,6 +1353,9 @@ void arm_cpu_pauth_finalize(ARMCPU *cpu, Error **errp);
void arm_cpu_lpa2_finalize(ARMCPU *cpu, Error **errp);
#endif
+/* Read the CONTROL register as the MRS instruction would. */
+uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure);
+
#ifdef CONFIG_USER_ONLY
static inline void define_cortex_a72_a57_a53_cp_reginfo(ARMCPU *cpu) { }
#else
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index e7e746ea18..c20bcac977 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -53,7 +53,7 @@ static uint32_t v7m_mrs_xpsr(CPUARMState *env, uint32_t reg, unsigned el)
return xpsr_read(env) & mask;
}
-static uint32_t v7m_mrs_control(CPUARMState *env, uint32_t secure)
+uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure)
{
uint32_t value = env->v7m.control[secure];
@@ -90,7 +90,7 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
case 0 ... 7: /* xPSR sub-fields */
return v7m_mrs_xpsr(env, reg, 0);
case 20: /* CONTROL */
- return v7m_mrs_control(env, 0);
+ return arm_v7m_mrs_control(env, 0);
default:
/* Unprivileged reads others as zero. */
return 0;
@@ -2420,7 +2420,7 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
case 0 ... 7: /* xPSR sub-fields */
return v7m_mrs_xpsr(env, reg, el);
case 20: /* CONTROL */
- return v7m_mrs_control(env, env->v7m.secure);
+ return arm_v7m_mrs_control(env, env->v7m.secure);
case 0x94: /* CONTROL_NS */
/*
* We have to handle this here because unprivileged Secure code
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH 13/14] target/arm: Export arm_v7m_get_sp_ptr
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (11 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 12/14] target/arm: Export arm_v7m_mrs_control Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-20 16:51 ` Peter Maydell
2023-02-14 16:30 ` [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb Richard Henderson
13 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, David Reiss
From: David Reiss <dreiss@meta.com>
Allow the function to be used outside of m_helper.c.
Move to be outside of ifndef CONFIG_USER_ONLY block.
Rename from get_v7m_sp_ptr.
Signed-off-by: David Reiss <dreiss@meta.com>
[rth: Split out of a larger patch]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/internals.h | 10 +++++
target/arm/m_helper.c | 84 +++++++++++++++++++++---------------------
2 files changed, 51 insertions(+), 43 deletions(-)
diff --git a/target/arm/internals.h b/target/arm/internals.h
index 127a425961..6ad14048db 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -1356,6 +1356,16 @@ void arm_cpu_lpa2_finalize(ARMCPU *cpu, Error **errp);
/* Read the CONTROL register as the MRS instruction would. */
uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure);
+/*
+ * Return a pointer to the location where we currently store the
+ * stack pointer for the requested security state and thread mode.
+ * This pointer will become invalid if the CPU state is updated
+ * such that the stack pointers are switched around (eg changing
+ * the SPSEL control bit).
+ */
+uint32_t *arm_v7m_get_sp_ptr(CPUARMState *env, bool secure,
+ bool threadmode, bool spsel);
+
#ifdef CONFIG_USER_ONLY
static inline void define_cortex_a72_a57_a53_cp_reginfo(ARMCPU *cpu) { }
#else
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index c20bcac977..87e30c59a8 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -605,42 +605,6 @@ void HELPER(v7m_blxns)(CPUARMState *env, uint32_t dest)
arm_rebuild_hflags(env);
}
-static uint32_t *get_v7m_sp_ptr(CPUARMState *env, bool secure, bool threadmode,
- bool spsel)
-{
- /*
- * Return a pointer to the location where we currently store the
- * stack pointer for the requested security state and thread mode.
- * This pointer will become invalid if the CPU state is updated
- * such that the stack pointers are switched around (eg changing
- * the SPSEL control bit).
- * Compare the v8M ARM ARM pseudocode LookUpSP_with_security_mode().
- * Unlike that pseudocode, we require the caller to pass us in the
- * SPSEL control bit value; this is because we also use this
- * function in handling of pushing of the callee-saves registers
- * part of the v8M stack frame (pseudocode PushCalleeStack()),
- * and in the tailchain codepath the SPSEL bit comes from the exception
- * return magic LR value from the previous exception. The pseudocode
- * opencodes the stack-selection in PushCalleeStack(), but we prefer
- * to make this utility function generic enough to do the job.
- */
- bool want_psp = threadmode && spsel;
-
- if (secure == env->v7m.secure) {
- if (want_psp == v7m_using_psp(env)) {
- return &env->regs[13];
- } else {
- return &env->v7m.other_sp;
- }
- } else {
- if (want_psp) {
- return &env->v7m.other_ss_psp;
- } else {
- return &env->v7m.other_ss_msp;
- }
- }
-}
-
static bool arm_v7m_load_vector(ARMCPU *cpu, int exc, bool targets_secure,
uint32_t *pvec)
{
@@ -765,8 +729,8 @@ static bool v7m_push_callee_stack(ARMCPU *cpu, uint32_t lr, bool dotailchain,
!mode;
mmu_idx = arm_v7m_mmu_idx_for_secstate_and_priv(env, M_REG_S, priv);
- frame_sp_p = get_v7m_sp_ptr(env, M_REG_S, mode,
- lr & R_V7M_EXCRET_SPSEL_MASK);
+ frame_sp_p = arm_v7m_get_sp_ptr(env, M_REG_S, mode,
+ lr & R_V7M_EXCRET_SPSEL_MASK);
want_psp = mode && (lr & R_V7M_EXCRET_SPSEL_MASK);
if (want_psp) {
limit = env->v7m.psplim[M_REG_S];
@@ -1611,10 +1575,8 @@ static void do_v7m_exception_exit(ARMCPU *cpu)
* use 'frame_sp_p' after we do something that makes it invalid.
*/
bool spsel = env->v7m.control[return_to_secure] & R_V7M_CONTROL_SPSEL_MASK;
- uint32_t *frame_sp_p = get_v7m_sp_ptr(env,
- return_to_secure,
- !return_to_handler,
- spsel);
+ uint32_t *frame_sp_p = arm_v7m_get_sp_ptr(env, return_to_secure,
+ !return_to_handler, spsel);
uint32_t frameptr = *frame_sp_p;
bool pop_ok = true;
ARMMMUIdx mmu_idx;
@@ -1920,7 +1882,7 @@ static bool do_v7m_function_return(ARMCPU *cpu)
threadmode = !arm_v7m_is_handler_mode(env);
spsel = env->v7m.control[M_REG_S] & R_V7M_CONTROL_SPSEL_MASK;
- frame_sp_p = get_v7m_sp_ptr(env, true, threadmode, spsel);
+ frame_sp_p = arm_v7m_get_sp_ptr(env, true, threadmode, spsel);
frameptr = *frame_sp_p;
/*
@@ -2856,6 +2818,42 @@ uint32_t HELPER(v7m_tt)(CPUARMState *env, uint32_t addr, uint32_t op)
#endif /* !CONFIG_USER_ONLY */
+uint32_t *arm_v7m_get_sp_ptr(CPUARMState *env, bool secure, bool threadmode,
+ bool spsel)
+{
+ /*
+ * Return a pointer to the location where we currently store the
+ * stack pointer for the requested security state and thread mode.
+ * This pointer will become invalid if the CPU state is updated
+ * such that the stack pointers are switched around (eg changing
+ * the SPSEL control bit).
+ * Compare the v8M ARM ARM pseudocode LookUpSP_with_security_mode().
+ * Unlike that pseudocode, we require the caller to pass us in the
+ * SPSEL control bit value; this is because we also use this
+ * function in handling of pushing of the callee-saves registers
+ * part of the v8M stack frame (pseudocode PushCalleeStack()),
+ * and in the tailchain codepath the SPSEL bit comes from the exception
+ * return magic LR value from the previous exception. The pseudocode
+ * opencodes the stack-selection in PushCalleeStack(), but we prefer
+ * to make this utility function generic enough to do the job.
+ */
+ bool want_psp = threadmode && spsel;
+
+ if (secure == env->v7m.secure) {
+ if (want_psp == v7m_using_psp(env)) {
+ return &env->regs[13];
+ } else {
+ return &env->v7m.other_sp;
+ }
+ } else {
+ if (want_psp) {
+ return &env->v7m.other_ss_psp;
+ } else {
+ return &env->v7m.other_ss_msp;
+ }
+ }
+}
+
ARMMMUIdx arm_v7m_mmu_idx_all(CPUARMState *env,
bool secstate, bool priv, bool negpri)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 13/14] target/arm: Export arm_v7m_get_sp_ptr
2023-02-14 16:30 ` [PATCH 13/14] target/arm: Export arm_v7m_get_sp_ptr Richard Henderson
@ 2023-02-20 16:51 ` Peter Maydell
0 siblings, 0 replies; 35+ messages in thread
From: Peter Maydell @ 2023-02-20 16:51 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-arm, David Reiss
On Tue, 14 Feb 2023 at 16:32, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> From: David Reiss <dreiss@meta.com>
>
> Allow the function to be used outside of m_helper.c.
> Move to be outside of ifndef CONFIG_USER_ONLY block.
> Rename from get_v7m_sp_ptr.
>
> Signed-off-by: David Reiss <dreiss@meta.com>
> [rth: Split out of a larger patch]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/internals.h | 10 +++++
> target/arm/m_helper.c | 84 +++++++++++++++++++++---------------------
> 2 files changed, 51 insertions(+), 43 deletions(-)
Depending on the ifdeffery in patch 14, if all the callsites
are inside ifndef CONFIG_USER_ONLY then we won't need to
move the function definition. Otherwise:
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
thanks
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb
2023-02-14 16:30 [PATCH 00/14] target/arm: gdbstub cleanups and additions Richard Henderson
` (12 preceding siblings ...)
2023-02-14 16:30 ` [PATCH 13/14] target/arm: Export arm_v7m_get_sp_ptr Richard Henderson
@ 2023-02-14 16:30 ` Richard Henderson
2023-02-14 16:33 ` Richard Henderson
2023-02-20 16:02 ` Peter Maydell
13 siblings, 2 replies; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:30 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-arm, David Reiss
From: David Reiss <dreiss@meta.com>
Follows a fairly similar pattern to the existing special register
debug support. Only reading is implemented, but it should be
possible to implement writes.
Signed-off-by: David Reiss <dreiss@meta.com>
[rth: Split out from two other patches;
Use an enumeration to locally number the registers.
Use a structure to list and control runtime visibility.
Handle security extension with the same code.]
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/cpu.h | 1 +
target/arm/gdbstub.c | 169 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 170 insertions(+)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index c9f768f945..536e60d48c 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -867,6 +867,7 @@ struct ArchCPU {
DynamicGDBXMLInfo dyn_sysreg_xml;
DynamicGDBXMLInfo dyn_svereg_xml;
+ DynamicGDBXMLInfo dyn_m_systemreg_xml;
/* Timers used by the generic (architected) timer */
QEMUTimer *gt_timer[NUM_GTIMERS];
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 062c8d447a..a8848c7fee 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -322,6 +322,167 @@ static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg)
return cpu->dyn_sysreg_xml.num;
}
+enum {
+ M_SYSREG_MSP = 0,
+ M_SYSREG_PSP = 1,
+ M_SYSREG_PRIMASK = 2,
+ M_SYSREG_CONTROL = 3,
+ M_SYSREG_BASEPRI = 4,
+ M_SYSREG_FAULTMASK = 5,
+ M_SYSREG_MSPLIM = 6,
+ M_SYSREG_PSPLIM = 7,
+ M_SYSREG_REG_MASK = 7,
+
+ /*
+ * NOTE: MSP, PSP, MSPLIM, PSPLIM technically don't exist if the
+ * secure extension is present (replaced by MSP_S, MSP_NS, et al).
+ * However, the MRS instruction is still allowed to read from MSP and PSP,
+ * and will return the value associated with the current security state.
+ * We replicate this behavior for the convenience of users, who will see
+ * GDB behave similarly to their assembly code, even if they are oblivious
+ * to the security extension.
+ */
+ M_SYSREG_CURRENT = 0 << 3,
+ M_SYSREG_NONSECURE = 1 << 3,
+ M_SYSREG_SECURE = 2 << 3,
+ M_SYSREG_MODE_MASK = 3 << 3,
+};
+
+static const struct {
+ const char *name;
+ int feature;
+} m_systemreg_def[] = {
+ [M_SYSREG_MSP] = { "msp", ARM_FEATURE_M },
+ [M_SYSREG_PSP] = { "psp", ARM_FEATURE_M },
+ [M_SYSREG_PRIMASK] = { "primask", ARM_FEATURE_M },
+ [M_SYSREG_CONTROL] = { "control", ARM_FEATURE_M },
+ [M_SYSREG_BASEPRI] = { "basepri", ARM_FEATURE_M_MAIN },
+ [M_SYSREG_FAULTMASK] = { "faultmask", ARM_FEATURE_M_MAIN },
+ [M_SYSREG_MSPLIM] = { "msplim", ARM_FEATURE_V8 },
+ [M_SYSREG_PSPLIM] = { "psplim", ARM_FEATURE_V8 },
+};
+
+static int arm_gdb_get_m_systemreg(CPUARMState *env, GByteArray *buf, int reg)
+{
+ int mode = reg & M_SYSREG_MODE_MASK;
+ bool secure;
+ uint32_t val;
+
+ switch (mode) {
+ case M_SYSREG_CURRENT:
+ secure = env->v7m.secure;
+ break;
+ case M_SYSREG_NONSECURE:
+ secure = false;
+ break;
+ case M_SYSREG_SECURE:
+ secure = true;
+ break;
+ default:
+ return 0;
+ }
+
+ reg &= M_SYSREG_REG_MASK;
+ if (reg >= ARRAY_SIZE(m_systemreg_def)) {
+ return 0;
+ }
+ if (!arm_feature(env, m_systemreg_def[reg].feature)) {
+ return 0;
+ }
+
+ /* NOTE: This implementation shares a lot of logic with v7m_mrs. */
+ switch (reg) {
+ case M_SYSREG_MSP:
+ val = *arm_v7m_get_sp_ptr(env, secure, false, true);
+ break;
+ case M_SYSREG_PSP:
+ val = *arm_v7m_get_sp_ptr(env, secure, true, true);
+ break;
+ case M_SYSREG_MSPLIM:
+ val = env->v7m.msplim[secure];
+ break;
+ case M_SYSREG_PSPLIM:
+ val = env->v7m.psplim[secure];
+ break;
+ case M_SYSREG_PRIMASK:
+ val = env->v7m.primask[secure];
+ break;
+ case M_SYSREG_BASEPRI:
+ val = env->v7m.basepri[secure];
+ break;
+ case M_SYSREG_FAULTMASK:
+ val = env->v7m.faultmask[secure];
+ break;
+ case M_SYSREG_CONTROL:
+ /*
+ * NOTE: CONTROL has a mix of banked and non-banked bits.
+ * For "current", we emulate the MRS instruction.
+ * Unfortunately, this gives GDB no way to read the SFPA bit
+ * when the CPU is in a non-secure state.
+ */
+ if (mode == M_SYSREG_CURRENT) {
+ val = arm_v7m_mrs_control(env, secure);
+ } else {
+ val = env->v7m.control[secure];
+ }
+ break;
+ default:
+ g_assert_not_reached();
+ }
+
+ return gdb_get_reg32(buf, val);
+}
+
+static int arm_gdb_set_m_systemreg(CPUARMState *env, uint8_t *buf, int reg)
+{
+ /* TODO: Implement. */
+ return 0;
+}
+
+static int arm_gen_dynamic_m_systemreg_xml(CPUState *cs, int base_reg)
+{
+ ARMCPU *cpu = ARM_CPU(cs);
+ CPUARMState *env = &cpu->env;
+ GString *s = g_string_new(NULL);
+ int i, ret;
+
+ 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");
+
+ QEMU_BUILD_BUG_ON(M_SYSREG_CURRENT != 0);
+ ret = ARRAY_SIZE(m_systemreg_def);
+
+ for (i = 0; i < ret; i++) {
+ if (arm_feature(env, m_systemreg_def[i].feature)) {
+ g_string_append_printf(s,
+ "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n",
+ m_systemreg_def[i].name, base_reg + i);
+ }
+ }
+
+ if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
+ for (i = 0; i < ret; i++) {
+ g_string_append_printf(s,
+ "<reg name=\"%s_ns\" bitsize=\"32\" regnum=\"%d\"/>\n",
+ m_systemreg_def[i].name, base_reg + (i | M_SYSREG_NONSECURE));
+ }
+ for (i = 0; i < ret; i++) {
+ g_string_append_printf(s,
+ "<reg name=\"%s_s\" bitsize=\"32\" regnum=\"%d\"/>\n",
+ m_systemreg_def[i].name, base_reg + (i | M_SYSREG_SECURE));
+ }
+ QEMU_BUILD_BUG_ON(M_SYSREG_SECURE < M_SYSREG_NONSECURE);
+ ret |= M_SYSREG_SECURE;
+ }
+
+ g_string_append_printf(s, "</feature>");
+
+ cpu->dyn_m_systemreg_xml.desc = g_string_free(s, false);
+ cpu->dyn_m_systemreg_xml.num = ret;
+ return ret;
+}
+
const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
{
ARMCPU *cpu = ARM_CPU(cs);
@@ -330,6 +491,8 @@ const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
return cpu->dyn_sysreg_xml.desc;
} else if (strcmp(xmlname, "sve-registers.xml") == 0) {
return cpu->dyn_svereg_xml.desc;
+ } else if (strcmp(xmlname, "arm-m-system.xml") == 0) {
+ return cpu->dyn_m_systemreg_xml.desc;
}
return NULL;
}
@@ -389,4 +552,10 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs),
"system-registers.xml", 0);
+ if (arm_feature(env, ARM_FEATURE_M)) {
+ gdb_register_coprocessor(cs,
+ arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg,
+ arm_gen_dynamic_m_systemreg_xml(cs, cs->gdb_num_regs),
+ "arm-m-system.xml", 0);
+ }
}
--
2.34.1
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb
2023-02-14 16:30 ` [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb Richard Henderson
@ 2023-02-14 16:33 ` Richard Henderson
2023-02-20 16:02 ` Peter Maydell
1 sibling, 0 replies; 35+ messages in thread
From: Richard Henderson @ 2023-02-14 16:33 UTC (permalink / raw)
To: qemu-devel, Peter Maydell; +Cc: qemu-arm, David Reiss
On 2/14/23 06:30, Richard Henderson wrote:
> + for (i = 0; i < ret; i++) {
> + if (arm_feature(env, m_systemreg_def[i].feature)) {
> + g_string_append_printf(s,
> + "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n",
> + m_systemreg_def[i].name, base_reg + i);
> + }
> + }
> +
> + if (arm_feature(env, ARM_FEATURE_M_SECURITY)) {
> + for (i = 0; i < ret; i++) {
> + g_string_append_printf(s,
> + "<reg name=\"%s_ns\" bitsize=\"32\" regnum=\"%d\"/>\n",
> + m_systemreg_def[i].name, base_reg + (i | M_SYSREG_NONSECURE));
> + }
> + for (i = 0; i < ret; i++) {
> + g_string_append_printf(s,
> + "<reg name=\"%s_s\" bitsize=\"32\" regnum=\"%d\"/>\n",
> + m_systemreg_def[i].name, base_reg + (i | M_SYSREG_SECURE));
> + }
> + QEMU_BUILD_BUG_ON(M_SYSREG_SECURE < M_SYSREG_NONSECURE);
> + ret |= M_SYSREG_SECURE;
> + }
Peter, from our chat on IRC it sounds as if you would put this block #ifndef CONFIG_USER_ONLY.
r~
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb
2023-02-14 16:30 ` [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb Richard Henderson
2023-02-14 16:33 ` Richard Henderson
@ 2023-02-20 16:02 ` Peter Maydell
2023-02-20 17:00 ` Richard Henderson
1 sibling, 1 reply; 35+ messages in thread
From: Peter Maydell @ 2023-02-20 16:02 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-arm, David Reiss
On Tue, 14 Feb 2023 at 16:31, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> From: David Reiss <dreiss@meta.com>
>
> Follows a fairly similar pattern to the existing special register
> debug support. Only reading is implemented, but it should be
> possible to implement writes.
>
> Signed-off-by: David Reiss <dreiss@meta.com>
> [rth: Split out from two other patches;
> Use an enumeration to locally number the registers.
> Use a structure to list and control runtime visibility.
> Handle security extension with the same code.]
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/cpu.h | 1 +
> target/arm/gdbstub.c | 169 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 170 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index c9f768f945..536e60d48c 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -867,6 +867,7 @@ struct ArchCPU {
>
> DynamicGDBXMLInfo dyn_sysreg_xml;
> DynamicGDBXMLInfo dyn_svereg_xml;
> + DynamicGDBXMLInfo dyn_m_systemreg_xml;
>
> /* Timers used by the generic (architected) timer */
> QEMUTimer *gt_timer[NUM_GTIMERS];
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index 062c8d447a..a8848c7fee 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -322,6 +322,167 @@ static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg)
> return cpu->dyn_sysreg_xml.num;
> }
>
> +enum {
> + M_SYSREG_MSP = 0,
> + M_SYSREG_PSP = 1,
> + M_SYSREG_PRIMASK = 2,
> + M_SYSREG_CONTROL = 3,
> + M_SYSREG_BASEPRI = 4,
> + M_SYSREG_FAULTMASK = 5,
> + M_SYSREG_MSPLIM = 6,
> + M_SYSREG_PSPLIM = 7,
> + M_SYSREG_REG_MASK = 7,
> +
> + /*
> + * NOTE: MSP, PSP, MSPLIM, PSPLIM technically don't exist if the
> + * secure extension is present (replaced by MSP_S, MSP_NS, et al).
> + * However, the MRS instruction is still allowed to read from MSP and PSP,
> + * and will return the value associated with the current security state.
What's "don't exist" based on here? Architecturally MSPLIM, PSPLIM etc
are banked registers; MRS/MSR let you access the one for the current
security state, and (if you are Secure) the one for the NS state.
> + * We replicate this behavior for the convenience of users, who will see
> + * GDB behave similarly to their assembly code, even if they are oblivious
> + * to the security extension.
> + */
> + M_SYSREG_CURRENT = 0 << 3,
> + M_SYSREG_NONSECURE = 1 << 3,
> + M_SYSREG_SECURE = 2 << 3,
> + M_SYSREG_MODE_MASK = 3 << 3,
> +};
> +
> +static int arm_gen_dynamic_m_systemreg_xml(CPUState *cs, int base_reg)
> +{
> + ARMCPU *cpu = ARM_CPU(cs);
> + CPUARMState *env = &cpu->env;
> + GString *s = g_string_new(NULL);
> + int i, ret;
> +
> + 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");
Half of these need to be in org.gnu.gdb.arm.secext.
These aren't our own XML features we're making up (if they
were then they would be in org.qemu.something), so we should
follow the existing precedent about what registers go in them.
thanks
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb
2023-02-20 16:02 ` Peter Maydell
@ 2023-02-20 17:00 ` Richard Henderson
2023-02-20 17:37 ` Peter Maydell
0 siblings, 1 reply; 35+ messages in thread
From: Richard Henderson @ 2023-02-20 17:00 UTC (permalink / raw)
To: Peter Maydell; +Cc: qemu-devel, qemu-arm, David Reiss
On 2/20/23 06:02, Peter Maydell wrote:
>> + /*
>> + * NOTE: MSP, PSP, MSPLIM, PSPLIM technically don't exist if the
>> + * secure extension is present (replaced by MSP_S, MSP_NS, et al).
>> + * However, the MRS instruction is still allowed to read from MSP and PSP,
>> + * and will return the value associated with the current security state.
>
> What's "don't exist" based on here? Architecturally MSPLIM, PSPLIM etc
> are banked registers; MRS/MSR let you access the one for the current
> security state, and (if you are Secure) the one for the NS state.
Copied that wording from David.
>> + 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");
>
> Half of these need to be in org.gnu.gdb.arm.secext.
> These aren't our own XML features we're making up (if they
> were then they would be in org.qemu.something), so we should
> follow the existing precedent about what registers go in them.
Now that you point it out (and I should have checked myself), we are kinda making them up.
The only registers within upstream gdb m-system and secext are MSP, PSP, MSP_NS, MSP_S,
PSP_NS, PSP_S. All the others are our own addition.
Should all the rest be in a third bit of xml?
r~
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb
2023-02-20 17:00 ` Richard Henderson
@ 2023-02-20 17:37 ` Peter Maydell
2023-02-20 18:27 ` Luis Machado
0 siblings, 1 reply; 35+ messages in thread
From: Peter Maydell @ 2023-02-20 17:37 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, qemu-arm, David Reiss, Luis Machado
On Mon, 20 Feb 2023 at 17:00, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 2/20/23 06:02, Peter Maydell wrote:
> >> + 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");
> >
> > Half of these need to be in org.gnu.gdb.arm.secext.
> > These aren't our own XML features we're making up (if they
> > were then they would be in org.qemu.something), so we should
> > follow the existing precedent about what registers go in them.
>
> Now that you point it out (and I should have checked myself), we are kinda making them up.
> The only registers within upstream gdb m-system and secext are MSP, PSP, MSP_NS, MSP_S,
> PSP_NS, PSP_S. All the others are our own addition.
I think OpenOCD's implementation includes more than that:
https://openocd.org/doc-release/doxygen/armv7m_8c_source.html
> Should all the rest be in a third bit of xml?
Luis, do you have the specs for what the existing implementations
are doing here ?
Ideally gdb should document for every bit of XML it is the
official owner of (ie in the org.gnu.gdb namespace) what the
required and optional register values are, including details
like the register width (which I think the two existing
implementations that output m-system disagree on).
thanks
-- PMM
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 14/14] target/arm: Support reading m-profile system registers from gdb
2023-02-20 17:37 ` Peter Maydell
@ 2023-02-20 18:27 ` Luis Machado
0 siblings, 0 replies; 35+ messages in thread
From: Luis Machado @ 2023-02-20 18:27 UTC (permalink / raw)
To: Peter Maydell, Richard Henderson
Cc: qemu-devel, qemu-arm, David Reiss, Torbjorn SVENSSON,
Yvan ROUX - foss
Hi,
On 2/20/23 17:37, Peter Maydell wrote:
> On Mon, 20 Feb 2023 at 17:00, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> On 2/20/23 06:02, Peter Maydell wrote:
>>>> + 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");
>>>
>>> Half of these need to be in org.gnu.gdb.arm.secext.
>>> These aren't our own XML features we're making up (if they
>>> were then they would be in org.qemu.something), so we should
>>> follow the existing precedent about what registers go in them.
>>
>> Now that you point it out (and I should have checked myself), we are kinda making them up.
>> The only registers within upstream gdb m-system and secext are MSP, PSP, MSP_NS, MSP_S,
>> PSP_NS, PSP_S. All the others are our own addition.
>
> I think OpenOCD's implementation includes more than that:
> https://openocd.org/doc-release/doxygen/armv7m_8c_source.html
>
>> Should all the rest be in a third bit of xml?
>
> Luis, do you have the specs for what the existing implementations
> are doing here ?
Support for the extra stack pointers was contributed by ST (Torbjörn and Yvan cc-ed), so I'd say ST-Link was the debugging stub the GDB changes were based on (to my knowledge). This support includes the org.gnu.gdb.arm.m-system and org.gnu.gdb.arm.secext features.
The org.gnu.gdb.arm.m-system feature contains msp and psp (gdb/features/arm/arm-m-system.xml) and the org.gnu.gdb.arm.secext feature contains the msp_ns, msp_s, psp_ns and psp_s registers (gdb/features/arm/arm-secext.xml).
All of the registers should be 32-bit in size. There could be extra registers in those two features, but gdb only cares about the 6 registers listed above. It is meant for gdb to detect if the additional stack pointer registers are available and, if so, track their values.
Adding the stack pointers to other features should be OK, but gdb needs to know about that so it can go looking for them. The most convenient way to do it is to follow the original features we modeled things from. In this case, the XML layout from ST's contributions.
>
> Ideally gdb should document for every bit of XML it is the
> official owner of (ie in the org.gnu.gdb namespace) what the
> required and optional register values are, including details
> like the register width (which I think the two existing
> implementations that output m-system disagree on).
I went through the OpenOCD code and it seems it has additional registers in the m-system category. That should be fine, as long as gdb sees msp and psp.
Of course, all of this would be much better if properly documented in the gdb manual, which unfortunately it isn't (in much or any detail). Apologies for that.
I'll set some time aside to get the documentation updated / written for these features.
In the mean time, feel free to ping me if something needs to be clarified.
>
> thanks
> -- PMM
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
^ permalink raw reply [flat|nested] 35+ messages in thread