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