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

[Huang, Kai]

On Wed, 2023-04-12 at 08:22 -0700, Sean Christopherson wrote:
> On Wed, Apr 12, 2023, Kai Huang wrote:
> > On Thu, 2023-04-06 at 13:01 +0300, Zhi Wang wrote:
> > > On Wed, 5 Apr 2023 19:10:40 -0700
> > > Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > > TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.
> > > 
> > > Interesting point. I was digging the CPUID virtualization OF TDX/SNP.
> > > It would be nice to have a conclusion of what is "sane" and what is the
> > > proper role for KVM, as firmware/TDX module is going to validate the "sane"
> > > CPUID.
> > > 
> > > TDX/SNP requires the CPUID to be pre-configured and validated before creating
> > > a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in
> > > SNP_LAUNCH_UPDATE in SNP).
> > > 
> > > IIUC according to what you mentioned, KVM should be treated like "CPUID box"
> > > for QEMU and the checks in KVM is only to ensure the requirements of a chosen
> > > one is literally possible and correct. KVM should not care if the
> > > combination, the usage of the chosen ones is insane or not, which gives
> > > QEMU flexibility.
> > > 
> > > As the valid CPUIDs have been decided when creating a CC guest, what should be
> > > the proper behavior (basically any new checks?) of KVM for the later
> > > SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced
> > > at least, because some KVM code paths rely on guest CPUID configuration.
> > 
> > For TDX guest my preference is KVM to save all CPUID entries in TDH.MNG.INIT and
> > manually make vcpu's CPUID point to the saved CPUIDs.  And then KVM just ignore
> > the SET_CPUID2 for TDX guest.
> 
> It's been a long while since I looked at TDX's CPUID management, but IIRC ignoring
> SET_CPUID2 is not an option becuase the TDH.MNG.INIT only allows leafs that are
> known to the TDX Module, e.g. KVM's paravirt CPUID leafs can't be communicated via
> TDH.MNG.INIT.  
> 

Oh yes.  I forgot this.

> 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.

> 
> 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}?

Looks this is over-complicated, _if_ the "only filtering out PV related CPUIDs
when applying CPUIDs to TDH.MNG.INIT" approach works.