Re: [PATCHv1 3/5] x86/fpu: Add a per-domain field to set the width of FIP/FDP

[David Vrabel <david.vrabel@citrix.com>]

On 19/02/16 15:43, David Vrabel wrote:
> On 19/02/16 15:14, Jan Beulich wrote:
>>
>> Iirc the issue was with a 64-bit XSAVE having got the selector
>> values stuck in via FNSTENV (forcing word size to 4), and a
>> subsequent XSAVEOPT not further touching the fields
>> (because no change to the FPU state had occurred), leading
>> to (without the code you're deleting) the word size for the
>> restore wrongly getting set to 8, and the selector values then
>> lost. I cannot see how this situation would be handled with
>> this patch in place.
> 
> Er, yes. You're right. Sorry.

Would this function be less confusing with something like this?

The resulting code is a lot smaller too (by 163 bytes) and has fewer branches.

--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -263,41 +263,24 @@ void xsave(struct vcpu *v, uint64_t mask)
 
     if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
     {
-        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
-        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
+        uint64_t bad_fip;
 
-        if ( cpu_has_xsaveopt || cpu_has_xsaves )
-        {
-            /*
-             * XSAVEOPT/XSAVES may not write the FPU portion even when the
-             * respective mask bit is set. For the check further down to work
-             * we hence need to put the save image back into the state that
-             * it was in right after the previous XSAVEOPT.
-             */
-            if ( word_size > 0 &&
-                 (ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] == 4 ||
-                  ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] == 2) )
-            {
-                ptr->fpu_sse.fip.sel = 0;
-                ptr->fpu_sse.fdp.sel = 0;
-            }
-        }
+        /*
+         * FIP/FDP may not be written in some cases (e.g., if
+         * XSAVEOPT/XSAVES is used, or on AMD CPUs if an exception
+         * isn't pending).
+         *
+         * To tell if the hardware writes these fields, make the FIP
+         * field non-canonical by flipping the top bit.
+         */
+        bad_fip = ptr->fpu_sse.fip.addr ^= 1 << 63;
 
         XSAVE("0x48,");
 
-        if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
-             /*
-              * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception
-              * is pending.
-              */
-             (!(ptr->fpu_sse.fsw & 0x0080) &&
-              boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
+        /* FIP/FDP not updated? Restore the old value. */
+        if ( ptr->fpu_sse.fip.addr == bad_fip )
         {
-            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
-            {
-                ptr->fpu_sse.fip.sel = fcs;
-                ptr->fpu_sse.fdp.sel = fds;
-            }
+            ptr->fpu_sse.fip.addr ^= 1 << 63;
             return;
         }