Received-SPF: pass (google.com: domain of linux-kernel+bounces-16818-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <3d8f5987-e09c-4dd2-a9c0-8ba22c9e948a@paulmck-laptop>
 <88f49775-2b56-48cc-81b8-651a940b7d6b@paulmck-laptop> <ZZX6pkHnZP777DVi@google.com>
 <77d7a3e3-f35e-4507-82c2-488405b25fa4@paulmck-laptop> <c6d5dd6e-2dec-423c-af39-213f17b1a9db@paulmck-laptop>
In-Reply-To: <c6d5dd6e-2dec-423c-af39-213f17b1a9db@paulmck-laptop>
From: Paolo Bonzini <pbonzini@redhat.com>
Date: Thu, 4 Jan 2024 15:59:52 +0100
Message-ID: <CABgObfYG-ZwiRiFeGbAgctLfj7+PSmgauN9RwGMvZRfxvmD_XQ@mail.gmail.com>
Subject: Re: [BUG] Guest OSes die simultaneously (bisected)
To: paulmck@kernel.org
Cc: Sean Christopherson <seanjc@google.com>, Like Xu <like.xu@linux.intel.com>, 
	Andi Kleen <ak@linux.intel.com>, Kan Liang <kan.liang@linux.intel.com>, 
	Luwei Kang <luwei.kang@intel.com>, Peter Zijlstra <peterz@infradead.org>, 
	linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org, 
	kvm@vger.kernel.org, Breno Leitao <leitao@debian.org>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Ingo Molnar <mingo@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Jan 4, 2024 at 3:58=E2=80=AFPM Paul E. McKenney <paulmck@kernel.org=
> wrote:
>
> On Wed, Jan 03, 2024 at 05:00:35PM -0800, Paul E. McKenney wrote:
> > On Wed, Jan 03, 2024 at 04:24:06PM -0800, Sean Christopherson wrote:
> > > On Wed, Jan 03, 2024, Paul E. McKenney wrote:
> > > > On Wed, Jan 03, 2024 at 02:22:23PM -0800, Paul E. McKenney wrote:
> > > > > Hello!
> > > > >
> > > > > Since some time between v5.19 and v6.4, long-running rcutorture t=
ests
> > > > > would (rarely but intolerably often) have all guests on a given h=
ost die
> > > > > simultaneously with something like an instruction fault or a segm=
entation
> > > > > violation.
> > > > >
> > > > > Each bisection step required 20 hosts running 10 hours each, and
> > > > > this eventually fingered commit c59a1f106f5c ("KVM: x86/pmu: Add
> > > > > IA32_PEBS_ENABLE MSR emulation for extended PEBS").  Although thi=
s commit
> > > > > is certainly messing with things that could possibly cause all ma=
nner
> > > > > of mischief, I don't immediately see a smoking gun.  Except that =
the
> > > > > commit prior to this one is rock solid.
> > > > > Just to make things a bit more exciting, bisection in mainline pr=
oved
> > > > > to be problematic due to bugs of various kinds that hid this one.=
  I was
> > > > > therefore forced to bisect among the commits backported to the in=
ternal
> > > > > v5.19-based kernel, which fingered the backported version of the =
patch
> > > > > called out above.
> > > >
> > > > Ah, and so why do I believe that this is a problem in mainline rath=
er
> > > > than just (say) a backporting mistake?
> > > >
> > > > Because this issue was first located in v6.4, which already has thi=
s
> > > > commit included.
> > > >
> > > >                                                   Thanx, Paul
> > > >
> > > > > Please note that this is not (yet) an emergency.  I will just con=
tinue
> > > > > to run rcutorture on v5.19-based hypervisors in the meantime.
> > > > >
> > > > > Any suggestions for debugging or fixing?
> > >
> > > This looks suspect:
> > >
> > > +       u64 pebs_mask =3D cpuc->pebs_enabled & x86_pmu.pebs_capable;
> > > +       int global_ctrl, pebs_enable;
> > >
> > > -       arr[0].msr =3D MSR_CORE_PERF_GLOBAL_CTRL;
> > > -       arr[0].host =3D intel_ctrl & ~cpuc->intel_ctrl_guest_mask;
> > > -       arr[0].guest =3D intel_ctrl & ~cpuc->intel_ctrl_host_mask;
> > > -       arr[0].guest &=3D ~(cpuc->pebs_enabled & x86_pmu.pebs_capable=
);
> > > -       *nr =3D 1;
> > > +       *nr =3D 0;
> > > +       global_ctrl =3D (*nr)++;
> > > +       arr[global_ctrl] =3D (struct perf_guest_switch_msr){
> > > +               .msr =3D MSR_CORE_PERF_GLOBAL_CTRL,
> > > +               .host =3D intel_ctrl & ~cpuc->intel_ctrl_guest_mask,
> > > +               .guest =3D intel_ctrl & (~cpuc->intel_ctrl_host_mask =
| ~pebs_mask),
> > > +       };
> > >
> > >
> > > IIUC (always a big if with this code), the intent is that the guest's=
 version of
> > > PERF_GLOBAL_CTRL gets bits that are (a) not exclusive to the host and=
 (b) not
> > > being used for PEBS.  (b) is necessary because PEBS generates records=
 in memory
> > > using virtual addresses, i.e. the CPU will write to memory using a vi=
rtual address
> > > that is valid for the host but not the guest.  And so PMU counters th=
at are
> > > configured to generate PEBS records need to be disabled while running=
 the guest.
> > >
> > > Before that commit, the logic was:
> > >
> > >   guest[PERF_GLOBAL_CTRL] =3D ctrl & ~host;
> > >   guest[PERF_GLOBAL_CTRL] &=3D ~pebs;
> > >
> > > But after, it's now:
> > >
> > >   guest[PERF_GLOBAL_CTRL] =3D ctrl & (~host | ~pebs);
> > >
> > > I.e. the kernel is enabled counters in the guest that are not host-on=
ly OR not
> > > PEBS.  E.g. if only counter 0 is in use, it's using PEBS, but it's no=
t exclusive
> > > to the host, then the new code will yield (truncated to a single byte=
 for sanity)
> > >
> > >   1 =3D 1 & (0xf | 0xe)
> > >
> > > and thus keep counter 0 enabled, whereas the old code would yield
> > >
> > >   1 =3D 1 & 0xf
> > >   0 =3D 1 & 0xe
> > >
> > > A bit of a shot in the dark and completed untested, but I think this =
is the correct
> > > fix?
> >
> > I am firing off some tests, and either way, thank you very much!!!
>
> Woo-hoo!!!  ;-)
>
> Tested-by: Paul E. McKenney <paulmck@kernel.org>
>
> Will you be sending a proper patch, or would you prefer that I do so?
> In the latter case, I would need your Signed-off-by.

I will fast track this one to Linus.

Paolo

> And again, thank you very much!!!
>
>                                                         Thanx, Paul
>
> > > diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/cor=
e.c
> > > index a08f794a0e79..92d5a3464cb2 100644
> > > --- a/arch/x86/events/intel/core.c
> > > +++ b/arch/x86/events/intel/core.c
> > > @@ -4056,7 +4056,7 @@ static struct perf_guest_switch_msr *intel_gues=
t_get_msrs(int *nr, void *data)
> > >         arr[global_ctrl] =3D (struct perf_guest_switch_msr){
> > >                 .msr =3D MSR_CORE_PERF_GLOBAL_CTRL,
> > >                 .host =3D intel_ctrl & ~cpuc->intel_ctrl_guest_mask,
> > > -               .guest =3D intel_ctrl & (~cpuc->intel_ctrl_host_mask =
| ~pebs_mask),
> > > +               .guest =3D intel_ctrl & ~(cpuc->intel_ctrl_host_mask =
| pebs_mask),
> > >         };
> > >
> > >         if (!x86_pmu.pebs)
> > >
>