Re: FPU issue, xm dmesg

[Manuel Bouyer <bouyer%antioche.eu.org@localhost>]

On Wed, Nov 02, 2005 at 04:08:24PM +0100, Manuel Bouyer wrote:
> Ha, this is interesting. I can reproduce it too with paranoia. I tried a few
> other tests but couldn't reproduce the failure.
> 
> One though I had about this is that it's possible that the hypervisor uses the
> FPU/MMX registers but don't save/restore them. Maybe turning off our lazy FPU
> save/restore would fix this ?

OK, I've done a few experiments on this, but couldn't come to a real conclusion
I've come up with the attached patch, which seems to properly
save/restore the FPU registers on each context switch (I don't see FPU traps
anymore, and things don't seem to be worse than before).

Now paranoia always find one, and only one flaw:
Checking rounding on multiply, divide and add/subtract.
* is neither chopped nor correctly rounded.
/ is neither chopped nor correctly rounded.
Addition/Subtraction neither rounds nor chops.
Sticky bit used incorrectly or not at all.
FLAW:  lack(s) of guard digits or failure(s) to correctly round or chop
(noted above) count as one flaw in the final tally below.

Could this be the proof that we don't properly save/restore FPU registers ?
If this was the case, I think the behavior would be more random. Even
running 2 copy of paranoia in parallel cause the same deterministic behavior.

Note that, from what I understood, Xen only allows use of the legacy FPU
registers. Things like MMX registers and fxsave can't be used. I don't know
if this affects only the kernel, or if userland programs have this constraint
too.

Any more idea on this ?

-- 
Manuel Bouyer <bouyer%antioche.eu.org@localhost>
     NetBSD: 26 ans d'experience feront toujours la difference
--

Index: i386/machdep.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/i386/machdep.c,v
retrieving revision 1.18
diff -u -r1.18 machdep.c
--- i386/machdep.c      19 Aug 2005 16:06:12 -0000      1.18
+++ i386/machdep.c      7 Nov 2005 15:55:22 -0000
@@ -2419,6 +2419,7 @@
                if (l->l_addr->u_pcb.pcb_fpcpu != NULL)
                        npxsave_lwp(l, 0);
 #endif
+               KASSERT((flags & _UC_FXSAVE) == 0);
                if (flags & _UC_FXSAVE) {
                        if (i386_use_fxsave) {
                                memcpy(
@@ -2444,6 +2445,11 @@
                }
                /* If not set already. */
                l->l_md.md_flags |= MDL_USEDFPU;
+#define frstor(addr)            __asm("frstor %0" : : "m" (*addr))
+               frstor(&l->l_addr->u_pcb.pcb_savefpu.sv_87);
+#undef frstor
+               l->l_addr->u_pcb.pcb_fpcpu = curcpu();
+               curcpu()->ci_fpcurlwp = l;
 #if 0
                /* Apparently unused. */
                l->l_addr->u_pcb.pcb_saveemc = mcp->mc_fp.fp_emcsts;
Index: i386/npx.c
===================================================================
RCS file: /cvsroot/src/sys/arch/xen/i386/npx.c,v
retrieving revision 1.4
diff -u -r1.4 npx.c
--- i386/npx.c  20 Mar 2005 13:12:59 -0000      1.4
+++ i386/npx.c  7 Nov 2005 15:55:22 -0000
@@ -70,7 +70,7 @@
 #include <sys/cdefs.h>
 __KERNEL_RCSID(0, "$NetBSD: npx.c,v 1.4 2005/03/20 13:12:59 bouyer Exp $");
 
-#if 0
+#if 1
 #define IPRINTF(x)     printf x
 #else
 #define        IPRINTF(x)
@@ -116,7 +116,7 @@
  * 387 and 287 Numeric Coprocessor Extension (NPX) Driver.
  *
  * We do lazy initialization and switching using the TS bit in cr0 and the
- * MDL_USEDFPU bit in mdproc.
+ * MDL_USEDFPU bit in mdlwp.
  *
  * DNA exceptions are handled like this:
  *
@@ -149,7 +149,6 @@
 #define        stts()                  HYPERVISOR_fpu_taskswitch()
 #endif
 
-int npxdna(struct cpu_info *);
 static int     npxdna_notset(struct cpu_info *);
 static int     npxdna_s87(struct cpu_info *);
 #ifdef I686_CPU
@@ -319,8 +318,7 @@
 }
 #endif
 
-void npxinit(ci)
-       struct cpu_info *ci;
+void npxinit(struct cpu_info *ci)
 {
        lcr0(rcr0() & ~(CR0_EM|CR0_TS));
        fninit();
@@ -534,7 +532,7 @@
 {
        struct lwp *l;
        int s;
-#ifdef XXXXENDEBUG_LOW
+#if 1 /* def XXXXENDEBUG_LOW */
        struct trapframe *f = (struct trapframe *)((uint32_t *)(void *)&ci)[1];
 #endif
 
@@ -565,9 +563,11 @@
                IPRINTF(("Save"));
                npxsave_cpu(ci, 1);
        } else {
+               clts();
                IPRINTF(("Init"));
                fninit();
                fwait();
+               stts();
        }
        splx(s);
 
@@ -610,8 +610,11 @@
         * fpu_save, if we're saving the FPU state not from interrupt
         * context (f.i. during fork()).  Just return in this case.
         */
-       if (ci->ci_fpsaving)
+       if (ci->ci_fpsaving) {
+               printf("recursive npx trap; cr0=%x\n", rcr0());
+               Debugger();
                return (1);
+       }
 
        s = splipi();           /* lock out IPI's while we clean house.. */
 #ifdef MULTIPROCESSOR
@@ -650,7 +653,7 @@
        s = splipi();
        ci->ci_fpcurlwp = l;
        l->l_addr->u_pcb.pcb_fpcpu = ci;
-       ci->ci_fpused = 1;
+       //ci->ci_fpused = 1;
        splx(s);
 
 
@@ -689,8 +692,8 @@
        if (l == NULL)
                return;
 
-       IPRINTF(("%s: fp CPU %s lwp %p\n", ci->ci_dev->dv_xname,
-           save? "save" : "flush", l));
+       /* IPRINTF(("%s: fp CPU %s lwp %p\n", ci->ci_dev->dv_xname,
+           save? "save" : "flush", l)); */
 
        if (save) {
 #ifdef DIAGNOSTIC
@@ -720,12 +723,12 @@
         * We set the TS bit in the saved CR0 for this process, so that it
         * will get a DNA exception on any FPU instruction and force a reload.
         */
-       l->l_addr->u_pcb.pcb_cr0 |= CR0_TS;
+       //l->l_addr->u_pcb.pcb_cr0 |= CR0_TS;
 
        s = splipi();
        l->l_addr->u_pcb.pcb_fpcpu = NULL;
        ci->ci_fpcurlwp = NULL;
-       ci->ci_fpused = 1;
+       //ci->ci_fpused = 1;
        splx(s);
 }