[PATCH V3 12/13] target/i386: Clean up Intel Debug Store feature dependencies

Zide Chen posted 13 patches 1 month, 1 week ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Zhao Liu <zhao1.liu@intel.com>, Marcelo Tosatti <mtosatti@redhat.com>
[PATCH V3 12/13] target/i386: Clean up Intel Debug Store feature dependencies
Posted by Zide Chen 1 month, 1 week ago
- 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 <zide.chen@intel.com>
---
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;
     }
 
-- 
2.53.0
Re: [PATCH V3 12/13] target/i386: Clean up Intel Debug Store feature dependencies
Posted by Chenyi Qiang 3 weeks, 5 days ago

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 <zide.chen@intel.com>
> ---
> 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:

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++) {



>      }
>
Re: [PATCH V3 12/13] target/i386: Clean up Intel Debug Store feature dependencies
Posted by Chen, Zide 3 weeks, 5 days ago

On 3/15/2026 8:21 PM, 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 <zide.chen@intel.com>
>> ---
>> 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].

Yes, I agree. In the TDX case, neither the DTS nor the arch_lbr bit
should be cleared.


> These features' configuration do not have dependencies on each other. So how about skipping the TD VM case like:
> 
> 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++) {
> 
> 
> 
>>      }
>>  
>
Re: [PATCH V3 12/13] target/i386: Clean up Intel Debug Store feature dependencies
Posted by Xiaoyao Li 3 weeks, 5 days ago
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 <zide.chen@intel.com>
>> ---
>> 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"

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

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);
	}

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.

> 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++) {
> 
> 
> 
>>       }
>>   
>
Re: [PATCH V3 12/13] target/i386: Clean up Intel Debug Store feature dependencies
Posted by Chen, Zide 3 weeks, 5 days ago

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 <zide.chen@intel.com>
>>> ---
>>> 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++) {
>>
>>
>>
>>>       }
>>>   
>>
> 


Re: [PATCH V3 12/13] target/i386: Clean up Intel Debug Store feature dependencies
Posted by Mi, Dapeng 1 month ago
Reviewed-by: Dapeng Mi <dapeng1.mi@linux.intel.com>

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 <zide.chen@intel.com>
> ---
> 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;
>      }
>