| Message ID | 20250302220112.17653-8-dongli.zhang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/i386/kvm/pmu: PMU Enhancement, Bugfix and Cleanup | expand |
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. Thanks, Zhao
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
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) { 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 > >
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c index e895d22f94..efba3ae7a4 100644 --- a/target/i386/kvm/kvm.c +++ b/target/i386/kvm/kvm.c @@ -184,6 +184,10 @@ static int has_triple_fault_event; static bool has_msr_mcg_ext_ctl; static int has_pmu_cap; +/* + * Read from /sys/module/kvm/parameters/enable_pmu. + */ +static bool kvm_pmu_disabled; static struct kvm_cpuid2 *cpuid_cache; static struct kvm_cpuid2 *hv_cpuid_cache; @@ -3256,6 +3260,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s) int ret; struct utsname utsname; Error *local_err = NULL; + g_autofree char *kvm_enable_pmu; /* * Initialize SEV context, if required @@ -3401,6 +3406,17 @@ int kvm_arch_init(MachineState *ms, KVMState *s) has_pmu_cap = kvm_check_extension(s, KVM_CAP_PMU_CAPABILITY); + /* + * 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') { + kvm_pmu_disabled = true; + } + } + return 0; }
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 'has_pmu_version', 'num_pmu_gp_counters' or 'num_pmu_fixed_counters', if kvm.enable_pmu=N. Signed-off-by: Dongli Zhang <dongli.zhang@oracle.com> --- target/i386/kvm/kvm.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)