Re: [PATCH v7 2/9] target/i386: disable PERFCORE when "-pmu" is configured

[Zhao Liu <zhao1.liu@intel.com>]

Hi Dongli,

> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 3653f8953e..4fcade89bc 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -8360,6 +8360,10 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>              !(env->hflags & HF_LMA_MASK)) {
>              *edx &= ~CPUID_EXT2_SYSCALL;
>          }
> +
> +        if (!cpu->enable_pmu) {
> +            *ecx &= ~CPUID_EXT3_PERFCORE;
> +        }

Learned the lessons from PDCM [*]:

[*] https://lore.kernel.org/qemu-devel/20250923104136.133875-3-pbonzini@redhat.com/

Directly masking CPUID bit off is not a good idea... modifying the
CPUID, even when fixing or adding dependencies, can easily break
migration.

So a safe way is to add a compat option. And I think it would be better
if patch 1 also has a compat option. A single compat option could cover
patch 1 & 2.

I have these thoughts:

* For "-pmu" dependency, it can be checked as [*] did.
* For normal feature bit dependency (patch 1), it seems possible to add
  compat_prop condition in feature_dependencies[].

I attached a draft for discussion (which is on the top of the whole
series).

Note, we are currently in the RC phase for v10.2, and the
pc_compat_10_2[] array is not yet added, which will be added before the
release of v10.2. Therefore, we have enough time for discussion I think.

If you think it's fine, I'll post compat_prop support separately as an
RFC. The specific compat option can be left to this series.

Although wait for a while, in a way, it is lucky :) - there's an
opportunity to avoid making mistakes for us.

Regards,
Zhao

------------------------------------------------
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f8b919cb6c47..d2b8e83e556d 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -83,6 +83,7 @@

 GlobalProperty pc_compat_10_1[] = {
     { "mch", "extended-tseg-mbytes", "16" },
+    { TYPE_X86_CPU, "x-amd-perfmon-always-on", "true" }, // This should be added to pc_compat_10_2!
 };
 const size_t pc_compat_10_1_len = G_N_ELEMENTS(pc_compat_10_1);

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 4fcade89bcea..8c56789eb296 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1997,6 +1997,8 @@ static FeatureDep feature_dependencies[] = {
     {
         .from = { FEAT_8000_0001_ECX,       CPUID_EXT3_PERFCORE },
         .to = { FEAT_8000_0022_EAX,         CPUID_8000_0022_EAX_PERFMON_V2 },
+        .compat_prop =
+            { "x-amd-perfmon-always-on",    "false" },
     },
 };

@@ -8998,15 +9000,36 @@ void x86_cpu_expand_features(X86CPU *cpu, Error **errp)
         }
     }

-    if (!cpu->pdcm_on_even_without_pmu) {
+    if (!cpu->enable_pmu) {
         /* PDCM is fixed1 bit for TDX */
-        if (!cpu->enable_pmu && !is_tdx_vm()) {
+        if (!cpu->pdcm_on_even_without_pmu && !is_tdx_vm()) {
             env->features[FEAT_1_ECX] &= ~CPUID_EXT_PDCM;
         }
+
+        if (!cpu->amd_perfmon_always_on) {
+            env->features[FEAT_8000_0001_ECX] &= ~CPUID_EXT3_PERFCORE;
+        }
     }

     for (i = 0; i < ARRAY_SIZE(feature_dependencies); i++) {
         FeatureDep *d = &feature_dependencies[i];
+        bool compat_omit = false;
+        PropValue *prop = &d->compat_prop;
+
+        if (prop->value) {
+            char *v = object_property_print(OBJECT(cpu), prop->prop, false, NULL);
+            if (v) {
+                compat_omit = !strcmp(prop->value, v);
+                printf("feature_dependencies: v: %s, value: %s, omit: %d\n",
+                       v, prop->value, compat_omit);
+                g_free(v);
+            }
+
+            if (compat_omit) {
+                continue;
+            }
+        }
+
         if (!(env->features[d->from.index] & d->from.mask)) {
             uint64_t unavailable_features = env->features[d->to.index] & d->to.mask;

@@ -10065,6 +10088,8 @@ static const Property x86_cpu_properties[] = {
                      arch_cap_always_on, false),
     DEFINE_PROP_BOOL("x-pdcm-on-even-without-pmu", X86CPU,
                      pdcm_on_even_without_pmu, false),
+    DEFINE_PROP_BOOL("x-amd-perfmon-always-on", X86CPU,
+                     amd_perfmon_always_on, false),
 };

 #ifndef CONFIG_USER_ONLY
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 6d78f3995b6b..ac8dd92efc59 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -696,8 +696,14 @@ typedef struct FeatureMask {
     uint64_t mask;
 } FeatureMask;

+typedef struct PropValue {
+    const char *prop, *value;
+} PropValue;
+
 typedef struct FeatureDep {
     FeatureMask from, to;
+    /* Once set, only when prop is satisfied, dependency can be checked. */
+    PropValue compat_prop;
 } FeatureDep;

 typedef uint64_t FeatureWordArray[FEATURE_WORDS];
@@ -2353,6 +2359,16 @@ struct ArchCPU {
      */
     bool pdcm_on_even_without_pmu;

+    /*
+     * Backwards compatibility with QEMU <10.2. (TODO: change 10.2 to 11.0!)
+     * This flag covers 2 parts:
+     * - The PERFCORE feature is now disabled when PMU is not available, but prior to
+     *   10.2 it was enabled even if PMU is off.
+     * - The PerfMonV2 feature is now disabled when PERFCORE is disabled, but prior to
+     *   10.2 it was enabled even if PERFCORE is off.
+     */
+    bool amd_perfmon_always_on;
+
     /* Number of physical address bits supported */
     uint32_t phys_bits;

@@ -2579,9 +2595,7 @@ bool cpu_x86_xrstor(CPUX86State *s, void *host, size_t len, uint64_t rbfm);
 /* cpu.c */
 void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
                               uint32_t vendor2, uint32_t vendor3);
-typedef struct PropValue {
-    const char *prop, *value;
-} PropValue;
+
 void x86_cpu_apply_props(X86CPU *cpu, PropValue *props);

 void x86_cpu_after_reset(X86CPU *cpu);