Xen devel - Apr 2011 - [PATCH][RFC] FPU LWP 1/5: clean up the FPU code

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.

Signed-off-by: Wei Huang <wei.huang2@amd.com>





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 14/04/2011 21:37, "Wei Huang" <wei.huang2@amd.com> 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@amd.com>
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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@gmail.com] On Behalf Of Keir Fraser
Sent: Friday, April 15, 2011 4:20 AM
To: Huang2, Wei; ''xen-devel@lists.xensource.com''
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@amd.com> 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@amd.com>
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel