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