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 :)
> +
> 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)
Thanks,
Reiji
> /* Route all software debug exceptions to EL2 */
> vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>
> @@ -160,8 +163,11 @@ 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 the guest is being debugged or if the guest has enabled the
> + * OS lock.
> + */
> + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
> /* Save guest debug state */
> save_guest_debug_regs(vcpu);
>
> @@ -223,6 +229,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 +254,7 @@ void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
> {
> trace_kvm_arm_clear_debug(vcpu->guest_debug);
>
> - if (vcpu->guest_debug) {
> + if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
> restore_guest_debug_regs(vcpu);
>
> /*
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 275a27368a04..a7136888434d 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -119,6 +119,14 @@ static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu)
> {
> struct kvm_run *run = vcpu->run;
> u32 esr = kvm_vcpu_get_esr(vcpu);
> + u8 esr_ec = ESR_ELx_EC(esr);
> +
> + if (!vcpu->guest_debug) {
> + WARN_ONCE(esr_ec != ESR_ELx_EC_BRK64 || esr_ec != ESR_ELx_EC_BKPT32,
> + "Unexpected debug exception\n");
> + kvm_incr_pc(vcpu);
> + return 1;
> + }
>
> run->exit_reason = KVM_EXIT_DEBUG;
> run->debug.arch.hsr = esr;
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index acd8aa2e5a44..d336e4c66870 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -1446,9 +1446,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 },
> --
> 2.33.1.1089.g2158813163f-goog
>
