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

On Thu, Nov 4, 2021 at 8:56 PM Reiji Watanabe [off-list ref] wrote:

Hi Oliver,

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton [off-list ref] wrote:

quoted

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 <redacted>
---
 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.

quoted

+
 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel