Re: [PATCH v2 00/25] TDX vCPU/VM creation

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

On Mon, 2024-12-23 at 17:25 +0100, Paolo Bonzini wrote:
> To sum up:
> 
> removed:
> 04 replaced by add wrapper functions for SEAMCALLs subseries
> 06: not needed anymore, all logic for KeyID mgmt now in x86/virt/tdx
> 10: tdx_capabilities dropped, replaced mostly by 02

Sorry, what is this? Not from patch 10 "x86/virt/tdx: Add SEAMCALL wrappers for
TDX flush operations". What was dropped from which patch?

> 11: KVM_TDX_CAPABILITIES moved to patch 16
> 19: not needed anymore

I guess this is not referring to "KVM: TDX: initialize VM with TDX specific
parameters", so not sure which one is dropped.

> 20: was needed by patch 24
> 22: folded in other patches

> 24: left for later
> 25: left for later/for userspace
Ok.

I'm can't figure out what these numbers correspond to, but kvm-coco-queue
doesn't seem to have dropped any patches yet, so maybe it will make more sense
when I can take a look at the refresh there.

> 
> 01/02:ok
> 03: need to change 32 to 128
> 04: ok
> 05/06/07/08/09/10: replaced with
> https://lore.kernel.org/kvm/20241203010317.827803-2-rick.p.edgecombe@intel.com/
> 11: see the type safety comment above:
> > The ugly part here is the type-unsafety of to_vmx/to_tdx.  We probably
> > should add some "#pragma poison" of to_vmx/to_tdx: for example both can
> > be poisoned in pmu_intel.c after the definition of
> > vcpu_to_lbr_records(), while one of them can be poisoned in
> > sgx.c/posted_intr.c/vmx.c/tdx.c.

I left it off because you said "Not a strict requirement though." and gave it a
RB tag. Other stuff seemed higher priority. We can look at some options for a
follow on patch if it lightens your load.

> 
> 12/13/14/15: ok
> 16/17: to review
> 18: not sure why the check against num_present_cpus() is needed?

The per-vm KVM_MAX_VCPUS will be min_t(int, kvm->max_vcpus, num_present_cpus()).
So if td_conf->max_vcpus_per_td < num_present_cpus(), then it might report
supporting more CPUs then actually supported by the TDX module.

As to why not just report td_conf->max_vcpus_per_td, that value is the max CPUs
that are supported by any platform the TDX module supports. So it is more about
what the TDX module supports, then what userspace cares about (how many vCPUs
they can use).

I think we could probably get by without the check and blame the TDX module if
it does something strange. It is seems safer ABI-wise to have the check. But we
are being a bit more cavalier around protecting against TDX supported CPUID bit
changes then originally planned, so the check here now seems inconsistent.

Let me flag Kai to confirm there was not some known violating configuration. He
explored a bunch of edge cases on this corner.

> 19: ok
> 20: ok
> 21: ok
> 
> 22: missing review comment from v1
> 
> > +     /* TDX only supports x2APIC, which requires an in-kernel local APIC. */
> > +     if (!vcpu->arch.apic)
> > +             return -EINVAL;
> 
> nit: Use kvm_apic_present()

Oops, nice catch.

> 
> 23: ok
> 
> 24: need to apply fix
> 
> -       if (sub_leaf & TDX_MD_UNREADABLE_LEAF_MASK ||
> +       if (leaf & TDX_MD_UNREADABLE_LEAF_MASK ||
> 
> 25: ok