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 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; }