| Message ID | 20211102094651.2071532-5-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Emulate the OS lock | expand |
On Tue, Nov 02, 2021 at 09:46:49AM +0000, Oliver Upton wrote: > The OS lock blocks all debug exceptions at every EL. To date, KVM has > not implemented the OS lock for its guests, despite the fact that it is > mandatory per the architecture. Simple context switching between the > guest and host is not appropriate, as its effects are not constrained to > the guest context. > > Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby > blocking all but software breakpoint instructions. To handle breakpoint > instructions, trap debug exceptions to EL2 and skip the instruction. > > Signed-off-by: Oliver Upton <oupton@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 4 ++++ > arch/arm64/kvm/debug.c | 20 +++++++++++++++----- > arch/arm64/kvm/handle_exit.c | 8 ++++++++ > arch/arm64/kvm/sys_regs.c | 6 +++--- > 4 files changed, 30 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index c98f65c4a1f7..f13b8b79b06d 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu); > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); > + > +#define kvm_vcpu_os_lock_enabled(vcpu) \ > + (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK) > + > int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > struct kvm_device_attr *attr); > int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index db9361338b2a..5690a9c99c89 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu) > MDCR_EL2_TDRA | > MDCR_EL2_TDOSA); > > - /* Is the VM being debugged by userspace? */ > - if (vcpu->guest_debug) > + /* > + * Check if the VM is being debugged by userspace or the guest has > + * enabled the OS lock. > + */ > + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) > /* Route all software debug exceptions to EL2 */ > vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE; > > @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > > kvm_arm_setup_mdcr_el2(vcpu); > > - /* Is Guest debugging in effect? */ > - if (vcpu->guest_debug) { > + /* > + * Check if the guest is being debugged or if the guest has enabled the > + * OS lock. > + */ > + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) { > /* Save guest debug state */ > save_guest_debug_regs(vcpu); > > @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > trace_kvm_arm_set_regset("WAPTS", get_num_wrps(), > &vcpu->arch.debug_ptr->dbg_wcr[0], > &vcpu->arch.debug_ptr->dbg_wvr[0]); > + } else if (kvm_vcpu_os_lock_enabled(vcpu)) { > + mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1); > + mdscr &= ~DBG_MDSCR_MDE; > + vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1); I think this is missing the case where the guest is being debugged by userspace _and_ from inside (the guest) at the same time. In this situation, the vmm gets a KVM_EXIT_DEBUG and if it doesn't know what to do with it, it injects the exception into the guest (1). With this "else if", the guest would still get the debug exception when the os lock is enabled. (1) kvm_arm_handle_debug() is the one doing this in QEMU source code. > } > } > > @@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) > { > trace_kvm_arm_clear_debug(vcpu->guest_debug); > > - if (vcpu->guest_debug) { > + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) { > restore_guest_debug_regs(vcpu); > > /* > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index 275a27368a04..a7136888434d 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu) > { > struct kvm_run *run = vcpu->run; > u32 esr = kvm_vcpu_get_esr(vcpu); > + u8 esr_ec = ESR_ELx_EC(esr); > + > + if (!vcpu->guest_debug) { > + WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32, > + "Unexpected debug exception\n"); > + kvm_incr_pc(vcpu); > + return 1; > + } > > run->exit_reason = KVM_EXIT_DEBUG; > run->debug.arch.hsr = esr; > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index acd8aa2e5a44..d336e4c66870 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu, > * Debug handling: We do trap most, if not all debug related system > * registers. The implementation is good enough to ensure that a guest > * can use these with minimal performance degradation. The drawback is > - * that we don't implement any of the external debug, none of the > - * OSlock protocol. This should be revisited if we ever encounter a > - * more demanding guest... > + * that we don't implement any of the external debug architecture. > + * This should be revisited if we ever encounter a more demanding > + * guest... > */ > static const struct sys_reg_desc sys_reg_descs[] = { > { SYS_DESC(SYS_DC_ISW), access_dcsw }, > -- > 2.33.1.1089.g2158813163f-goog >
Hi Ricardo, On Tue, Nov 2, 2021 at 4:45 PM Ricardo Koller <ricarkol@google.com> wrote: > > On Tue, Nov 02, 2021 at 09:46:49AM +0000, Oliver Upton wrote: > > The OS lock blocks all debug exceptions at every EL. To date, KVM has > > not implemented the OS lock for its guests, despite the fact that it is > > mandatory per the architecture. Simple context switching between the > > guest and host is not appropriate, as its effects are not constrained to > > the guest context. > > > > Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby > > blocking all but software breakpoint instructions. To handle breakpoint > > instructions, trap debug exceptions to EL2 and skip the instruction. > > > > Signed-off-by: Oliver Upton <oupton@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 4 ++++ > > arch/arm64/kvm/debug.c | 20 +++++++++++++++----- > > arch/arm64/kvm/handle_exit.c | 8 ++++++++ > > arch/arm64/kvm/sys_regs.c | 6 +++--- > > 4 files changed, 30 insertions(+), 8 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index c98f65c4a1f7..f13b8b79b06d 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu); > > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > > void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > > void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); > > + > > +#define kvm_vcpu_os_lock_enabled(vcpu) \ > > + (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK) > > + > > int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > > struct kvm_device_attr *attr); > > int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > > index db9361338b2a..5690a9c99c89 100644 > > --- a/arch/arm64/kvm/debug.c > > +++ b/arch/arm64/kvm/debug.c > > @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu) > > MDCR_EL2_TDRA | > > MDCR_EL2_TDOSA); > > > > - /* Is the VM being debugged by userspace? */ > > - if (vcpu->guest_debug) > > + /* > > + * Check if the VM is being debugged by userspace or the guest has > > + * enabled the OS lock. > > + */ > > + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) > > /* Route all software debug exceptions to EL2 */ > > vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE; > > > > @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > > > > kvm_arm_setup_mdcr_el2(vcpu); > > > > - /* Is Guest debugging in effect? */ > > - if (vcpu->guest_debug) { > > + /* > > + * Check if the guest is being debugged or if the guest has enabled the > > + * OS lock. > > + */ > > + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) { > > /* Save guest debug state */ > > save_guest_debug_regs(vcpu); > > > > @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > > trace_kvm_arm_set_regset("WAPTS", get_num_wrps(), > > &vcpu->arch.debug_ptr->dbg_wcr[0], > > &vcpu->arch.debug_ptr->dbg_wvr[0]); > > + } else if (kvm_vcpu_os_lock_enabled(vcpu)) { > > + mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1); > > + mdscr &= ~DBG_MDSCR_MDE; > > + vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1); > > I think this is missing the case where the guest is being debugged by > userspace _and_ from inside (the guest) at the same time. In this > situation, the vmm gets a KVM_EXIT_DEBUG and if it doesn't know what to > do with it, it injects the exception into the guest (1). With this "else > if", the guest would still get the debug exception when the os lock is > enabled. > > (1) kvm_arm_handle_debug() is the one doing this in QEMU source code. I wonder if this is a problem that KVM should even handle. KVM doesn't do anything to help userspace inject the debug exception into the guest, and from reading kvm_arm_handle_debug() it would seem that QEMU is manually injecting the exception to EL1 and setting the PC to the appropriate vector. There is an issue, though, with migration: older KVM will not show OSLSR_EL1 on KVM_GET_REG_LIST. However, in order to provide an architectural OS Lock, its reset value must be 1 (enabled). This would all have the effect of discarding the guest's OS lock value and blocking all debug exceptions intended for the guest until the next reboot. So it would seem that userspace needs to know about the OSLK bit to correctly inject debug exceptions and migrate guests. If opt-in is heavyweight, we could cure the migration issue by explicitly documenting the OS lock being disabled out of reset as an erratum of KVM. Doing so would be consistent with all prior versions of KVM. Of course, adopting nonarchitected behavior in perpetuity seems a bit unsavory :-) -- Oliver
Hi Oliver, On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote: > > The OS lock blocks all debug exceptions at every EL. To date, KVM has > not implemented the OS lock for its guests, despite the fact that it is > mandatory per the architecture. Simple context switching between the > guest and host is not appropriate, as its effects are not constrained to > the guest context. > > Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby > blocking all but software breakpoint instructions. To handle breakpoint > instructions, trap debug exceptions to EL2 and skip the instruction. > > Signed-off-by: Oliver Upton <oupton@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 4 ++++ > arch/arm64/kvm/debug.c | 20 +++++++++++++++----- > arch/arm64/kvm/handle_exit.c | 8 ++++++++ > arch/arm64/kvm/sys_regs.c | 6 +++--- > 4 files changed, 30 insertions(+), 8 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index c98f65c4a1f7..f13b8b79b06d 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu); > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); > + > +#define kvm_vcpu_os_lock_enabled(vcpu) \ > + (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK) I would think the name of this macro might sound like it generates a code that is evaluated as bool :) > + > int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > struct kvm_device_attr *attr); > int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > index db9361338b2a..5690a9c99c89 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu) > MDCR_EL2_TDRA | > MDCR_EL2_TDOSA); > > - /* Is the VM being debugged by userspace? */ > - if (vcpu->guest_debug) > + /* > + * Check if the VM is being debugged by userspace or the guest has > + * enabled the OS lock. > + */ > + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) IMHO, it might be nicer to create a macro or function that abstracts the condition that needs save_guest_debug_regs/restore_guest_debug_regs. (rather than putting those conditions in each part of codes where they are needed) Thanks, Reiji > /* Route all software debug exceptions to EL2 */ > vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE; > > @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > > kvm_arm_setup_mdcr_el2(vcpu); > > - /* Is Guest debugging in effect? */ > - if (vcpu->guest_debug) { > + /* > + * Check if the guest is being debugged or if the guest has enabled the > + * OS lock. > + */ > + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) { > /* Save guest debug state */ > save_guest_debug_regs(vcpu); > > @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) > trace_kvm_arm_set_regset("WAPTS", get_num_wrps(), > &vcpu->arch.debug_ptr->dbg_wcr[0], > &vcpu->arch.debug_ptr->dbg_wvr[0]); > + } else if (kvm_vcpu_os_lock_enabled(vcpu)) { > + mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1); > + mdscr &= ~DBG_MDSCR_MDE; > + vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1); > } > } > > @@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) > { > trace_kvm_arm_clear_debug(vcpu->guest_debug); > > - if (vcpu->guest_debug) { > + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) { > restore_guest_debug_regs(vcpu); > > /* > diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c > index 275a27368a04..a7136888434d 100644 > --- a/arch/arm64/kvm/handle_exit.c > +++ b/arch/arm64/kvm/handle_exit.c > @@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu) > { > struct kvm_run *run = vcpu->run; > u32 esr = kvm_vcpu_get_esr(vcpu); > + u8 esr_ec = ESR_ELx_EC(esr); > + > + if (!vcpu->guest_debug) { > + WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32, > + "Unexpected debug exception\n"); > + kvm_incr_pc(vcpu); > + return 1; > + } > > run->exit_reason = KVM_EXIT_DEBUG; > run->debug.arch.hsr = esr; > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index acd8aa2e5a44..d336e4c66870 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu, > * Debug handling: We do trap most, if not all debug related system > * registers. The implementation is good enough to ensure that a guest > * can use these with minimal performance degradation. The drawback is > - * that we don't implement any of the external debug, none of the > - * OSlock protocol. This should be revisited if we ever encounter a > - * more demanding guest... > + * that we don't implement any of the external debug architecture. > + * This should be revisited if we ever encounter a more demanding > + * guest... > */ > static const struct sys_reg_desc sys_reg_descs[] = { > { SYS_DESC(SYS_DC_ISW), access_dcsw }, > -- > 2.33.1.1089.g2158813163f-goog >
On Thu, Nov 4, 2021 at 8:56 PM Reiji Watanabe <reijiw@google.com> wrote: > > Hi Oliver, > > On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@google.com> wrote: > > > > The OS lock blocks all debug exceptions at every EL. To date, KVM has > > not implemented the OS lock for its guests, despite the fact that it is > > mandatory per the architecture. Simple context switching between the > > guest and host is not appropriate, as its effects are not constrained to > > the guest context. > > > > Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby > > blocking all but software breakpoint instructions. To handle breakpoint > > instructions, trap debug exceptions to EL2 and skip the instruction. > > > > Signed-off-by: Oliver Upton <oupton@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 4 ++++ > > arch/arm64/kvm/debug.c | 20 +++++++++++++++----- > > arch/arm64/kvm/handle_exit.c | 8 ++++++++ > > arch/arm64/kvm/sys_regs.c | 6 +++--- > > 4 files changed, 30 insertions(+), 8 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index c98f65c4a1f7..f13b8b79b06d 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu); > > void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); > > void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); > > void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); > > + > > +#define kvm_vcpu_os_lock_enabled(vcpu) \ > > + (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK) > > I would think the name of this macro might sound like it generates > a code that is evaluated as bool :) Hey! Nobody ever said this would coerce the returned value into a bool :-P In all seriousness, good point. I agree that the statement should obviously evaluate to a bool, given the naming of the macro. > > > + > > int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, > > struct kvm_device_attr *attr); > > int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > > diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c > > index db9361338b2a..5690a9c99c89 100644 > > --- a/arch/arm64/kvm/debug.c > > +++ b/arch/arm64/kvm/debug.c > > @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu) > > MDCR_EL2_TDRA | > > MDCR_EL2_TDOSA); > > > > - /* Is the VM being debugged by userspace? */ > > - if (vcpu->guest_debug) > > + /* > > + * Check if the VM is being debugged by userspace or the guest has > > + * enabled the OS lock. > > + */ > > + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) > > IMHO, it might be nicer to create a macro or function that abstracts the > condition that needs save_guest_debug_regs/restore_guest_debug_regs. > (rather than putting those conditions in each part of codes where they > are needed) > I completely agree, and it comes with the added benefit that the macro/function can be named something informative so as to suggest the purpose for saving guest registers. Thanks for the review! -- Oliver
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index c98f65c4a1f7..f13b8b79b06d 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -724,6 +724,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu); void kvm_arm_setup_debug(struct kvm_vcpu *vcpu); void kvm_arm_clear_debug(struct kvm_vcpu *vcpu); void kvm_arm_reset_debug_ptr(struct kvm_vcpu *vcpu); + +#define kvm_vcpu_os_lock_enabled(vcpu) \ + (__vcpu_sys_reg(vcpu, OSLSR_EL1) & SYS_OSLSR_OSLK) + int kvm_arm_vcpu_arch_set_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr); int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c index db9361338b2a..5690a9c99c89 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c @@ -95,8 +95,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu) MDCR_EL2_TDRA | MDCR_EL2_TDOSA); - /* Is the VM being debugged by userspace? */ - if (vcpu->guest_debug) + /* + * Check if the VM is being debugged by userspace or the guest has + * enabled the OS lock. + */ + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) /* Route all software debug exceptions to EL2 */ vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE; @@ -160,8 +163,11 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) kvm_arm_setup_mdcr_el2(vcpu); - /* Is Guest debugging in effect? */ - if (vcpu->guest_debug) { + /* + * Check if the guest is being debugged or if the guest has enabled the + * OS lock. + */ + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) { /* Save guest debug state */ save_guest_debug_regs(vcpu); @@ -223,6 +229,10 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu) trace_kvm_arm_set_regset("WAPTS", get_num_wrps(), &vcpu->arch.debug_ptr->dbg_wcr[0], &vcpu->arch.debug_ptr->dbg_wvr[0]); + } else if (kvm_vcpu_os_lock_enabled(vcpu)) { + mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1); + mdscr &= ~DBG_MDSCR_MDE; + vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1); } } @@ -244,7 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) { trace_kvm_arm_clear_debug(vcpu->guest_debug); - if (vcpu->guest_debug) { + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) { restore_guest_debug_regs(vcpu); /* diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c index 275a27368a04..a7136888434d 100644 --- a/arch/arm64/kvm/handle_exit.c +++ b/arch/arm64/kvm/handle_exit.c @@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu) { struct kvm_run *run = vcpu->run; u32 esr = kvm_vcpu_get_esr(vcpu); + u8 esr_ec = ESR_ELx_EC(esr); + + if (!vcpu->guest_debug) { + WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32, + "Unexpected debug exception\n"); + kvm_incr_pc(vcpu); + return 1; + } run->exit_reason = KVM_EXIT_DEBUG; run->debug.arch.hsr = esr; diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index acd8aa2e5a44..d336e4c66870 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1446,9 +1446,9 @@ static unsigned int mte_visibility(const struct kvm_vcpu *vcpu, * Debug handling: We do trap most, if not all debug related system * registers. The implementation is good enough to ensure that a guest * can use these with minimal performance degradation. The drawback is - * that we don't implement any of the external debug, none of the - * OSlock protocol. This should be revisited if we ever encounter a - * more demanding guest... + * that we don't implement any of the external debug architecture. + * This should be revisited if we ever encounter a more demanding + * guest... */ static const struct sys_reg_desc sys_reg_descs[] = { { SYS_DESC(SYS_DC_ISW), access_dcsw },
The OS lock blocks all debug exceptions at every EL. To date, KVM has not implemented the OS lock for its guests, despite the fact that it is mandatory per the architecture. Simple context switching between the guest and host is not appropriate, as its effects are not constrained to the guest context. Emulate the OS Lock by clearing MDE and SS in MDSCR_EL1, thereby blocking all but software breakpoint instructions. To handle breakpoint instructions, trap debug exceptions to EL2 and skip the instruction. Signed-off-by: Oliver Upton <oupton@google.com> --- arch/arm64/include/asm/kvm_host.h | 4 ++++ arch/arm64/kvm/debug.c | 20 +++++++++++++++----- arch/arm64/kvm/handle_exit.c | 8 ++++++++ arch/arm64/kvm/sys_regs.c | 6 +++--- 4 files changed, 30 insertions(+), 8 deletions(-)