[PATCH qemu v2 1/3] target/arm: Unify checking for M Main Extension in MRS/MSR

[PATCH qemu v2 1/3] target/arm: Unify checking for M Main Extension in MRS/MSR

[~dreiss-meta]

From: David Reiss <dreiss@meta.com>

BASEPRI, FAULTMASK, and their _NS equivalents only exist on devices with
the Main Extension.  However, the MRS instruction did not check this,
and the MSR instruction handled it inconsistently (warning BASEPRI, but
silently ignoring writes to BASEPRI_NS).  Unify this behavior and always
warn when reading or writing any of these registers if the extension is
not present.

Signed-off-by: David Reiss <dreiss@meta.com>
---
 target/arm/m_helper.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index 033a4d9261..d87b9ecd12 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2465,11 +2465,17 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
             }
             return env->v7m.primask[M_REG_NS];
         case 0x91: /* BASEPRI_NS */
+            if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+                goto bad_reg;
+            }
             if (!env->v7m.secure) {
                 return 0;
             }
             return env->v7m.basepri[M_REG_NS];
         case 0x93: /* FAULTMASK_NS */
+            if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+                goto bad_reg;
+            }
             if (!env->v7m.secure) {
                 return 0;
             }
@@ -2515,8 +2521,14 @@ uint32_t HELPER(v7m_mrs)(CPUARMState *env, uint32_t reg)
         return env->v7m.primask[env->v7m.secure];
     case 17: /* BASEPRI */
     case 18: /* BASEPRI_MAX */
+        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+            goto bad_reg;
+        }
         return env->v7m.basepri[env->v7m.secure];
     case 19: /* FAULTMASK */
+        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+            goto bad_reg;
+        }
         return env->v7m.faultmask[env->v7m.secure];
     default:
     bad_reg:
@@ -2581,13 +2593,19 @@ void HELPER(v7m_msr)(CPUARMState *env, uint32_t maskreg, uint32_t val)
             env->v7m.primask[M_REG_NS] = val & 1;
             return;
         case 0x91: /* BASEPRI_NS */
-            if (!env->v7m.secure || !arm_feature(env, ARM_FEATURE_M_MAIN)) {
+            if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+                goto bad_reg;
+            }
+            if (!env->v7m.secure) {
                 return;
             }
             env->v7m.basepri[M_REG_NS] = val & 0xff;
             return;
         case 0x93: /* FAULTMASK_NS */
-            if (!env->v7m.secure || !arm_feature(env, ARM_FEATURE_M_MAIN)) {
+            if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+                goto bad_reg;
+            }
+            if (!env->v7m.secure) {
                 return;
             }
             env->v7m.faultmask[M_REG_NS] = val & 1;
-- 
2.34.5

[PATCH qemu v2 2/3] target/arm/gdbstub: Support reading M system registers from GDB

[~dreiss-meta]

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.

`v7m_mrs_control` was renamed `arm_v7m_mrs_control` and made
non-static so this logic could be shared between the MRS instruction and
the GDB stub.

Signed-off-by: David Reiss <dreiss@meta.com>
---
 target/arm/cpu.h      |  12 +++-
 target/arm/gdbstub.c  | 125 ++++++++++++++++++++++++++++++++++++++++++
 target/arm/m_helper.c |   6 +-
 3 files changed, 138 insertions(+), 5 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index bf2bce046d..fdbb0d9107 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -856,6 +856,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];
@@ -1100,11 +1101,13 @@ 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.
+ * Helpers to dynamically generate XML descriptions of the sysregs,
+ * SVE registers, and M-profile system 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);
+int arm_gen_dynamic_m_systemreg_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
@@ -1496,6 +1499,11 @@ FIELD(SVCR, ZA, 1, 1)
 FIELD(SMCR, LEN, 0, 4)
 FIELD(SMCR, FA64, 31, 1)
 
+/*
+ * Read the CONTROL register as the MRS instruction would.
+ */
+uint32_t arm_v7m_mrs_control(CPUARMState *env, uint32_t secure);
+
 /* Write a new value to v7m.exception, thus transitioning into or out
  * of Handler mode; this may result in a change of active stack pointer.
  */
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 2f806512d0..ae7fe2c800 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -322,6 +322,121 @@ int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg)
     return cpu->dyn_sysreg_xml.num;
 }
 
+/*
+ * Helper required because g_array_append_val is a macro
+ * that cannot handle string literals.
+ */
+static inline void g_array_append_str_literal(GArray *array, const char *str)
+{
+    g_array_append_val(array, str);
+}
+
+static int arm_gdb_get_m_systemreg(CPUARMState *env, GByteArray *buf, int reg)
+{
+    /* NOTE: This implementation shares a lot of logic with v7m_mrs. */
+    switch (reg) {
+
+    /*
+     * NOTE: MSP and PSP technically don't exist if the secure extension
+     * is present (replaced by MSP_S, MSP_NS, PSP_S, PSP_NS).  Similar for
+     * MSPLIM and PSPLIM.
+     * 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.
+     */
+    case 0: /* MSP */
+        return gdb_get_reg32(buf,
+            v7m_using_psp(env) ? env->v7m.other_sp : env->regs[13]);
+    case 1: /* PSP */
+        return gdb_get_reg32(buf,
+            v7m_using_psp(env) ? env->regs[13] : env->v7m.other_sp);
+    case 6: /* MSPLIM */
+        if (!arm_feature(env, ARM_FEATURE_V8)) {
+            return 0;
+        }
+        return gdb_get_reg32(buf, env->v7m.msplim[env->v7m.secure]);
+    case 7: /* PSPLIM */
+        if (!arm_feature(env, ARM_FEATURE_V8)) {
+            return 0;
+        }
+        return gdb_get_reg32(buf, env->v7m.psplim[env->v7m.secure]);
+
+    /*
+     * NOTE: PRIMASK, BASEPRI, and FAULTMASK are defined a bit differently
+     * from the SP family, but have similar banking behavior.
+     */
+    case 2: /* PRIMASK */
+        return gdb_get_reg32(buf, env->v7m.primask[env->v7m.secure]);
+    case 3: /* BASEPRI */
+        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+            return 0;
+        }
+        return gdb_get_reg32(buf, env->v7m.basepri[env->v7m.secure]);
+    case 4: /* FAULTMASK */
+        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+            return 0;
+        }
+        return gdb_get_reg32(buf, env->v7m.faultmask[env->v7m.secure]);
+
+    /*
+     * NOTE: CONTROL has a mix of banked and non-banked bits.  We continue
+     * to emulate the MRS instruction.  Unfortunately, this gives GDB no way
+     * to read the SFPA bit when the CPU is in a non-secure state.
+     */
+    case 5: /* CONTROL */
+        return gdb_get_reg32(buf, arm_v7m_mrs_control(env, env->v7m.secure));
+    }
+
+    return 0;
+}
+
+static int arm_gdb_set_m_systemreg(CPUARMState *env, uint8_t *buf, int reg)
+{
+    /* TODO: Implement. */
+    return 0;
+}
+
+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);
+    DynamicGDBXMLInfo *info = &cpu->dyn_m_systemreg_xml;
+    bool is_v8 = arm_feature(env, ARM_FEATURE_V8);
+    bool is_main = arm_feature(env, ARM_FEATURE_M_MAIN);
+
+    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");
+
+    g_autoptr(GArray) regs = g_array_new(false, true, sizeof(const char *));
+    /* 0 */ g_array_append_str_literal(regs, "msp");
+    /* 1 */ g_array_append_str_literal(regs, "psp");
+    /* 2 */ g_array_append_str_literal(regs, "primask");
+    /* 3 */ g_array_append_str_literal(regs, is_main ? "basepri" : "");
+    /* 4 */ g_array_append_str_literal(regs, is_main ? "faultmask" : "");
+    /* 5 */ g_array_append_str_literal(regs, "control");
+    /* 6 */ g_array_append_str_literal(regs, is_v8 ? "msplim" : "");
+    /* 7 */ g_array_append_str_literal(regs, is_v8 ? "psplim" : "");
+
+    for (int idx = 0; idx < regs->len; idx++) {
+        const char *name = g_array_index(regs, const char *, idx);
+        if (*name != '\0') {
+            g_string_append_printf(s,
+                        "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n",
+                        name, base_reg);
+        }
+        base_reg++;
+    }
+    info->num = regs->len;
+
+    g_string_append_printf(s, "</feature>");
+    info->desc = g_string_free(s, false);
+    return info->num;
+}
+
 struct TypeSize {
     const char *gdb_type;
     int  size;
@@ -450,6 +565,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;
 }
@@ -493,6 +610,14 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
              */
             gdb_register_coprocessor(cs, vfp_gdb_get_sysreg, vfp_gdb_set_sysreg,
                                      2, "arm-vfp-sysregs.xml", 0);
+        } else {
+            /* M-profile coprocessors. */
+            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);
         }
     }
     if (cpu_isar_feature(aa32_mve, cpu)) {
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index d87b9ecd12..e8fbe1599a 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.5

[PATCH qemu v2 3/3] target/arm/gdbstub: Support reading M security extension registers from GDB

[~dreiss-meta]

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>
---
 target/arm/cpu.h      |  15 +++++-
 target/arm/gdbstub.c  | 116 ++++++++++++++++++++++++++++++++++++++++++
 target/arm/m_helper.c |  23 ++++-----
 3 files changed, 139 insertions(+), 15 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index fdbb0d9107..661ca832bf 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -857,6 +857,7 @@ struct ArchCPU {
     DynamicGDBXMLInfo dyn_sysreg_xml;
     DynamicGDBXMLInfo dyn_svereg_xml;
     DynamicGDBXMLInfo dyn_m_systemreg_xml;
+    DynamicGDBXMLInfo dyn_m_securereg_xml;
 
     /* Timers used by the generic (architected) timer */
     QEMUTimer *gt_timer[NUM_GTIMERS];
@@ -1102,12 +1103,13 @@ int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 
 /*
  * Helpers to dynamically generate XML descriptions of the sysregs,
- * SVE registers, and M-profile system registers.
+ * SVE registers, M-profile system, and M-profile secure extension 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);
 int arm_gen_dynamic_m_systemreg_xml(CPUState *cpu, int base_reg);
+int arm_gen_dynamic_m_securereg_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
@@ -1607,6 +1609,17 @@ static inline void xpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
 #endif
 }
 
+/*
+ * 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);
+
+
 #define HCR_VM        (1ULL << 0)
 #define HCR_SWIO      (1ULL << 1)
 #define HCR_PTW       (1ULL << 2)
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index ae7fe2c800..cbccd4aa2c 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -437,6 +437,110 @@ int arm_gen_dynamic_m_systemreg_xml(CPUState *cs, int base_reg)
     return info->num;
 }
 
+static int arm_gdb_get_m_secextreg(CPUARMState *env, GByteArray *buf, int reg)
+{
+    switch (reg) {
+    case  0:  /* MSP_S */
+        return gdb_get_reg32(buf, *arm_v7m_get_sp_ptr(env, true, false, true));
+    case  1:  /* PSP_S */
+        return gdb_get_reg32(buf, *arm_v7m_get_sp_ptr(env, true, true, true));
+    case  2:  /* MSPLIM_S */
+        return gdb_get_reg32(buf, env->v7m.msplim[M_REG_S]);
+    case  3:  /* PSPLIM_S */
+        return gdb_get_reg32(buf, env->v7m.psplim[M_REG_S]);
+    case  4:  /* PRIMASK_S */
+        return gdb_get_reg32(buf, env->v7m.primask[M_REG_S]);
+    case  5:  /* BASEPRI_S */
+        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+            return 0;
+        }
+        return gdb_get_reg32(buf, env->v7m.basepri[M_REG_S]);
+    case  6:  /* FAULTMASK_S */
+        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+            return 0;
+        }
+        return gdb_get_reg32(buf, env->v7m.faultmask[M_REG_S]);
+    case  7:  /* CONTROL_S */
+        return gdb_get_reg32(buf, env->v7m.control[M_REG_S]);
+    case  8:  /* MSP_NS */
+        return gdb_get_reg32(buf, *arm_v7m_get_sp_ptr(env, false, false, true));
+    case  9:  /* PSP_NS */
+        return gdb_get_reg32(buf, *arm_v7m_get_sp_ptr(env, false, true, true));
+    case 10:  /* MSPLIM_NS */
+        return gdb_get_reg32(buf, env->v7m.msplim[M_REG_NS]);
+    case 11:  /* PSPLIM_NS */
+        return gdb_get_reg32(buf, env->v7m.psplim[M_REG_NS]);
+    case 12:  /* PRIMASK_NS */
+        return gdb_get_reg32(buf, env->v7m.primask[M_REG_NS]);
+    case 13:  /* BASEPRI_NS */
+        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+            return 0;
+        }
+        return gdb_get_reg32(buf, env->v7m.basepri[M_REG_NS]);
+    case 14:  /* FAULTMASK_NS */
+        if (!arm_feature(env, ARM_FEATURE_M_MAIN)) {
+            return 0;
+        }
+        return gdb_get_reg32(buf, env->v7m.faultmask[M_REG_NS]);
+    case 15:  /* CONTROL_NS */
+        return gdb_get_reg32(buf, env->v7m.control[M_REG_NS]);
+    }
+
+    return 0;
+}
+
+static int arm_gdb_set_m_secextreg(CPUARMState *env, uint8_t *buf, int reg)
+{
+    /* TODO: Implement. */
+    return 0;
+}
+
+int arm_gen_dynamic_m_securereg_xml(CPUState *cs, int base_reg)
+{
+    ARMCPU *cpu = ARM_CPU(cs);
+    CPUARMState *env = &cpu->env;
+    GString *s = g_string_new(NULL);
+    DynamicGDBXMLInfo *info = &cpu->dyn_m_securereg_xml;
+    bool is_main = arm_feature(env, ARM_FEATURE_M_MAIN);
+
+    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.secext\">\n");
+
+    g_autoptr(GArray) regs = g_array_new(false, true, sizeof(const char *));
+    /*  0 */ g_array_append_str_literal(regs, "msp_s");
+    /*  1 */ g_array_append_str_literal(regs, "psp_s");
+    /*  2 */ g_array_append_str_literal(regs, "msplim_s");
+    /*  3 */ g_array_append_str_literal(regs, "psplim_s");
+    /*  4 */ g_array_append_str_literal(regs, "primask_s");
+    /*  5 */ g_array_append_str_literal(regs, is_main ? "basepri_s" : "");
+    /*  6 */ g_array_append_str_literal(regs, is_main ? "faultmask_s" : "");
+    /*  7 */ g_array_append_str_literal(regs, "control_s");
+    /*  8 */ g_array_append_str_literal(regs, "msp_ns");
+    /*  9 */ g_array_append_str_literal(regs, "psp_ns");
+    /* 10 */ g_array_append_str_literal(regs, "msplim_ns");
+    /* 11 */ g_array_append_str_literal(regs, "psplim_ns");
+    /* 12 */ g_array_append_str_literal(regs, "primask_ns");
+    /* 13 */ g_array_append_str_literal(regs, is_main ? "basepri_ns" : "");
+    /* 14 */ g_array_append_str_literal(regs, is_main ? "faultmask_ns" : "");
+    /* 15 */ g_array_append_str_literal(regs, "control_ns");
+
+    for (int idx = 0; idx < regs->len; idx++) {
+        const char *name = g_array_index(regs, const char *, idx);
+        if (*name != '\0') {
+            g_string_append_printf(s,
+                        "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n",
+                        name, base_reg);
+        }
+        base_reg++;
+    }
+    info->num = regs->len;
+
+    g_string_append_printf(s, "</feature>");
+    info->desc = g_string_free(s, false);
+    return info->num;
+}
+
 struct TypeSize {
     const char *gdb_type;
     int  size;
@@ -567,6 +671,8 @@ const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname)
         return cpu->dyn_svereg_xml.desc;
     } else if (strcmp(xmlname, "arm-m-system.xml") == 0) {
         return cpu->dyn_m_systemreg_xml.desc;
+    } else if (strcmp(xmlname, "arm-m-secext.xml") == 0) {
+        return cpu->dyn_m_securereg_xml.desc;
     }
     return NULL;
 }
@@ -618,6 +724,16 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu)
                                      arm_gen_dynamic_m_systemreg_xml(
                                          cs, cs->gdb_num_regs),
                                      "arm-m-system.xml", 0);
+            if (arm_feature(env, ARM_FEATURE_V8) &&
+                    arm_feature(env, ARM_FEATURE_M_SECURITY)) {
+                gdb_register_coprocessor(cs,
+                                         arm_gdb_get_m_secextreg,
+                                         arm_gdb_set_m_secextreg,
+                                         arm_gen_dynamic_m_securereg_xml(
+                                             cs, cs->gdb_num_regs),
+                                         "arm-m-secext.xml", 0);
+
+            }
         }
     }
     if (cpu_isar_feature(aa32_mve, cpu)) {
diff --git a/target/arm/m_helper.c b/target/arm/m_helper.c
index e8fbe1599a..a9b27ad060 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -605,15 +605,10 @@ 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)
+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
@@ -765,8 +760,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 +1606,10 @@ 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 +1915,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;
 
         /*
-- 
2.34.5

[PATCH qemu v2 0/3] ARM: Add support for V8M special registers in GDB stub

[~dreiss-meta]

Patch 1/3 was already accepted, but it seems is not in master yet.

Comments addressed in patches 2 and 3.
Let me know if you'd like me to split out a separate commit
for renaming arm_v7m_get_sp_ptr.

David Reiss (3):
  target/arm: Unify checking for M Main Extension in MRS/MSR
  target/arm/gdbstub: Support reading M system registers from GDB
  target/arm/gdbstub: Support reading M security extension registers
    from GDB

 target/arm/cpu.h      |  25 ++++-
 target/arm/gdbstub.c  | 241 ++++++++++++++++++++++++++++++++++++++++++
 target/arm/m_helper.c |  51 +++++----
 3 files changed, 296 insertions(+), 21 deletions(-)

-- 
2.34.5

Re: [PATCH qemu v2 2/3] target/arm/gdbstub: Support reading M system registers from GDB

[Richard Henderson]

On 1/9/23 13:05, ~dreiss-meta 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.
> 
> `v7m_mrs_control` was renamed `arm_v7m_mrs_control` and made
> non-static so this logic could be shared between the MRS instruction and
> the GDB stub.
> 
> Signed-off-by: David Reiss <dreiss@meta.com>
> ---
>   target/arm/cpu.h      |  12 +++-
>   target/arm/gdbstub.c  | 125 ++++++++++++++++++++++++++++++++++++++++++
>   target/arm/m_helper.c |   6 +-
>   3 files changed, 138 insertions(+), 5 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index bf2bce046d..fdbb0d9107 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -856,6 +856,7 @@ struct ArchCPU {
>   
>       DynamicGDBXMLInfo dyn_sysreg_xml;
>       DynamicGDBXMLInfo dyn_svereg_xml;
> +    DynamicGDBXMLInfo dyn_m_systemreg_xml;

You don't need a new variable here, because a given cpu cannot be both a-profile and 
m-profile -- dyn_sysreg_xml can hold the xml for the current set of system registers.


> +    g_autoptr(GArray) regs = g_array_new(false, true, sizeof(const char *));
> +    /* 0 */ g_array_append_str_literal(regs, "msp");
> +    /* 1 */ g_array_append_str_literal(regs, "psp");
> +    /* 2 */ g_array_append_str_literal(regs, "primask");
> +    /* 3 */ g_array_append_str_literal(regs, is_main ? "basepri" : "");
> +    /* 4 */ g_array_append_str_literal(regs, is_main ? "faultmask" : "");
> +    /* 5 */ g_array_append_str_literal(regs, "control");
> +    /* 6 */ g_array_append_str_literal(regs, is_v8 ? "msplim" : "");
> +    /* 7 */ g_array_append_str_literal(regs, is_v8 ? "psplim" : "");

Can use NULL instead of ""?

Otherwise it looks plausible.


r~

Re: [PATCH qemu v2 3/3] target/arm/gdbstub: Support reading M security extension registers from GDB

[Richard Henderson]

On 1/9/23 13:05, ~dreiss-meta 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>
> ---
>   target/arm/cpu.h      |  15 +++++-
>   target/arm/gdbstub.c  | 116 ++++++++++++++++++++++++++++++++++++++++++
>   target/arm/m_helper.c |  23 ++++-----
>   3 files changed, 139 insertions(+), 15 deletions(-)

Is there a reason why these are separate from m_systemreg?


r~

Re: [PATCH qemu v2 2/3] target/arm/gdbstub: Support reading M system registers from GDB

[David Reiss]

On 1/17/23 1:40 PM, Richard Henderson wrote:
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index bf2bce046d..fdbb0d9107 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -856,6 +856,7 @@ struct ArchCPU {
>>         DynamicGDBXMLInfo dyn_sysreg_xml;
>>       DynamicGDBXMLInfo dyn_svereg_xml;
>> +    DynamicGDBXMLInfo dyn_m_systemreg_xml;
> 
> You don't need a new variable here, because a given cpu cannot be both a-profile and m-profile -- dyn_sysreg_xml can hold the xml for the current set of system registers.

Maybe I'm missing something?  It looks like arm_cpu_register_gdb_regs_for_features
unconditionally calls

gdb_register_coprocessor(..., arm_gen_dynamic_sysreg_xml(...), ...);

which would cause a collision.

Re: [PATCH qemu v2 3/3] target/arm/gdbstub: Support reading M security extension registers from GDB

[David Reiss]

On 1/17/23 1:42 PM, Richard Henderson wrote:
> Is there a reason why these are separate from m_systemreg?

GDB puts these in a separate file, and J-Link puts them in a separate feature block.
In general, I think it's nice to separate stuff related to the secure extension
so folks not working with it can ignore it more easily.

Re: [PATCH qemu v2 3/3] target/arm/gdbstub: Support reading M security extension registers from GDB

[Peter Maydell]

On Tue, 17 Jan 2023 at 22:48, David Reiss <dreiss@meta.com> wrote:
>
> On 1/17/23 1:42 PM, Richard Henderson wrote:
> > Is there a reason why these are separate from m_systemreg?
>
> GDB puts these in a separate file, and J-Link puts them in a separate feature block.
> In general, I think it's nice to separate stuff related to the secure extension
> so folks not working with it can ignore it more easily.

The reason is "there is an unwritten standard for where the
registers are supposed to go" -- if we don't follow it then
although gdb will display the registers to the user fine it
won't recognize them for purposes of doing operations like
bactracing across secure-calls. I have nudged a local gdb dev
to suggest that the gdb remote docs be improved to write down
what gdb requires in the way of XML feature and register names.

thanks
-- PMM

Re: [PATCH qemu v2 0/3] ARM: Add support for V8M special registers in GDB stub

[Peter Maydell]

On Tue, 17 Jan 2023 at 19:44, ~dreiss-meta <dreiss-meta@git.sr.ht> wrote:
>
> Patch 1/3 was already accepted, but it seems is not in master yet.
>
> Comments addressed in patches 2 and 3.
> Let me know if you'd like me to split out a separate commit
> for renaming arm_v7m_get_sp_ptr.

Hi; these patches look good to me. If you could respin
to fix the thing RTH pointed out about using NULL instead
of "" for "not present" then I'll take them.

thanks
-- PMM