-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2011/#review4701
-----------------------------------------------------------
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
<http://reviews.gem5.org/r/2011/#comment4443>
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.
- Steve Reinhardt
On Sept. 10, 2013, 6:34 a.m., Andreas Sandberg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/2011/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2013, 6:34 a.m.)
>
>
> Review request for Default.
>
>
> Repository: gem5
>
>
> Description
> -------
>
> 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.
>
>
> Diffs
> -----
>
> src/cpu/kvm/X86KvmCPU.py PRE-CREATION
> src/cpu/kvm/x86_cpu.hh PRE-CREATION
> src/cpu/kvm/x86_cpu.cc PRE-CREATION
>
> Diff: http://reviews.gem5.org/r/2011/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andreas Sandberg
>
>
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
