Re: [PATCH v2 07/10] target/i386/kvm: query kvm.enable_pmu parameter

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

On Sun, Mar 02, 2025 at 02:00:15PM -0800, Dongli Zhang wrote:
> Date: Sun,  2 Mar 2025 14:00:15 -0800
> From: Dongli Zhang <dongli.zhang@oracle.com>
> Subject: [PATCH v2 07/10] target/i386/kvm: query kvm.enable_pmu parameter
> X-Mailer: git-send-email 2.43.5
> 
> There is no way to distinguish between the following scenarios:
> 
> (1) KVM_CAP_PMU_CAPABILITY is not supported.
> (2) KVM_CAP_PMU_CAPABILITY is supported but disabled via the module
> parameter kvm.enable_pmu=N.
> 
> In scenario (1), there is no way to fully disable AMD PMU virtualization.
> 
> In scenario (2), PMU virtualization is completely disabled by the KVM
> module.

KVM_CAP_PMU_CAPABILITY is introduced since ba7bb663f554 ("KVM: x86:
Provide per VM capability for disabling PMU virtualization") in v5.18,
so I understand you want to handle the old linux before v5.18.

Let's sort out all the cases:

1) v5.18 and after, if the parameter "enable_pmu" is Y and then
   KVM_CAP_PMU_CAPABILITY exists, so everything could work.

2) v5.18 and after, "enable_pmu" is N and then KVM_CAP_PMU_CAPABILITY
   doesn't exist, QEMU needs to helpe user disable vPMU.

3) v5.17 (since "enable_pmu" is introduced in v5.17 since 4732f2444acd
   ("KVM: x86: Making the module parameter of vPMU more common")),
   there's no KVM_CAP_PMU_CAPABILITY and vPMU enablement depends on
   "enable_pmu". QEMU's enable_pmu option should depend on kvm
   parameter.

4) before v5.17, there's no "enable_pmu" so that there's no way to
   fully disable AMD PMU.

IIUC, you want to distinguish 2) and 3). And your current codes won't
break old kernels on 4) because "kvm_pmu_disabled" defaults false.
Therefore, overall the idea of this patch is good for me.

But IMO, the logics all above can be compatible by:

 * First check the KVM_CAP_PMU_CAPABILITY,
 * Only if KVM_CAP_PMU_CAPABILITY doesn't exist, then check the kvm parameter

...instead of always checking the parameter as you are currently doing.

What about this change? :-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 4902694129f9..9a6044e41a82 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2055,13 +2055,34 @@ int kvm_arch_pre_create_vcpu(CPUState *cpu, Error **errp)
          * behavior on Intel platform because current "pmu" property works
          * as expected.
          */
-        if (has_pmu_cap && !X86_CPU(cpu)->enable_pmu) {
-            ret = kvm_vm_enable_cap(kvm_state, KVM_CAP_PMU_CAPABILITY, 0,
-                                    KVM_PMU_CAP_DISABLE);
-            if (ret < 0) {
-                error_setg_errno(errp, -ret,
-                                 "Failed to set KVM_PMU_CAP_DISABLE");
-                return ret;
+        if (has_pmu_cap) {
+            if (!X86_CPU(cpu)->enable_pmu) {
+                ret = kvm_vm_enable_cap(kvm_state, KVM_CAP_PMU_CAPABILITY, 0,
+                                        KVM_PMU_CAP_DISABLE);
+                if (ret < 0) {
+                    error_setg_errno(errp, -ret,
+                                     "Failed to set KVM_PMU_CAP_DISABLE");
+                    return ret;
+                }
+            }
+        } else {
+            /*
+             * KVM_CAP_PMU_CAPABILITY is introduced in Linux v5.18. For old linux,
+             * we have to check enable_pmu parameter for vPMU support.
+             */
+            g_autofree char *kvm_enable_pmu;
+
+            /*
+             * The kvm.enable_pmu's permission is 0444. It does not change until a
+             * reload of the KVM module.
+             */
+            if (g_file_get_contents("/sys/module/kvm/parameters/enable_pmu",
+                &kvm_enable_pmu, NULL, NULL)) {
+                if (*kvm_enable_pmu == 'N' && !X86_CPU(cpu)->enable_pmu) {
+                    error_setg(errp, "Failed to enable PMU since "
+                               "KVM's enable_pmu parameter is disabled");
+                    return -1;
+                }
             }
         }
     }

---

This example not only eliminates the static variable “kvm_pmu_disabled”,
but also explicitly informs the user that vPMU is not available and
QEMU's "pmu" option doesn't work.

As a comparison, your patch 8 actually "silently" disables PMU (in the
kvm_init_pmu_info()) and user can only find it in Guest through PMU
exceptions.

Thanks,
Zhao