Received-SPF: pass (google.com: domain of linux-kernel+bounces-48966-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 1 Feb 2024 14:53:28 -0800
In-Reply-To: <CAL715WJ_VT2E5bjgvC89Dk0j1Mft9PcGtEBkkAxkKMF0=+Uimw@mail.gmail.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240123221220.3911317-1-mizhang@google.com> <ZbpqoU49k44xR4zB@google.com>
 <368248d0-d379-23c8-dedf-af7e1e8d23c7@oracle.com> <CAL715WJDesggP0S0M0SWX2QaFfjBNdqD1j1tDU10Qxk6h7O0pA@mail.gmail.com>
 <ZbvUyaEypRmb2s73@google.com> <ZbvjKtsVjpuQmKE2@google.com>
 <ZbvyrvvZM-Tocza2@google.com> <CAL715WJ_VT2E5bjgvC89Dk0j1Mft9PcGtEBkkAxkKMF0=+Uimw@mail.gmail.com>
Message-ID: <Zbwg6LlRrkq8jk9Q@google.com>
Subject: Re: [PATCH] KVM: x86/pmu: Fix type length error when reading pmu->fixed_ctr_ctrl
From: Sean Christopherson <seanjc@google.com>
To: Mingwei Zhang <mizhang@google.com>
Cc: Dongli Zhang <dongli.zhang@oracle.com>, Paolo Bonzini <pbonzini@redhat.com>, 
	"H. Peter Anvin" <hpa@zytor.com>, kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 01, 2024, Mingwei Zhang wrote:
> On Thu, Feb 1, 2024 at 11:36=E2=80=AFAM Sean Christopherson <seanjc@googl=
e.com> 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() i=
s 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. Th=
is
> > > > > leads to the counter continuously increasing until it overflows, =
but
> > > > > guest PMU thought it had disabled it. That's why you got this war=
ning?
> > > >
> > > > No, that can't happen, and KVM would have a massive bug if that wer=
e the case.
> > > > The truncation can _only_ cause bits to disappear, it can't magical=
ly 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 globa=
l
> > > disable part, ie., when guest PMU writes to MSR 0x38f.
> >
> >
> > > > fixed PMC is disabled. KVM will pause the counter in reprogram_coun=
ter(), 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 si=
mply reprogram
> > > > the PMC.  Reprogramming is unnecessary and wasteful, but it's not b=
roken.
> > >
> > > 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=3D>0, but not 0=3D>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_ct=
rl
> >   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, whic=
h can
> >   leads to spurious PMIs in the guest.
>=20
> That works for me. Maybe scoping that to the guest VMs with PerfMon v1 en=
abled?

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 choos=
e to
always leave bits set in PERF_GLOBAL_CTRL and instead use IA32_FIXED_CTR_CT=
RL to
toggle PMCs on and off.

  Each enable bit in MSR_PERF_GLOBAL_CTRL is AND=E2=80=99ed with the enable=
 bits for all
  privilege levels in the respective IA32_PERFEVTSELx or IA32_FIXED_CTR_CTR=
L MSRs
  to start/stop the counting of respective counters. Counting is enabled if=
 the
  AND=E2=80=99ed results is true; counting is disabled when the result is f=
alse.

I'm not saying that such guests are likely to show up in the wild, but I do=
n'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.