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

From: Reiji Watanabe <reijiw@google.com>
To: Oliver Upton <oupton@google.com>
Cc: kvmarm@lists.cs.columbia.edu, kvm@vger.kernel.org,
	Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	Andrew Jones <drjones@redhat.com>,
	Peter Shier <pshier@google.com>,
	Ricardo Koller <ricarkol@google.com>
Subject: Re: [PATCH v2 4/6] KVM: arm64: Emulate the OS Lock
Date: Thu, 4 Nov 2021 20:56:18 -0700	[thread overview]
Message-ID: <CAAeT=FwKJLaxNU+2BGWZh=HdTY=NWBzGdN=cTDPKv3x6cG2UsA@mail.gmail.com> (raw)
In-Reply-To: <20211102094651.2071532-5-oupton@google.com>

Hi Oliver,

On Tue, Nov 2, 2021 at 2:47 AM Oliver Upton <oupton@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@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 :)

> +
>  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
>