Guest Function reset tests for registers #906
Conversation
jsturtevant
requested review from
danbugs,
dblnz,
devigned,
jprendes,
ludfjig,
marosset,
simongdavies and
syntactically
as code owners
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
jsturtevant
force-pushed
the
reset-mxcsr-register
branch
from
17ed512 to
c301f4f
Compare
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
Signed-off-by: James Sturtevant <jsturtevant@gmail.com>
jsturtevant
force-pushed
the
reset-mxcsr-register
branch
from
c301f4f to
9b82922
Compare
jsturtevant
changed the title
| // https://elixir.bootlin.com/linux/v6.16/source/arch/x86/kvm/x86.c#L12229 | ||
| let fpu = kvm_fpu { | ||
| fcw: FP_CONTROL_WORD_DEFAULT, | ||
| ftwx: FP_TAG_WORD_DEFAULT, |
There was a problem hiding this comment.
nit: Since it's 0 we can just remove it from here and let the ..Default::default() zero it. We can probably remove the constant completely in this case unless it's used for something else
There was a problem hiding this comment.
Thinking on this a bit and I think keeping it here as a const is a good idea since it allows us to be explicit about using the FXSAVE format (00 vs 11). It shouldn't have any impact but allows us to document that we are using the setting we explicitly want.
| // AMD64 Architecture Programmer's Manual, Volume 2 | ||
| // 11.5.10 Mode-Specific XSAVE/XRSTOR State Management | ||
| // MXCSR is at byte offset 24 -> u32 index 6 | ||
| let mut xsave = match self.vcpu_fd.get_xsave() { |
There was a problem hiding this comment.
If we're doing xsave, can we omit the existing self.vcpu_fd.set_fpu(&fpu)?; call? Do they overlap in functionality?
Also: Maybe we should save xsave after calling initialize, and restore to that known good state after each to dispatch_call_from_host?
There was a problem hiding this comment.
If we're doing xsave, can we omit the existing self.vcpu_fd.set_fpu(&fpu)?; call?
I considered this but felt the the xsave structure was pretty opaque with lots that could change. It felt clearer/safer to do this and there didn't seem to be a noticeable perf difference.
Also: Maybe we should save xsave after calling initialize, and restore to that known good state after each to dispatch_call_from_host?
This is a good idea! Let me take a look at this.
There was a problem hiding this comment.
Also: Maybe we should save xsave after calling initialize, and restore to that known good state after each to dispatch_call_from_host?
This is a good idea! Let me take a look at this.
I tried this out by initializing the FP registers once and storing them. I didn't notice any impact on performance (but it's hard to measure at the function call level due to so many variances). Saving does incur an additional ~4k of memory per vm instance. I lean towards leaving this approach and revisiting it if required. what do you think?
There was a problem hiding this comment.
I think that's fine. However, I think I am a little skeptical in general about using xsave just for the purpose of resetting just the single MXCSR register. There are plenty other registers we don't reset currently, that we probably should. Maybe let's take the opportunity to reset some more registers? What do you think?
Also, regarding
If we're doing xsave, can we omit the existing self.vcpu_fd.set_fpu(&fpu)?; call?
I considered this but felt the the xsave structure was pretty opaque with lots that could change. It felt clearer/safer to do this and there didn't seem to be a noticeable perf difference.
I think xsave should be pretty standardized, at least per-hypervisor, so I do think it's worth looking into this
There was a problem hiding this comment.
There are plenty other registers we don't reset currently, that we probably should. Maybe let's take the opportunity to reset some more registers? What do you think?
Yes, we have #791 and I started to look into other registers. I was using this as a way to get a pattern set up without going after everything in one go.
I think xsave should be pretty standardized, at least per-hypervisor, so I do think it's worth looking into this
xsave does have a specific layout depending on the features enabled. Maybe we could do this after #907 and other related changes.
2 participants
Fixes #904 #905
This pull request improves the correctness and test coverage of FPU and MXCSR register handling in the hypervisor and guest code. It fixes how the x87 FPU tag word is initialized, ensures MXCSR is properly reset for each guest call, and adds comprehensive tests (including integration and guest-side changes) to verify that FPU and MXCSR state is correctly reset and restored.
I did run benchmarks against using xsave on each call and had no noticeable different when using it. There is enough noise in a single get call that any change wasn't tractable.
FPU and MXCSR register initialization and handling:
FP_TAG_WORD_DEFAULTto0x00to match the FXSAVE format, where a two-bit value of 11 (encoded as 0) indicates an empty x87 FPU register.set_fpudoes not handle MXCSR. This ensures the guest's MXCSR is properly initialized. [1] [2]Testing and validation:
ModifyFpuMxcsr,ReadFpuMxcsr) and registered them, allowing host tests to manipulate and inspect FPU/MXCSR state inside the guest. [1] [2]