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 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.

However, to make progress with the actual issue, perhaps
re-ordering the series might be worth considering.

Jan