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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <94e37a815632447d4d16df0a85f3ec2e346fca49.camel@intel.com>
Date: Sat, 4 Jan 2025 01:43:56 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "pbonzini@...hat.com" <pbonzini@...hat.com>
CC: "seanjc@...gle.com" <seanjc@...gle.com>, "Huang, Kai"
	<kai.huang@...el.com>, "Li, Xiaoyao" <xiaoyao.li@...el.com>,
	"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"tony.lindgren@...ux.intel.com" <tony.lindgren@...ux.intel.com>,
	"kvm@...r.kernel.org" <kvm@...r.kernel.org>, "Zhao, Yan Y"
	<yan.y.zhao@...el.com>, "Chatre, Reinette" <reinette.chatre@...el.com>
Subject: Re: [PATCH v2 00/25] TDX vCPU/VM creation

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