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

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

Hi Zhao,

On 3/9/25 11:14 PM, Zhao Liu wrote:
> 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.
> 

Thank you very much!

I will change the code following your suggestion.

Dongli Zhang