Hi Keir,
Following your comments, I will separate out xsave out of FPU code and put them
into xstate.[ch]. The function will be renamed consistently.
Thanks,
-Wei
-----Original Message-----
From: Keir Fraser [mailto:keir.xen@xxxxxxxxx] On Behalf Of Keir Fraser
Sent: Friday, April 15, 2011 4:20 AM
To: Huang2, Wei; 'xen-devel@xxxxxxxxxxxxxxxxxxx'
Subject: Re: [Xen-devel] [PATCH][RFC] FPU LWP 1/5: clean up the FPU code
On 14/04/2011 21:37, "Wei Huang" <wei.huang2@xxxxxxx> wrote:
> This patch reorganizes and cleans up the FPU code. Main changes include:
>
> 1. Related functions are re-arranged together.
> 2. FPU save/restore code are extracted out as independent functions
> (fsave/frstor, fxsave/fxrstor, xsave/xrstor).
> 3. Two FPU entry functions, fpu_save() and fpu_restore(), are defined.
> They utilize xsave/xrstor to save/restore FPU state if CPU supports
> extended state. Otherwise they fall back to fxsave/fxrstor or fsave/frstor.
Wei,
If you're going to clean this up, please don't politely skirt around the
existing file- and function-name conventions. The fact is that i387/fpu is
but one item of state handled by XSAVE/XRSTOR and friends. Thus it is the
tail wagging the dog to keep all the new mechanism in files and functions
called {i387,fpu}*. I'd prefer you to move some or all functionality into
newly-named files and functions -- perhaps using a prefix like xstate_ on
functions and sticking it all in files xstate.[ch]. Perhaps the old i387
files would disappear completely, or perhaps you'll find a few bits and
pieces logically remain in them, I don't mind either way as long as
everything is in a logical place with a logical name.
The naming in your 3rd patch is also unnecessarily confusing: is it really
clear the difference between *_reload and *_restore, for example, that one
is done on context switch and the other on #NM? We need better names; for
example:
xstate_save: Straightforward I guess as we always unlazily save everything
that's dirty straight away during context switch.
xstate_restore_lazy: Handle stuff we restore lazily on #NM.
xstate_restore_eager: Handle stuff we restore unconditionally (if in use by
the guest) on ctxt switch.
These names would mean a lot more to me than what you chose. However, feel
free to work out a comprehensive naming scheme that you think works best.
All I'm saying is that our current naming scheme is already pretty crappy,
and you make it quite a bit worse by following the existing direction way
too much!
-- Keir
> Signed-off-by: Wei Huang <wei.huang2@xxxxxxx>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home