Date: Wed, 18 Jul 2012 08:09:58 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Bruce Evans <brde@optusnet.com.au> Cc: Mark Linimon <linimon@lonesome.com>, freebsd-amd64@freebsd.org Subject: Re: amd64/169927: siginfo, si_code for fpe errors when error occurs using the SSE math processor Message-ID: <20120718050958.GQ2676@deviant.kiev.zoral.com.ua> In-Reply-To: <20120718140523.X1862@besplex.bde.org> References: <201207172150.q6HLoFvB096742@freefall.freebsd.org> <20120718103210.E834@besplex.bde.org> <20120718035220.GO2676@deviant.kiev.zoral.com.ua> <20120718140523.X1862@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--s3R87C3fwYeCSZ0b Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jul 18, 2012 at 02:36:16PM +1000, Bruce Evans wrote: > On Wed, 18 Jul 2012, Konstantin Belousov wrote: >=20 > >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 clobberi= ng > >>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. >=20 > 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. >=20 > >>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] =3D { > >>> * 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. >=20 > 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=20 > ried it, but it apparently works in -current. >=20 > 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()). >=20 > 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 =3D __curpcb() instead of PCPU_GET(curpcb) is > simpler for clients as well as faster. >=20 > >diff --git a/sys/amd64/amd64/fpu.c b/sys/amd64/amd64/fpu.c > >... >=20 > 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 *); =20 +static __inline __pure2 struct pcb * +__curpcb(void) +{ + struct pcb *pcb; + + __asm("movq %%gs:%1,%0" : "=3Dr" (pcb) + : "i" (offsetof(struct pcpu, pc_curpcb))); + return (pcb); +} +#define curpcb (__curpcb()) + #endif =20 #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_ =20 +#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). --s3R87C3fwYeCSZ0b Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAlAGRSYACgkQC3+MBN1Mb4hZqgCfT2huy84BV5OvSd3MX/KFG6kr +twAoOxaFP3EY9GfZSG2Nso+Rka0Qi2V =bPlt -----END PGP SIGNATURE----- --s3R87C3fwYeCSZ0b--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120718050958.GQ2676>