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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > >
> > > ---
> > > .../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
> > >
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
