> On Sept. 18, 2013, 11:58 p.m., Steve Reinhardt wrote:
> > 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
> > }
> >
Sound like a good approach. I added the top-level method and renamed the old
methods to updateKvmStateFPUXSave and updateKvmStateFPULegacy. Additionally, I
made it very clear in the doxygen documentation that the latter two should only
be called through updateKvmStateFPU, which chooses the right API.
> On Sept. 18, 2013, 11:58 p.m., Steve Reinhardt wrote:
> > src/cpu/kvm/x86_cpu.cc, line 384
> > <http://reviews.gem5.org/r/2011/diff/1/?file=37559#file37559line384>
> >
> > 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.
You're right, it seems a bit strange that the warning gets printed when
useXSave is false. I've changed the condition to only print the first warning
if XSave is requested but not present.
- Andreas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.gem5.org/r/2011/#review4701
-----------------------------------------------------------
On Sept. 10, 2013, 3:34 p.m., Andreas Sandberg wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviews.gem5.org/r/2011/
> -----------------------------------------------------------
>
> (Updated Sept. 10, 2013, 3:34 p.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
