Re: [PATCH v3 1/4] KVM: TDX: Track configurable CPUID bits allowed by KVM

[Binbin Wu <binbin.wu@linux.intel.com>]

On 9/3/2026 3:28 PM, Xiaoyao Li wrote:
> On 9/3/2026 12:19 AM, Binbin Wu wrote:
>> On 9/2/2026 11:09 PM, Xiaoyao Li wrote:
>>> On 9/2/2026 8:33 AM, Binbin Wu wrote:
>>>>>> +static void __init tdx_initialize_cpu_cfg_caps(void)
>>>>>> +{
>>>>>> +    tdx_cpu_cfg_cap_init(CPUID_1_ECX,
>>>>>> +        TDX_CFG_EXTRA_F(MWAIT),
>>>>>> +        TDX_CFG_F(TSC_DEADLINE_TIMER),
>>>>>> +        TDX_CFG_F(AVX),
>>>>>> +        TDX_CFG_F(F16C),
>>>>>> +    );
>>>>> TDX 1.5.24 on SPR report configurable bits of CPUID_1_ECX as
>>>>> 0x31044988, which have
>>>>>
>>>>> - bit 3        MWAIT
>>>>> - bit 7        EST
>>>>> - bit 8        TM2
>>>>> - bit 11    SDBG
>>>>> - bit 14    XTPR
>>>>> - bit 18    DCA
>>>>> - bit 24    TSC_DEADLINE_TIMER
>>>>> - bit 28    AVX
>>>>> - bit 29    F16C
>>>>>
>>>>> but EST/TM2/SDBG/XTPR/DCA are not list here. I guess the reason is kvm_cpu_cap[] doesn't support it. If so, it seems to guard twice:
>>>>> 1. mentally/manually check if it a feature is supported in kvm_cpu_caps[]
>>>>>
>>>>> 2. kvm_cpu_caps guarding in tdx_cpu_cfg_cap_init().
>>>>>
>>>>> I think 1) is not necessary, we can rely on 2)
>>>> In general, if a feature is not supported by the common KVM CPU caps, 
>>> For kvm-intel.ko, kvm_cpu_caps[] just means the supported CPUID features
>>> for VMX VMs. Treat it as the common KVM CPU caps is a bit arguable.
>>>
>>>> I prefer not
>>>> to add it to the list to save a few lines of code, which probably is dead code,
>>> I don't think it's dead code. It shows that these features are
>>> virtualizable to TDs from the POV. of TDX.
>> It depends on whether KVM allows userspace to set features for TDs that are not support
>> for non-TDX VMs (,except for a few exceptions).
>>
>> In this version, TDX_CFG_F() already check against kvm_cpu_caps[], if these features
>> are not in kvm_cpu_caps[], it will not be exposed to userspace anyway.
>>
> 
> I still think the reasoning that we omit them because they are not
> contained in kvm_cpu_caps[] sounds not right. 

TBH,I omitted these bits for simplicity.
It's not just adding a TDX_CFG_F(XXX) for a feature, but also need to
add new X86_FEATURE_XXX definitions sometimes.
E.g. CPUID.7.0.ECX[0] (PREFETCHWT1), which is directly configurable and not
supported by KVM. It has no definition in <asm/cpufeatures.h>.

I am not sure that's worth it.

> Based on it, so when we are
> going to add a new feature for TDX, we need to first manually check the KVM
> code to see if that feature is contained in kvm_cpu_caps[] already. If not,
> we just don't add it to TDX's list. Then why need to cap the result
> kvm_cpu_caps[] for TDX_CFG_F() again? 

This is due to "generally speaking, KVM shouldn't allow features that KVM doesn't
support for non-TDX VMs".

I'm hoping Sean can weigh in on whether KVM should restrict TDX's CPU capabilities
in this way.

> Just for safety in case human make
> mistake and misread the code of kvm_cpu_caps[]?
> 
> I think they are two independent steps:
> 1. list the CPUID features that KVM can support for TDs.
> 2. apply additional restrictions, e.g., if a feature is not allowed for
> non-TDX VMs, it cannot be allowed for TDs.
> 
>>> In the end, they might be disallowed to be configured to TDs because KVM
>>> doesn't allow them for VMX VMs. This is also the point I want to discuss.
>>> Do we really want to make such restriction that KVM cannot enable/allow a
>>> feature for TDs unless KVM first enables/allows it for VMX VMs? What's
>>> reason behind it?
>> Sean mentioned it that "generally speaking, KVM shouldn't allow features
>> that KVM doesn't support for non-TDX VMs" in
>> https://lore.kernel.org/kvm/aj1fi_0SBxMK5WOB@google.com/
> 
> For existing features, it might make some sense. But for new features, I
> don't think so. It defines the enabling order for new features that we must
> enable a feature for non-TDX VMs first and then TDs. And people might want
> to bypass this rule by abusing the TDX_CFG_EXTRA_F() when only one line of
> TDX_CFG_EXTRA_F() is enough to enable a feature for TDs but more effort
> required to enable it for non-TDX VMs.

Oh, you just remind me that I should add comment for TDX_CFG_EXTRA_F() that
it must be used with a strong justification.

> 
> Maybe I miss somthing. I would like to see stronger reasons for such decision.