Re: [PATCH 08/15] KVM: arm64: Select debug state to save/restore based on debug owner

[Marc Zyngier <maz@kernel.org>]

On Fri, 08 Nov 2024 22:24:12 +0000,
Oliver Upton <oliver.upton@linux.dev> wrote:
> 
> Select the set of debug registers to use based on the owner rather than
> relying on debug_ptr.
> 
> Signed-off-by: Oliver Upton <oliver.upton@linux.dev>
> ---
>  arch/arm64/include/asm/kvm_host.h         | 21 +++++++++++++++++++++
>  arch/arm64/kvm/hyp/include/hyp/debug-sr.h |  8 ++++----
>  2 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index d76e4e44b65b..2d9aee66d5d4 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -1357,9 +1357,30 @@ void kvm_handle_debug_access(struct kvm_vcpu *vcpu);
>  #define kvm_vcpu_os_lock_enabled(vcpu)		\
>  	(!!(__vcpu_sys_reg(vcpu, OSLSR_EL1) & OSLSR_EL1_OSLK))
>  
> +#define kvm_debug_regs_in_use(vcpu)		\
> +	((vcpu)->arch.debug_owner != VCPU_DEBUG_FREE)
>  #define kvm_host_owns_debug_regs(vcpu)		\
>  	((vcpu)->arch.debug_owner == VCPU_DEBUG_HOST_OWNED)
>  
> +#define vcpu_debug_regs(vcpu)					\
> +({								\
> +	struct kvm_guest_debug_arch *__d;			\
> +								\
> +	switch ((vcpu)->arch.debug_owner) {			\
> +	case VCPU_DEBUG_FREE:					\
> +		WARN_ON_ONCE(1);				\
> +		fallthrough;					\
> +	case VCPU_DEBUG_GUEST_OWNED:				\
> +		__d = &(vcpu)->arch.vcpu_debug_state;		\
> +		break;						\
> +	case VCPU_DEBUG_HOST_OWNED:				\
> +		__d = &(vcpu)->arch.external_debug_state;	\
> +		break;						\
> +	}							\
> +								\
> +	__d;							\
> +})

Any particular reason why this can't be an actual function? Or at
least a function taking a kvm_ vcpu_arch as a parameter, and a
preprocessor wrapper for the vcpu->arch pointer chasing?

> +
>  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/hyp/include/hyp/debug-sr.h b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> index d00093699aaf..acc47f77b3d0 100644
> --- a/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> +++ b/arch/arm64/kvm/hyp/include/hyp/debug-sr.h
> @@ -132,13 +132,13 @@ static inline void __debug_switch_to_guest_common(struct kvm_vcpu *vcpu)
>  	struct kvm_guest_debug_arch *host_dbg;
>  	struct kvm_guest_debug_arch *guest_dbg;
>  
> -	if (!vcpu_get_flag(vcpu, DEBUG_DIRTY))
> +	if (!kvm_debug_regs_in_use(vcpu))
>  		return;
>  
>  	host_ctxt = host_data_ptr(host_ctxt);
>  	guest_ctxt = &vcpu->arch.ctxt;
>  	host_dbg = host_data_ptr(host_debug_state.regs);
> -	guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
> +	guest_dbg = vcpu_debug_regs(vcpu);

One thing you may want to note in the commit log is that this also
allows us to drop a couple of kern_hyp_va() instances, which is always
a good thing.

>  
>  	__debug_save_state(host_dbg, host_ctxt);
>  	__debug_restore_state(guest_dbg, guest_ctxt);
> @@ -151,13 +151,13 @@ static inline void __debug_switch_to_host_common(struct kvm_vcpu *vcpu)
>  	struct kvm_guest_debug_arch *host_dbg;
>  	struct kvm_guest_debug_arch *guest_dbg;
>  
> -	if (!vcpu_get_flag(vcpu, DEBUG_DIRTY))
> +	if (!kvm_debug_regs_in_use(vcpu))
>  		return;
>  
>  	host_ctxt = host_data_ptr(host_ctxt);
>  	guest_ctxt = &vcpu->arch.ctxt;
>  	host_dbg = host_data_ptr(host_debug_state.regs);
> -	guest_dbg = kern_hyp_va(vcpu->arch.debug_ptr);
> +	guest_dbg = vcpu_debug_regs(vcpu);
>  
>  	__debug_save_state(guest_dbg, guest_ctxt);
>  	__debug_restore_state(host_dbg, host_ctxt);

Thanks,

	M.

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