Re: Possible bug in Aarch64 single-stepping [PATCH]

[Chris Howard <cvz185@web.de>]

On 8. May 2022, at 14:18, Peter Maydell <peter.maydell@linaro.org> wrote:

> On Sat, 7 May 2022 at 15:18, Chris Howard <cvz185@web.de> 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);
      }
  }