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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.02.16 at 17:38, <david.vrabel@xxxxxxxxxx> 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;
>          }




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel