Re: [PATCH 13/15] KVM: arm64: Compute MDCR_EL2 at vcpu_load()

[Marc Zyngier <maz@kernel.org>]

On Fri, 08 Nov 2024 22:24:17 +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            | 28 +++++++---------------------
>  3 files changed, 7 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 91341fb3405d..f8c7e37180ab 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 04a62660ca5b..9ac237c3ae0c 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -91,20 +91,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
>   *
> @@ -122,12 +108,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 */
> @@ -197,10 +181,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)
> @@ -265,6 +245,8 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
>  		else
>  			vcpu->arch.debug_owner = VCPU_DEBUG_FREE;
>  	}
> +
> +	kvm_arm_setup_mdcr_el2(vcpu);
>  }
>  
>  void kvm_handle_debug_access(struct kvm_vcpu *vcpu)
> @@ -274,6 +256,10 @@ void kvm_handle_debug_access(struct kvm_vcpu *vcpu)
>  
>  	WARN_ON_ONCE(vcpu->arch.debug_owner == VCPU_DEBUG_GUEST_OWNED);
>  	vcpu->arch.debug_owner = VCPU_DEBUG_GUEST_OWNED;
> +	kvm_arm_setup_mdcr_el2(vcpu);
> +
> +	if (has_vhe())
> +		write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2);

OK, now I see this function does a bit more than just ownership. Oh
well.

But it now also appears that vcpu->arch.mdcr_el2 isn't part of the
vcpu state. It really is host state that we manage for the sake of
emulation.

How about kicking it where it belongs? It would definitely help
understanding the lifetime of this piece of state.

Thanks,

	M.

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