Re: [PATCH 7/7] KVM: TDX: Add TSX_CTRL msr into uret_msrs list

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

On Wed, 2024-12-04 at 13:55 +0200, Adrian Hunter wrote:
> > > > 
> > > > They are cleared from the configurable bitmap by
> > > > tdx_clear_unsupported_cpuid(),
> > > > so they are not configurable from a userspace perspective. Did I miss
> > > > anything?
> > > > KVM should check user inputs against its adjusted configurable bitmap,
> > > > right?
> > > 
> > > Maybe I misunderstand but we rely on the TDX module to reject
> > > invalid configuration.  We don't check exactly what is configurable
> > > for the TDX Module.
> > 
> > Ok, this is what I missed. I thought KVM validated user input and masked
> > out all unsupported features. sorry for this.

This used to be how it behaved, but IIRC Paolo had suggested to simplify it by
letting the TDX module do the rejection. But that was under the assumption there
wasn't any TDX supported CPUID configuration that was harmful to KVM.

We also used to filter which CPUID features that weren't supported by KVM, but
this was also dropped to make things simpler.

> > 
> > > 
> > > TSX and WAITPKG are not invalid for the TDX Module, but KVM
> > > must either support them by restoring their MSRs, or disallow
> > > them.  This patch disallows them for now.
> > 
> > Yes. I agree. what if a new feature (supported by a future TDX module) also
> > needs KVM to restore some MSRs? current KVM will allow it to be exposed
> > (since
> > only TSX/WAITPKG are checked); then some MSRs may get corrupted. I may think
> > this is not a good design. Current KVM should work with future TDX modules.
> 
> With respect to CPUID, I gather this kind of thing has been
> discussed, such as here:
> 
> 	https://lore.kernel.org/all/ZhVsHVqaff7AKagu@google.com/
> 
> and Rick and Xiaoyao are working on something.

This is around fixed 0/1 bits, and just to help userspace understand what
configurations are possible. It isn't intended to filter any bits on the KVM
side.

> 
> In general, I would expect a new TDX Module would advertise support for
> new features, but KVM would have to opt in to use them.

This is true for attributes/xfam, but isn't for CPUID leafs. We used to filter
them in various ways but it was messy. The suggestion was to simplify it. The
current approach is to treat any TDX module changes that break the host like
that as TDX module bugs. See this coverletter for more info on the history:

	https://lore.kernel.org/kvm/20240812224820.34826-1-rick.p.edgecombe@intel.com/