Hi Dongli,
The logic is fine for me :-) And thank you to take my previous
suggestion. When I revisit here after these few weeks, I have some
thoughts:
> + if (pmu_cap) {
> + if ((pmu_cap & KVM_PMU_CAP_DISABLE) &&
> + !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;
> + }
> + }
This case enhances vPMU disablement.
> + } 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 -EPERM;
> + }
And this case checks if vPMU could enable.
> }
> }
> }
So I feel it's not good enough to check based on pmu_cap, we can
re-split it into these two cases: enable_pmu and !enable_pmu. Then we
can make the code path more clear!
Just like:
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index f68d5a057882..d728fb5eaec6 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2041,44 +2041,42 @@ int kvm_arch_pre_create_vcpu(CPUState *cpu, Error
**errp)
if (first) {
first = false;
- /*
- * Since Linux v5.18, KVM provides a VM-level capability to easily
- * disable PMUs; however, QEMU has been providing PMU property per
- * CPU since v1.6. In order to accommodate both, have to configure
- * the VM-level capability here.
- *
- * KVM_PMU_CAP_DISABLE doesn't change the PMU
- * behavior on Intel platform because current "pmu" property works
- * as expected.
- */
- if (pmu_cap) {
- if ((pmu_cap & KVM_PMU_CAP_DISABLE) &&
- !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.
- */
+ if (X86_CPU(cpu)->enable_pmu) {
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.
+ * The enable_pmu parameter is introduced since Linux v5.17,
+ * give a chance to provide more information about vPMU
+ * enablement.
+ *
+ * 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 "
+ if (*kvm_enable_pmu == 'N') {
+ warn_report("Failed to enable PMU since "
"KVM's enable_pmu parameter is disabled");
- return -EPERM;
+ }
+ }
+ } else {
+ /*
+ * Since Linux v5.18, KVM provides a VM-level capability to easily
+ * disable PMUs; however, QEMU has been providing PMU property per
+ * CPU since v1.6. In order to accommodate both, have to configure
+ * the VM-level capability here.
+ *
+ * KVM_PMU_CAP_DISABLE doesn't change the PMU
+ * behavior on Intel platform because current "pmu" property works
+ * as expected.
+ */
+ if ((pmu_cap & KVM_PMU_CAP_DISABLE)) {
+ 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;
}
}
}
