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

[Dongli Zhang <dongli.zhang@oracle.com>]

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