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

[Zhi Wang]

On Fri, 14 Apr 2023 13:42:11 +0000
"Huang, Kai" <kai.huang@xxxxxxxxx> wrote:

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

I did some investigations and I think this approach would work on both TDX
and SNP, as both of them can let a CC guest handle the firmware-not-aware CPUID
in #VE or #VC. E.g. KVM paravirt CPUIDs. And we can factor out and re-use the
"checking-CPUID-is-equal" in KVM_SET_CPUID{2}. But I think TDX needs to
filter out the firmware-not-aware CPUIDs in TDH.MNG.INIT to pass the check?
(SNP firmware can adjust them automatically). I attached some details I found
in case you are interested in digging.

For TDX, KVM provides a CPUID table in TDH.MNG.INIT, and there are two polices
for the following CPUID virtualization: 1) TDX-module handle the CPUID
interception from a TD guest and emulated according to the CPUID table in
TDH.MNG.INIT. If TDX-module doesn't know this CPUID, #VE is injected 2) A TD
guest can request to handle the CPUID by itself via calling TDG.VP_CPUIDVE_SET.
Then a CPUID TD exit will be forwarded to the guest as #VE. The code snippet
of TDX module handling TD CPUID exit can be found here[1].

For SNP, userspace provides a CPUID table in SNP_LAUNCH_UPDATE with
PAGE_TYPE_CPUID. PSP will check and validate the CPUID in the table. It will be
part of the SNP metadata secrets, and passed to the guest later. A guest can
refer to the validated CPUID table when handling CPUID #VC, but can also handle
CPUIDs not in the table[2] (e.g. paravirt CPUID).


[1] https://downloadmirror.intel.com/738876/tdx-module-v1.0.01.01.zip/src/td_dispatcher/vm_exits/td_cpuid.c
[2] https://github.com/AMDESE/linux-svsm/blob/main/src/cpu/vc.rs#L571
> 
> > 
> > > 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.
>