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

[PATCH qemu 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 355cd4d60a..e5ccfa9615 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -2481,11 +2481,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;
             }
@@ -2531,8 +2537,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:
@@ -2597,13 +2609,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 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      |  11 +++-
 target/arm/gdbstub.c  | 125 ++++++++++++++++++++++++++++++++++++++++++
 target/arm/m_helper.c |   6 +-
 3 files changed, 137 insertions(+), 5 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 2b4bd20f9d..5cf86cf2d7 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -852,6 +852,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];
@@ -1094,11 +1095,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 generates 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
@@ -1490,6 +1493,10 @@ 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..4456892e91 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: PRIMAKS, 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;
+    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");
+
+    int is_v8 = arm_feature(env, ARM_FEATURE_V8);
+    int is_main = arm_feature(env, ARM_FEATURE_M_MAIN);
+
+    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 e5ccfa9615..31d4dfba4b 100644
--- a/target/arm/m_helper.c
+++ b/target/arm/m_helper.c
@@ -69,7 +69,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];
 
@@ -106,7 +106,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;
@@ -2436,7 +2436,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

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

[Peter Maydell]

On Mon, 9 Jan 2023 at 23:18, ~dreiss-meta <dreiss-meta@git.sr.ht> 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      |  11 +++-
>  target/arm/gdbstub.c  | 125 ++++++++++++++++++++++++++++++++++++++++++
>  target/arm/m_helper.c |   6 +-
>  3 files changed, 137 insertions(+), 5 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 2b4bd20f9d..5cf86cf2d7 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -852,6 +852,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];
> @@ -1094,11 +1095,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 generates XML descriptions of the sysregs,

we can fix the typo while we're changing this line: "dynamically generate"

> + * 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
> @@ -1490,6 +1493,10 @@ FIELD(SVCR, ZA, 1, 1)
>  FIELD(SMCR, LEN, 0, 4)
>  FIELD(SMCR, FA64, 31, 1)
>
> +/* Read the CONTROL register as the MRS instruction would.
> + */

QEMU comment style is either
/* one line comment */
or
/*
 * multiline comment, with the opening and closing
 * slash-star and star-slash on lines of their own
 */

We do still have some older parts of the codebase with different
styles, but new code should follow the coding style.
scripts/checkpatch.pl usually but doesn't always catch this.

> +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..4456892e91 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: PRIMAKS, 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.
> +     */

Indent on this comment seems odd.

> +    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;
> +    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");
> +
> +    int is_v8 = arm_feature(env, ARM_FEATURE_V8);
> +    int is_main = arm_feature(env, ARM_FEATURE_M_MAIN);

Use bool for these, please. Also, coding style says don't declare
variables in the middle of a block.

> +
> +    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);

Opinion seems to be divided on whether the registers here that
don't define all 32 bits should be reported as bitsize="32" or
not, eg OpenOCD reports primask etc as bitsize="1". But I found
at least one other generator of this XML which uses bitsize=32
throughout, so I guess we're good to do so also.

> +        }
> +        base_reg++;
> +    }
> +    info->num = regs->len;
> +
> +    g_string_append_printf(s, "</feature>");
> +    info->desc = g_string_free(s, false);
> +    return info->num;
> +}

thanks
-- PMM

[PATCH qemu 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     |   4 +-
 target/arm/gdbstub.c | 149 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 152 insertions(+), 1 deletion(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 5cf86cf2d7..b975c7d7db 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -853,6 +853,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];
@@ -1096,12 +1097,13 @@ int arm_cpu_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
 
 /*
  * Helpers to dynamically generates 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
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 4456892e91..2bf5a63534 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -437,6 +437,143 @@ int arm_gen_dynamic_m_systemreg_xml(CPUState *cs, int base_reg)
     return info->num;
 }
 
+struct v8m_stack_registers {
+    uint32_t msp_s;
+    uint32_t psp_s;
+    uint32_t msp_ns;
+    uint32_t psp_ns;
+};
+
+static struct v8m_stack_registers get_v8m_stack_registers(CPUARMState *env)
+{
+    uint32_t current_ss_msp;
+    uint32_t current_ss_psp;
+    if (v7m_using_psp(env)) {
+        current_ss_msp = env->v7m.other_sp;
+        current_ss_psp = env->regs[13];
+    } else {
+        current_ss_msp = env->regs[13];
+        current_ss_psp = env->v7m.other_sp;
+
+    }
+
+    struct v8m_stack_registers ret;
+    if (env->v7m.secure) {
+        ret.msp_s = current_ss_msp;
+        ret.psp_s = current_ss_psp;
+        ret.msp_ns = env->v7m.other_ss_msp;
+        ret.psp_ns = env->v7m.other_ss_psp;
+    } else {
+        ret.msp_s = env->v7m.other_ss_msp;
+        ret.psp_s = env->v7m.other_ss_psp;
+        ret.msp_ns = current_ss_msp;
+        ret.psp_ns = current_ss_psp;
+    }
+
+    return ret;
+}
+
+static int arm_gdb_get_m_secextreg(CPUARMState *env, GByteArray *buf, int reg)
+{
+    switch (reg) {
+    case  0:  /* MSP_S */
+        return gdb_get_reg32(buf, get_v8m_stack_registers(env).msp_s);
+    case  1:  /* PSP_S */
+        return gdb_get_reg32(buf, get_v8m_stack_registers(env).psp_s);
+    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, get_v8m_stack_registers(env).msp_ns);
+    case  9:  /* PSP_NS */
+        return gdb_get_reg32(buf, get_v8m_stack_registers(env).psp_ns);
+    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);
+    GString *s = g_string_new(NULL);
+    DynamicGDBXMLInfo *info = &cpu->dyn_m_securereg_xml;
+    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, "basepri_s");
+    /*  6 */ g_array_append_str_literal(regs, "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, "basepri_ns");
+    /* 14 */ g_array_append_str_literal(regs, "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 +704,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 +757,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)) {
-- 
2.34.5

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

[Peter Maydell]

On Mon, 9 Jan 2023 at 23:18, ~dreiss-meta <dreiss-meta@git.sr.ht> 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>

> +static struct v8m_stack_registers get_v8m_stack_registers(CPUARMState *env)
> +{
> +    uint32_t current_ss_msp;
> +    uint32_t current_ss_psp;
> +    if (v7m_using_psp(env)) {
> +        current_ss_msp = env->v7m.other_sp;
> +        current_ss_psp = env->regs[13];
> +    } else {
> +        current_ss_msp = env->regs[13];
> +        current_ss_psp = env->v7m.other_sp;
> +
> +    }
> +
> +    struct v8m_stack_registers ret;
> +    if (env->v7m.secure) {
> +        ret.msp_s = current_ss_msp;
> +        ret.psp_s = current_ss_psp;
> +        ret.msp_ns = env->v7m.other_ss_msp;
> +        ret.psp_ns = env->v7m.other_ss_psp;
> +    } else {
> +        ret.msp_s = env->v7m.other_ss_msp;
> +        ret.psp_s = env->v7m.other_ss_psp;
> +        ret.msp_ns = current_ss_msp;
> +        ret.psp_ns = current_ss_psp;
> +    }
> +
> +    return ret;
> +}

I wondered about using get_v7m_sp_ptr(), but this is fine I guess.

> +static int arm_gdb_get_m_secextreg(CPUARMState *env, GByteArray *buf, int reg)
> +{
> +    switch (reg) {
> +    case  0:  /* MSP_S */
> +        return gdb_get_reg32(buf, get_v8m_stack_registers(env).msp_s);
> +    case  1:  /* PSP_S */
> +        return gdb_get_reg32(buf, get_v8m_stack_registers(env).psp_s);
> +    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, get_v8m_stack_registers(env).msp_ns);
> +    case  9:  /* PSP_NS */
> +        return gdb_get_reg32(buf, get_v8m_stack_registers(env).psp_ns);
> +    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);
> +    GString *s = g_string_new(NULL);
> +    DynamicGDBXMLInfo *info = &cpu->dyn_m_securereg_xml;
> +    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, "basepri_s");
> +    /*  6 */ g_array_append_str_literal(regs, "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, "basepri_ns");
> +    /* 14 */ g_array_append_str_literal(regs, "faultmask_ns");
> +    /* 15 */ g_array_append_str_literal(regs, "control_ns");

In patch 1 you skip the registers that don't exist without
the main extension, but here you throw them all in regardless.
Why the difference ?

(If we do want to have the xml be fixed and not dependent on
whether the main extension exists, we don't need to runtime
generate it, but can instead use a fixed xml file, as we do with
eg arm-m-profile-mve.xml.)

> +
> +    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;
> +}

thanks
-- PMM

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

[David Reiss]

On 1/17/23 5:37 AM, Peter Maydell wrote:

> In patch 1 you skip the registers that don't exist without
> the main extension, but here you throw them all in regardless.
> Why the difference ?
Ah, yes.  This was an oversight.  I'm not sure if there are any
chips that support the security extension but not the main extension,
but it is technically possible.

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

[Peter Maydell]

On Mon, 9 Jan 2023 at 23:18, ~dreiss-meta <dreiss-meta@git.sr.ht> wrote:
>
> 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>
> ---

Hi; it looks like you didn't send a cover letter for this patchset.
If you're making contributions in future, if you could send a cover
letter for a multi-patch submission, that's helpful because our
automated tooling tends to get confused by patchsets which don't
have one. (A single standalone patch doesn't need a cover letter.)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

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

[Peter Maydell]

On Tue, 17 Jan 2023 at 11:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 9 Jan 2023 at 23:18, ~dreiss-meta <dreiss-meta@git.sr.ht> wrote:
> >
> > 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>
> > ---
>
> Hi; it looks like you didn't send a cover letter for this patchset.
> If you're making contributions in future, if you could send a cover
> letter for a multi-patch submission, that's helpful because our
> automated tooling tends to get confused by patchsets which don't
> have one. (A single standalone patch doesn't need a cover letter.)
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Since this patch is OK and not really related to 2 and 3,
I'm going to apply this to target-arm.next. I've left
review comments on the other two patches.

thanks
-- PMM