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
