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

["Tian, Kevin" <kevin.tian@intel.com>]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Wednesday, February 24, 2016 3:51 PM
> 
> >>> On 23.02.16 at 18:42, <david.vrabel@citrix.com> wrote:
> > On 23/02/16 14:59, Jan Beulich wrote:
> >>>>> 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.
> >
> > I'm not sure what you're concerned about:
> >
> > a) Executing a FP instruction might load FIP with a non-canonical RIP?
> >
> > b) All 2^64 addresses might be canonical if the valid virtual address is
> > 64-bits wide?
> 
> Neither of these two, ...
> 
> > c) A guest might load arbitrary data into a 64-bit wide FIP register
> > (which may look like a non-canonical address)?
> 
> ... but this one.
> 
> > But whatever, I'll drop this patch.
> 
> Prior to dropping, perhaps we should indeed see if we can get
> feedback from Intel and AMD. If the currently observed behavior
> would get documented (for at least FIP), the patch would be fine.
> 

Sorry I didn't quite get the question here. Could anyone of you
write down a standalone description of the problem then I can
forward internally to confirm since my translation might be
inaccurate here?

Thanks
Kevin