Re: [PATCH 14/15] KVM: arm64: Don't hijack guest context MDSCR_EL1

[Marc Zyngier <maz@kernel.org>]

On Fri, 08 Nov 2024 22:24:18 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Stealing MDSCR_EL1 in the guest's kvm_cpu_context for external debugging
> is rather gross. Just add a field for this instead and let the context
> switch code pick the correct one based on the debug owner.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h          |  2 +-
>  arch/arm64/kvm/debug.c                     | 45 ++++------------------
>  arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 38 ++++++++++++------
>  3 files changed, 35 insertions(+), 50 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f8c7e37180ab..036c6de5819e 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -751,6 +751,7 @@ struct kvm_vcpu_arch {
>  	 */
>  	struct kvm_guest_debug_arch vcpu_debug_state;
>  	struct kvm_guest_debug_arch external_debug_state;
> +	u64 external_mdscr_el1;
>  
>  	enum {
>  		VCPU_DEBUG_FREE,
> @@ -771,7 +772,6 @@ struct kvm_vcpu_arch {
>  	 * are using guest debug.
>  	 */
>  	struct {
> -		u32	mdscr_el1;
>  		bool	pstate_ss;
>  	} guest_debug_preserved;
>  
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 9ac237c3ae0c..a23d214a0fb0 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -32,19 +32,12 @@
>   */
>  static void save_guest_debug_regs(struct kvm_vcpu *vcpu)
>  {
> -	u64 val = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> -
> -	vcpu->arch.guest_debug_preserved.mdscr_el1 = val;
>  	vcpu->arch.guest_debug_preserved.pstate_ss =
>  					(*vcpu_cpsr(vcpu) & DBG_SPSR_SS);
>  }
>  
>  static void restore_guest_debug_regs(struct kvm_vcpu *vcpu)
>  {
> -	u64 val = vcpu->arch.guest_debug_preserved.mdscr_el1;
> -
> -	vcpu_write_sys_reg(vcpu, val, MDSCR_EL1);
> -
>  	if (vcpu->arch.guest_debug_preserved.pstate_ss)
>  		*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
>  	else
> @@ -149,36 +142,6 @@ void kvm_arm_setup_debug(struct kvm_vcpu *vcpu)
>  				*vcpu_cpsr(vcpu) |= DBG_SPSR_SS;
>  			else
>  				*vcpu_cpsr(vcpu) &= ~DBG_SPSR_SS;
> -
> -			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> -			mdscr |= DBG_MDSCR_SS;
> -			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
> -		} else {
> -			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> -			mdscr &= ~DBG_MDSCR_SS;
> -			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
> -		}
> -
> -		/*
> -		 * Enable breakpoints and watchpoints if userspace wants them.
> -		 */
> -		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW) {
> -			mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> -			mdscr |= DBG_MDSCR_MDE;
> -			vcpu_write_sys_reg(vcpu, mdscr, MDSCR_EL1);
> -
> -		/*
> -		 * The OS Lock blocks debug exceptions in all ELs when it is
> -		 * enabled. If the guest has enabled the OS Lock, constrain its
> -		 * effects to the guest. Emulate the behavior by clearing
> -		 * MDSCR_EL1.MDE. In so doing, we ensure that host debug
> -		 * exceptions are unaffected by guest configuration of the OS
> -		 * Lock.
> -		 */
> -		} 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);
>  		}
>  	}
>  }
> @@ -232,6 +195,14 @@ void kvm_vcpu_load_debug(struct kvm_vcpu *vcpu)
>  	KVM_BUG_ON(vcpu_get_flag(vcpu, SYSREGS_ON_CPU), vcpu->kvm);
>  
>  	if (vcpu->guest_debug || kvm_vcpu_os_lock_enabled(vcpu)) {
> +		mdscr = MDSCR_EL1_TDCC;

Eh... this is new. Why?

> +
> +		if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> +			mdscr |= MDSCR_EL1_SS;
> +		if (vcpu->guest_debug & KVM_GUESTDBG_USE_HW)
> +			mdscr |= MDSCR_EL1_MDE;

I think this would at least deserve a comment that guest_debug
indicates that *the host* is debugging the guest. It is one of the
thing that throws me every time I read this code.

The other thing is that I can't completely convince myself that this
rewrite is strictly equivalent, because we now operate on different
states. For example, I don't see that we effectively block debug
exceptions when the OS Lock is enabled. Where is that handled now?

> +
> +		vcpu->arch.external_mdscr_el1 = mdscr;
>  		vcpu->arch.debug_owner = VCPU_DEBUG_HOST_OWNED;
>  	} else {
>  		mdscr = vcpu_read_sys_reg(vcpu, MDSCR_EL1);
> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> index a998b2f6abcb..49770963cf84 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
> @@ -18,9 +18,33 @@
>  
>  static inline bool ctxt_has_s1poe(struct kvm_cpu_context *ctxt);
>  
> +static inline struct kvm_vcpu *ctxt_to_vcpu(struct kvm_cpu_context *ctxt)
> +{
> +	struct kvm_vcpu *vcpu = ctxt->__hyp_running_vcpu;
> +
> +	if (!vcpu)
> +		vcpu = container_of(ctxt, struct kvm_vcpu, arch.ctxt);
> +
> +	return vcpu;
> +}
> +
> +static inline bool ctxt_is_guest(struct kvm_cpu_context *ctxt)
> +{
> +	return host_data_ptr(host_ctxt) != ctxt;
> +}
> +
> +#define ctxt_mdscr_el1(ctxt)						\
> +({									\
> +	u64 *__p = &ctxt_sys_reg(ctxt, MDSCR_EL1);			\
> +	struct kvm_vcpu *__v = ctxt_to_vcpu(ctxt);			\
> +	if (ctxt_is_guest(ctxt) && kvm_host_owns_debug_regs(__v))	\
> +		__p = &(__v)->arch.external_mdscr_el1;			\
> +	__p;								\
> +})
> +

Anything that prevents this helper from being an actual (inline) function?

>  static inline void __sysreg_save_common_state(struct kvm_cpu_context *ctxt)
>  {
> -	ctxt_sys_reg(ctxt, MDSCR_EL1)	= read_sysreg(mdscr_el1);
> +	*ctxt_mdscr_el1(ctxt)	= read_sysreg(mdscr_el1);
>  
>  	// POR_EL0 can affect uaccess, so must be saved/restored early.
>  	if (ctxt_has_s1poe(ctxt))
> @@ -33,16 +57,6 @@ static inline void __sysreg_save_user_state(struct kvm_cpu_context *ctxt)
>  	ctxt_sys_reg(ctxt, TPIDRRO_EL0)	= read_sysreg(tpidrro_el0);
>  }
>  
> -static inline struct kvm_vcpu *ctxt_to_vcpu(struct kvm_cpu_context *ctxt)
> -{
> -	struct kvm_vcpu *vcpu = ctxt->__hyp_running_vcpu;
> -
> -	if (!vcpu)
> -		vcpu = container_of(ctxt, struct kvm_vcpu, arch.ctxt);
> -
> -	return vcpu;
> -}
> -
>  static inline bool ctxt_has_mte(struct kvm_cpu_context *ctxt)
>  {
>  	struct kvm_vcpu *vcpu = ctxt_to_vcpu(ctxt);
> @@ -139,7 +153,7 @@ static inline void __sysreg_save_el2_return_state(struct kvm_cpu_context *ctxt)
>  
>  static inline void __sysreg_restore_common_state(struct kvm_cpu_context *ctxt)
>  {
> -	write_sysreg(ctxt_sys_reg(ctxt, MDSCR_EL1),  mdscr_el1);
> +	write_sysreg(*ctxt_mdscr_el1(ctxt),  mdscr_el1);
>  
>  	// POR_EL0 can affect uaccess, so must be saved/restored early.
>  	if (ctxt_has_s1poe(ctxt))

Thanks,

	M.

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