Received-SPF: pass (google.com: domain of linux-kernel+bounces-55933-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 6 Feb 2024 20:28:24 -0800
In-Reply-To: <165f07deb8d1e082756e6cae21a25b0060c18f85.camel@infradead.org>
Precedence: bulk
Mime-Version: 1.0
References: <f21ee3bd852761e7808240d4ecaec3013c649dc7.camel@infradead.org>
 <ZcJ9bXxU_Pthq_eh@google.com> <19a1ac538e6cb1b479122df677909fb49fedbb28.camel@infradead.org>
 <ZcLxzrbvSs0jNeR4@google.com> <165f07deb8d1e082756e6cae21a25b0060c18f85.camel@infradead.org>
Message-ID: <ZcMG6NA63MqOR1V9@google.com>
Subject: Re: [PATCH v3] KVM: x86: Use fast path for Xen timer delivery
From: Sean Christopherson <seanjc@google.com>
To: David Woodhouse <dwmw2@infradead.org>, g@google.com
Cc: kvm <kvm@vger.kernel.org>, Paul Durrant <paul@xen.org>, 
	Paolo Bonzini <pbonzini@redhat.com>, Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>, 
	Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org, 
	"H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Feb 06, 2024, David Woodhouse wrote:
> On Tue, 2024-02-06 at 18:58 -0800, Sean Christopherson wrote:
> > On Tue, Feb 06, 2024, David Woodhouse wrote:
> > > On Tue, 2024-02-06 at 10:41 -0800, Sean Christopherson wrote:
> > > >=20
> > > > This has an obvious-in-hindsight recursive deadlock bug.=C2=A0 If K=
VM actually needs
> > > > to inject a timer IRQ, and the fast path fails, i.e. the gpc is inv=
alid,
> > > > kvm_xen_set_evtchn() will attempt to acquire xen.xen_lock, which is=
 already held
> > >=20
> > > Hm, right. In fact, kvm_xen_set_evtchn() shouldn't actually *need* th=
e
> > > xen_lock in an ideal world; it's only taking it in order to work arou=
nd
> > > the fact that the gfn_to_pfn_cache doesn't have its *own* self-
> > > sufficient locking. I have patches for that...
> > >=20
> > > I think the *simplest* of the "patches for that" approaches is just t=
o
> > > use the gpc->refresh_lock to cover all activate, refresh and deactiva=
te
> > > calls. I was waiting for Paul's series to land before sending that on=
e,
> > > but I'll work on it today, and double-check my belief that we can the=
n
> > > just drop xen_lock from kvm_xen_set_evtchn().
> >=20
> > While I definitely want to get rid of arch.xen.xen_lock, I don't want t=
o address
> > the deadlock by relying on adding more locking to the gpc code.=C2=A0 I=
 want a teeny
> > tiny patch that is easy to review and backport.=C2=A0 Y'all are *proabl=
y* the only
> > folks that care about Xen emulation, but even so, that's not a valid re=
ason for
> > taking a roundabout way to fixing a deadlock.
>=20
> I strongly disagree. I get that you're reticent about fixing the gpc
> locking, but what I'm proposing is absolutely *not* a 'roundabout way
> to fixing a deadlock'. The kvm_xen_set_evtchn() function shouldn't
> *need* that lock; it's only taking it because of the underlying problem
> with the gpc itself, which needs its caller to do its locking for it.
>=20
> The solution is not to do further gymnastics with the xen_lock.

I agree that's the long term solution, but I am not entirely confident that=
 a big
overhaul is 6.9 material at this point.  Squeezing an overhaul into 6.8 (an=
d if
we're being nitpicky, backporting to 6.7) is out of the question.