On 5/16/2025 3:22 AM, Sean Christopherson wrote:
On Thu, May 15, 2025, Dapeng Mi wrote:
On 5/15/2025 8:37 AM, Sean Christopherson wrote:
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) {
ARGH!!!!! I saw the pmu->version => kvm_pmu_cap.version change when going through this patch, but assumed it was simply a refactoring bug and ignored it.
Nope, 100% intentional, as I discovered after spending the better part of an hour debugging. Stuffing pmu->global_ctrl when it doesn't exist is necessary when the mediated PMU is enabled, because pmu->global_ctrl will always be loaded in hardware. And so loading '0' means the PMU is effectively disabled, because KVM disallows the guest from writing the MSR.
_That_ is the type of thing that absolutely needs a comment and a lengthy explanation in the changelog.
Yes, this change is intended. As long as HW supports Global_CTRL MSR, KVM should write available value into it at vm-entry regardless of if guest PMU has GLOBAL_CTRL (pmu version < 2), otherwise guest counters won't be really enabled on HW.
yeah, it's indeed easily confused and should put a comment here. My fault ...
Intel also needs the same treatment for PMU v1. And since there's no hackery that I can see, that suggests PMU v1 guests haven't been tested with the mediated PMU on Intel.
I guess congratulations are in order, because this patch just became my new goto example of why I'm so strict about on the "one thing per patch" rule.
Embarrassing ....
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;