kern/98460 : [kernel] [patch] fpu_clean_state() cannot be disabled for not AMD processors, those are not vulnerable to FreeBSD-SA-06:14.fpu

[Bruce Evans]

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

From: Bruce Evans <bde at zeta.org.au>
To: Rostislav Krasny <rosti.bsd at gmail.com>
Cc: freebsd-gnats-submit at freebsd.org
Subject: Re: kern/98460 : [kernel] [patch] fpu_clean_state() cannot be disabled
 for not AMD processors, those are not vulnerable to FreeBSD-SA-06:14.fpu
Date: Sat, 10 Jun 2006 11:26:20 +1000 (EST)

 On Fri, 9 Jun 2006, Rostislav Krasny wrote:
 
 > On Wed, 7 Jun 2006 12:09:10 +1000 (EST)
 > Bruce Evans <bde at zeta.org.au> wrote:
 >
 >>> And then you want to call the fpu_clean_state() function conditionally,
 >>> like in following example?
 >>>
 >>> if (cpu_fxsr & CPU_FXSR_NEEDCLEAN)
 >>>        fpu_clean_state();
 >>
 >> Not quite like that.  In my version there is no function call -- the code
 >> is excecuted in the one place where it is needed, so there is no function
 >> call overhead or possible branch prediction oferhead for the function call.
 >
 > Could you please explain in more detail how that can be done?
 
 Just do it.  The easiest way is define the new function as inline.
 This just works because the function is defined before it is used.
 
 My version uses open-coded inlining.  This works because in -current
 the function is called twice, but only one of the calls is non-bogus,
 so open-coded inlining of the calls needed doesn't result in multiple
 copies of the code.  In the amd64 version, there are 2 explicit calls,
 with a bogus one in npx_getregs() that just cleans the state already
 owned by the thread.  In the i386 version, there is one call a low-level
 function that is called twice.  (If we really cared about efficiency,
 then we might inline this function too, but it it would be even better
 handle the differences between the fxsr and non-fxsr cases at a higher
 level.)
 
 Here is my version using open-coded inlining.
 
 % Index: npx.c
 % ===================================================================
 % RCS file: /home/ncvs/src/sys/i386/isa/npx.c,v
 % retrieving revision 1.152
 % diff -u -1 -r1.152 npx.c
 % --- npx.c	19 Jun 2004 22:24:16 -0000	1.152
 % +++ npx.c	22 Apr 2006 11:58:31 -0000
 % @@ -173,2 +176,3 @@
 % 
 % +static	float			npx_cleandata;
 %  static	union savefpu		npx_cleanstate;
 
 This also fixes a type mismatch and style bugs in the new variable.
 
 % @@ -796,13 +848,34 @@
 %  	PCPU_SET(fpcurthread, curthread);
 % -	pcb = PCPU_GET(curpcb);
 % 
 % +#ifdef CPU_ENABLE_SSE
 % +	/*
 % +	 * In the fxsr case, do a dummy load to set the last-instruction
 % +	 * pointers and opcode to constant (kernel) values in case
 % +	 * fpurstor() doesn't set them.  Certain AMD CPUs are too lazy
 % +	 * about saving and restoring them, so on these CPUs we have broken
 % +	 * context switching and would have a security hole if we didn't
 % +	 * force a setting here.  First, clear any pending exceptions to
 % +	 * ensure that the load doesn't trap (userland may have left us
 % +	 * with an unmasked pending exception since fxsave doesn't do an
 % +	 * implicit fninit).  The load itself will cause an exception if
 % +	 * userland has left us with a full stack; we let this happen
 % +	 * since it is harmless except for being much slower in the rare
 % +	 * case that it happens.
 % +	 */
 % +	if (cpu_fxsr /* && cpu_fxcsw_broken */) {
 % +		fnstsw(&status);
 % +		if (status & 0x80)
 % +			fnclex();
 % +		__asm("flds %0" : : "m" (npx_cleandata));
 % +	}
 % +#endif /* CPU_ENABLE_SSE */
 % +
 % +	pcb = PCPU_GET(curpcb);
 
 The inline function provides a better place to attach verbose comments.
 It has verbose comments that emphasize different details than the
 verbose comment above.  I would prefer to have less-verbose comments.
 
 The code in the above is identical with that in the function except it
 does the cpu_fxsr check more directly and it doesn't do the ffree (see
 the comment).
 
 Note that the fixup can probably be done much more optimally and/or
 simply by doing it in a less time-critical place than here.  The
 relevant CPUs have out-of-order exceution with the FPUs independent of
 the integer ALUs.  It might not matter that fnclex is slow or that the
 fixup takes any cycles at all, since it runs almost entirely on the
 FPUs and the FPUs can proceed independently.  Unfortunately, placing
 the fixup here almost certainly gives a bottleneck in the FPU instruction
 scheduling.  Userland has just tried to execute an FPU instruction,
 and cannot proceeed with at least that instruction until we have
 executed the fnstsw/[fnclex/]fld/fxrstor sequence here, and the sequence
 must be executed sequentially too.  Userland may be able to proceed
 with integer instructions but it is likely to stall on a dependency
 on the FPU instruction since the FPU instruction takes a long time for
 the trap here, and any addition to the time here is unlikely to be
 hidden by parallelism.  Placing the fixup after the fxsave on context
 switches would probably be more efficient.  On context switches, the
 kernel runs (or can be arranged to run) for a long time doing only
 integer operations after doing the floating point part of the switch,
 so an fxsave/fnstsw/[fnclex/]fld sequence can run in parallel.
 
 Parallel execution also affects the issue of whether FPU state for the
 switched-to thread should be loaded at context switch time instead of
 setting TS and only loading it (in the trap handler just below here) if
 they are used.  A direct load might be free since it can run in parallel.
 The trap handling certainly isn't free since it can't run in parallel.
 
 Fixes for an indirectly-related nearby bug (already fixed in the amd64
 version) that happen to be in the same patch:
 
 %  	if ((pcb->pcb_flags & PCB_NPXINITDONE) == 0) {
 %  		/*
 % -		 * This is the first time this thread has used the FPU or
 % -		 * the PCB doesn't contain a clean FPU state.  Explicitly
 % -		 * initialize the FPU and load the default control word.
 % +		 * This is the first time this thread has used the FPU, or
 % +		 * the PCB doesn't contain a clean FPU state.  Load the
 % +		 * initial state (XXX reword more).
 %  		 */
 % -		fninit();
 % -		control = __INITIAL_NPXCW__;
 % -		fldcw(&control);
 % +		fpurstor(&npx_cleanstate);
 %  		pcb->pcb_flags |= PCB_NPXINITDONE;
 
 Bruce