[PATCH v2 4/6] KVM: arm64: Emulate the OS Lock

[Oliver Upton]

On Thu, Nov 4, 2021 at 8:56 PM Reiji Watanabe <reijiw at google.com> wrote:
>
> Hi Oliver,
>
> On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton at 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 at 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