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@xxxxxxxxxx> wrote:
>
> On Tue, 23 Nov 2021 21:01:07 +0000,
> Oliver Upton <oupton@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
