From owner-cvs-src@FreeBSD.ORG Sun May 28 13:58:26 2006 Return-Path: X-Original-To: cvs-src@freebsd.org Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 0233716AD19; Sun, 28 May 2006 13:58:26 +0000 (UTC) (envelope-from bde@zeta.org.au) Received: from mailout1.pacific.net.au (mailout1.pacific.net.au [61.8.0.84]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1A1B143D55; Sun, 28 May 2006 13:58:25 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au [61.8.0.87]) by mailout1.pacific.net.au (Postfix) with ESMTP id EC109329FE6; Sun, 28 May 2006 23:58:23 +1000 (EST) Received: from epsplex.bde.org (katana.zip.com.au [61.8.7.246]) by mailproxy2.pacific.net.au (8.13.4/8.13.4/Debian-3sarge1) with ESMTP id k4SDwL0a008692; Sun, 28 May 2006 23:58:22 +1000 Date: Sun, 28 May 2006 23:58:21 +1000 (EST) From: Bruce Evans X-X-Sender: bde@epsplex.bde.org To: David Xu In-Reply-To: <200605282006.26364.davidxu@freebsd.org> Message-ID: <20060528223012.H2592@epsplex.bde.org> References: <200605280440.k4S4ej96064322@repoman.freebsd.org> <200605281741.52752.davidxu@freebsd.org> <20060528201544.L20679@delplex.bde.org> <200605282006.26364.davidxu@freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/i386/isa npx.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 28 May 2006 13:58:27 -0000 On Sun, 28 May 2006, David Xu wrote: > On Sunday 28 May 2006 19:15, Bruce Evans wrote: > >> Starting with a clean FP state like the old code does is safest, but POSIX >> explicitly requires copying the environment, and bugs in the new code >> result in half of the most dangerous part of the environment (the SSE >> half of the exception flags) being copied anyway. >> >> Pending exceptions are not the only problem here. Pending exceptions >> are an i387 thing. Most exceptions are non-pending ones for IEEE inexact. > > how about following patch ? I want one that removes about 30 lines, not one that adds about 20 lines. > Index: npx.c > =================================================================== > RCS file: /home/ncvs/src/sys/i386/isa/npx.c,v > retrieving revision 1.168 > diff -u -u -r1.168 npx.c > --- npx.c 28 May 2006 04:40:45 -0000 1.168 > +++ npx.c 28 May 2006 11:40:58 -0000 > @@ -99,6 +99,7 @@ > #define fxrstor(addr) __asm("fxrstor %0" : : "m" (*(addr))) > #define fxsave(addr) __asm __volatile("fxsave %0" : "=m" (*(addr))) > #define ldmxcsr(__csr) __asm __volatile("ldmxcsr %0" : : "m" (__csr)) > +#define stmxcsr(addr) __asm __volatile("stmxcsr %0" : "=m" (*(addr))) > #endif > #define start_emulating() __asm("smsw %%ax; orb %0,%%al; lmsw %%ax" \ > : : "n" (CR0_TS) : "ax") > @@ -950,7 +951,8 @@ > { > union savefpu *state; > u_int32_t mxcsr; > - u_int32_t cw; > + u_int16_t cw; > + register_t s; > > if (!(td->td_pcb->pcb_flags & PCB_NPXINITDONE)) { > newtd->td_pcb->pcb_flags &= ~PCB_NPXINITDONE; > @@ -958,21 +960,40 @@ > } > > state = &newtd->td_pcb->pcb_save; > - /* get control word */ > - if (npxgetregs(td, state)) > - return; > - if (cpu_fxsr) { > - mxcsr = state->sv_xmm.sv_env.en_mxcsr; > - cw = state->sv_xmm.sv_env.en_cw; > + s = intr_disable(); > + if (curthread == PCPU_GET(fpcurthread)) { > +#ifdef CPU_ENABLE_SSE > + if (cpu_fxsr) { > + stmxcsr(&mxcsr); > + fnstcw(&cw); > + } > + else > +#endif > + { > + mxcsr = 0; > + fnstcw(&cw); > + } > } else { > - cw = state->sv_87.sv_env.en_cw; > - mxcsr = 0; > +#ifdef CPU_ENABLE_SSE > + if (cpu_fxsr) { > + mxcsr = td->td_pcb->pcb_save.sv_xmm.sv_env.en_mxcsr; > + cw = td->td_pcb->pcb_save.sv_87.sv_env.en_cw; > + } > + else > +#endif > + { > + mxcsr = 0; > + cw = td->td_pcb->pcb_save.sv_87.sv_env.en_cw; > + } > } > + intr_restore(s); Save the parent's state using npxsave() as in cpu_fork(). This is much cleaner and and only slightly less efficient. I still think the entire environment in that state needs to be copied. > bcopy(&npx_cleanstate, state, sizeof(*state)); > +#ifdef CPU_ENABLE_SSE > if (cpu_fxsr) { > state->sv_xmm.sv_env.en_cw = cw; > - state->sv_xmm.sv_env.en_mxcsr = mxcsr; > + state->sv_xmm.sv_env.en_mxcsr = mxcsr & ~0x3F; OK if it is correct to not copy the environment. BTW, your other patch that masks the top 16 bits in mxcsr needs to be extended to sometimes mask 0x40 (DAZ). The amd64 arch manual says to mask with mxcsr_mask which is in the fxsave image. Cleared bits in mxcsr_mask indicate reserved bits in a future-proof way. Unfortunately, this isn't past-proof so the following complications are needed: if the CPU stores 0 in mxcsr_mask, it means that mxcsr_mask is not supported; old CPUs (at least amd64 ones) that don't support mxcsr_mask also don't support DAZ, so the correct mask for them is 0xFFBF. These complications are implemented in Linux's i386 so I suppose they are no-ops at worst for non-amd64 CPUs. Linux stores the mask in a global variable. I think this breaks the asymmetric SMP case more than necessary. FreeBSD's amd64 uses mxcsr_mask without fixing up the case where it is 0. So does Linux-2.6.10's x86_64 (it Initializes the global variable but doesn't use it). Behaviour of mxcsr and DAZ on some CPUs: AXP2600 (Barton): mxcsr_mask = 0; attempting to set DAZ traps P3-800 (freefall): same as AXP2600 A64 (Id0xf48 stepping 8): mxcsr_mask = 0xFFFF; setting DAZ works > } else > +#endif vm_machdep.c is missing a DEV_NPX ifdef around the call here. Of course, lots of things would break if the option to not use npx were exercised. CPU_ENABLE_SSE is a silly option too. > state->sv_87.sv_env.en_cw = cw; > newtd->td_pcb->pcb_flags |= PCB_NPXINITDONE; > } > Bruce