[PATCH 0/2] i386: Adjust CPUID_EXT_PDCM based on enable_pmu at realization

[PATCH 0/2] i386: Adjust CPUID_EXT_PDCM based on enable_pmu at realization

[Xiaoyao Li]

First, it's not a good practice that values in env->features[] cannot be
directly used for guest CPUID in void cpu_x86_cpuid(), but require further
adjustment there. env->features[] are supposed to be finalized at cpu
realization, so that after it env->features[] is reliable.

Second, there is one dependency entry relates to CPUID_EXT_PDCM in
feature_dependencies[]. QEMU needs to get correct value of
CPUID_EXT_PDCM in env->features[] to ensure applying the dependencies
correctly.

Patch 1 resolves above two points.

Patch 2 is a enhancement to give users a warning when they request pdcm
explicitly while PMU disabled.

Xiaoyao Li (2):
  i386/cpu: Move adjustment of CPUID_EXT_PDCM before
    feature_dependencies[] check
  i386/cpu: Warn about why CPUID_EXT_PDCM is not available

 target/i386/cpu.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

-- 
2.34.1

[PATCH 1/2] i386/cpu: Move adjustment of CPUID_EXT_PDCM before feature_dependencies[] check

[Xiaoyao Li]

There is one entry relates to CPUID_EXT_PDCM in feature_dependencies[].
So it needs to get correct value of CPUID_EXT_PDCM before using
feature_dependencies[] to apply dependencies.

Besides, it also ensures CPUID_EXT_PDCM value is tracked in
env->features[FEAT_1_ECX].

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/cpu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 72ab147e851a..2bf6495140a0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6693,9 +6693,6 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         if (threads_per_pkg > 1) {
             *ebx |= threads_per_pkg << 16;
         }
-        if (!cpu->enable_pmu) {
-            *ecx &= ~CPUID_EXT_PDCM;
-        }
         break;
     case 2:
         /* cache info: needed for Pentium Pro compatibility */
@@ -7684,6 +7681,10 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
         }
     }
 
+    if (!cpu->enable_pmu) {
+        env->features[FEAT_1_ECX] &= ~CPUID_EXT_PDCM;
+    }
+
     for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
         FeatureDep *d = &feature_dependencies[i];
         if (!(env->features[d->from.index] & d->from.mask)) {
-- 
2.34.1

Re: [PATCH 1/2] i386/cpu: Move adjustment of CPUID_EXT_PDCM before feature_dependencies[] check

[Zhao Liu]

On Tue, Mar 04, 2025 at 12:24:49AM -0500, Xiaoyao Li wrote:
> Date: Tue, 4 Mar 2025 00:24:49 -0500
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH 1/2] i386/cpu: Move adjustment of CPUID_EXT_PDCM before
>  feature_dependencies[] check
> X-Mailer: git-send-email 2.34.1
> 
> There is one entry relates to CPUID_EXT_PDCM in feature_dependencies[].
> So it needs to get correct value of CPUID_EXT_PDCM before using
> feature_dependencies[] to apply dependencies.
> 
> Besides, it also ensures CPUID_EXT_PDCM value is tracked in
> env->features[FEAT_1_ECX].
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/cpu.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

[PATCH 2/2] i386/cpu: Warn about why CPUID_EXT_PDCM is not available

[Xiaoyao Li]

When user requests PDCM explicitly via "+pdcm" without PMU enabled, emit
a warning to inform the user.

Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
 target/i386/cpu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 2bf6495140a0..2aa2bab12100 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7682,6 +7682,9 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
     }
 
     if (!cpu->enable_pmu) {
+        mark_unavailable_features(cpu, FEAT_1_ECX,
+                                  env->user_features[FEAT_1_ECX] & CPUID_EXT_PDCM,
+                                  "This feature is not available due to PMU disabled");
         env->features[FEAT_1_ECX] &= ~CPUID_EXT_PDCM;
     }
 
-- 
2.34.1

Re: [PATCH 2/2] i386/cpu: Warn about why CPUID_EXT_PDCM is not available

[Zhao Liu]

On Tue, Mar 04, 2025 at 12:24:50AM -0500, Xiaoyao Li wrote:
> Date: Tue, 4 Mar 2025 00:24:50 -0500
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Subject: [PATCH 2/2] i386/cpu: Warn about why CPUID_EXT_PDCM is not
>  available
> X-Mailer: git-send-email 2.34.1
> 
> When user requests PDCM explicitly via "+pdcm" without PMU enabled, emit
> a warning to inform the user.
> 
> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> ---
>  target/i386/cpu.c | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Zhao Liu <zhao1.liu@intel.com>

Re: [PATCH 0/2] i386: Adjust CPUID_EXT_PDCM based on enable_pmu at realization

[Zhao Liu]

Hi Xiaoyao,

> First, it's not a good practice that values in env->features[] cannot be
> directly used for guest CPUID in void cpu_x86_cpuid(), but require further
> adjustment there. env->features[] are supposed to be finalized at cpu
> realization, so that after it env->features[] is reliable.
> 
> Second, there is one dependency entry relates to CPUID_EXT_PDCM in
> feature_dependencies[]. QEMU needs to get correct value of
> CPUID_EXT_PDCM in env->features[] to ensure applying the dependencies
> correctly.

I agree that this is a very good idea, especially since PDCM has a
dependency entry.

"pmu" is totally a property rather than a feature bit, which makes the
dependency relationships in the code complex. Therefore, I think it's
worth having a series to clarify the dependencies of pmu as much as
possible.

I remember Dapeng/Zide also have fixes for pmu dependencies, and if
possible, I could help you combine this series with others' cleanups.

Additionally, I think patch 1 and patch 2 can be merged together. Do you
agree?

Thanks,
Zhao

Re: [PATCH 0/2] i386: Adjust CPUID_EXT_PDCM based on enable_pmu at realization

[Xiaoyao Li]

On 3/7/2025 12:22 AM, Zhao Liu wrote:
> Hi Xiaoyao,
> 
>> First, it's not a good practice that values in env->features[] cannot be
>> directly used for guest CPUID in void cpu_x86_cpuid(), but require further
>> adjustment there. env->features[] are supposed to be finalized at cpu
>> realization, so that after it env->features[] is reliable.
>>
>> Second, there is one dependency entry relates to CPUID_EXT_PDCM in
>> feature_dependencies[]. QEMU needs to get correct value of
>> CPUID_EXT_PDCM in env->features[] to ensure applying the dependencies
>> correctly.
> 
> I agree that this is a very good idea, especially since PDCM has a
> dependency entry.
> 
> "pmu" is totally a property rather than a feature bit, which makes the
> dependency relationships in the code complex. Therefore, I think it's
> worth having a series to clarify the dependencies of pmu as much as
> possible.
> 
> I remember Dapeng/Zide also have fixes for pmu dependencies, and if
> possible, I could help you combine this series with others' cleanups.

The reason I sent out this small series quickly is mainly for Dongli to 
as a reference.

In fact, there are mess on LBR enabling that it checks enable_pmu 
everytime with CPUID_7_0_EDX_ARCH_LBR as well as 
CPUID_8000_0022_EAX_PERFMON_V2. That's on my WIP list to clean it up.

I think I need to check if they are duplicated with Dapeng/Zide's series.

> Additionally, I think patch 1 and patch 2 can be merged together. Do you
> agree?

IMHO, they stand as their own. I'll leave it to Paolo to make the decision.

> Thanks,
> Zhao
> 

Re: [PATCH 0/2] i386: Adjust CPUID_EXT_PDCM based on enable_pmu at realization

[Paolo Bonzini]

Queued, thanks; sorry about the delay.

Paolo