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

[Ricardo Koller <ricarkol@google.com>]

On Fri, Sep 09, 2022 at 01:18:28PM -0700, Ricardo Koller wrote:
> 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.

Sorry, the correct sequence should be "1, .., 10, DONE". And also, what
I meant to say is that *original* intention was to check that, which
wasn't actually completed as the incorrect sequence would also succeed.

> 
> > > +       /* 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