From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH][RFC] FPU LWP 1/5: clean up the FPU code Date: Fri, 15 Apr 2011 10:20:13 +0100 Message-ID: References: <4DA75B22.1010702@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <4DA75B22.1010702@amd.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Wei Huang , "'xen-devel@lists.xensource.com'" List-Id: xen-devel@lists.xenproject.org On 14/04/2011 21:37, "Wei Huang" 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 > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel