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.

# HG changeset patch
# User Wei Huang <[email protected]>
# Date 1302811927 18000
# Branch lwp3
# Node ID 4ed4e3d9106e691dddf3bbf54afd935ef0a9000f # Parent 476b4ba2c3b55f95b6e80bbe94a24bab3b3779d9
FPU: 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.

diff -r 476b4ba2c3b5 -r 4ed4e3d9106e xen/arch/x86/acpi/suspend.c
@@ -24,7 +24,7 @@
 
 void save_rest_processor_state(void)
 {
-    save_init_fpu(current);
+    fpu_save(current);
 
 #if defined(CONFIG_X86_64)
     asm volatile (

diff -r 476b4ba2c3b5 -r 4ed4e3d9106e xen/arch/x86/domain.c
@@ -1576,7 +1576,7 @@
     if ( !is_idle_vcpu(p) )
     {
         memcpy(&p->arch.user_regs, stack_regs, CTXT_SWITCH_STACK_BYTES);
-        save_init_fpu(p);
+        fpu_save(p);
         p->arch.ctxt_switch_from(p);
     }
 

diff -r 476b4ba2c3b5 -r 4ed4e3d9106e xen/arch/x86/hvm/svm/svm.c
@@ -348,7 +348,7 @@
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
 
-    setup_fpu(v);
+    fpu_restore(v);
     vmcb_set_exception_intercepts(
         vmcb, vmcb_get_exception_intercepts(vmcb) & ~(1U << TRAP_no_device));
 }

diff -r 476b4ba2c3b5 -r 4ed4e3d9106e xen/arch/x86/hvm/vmx/vmx.c
@@ -612,7 +612,7 @@
 
 static void vmx_fpu_enter(struct vcpu *v)
 {
-    setup_fpu(v);
+    fpu_restore(v);
     v->arch.hvm_vmx.exception_bitmap &= ~(1u << TRAP_no_device);
     vmx_update_exception_bitmap(v);
     v->arch.hvm_vmx.host_cr0 &= ~X86_CR0_TS;

diff -r 476b4ba2c3b5 -r 4ed4e3d9106e xen/arch/x86/traps.c
@@ -3167,7 +3167,7 @@
 
     BUG_ON(!guest_mode(regs));
 
-    setup_fpu(curr);
+    fpu_restore(curr);
 
     if ( curr->arch.pv_vcpu.ctrlreg[0] & X86_CR0_TS )
     {

diff -r 476b4ba2c3b5 -r 4ed4e3d9106e xen/include/asm-x86/i387.h
@@ -14,15 +14,13 @@
 #include <xen/types.h>
 #include <xen/percpu.h>
 
+/******************************/
+/*      x86 XSAVE/XRSTOR      */
+/******************************/
 struct vcpu;
 
 extern unsigned int xsave_cntxt_size;
 extern u64 xfeature_mask;
-
-void xsave_init(void);
-int xsave_alloc_save_area(struct vcpu *v);
-void xsave_free_save_area(struct vcpu *v);
-bool_t xsave_enabled(const struct vcpu *v);
 
 #define XSAVE_AREA_MIN_SIZE (512 + 64) /* FP/SSE + XSAVE.HEADER */
 #define XSTATE_FP       (1ULL << 0)

@@ -34,6 +32,16 @@
 #define XSTATE_YMM_OFFSET  XSAVE_AREA_MIN_SIZE
 #define XSTATE_YMM_SIZE    256
 #define XSAVEOPT        (1 << 0)
+
+#define XCR_XFEATURE_ENABLED_MASK   0
+
+#ifdef CONFIG_X86_64
+#define REX_PREFIX "0x48, "
+#else
+#define REX_PREFIX
+#endif
+
+DECLARE_PER_CPU(uint64_t, xcr0);
 
 struct xsave_struct
 {

@@ -48,15 +56,10 @@
     char   data[];                           /* Future new states */
 } __attribute__ ((packed, aligned (64)));
 
-#define XCR_XFEATURE_ENABLED_MASK   0
-
-#ifdef CONFIG_X86_64
-#define REX_PREFIX "0x48, "
-#else
-#define REX_PREFIX
-#endif
-
-DECLARE_PER_CPU(uint64_t, xcr0);
+void xsave_init(void);
+int xsave_alloc_save_area(struct vcpu *v);
+void xsave_free_save_area(struct vcpu *v);
+bool_t xsave_enabled(const struct vcpu *v);
 
 static inline void xsetbv(u32 index, u64 xfeatures)
 {

@@ -78,7 +81,10 @@
     return this_cpu(xcr0);
 }
 
-extern void setup_fpu(struct vcpu *v);
-extern void save_init_fpu(struct vcpu *v);
+/******************************/
+/*          x86 FPU           */
+/******************************/
+extern void fpu_save(struct vcpu *v);
+extern void fpu_restore(struct vcpu *v);
 
 #endif /* __ASM_I386_I387_H */

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

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

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:[email protected]] On Behalf Of Keir Fraser
Sent: Friday, April 15, 2011 4:20 AM
To: Huang2, Wei; '[email protected]'
Subject: Re: [Xen-devel] [PATCH][RFC] FPU LWP 1/5: clean up the FPU code

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