kvm: FPU synchronization support on x86
Review Request #2011 - Created Sept. 10, 2013 and submitted
| Information | |
|---|---|
| Andreas Sandberg | |
| gem5 | |
| default | |
| Reviewers | |
| Default | |
Changeset 9871:2c15197c07e7 --------------------------- kvm: FPU synchronization support on x86 This changeset adds support for synchronizing the FPU and SIMD state of a virtual x86 CPU with gem5. It supports both the XSave API and the KVM_(GET|SET)_FPU kernel API. The XSave interface can be disabled using the useXSave parameter (in case of kernel issues). Unfortunately, KVM_(GET|SET)_FPU interface seems to be buggy in some kernels (specifically, the MXCSR register isn't always synchronized), which means that it might not be possible to synchronize MXCSR on old kernels without the XSave interface. ** NOTE ** This changeset depends on the __float80 type in gcc and might not build using llvm. llvm users are encouraged to test this. *** This patch is a part of series of changes to add support for KVM on x86. See https://github.com/andysan/gem5/tree/kvm-x86 for the full series.
Posted (Sept. 18, 2013, 2:58 p.m.)
Hi Andreas,
The basic structure looks fine, I just have some minor concerns. In particular, for people not familiar with x86, the function names ending in XSave don't indicate that they have anything to do with the FPU. In addition, other than looking at the if/else bits of code, it's not clear that using xsave is an alternative to another (not clearly named) approach.
It might be overkill, but one approach would be to push the test down a level, i.e., leave the top-level function updateKvmStateFPU(), but define it like:
void
updateKvmStateFPU()
{
if (useXSave)
updateKvmStateFPUviaXSave();
else
updateKvmStateFPUviaOtherNonXSaveMethod(); // assuming you have a better name
}
-
src/cpu/kvm/x86_cpu.cc (Diff revision 1) -
It looks like this warning gets printed even if the user set useXSave to false... is that really what we want? Seems to me that if the user set useXSave to false and haveXSave is also false, either the second warning should get printed, or maybe none at all.
I don't see an updated patch here, but looking at the diff on the new version of the other patch, I saw most of the changes anyway. Between that and your descriptions, I'm sure the updated patch is fine.