Maybe need to reset fpu state after saving it

[nyh]

This issue was created due to a thought experiment - I don't have evidence that it leads to an actual bug.

When OSv code runs in the middle of some user code, e.g., interrupt handling, we use fpu_lock() to save the user code's FPU state so we can restore it later. However, after the kernel saves the FPU state, it doesn't attempt to clear it. The kernel, which needs to do FPU calculations (e.g., in the scheduler), might inherit weird control bits or a full FPU stack, and misbehave.

I think maybe after saving the FPU state, we should run the FNINIT instruction, or our fpu_init() function (which also resets the sse control register).

[wkozaczuk]

Doing FNINIT after we save FPU state would only slow down task switch procedure for involuntary switches (interrupt triggered). However doing it always in switch_to() should cover all scenarios.

[nyh]

@wkozaczuk on the other hand, interrupts are already slow - they usually had an exit to the hypervisor involved, and they require a full FPU save anyway, so adding additional FNINIT to them should hardly (I hope...) be noticable. Voluntary context switches between threads didn't need the full FPU saving until now, and it will be nice not to add things to slow them down. The tests/misc-ctxsw.cc, for example, measures these voluntary context switches.

[nyh]

By the way, in addition to FNINIT, we should probably also use EMMS: The ABI document states:

The CPU shall be in x87 mode upon entry to a function. Therefore, every function that uses the
MMX registers is required to issue an emms or femms instruction after using MMX registers, before returning or calling another function

Because we don't know if the FPU state we just saved did this, we should, so we can use FPU in the kernel (and later threads we switch to) normally.
But even better - I think that only EMMS, without FNINIT, should be enough. This is because if I understand correctly, EMMS also resets the top-of-stack in the fpu control word.
Morover, I think FEMMS, the faster variant of EMMS, should work too. FEMMS is like EMMS, but doesn't bother zeroing the actual values of the registers - which we don't really need to do (we just need to mark the registers FPU and to reset the top-of-stack).

[wkozaczuk]

I am assuming we would only do FNINIT/FEMMS after saving FPU state ONLY in interrupt handling code. Or there is a benefit to do it in all fpu_lock usage scenarios (syscall, etc)? My understand is that other places are not involved in a thread switch unlike interrupt triggered one.

Also I wonder if FEMMS is always available or we should use depending on CPU features?

[nyh]

1. It needs to be in fpu_lock, for all uses - not just interrupts, also exceptions (e.g., division by zero, page fault, etc.), signals, system calls - in all these cases, these are places where the code doesn't know it is calling a function, so the code might be in the middle of an FPU computation and doesn't clean it up before generating the exception, calling the SYSCALL instruction, and so on.
2. All these places can also cause a thread switch - e.g., on page fault (an exception, not an interrupt) or syscall, we may do something which blocks the thread, so switches to a different one. But even if there is no thread switch, there can be FPU calculations in the kernel so we need a working (clean) FPU state.
3. FEMMS was introduced 20 years ago... I don't know if we can assume it is always available, we can think about that again when we're sure the solution works, and how (whether we do it like in this issue, or Maybe need to reset fpu status word on context switch #1020). We can always check the feature bit and conditionally call FNINIT or FEMMS.

[wkozaczuk]

Thanks @nyh. The fact that switch_to() happens after FPU save which would now clean FPU state, should not interfere with saving/restoring FPU control word in switch_to()?

[nyh]

Good point! If we really want to use FNINT in fpu_lock then we'll need to save the current thread's last known status word in the fpu_lock (before FNINIT), instead of just before the context switch but remember that we did this - and this complicates everything.
If we use EMMS, which does not touch the control word, then we should be fine. The downside, however, is that the kernel runs with whatever control word the last thread used - which can theoretically change the kernel's behavior when it does floating point calculations. However, I don't think any of those flags (exceptions, rounding, etc.) makes any difference to the kernel.

P.S. I was wrong to suggest FEMMS, it only exists on AMD machines (it seems), and intel only has EMMS. According to https://www.agner.org/optimize/instruction_tables.pdf the EMMS instruction only takes 1 cycle (but there are various weird rules on how much time the next sse or x87 instructions are delayed).

[wkozaczuk]

Applying this patch:

diff --git a/arch/x64/arch-cpu.cc b/arch/x64/arch-cpu.cc
index bb96af3d..05792008 100644
--- a/arch/x64/arch-cpu.cc
+++ b/arch/x64/arch-cpu.cc
@@ -61,11 +61,13 @@ void fpu_state_init_fxsave(processor::fpu_state *s) {
 extern "C"
 void fpu_state_save_xsave(processor::fpu_state *s) {
     processor::xsave(s, -1);
+   asm volatile("emms");
 }
 
 extern "C"
 void fpu_state_save_fxsave(processor::fpu_state *s) {
     processor::fxsave(s);
+   asm volatile("emms");
 }
 
 extern "C"

does not seem help solve #1010 which is very repeatable. I have not tested others.

It could be that even though we clean FPU status word right after saving FPU state it gets somehow modified (maybe in kernel thread scheduler logic) before we switch to new thread which is what switch_to().

It could be that in case of voluntary task switch which leaves FPU status word dirty, we switch to thread that also uses FPU which would then fail. I looks like clearing FPU status word might help in some scenarios but by itself is not enough to fix at least some related issues.