| Message ID | 20211123210109.1605642-5-oupton@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Emulate the OS lock | expand |
On Tue, 23 Nov 2021 21:01:07 +0000, 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. Skipping breakpoint instructions? I don't think you can do that, as the guest does rely on BRK always being effective. I also don't see where you do that... > > Signed-off-by: Oliver Upton <oupton@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 4 ++++ > arch/arm64/kvm/debug.c | 27 +++++++++++++++++++++++---- > arch/arm64/kvm/sys_regs.c | 6 +++--- > 3 files changed, 30 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 53fc8a6eaf1c..e5a06ff1cba6 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -726,6 +726,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..7835c76347ce 100644 > --- a/arch/arm64/kvm/debug.c > +++ b/arch/arm64/kvm/debug.c > @@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu) > vcpu_read_sys_reg(vcpu, MDSCR_EL1)); > } > > +/* > + * Returns true if the host needs to use the debug registers. > + */ > +static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu) > +{ > + return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu); Just the name of the function has sent my head spinning. Even if the *effects* of the host debug and the OS Lock are vaguely similar from the guest PoV, they really are different things, and I'd rather not lob them together. > +} > + > /** > * kvm_arm_init_debug - grab what we need for debug > * > @@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu) > * - Userspace is using the hardware to debug the guest > * (KVM_GUESTDBG_USE_HW is set). > * - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear). > + * - The guest has enabled the OS Lock (debug exceptions are blocked). > */ > if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) || > - !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)) > + !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) || > + kvm_vcpu_os_lock_enabled(vcpu)) > vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; > > trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2); > @@ -160,8 +170,10 @@ 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 we need to use the debug registers. > + */ > + if (host_using_debug_regs(vcpu)) { I'd rather you expand the helper here and add the comment you have in the commit message explaining the machine-wide effect of the OS Lock. > /* Save guest debug state */ > save_guest_debug_regs(vcpu); > > @@ -223,6 +235,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 +260,10 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) > { > trace_kvm_arm_clear_debug(vcpu->guest_debug); > > - if (vcpu->guest_debug) { > + /* > + * Restore the guest's debug registers if we were using them. > + */ > + if (host_using_debug_regs(vcpu)) { > restore_guest_debug_regs(vcpu); > > /* > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 5dbdb45d6d44..1346906f5c46 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -1453,9 +1453,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 }, Thanks, M.
Hey Marc, Apologies for my delay in getting back to you, I was OOO for a while. On Mon, Nov 29, 2021 at 8:16 AM Marc Zyngier <maz@kernel.org> wrote: > > On Tue, 23 Nov 2021 21:01:07 +0000, > 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. > > Skipping breakpoint instructions? I don't think you can do that, as > the guest does rely on BRK always being effective. I also don't see > where you do that... Right, this comment in the commit message is stale. In the previous iteration I had done this, but removed it per your suggestion. I'll fix the msg in the next round. > > > > Signed-off-by: Oliver Upton <oupton@google.com> > > --- > > arch/arm64/include/asm/kvm_host.h | 4 ++++ > > arch/arm64/kvm/debug.c | 27 +++++++++++++++++++++++---- > > arch/arm64/kvm/sys_regs.c | 6 +++--- > > 3 files changed, 30 insertions(+), 7 deletions(-) > > > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index 53fc8a6eaf1c..e5a06ff1cba6 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h > > @@ -726,6 +726,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..7835c76347ce 100644 > > --- a/arch/arm64/kvm/debug.c > > +++ b/arch/arm64/kvm/debug.c > > @@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu) > > vcpu_read_sys_reg(vcpu, MDSCR_EL1)); > > } > > > > +/* > > + * Returns true if the host needs to use the debug registers. > > + */ > > +static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu) > > +{ > > + return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu); > > Just the name of the function has sent my head spinning. Even if the > *effects* of the host debug and the OS Lock are vaguely similar from > the guest PoV, they really are different things, and I'd rather not > lob them together. > > > +} > > + > > /** > > * kvm_arm_init_debug - grab what we need for debug > > * > > @@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu) > > * - Userspace is using the hardware to debug the guest > > * (KVM_GUESTDBG_USE_HW is set). > > * - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear). > > + * - The guest has enabled the OS Lock (debug exceptions are blocked). > > */ > > if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) || > > - !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)) > > + !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) || > > + kvm_vcpu_os_lock_enabled(vcpu)) > > vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; > > > > trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2); > > @@ -160,8 +170,10 @@ 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 we need to use the debug registers. > > + */ > > + if (host_using_debug_regs(vcpu)) { > > I'd rather you expand the helper here and add the comment you have in > the commit message explaining the machine-wide effect of the OS Lock. Ack to here and the above comment. Thanks for the review! -- Oliver
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 53fc8a6eaf1c..e5a06ff1cba6 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -726,6 +726,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..7835c76347ce 100644 --- a/arch/arm64/kvm/debug.c +++ b/arch/arm64/kvm/debug.c @@ -53,6 +53,14 @@ static void restore_guest_debug_regs(struct kvm_vcpu *vcpu) vcpu_read_sys_reg(vcpu, MDSCR_EL1)); } +/* + * Returns true if the host needs to use the debug registers. + */ +static inline bool host_using_debug_regs(struct kvm_vcpu *vcpu) +{ + return vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu); +} + /** * kvm_arm_init_debug - grab what we need for debug * @@ -105,9 +113,11 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu) * - Userspace is using the hardware to debug the guest * (KVM_GUESTDBG_USE_HW is set). * - The guest is not using debug (KVM_ARM64_DEBUG_DIRTY is clear). + * - The guest has enabled the OS Lock (debug exceptions are blocked). */ if ((vcpu->guest_debug & KVM_GUESTDBG_USE_HW) || - !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)) + !(vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY) || + kvm_vcpu_os_lock_enabled(vcpu)) vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA; trace_kvm_arm_set_dreg32("MDCR_EL2", vcpu->arch.mdcr_el2); @@ -160,8 +170,10 @@ 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 we need to use the debug registers. + */ + if (host_using_debug_regs(vcpu)) { /* Save guest debug state */ save_guest_debug_regs(vcpu); @@ -223,6 +235,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 +260,10 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu) { trace_kvm_arm_clear_debug(vcpu->guest_debug); - if (vcpu->guest_debug) { + /* + * Restore the guest's debug registers if we were using them. + */ + if (host_using_debug_regs(vcpu)) { restore_guest_debug_regs(vcpu); /* diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 5dbdb45d6d44..1346906f5c46 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -1453,9 +1453,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 | 27 +++++++++++++++++++++++---- arch/arm64/kvm/sys_regs.c | 6 +++--- 3 files changed, 30 insertions(+), 7 deletions(-)