RE: [ros-diffs] [CVS reactos] FPU/SSE state saving on taskswitching, FPU and SSE exception support.

[Hartmut Birr]

Hi Gregor,

I've some problems with your changes. On my smp machine I get an assertion
from fpu.c line 452. I think, it isn't possible to use
KPCR.PrcbData.NpxThread on a smp machine. With your first patch I don't have
any problems.

- Hartmut

[Anich Gregor]

Hi!

On Sunday 21 November 2004 12:45, Hartmut Birr wrote:
> Hi Gregor,
>
> I've some problems with your changes. On my smp machine I get an assertion
> from fpu.c line 452. I think, it isn't possible to use
> KPCR.PrcbData.NpxThread on a smp machine. With your first patch I don't
> have any problems.
> Hartmut

Hm. I was afraid there were going to be some problems with SMP... but the 
problem is that I do not really know anything about SMP =/
The KPCR is per-processor, isnt it?
Is it possible that in a SMP system there is one CPU which has less features 
than the other (i.e. FXSR) or is it somewhere specified that you must use 
exactly same CPUs?
Is there some reactos site which tells one what to take care of for SMP?

I will try to find out how to fix the fpu stuff on SMP...

Thanks,
  -blight

[Hartmut Birr]

Hi,
> -----Original Message-----
> From: [e-mail] 
> [mailto:[e-mail]] On Behalf Of Anich Gregor
> Sent: Sunday, November 21, 2004 2:47 PM
> To: ReactOS Development List
> Subject: Re: [ros-dev] RE: [ros-diffs] [CVS reactos] FPU/SSE 
> state saving ontaskswitching, FPU and SSE exception support.
>
> The KPCR is per-processor, isnt it?

Correct.
> Is it possible that in a SMP system there is one CPU which 
> has less features 
> than the other (i.e. FXSR) or is it somewhere specified that 
> you must use 
> exactly same CPUs?

The cpu's are the same and KiCheckFPU is called for both cpu's. I think that
the problem is NpxThread. If a thread uses the fpu and is switched between
different cpu's, the state of the fpu is only saved if an other thread uses
the fpu.
> I will try to find out how to fix the fpu stuff on SMP...

I've revert some parts of fpu.c to your first patch. It works now on my smp
machine.

The current cvs tree doesn't work on a smp machine. If someone will test ros
on a smp machine, I should clean up and commit my changes in ntoskrnl and in
hal.

- Hartmut

[Gregor Anich]

Hartmut Birr wrote:
> Hi,
> > -----Original Message-----
> > From: [e-mail] 
> > [mailto:[e-mail]] On Behalf Of Anich Gregor
> > Sent: Sunday, November 21, 2004 2:47 PM
> > To: ReactOS Development List
> > Subject: Re: [ros-dev] RE: [ros-diffs] [CVS reactos] FPU/SSE 
> > state saving ontaskswitching, FPU and SSE exception support.
> >
> > The KPCR is per-processor, isnt it?
>
> Correct.
> > Is it possible that in a SMP system there is one CPU which 
> > has less features 
> > than the other (i.e. FXSR) or is it somewhere specified that 
> > you must use 
> > exactly same CPUs?
>
> The cpu's are the same and KiCheckFPU is called for both cpu's. I think that
> the problem is NpxThread. If a thread uses the fpu and is switched between
> different cpu's, the state of the fpu is only saved if an other thread uses
> the fpu.

I do not know how threads are scheduled for SMP, but I guess they can 
move between CPUs at any time? If so we cannot delay saving of the FPU 
state on SMP machines I think because it will not be possible (at least 
not easy) to get the FPU state of another CPU if the thread has moved 
and needs the FPU state which it has left behind on the other.
Then I would change the behaviour for SMP to save the FPU state when a 
thread is switched away from and delay loading it.
> > I will try to find out how to fix the fpu stuff on SMP...
>
> I've revert some parts of fpu.c to your first patch. It works now on my smp
> machine.

What have you changed? Did you do what I described above or is the 
problem something different?
> The current cvs tree doesn't work on a smp machine. If someone will test ros
> on a smp machine, I should clean up and commit my changes in ntoskrnl and in
> hal.

Good to know! I have spent all afternoon trying to compile bochs for 
windows with SMP support but no luck so far.
> Hartmut

:-)

[Gregor Anich]

Hello again!

Hartmut Birr wrote:
> I've revert some parts of fpu.c to your first patch. It works now on my smp
> machine.

I have attached a patch to this email, maybe you can try if it works 
with this patch. It is a patch against current reactos CVS.

I didn't see diff which you attached to your mail the first time, but it 
looks like it's an old version of the fpu.c which I have modified... 
maybe it doesn't bugcheck but I think it won't work (unless you have 
also reverted tskswitch.S...)

I have also attached a fputest program which tests wether the FPU state 
is preserved or not.

- blight

Index: ntoskrnl/ke/i386/fpu.c
===================================================================
RCS file: /CVS/ReactOS/reactos/ntoskrnl/ke/i386/fpu.c,v
retrieving revision 1.16
diff -u -r1.16 fpu.c
--- ntoskrnl/ke/i386/fpu.c	21 Nov 2004 13:33:34 -0000	1.16
+++ ntoskrnl/ke/i386/fpu.c	23 Nov 2004 20:36:58 -0000
@@ -28,6 +28,7 @@

/* INCLUDES *****************************************************************/

+#include <roscfg.h>
 #include <ntoskrnl.h>
 #define NDEBUG
 #include <internal/debug.h>
@@ -67,7 +68,8 @@
 /* GLOBALS *******************************************************************/

ULONG HardwareMathSupport = 0;
-static ULONG MxcsrFeatureMask = 0, FxsrSupport = 0, XmmSupport = 0;
+static ULONG MxcsrFeatureMask = 0, XmmSupport = 0;
+ULONG FxsrSupport = 0; /* used by Ki386ContextSwitch for MP */

/* FUNCTIONS *****************************************************************/

@@ -406,10 +408,14 @@
 {
   if (ExceptionNr == 7) /* device not present */
     {
+      BOOL FpuInitialized = FALSE;
       unsigned int cr0 = Ke386GetCr0();
-      PKTHREAD CurrentThread, NpxThread;
+      PKTHREAD CurrentThread;
       PFX_SAVE_AREA FxSaveArea;
       KIRQL oldIrql;
+#ifndef MP
+      PKTHREAD NpxThread;
+#endif

(void) cr0;
       ASSERT((cr0 & X86_CR0_TS) == X86_CR0_TS);
@@ -422,15 +428,17 @@
       asm volatile("clts");

CurrentThread = KeGetCurrentThread();
+#ifndef MP
       NpxThread = KeGetCurrentKPCR()->PrcbData.NpxThread;
+#endif

ASSERT(CurrentThread != NULL);
-      DPRINT("Device not present exception happened! (Cr0 = 0x%x, NpxState = 0x%x)\n", Ke386GetCr0(), CurrentThread->NpxState);
+      DPRINT("Device not present exception happened! (Cr0 = 0x%x, NpxState = 0x%x)\n", cr0, CurrentThread->NpxState);

+#ifndef MP
       /* check if the current thread already owns the FPU */
       if (NpxThread != CurrentThread) /* FIXME: maybe this could be an assertation */
         {
-          BOOL FpuInitialized = FALSE;
           /* save the FPU state into the owner's save area */
           if (NpxThread != NULL)
             {
@@ -448,6 +456,7 @@
                 }
               NpxThread->NpxState = NPX_STATE_VALID;
             }
+#endif /* !MP */

/* restore the state of the current thread */
           ASSERT((CurrentThread->NpxState & NPX_STATE_DIRTY) == 0);
@@ -483,7 +492,9 @@
                 }
             }
           KeGetCurrentKPCR()->PrcbData.NpxThread = CurrentThread;
+#ifndef MP
         }
+#endif

CurrentThread->NpxState |= NPX_STATE_DIRTY;
       KeLowerIrql(oldIrql);
@@ -508,8 +519,8 @@
       CurrentThread = KeGetCurrentThread();
       if (NpxThread == NULL)
         {
-          DPRINT1("!!! Math/Xmm fault ignored! (NpxThread == NULL)\n");
           KeLowerIrql(oldIrql);
+          DPRINT1("!!! Math/Xmm fault ignored! (NpxThread == NULL)\n");
           return STATUS_SUCCESS;
         }

@@ -540,7 +551,6 @@
           PFNSAVE_FORMAT FnSave = (PFNSAVE_FORMAT)&Context->FloatSave;
           asm volatile("fnsave %0" : : "m"(*FnSave));
           KeLowerIrql(oldIrql);
-          memset(Context->FloatSave.RegisterArea, 0, sizeof(Context->FloatSave.RegisterArea));
           KiFnsaveToFxsaveFormat((PFXSAVE_FORMAT)Context->ExtendedRegisters, FnSave);
         }

Index: ntoskrnl/ke/i386/tskswitch.S
===================================================================
RCS file: /CVS/ReactOS/reactos/ntoskrnl/ke/i386/tskswitch.S,v
retrieving revision 1.20
diff -u -r1.20 tskswitch.S
--- ntoskrnl/ke/i386/tskswitch.S	20 Nov 2004 23:46:36 -0000	1.20
+++ ntoskrnl/ke/i386/tskswitch.S	23 Nov 2004 20:13:31 -0000
@@ -26,6 +26,7 @@

/* INCLUDES ******************************************************************/

+#include <roscfg.h>
 #include <internal/i386/segment.h>
 #include <internal/i386/ke.h>
 #include <internal/i386/fpu.h>
@@ -105,8 +106,30 @@
 0:
    lldtw	%ax

+	/*
+	 * Get the pointer to the old thread.
+	 */
    movl	12(%ebp), %ebx

+#ifdef MP
+	/*
+	 * Save FPU state if the thread has used it.
+	 */
+	movl	$0, %fs:KPCR_NPX_THREAD
+	testl	$NPX_STATE_DIRTY, KTHREAD_NPX_STATE(%ebx)
+	jz	3f
+	movl	KTHREAD_INITIAL_STACK(%ebx), %eax
+	cmpl	$0, _FxsrSupport
+	je	1f
+	fxsave	-SIZEOF_FX_SAVE_AREA(%eax)
+	jmp	2f
+1:
+	fnsave	-SIZEOF_FX_SAVE_AREA(%eax)
+2:
+	movl	$NPX_STATE_VALID, KTHREAD_NPX_STATE(%ebx)
+3:
+#endif /* MP */
+
    /*
     * FIXME: Save debugging state.
     */
@@ -152,14 +175,16 @@

/*
     * Set TS in cr0 to catch FPU code and load the FPU state when needed
-	 * We do this only if NewThread != KPCR->NpxThread
+	 * For uni-processor we do this only if NewThread != KPCR->NpxThread
     */
+#ifndef MP
    cmpl	%ebx, %fs:KPCR_NPX_THREAD
-	je	1f
+	je	4f
+#endif /* !MP */
    movl	%cr0, %eax
    orl	$X86_CR0_TS, %eax
    movl	%eax, %cr0
-1:
+4:

/*
     * FIXME: Restore debugging state
@@ -174,9 +199,9 @@
    call	_KeReleaseSpinLockFromDpcLevel@4

cmpl	$0, _PiNrThreadsAwaitingReaping
-	je	4f
+	je	5f
    call	_PiWakeupReaperThread@0
-4:
+5:

/*
     * Restore the saved register and exit

[Hartmut Birr]

Hi,

with one little change, your patch works perfect on my smp machine. It must
be changed a 'movl' to a 'movb':

movb	$NPX_STATE_VALID, KTHREAD_NPX_STATE(%ebx)

I've expand your test program a little bit. It works now with four threads.

- Hartmut
> -----Original Message-----
> From: [e-mail] 
> [mailto:[e-mail]] On Behalf Of Gregor Anich
> Sent: Tuesday, November 23, 2004 9:56 PM
> To: ReactOS Development List
> Subject: Re: [ros-dev] RE: [ros-diffs] [CVS reactos] FPU/SSE 
> state savingontaskswitching, FPU and SSE exception support.
>
> Hello again!
>
> Hartmut Birr wrote:
> > I've revert some parts of fpu.c to your first patch. It
>
> works now on my smp
> > machine.
>
> I have attached a patch to this email, maybe you can try if it works 
> with this patch. It is a patch against current reactos CVS.
>
> I didn't see diff which you attached to your mail the first 
> time, but it 
> looks like it's an old version of the fpu.c which I have modified... 
> maybe it doesn't bugcheck but I think it won't work (unless you have 
> also reverted tskswitch.S...)
>
> I have also attached a fputest program which tests wether the 
> FPU state 
> is preserved or not.
> blight

[Anich Gregor]

Hello!

On Thursday 25 November 2004 01:02, Hartmut Birr wrote:
> Hi,
>
> with one little change, your patch works perfect on my smp machine. It must
> be changed a 'movl' to a 'movb':
>
> movb $NPX_STATE_VALID, KTHREAD_NPX_STATE(%ebx)

My bad, of course it must be movb, not movl - sorry!
> I've expand your test program a little bit. It works now with four threads.
> Hartmut

Yeah i guessed something like four threads would be better than two for 
reliability on SMP.

I will commit the changes today or sometime soon (with the movl change to movb 
of course)

Thank you,
  blight