Re: [PATCH 18/25] KVM: TDX: Do TDX specific vcpu initialization

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 10/17/2024 1:42 AM, Edgecombe, Rick P wrote:
> On Wed, 2024-10-09 at 18:01 +0300, Adrian Hunter wrote:
>> tdh_vp_init_apicid() passes x2APIC ID to TDH.VP.INIT which
>> is one of the steps for the TDX Module to support topology
>> information for the guest i.e. CPUID leaf 0xB and CPUID leaf 0x1F.
>>
>> If the host VMM does not provide CPUID leaf 0x1F values
>> (i.e. the values are 0), then the TDX Module will use native
>> values for both CPUID leaf 0x1F and CPUID leaf 0xB.
>>
>> To get 0x1F/0xB the guest must also opt-in by setting
>> TDCS.TD_CTLS.ENUM_TOPOLOGY to 1.  AFAICT currently Linux
>> does not do that.
>>
>> In the tdh_vp_init() case, topology information will not be
>> supported.
>>
>> If topology information is not supported CPUID leaf 0xB and
>> CPUID leaf 0x1F will #VE, and a Linux guest will return zeros.
>>
>> So, yes, it seems like tdh_vp_init_apicid() should only
>> be called if there is non-zero CPUID leaf 0x1F values provided
>> by host VMM. e.g. add a helper function
>>
>> bool tdx_td_enum_topology(struct kvm_cpuid2 *cpuid)
>> {
>> 	const struct tdx_sys_info_features *modinfo = &tdx_sysinfo->features;
>> 	const struct kvm_cpuid_entry2 *entry;
>>
>> 	if (!(modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM))
>> 		return false;
>>
>> 	entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0x1f, 0);
>> 	if (!entry)
>> 		return false;
>>
>> 	return entry->eax || entry->ebx || entry->ecx || entry->edx;
>> }
> 
> KVM usually leaves it up to userspace to not create nonsensical VMs. So I think
> we can skip the check in KVM.

It's not nonsensical unless KVM announces its own requirement for TD 
guest that userspace VMM must provide valid CPUID leaf 0x1f value for 
topology.

It's architectural valid that userspace VMM creates a TD with legacy 
topology, i.e., topology enumerated via CPUID 0x1 and 0x4.

> In that case, do you see a need for the vanilla tdh_vp_init() SEAMCALL wrapper?
> 
> The TDX module version we need already supports enum_topology, so the code:
> 	if (modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)
> 		err = tdh_vp_init_apicid(tdx, vcpu_rcx, vcpu->vcpu_id);
> 	else
> 		err = tdh_vp_init(tdx, vcpu_rcx);
> 
> The tdh_vp_init() branch shouldn't be hit.

We cannot know what version of TDX module user might use thus we cannot 
assume enum_topology is always there unless we make it a hard 
requirement in KVM that TDX fails being enabled when

   !(modinfo->tdx_features0 & MD_FIELD_ID_FEATURES0_TOPOLOGY_ENUM)