Re: [PATCH v6 8/8] KVM: VMX: Preserve host's DEBUGCTLMSR_FREEZE_IN_SMM while running the guest

[mlevitsk@redhat.com]

On Thu, 2025-06-26 at 09:17 -0700, Sean Christopherson wrote:
> On Tue, Jun 24, 2025, mlevitsk@redhat.com wrote:
> > On Tue, 2025-06-10 at 16:20 -0700, Sean Christopherson wrote:
> > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > > index c20a4185d10a..076af78af151 100644
> > > --- a/arch/x86/kvm/vmx/vmx.h
> > > +++ b/arch/x86/kvm/vmx/vmx.h
> > > @@ -419,12 +419,25 @@ bool vmx_is_valid_debugctl(struct kvm_vcpu *vcpu, u64 data, bool host_initiated)
> > >  
> > >  static inline void vmx_guest_debugctl_write(struct kvm_vcpu *vcpu, u64 val)
> > >  {
> > > +	WARN_ON_ONCE(val & DEBUGCTLMSR_FREEZE_IN_SMM);
> > > +
> > > +	val |= vcpu->arch.host_debugctl & DEBUGCTLMSR_FREEZE_IN_SMM;
> > >  	vmcs_write64(GUEST_IA32_DEBUGCTL, val);
> > >  }
> > >  
> > >  static inline u64 vmx_guest_debugctl_read(void)
> > >  {
> > > -	return vmcs_read64(GUEST_IA32_DEBUGCTL);
> > > +	return vmcs_read64(GUEST_IA32_DEBUGCTL) & ~DEBUGCTLMSR_FREEZE_IN_SMM;
> > > +}
> > > +
> > > +static inline void vmx_reload_guest_debugctl(struct kvm_vcpu *vcpu)
> > > +{
> > > +	u64 val = vmcs_read64(GUEST_IA32_DEBUGCTL);
> > > +
> > > +	if (!((val ^ vcpu->arch.host_debugctl) & DEBUGCTLMSR_FREEZE_IN_SMM))
> > > +		return;
> > > +
> > > +	vmx_guest_debugctl_write(vcpu, val & ~DEBUGCTLMSR_FREEZE_IN_SMM);
> > >  }
> > 
> > 
> > Wouldn't it be better to use kvm_x86_ops.HOST_OWNED_DEBUGCTL here as well
> > to avoid logic duplication?
> 
> Hmm, yeah.  I used DEBUGCTLMSR_FREEZE_IN_SMM directly to avoid a memory load
> just to get at a constant literal.
> 
> What about this?  It doesn't completely dedup the logic, but I think it gets us
> close enough to a single source of truth.
> 
> --
> From: Sean Christopherson <seanjc@google.com>
> Date: Thu, 26 Jun 2025 09:14:20 -0700
> Subject: [PATCH] KVM: VMX: Add a macro to track which DEBUGCTL bits are
>  host-owned
> 
> Add VMX_HOST_OWNED_DEBUGCTL_BITS to track which bits are host-owned, i.e.
> need to be preserved when running the guest, to dedup the logic without
> having to incur a memory load to get at kvm_x86_ops.HOST_OWNED_DEBUGCTL.
> 
> No functional change intended.
> 
> Suggested-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/main.c |  2 +-
>  arch/x86/kvm/vmx/vmx.h  | 12 +++++++-----
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
> index 8c6435fdda18..dbab1c15b0cd 100644
> --- a/arch/x86/kvm/vmx/main.c
> +++ b/arch/x86/kvm/vmx/main.c
> @@ -883,7 +883,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
>  	.vcpu_load = vt_op(vcpu_load),
>  	.vcpu_put = vt_op(vcpu_put),
>  
> -	.HOST_OWNED_DEBUGCTL = DEBUGCTLMSR_FREEZE_IN_SMM,
> +	.HOST_OWNED_DEBUGCTL = VMX_HOST_OWNED_DEBUGCTL_BITS,
>  
>  	.update_exception_bitmap = vt_op(update_exception_bitmap),
>  	.get_feature_msr = vmx_get_feature_msr,
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index 87174d961c85..d3389baf3ab3 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -410,27 +410,29 @@ void vmx_update_cpu_dirty_logging(struct kvm_vcpu *vcpu);
>  u64 vmx_get_supported_debugctl(struct kvm_vcpu *vcpu, bool host_initiated);
>  bool vmx_is_valid_debugctl(struct kvm_vcpu *vcpu, u64 data, bool host_initiated);
>  
> +#define VMX_HOST_OWNED_DEBUGCTL_BITS	(DEBUGCTLMSR_FREEZE_IN_SMM)
> +
>  static inline void vmx_guest_debugctl_write(struct kvm_vcpu *vcpu, u64 val)
>  {
> -	WARN_ON_ONCE(val & DEBUGCTLMSR_FREEZE_IN_SMM);
> +	WARN_ON_ONCE(val & VMX_HOST_OWNED_DEBUGCTL_BITS);
>  
> -	val |= vcpu->arch.host_debugctl & DEBUGCTLMSR_FREEZE_IN_SMM;
> +	val |= vcpu->arch.host_debugctl & VMX_HOST_OWNED_DEBUGCTL_BITS;
>  	vmcs_write64(GUEST_IA32_DEBUGCTL, val);
>  }
>  
>  static inline u64 vmx_guest_debugctl_read(void)
>  {
> -	return vmcs_read64(GUEST_IA32_DEBUGCTL) & ~DEBUGCTLMSR_FREEZE_IN_SMM;
> +	return vmcs_read64(GUEST_IA32_DEBUGCTL) & ~VMX_HOST_OWNED_DEBUGCTL_BITS;
>  }
>  
>  static inline void vmx_reload_guest_debugctl(struct kvm_vcpu *vcpu)
>  {
>  	u64 val = vmcs_read64(GUEST_IA32_DEBUGCTL);
>  
> -	if (!((val ^ vcpu->arch.host_debugctl) & DEBUGCTLMSR_FREEZE_IN_SMM))
> +	if (!((val ^ vcpu->arch.host_debugctl) & VMX_HOST_OWNED_DEBUGCTL_BITS))
>  		return;
>  
> -	vmx_guest_debugctl_write(vcpu, val & ~DEBUGCTLMSR_FREEZE_IN_SMM);
> +	vmx_guest_debugctl_write(vcpu, val & ~VMX_HOST_OWNED_DEBUGCTL_BITS);
>  }
>  
>  /*
> 
> base-commit: 6c7ecd725e503bf2ca69ff52c6cc48bb650b1f11
> --
> 

This looks reasonable.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>


Best regards,
	Maxim Levitsky