| Message ID | 20220825050846.3418868-8-reijiw@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: selftests: Test linked {break,watch}points | expand |
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 > + /* 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 >
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 > >
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 > > >
On Wed, Aug 24, 2022 at 10:08:44PM -0700, Reiji Watanabe 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. I would add some more detail, like the fact that this is a pair of breakpoints: one is a context-aware breakpoint, and the other one is an address breakpoint linked to the first one. > > 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; ^^^^^ isn't this a regular context-aware breakpoint? the other one is the linked one. > + 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); Just a curiosity, can the context-aware one link to this one? > + 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 */ nit: make this number a bit more unlikely to happen by mistake. I guess you could use all available 32 bits. > + > 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); > > + /* 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); If brps is "number of breakpoints", then there should be a "+ 1" above. Otherwise brps is really "last breakpoint" (last_brp). > + __TEST_REQUIRE(brps > 0, "At least two breakpoints are required"); Yes, based on this test, brps is really "last breakpoint". I would suggest changing the name to "last_brp" (or something similar). > + > + /* > + * Run tests with breakpoint#0 and watchpoint#0, and the higiest * Run tests with breakpoint#0, watchpoint#0, and the highest > + * 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 >
Hi Ricardo, Thank you for the review! On Fri, Sep 9, 2022 at 2:01 PM Ricardo Koller <ricarkol@google.com> wrote: > > On Wed, Aug 24, 2022 at 10:08:44PM -0700, Reiji Watanabe 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. > > I would add some more detail, like the fact that this is a pair of > breakpoints: one is a context-aware breakpoint, and the other one > is an address breakpoint linked to the first one. Sure, I would add more detail. > > > > > 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; > ^^^^^ > isn't this a regular context-aware breakpoint? > the other one is the linked one. That is one of the types that we could use only for context-aware breakpoints (Linked Context ID Match breakpoint). I should probably have stated we use Linked Context ID Match breakpoint for the context-aware breakpoint ? > > > + 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); > > Just a curiosity, can the context-aware one link to this one? No, it can't (LBN field for the Context breakpoint is ignored). > > > + 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 */ > > nit: make this number a bit more unlikely to happen by mistake. > I guess you could use all available 32 bits. Sure, I could change it to some different 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); > > > > + /* 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); > > If brps is "number of breakpoints", then there should be a "+ 1" above. > Otherwise brps is really "last breakpoint" (last_brp). > > > + __TEST_REQUIRE(brps > 0, "At least two breakpoints are required"); > > Yes, based on this test, brps is really "last breakpoint". I would > suggest changing the name to "last_brp" (or something similar). The 'brps' I meant is simply 'BRPS' field value of ID_AA64DFR0_EL1. I agree that it could be misleading. The following patches use xxx_num for the number of watch/break points. So, I am thinking of changing it brp_num to indicate the number of breakpoints (and add 1). > > + > > + /* > > + * Run tests with breakpoint#0 and watchpoint#0, and the higiest > > * Run tests with breakpoint#0, watchpoint#0, and the highest Will fix this. Thank you, Reiji
Hi Ricardo, > > > > -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. Thank you for the comments and explaining the original intention. I will remove that. Thank you, Reiji
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); + /* 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);
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(-)