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

[Mingwei Zhang]

On Thu, Feb 1, 2024 at 2:53 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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.

I agree with you. We don't know what kind of guest we are serving. It
may not be Linux, it may not even be the perf subsystem in Linux. They
could choose to do whatever style that is allowed architecturally to
start/stop/reprogram the fixed counter.

Thanks.

-Mingwei