| Message ID | 20211102094651.2071532-7-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Emulate the OS lock | expand |
Hi Oliver, On Tue, 02 Nov 2021 09:46:51 +0000, Oliver Upton <oupton@google.com> wrote: > > KVM now correctly handles the OS Lock for its guests. When set, KVM > blocks all debug exceptions originating from the guest. Add test cases > to the debug-exceptions test to assert that software breakpoint, > hardware breakpoint, watchpoint, and single-step exceptions are in fact > blocked. > > Signed-off-by: Oliver Upton <oupton@google.com> > --- > .../selftests/kvm/aarch64/debug-exceptions.c | 58 ++++++++++++++++++- > 1 file changed, 56 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > index e5e6c92b60da..6b6ff81cdd23 100644 > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > @@ -23,7 +23,7 @@ > #define SPSR_D (1 << 9) > #define SPSR_SS (1 << 21) > > -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start; > +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start; > 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; > @@ -47,6 +47,14 @@ static void reset_debug_state(void) > isb(); > } > > +static void enable_os_lock(void) > +{ > + write_sysreg(oslar_el1, 1); > + isb(); > + > + GUEST_ASSERT(read_sysreg(oslsr_el1) & 2); > +} > + > static void install_wp(uint64_t addr) > { > uint32_t wcr; > @@ -99,6 +107,7 @@ static void guest_code(void) > GUEST_SYNC(0); > > /* Software-breakpoint */ > + reset_debug_state(); > asm volatile("sw_bp: brk #0"); > GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp)); > > @@ -152,6 +161,51 @@ static void guest_code(void) > GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4); > GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8); > > + GUEST_SYNC(6); > + > + /* OS Lock blocking software-breakpoint */ > + reset_debug_state(); > + enable_os_lock(); > + sw_bp_addr = 0; > + asm volatile("brk #0"); > + GUEST_ASSERT_EQ(sw_bp_addr, 0); I haven't had a change to properly review the series, but this one definitely caught my eye. My expectations are that BRK is *not* affected by the OS Lock. The ARMv8 ARM goes as far as saying: <quote> Breakpoint Instruction exceptions are enabled regardless of the state of the OS Lock and the OS Double Lock. </quote> as well as: <quote> There is no enable control for Breakpoint Instruction exceptions. They are always enabled, and cannot be masked. </quote> I wonder how your test succeeds, though. Thanks, M.
Hey Marc, On Tue, Nov 2, 2021 at 4:09 AM Marc Zyngier <maz@kernel.org> wrote: > > Hi Oliver, > > On Tue, 02 Nov 2021 09:46:51 +0000, > Oliver Upton <oupton@google.com> wrote: > > > > KVM now correctly handles the OS Lock for its guests. When set, KVM > > blocks all debug exceptions originating from the guest. Add test cases > > to the debug-exceptions test to assert that software breakpoint, > > hardware breakpoint, watchpoint, and single-step exceptions are in fact > > blocked. > > > > Signed-off-by: Oliver Upton <oupton@google.com> > > --- > > .../selftests/kvm/aarch64/debug-exceptions.c | 58 ++++++++++++++++++- > > 1 file changed, 56 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > > index e5e6c92b60da..6b6ff81cdd23 100644 > > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > > @@ -23,7 +23,7 @@ > > #define SPSR_D (1 << 9) > > #define SPSR_SS (1 << 21) > > > > -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start; > > +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start; > > 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; > > @@ -47,6 +47,14 @@ static void reset_debug_state(void) > > isb(); > > } > > > > +static void enable_os_lock(void) > > +{ > > + write_sysreg(oslar_el1, 1); > > + isb(); > > + > > + GUEST_ASSERT(read_sysreg(oslsr_el1) & 2); > > +} > > + > > static void install_wp(uint64_t addr) > > { > > uint32_t wcr; > > @@ -99,6 +107,7 @@ static void guest_code(void) > > GUEST_SYNC(0); > > > > /* Software-breakpoint */ > > + reset_debug_state(); > > asm volatile("sw_bp: brk #0"); > > GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp)); > > > > @@ -152,6 +161,51 @@ static void guest_code(void) > > GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4); > > GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8); > > > > + GUEST_SYNC(6); > > + > > + /* OS Lock blocking software-breakpoint */ > > + reset_debug_state(); > > + enable_os_lock(); > > + sw_bp_addr = 0; > > + asm volatile("brk #0"); > > + GUEST_ASSERT_EQ(sw_bp_addr, 0); > > I haven't had a change to properly review the series, but this one > definitely caught my eye. My expectations are that BRK is *not* > affected by the OS Lock. The ARMv8 ARM goes as far as saying: > > <quote> > Breakpoint Instruction exceptions are enabled regardless of the state > of the OS Lock and the OS Double Lock. > </quote> > > as well as: > > <quote> > There is no enable control for Breakpoint Instruction exceptions. They > are always enabled, and cannot be masked. > </quote> /facepalm I had thought I read "Breakpoint Instruction exceptions" in the list on D2.5 "The effect of powerdown on debug exceptions", although on second read I most definitely did not. And if I had read the bottom of the section, I'd of seen one of the quotes. > I wonder how your test succeeds, though. Probably because the expectations I wrote match the non-architected behavior I implemented :-) -- Thanks, Oliver
On Tue, Nov 2, 2021 at 7:53 AM Oliver Upton <oupton@google.com> wrote: > > > > I haven't had a change to properly review the series, but this one > > definitely caught my eye. My expectations are that BRK is *not* > > affected by the OS Lock. The ARMv8 ARM goes as far as saying: > > > > <quote> > > Breakpoint Instruction exceptions are enabled regardless of the state > > of the OS Lock and the OS Double Lock. > > </quote> > > > > as well as: > > > > <quote> > > There is no enable control for Breakpoint Instruction exceptions. They > > are always enabled, and cannot be masked. > > </quote> > > /facepalm I had thought I read "Breakpoint Instruction exceptions" in > the list on D2.5 "The effect of powerdown on debug exceptions", > although on second read I most definitely did not. And if I had read > the bottom of the section, I'd of seen one of the quotes. > > > I wonder how your test succeeds, though. > > Probably because the expectations I wrote match the non-architected > behavior I implemented :-) Alright, gave the series a good once over after this and fixed up quite a few things. Unless you're ready for it, I'll hold back for a bit to avoid spamming inboxes. As an FYI, here's the fixes I have queued up: v2 -> v3: - Stop trapping debug exceptions when the OS Lock is enabled, as it does *not* block software breakpoint exceptions (Marc) - Trap accesses to debug registers if the OS Lock is enabled to prevent the guest from wiping out KVM's configuration of MDSCR_EL1 - Update the debug-exceptions test to expect a software breakpoint exception even when the OS Lock is enabled. -- Thanks, Oliver
On Tue, Nov 02, 2021 at 09:46:51AM +0000, Oliver Upton wrote: > KVM now correctly handles the OS Lock for its guests. When set, KVM > blocks all debug exceptions originating from the guest. Add test cases > to the debug-exceptions test to assert that software breakpoint, > hardware breakpoint, watchpoint, and single-step exceptions are in fact > blocked. > > Signed-off-by: Oliver Upton <oupton@google.com> > --- > .../selftests/kvm/aarch64/debug-exceptions.c | 58 ++++++++++++++++++- > 1 file changed, 56 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > index e5e6c92b60da..6b6ff81cdd23 100644 > --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c > @@ -23,7 +23,7 @@ > #define SPSR_D (1 << 9) > #define SPSR_SS (1 << 21) > > -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start; > +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start; > 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; > @@ -47,6 +47,14 @@ static void reset_debug_state(void) > isb(); > } > > +static void enable_os_lock(void) > +{ > + write_sysreg(oslar_el1, 1); should be: write_sysreg(val, reg); > + isb(); > + > + GUEST_ASSERT(read_sysreg(oslsr_el1) & 2); > +} > + > static void install_wp(uint64_t addr) > { > uint32_t wcr; > @@ -99,6 +107,7 @@ static void guest_code(void) > GUEST_SYNC(0); > > /* Software-breakpoint */ > + reset_debug_state(); > asm volatile("sw_bp: brk #0"); > GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp)); > > @@ -152,6 +161,51 @@ static void guest_code(void) > GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4); > GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8); > > + GUEST_SYNC(6); > + > + /* OS Lock blocking software-breakpoint */ > + reset_debug_state(); > + enable_os_lock(); > + sw_bp_addr = 0; > + asm volatile("brk #0"); > + GUEST_ASSERT_EQ(sw_bp_addr, 0); > + > + GUEST_SYNC(7); > + > + /* OS Lock blocking hardware-breakpoint */ > + reset_debug_state(); > + enable_os_lock(); > + install_hw_bp(PC(hw_bp2)); > + hw_bp_addr = 0; > + asm volatile("hw_bp2: nop"); > + GUEST_ASSERT_EQ(hw_bp_addr, 0); > + > + GUEST_SYNC(8); > + > + /* OS Lock blocking watchpoint */ > + reset_debug_state(); > + enable_os_lock(); > + write_data = '\0'; > + wp_data_addr = 0; > + install_wp(PC(write_data)); > + write_data = 'x'; > + GUEST_ASSERT_EQ(write_data, 'x'); > + GUEST_ASSERT_EQ(wp_data_addr, 0); > + > + GUEST_SYNC(9); > + > + /* OS Lock blocking single-step */ > + reset_debug_state(); > + enable_os_lock(); > + ss_addr[0] = 0; > + install_ss(); > + ss_idx = 0; > + asm volatile("mrs x0, esr_el1\n\t" > + "add x0, x0, #1\n\t" > + "msr daifset, #8\n\t" > + : : : "x0"); > + GUEST_ASSERT_EQ(ss_addr[0], 0); > + > GUEST_DONE(); > } > > @@ -223,7 +277,7 @@ int main(int argc, char *argv[]) > vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT, > ESR_EC_SVC64, guest_svc_handler); > > - for (stage = 0; stage < 7; stage++) { > + for (stage = 0; stage < 11; stage++) { > vcpu_run(vm, VCPU_ID); > > switch (get_ucall(vm, VCPU_ID, &uc)) { > -- > 2.33.1.1089.g2158813163f-goog >
Hi Ricardo, On Tue, Nov 2, 2021 at 4:27 PM Ricardo Koller <ricarkol@google.com> wrote: > > +static void enable_os_lock(void) > > +{ > > + write_sysreg(oslar_el1, 1); > > should be: write_sysreg(val, reg); Yep, I'll do this once I rebase onto 5.16, as the sysreg rework isn't available til then. -- Thanks, Oliver
diff --git a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c index e5e6c92b60da..6b6ff81cdd23 100644 --- a/tools/testing/selftests/kvm/aarch64/debug-exceptions.c +++ b/tools/testing/selftests/kvm/aarch64/debug-exceptions.c @@ -23,7 +23,7 @@ #define SPSR_D (1 << 9) #define SPSR_SS (1 << 21) -extern unsigned char sw_bp, hw_bp, bp_svc, bp_brk, hw_wp, ss_start; +extern unsigned char sw_bp, hw_bp, hw_bp2, bp_svc, bp_brk, hw_wp, ss_start; 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; @@ -47,6 +47,14 @@ static void reset_debug_state(void) isb(); } +static void enable_os_lock(void) +{ + write_sysreg(oslar_el1, 1); + isb(); + + GUEST_ASSERT(read_sysreg(oslsr_el1) & 2); +} + static void install_wp(uint64_t addr) { uint32_t wcr; @@ -99,6 +107,7 @@ static void guest_code(void) GUEST_SYNC(0); /* Software-breakpoint */ + reset_debug_state(); asm volatile("sw_bp: brk #0"); GUEST_ASSERT_EQ(sw_bp_addr, PC(sw_bp)); @@ -152,6 +161,51 @@ static void guest_code(void) GUEST_ASSERT_EQ(ss_addr[1], PC(ss_start) + 4); GUEST_ASSERT_EQ(ss_addr[2], PC(ss_start) + 8); + GUEST_SYNC(6); + + /* OS Lock blocking software-breakpoint */ + reset_debug_state(); + enable_os_lock(); + sw_bp_addr = 0; + asm volatile("brk #0"); + GUEST_ASSERT_EQ(sw_bp_addr, 0); + + GUEST_SYNC(7); + + /* OS Lock blocking hardware-breakpoint */ + reset_debug_state(); + enable_os_lock(); + install_hw_bp(PC(hw_bp2)); + hw_bp_addr = 0; + asm volatile("hw_bp2: nop"); + GUEST_ASSERT_EQ(hw_bp_addr, 0); + + GUEST_SYNC(8); + + /* OS Lock blocking watchpoint */ + reset_debug_state(); + enable_os_lock(); + write_data = '\0'; + wp_data_addr = 0; + install_wp(PC(write_data)); + write_data = 'x'; + GUEST_ASSERT_EQ(write_data, 'x'); + GUEST_ASSERT_EQ(wp_data_addr, 0); + + GUEST_SYNC(9); + + /* OS Lock blocking single-step */ + reset_debug_state(); + enable_os_lock(); + ss_addr[0] = 0; + install_ss(); + ss_idx = 0; + asm volatile("mrs x0, esr_el1\n\t" + "add x0, x0, #1\n\t" + "msr daifset, #8\n\t" + : : : "x0"); + GUEST_ASSERT_EQ(ss_addr[0], 0); + GUEST_DONE(); } @@ -223,7 +277,7 @@ int main(int argc, char *argv[]) vm_install_sync_handler(vm, VECTOR_SYNC_CURRENT, ESR_EC_SVC64, guest_svc_handler); - for (stage = 0; stage < 7; stage++) { + for (stage = 0; stage < 11; stage++) { vcpu_run(vm, VCPU_ID); switch (get_ucall(vm, VCPU_ID, &uc)) {
KVM now correctly handles the OS Lock for its guests. When set, KVM blocks all debug exceptions originating from the guest. Add test cases to the debug-exceptions test to assert that software breakpoint, hardware breakpoint, watchpoint, and single-step exceptions are in fact blocked. Signed-off-by: Oliver Upton <oupton@google.com> --- .../selftests/kvm/aarch64/debug-exceptions.c | 58 ++++++++++++++++++- 1 file changed, 56 insertions(+), 2 deletions(-)