From owner-freebsd-amd64@FreeBSD.ORG Wed Jul 18 05:10:03 2012 Return-Path: Delivered-To: freebsd-amd64@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 1E696106564A for ; Wed, 18 Jul 2012 05:10:03 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id A1D668FC12 for ; Wed, 18 Jul 2012 05:10:02 +0000 (UTC) Received: from skuns.kiev.zoral.com.ua (localhost [127.0.0.1]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id q6I5ABmO073281; Wed, 18 Jul 2012 08:10:11 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5) with ESMTP id q6I59wj2028201; Wed, 18 Jul 2012 08:09:58 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.5/8.14.5/Submit) id q6I59wLo028200; Wed, 18 Jul 2012 08:09:58 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 18 Jul 2012 08:09:58 +0300 From: Konstantin Belousov To: Bruce Evans Message-ID: <20120718050958.GQ2676@deviant.kiev.zoral.com.ua> References: <201207172150.q6HLoFvB096742@freefall.freebsd.org> <20120718103210.E834@besplex.bde.org> <20120718035220.GO2676@deviant.kiev.zoral.com.ua> <20120718140523.X1862@besplex.bde.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="s3R87C3fwYeCSZ0b" Content-Disposition: inline In-Reply-To: <20120718140523.X1862@besplex.bde.org> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-4.0 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: Mark Linimon , freebsd-amd64@freebsd.org Subject: Re: amd64/169927: siginfo, si_code for fpe errors when error occurs using the SSE math processor X-BeenThere: freebsd-amd64@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Porting FreeBSD to the AMD64 platform List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Jul 2012 05:10:03 -0000 --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 #include #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--