Re: [PATCH 7/9] KVM: arm64: selftests: Add a test case for a linked breakpoint

[Ricardo Koller <ricarkol@google.com>]

On Thu, Aug 25, 2022 at 06:29:34PM -0700, Reiji Watanabe wrote:
> On Wed, Aug 24, 2022 at 10:10 PM Reiji Watanabe <reijiw@google.com> wrote:
> >
> > Currently, the debug-exceptions test doesn't have a test case for
> > a linked breakpoint. Add a test case for the linked breakpoint to
> > the test.
> >
> > Signed-off-by: Reiji Watanabe <reijiw@google.com>
> >
> > ---
> >  .../selftests/kvm/aarch64/debug-exceptions.c  | 59 +++++++++++++++++--
> >  1 file changed, 55 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > index ab8860e3a9fa..9fccfeebccd3 100644
> > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c
> > @@ -11,6 +11,10 @@
> >  #define DBGBCR_EXEC    (0x0 << 3)
> >  #define DBGBCR_EL1     (0x1 << 1)
> >  #define DBGBCR_E       (0x1 << 0)
> > +#define DBGBCR_LBN_SHIFT       16
> > +#define DBGBCR_BT_SHIFT                20
> > +#define DBGBCR_BT_ADDR_LINK_CTX        (0x1 << DBGBCR_BT_SHIFT)
> > +#define DBGBCR_BT_CTX_LINK     (0x3 << DBGBCR_BT_SHIFT)
> >
> >  #define DBGWCR_LEN8    (0xff << 5)
> >  #define DBGWCR_RD      (0x1 << 3)
> > @@ -21,7 +25,7 @@
> >  #define SPSR_D         (1 << 9)
> >  #define SPSR_SS                (1 << 21)
> >
> > -extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start;
> > +extern unsigned char sw_bp, sw_bp2, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start, hw_bp_ctx;
> >  static volatile uint64_t sw_bp_addr, hw_bp_addr;
> >  static volatile uint64_t wp_addr, wp_data_addr;
> >  static volatile uint64_t svc_addr;
> > @@ -103,6 +107,7 @@ static void reset_debug_state(void)
> >         isb();
> >
> >         write_sysreg(0, mdscr_el1);
> > +       write_sysreg(0, contextidr_el1);
> >
> >         /* Reset all bcr/bvr/wcr/wvr registers */
> >         dfr0 = read_sysreg(id_aa64dfr0_el1);
> > @@ -164,6 +169,28 @@ static void install_hw_bp(uint8_t bpn, uint64_t addr)
> >         enable_debug_bwp_exception();
> >  }
> >
> > +void install_hw_bp_ctx(uint8_t addr_bp, uint8_t ctx_bp, uint64_t addr,
> > +                      uint64_t ctx)
> > +{
> > +       uint32_t addr_bcr, ctx_bcr;
> > +
> > +       /* Setup a context-aware breakpoint */
> > +       ctx_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> > +                 DBGBCR_BT_CTX_LINK;
> > +       write_dbgbcr(ctx_bp, ctx_bcr);
> > +       write_dbgbvr(ctx_bp, ctx);
> > +
> > +       /* Setup a linked breakpoint (linked to the context-aware breakpoint) */
> > +       addr_bcr = DBGBCR_LEN8 | DBGBCR_EXEC | DBGBCR_EL1 | DBGBCR_E |
> > +                  DBGBCR_BT_ADDR_LINK_CTX |
> > +                  ((uint32_t)ctx_bp << DBGBCR_LBN_SHIFT);
> > +       write_dbgbcr(addr_bp, addr_bcr);
> > +       write_dbgbvr(addr_bp, addr);
> > +       isb();
> > +
> > +       enable_debug_bwp_exception();
> > +}
> > +
> >  static void install_ss(void)
> >  {
> >         uint32_t mdscr;
> > @@ -177,8 +204,10 @@ static void install_ss(void)
> >
> >  static volatile char write_data;
> >
> > -static void guest_code(uint8_t bpn, uint8_t wpn)
> > +static void guest_code(uint8_t bpn, uint8_t wpn, uint8_t ctx_bpn)
> >  {
> > +       uint64_t ctx = 0x1;     /* a random context number */
> > +
> >         GUEST_SYNC(0);
> >
> >         /* Software-breakpoint */
> > @@ -281,6 +310,19 @@ static void guest_code(uint8_t bpn, uint8_t wpn)
> >                      : : : "x0");
> >         GUEST_ASSERT_EQ(ss_addr[0], 0);
> >
> 
> I've just noticed that I should add GUEST_SYNC(10) here, use
> GUEST_SYNC(11) for the following test case, and update the
> stage limit value in the loop in userspace code.
> 
> Or I might consider removing the stage management code itself.
> It doesn't appear to be very useful to me, and I would think
> we could easily forget to update it :-)
> 
> Thank you,
> Reiji
>

Yes, it's better to remove it. The intention was to make sure the guest
generates the expected sequence of exits. In this case for example,
"1, .., 11, DONE" would be correct, but "1, .., 11, 12, DONE" would not.

> > +       /* Linked hardware-breakpoint */
> > +       hw_bp_addr = 0;
> > +       reset_debug_state();
> > +       install_hw_bp_ctx(bpn, ctx_bpn, PC(hw_bp_ctx), ctx);
> > +       /* Set context id */
> > +       write_sysreg(ctx, contextidr_el1);
> > +       isb();
> > +       asm volatile("hw_bp_ctx: nop");
> > +       write_sysreg(0, contextidr_el1);
> > +       GUEST_ASSERT_EQ(hw_bp_addr, PC(hw_bp_ctx));
> > +
> > +       GUEST_SYNC(10);
> > +
> >         GUEST_DONE();
> >  }
> >
> > @@ -327,6 +369,7 @@ int main(int argc, char *argv[])
> >         struct ucall uc;
> >         int stage;
> >         uint64_t aa64dfr0;
> > +       uint8_t brps;
> >
> >         vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> >         ucall_init(vm, NULL);
> > @@ -349,8 +392,16 @@ int main(int argc, char *argv[])
> >         vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT,
> >                                 ESR_EC_SVC64, guest_svc_handler);
> >
> > -       /* Run tests with breakpoint#0 and watchpoint#0. */
> > -       vcpu_args_set(vcpu, 2, 0, 0);
> > +       /* Number of breakpoints, minus 1 */
> > +       brps = cpuid_get_ufield(aa64dfr0, ID_AA64DFR0_BRPS_SHIFT);
> > +       __TEST_REQUIRE(brps > 0, "At least two breakpoints are required");
> > +
> > +       /*
> > +        * Run tests with breakpoint#0 and watchpoint#0, and the higiest
> > +        * numbered (context-aware) breakpoint.
> > +        */
> > +       vcpu_args_set(vcpu, 3, 0, 0, brps);
> > +
> >         for (stage = 0; stage < 11; stage++) {
> >                 vcpu_run(vcpu);
> >
> > --
> > 2.37.1.595.g718a3a8f04-goog
> >

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel