Index | Thread | Search

From:
Mark Kettenis <mark.kettenis@xs4all.nl>
Subject:
Re: amd64/fpu: Avoid multiple FPU resets
To:
Claudio Jeker <cjeker@diehard.n-r-g.com>
Cc:
deraadt@openbsd.org, guenther@gmail.com, cludwig@genua.de, tech@openbsd.org
Date:
Sun, 15 Jun 2025 21:06:34 +0200

Download raw body.

Thread
> Date: Sun, 15 Jun 2025 20:10:15 +0200
> From: Claudio Jeker <cjeker@diehard.n-r-g.com>
> 
> On Sun, Jun 15, 2025 at 06:35:43PM +0200, Mark Kettenis wrote:
> > > 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 does nothing. The code always runs at IPL_NONE or higher.
> splassert() always allows a higher SPL.

Well, yes, it does allow a higher SPL.  However, it does not allow a
higher IPL.  See arch/amd64/amd64/machdep.c:splassert_check().  That
second check isn't implemented on all architectures IIRC, but...

> 
> We need something else to replace all those bogus splassert(IPL_NONE);
> checks done.

...those checks aren't bogus.

> > 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().
> > 
> 
> -- 
> :wq Claudio
>