| Message ID | 20250331013307.11937-8-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | target/i386/kvm/pmu: PMU Enhancement, Bugfix and Cleanup | expand |
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; } } }
Hi Zhao, On 4/9/25 10:05 PM, Zhao Liu wrote: > 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; > } > } > } > Thank you very much! I will split based on (enable_pmu) and (!enable_pmu) following your suggestion. Dongli Zhang
Hi Zhao, On 4/9/25 10:05 PM, Zhao Liu wrote: > 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; Base on QA of v4 patchset, since we are not going to exit with an error (-EPERM), I will need to bring back the global variable as in v2: kvm_pmu_disabled. https://lore.kernel.org/all/20250302220112.17653-8-dongli.zhang@oracle.com/ Because we don't exit with error, I need kvm_pmu_disabled in PATCH 08 to determine whether to skip the PMU info initialization, i.e.: - +pmu - enable_pmu=N In this case, we don't need to initialize pmu_version or num_pmu_gp_counters. Thank you very much! Dongli Zhang
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index 6b49549f1b..f68d5a0578 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -2051,13 +2051,35 @@ int kvm_arch_pre_create_vcpu(CPUState *cpu, Error **errp) * behavior on Intel platform because current "pmu" property works * as expected. */ - 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; + 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. + */ + 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; + } } } }
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. To help determine the scenario, read the kvm.enable_pmu value from the sysfs module parameter. There isn't any requirement to initialize 'pmu_version', 'num_pmu_gp_counters' or 'num_pmu_fixed_counters', if kvm.enable_pmu=N. In addition, return error when kvm.enable_pmu=N but the user wants to enable vPMU. Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- Changed since v2: - Rework the code flow following Zhao's suggestion. - Return error when: (*kvm_enable_pmu == 'N' && X86_CPU(cpu)->enable_pmu) target/i386/kvm/kvm.c | 36 +++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-)