Re: [ros-diffs] [CVS reactos] - Saved the state of the fpu at a win32 call and restored the state

[Gregor Anich]

Hi Hartmut!

You have changed the w32 callback to save the FPU state, this is wrong I 
think!
I was wondering too wether it should be saved so GvG suggested to check 
myself and I modified our winhello app...
It prints out the FPU control word, calls CreateWindow and printf the 
FPU control word again.
In the callback for the WM_CREATE message (produced by CreateWindow) it 
also prints the control word, then modifies it (and prints the modified one)

This is the output on windows2000:
Original FPU control word: 0x8001f
WM_CREATE: Original FPU control word: 0x8001f
WM_CREATE: Modified FPU control word: 0xa001f
FPU control word after CreateWindow: 0xa001f

Of course I have attached the program.

I am not sure but I think in the KiClearFloatingPointState which you 
have added you have to set KPCR->NpxThread to NULL if it's CurrentThread 
before you do the fn/xsave because it could raise a delayed FPU 
exception (which will be ignored when KPCR->CurrentThread is NULL and be 
delivered when the saved FPU state is restored for the thread)

I think I have made the same mistake in tskswitch.S - I am pretty sure 
it has to be fixed there because we don't want a FPU exception to be 
raised while we are in a cli/sti block and switching to another task.

- blight

[Hartmut Birr]

Hi,

I've made this changes, because syssetup does always crash ros in
KiHandleFpuFault line #462 on my smp machine. The crash is triggered from
fxsave in tskswitch.S after a win32 callback. I revert my changes.

- Hartmut  

>   -----Original Message-----
>  From: ros-dev-bounces(a)reactos.com 
>  [mailto:[e-mail]] On Behalf Of Gregor Anich
>  Sent: Saturday, November 27, 2004 10:57 PM
>  To: ReactOS Development List
>  Subject: [ros-dev] Re: [ros-diffs] [CVS reactos] - Saved the 
>  state of the fpu at a win32 call and restored the state
>  
>  
>  Hi Hartmut!
>  
>  You have changed the w32 callback to save the FPU state, this 
>  is wrong I 
>  think!
>  I was wondering too wether it should be saved so GvG 
>  suggested to check 
>  myself and I modified our winhello app...
>  It prints out the FPU control word, calls CreateWindow and printf the 
>  FPU control word again.
>  In the callback for the WM_CREATE message (produced by 
>  CreateWindow) it 
>  also prints the control word, then modifies it (and prints 
>  the modified one)
>  
>  This is the output on windows2000:
>  Original FPU control word: 0x8001f
>  WM_CREATE: Original FPU control word: 0x8001f
>  WM_CREATE: Modified FPU control word: 0xa001f
>  FPU control word after CreateWindow: 0xa001f
>  
>  Of course I have attached the program.
>  
>  I am not sure but I think in the KiClearFloatingPointState which you 
>  have added you have to set KPCR->NpxThread to NULL if it's 
>  CurrentThread 
>  before you do the fn/xsave because it could raise a delayed FPU 
>  exception (which will be ignored when KPCR->CurrentThread is 
>  NULL and be 
>  delivered when the saved FPU state is restored for the thread)
>  
>  I think I have made the same mistake in tskswitch.S - I am 
>  pretty sure 
>  it has to be fixed there because we don't want a FPU exception to be 
>  raised while we are in a cli/sti block and switching to another task.
>  
>  - blight
>

[Gregor Anich]

Hello!

Do you mean line 462 in revision 1.17? If so then my "fix for SMP" has 
not fixed the problem I guess. It should be like this:
When a thread uses the FPU for the first time after it was switched to 
the KiHandleFpuFault ors the NpxState with NPX_STATE_DIRTY and unsets TS 
in cr0, then the thread can happily use the FPU. When the thread is 
switched away from it is checked wether the FPU state is dirty and if it 
is dirty the state is saved and set to NPX_STATE_VALID (so 
NPX_STATE_DIRTY is unset there) and then TS in cr0 is set again, so only 
after NPX_STATE_DIRTY was unset KiHandleFpuFault should be called.
If the dirty flag is still set it means that the FPU context was not 
saved where it should have been saved (i.e. when setting TS in cr0)

Have you already commited your SMP fixes? Then I could try to get 
reactos with SMP running with bochs and look at it myself.

- blight

Hartmut Birr wrote:

>  Hi,
>
> I've made this changes, because syssetup does always crash ros in
> KiHandleFpuFault line #462 on my smp machine. The crash is triggered from
> fxsave in tskswitch.S after a win32 callback. I revert my changes.
>
> - Hartmut  
>   
>

[Hartmut Birr]

Hi,

it seems that my problem is in w32call.c line #143. We should not set the TS
if Thread->Tcb.NpxState is NPX_STATE_DIRTY. On a thread switch the fpu state
is saved because NpxState is NPX_STATE_DIRTY. This triggers the fpu
exception. If I remove this line it works again.

- Hartmut

[Gregor Anich]

Hi!

I think you are right, this line looks wrong.
Above this line is a block which copies the FPU state from the "real" 
InitialStack into the changed one if NPX_STATE_VALID is set in NpxState 
and CurrentThread != KPCR->NpxThread (should be the same as NpxState & 
NPX_STATE_DIRTY == 0) - this means the FPU state is only copied if the 
thread has not yet used the FPU because it might use it in the callback 
and then we need the state there to restore it (thus only in this case 
we need TS set in cr0, but it should already be set if everything is ok 
since the thread has not yet used the FPU)

Summary: Removing the line is ok, please commit the change - I am pretty 
sure it is a bug.

Thank you :-)
 -blight


Hartmut Birr wrote:

>  Hi,
>
> it seems that my problem is in w32call.c line #143. We should not set the TS
> if Thread->Tcb.NpxState is NPX_STATE_DIRTY. On a thread switch the fpu state
> is saved because NpxState is NPX_STATE_DIRTY. This triggers the fpu
> exception. If I remove this line it works again.
>
> - Hartmut 
>   
>