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.


freebsd-acpi@freebsd.org mailing list
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"

Reply via email to