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 <[email protected]>
>> 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) {
BTW, may I assume you meant:
if (*kvm_enable_pmu == 'N' && X86_CPU(cpu)->enable_pmu) {
not
if (*kvm_enable_pmu == 'N' && !X86_CPU(cpu)->enable_pmu) {
That is, return error because the QEMU isn't able to enable vPMU, because
of the kernel module configuration.
> + 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.
As replied in PATCH 08, we may still need a static variable
"kvm_pmu_disabled", in order to tell if we need to reset PMU registers when:
- X86_CPU(cpu)->enable_pmu = false.
- KVM_CAP_PMU_CAPABILITY returns 0.
If (kvm.enable_pmu=N)
It is safe to skip PMU registers' reset
Otherwise
We cannot skip reset.
Dongli Zhang
>
> Thanks,
> Zhao
>
>
