Re: [PATCH v3 12/16] KVM: arm64: Compute MDCR_EL2 at vcpu_load()

[Marc Zyngier <maz@kernel.org>]

On Mon, 09 Dec 2024 18:09:22 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> KVM has picked up several hacks to cope with vcpu->arch.mdcr_el2 needing
> to be prepared before vcpu_load(), which is when it gets programmed
> into hardware on VHE.
> 
> Now that the flows for reprogramming MDCR_EL2 have been simplified, move
> that computation to vcpu_load().
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 -
>  arch/arm64/kvm/arm.c              |  2 --
>  arch/arm64/kvm/debug.c            | 30 +++++++++---------------------
>  3 files changed, 9 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d61b8ed32a21..c630d59e7f4c 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1341,7 +1341,6 @@ static inline bool kvm_system_needs_idmapped_vectors(void)
>  static inline void kvm_arch_sync_events(struct kvm *kvm) {}
>  
>  void kvm_init_host_debug_data(void);
> -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_vcpu_load_debug(struct kvm_vcpu *vcpu);
> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index 44a6093b0d9e..ec581aeb41f9 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -804,8 +804,6 @@ int kvm_arch_vcpu_run_pid_change(struct kvm_vcpu *vcpu)
>  
>  	kvm_init_mpidr_data(kvm);
>  
> -	kvm_arm_vcpu_init_debug(vcpu);
> -
>  	if (likely(irqchip_in_kernel(kvm))) {
>  		/*
>  		 * Map the VGIC hardware resources before running a vcpu the
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 91f484a566e4..eb645ede2053 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -90,20 +90,6 @@ static void kvm_arm_setup_mdcr_el2(struct kvm_vcpu *vcpu)
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  }
>  
> -/**
> - * kvm_arm_vcpu_init_debug - setup vcpu debug traps
> - *
> - * @vcpu:	the vcpu pointer
> - *
> - * Set vcpu initial mdcr_el2 value.
> - */
> -void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu)
> -{
> -	preempt_disable();
> -	kvm_arm_setup_mdcr_el2(vcpu);
> -	preempt_enable();
> -}
> -
>  /**
>   * kvm_arm_setup_debug - set up debug related stuff
>   *
> @@ -121,12 +107,10 @@ void kvm_arm_vcpu_init_debug(struct kvm_vcpu *vcpu)
>  
>  void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  {
> -	unsigned long mdscr, orig_mdcr_el2 = vcpu->arch.mdcr_el2;
> +	unsigned long mdscr;
>  
>  	trace_kvm_arm_setup_debug(vcpu, vcpu->guest_debug);
>  
> -	kvm_arm_setup_mdcr_el2(vcpu);
> -
>  	/* Check if we need to use the debug registers. */
>  	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
>  		/* Save guest debug state */
> @@ -196,10 +180,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
>  		}
>  	}
> -
> -	/* Write mdcr_el2 changes since vcpu_load on VHE systems */
> -	if (has_vhe() && orig_mdcr_el2 != vcpu->arch.mdcr_el2)
> -		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
>  }
>  
>  void kvm_arm_clear_debug(struct kvm_vcpu *vcpu)
> @@ -273,6 +253,8 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
>  		else
>  			vcpu->arch.debug_owner = VCPU_DEBUG_FREE;
>  	}
> +
> +	kvm_arm_setup_mdcr_el2(vcpu);
>  }
>  
>  /*
> @@ -288,6 +270,12 @@ void kvm_debug_set_guest_ownership(struct kvm_vcpu *vcpu)
>  
>  	WARN_ON_ONCE(vcpu->arch.debug_owner == VCPU_DEBUG_GUEST_OWNED);
>  	vcpu->arch.debug_owner = VCPU_DEBUG_GUEST_OWNED;
> +
> +	preempt_disable();
> +	kvm_arm_setup_mdcr_el2(vcpu);
> +	if (has_vhe())
> +		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);
> +	preempt_enable();

I'm not confident that the preemption_disable() does anything.
Specially given that it doesn't include setting the ownership as part
of the critical section. So if that works at it is now, then the
critical section is pointless.

I also find it rather fragile that we have multiple places where we
write MDCR_EL2, and I would love this to go for good. Any idea?

	M.

-- 
Without deviation from the norm, progress is not possible.