On Thu, Feb 01, 2024, Mingwei Zhang wrote: > On Thu, Feb 1, 2024 at 11:36 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > On Thu, Feb 01, 2024, Mingwei Zhang wrote: > > > On Thu, Feb 01, 2024, Sean Christopherson wrote: > > > > On Wed, Jan 31, 2024, Mingwei Zhang wrote: > > > > > > The PMC is still active while the VM side handle_pmi_common() is not going to handle it? > > > > > > > > > > hmm, so the new value is '0', but the old value is non-zero, KVM is > > > > > supposed to zero out (stop) the fix counter), but it skips it. This > > > > > leads to the counter continuously increasing until it overflows, but > > > > > guest PMU thought it had disabled it. That's why you got this warning? > > > > > > > > No, that can't happen, and KVM would have a massive bug if that were the case. > > > > The truncation can _only_ cause bits to disappear, it can't magically make bits > > > > appear, i.e. the _only_ way this can cause a problem is for KVM to incorrectly > > > > think a PMC is being disabled. > > > > > > The reason why the bug does not happen is because there is global > > > control. So disabling a counter will be effectively done in the global > > > disable part, ie., when guest PMU writes to MSR 0x38f. > > > > > > > > fixed PMC is disabled. KVM will pause the counter in reprogram_counter(), and > > > > then leave the perf event paused counter as pmc_event_is_allowed() will return > > > > %false due to the PMC being locally disabled. > > > > > > > > But in this case, _if_ the counter is actually enabled, KVM will simply reprogram > > > > the PMC. Reprogramming is unnecessary and wasteful, but it's not broken. > > > > > > no, if the counter is actually enabled, but then it is assigned to > > > old_fixed_ctr_ctrl, the value is truncated. When control goes to the > > > check at the time of disabling the counter, KVM thinks it is disabled, > > > since the value is already truncated to 0. So KVM will skip by saying > > > "oh, the counter is already disabled, why reprogram? No need!". > > > > Ooh, I had them backwards. KVM can miss 1=>0, but not 0=>1. I'll apply this > > for 6.8; does this changelog work for you? > > > > Use a u64 instead of a u8 when taking a snapshot of pmu->fixed_ctr_ctrl > > when reprogramming fixed counters, as truncating the value results in KVM > > thinking all fixed counters, except counter 0, are already disabled. As > > a result, if the guest disables a fixed counter, KVM will get a false > > negative and fail to reprogram/disable emulation of the counter, which can > > leads to spurious PMIs in the guest. > > That works for me. Maybe scoping that to the guest VMs with PerfMon v1 enabled? No, because from a purely architectural perspective, the bug isn't limited to VMs without PERF_GLOBAL_CTRL. Linux may always clear the associated enable bit in PERF_GLOBAL_CTRL, but that's not a hard requirement, a guest could choose to always leave bits set in PERF_GLOBAL_CTRL and instead use IA32_FIXED_CTR_CTRL to toggle PMCs on and off. Each enable bit in MSR_PERF_GLOBAL_CTRL is AND’ed with the enable bits for all privilege levels in the respective IA32_PERFEVTSELx or IA32_FIXED_CTR_CTRL MSRs to start/stop the counting of respective counters. Counting is enabled if the AND’ed results is true; counting is disabled when the result is false. I'm not saying that such guests are likely to show up in the wild, but I don't want to make any assumptions about what the guest does or does not do when it comes to making statements about the impact of bugs.
