Re: kern/57878: i915 calls agp_i810_chipset_flush with spin lock held but agp_i810_chipset_flush waits for xcall

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

The following reply was made to PR kern/57878; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: gnats-bugs%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost
Cc: 
Subject: Re: kern/57878: i915 calls agp_i810_chipset_flush with spin lock held but agp_i810_chipset_flush waits for xcall
Date: Fri, 26 Jan 2024 22:26:39 +0000

 This is a multi-part message in MIME format.
 --=_jGdl80rZzbJ3AV6nW+2j7DYaEAx46dnv
 
 Correction: need kpreempt_disable/enable around ipi(9) if we _don't_
 hold a spin lock when we call it.
 
 --=_jGdl80rZzbJ3AV6nW+2j7DYaEAx46dnv
 Content-Type: text/plain; charset="ISO-8859-1"; name="pr57878-agpcacheflushipi-v2"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="pr57878-agpcacheflushipi-v2.patch"
 
 From eac456a8881930d2b1f66a87850eaa13be51d093 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Thu, 25 Jan 2024 23:17:34 +0000
 Subject: [PATCH] agp_i810(4): Use ipi(9) for chipset flush on all CPUs, not
  xcall(9).
 
 i915 now calls into this with a spin lock held, so we have to use
 ipi(9), which spin-waits for the other CPUs to complete, rather than
 xcall(9), which may sleep-wait.
 
 Fortunately, this is just to execute WBINVD on x86 (and if this code
 ever runs on other architectures, which it probably doesn't, it'll be
 a similar barrier instruction), so spinning to wait for that on all
 CPUs isn't too costly.
 
 PR kern/57878
 
 XXX pullup-10
 ---
  sys/dev/pci/agp_i810.c | 18 +++++++++++++-----
  1 file changed, 13 insertions(+), 5 deletions(-)
 
 diff --git a/sys/dev/pci/agp_i810.c b/sys/dev/pci/agp_i810.c
 index adfb5716d4a3..961a3a5b077b 100644
 --- a/sys/dev/pci/agp_i810.c
 +++ b/sys/dev/pci/agp_i810.c
 @@ -180,7 +180,7 @@ agp_i810_post_gtt_entry(struct agp_i810_softc *isc, off=
 _t off)
  }
 =20
  static void
 -agp_flush_cache_xc(void *a __unused, void *b __unused)
 +agp_flush_cache_ipi(void *cookie __unused)
  {
 =20
  	agp_flush_cache();
 @@ -204,11 +204,19 @@ agp_i810_chipset_flush(struct agp_i810_softc *isc)
  		 * XXX Come to think of it, do these chipsets appear in
  		 * any multi-CPU systems?
  		 */
 -		if (cold)
 +		if (cold) {
  			agp_flush_cache();
 -		else
 -			xc_wait(xc_broadcast(0, &agp_flush_cache_xc,
 -				NULL, NULL));
 +		} else {
 +			/*
 +			 * Caller may hold a spin lock, so use ipi(9)
 +			 * rather than xcall(9) here.
 +			 */
 +			ipi_msg_t msg =3D { .func =3D agp_flush_cache_ipi };
 +			kpreempt_disable();
 +			ipi_broadcast(&msg, /*skip_self*/false);
 +			ipi_wait(&msg);
 +			kpreempt_enable();
 +		}
  		WRITE4(AGP_I830_HIC, READ4(AGP_I830_HIC) | __BIT(31));
  		while (ISSET(READ4(AGP_I830_HIC), __BIT(31))) {
  			if (timo-- =3D=3D 0)
 
 --=_jGdl80rZzbJ3AV6nW+2j7DYaEAx46dnv--