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

Commit Message

David Vrabel Feb. 19, 2016, 4:38 p.m. UTC

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.

Comments

Jan Beulich Feb. 19, 2016, 5:20 p.m. UTC | #1

>>> On 19.02.16 at 17:38, <[e-mail]> wrote:
> 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.

Well, if it works everywhere - why not.

Jan

> --- 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;
>          }

Patch

--- 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;
         }