Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: amd64/fpu: Avoid multiple FPU resets
To:
Mark Kettenis <mark.kettenis@xs4all.nl>
Cc:
deraadt@openbsd.org, guenther@gmail.com, cludwig@genua.de, tech@openbsd.org
Date:
Mon, 16 Jun 2025 17:55:29 +0200

Download raw body.

Thread
> Date: Sun, 15 Jun 2025 18:35:43 +0200
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
> 
> > From: "Theo de Raadt" <deraadt@openbsd.org>
> > Date: Sun, 15 Jun 2025 10:16:57 -0600
> > 
> > Philip Guenther <guenther@gmail.com> wrote:
> > 
> > > Okay, then I think the original diff is correct, though maybe we add
> > > the assertwaitok() to fpu_kernel_enter() so we remain confident of
> > > that.
> > 
> > If an assert triggers at this point, in drm code, it is unlikely that
> > a user will see it.  I expect it will deadlock the kernel hard in a
> > very invisible fashion...
> 
> Well, assertwaitok() does a few checks.  The first one is:
> 
>     splassert(IPL_NONE);
> 
> This is the check we want to make sure that we're not called from
> interrupt context.  By default, this just prints a warning message,
> which shouldn't cause a deadlock.
> 
> There are other checks that may trigger a panic and therefore may
> appear to the user as a deadlock.  So I think it is better to just add
> an splassert(IPL_NONE) into fpu_kernel_enter().

So like this.  Should this go into snaps?  Should I just commit it?


Index: arch/amd64/amd64/fpu.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/fpu.c,v
diff -u -p -r1.44 fpu.c
--- arch/amd64/amd64/fpu.c	22 May 2023 00:39:57 -0000	1.44
+++ arch/amd64/amd64/fpu.c	16 Jun 2025 15:51:15 -0000
@@ -146,6 +146,8 @@ fpu_kernel_enter(void)
 {
 	struct cpu_info *ci = curcpu();
 
+	splassert(IPL_NONE);
+
 	/* save curproc's FPU state if we haven't already */
 	if (ci->ci_pflags & CPUPF_USERXSTATE) {
 		ci->ci_pflags &= ~CPUPF_USERXSTATE;