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 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/m_helper.c +++ b/target/arm/m_helper.c @@ -XXX,XX +XXX,XX @@ 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; } @@ -XXX,XX +XXX,XX @@ 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: @@ -XXX,XX +XXX,XX @@ 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 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ 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]; @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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; } @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/m_helper.c +++ b/target/arm/m_helper.c @@ -XXX,XX +XXX,XX @@ 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]; @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ 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]; @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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; } @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/m_helper.c +++ b/target/arm/m_helper.c @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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]; @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 Rebased and updated to use NULL as a sentinel. David Reiss (2): 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 | 29 ++--- tests/lcitool/libvirt-ci | 2 +- 4 files changed, 277 insertions(+), 20 deletions(-) -- 2.34.5 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 +- tests/lcitool/libvirt-ci | 2 +- 4 files changed, 139 insertions(+), 6 deletions(-) diff --git a/target/arm/cpu.h b/target/arm/cpu.h index XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ 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]; @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -XXX,XX +XXX,XX @@ 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" : NULL); + /* 4 */ g_array_append_str_literal(regs, is_main ? "faultmask" : NULL); + /* 5 */ g_array_append_str_literal(regs, "control"); + /* 6 */ g_array_append_str_literal(regs, is_v8 ? "msplim" : NULL); + /* 7 */ g_array_append_str_literal(regs, is_v8 ? "psplim" : NULL); + + for (int idx = 0; idx < regs->len; idx++) { + const char *name = g_array_index(regs, const char *, idx); + if (name) { + 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; @@ -XXX,XX +XXX,XX @@ 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; } @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/m_helper.c +++ b/target/arm/m_helper.c @@ -XXX,XX +XXX,XX @@ 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]; @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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 diff --git a/tests/lcitool/libvirt-ci b/tests/lcitool/libvirt-ci index XXXXXXX..XXXXXXX 160000 --- a/tests/lcitool/libvirt-ci +++ b/tests/lcitool/libvirt-ci @@ -1 +1 @@ -Subproject commit 319a534c220f53fc8670254cac25d6f662c82112 +Subproject commit e3eb28cf2e17fbcf7fe7e19505ee432b8ec5bbb5 -- 2.34.5 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -XXX,XX +XXX,XX @@ 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]; @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -XXX,XX +XXX,XX @@ 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" : NULL); + /* 6 */ g_array_append_str_literal(regs, is_main ? "faultmask_s" : NULL); + /* 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" : NULL); + /* 14 */ g_array_append_str_literal(regs, is_main ? "faultmask_ns" : NULL); + /* 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) { + 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; @@ -XXX,XX +XXX,XX @@ 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; } @@ -XXX,XX +XXX,XX @@ 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 XXXXXXX..XXXXXXX 100644 --- a/target/arm/m_helper.c +++ b/target/arm/m_helper.c @@ -XXX,XX +XXX,XX @@ 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 @@ -XXX,XX +XXX,XX @@ 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]; @@ -XXX,XX +XXX,XX @@ 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; @@ -XXX,XX +XXX,XX @@ 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