Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Huang, Kai]

On Thu, 2023-04-13 at 15:48 -0700, Sean Christopherson wrote:
> On Thu, Apr 13, 2023, Kai Huang wrote:
> > On Wed, 2023-04-12 at 08:22 -0700, Sean Christopherson wrote:
> > > KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out
> > > unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf
> > > that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module
> > > could result in previously allowed input becoming invalid.
> > 
> > How about only filtering out PV related CPUIDs when applying CPUIDs to
> > TDH.MNG.INIT?  I think we can assume they are not gonna be known to TDX module
> > anyway.
> 
> Nope, not going down that road.  Fool me once[*], shame on you.  Fool me twice,
> shame on me :-)

Ah OK :)

> 
> Objections to hardware vendors defining PV interfaces aside, there exist leafs
> that are neither PV related nor known to the TDX module, e.g. Centaur leafs.  I
> think it's extremely unlikely (understatement) that anyone will want to expose
> Centaur leafs to a TDX guest, but again I want to say out of the business of
> telling userspace what is and isn't sane CPUID models.

Right.  There might be use case that TDX guest wants to use some CPUID which
isn't handled by the TDX module but purely by KVM.  We don't want to limit the
possibility.  Totally agree.

> 
> [*] https://lore.kernel.org/all/20221210160046.2608762-6-chen.zhang@xxxxxxxxx
> 
> > > Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option
> > > becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the
> > > leaf would effectively make _everything_ valid input.  If KVM were to rely solely
> > > on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.
> > 
> > Right.  Disallowing SET_CPUID{2} probably is better, as it gives userspace a
> > more concrete result.  
> > 
> > > 
> > > Back to Zhi's question, the best thing to do for TDX and SNP is likely to require
> > > that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent.  The
> > > key difference is that KVM would be enforcing consistency, not sanity.  I.e. KVM
> > > isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring
> > > that userspace provide a CPUID model that's consistent with what userspace provided
> > > earlier.
> > 
> > So IIUC, you prefer to verifying the CPUIDs in SET_CPUID{2} are a super set of
> > the CPUIDs provided in TDH.MNG.INIT?  And KVM manually verifies all CPUIDs for
> > all vcpus are consistent (the same) in SET_CPUID{2}?
> 
> Yes, except KVM doesn't need to verify vCPUs are consistent with respect to each
> other, just that each vCPU is consistent with respect to what was reported to the
> TDX Module.

OK.  Fine to me.

> 
> > Looks this is over-complicated, _if_ the "only filtering out PV related CPUIDs
> > when applying CPUIDs to TDH.MNG.INIT" approach works. 
> 
> It's not complicated at all.  Walk through the leafs defined during TDH.MNG.INIT,
> reject KVM_SET_CPUID if a leaf isn't present or doesn't match exactly.  Or has
> the TDX spec changed and it's no longer that simple?

No the module hasn't been changed, and yes it should be as simple as you said. 
I just had some first impression that handling CPUID in one IOCTL (TDH.MNG.INIT)
should be simpler than handling CPUID in two IOCTLs, but I guess this might not
be true :)

Anyway I agree with your suggestion.  Thanks.