| Message ID | 20230214163048.903964-15-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: gdbstub cleanups and additions | expand |
On 2/14/23 06:30, Richard Henderson wrote: > + for (i = 0; i < ret; i++) { > + if (arm_feature(env, m_systemreg_def[i].feature)) { > + g_string_append_printf(s, > + "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", > + m_systemreg_def[i].name, base_reg + i); > + } > + } > + > + if (arm_feature(env, ARM_FEATURE_M_SECURITY)) { > + for (i = 0; i < ret; i++) { > + g_string_append_printf(s, > + "<reg name=\"%s_ns\" bitsize=\"32\" regnum=\"%d\"/>\n", > + m_systemreg_def[i].name, base_reg + (i | M_SYSREG_NONSECURE)); > + } > + for (i = 0; i < ret; i++) { > + g_string_append_printf(s, > + "<reg name=\"%s_s\" bitsize=\"32\" regnum=\"%d\"/>\n", > + m_systemreg_def[i].name, base_reg + (i | M_SYSREG_SECURE)); > + } > + QEMU_BUILD_BUG_ON(M_SYSREG_SECURE < M_SYSREG_NONSECURE); > + ret |= M_SYSREG_SECURE; > + } Peter, from our chat on IRC it sounds as if you would put this block #ifndef CONFIG_USER_ONLY. r~
On Tue, 14 Feb 2023 at 16:31, Richard Henderson <richard.henderson@linaro.org> wrote: > > From: David Reiss <dreiss@meta.com> > > Follows a fairly similar pattern to the existing special register > debug support. Only reading is implemented, but it should be > possible to implement writes. > > Signed-off-by: David Reiss <dreiss@meta.com> > [rth: Split out from two other patches; > Use an enumeration to locally number the registers. > Use a structure to list and control runtime visibility. > Handle security extension with the same code.] > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/cpu.h | 1 + > target/arm/gdbstub.c | 169 +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 170 insertions(+) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index c9f768f945..536e60d48c 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -867,6 +867,7 @@ struct ArchCPU { > > DynamicGDBXMLInfo dyn_sysreg_xml; > DynamicGDBXMLInfo dyn_svereg_xml; > + DynamicGDBXMLInfo dyn_m_systemreg_xml; > > /* Timers used by the generic (architected) timer */ > QEMUTimer *gt_timer[NUM_GTIMERS]; > diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c > index 062c8d447a..a8848c7fee 100644 > --- a/target/arm/gdbstub.c > +++ b/target/arm/gdbstub.c > @@ -322,6 +322,167 @@ static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg) > return cpu->dyn_sysreg_xml.num; > } > > +enum { > + M_SYSREG_MSP = 0, > + M_SYSREG_PSP = 1, > + M_SYSREG_PRIMASK = 2, > + M_SYSREG_CONTROL = 3, > + M_SYSREG_BASEPRI = 4, > + M_SYSREG_FAULTMASK = 5, > + M_SYSREG_MSPLIM = 6, > + M_SYSREG_PSPLIM = 7, > + M_SYSREG_REG_MASK = 7, > + > + /* > + * NOTE: MSP, PSP, MSPLIM, PSPLIM technically don't exist if the > + * secure extension is present (replaced by MSP_S, MSP_NS, et al). > + * However, the MRS instruction is still allowed to read from MSP and PSP, > + * and will return the value associated with the current security state. What's "don't exist" based on here? Architecturally MSPLIM, PSPLIM etc are banked registers; MRS/MSR let you access the one for the current security state, and (if you are Secure) the one for the NS state. > + * We replicate this behavior for the convenience of users, who will see > + * GDB behave similarly to their assembly code, even if they are oblivious > + * to the security extension. > + */ > + M_SYSREG_CURRENT = 0 << 3, > + M_SYSREG_NONSECURE = 1 << 3, > + M_SYSREG_SECURE = 2 << 3, > + M_SYSREG_MODE_MASK = 3 << 3, > +}; > + > +static int arm_gen_dynamic_m_systemreg_xml(CPUState *cs, int base_reg) > +{ > + ARMCPU *cpu = ARM_CPU(cs); > + CPUARMState *env = &cpu->env; > + GString *s = g_string_new(NULL); > + int i, ret; > + > + g_string_printf(s, "<?xml version=\"1.0\"?>"); > + g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"); > + g_string_append_printf(s, "<feature name=\"org.gnu.gdb.arm.m-system\">\n"); Half of these need to be in org.gnu.gdb.arm.secext. These aren't our own XML features we're making up (if they were then they would be in org.qemu.something), so we should follow the existing precedent about what registers go in them. thanks -- PMM
On 2/20/23 06:02, Peter Maydell wrote: >> + /* >> + * NOTE: MSP, PSP, MSPLIM, PSPLIM technically don't exist if the >> + * secure extension is present (replaced by MSP_S, MSP_NS, et al). >> + * However, the MRS instruction is still allowed to read from MSP and PSP, >> + * and will return the value associated with the current security state. > > What's "don't exist" based on here? Architecturally MSPLIM, PSPLIM etc > are banked registers; MRS/MSR let you access the one for the current > security state, and (if you are Secure) the one for the NS state. Copied that wording from David. >> + g_string_printf(s, "<?xml version=\"1.0\"?>"); >> + g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"); >> + g_string_append_printf(s, "<feature name=\"org.gnu.gdb.arm.m-system\">\n"); > > Half of these need to be in org.gnu.gdb.arm.secext. > These aren't our own XML features we're making up (if they > were then they would be in org.qemu.something), so we should > follow the existing precedent about what registers go in them. Now that you point it out (and I should have checked myself), we are kinda making them up. The only registers within upstream gdb m-system and secext are MSP, PSP, MSP_NS, MSP_S, PSP_NS, PSP_S. All the others are our own addition. Should all the rest be in a third bit of xml? r~
On Mon, 20 Feb 2023 at 17:00, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/20/23 06:02, Peter Maydell wrote: > >> + g_string_printf(s, "<?xml version=\"1.0\"?>"); > >> + g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"); > >> + g_string_append_printf(s, "<feature name=\"org.gnu.gdb.arm.m-system\">\n"); > > > > Half of these need to be in org.gnu.gdb.arm.secext. > > These aren't our own XML features we're making up (if they > > were then they would be in org.qemu.something), so we should > > follow the existing precedent about what registers go in them. > > Now that you point it out (and I should have checked myself), we are kinda making them up. > The only registers within upstream gdb m-system and secext are MSP, PSP, MSP_NS, MSP_S, > PSP_NS, PSP_S. All the others are our own addition. I think OpenOCD's implementation includes more than that: https://openocd.org/doc-release/doxygen/armv7m_8c_source.html > Should all the rest be in a third bit of xml? Luis, do you have the specs for what the existing implementations are doing here ? Ideally gdb should document for every bit of XML it is the official owner of (ie in the org.gnu.gdb namespace) what the required and optional register values are, including details like the register width (which I think the two existing implementations that output m-system disagree on). thanks -- PMM
Hi, On 2/20/23 17:37, Peter Maydell wrote: > On Mon, 20 Feb 2023 at 17:00, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 2/20/23 06:02, Peter Maydell wrote: >>>> + g_string_printf(s, "<?xml version=\"1.0\"?>"); >>>> + g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"); >>>> + g_string_append_printf(s, "<feature name=\"org.gnu.gdb.arm.m-system\">\n"); >>> >>> Half of these need to be in org.gnu.gdb.arm.secext. >>> These aren't our own XML features we're making up (if they >>> were then they would be in org.qemu.something), so we should >>> follow the existing precedent about what registers go in them. >> >> Now that you point it out (and I should have checked myself), we are kinda making them up. >> The only registers within upstream gdb m-system and secext are MSP, PSP, MSP_NS, MSP_S, >> PSP_NS, PSP_S. All the others are our own addition. > > I think OpenOCD's implementation includes more than that: > https://openocd.org/doc-release/doxygen/armv7m_8c_source.html > >> Should all the rest be in a third bit of xml? > > Luis, do you have the specs for what the existing implementations > are doing here ? Support for the extra stack pointers was contributed by ST (Torbjörn and Yvan cc-ed), so I'd say ST-Link was the debugging stub the GDB changes were based on (to my knowledge). This support includes the org.gnu.gdb.arm.m-system and org.gnu.gdb.arm.secext features. The org.gnu.gdb.arm.m-system feature contains msp and psp (gdb/features/arm/arm-m-system.xml) and the org.gnu.gdb.arm.secext feature contains the msp_ns, msp_s, psp_ns and psp_s registers (gdb/features/arm/arm-secext.xml). All of the registers should be 32-bit in size. There could be extra registers in those two features, but gdb only cares about the 6 registers listed above. It is meant for gdb to detect if the additional stack pointer registers are available and, if so, track their values. Adding the stack pointers to other features should be OK, but gdb needs to know about that so it can go looking for them. The most convenient way to do it is to follow the original features we modeled things from. In this case, the XML layout from ST's contributions. > > Ideally gdb should document for every bit of XML it is the > official owner of (ie in the org.gnu.gdb namespace) what the > required and optional register values are, including details > like the register width (which I think the two existing > implementations that output m-system disagree on). I went through the OpenOCD code and it seems it has additional registers in the m-system category. That should be fine, as long as gdb sees msp and psp. Of course, all of this would be much better if properly documented in the gdb manual, which unfortunately it isn't (in much or any detail). Apologies for that. I'll set some time aside to get the documentation updated / written for these features. In the mean time, feel free to ping me if something needs to be clarified. > > thanks > -- PMM IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index c9f768f945..536e60d48c 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -867,6 +867,7 @@ struct ArchCPU { DynamicGDBXMLInfo dyn_sysreg_xml; DynamicGDBXMLInfo dyn_svereg_xml; + DynamicGDBXMLInfo dyn_m_systemreg_xml; /* Timers used by the generic (architected) timer */ QEMUTimer *gt_timer[NUM_GTIMERS]; diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c index 062c8d447a..a8848c7fee 100644 --- a/target/arm/gdbstub.c +++ b/target/arm/gdbstub.c @@ -322,6 +322,167 @@ static int arm_gen_dynamic_sysreg_xml(CPUState *cs, int base_reg) return cpu->dyn_sysreg_xml.num; } +enum { + M_SYSREG_MSP = 0, + M_SYSREG_PSP = 1, + M_SYSREG_PRIMASK = 2, + M_SYSREG_CONTROL = 3, + M_SYSREG_BASEPRI = 4, + M_SYSREG_FAULTMASK = 5, + M_SYSREG_MSPLIM = 6, + M_SYSREG_PSPLIM = 7, + M_SYSREG_REG_MASK = 7, + + /* + * NOTE: MSP, PSP, MSPLIM, PSPLIM technically don't exist if the + * secure extension is present (replaced by MSP_S, MSP_NS, et al). + * However, the MRS instruction is still allowed to read from MSP and PSP, + * and will return the value associated with the current security state. + * We replicate this behavior for the convenience of users, who will see + * GDB behave similarly to their assembly code, even if they are oblivious + * to the security extension. + */ + M_SYSREG_CURRENT = 0 << 3, + M_SYSREG_NONSECURE = 1 << 3, + M_SYSREG_SECURE = 2 << 3, + M_SYSREG_MODE_MASK = 3 << 3, +}; + +static const struct { + const char *name; + int feature; +} m_systemreg_def[] = { + [M_SYSREG_MSP] = { "msp", ARM_FEATURE_M }, + [M_SYSREG_PSP] = { "psp", ARM_FEATURE_M }, + [M_SYSREG_PRIMASK] = { "primask", ARM_FEATURE_M }, + [M_SYSREG_CONTROL] = { "control", ARM_FEATURE_M }, + [M_SYSREG_BASEPRI] = { "basepri", ARM_FEATURE_M_MAIN }, + [M_SYSREG_FAULTMASK] = { "faultmask", ARM_FEATURE_M_MAIN }, + [M_SYSREG_MSPLIM] = { "msplim", ARM_FEATURE_V8 }, + [M_SYSREG_PSPLIM] = { "psplim", ARM_FEATURE_V8 }, +}; + +static int arm_gdb_get_m_systemreg(CPUARMState *env, GByteArray *buf, int reg) +{ + int mode = reg & M_SYSREG_MODE_MASK; + bool secure; + uint32_t val; + + switch (mode) { + case M_SYSREG_CURRENT: + secure = env->v7m.secure; + break; + case M_SYSREG_NONSECURE: + secure = false; + break; + case M_SYSREG_SECURE: + secure = true; + break; + default: + return 0; + } + + reg &= M_SYSREG_REG_MASK; + if (reg >= ARRAY_SIZE(m_systemreg_def)) { + return 0; + } + if (!arm_feature(env, m_systemreg_def[reg].feature)) { + return 0; + } + + /* NOTE: This implementation shares a lot of logic with v7m_mrs. */ + switch (reg) { + case M_SYSREG_MSP: + val = *arm_v7m_get_sp_ptr(env, secure, false, true); + break; + case M_SYSREG_PSP: + val = *arm_v7m_get_sp_ptr(env, secure, true, true); + break; + case M_SYSREG_MSPLIM: + val = env->v7m.msplim[secure]; + break; + case M_SYSREG_PSPLIM: + val = env->v7m.psplim[secure]; + break; + case M_SYSREG_PRIMASK: + val = env->v7m.primask[secure]; + break; + case M_SYSREG_BASEPRI: + val = env->v7m.basepri[secure]; + break; + case M_SYSREG_FAULTMASK: + val = env->v7m.faultmask[secure]; + break; + case M_SYSREG_CONTROL: + /* + * NOTE: CONTROL has a mix of banked and non-banked bits. + * For "current", we emulate the MRS instruction. + * Unfortunately, this gives GDB no way to read the SFPA bit + * when the CPU is in a non-secure state. + */ + if (mode == M_SYSREG_CURRENT) { + val = arm_v7m_mrs_control(env, secure); + } else { + val = env->v7m.control[secure]; + } + break; + default: + g_assert_not_reached(); + } + + return gdb_get_reg32(buf, val); +} + +static int arm_gdb_set_m_systemreg(CPUARMState *env, uint8_t *buf, int reg) +{ + /* TODO: Implement. */ + return 0; +} + +static int arm_gen_dynamic_m_systemreg_xml(CPUState *cs, int base_reg) +{ + ARMCPU *cpu = ARM_CPU(cs); + CPUARMState *env = &cpu->env; + GString *s = g_string_new(NULL); + int i, ret; + + g_string_printf(s, "<?xml version=\"1.0\"?>"); + g_string_append_printf(s, "<!DOCTYPE target SYSTEM \"gdb-target.dtd\">"); + g_string_append_printf(s, "<feature name=\"org.gnu.gdb.arm.m-system\">\n"); + + QEMU_BUILD_BUG_ON(M_SYSREG_CURRENT != 0); + ret = ARRAY_SIZE(m_systemreg_def); + + for (i = 0; i < ret; i++) { + if (arm_feature(env, m_systemreg_def[i].feature)) { + g_string_append_printf(s, + "<reg name=\"%s\" bitsize=\"32\" regnum=\"%d\"/>\n", + m_systemreg_def[i].name, base_reg + i); + } + } + + if (arm_feature(env, ARM_FEATURE_M_SECURITY)) { + for (i = 0; i < ret; i++) { + g_string_append_printf(s, + "<reg name=\"%s_ns\" bitsize=\"32\" regnum=\"%d\"/>\n", + m_systemreg_def[i].name, base_reg + (i | M_SYSREG_NONSECURE)); + } + for (i = 0; i < ret; i++) { + g_string_append_printf(s, + "<reg name=\"%s_s\" bitsize=\"32\" regnum=\"%d\"/>\n", + m_systemreg_def[i].name, base_reg + (i | M_SYSREG_SECURE)); + } + QEMU_BUILD_BUG_ON(M_SYSREG_SECURE < M_SYSREG_NONSECURE); + ret |= M_SYSREG_SECURE; + } + + g_string_append_printf(s, "</feature>"); + + cpu->dyn_m_systemreg_xml.desc = g_string_free(s, false); + cpu->dyn_m_systemreg_xml.num = ret; + return ret; +} + const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname) { ARMCPU *cpu = ARM_CPU(cs); @@ -330,6 +491,8 @@ const char *arm_gdb_get_dynamic_xml(CPUState *cs, const char *xmlname) return cpu->dyn_sysreg_xml.desc; } else if (strcmp(xmlname, "sve-registers.xml") == 0) { return cpu->dyn_svereg_xml.desc; + } else if (strcmp(xmlname, "arm-m-system.xml") == 0) { + return cpu->dyn_m_systemreg_xml.desc; } return NULL; } @@ -389,4 +552,10 @@ void arm_cpu_register_gdb_regs_for_features(ARMCPU *cpu) arm_gen_dynamic_sysreg_xml(cs, cs->gdb_num_regs), "system-registers.xml", 0); + if (arm_feature(env, ARM_FEATURE_M)) { + gdb_register_coprocessor(cs, + arm_gdb_get_m_systemreg, arm_gdb_set_m_systemreg, + arm_gen_dynamic_m_systemreg_xml(cs, cs->gdb_num_regs), + "arm-m-system.xml", 0); + } }