Revisiting FPU context resume on i386

[Bruce Evans]

On Thu, 18 Jul 2013, Konstantin Belousov wrote:

> On Thu, Jul 18, 2013 at 03:33:59PM +1000, Bruce Evans wrote:
>> On Wed, 17 Jul 2013, Konstantin Belousov wrote:
>>
>>> On Tue, Jul 16, 2013 at 06:54:57PM +1000, Bruce Evans wrote:
>>>> ISTR disagreeing with jkim on using a special save area.
>>> I believe the normal save area cannot be used there at all, since
>>> the suspension is async and fpu.c could perform some operation on
>>> the PCB-pointed save area while suspend IPI is received.
>>
>> If that happens, then suspension is broken anyway.  Any npx operation
> Yes, it is broken, but not for FPU.  For the fpu.c, the actions in the
> IPI handler and later in resume path are invisible.
>
>> on the pcb (or any other operation on the pcb) between savectx() and
>> resumectx() makes the state saved by savectx() out of date.  In normal
>> kernel operations, the npx state is changed from volatile to non-volatile
>> by pushing it to the pcb.  This prevents context switches from changing
>> it underneath you by doing the same push to the pcb.  (The state is not
>> really changed, but the place where it lives and pointers and flags that
>> say where it lives are changed.)  I think this works perfectly for
>> suspend/resume too.  It is less clear what happens for non-npx parts of
>> the pcb.  Hopefully there are no races for them (and the special save
>> area is not needed) because there is no lazy context switching for them
>> -- they live either in the CPU or the pcb, and are switched between the
>> CPU and the pcu atomically on every context switch.  Clearly context
>> switches must not be allowed between suspend and resume, or the resume
>> must be on the same thread as the suspend.  Otherwise resume restores
>> state for a wrong thread, which is a much larger bug than races and
>> inconsistencies in accessing separate save areas for the same thread.
> I do not understand what you described there.  Assume that e.g. the CPU
> was in fpudna() code and performs the bcopy() from fpu_initialstate
> when the suspend IPI was delivered.  Then saving the current HW state
> into the same PCB save area causes corruption.

Hmm.  The npx code was designed to be safe when interrupted (because
on i386's without exception 16, irq13 was used for npx traps and
protection against that protects against other interrupts too).  It
has rotted a bit, but still seems to be safe against preemption by
(maskable) IPIs except for that bcopy(), and where you changed the
interrupt disabling to critical sections, and perhaps in some of your
newer state-chaining code.  amd64 is not very different from i386,
except it misspells "npx" as "fpu".  The misspelling makes it hard to
refer to functions that act the same.  In the following, npx means
i386-only and NPX means either i386 or amd64.

   (You didn't change the comment about disabling interrupts before
   npxsave().  The comment still says that callers must disable
   interrupts, and on i386 the savectx() caller still disables
   interrupts.  I think the cpu_switch() caller uses a spinlock on
   amd64 and i386, so interrupts are disabled there too.  On amd64,
   ctx_fpusave() doesn't explicitly disable interrupts or use a
   critical section, but it doesn't need to since it doesn't change
   the state.)

All the old code except NPXgetregs() seems to be protected by either
a critical section or disabling interrupts.  NPXgetregs() uses a
critical section for the main part, but not for the special case which
does the bcopy() of the initial state.  This case assumes that it
cannot be interrupted.  This assumptiont was even correct for irq13
when irq13 was used, since irq13 can't happen before the npx is used.
Expand this critical section and change the critical sections back to
interrupts disabled and NPX should be almost interrupt-safe.

There seem to be some problems like the ones in NPXgetregs() in your
newer code.  For example, fpu_kern_enter() doesn't use a critical
section except when it calls NPXexit() (in the call).  fpu_kern_leave()
uses a critical section only around NPXdrop().  I had forgotten exactly
how much AVX support was in i386 and thought that it wasn't there yet,
since savectx() didn't't seem to do enough to support it.  It seems to
be all there.

Anyway, on i386 savectx() still always saves the npx state to the pcb
first, so the special save area doesn't help avoid the races for the
npx part of the pcb, and removing the copying via the pcb would fix
more than a style bug.  i386 seems to need a special npxsave() like
amd64, since the normal one has side effects on the pcb and pcpu even
when directed to write to a special save area.

Bruce
-

Bruce