Re: port-i386/57197: GENERIC kernel crash on pentium-III and earlier CPUs

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

The following reply was made to PR port-i386/57197; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: jdbaker%consolidated.net@localhost, uwe%NetBSD.org@localhost
Cc: gnats-bugs%netbsd.org@localhost, port-i386-maintainer%netbsd.org@localhost,
	gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
Subject: Re: port-i386/57197: GENERIC kernel crash on pentium-III and earlier CPUs
Date: Tue, 24 Jan 2023 10:57:49 +0000

 This is a multi-part message in MIME format.
 --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
 
 Can you try the attached patch?
 
 --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG
 Content-Type: text/plain; charset="ISO-8859-1"; name="x86clockintr"
 Content-Transfer-Encoding: quoted-printable
 Content-Disposition: attachment; filename="x86clockintr.patch"
 
 From e20d2a498a991899ab794174c5fcae888cbc84a4 Mon Sep 17 00:00:00 2001
 From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
 Date: Tue, 24 Jan 2023 10:00:45 +0000
 Subject: [PATCH] x86/intr: Work around sleazy clockintr with a secret frame
  argument.
 
 PR kern/57197
 ---
  sys/arch/x86/include/intr_private.h | 39 +++++++++++++++++++++++++++++
  sys/arch/x86/isa/clock.c            | 14 +++++++----
  sys/arch/x86/x86/intr.c             | 16 ++++++++++--
  3 files changed, 62 insertions(+), 7 deletions(-)
  create mode 100644 sys/arch/x86/include/intr_private.h
 
 diff --git a/sys/arch/x86/include/intr_private.h b/sys/arch/x86/include/int=
 r_private.h
 new file mode 100644
 index 000000000000..183e904a7dba
 --- /dev/null
 +++ b/sys/arch/x86/include/intr_private.h
 @@ -0,0 +1,39 @@
 +/*	$NetBSD$	*/
 +
 +/*-
 + * Copyright (c) 2023 The NetBSD Foundation, Inc.
 + * All rights reserved.
 + *
 + * Redistribution and use in source and binary forms, with or without
 + * modification, are permitted provided that the following conditions
 + * are met:
 + * 1. Redistributions of source code must retain the above copyright
 + *    notice, this list of conditions and the following disclaimer.
 + * 2. Redistributions in binary form must reproduce the above copyright
 + *    notice, this list of conditions and the following disclaimer in the
 + *    documentation and/or other materials provided with the distribution.
 + *
 + * THIS SOFTWARE IS PROVIDED BY THE NETBSD FOUNDATION, INC. AND CONTRIBUTO=
 RS
 + * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIM=
 ITED
 + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICU=
 LAR
 + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE FOUNDATION OR CONTRIBUTO=
 RS
 + * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
 + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
 + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
 + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
 + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
 + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF =
 THE
 + * POSSIBILITY OF SUCH DAMAGE.
 + */
 +
 +#ifndef	_X86_INTR_PRIVATE_H_
 +#define	_X86_INTR_PRIVATE_H_
 +
 +/*
 + * XXX This is a horrible kludge to let intr_establish_xname detect
 + * when it needs to handle a sleazy extra argument to the interrupt
 + * handler that's not part of the normal interrupt handler signature.
 + */
 +int i8254_clockintr(void *, struct intrframe *);
 +
 +#endif	/* _X86_INTR_PRIVATE_H_ */
 diff --git a/sys/arch/x86/isa/clock.c b/sys/arch/x86/isa/clock.c
 index c50704cd13a8..399bbd46c6ed 100644
 --- a/sys/arch/x86/isa/clock.c
 +++ b/sys/arch/x86/isa/clock.c
 @@ -152,6 +152,7 @@ __KERNEL_RCSID(0, "$NetBSD: clock.c,v 1.39 2020/05/29 1=
 2:30:41 rin Exp $");
  #include <x86/lock.h>
  #include <machine/specialreg.h>
  #include <x86/rtc.h>
 +#include <x86/intr_private.h>
 =20
  #ifndef __x86_64__
  #include "mca.h"
 @@ -188,8 +189,6 @@ void (*x86_delay)(unsigned int) =3D i8254_delay;
  void		sysbeep(int, int);
  static void     tickle_tc(void);
 =20
 -static int	clockintr(void *, struct intrframe *);
 -
  int 		sysbeepdetach(device_t, int);
 =20
  static unsigned int	gettick_broken_latch(void);
 @@ -371,8 +370,8 @@ tickle_tc(void)
 =20
  }
 =20
 -static int
 -clockintr(void *arg, struct intrframe *frame)
 +int
 +i8254_clockintr(void *arg, struct intrframe *frame)
  {
  	tickle_tc();
 =20
 @@ -555,9 +554,14 @@ i8254_initclocks(void)
  	/*
  	 * XXX If you're doing strange things with multiple clocks, you might
  	 * want to keep track of clock handlers.
 +	 *
 +	 * XXX This is an abuse of the interrupt handler signature with
 +	 * __FPTRCAST which requires a special case for IPL_CLOCK in
 +	 * intr_establish_xname.  Please fix this nonsense!  See also
 +	 * the comments about i8254_clockintr in x86/x86/intr.c.
  	 */
  	(void)isa_intr_establish(NULL, 0, IST_PULSE, IPL_CLOCK,
 -	    __FPTRCAST(int (*)(void *), clockintr), 0);
 +	    __FPTRCAST(int (*)(void *), i8254_clockintr), 0);
  }
 =20
  void
 diff --git a/sys/arch/x86/x86/intr.c b/sys/arch/x86/x86/intr.c
 index 5bde34cf7514..54474897377a 100644
 --- a/sys/arch/x86/x86/intr.c
 +++ b/sys/arch/x86/x86/intr.c
 @@ -162,6 +162,8 @@ __KERNEL_RCSID(0, "$NetBSD: intr.c,v 1.163 2022/10/29 1=
 3:59:04 riastradh Exp $")
  #include <machine/i8259.h>
  #include <machine/pio.h>
 =20
 +#include <x86/intr_private.h>
 +
  #include "ioapic.h"
  #include "lapic.h"
  #include "pci.h"
 @@ -944,11 +946,21 @@ intr_establish_xname(int legacy_irq, struct pic *pic,=
  int pin, int type,
  	ih->ih_slot =3D slot;
  	strlcpy(ih->ih_xname, xname, sizeof(ih->ih_xname));
  #ifdef KDTRACE_HOOKS
 -	ih->ih_fun =3D intr_kdtrace_wrapper;
 -	ih->ih_arg =3D ih;
 +	/*
 +	 * XXX i8254_clockintr is special -- takes a magic extra
 +	 * argument.  This should be fixed properly in some way that
 +	 * doesn't involve sketchy function pointer casts.  See also
 +	 * the comments in x86/isa/clock.c.
 +	 */
 +	if (handler !=3D __FPTRCAST(int (*)(void *), i8254_clockintr)) {
 +		ih->ih_fun =3D intr_kdtrace_wrapper;
 +		ih->ih_arg =3D ih;
 +	}
  #endif
  #ifdef MULTIPROCESSOR
  	if (!mpsafe) {
 +		KASSERT(handler !=3D			/* XXX */
 +		    __FPTRCAST(int (*)(void *), i8254_clockintr));
  		ih->ih_fun =3D intr_biglock_wrapper;
  		ih->ih_arg =3D ih;
  	}
 
 --=_La7nkjIJilZyzQ04m7iYdJZjSuvrF8DG--