amd64/169927: siginfo, si_code for fpe errors when error occurs using the SSE math processor

[Konstantin Belousov]

On Wed, Jul 18, 2012 at 02:36:16PM +1000, Bruce Evans wrote:
> On Wed, 18 Jul 2012, Konstantin Belousov wrote:
> 
> >On Wed, Jul 18, 2012 at 10:32:23AM +1000, Bruce Evans wrote:
> >>OK.  Also, you can tell instruction that faulted, provided there are
> >>no spurious faults.  I think spurious faults can only occur for IRQ13
> >>exception handling which was never supported for amd64 and is no longer
> >>supported for i386.  My old test program that is broken by not clobbering
> >>the status was mainly for finding and fixing such spurious faults.
> >>I never owned an i387 or i486SX and had to break i486 exception handling
> >>to test IRQ13.
> >486SX+487 never used irq13 as well, since 487 was the 'full' DX package,
> >which turned off SX socket.
> 
> The 487 is still a separate package.  I thought it had to use IRQ13 to
> communicate with the 486SX since the NE# (?) and ERR# (?) pins only
> work for a single package.  Do they actually work for the separate
> package?
487 packaged the whole CPU, it is not add-on FPU like 387.

486SX was turned off (electrically, AFAIR) when 487 was installed.
> 
> >>Just noticed another style/efficiency bug: this should use curpcb,
> >>not curthread->td_pcb.  I don't like the curpcb global, but as
> >>long as it exists, it should be used.  Every time I looked at
> >>removing this global, it seemed better to keep it.  C code can
> >>easily use curthread->td_pcb instead of curpcb, and this is faster
> >>if curthread is cached.  But CURPCB is easier to use in asm.
> >I changed to use curpcb (its use is indeed mainly for asm context switch
> >code). I do want to use pcb_save though, see below.
> >>...
> >>I now quote your newer patch for the fputrap_x87() cleanup:
> >>
> >>>@@ -531,8 +534,9 @@ static char fpetable[128] = {
> >>>  * solution for signals other than SIGFPE.
> >>>  */
> >>> int
> >>>-fputrap()
> >>>+fputrap_x87(void)
> >>> {
> >>>+	struct savefpu *pcb_save;
> >>
> >>No need for this before or after.  Let the compiler cache it.  Just use
> >>curpcb now.
> >The curpcb access shall be spelled as PCPU_GET(curpcb). Please note that
> >compiler is not allowed to cache the accesses, since there is __asm
> >__volatile expression to indirect through %gs-prefixed read.
> 
> Fix curpcb then.  curthread was de-pessimized to use __pure2 and to not
> use __volatile.  I could never this to work to cache curthread when I 
> ried it, but it apparently works in -current.
> 
> curpcb is no more fragile than curthread without the __volatile.
> Especially in fputrap*() where they are accessed under critical_enter().
> They are switched at the same time.  Any locking in pcpu access functions
> for them would be useless, since it goes away when the function returns.
> Apparently they are sufficiently locked by the logic of the code
> (unlike fpucurthread, which requires the critical_enter()).
> 
> curthread is the only pcpu variable important enough to have a special
> access function in amd64 pcpu.h.  curpcb is not very important, but
> spelling it curpcb = __curpcb() instead of PCPU_GET(curpcb) is
> simpler for clients as well as faster.
> 
> >diff --git a/sys/amd64/amd64/fpu.c b/sys/amd64/amd64/fpu.c
> >...
> 
> OK except for the curpcb caching and the fnclex removal, which should
> be done separately.

curpcb() could be implemented like this for amd64 only:

diff --git a/sys/amd64/include/pcb.h b/sys/amd64/include/pcb.h
index 22cbbe2..3417c52 100644
--- a/sys/amd64/include/pcb.h
+++ b/sys/amd64/include/pcb.h
@@ -144,6 +144,17 @@ void	makectx(struct trapframe *, struct pcb *);
 int	savectx(struct pcb *) __returns_twice;
 void	resumectx(struct pcb *);
 
+static __inline __pure2 struct pcb *
+__curpcb(void)
+{
+	struct pcb *pcb;
+
+	__asm("movq %%gs:%1,%0" : "=r" (pcb)
+	    : "i" (offsetof(struct pcpu, pc_curpcb)));
+	return (pcb);
+}
+#define	curpcb		(__curpcb())
+
 #endif
 
 #endif /* _AMD64_PCB_H_ */
diff --git a/sys/sys/user.h b/sys/sys/user.h
index accb7c3..ab1c7a7 100644
--- a/sys/sys/user.h
+++ b/sys/sys/user.h
@@ -35,6 +35,7 @@
 #ifndef _SYS_USER_H_
 #define _SYS_USER_H_
 
+#include <sys/pcpu.h>
 #include <machine/pcb.h>
 #ifndef _KERNEL
 /* stuff that *used* to be included by user.h, or is now needed */

Please note the location in pcb.h an not in machine/pcpu.h, where it
cannot work for technical reasons (struct pcpu is not defined yet).
This should be committed separetely, together with the pass over amd64/
to convert PCPU_GET(curpcb) into curpcb().

Do you agree with committing the PR fix as is and adding the curpcb() later ?
And removing fnclex() later (it seems I convinced enough for its removal).

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-amd64/attachments/20120718/658c7036/attachment.pgp