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

[Chris Howard]

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