On 3/15/2026 11:57 PM, Xiaoyao Li wrote:
> On 3/16/2026 11:21 AM, Chenyi Qiang wrote:
>>
>>
>> On 3/5/2026 2:07 AM, Zide Chen wrote:
>>> - 64-bit DS Area (CPUID.01H:ECX[2]) depends on DS (CPUID.01H:EDX[21]).
>>> - When PMU is disabled, Debug Store must not be exposed to the guest,
>>> which implicitly disables legacy DS-based PEBS.
>>>
>>> Signed-off-by: Zide Chen <[email protected]>
>>> ---
>>> V3:
>>> - Update title to be more accurate.
>>> - Make DTES64 depend on DS.
>>> - Mark MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL in previous patch.
>>> - Clean up the commit message.
>>>
>>> V2: New patch.
>>> ---
>>> target/i386/cpu.c | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>>> index 2e1dea65d708..3ff9f76cf7da 100644
>>> --- a/target/i386/cpu.c
>>> +++ b/target/i386/cpu.c
>>> @@ -1899,6 +1899,10 @@ static FeatureDep feature_dependencies[] = {
>>> .from = { FEAT_1_ECX, CPUID_EXT_PDCM },
>>> .to = { FEAT_PERF_CAPABILITIES, ~0ull },
>>> },
>>> + {
>>> + .from = { FEAT_1_EDX, CPUID_DTS},
>>> + .to = { FEAT_1_ECX, CPUID_EXT_DTES64},
>>> + },
>>> {
>>> .from = { FEAT_1_ECX, CPUID_EXT_VMX },
>>> .to = { FEAT_VMX_PROCBASED_CTLS, ~0ull },
>>> @@ -9471,6 +9475,7 @@ void x86_cpu_expand_features(X86CPU *cpu, Error
>>> **errp)
>>> env->features[FEAT_1_ECX] &= ~CPUID_EXT_PDCM;
>>> }
>>> + env->features[FEAT_1_EDX] &= ~CPUID_DTS;
>>> env->features[FEAT_7_0_EDX] &= ~CPUID_7_0_EDX_ARCH_LBR;
>>
>> This change, along with the original CPUID_7_0_EDX_ARCH_LBR clear,
>> will also affect the configuration for TD VMs.
>> For a TD VM, enable_pmu controls TDX_TD_ATTRIBUTES_PERFMON, CPUID_DTS
>> is fixed to 1, and arch_lbr is controlled by XFAM[15].
>> These features' configuration do not have dependencies on each other.
>> So how about skipping the TD VM case like:
>
> I think the dependency between enable_pmu and ARCH_LBR still applies to
> TDX. This dependency is defined by QEMU that enable_pmu controls all the
> PMU features, including ARCH_LBR. So we can enforce the rule of
> "XFAM[15] cannot be 1 when enable_pmu == 0"
Yes, when !enable_pmu, XSTATE_ARCH_LBR_MASK will be cleared in
FEAT_XSAVE_XSS_LO, and therefore the ARCH_LBR bit in XFAM will also be
cleared. As a result, CPUID_7_0_EDX_ARCH_LBR will ultimately be cleared
from the guest CPUID. However, it is better to follow the spec and leave
it unchanged here.
> For CPUID_DTS, it seems OK to expose it even when PMU is disabled?
> I sort of disagree with the statement in the changelog:
>
> - When PMU is disabled, Debug Store must not be exposed to the guest,
> which implicitly disables legacy DS-based PEBS
As you mentioned, the dependencies of enable_pmu are defined by QEMU.
If the DS bit remains set when !enable_pmu, it looks inconsistent.
> If I read SDM correctly, the availability of legacy PEBS can be
> enumerated by MSR_IA32_MISC_ENABLE.PEBS_UNAVAILABLE bit. And from the
> linux code, it also proves that DTS can be 1 while PEBS is 0:
>
> if (boot_cpu_has(X86_FEATURE_DS)) {
> unsigned int l1, l2;
>
> rdmsr(MSR_IA32_MISC_ENABLE, l1, l2);
> if (!(l1 & MSR_IA32_MISC_ENABLE_BTS_UNAVAIL))
> set_cpu_cap(c, X86_FEATURE_BTS);
> if (!(l1 & MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL))
> set_cpu_cap(c, X86_FEATURE_PEBS);
> }
MSR_IA32_MISC_ENABLE_PEBS_UNAVAIL are sufficient to prevent the
guest from enumerating the PEBS feature.
However, keeping CPUID_DTS set has some negative impacts. For example,
it may incorrectly determine the availability of IA32_DS_AREA.
> Since it will need nasty handling for TDX case, I would vote not
> clearing CPUID_DTS here when !PMU, unless a strong reason is provided.
The "nasty handling for TDX" code already there, adding CPUID_DTS may
not make it worse. :) Accepting Chenyi's suggested code seems reasonable
and clean.
>
>> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
>> index 98e95d0842..458bfb07b9 100644
>> --- a/target/i386/cpu.c
>> +++ b/target/i386/cpu.c
>> @@ -9736,8 +9736,10 @@ void x86_cpu_expand_features(X86CPU *cpu, Error
>> **errp)
>> env->features[FEAT_1_ECX] &= ~CPUID_EXT_PDCM;
>> }
>>
>> - env->features[FEAT_1_EDX] &= ~CPUID_DTS;
>> - env->features[FEAT_7_0_EDX] &= ~CPUID_7_0_EDX_ARCH_LBR;
>> + if (!is_tdx_vm()) {
>> + env->features[FEAT_1_EDX] &= ~CPUID_DTS;
>> + env->features[FEAT_7_0_EDX] &= ~CPUID_7_0_EDX_ARCH_LBR;
>> + }
>> }
>>
>> for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
>>
>>
>>
>>> }
>>>
>>
>
