On 5/15/2025 8:37 AM, Sean Christopherson wrote:
This is not an optimization in any sane interpretation of that word.
Yes, maybe clean up or bug fix is more accurate.
On Mon, Mar 24, 2025, Mingwei Zhang wrote:
From: Dapeng Mi dapeng1.mi@linux.intel.com
Currently pmu->global_ctrl is initialized in the common kvm_pmu_refresh() helper since both Intel and AMD CPUs set enable bits for all GP counters for PERF_GLOBAL_CTRL MSR. But it may be not the best place to initialize pmu->global_ctrl. Strictly speaking, pmu->global_ctrl is vendor specific
And? There's mounds of KVM code that show it's very, very easy to manage global_ctrl in common code.
The original intention is to put all initialization code into a same place, which looks more easily to maintain. But if you don't like it. would drop the change.
and there are lots of global_ctrl related processing in intel/amd_pmu_refresh() helpers, so better handle them in same place. Thus move pmu->global_ctrl initialization into intel/amd_pmu_refresh() helpers.
Besides, intel_pmu_refresh() doesn't handle global_ctrl_rsvd and global_status_rsvd properly and fix it.
Really? You mention a bug fix in passing, and squash it into an opinionated refactoring that is advertised as "optimizations" without even stating what the bug is? C'mon.
Sorry not clearly to describe the issue. global_ctrl_rsvd and global_status_rsvd should be updated only when pmu->verion >=2, but the original code doesn't.
Signed-off-by: Dapeng Mi dapeng1.mi@linux.intel.com Signed-off-by: Mingwei Zhang mizhang@google.com
arch/x86/kvm/pmu.c | 10 ------- arch/x86/kvm/svm/pmu.c | 14 +++++++-- arch/x86/kvm/vmx/pmu_intel.c | 55 ++++++++++++++++++------------------ 3 files changed, 39 insertions(+), 40 deletions(-)
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c index 4e8cefcce7ab..2ac4c039de8b 100644 --- a/arch/x86/kvm/pmu.c +++ b/arch/x86/kvm/pmu.c @@ -843,16 +843,6 @@ void kvm_pmu_refresh(struct kvm_vcpu *vcpu) return; kvm_pmu_call(refresh)(vcpu);
- /*
* At RESET, both Intel and AMD CPUs set all enable bits for general* purpose counters in IA32_PERF_GLOBAL_CTRL (so that software that* was written for v1 PMUs don't unknowingly leave GP counters disabled* in the global controls). Emulate that behavior when refreshing the* PMU so that userspace doesn't need to manually set PERF_GLOBAL_CTRL.*/- if (kvm_pmu_has_perf_global_ctrl(pmu) && pmu->nr_arch_gp_counters)
pmu->global_ctrl = GENMASK_ULL(pmu->nr_arch_gp_counters - 1, 0);Absolutely not, this code stays where it is.
Sure.
} void kvm_pmu_init(struct kvm_vcpu *vcpu) diff --git a/arch/x86/kvm/svm/pmu.c b/arch/x86/kvm/svm/pmu.c index 153972e944eb..eba086ef5eca 100644 --- a/arch/x86/kvm/svm/pmu.c +++ b/arch/x86/kvm/svm/pmu.c @@ -198,12 +198,20 @@ static void __amd_pmu_refresh(struct kvm_vcpu *vcpu) pmu->nr_arch_gp_counters = min_t(unsigned int, pmu->nr_arch_gp_counters, kvm_pmu_cap.num_counters_gp);
- if (pmu->version > 1) {
pmu->global_ctrl_rsvd = ~((1ull << pmu->nr_arch_gp_counters) - 1);
- if (kvm_pmu_cap.version > 1) {
It's not just global_ctrl. PEBS and the fixed counters also depend on v2+ (the SDM contradicts itself; KVM's ABI is that they're v2+).
/** At RESET, AMD CPUs set all enable bits for general purpose counters in* IA32_PERF_GLOBAL_CTRL (so that software that was written for v1 PMUs* don't unknowingly leave GP counters disabled in the global controls).* Emulate that behavior when refreshing the PMU so that userspace doesn't* need to manually set PERF_GLOBAL_CTRL.*/pmu->global_ctrl = BIT_ULL(pmu->nr_arch_gp_counters) - 1;pmu->global_ctrl_rsvd = ~pmu->global_ctrl;
- pmu->counter_bitmask[KVM_PMC_GP] = ((u64)1 << 48) - 1;
- pmu->counter_bitmask[KVM_PMC_GP] = BIT_ULL(48) - 1;
I like these cleanups, but they too belong in a separate patch.
Sure.
pmu->reserved_bits = 0xfffffff000280000ull; pmu->raw_event_mask = AMD64_RAW_EVENT_MASK; /* not applicable to AMD; but clean them to prevent any fall out */