Re: [PATCHv2 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

["Jan Beulich" <JBeulich@suse.com>]

>>> On 23.02.16 at 12:05, <david.vrabel@citrix.com> wrote:
> --- 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 ^= 1ull << 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 FIP 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 ^= 1ull << 63;
>              return;
>          }

While indeed this is a lot more simple, it puts us on thin ice,
utilizing undocumented behavior: You make us depend on FIP
actually being a 48-bit register which gets sign-extended to 64
bits upon saving, and truncated during restore. While all CPUs
I've tested so far match this requirement, Intel ones (other
than AMD's) do not match this in behavior for FDP. Since this
already makes clear that AMD's are buggy (losing relevant
state, since FPU operations using FS: or GS: may use non-
canonical virtual addresses, becoming canonical once
converted to linear ones) and hence need fixing, it would
remain to be seen whether they wouldn't at once extend both
FDP and FIP to 64 bits.

Furthermore neither vendor's documentation describes such
truncating behavior, i.e. purely based on that one might
assume that these fields can be used as odd storage for 64
bits of arbitrary data each.

Without a statement by both vendors that FIP is going to
retain the currently observed behavior I don't think we can
reasonably commit this change. I'm copying a number of
people from both AMD and Intel, in the hope that they may
provide clarification here.

Jan