On 5/17/2025 4:54 AM, Sean Christopherson wrote:
On Fri, May 16, 2025, Dapeng Mi wrote:
On 3/25/2025 1:31 AM, Mingwei Zhang wrote:
- if (kvm_mediated_pmu_enabled(pmu_to_vcpu(pmu))) {
bool allowed = check_pmu_event_filter(pmc);if (pmc_is_gp(pmc)) {if (allowed)pmc->eventsel_hw |= pmc->eventsel &ARCH_PERFMON_EVENTSEL_ENABLE;elsepmc->eventsel_hw &= ~ARCH_PERFMON_EVENTSEL_ENABLE;} else {int idx = pmc->idx - KVM_FIXED_PMC_BASE_IDX;if (allowed)pmu->fixed_ctr_ctrl_hw = pmu->fixed_ctr_ctrl;Sean, just found there is a potential bug here. The "pmu->fixed_ctr_ctrl_hw" should not be assigned to "pmu->fixed_ctr_ctrl" here, otherwise the other filtered fixed counter (not this allowed fixed counter) could be enabled accidentally.
diff --git a/arch/x86/kvm/pmu.c b/arch/x86/kvm/pmu.c index ba9d336f1d1d..f32e5f66f73b 100644 --- a/arch/x86/kvm/pmu.c +++ b/arch/x86/kvm/pmu.c @@ -473,7 +473,8 @@ static int reprogram_counter(struct kvm_pmc *pmc) int idx = pmc->idx - KVM_FIXED_PMC_BASE_IDX;
if (allowed) - pmu->fixed_ctr_ctrl_hw = pmu->fixed_ctr_ctrl; + pmu->fixed_ctr_ctrl_hw |= pmu->fixed_ctr_ctrl & + intel_fixed_bits_by_idx(idx, 0xf);
Hmm, I think that's fine, since pmu->fixed_ctr_ctrl should have changed? But I'd
Not exactly. Assume guest enables the fixed counter 0 and 1, then pmu->fixed_ctr_ctrl would be 0xbb. At the first time, user disables the fixed counter 0 by KVM event filter, then pmu->fixed_ctr_ctrl_hw would be 0xb0, secondly user disables the fixed counter 1 as well, so pmu->fixed_ctr_ctrl_hw is 0x0. At the third time, user re-enables fixed counter 1, so pmu->fixed_ctr_ctrl_hw is expected to be 0xb0, but it's not actually. Although the 1st calling (for fixed counter 0) to reprogram_counter() would disable it, but the 2nd calling (for fixed counter 1) to reprogram_counter() accidentally enables it (pmu->fixed_ctr_ctrl_hw becomes 0xbb eventually). This is incorrect.
rather play it safe and do (completely untested):
if (pmc_is_gp(pmc)) { pmc->eventsel_hw &= ~ARCH_PERFMON_EVENTSEL_ENABLE; if (allowed) pmc->eventsel_hw |= pmc->eventsel & ARCH_PERFMON_EVENTSEL_ENABLE; } else { u64 mask = intel_fixed_bits_by_idx(pmc->idx - KVM_FIXED_PMC_BASE_IDX, 0xf);
pmu->fixed_ctr_ctrl_hw &= ~mask; if (allowed) pmu->fixed_ctr_ctrl_hw |= pmu->fixed_ctr_ctrl & mask;}
The code looks good.