Re: [PATCH v4 1/4] KVM: TDX: Synchronize user-return MSRs immediately after VP.ENTER

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

+Adrian for TDX arch MSR clobbering details

On Thu, 2025-10-16 at 15:28 -0700, Sean Christopherson wrote:
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 326db9b9c567..2f3dfe9804b5 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -780,6 +780,14 @@ void tdx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
>  		vt->msr_host_kernel_gs_base = read_msr(MSR_KERNEL_GS_BASE);
>  
>  	vt->guest_state_loaded = true;
> +
> +	/*
> +	 * Several of KVM's user-return MSRs are clobbered by the TDX-Module if
> +	 * VP.ENTER succeeds, i.e. on TD-Exit.  Mark those MSRs as needing an
> +	 * update to synchronize the "current" value in KVM's cache with the
> +	 * value in hardware (loaded by the TDX-Module).
> +	 */

I think we should be synchronizing only after a successful VP.ENTER with a real
TD exit, but today instead we synchronize after any attempt to VP.ENTER. Or more
accurately, we plan to synchronize when returning to userspace in that case.

It looks to me that if we get some VP.ENTER errors, the registers should not get
clobbered (although I'd love a second assessment on this from other TDX devs).
Then we actually desync the registers with tdx_user_return_msr_update_cache().

I mention because I think this change widens the issue. For the
TDX_OPERAND_BUSY, etc cases the issue is mostly accidentally avoided, by re-
entering the TD before returning to userspace and doing the sync.

> +	to_tdx(vcpu)->need_user_return_msr_sync = true;
>  }
>  
>  struct tdx_uret_msr {
> @@ -807,7 +815,6 @@ static void tdx_user_return_msr_update_cache(void)
>  static void tdx_prepare_switch_to_host(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_vt *vt = to_vt(vcpu);
> -	struct vcpu_tdx *tdx = to_tdx(vcpu);
>  
>  	if (!vt->guest_state_loaded)
>  		return;
> @@ -815,11 +822,6 @@ static void tdx_prepare_switch_to_host(struct kvm_vcpu *vcpu)
>  	++vcpu->stat.host_state_reload;
>  	wrmsrl(MSR_KERNEL_GS_BASE, vt->msr_host_kernel_gs_base);
>  
> -	if (tdx->guest_entered) {
> -		tdx_user_return_msr_update_cache();
> -		tdx->guest_entered = false;
> -	}
> -
>  	vt->guest_state_loaded = false;
>  }
>  
> @@ -1059,7 +1061,11 @@ fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, u64 run_flags)
>  		update_debugctlmsr(vcpu->arch.host_debugctl);
>  
>  	tdx_load_host_xsave_state(vcpu);
> -	tdx->guest_entered = true;
> +
> +	if (tdx->need_user_return_msr_sync) {

Not sure what the purpose of need_user_return_msr_sync is now that this is moved
here. Before I guess guest_entered was trying to determine if VP.ENTER got
called, but now we know that is the case. So what condition is it avoiding?

But otherwise, as above, we might want to do it depending on the VP.ENTER error
code. Maybe:
if (!(vp_enter_ret & TDX_ERROR))?

> +		tdx_user_return_msr_update_cache();
> +		tdx->need_user_return_msr_sync = false;
> +	}
>  
>  	vcpu->arch.regs_avail &= TDX_REGS_AVAIL_SET;
>  
> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> index ca39a9391db1..9434a6371d67 100644
> --- a/arch/x86/kvm/vmx/tdx.h
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -67,7 +67,7 @@ struct vcpu_tdx {
>  	u64 vp_enter_ret;
>  
>  	enum vcpu_tdx_state state;
> -	bool guest_entered;
> +	bool need_user_return_msr_sync;
>  
>  	u64 map_gpa_next;
>  	u64 map_gpa_end;