Re: [PATCH v5 1/4] KVM: TDX: Explicitly set user-return MSRs that *may* be clobbered by the TDX-Module

[Yan Zhao <yan.y.zhao@intel.com>]

On Wed, Nov 05, 2025 at 05:16:56PM +0800, Xiaoyao Li wrote:
> On 11/5/2025 9:52 AM, Yan Zhao wrote:
> > On Tue, Nov 04, 2025 at 09:55:54AM -0800, Sean Christopherson wrote:
> > > On Tue, Nov 04, 2025, Yan Zhao wrote:
> > > > On Tue, Nov 04, 2025 at 04:40:44PM +0800, Xiaoyao Li wrote:
> > > > > On 11/4/2025 3:06 PM, Yan Zhao wrote:
> > > > > > Another nit:
> > > > > > Remove the tdx_user_return_msr_update_cache() in the comment of __tdx_bringup().
> > > > > > 
> > > > > > Or could we just invoke tdx_user_return_msr_update_cache() in
> > > > > > tdx_prepare_switch_to_guest()?
> > > > > 
> > > > > No. It lacks the WRMSR operation to update the hardware value, which is the
> > > > > key of this patch.
> > > > As [1], I don't think the WRMSR operation to update the hardware value is
> > > > necessary. The value will be updated to guest value soon any way if
> > > > tdh_vp_enter() succeeds, or the hardware value remains to be the host value or
> > > > the default value.
> > > 
> > > As explained in the original thread:
> > > 
> > >   : > If the MSR's do not get clobbered, does it matter whether or not they get
> > >   : > restored.
> > >   :
> > >   : It matters because KVM needs to know the actual value in hardware.  If KVM thinks
> > >   : an MSR is 'X', but it's actually 'Y', then KVM could fail to write the correct
> > >   : value into hardware when returning to userspace and/or when running a different
> > >   : vCPU.
> > > 
> > > I.e. updating the cache effectively corrupts state if the TDX-Module doesn't
> > > clobber MSRs as expected, i.e. if the current value is preserved in hardware.
> > I'm not against this patch. But I think the above explanation is not that
> > convincing, (or somewhat confusing).
> > 
> > 
> > By "if the TDX-Module doesn't clobber MSRs as expected",
> > - if it occurs due to tdh_vp_enter() failure, I think it's fine.
> >    Though KVM thinks the MSR is 'X', the actual value in hardware should be
> >    either 'Y' (the host value) or 'X' (the expected clobbered value).
> >    It's benign to preserving value 'Y', no?
> 
> For example, after tdh_vp_enter() failure, the state becomes
> 
>     .curr == 'X'
>     hardware == 'Y'
> 
> and the TD vcpu thread is preempted and the pcpu is scheduled to run another
> VM's vcpu, which is a normal VMX vcpu and it happens to have the MSR value
> of 'X'. So in
> 
>   vmx_prepare_switch_to_guest()
>     -> kvm_set_user_return_msr()
> 
> it will skip the WRMSR because written_value == .curr == 'X', but the
> hardware value is 'Y'. Then KVM fails to load the expected value 'X' for the
> VMX vcpu.
Oh. Thanks! I overlooked that there's another checking of .curr in
kvm_set_user_return_msr(). It explains why .curr must be equal to the hardware
value when outside guest mode.

> > - if it occurs due to TDX module bugs, e.g., if after a successful
> >    tdh_vp_enter() and VM exits, the TDX module clobbers the MSR to 'Z', while
> >    the host value for the MSR is 'Y' and KVM thinks the actual value is 'X'.
> >    Then the hardware state will be incorrect after returning to userspace if
> >    'X' == 'Y'. But this patch can't guard against this condition as well, right?
> > 
> > 
> > > > But I think invoking tdx_user_return_msr_update_cache() in
> > > > tdx_prepare_switch_to_guest() is better than in
> > > > tdx_prepare_switch_to_host().
> > > > 
> > > > [1] https://lore.kernel.org/kvm/aQhJol0CvT6bNCJQ@yzhao56-desk.sh.intel.com/
>