Re: [PATCH] KVM: x86/pmu: Fix type length error when reading pmu->fixed_ctr_ctrl

[Mingwei Zhang]

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?