On 8. May 2022, at 14:18, Peter Maydell <[email protected]> wrote:
> On Sat, 7 May 2022 at 15:18, Chris Howard <[email protected]> wrote: >> PS. In plain gdb (ie. no nice user interface) a large number (but not all) >> of the system registers gets displayed after each step. It would be nice if >> these were sorted in some way. At the moment they’re completely jumbled — >> not alphabetic, not grouped by EL, nor by “meaning” (DBGWVR0_EL1 isn’t >> necessarily next to DBGWCR0_EL1). >> >> Also, there are multiple (identical?) instances of “DBGBVR” and “DBGBCR” >> (and “DBGWVR” and “DBGWCR”) rather than the expected “DBGWVR0_EL1”, >> “DBGWVR1_EL1” etc. >> >> Would this be a QEMU or a GDB issue? Or isn’t it an issue at all? :-) > > My gdb doesn't do that. Basically QEMU provides gdb with some XML > telling it that the sysregs are present, but it's up to gdb at > what points it chooses to display what registers and how it does that. > > The system register read access via the gdbstub is "best-effort" > on QEMU's part -- we implement it to the extent that it wasn't too > difficult to do, but there are some sharp edges, like the > register names not always being quite right, and also the way > that if you try to read a register that isn't supposed to be > accessible by the current EL you might find it's not correct. > Trying to read SP_EL2 while at EL2 is an example of that. > > The reason register names are sometimes funny is that the > infrastructure for system registers within QEMU was originally > written with the assumption that the name strings were merely > for convenience when debugging QEMU itself, so it's sometimes > a bit careless about them. We only added the "tell GDB about > these" part later. > > That said, adding the numbers into the watchpoint and breakpoint > registers would be pretty easy, so we should do that. That is, > in this code: > https://gitlab.com/qemu-project/qemu/-/blob/master/target/arm/helper.c#L6567 > we should use g_strdup_printf() to create unique per-register > names, the same way we do for the PMU registers already here: > https://gitlab.com/qemu-project/qemu/-/blob/master/target/arm/helper.c#L6632 > > thanks > -- PMM Thanks for the explanation. What with this being “pretty easy” I’m attempting my first ever patch! :-) BE WARNED! This is a context diff with respect to the cloned git repository (Version 7.0.50) $ git clone https://gitlab.com/qemu-project/qemu.git created with $ diff -c qemu/target/arm/helper.c qemu-patch/target/arm/helper.c > aarch-dbg-regnames.patch to be applied (in the target/arm directory) with $ patch -p3 <../../../aarch-dbg-regnames.patch — chris *** qemu/target/arm/helper.c 2022-05-08 20:41:48.000000000 +0200 --- qemu-patch/target/arm/helper.c 2022-05-08 20:55:25.000000000 +0200 *************** *** 6551,6559 **** define_one_arm_cp_reg(cpu, &dbgdidr); } ! /* Note that all these register fields hold "number of Xs minus 1". */ ! brps = arm_num_brps(cpu); ! wrps = arm_num_wrps(cpu); ctx_cmps = arm_num_ctx_cmps(cpu); assert(ctx_cmps <= brps); --- 6551,6559 ---- define_one_arm_cp_reg(cpu, &dbgdidr); } ! /* Note that all these reg fields (in ID_AA64DFR0_EL1) hold "number of Xs minus 1". */ ! brps = arm_num_brps(cpu); /* returns actual number of breakpoints */ ! wrps = arm_num_wrps(cpu); /* returns actual number of watchpoints */ ctx_cmps = arm_num_ctx_cmps(cpu); assert(ctx_cmps <= brps); *************** *** 6565,6578 **** } for (i = 0; i < brps; i++) { ARMCPRegInfo dbgregs[] = { ! { .name = "DBGBVR", .state = ARM_CP_STATE_BOTH, .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 4, .access = PL1_RW, .accessfn = access_tda, .fieldoffset = offsetof(CPUARMState, cp15.dbgbvr[i]), .writefn = dbgbvr_write, .raw_writefn = raw_write }, ! { .name = "DBGBCR", .state = ARM_CP_STATE_BOTH, .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 5, .access = PL1_RW, .accessfn = access_tda, .fieldoffset = offsetof(CPUARMState, cp15.dbgbcr[i]), --- 6565,6580 ---- } for (i = 0; i < brps; i++) { + char *dbgbvr_el1_name = g_strdup_printf("DBGBVR%d_EL1", i); + char *dbgbcr_el1_name = g_strdup_printf("DBGBCR%d_EL1", i); ARMCPRegInfo dbgregs[] = { ! { .name = dbgbvr_el1_name, .state = ARM_CP_STATE_BOTH, .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 4, .access = PL1_RW, .accessfn = access_tda, .fieldoffset = offsetof(CPUARMState, cp15.dbgbvr[i]), .writefn = dbgbvr_write, .raw_writefn = raw_write }, ! { .name = dbgbcr_el1_name, .state = ARM_CP_STATE_BOTH, .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 5, .access = PL1_RW, .accessfn = access_tda, .fieldoffset = offsetof(CPUARMState, cp15.dbgbcr[i]), *************** *** 6580,6596 **** }, }; define_arm_cp_regs(cpu, dbgregs); } for (i = 0; i < wrps; i++) { ARMCPRegInfo dbgregs[] = { ! { .name = "DBGWVR", .state = ARM_CP_STATE_BOTH, .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 6, .access = PL1_RW, .accessfn = access_tda, .fieldoffset = offsetof(CPUARMState, cp15.dbgwvr[i]), .writefn = dbgwvr_write, .raw_writefn = raw_write }, ! { .name = "DBGWCR", .state = ARM_CP_STATE_BOTH, .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 7, .access = PL1_RW, .accessfn = access_tda, .fieldoffset = offsetof(CPUARMState, cp15.dbgwcr[i]), --- 6582,6602 ---- }, }; define_arm_cp_regs(cpu, dbgregs); + g_free(dbgbvr_el1_name); + g_free(dbgbcr_el1_name); } for (i = 0; i < wrps; i++) { + char *dbgwvr_el1_name = g_strdup_printf("DBGWVR%d_EL1", i); + char *dbgwcr_el1_name = g_strdup_printf("DBGWCR%d_EL1", i); ARMCPRegInfo dbgregs[] = { ! { .name = dbgwvr_el1_name, .state = ARM_CP_STATE_BOTH, .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 6, .access = PL1_RW, .accessfn = access_tda, .fieldoffset = offsetof(CPUARMState, cp15.dbgwvr[i]), .writefn = dbgwvr_write, .raw_writefn = raw_write }, ! { .name = dbgwcr_el1_name, .state = ARM_CP_STATE_BOTH, .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = i, .opc2 = 7, .access = PL1_RW, .accessfn = access_tda, .fieldoffset = offsetof(CPUARMState, cp15.dbgwcr[i]), *************** *** 6598,6603 **** --- 6604,6611 ---- }, }; define_arm_cp_regs(cpu, dbgregs); + g_free(dbgwvr_el1_name); + g_free(dbgwcr_el1_name); } }
