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
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
Hi,
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] 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
Hartmut Birr wrote:
Hi,
-----Original Message----- From: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] 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
:-)
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
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: ros-dev-bounces@reactos.com [mailto:ros-dev-bounces@reactos.com] 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
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