On Fri, Feb 02, 2024, Xiong Y Zhang wrote: > > > On 2/2/2024 3:36 AM, Sean Christopherson 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, > each counter has four bits in fixed_ctr_ctrl, here u8 could cover counter 0 > and counter 1, so "except counter 0" can be modified to "except counter 0 and > 1" Ugh, math. I'll adjust it to: 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 fixed counter 2 is already disabled (the bug also affects fixed counters 3+, but KVM doesn't yet support those). As a result, if the guest disables fixed counter 2, KVM will get a false negative and fail to reprogram/disable emulation of the counter, which can leads to incorrect counts and spurious PMIs in the guest. Thanks!
