[PATCHv2 0/3] x86: workaround inability to fully restore FPU state

[PATCHv2 0/3] x86: workaround inability to fully restore FPU state

[David Vrabel]

This series extends the workaround for the inability for some x86 CPUs
to fully restore the FPU exception state (64-bit FIP/FDP and FCS/FDS).

Toolstack (or the guest) may override the default behaviour to always
do a 32-bit save/restore.

Running Microsoft's Driver Verifier continues to work in a 32-bit
Windows guest and (if HVM_PARAM_X87_FIP_WIDTH is set to 4) now works
in a 64-bit Windows guest.

Changes in v2:

- Improve xsave()'s detection of whether the hardware updated FIP/FDP.
- Leave 64-bit PV guests in auto mode.
- Don't automatically set FIP width for Windows guests -- it's safer
  to leave auto-mode on and leave it up to the admin to enable the
  mode when running Driver Verifier.
- Use a HVM param to change the default.

David

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

[David Vrabel]

The hardware may not write the FIP/FDP fields with a XSAVE*
instruction.  e.g., with XSAVEOPT/XSAVES if the state hasn't changed
or on AMD CPUs when a floating point exception is not pending.  We
need to identify this case so we can correctly apply the check for
whether to save/restore FCS/FDS.

By toggling FIP[63] we can turn the field into a non-canonical address
and check for this value after the XSAVE instruction.

This results in smaller code with fewer branches and is more
understandable.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/arch/x86/xstate.c | 43 +++++++++++++------------------------------
 1 file changed, 13 insertions(+), 30 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 4f2fb8e..0869789 100644
--- 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;
         }
 
-- 
2.1.4

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

[Andrew Cooper]

On 23/02/16 11:05, David Vrabel wrote:
> The hardware may not write the FIP/FDP fields with a XSAVE*
> instruction.  e.g., with XSAVEOPT/XSAVES if the state hasn't changed
> or on AMD CPUs when a floating point exception is not pending.  We
> need to identify this case so we can correctly apply the check for
> whether to save/restore FCS/FDS.
>
> By toggling FIP[63] we can turn the field into a non-canonical address
> and check for this value after the XSAVE instruction.
>
> This results in smaller code with fewer branches and is more
> understandable.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

For consistently, the same change in detection logic should be applied
to fpu_fxsave()

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

[David Vrabel]

On 23/02/16 11:18, Andrew Cooper wrote:
> On 23/02/16 11:05, David Vrabel wrote:
>> The hardware may not write the FIP/FDP fields with a XSAVE*
>> instruction.  e.g., with XSAVEOPT/XSAVES if the state hasn't changed
>> or on AMD CPUs when a floating point exception is not pending.  We
>> need to identify this case so we can correctly apply the check for
>> whether to save/restore FCS/FDS.
>>
>> By toggling FIP[63] we can turn the field into a non-canonical address
>> and check for this value after the XSAVE instruction.
>>
>> This results in smaller code with fewer branches and is more
>> understandable.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> For consistently, the same change in detection logic should be applied
> to fpu_fxsave()

I don't think it is necessary.  fpu_fxsave() only needs to check for the
AMD case, and the logic is already simple.

David

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

[Jan Beulich]

>>> On 23.02.16 at 12:54, <david.vrabel@citrix.com> wrote:
> On 23/02/16 11:18, Andrew Cooper wrote:
>> On 23/02/16 11:05, David Vrabel wrote:
>>> The hardware may not write the FIP/FDP fields with a XSAVE*
>>> instruction.  e.g., with XSAVEOPT/XSAVES if the state hasn't changed
>>> or on AMD CPUs when a floating point exception is not pending.  We
>>> need to identify this case so we can correctly apply the check for
>>> whether to save/restore FCS/FDS.
>>>
>>> By toggling FIP[63] we can turn the field into a non-canonical address
>>> and check for this value after the XSAVE instruction.
>>>
>>> This results in smaller code with fewer branches and is more
>>> understandable.
>>>
>>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> 
>> For consistently, the same change in detection logic should be applied
>> to fpu_fxsave()
> 
> I don't think it is necessary.  fpu_fxsave() only needs to check for the
> AMD case, and the logic is already simple.

I agree.

Jan

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

[Jan Beulich]

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

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

[David Vrabel]

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?

c) A guest might load arbitrary data into a 64-bit wide FIP register
(which may look like a non-canonical address)?

But whatever, I'll drop this patch.

(I would hope that new AMD processors go down the Intel route and remove
the FCS/FDS registers.)

David

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

[Jan Beulich]

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

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

[Tian, Kevin]

> 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

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

[Jan Beulich]

>>> On 24.02.16 at 11:37, <kevin.tian@intel.com> wrote:
> 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?

What we'd like to get formally stated is whether FIP is guaranteed
to be treated as 48-bit pointer, which upon loading/storing by
64-bit {F,}X{XSAVE,RSTOR} will get truncated/canonicalized. With
FDP being a full 64-bit pointer on Intel CPUs (but only a 48 bit one
on AMD ones), and both your and their manuals implicitly describing
both as full 64-bit fields, FIP potentially also being a full 64-bit field
on past, present, or future CPUs would render David's intended
code improvement unsafe.

Jan

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

[Lai, Paul C]

Jan:

We have an answer from the architects:

"We can confirm that our CPUs do treat FIP as a 48-bit pointer.  When loaded by one of the restore instructions, bit 47 is sign-extended into bits 63:48, making the result canonical.  As a result, the save instructions will always save a canonical address for FIP."

Thanks for your patience,
-Paul

-----Original Message-----
From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Jan Beulich
Sent: Wednesday, February 24, 2016 2:50 AM
Subject: Re: [Xen-devel] [PATCHv2 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields

>>> On 24.02.16 at 11:37, <kevin.tian@intel.com> wrote:
> 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?

What we'd like to get formally stated is whether FIP is guaranteed to be treated as 48-bit pointer, which upon loading/storing by 64-bit {F,}X{XSAVE,RSTOR} will get truncated/canonicalized. With FDP being a full 64-bit pointer on Intel CPUs (but only a 48 bit one on AMD ones), and both your and their manuals implicitly describing both as full 64-bit fields, FIP potentially also being a full 64-bit field on past, present, or future CPUs would render David's intended code improvement unsafe.

Jan


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

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

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

[David Vrabel]

The x86 architecture allows either: a) the 64-bit FIP/FDP registers to
be restored (clearing FCS and FDS); or b) the 32-bit FIP/FDP and
FCS/FDS registers to be restored (clearing the upper 32-bits).

Add a per-domain field to indicate which of these options a guest
needs.  The options are: 8, 4 or 0.  Where 0 indicates that the
hypervisor should automatically guess the FIP width by checking the
value of FIP/FDP when saving the state (this is the existing
behaviour).

The FIP width is initially automatic but is set explicitly in the
following cases:

- 32-bit PV guest: 4
- Newer CPUs that do not save FCS/FDS: 8

The x87_fip_width field is placed into an existing 1 byte hole in
struct arch_domain.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
v2:
- Retain logic to detect if XSAVE* writes FIP/FDP.
- Keep 64-bit PV guests in auto-mode.
---
 xen/arch/x86/domain.c        | 10 ++++++++++
 xen/arch/x86/i387.c          | 15 ++++++++-------
 xen/arch/x86/xstate.c        | 18 +++++++++++-------
 xen/include/asm-x86/domain.h | 15 +++++++++++++++
 4 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 9d43f7b..e863bbd 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -343,6 +343,8 @@ int switch_native(struct domain *d)
             hvm_set_mode(v, 8);
     }
 
+    d->arch.x87_fip_width = cpu_has_fpu_sel ? 0 : 8;
+
     return 0;
 }
 
@@ -377,6 +379,8 @@ int switch_compat(struct domain *d)
 
     domain_set_alloc_bitsize(d);
 
+    d->arch.x87_fip_width = 4;
+
     return 0;
 
  undo_and_fail:
@@ -653,6 +657,12 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
     /* PV/PVH guests get an emulated PIT too for video BIOSes to use. */
     pit_init(d, cpu_khz);
 
+    /*
+     * If the FPU not to save FCS/FDS then we can always save/restore
+     * the 64-bit FIP/FDP and ignore the selectors.
+     */
+    d->arch.x87_fip_width = cpu_has_fpu_sel ? 0 : 8;
+
     return 0;
 
  fail:
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 67016c9..ef08a52 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -144,9 +144,9 @@ static inline void fpu_xsave(struct vcpu *v)
 static inline void fpu_fxsave(struct vcpu *v)
 {
     typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt;
-    int word_size = cpu_has_fpu_sel ? 8 : 0;
+    unsigned int fip_width = v->domain->arch.x87_fip_width;
 
-    if ( !is_pv_32bit_vcpu(v) )
+    if ( fip_width != 4 )
     {
         /*
          * The only way to force fxsaveq on a wide range of gas versions.
@@ -164,7 +164,7 @@ static inline void fpu_fxsave(struct vcpu *v)
              boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
             return;
 
-        if ( word_size > 0 &&
+        if ( !fip_width &&
              !((fpu_ctxt->fip.addr | fpu_ctxt->fdp.addr) >> 32) )
         {
             struct ix87_env fpu_env;
@@ -172,17 +172,18 @@ static inline void fpu_fxsave(struct vcpu *v)
             asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
             fpu_ctxt->fip.sel = fpu_env.fcs;
             fpu_ctxt->fdp.sel = fpu_env.fds;
-            word_size = 4;
+            fip_width = 4;
         }
+        else
+            fip_width = 8;
     }
     else
     {
         asm volatile ( "fxsave %0" : "=m" (*fpu_ctxt) );
-        word_size = 4;
+        fip_width = 4;
     }
 
-    if ( word_size >= 0 )
-        fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = word_size;
+    fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = fip_width;
 }
 
 /*******************************/
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 0869789..0d2a3a4 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -249,7 +249,7 @@ void xsave(struct vcpu *v, uint64_t mask)
     struct xsave_struct *ptr = v->arch.xsave_area;
     uint32_t hmask = mask >> 32;
     uint32_t lmask = mask;
-    int word_size = mask & XSTATE_FP ? (cpu_has_fpu_sel ? 8 : 0) : -1;
+    unsigned int fip_width = v->domain->arch.x87_fip_width;
 #define XSAVE(pfx) \
         alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \
                          ".byte " pfx "0x0f,0xae,0x37\n", /* xsaveopt */ \
@@ -261,7 +261,7 @@ void xsave(struct vcpu *v, uint64_t mask)
                          "=m" (*ptr), \
                          "a" (lmask), "d" (hmask), "D" (ptr))
 
-    if ( word_size <= 0 || !is_pv_32bit_vcpu(v) )
+    if ( fip_width != 4 )
     {
         uint64_t bad_fip;
 
@@ -284,7 +284,11 @@ void xsave(struct vcpu *v, uint64_t mask)
             return;
         }
 
-        if ( word_size > 0 &&
+        /*
+         * If the FIP/FDP[63:32] are both zero, it is safe to use the
+         * 32-bit restore to also restore the selectors.
+         */
+        if ( !fip_width &&
              !((ptr->fpu_sse.fip.addr | ptr->fpu_sse.fdp.addr) >> 32) )
         {
             struct ix87_env fpu_env;
@@ -292,17 +296,17 @@ void xsave(struct vcpu *v, uint64_t mask)
             asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
             ptr->fpu_sse.fip.sel = fpu_env.fcs;
             ptr->fpu_sse.fdp.sel = fpu_env.fds;
-            word_size = 4;
+            fip_width = 4;
         }
+        else
+            fip_width = 8;
     }
     else
     {
         XSAVE("");
-        word_size = 4;
     }
 #undef XSAVE
-    if ( word_size >= 0 )
-        ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
+    ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
 }
 
 void xrstor(struct vcpu *v, uint64_t mask)
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 4fad638..7135709 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -339,6 +339,21 @@ struct arch_domain
     u8 x86_vendor;           /* CPU vendor */
     u8 x86_model;            /* CPU model */
 
+    /*
+     * The width of the FIP/FDP register in the FPU that needs to be
+     * saved/restored during a context switch.  This is needed because
+     * the FPU can either: a) restore the 64-bit FIP/FDP and clear FCS
+     * and FDS; or b) restore the 32-bit FIP/FDP (clearing the upper
+     * 32-bits of FIP/FDP) and restore FCS/FDS.
+     *
+     * Which one is needed depends on the guest.
+     *
+     * This can be either: 8, 4 or 0.  0 means auto-detect the size
+     * based on the width of FIP/FDP values that are written by the
+     * guest.
+     */
+    uint8_t x87_fip_width;
+
     cpuid_input_t *cpuids;
 
     struct PITState vpit;
-- 
2.1.4

Re: [PATCHv2 2/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP

[Andrew Cooper]

On 23/02/16 11:05, David Vrabel wrote:
> @@ -653,6 +657,12 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>      /* PV/PVH guests get an emulated PIT too for video BIOSes to use. */
>      pit_init(d, cpu_khz);
>  
> +    /*
> +     * If the FPU not to save FCS/FDS then we can always save/restore

"If the FPU does not" ?

> @@ -284,7 +284,11 @@ void xsave(struct vcpu *v, uint64_t mask)
>              return;
>          }
>  
> -        if ( word_size > 0 &&
> +        /*
> +         * If the FIP/FDP[63:32] are both zero, it is safe to use the
> +         * 32-bit restore to also restore the selectors.
> +         */

This comment is presumably applicable to the fxsave path?

The functional content of the patch, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

Re: [PATCHv2 2/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP

[David Vrabel]

On 23/02/16 11:10, Andrew Cooper wrote:
> On 23/02/16 11:05, David Vrabel wrote:
>> @@ -653,6 +657,12 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags,
>>      /* PV/PVH guests get an emulated PIT too for video BIOSes to use. */
>>      pit_init(d, cpu_khz);
>>  
>> +    /*
>> +     * If the FPU not to save FCS/FDS then we can always save/restore
> 
> "If the FPU does not" ?
> 
>> @@ -284,7 +284,11 @@ void xsave(struct vcpu *v, uint64_t mask)
>>              return;
>>          }
>>  
>> -        if ( word_size > 0 &&
>> +        /*
>> +         * If the FIP/FDP[63:32] are both zero, it is safe to use the
>> +         * 32-bit restore to also restore the selectors.
>> +         */
> 
> This comment is presumably applicable to the fxsave path?

Yes.  Do you want this comment added there?

David

Re: [PATCHv2 2/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP

[Jan Beulich]

>>> On 23.02.16 at 12:05, <david.vrabel@citrix.com> wrote:
> @@ -292,17 +296,17 @@ void xsave(struct vcpu *v, uint64_t mask)
>              asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
>              ptr->fpu_sse.fip.sel = fpu_env.fcs;
>              ptr->fpu_sse.fdp.sel = fpu_env.fds;
> -            word_size = 4;
> +            fip_width = 4;
>          }
> +        else
> +            fip_width = 8;
>      }
>      else
>      {
>          XSAVE("");
> -        word_size = 4;
>      }
>  #undef XSAVE
> -    if ( word_size >= 0 )
> -        ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
> +    ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
>  }

There's actually a pre-existing bug here that I think should get
fixed at once: The 64-bit save path avoided to update
ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] when FPU state
didn't get saved (by setting word_size to -1), which continues to be
the case due to patch 1's changes. The 32-bit code path violated
this even before your change. I.e. the last else visible above
should get extended to return when !(mask & XSTATE_FP).

Jan

Re: [PATCHv2 2/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP

[David Vrabel]

On 23/02/16 15:24, Jan Beulich wrote:
>>>> On 23.02.16 at 12:05, <david.vrabel@citrix.com> wrote:
>> @@ -292,17 +296,17 @@ void xsave(struct vcpu *v, uint64_t mask)
>>              asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
>>              ptr->fpu_sse.fip.sel = fpu_env.fcs;
>>              ptr->fpu_sse.fdp.sel = fpu_env.fds;
>> -            word_size = 4;
>> +            fip_width = 4;
>>          }
>> +        else
>> +            fip_width = 8;
>>      }
>>      else
>>      {
>>          XSAVE("");
>> -        word_size = 4;
>>      }
>>  #undef XSAVE
>> -    if ( word_size >= 0 )
>> -        ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
>> +    ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
>>  }
> 
> There's actually a pre-existing bug here that I think should get
> fixed at once: The 64-bit save path avoided to update
> ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] when FPU state
> didn't get saved (by setting word_size to -1), which continues to be
> the case due to patch 1's changes. The 32-bit code path violated
> this even before your change. I.e. the last else visible above
> should get extended to return when !(mask & XSTATE_FP).

XSTATE_FP is always set in the mask. (see fpu_xsave()).

David

Re: [PATCHv2 2/3] x86/fpu: Add a per-domain field to set the width of FIP/FDP

[Jan Beulich]

>>> On 23.02.16 at 17:27, <david.vrabel@citrix.com> wrote:
> On 23/02/16 15:24, Jan Beulich wrote:
>>>>> On 23.02.16 at 12:05, <david.vrabel@citrix.com> wrote:
>>> @@ -292,17 +296,17 @@ void xsave(struct vcpu *v, uint64_t mask)
>>>              asm volatile ( "fnstenv %0" : "=m" (fpu_env) );
>>>              ptr->fpu_sse.fip.sel = fpu_env.fcs;
>>>              ptr->fpu_sse.fdp.sel = fpu_env.fds;
>>> -            word_size = 4;
>>> +            fip_width = 4;
>>>          }
>>> +        else
>>> +            fip_width = 8;
>>>      }
>>>      else
>>>      {
>>>          XSAVE("");
>>> -        word_size = 4;
>>>      }
>>>  #undef XSAVE
>>> -    if ( word_size >= 0 )
>>> -        ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
>>> +    ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
>>>  }
>> 
>> There's actually a pre-existing bug here that I think should get
>> fixed at once: The 64-bit save path avoided to update
>> ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] when FPU state
>> didn't get saved (by setting word_size to -1), which continues to be
>> the case due to patch 1's changes. The 32-bit code path violated
>> this even before your change. I.e. the last else visible above
>> should get extended to return when !(mask & XSTATE_FP).
> 
> XSTATE_FP is always set in the mask. (see fpu_xsave()).

fpu_xsave() sets the flag in XCR0, but not all values returned
from vcpu_xsave_mask() have that flag set (and hence the
"mask" parameter doesn't either).

Jan

[PATCHv2 3/3] x86/hvm: add HVM_PARAM_X87_FIP_WIDTH

[David Vrabel]

The HVM parameter HVM_PARAM_X87_FIP_WIDTH to allow tools and the guest
to adjust the width of the FIP/FDP registers to be saved/restored by
the hypervisor.  This is in case the hypervisor hueristics do not do
the right thing.

Add this parameter to the set saved during domain save/migrate.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 tools/libxc/xc_sr_save_x86_hvm.c |  1 +
 xen/arch/x86/hvm/hvm.c           | 13 +++++++++++++
 xen/include/public/hvm/params.h  | 24 +++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_sr_save_x86_hvm.c b/tools/libxc/xc_sr_save_x86_hvm.c
index e347b3b..ba50a43 100644
--- a/tools/libxc/xc_sr_save_x86_hvm.c
+++ b/tools/libxc/xc_sr_save_x86_hvm.c
@@ -76,6 +76,7 @@ static int write_hvm_params(struct xc_sr_context *ctx)
         HVM_PARAM_VM_GENERATION_ID_ADDR,
         HVM_PARAM_IOREQ_SERVER_PFN,
         HVM_PARAM_NR_IOREQ_SERVER_PAGES,
+        HVM_PARAM_X87_FIP_WIDTH,
     };
 
     xc_interface *xch = ctx->xch;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index fe382ce..3583c9e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -6027,6 +6027,7 @@ static int hvm_allow_set_param(struct domain *d,
     case HVM_PARAM_VM_GENERATION_ID_ADDR:
     case HVM_PARAM_STORE_EVTCHN:
     case HVM_PARAM_CONSOLE_EVTCHN:
+    case HVM_PARAM_X87_FIP_WIDTH:
         break;
     /*
      * The following parameters must not be set by the guest
@@ -6222,6 +6223,14 @@ static int hvmop_set_param(
 
         break;
     }
+    case HVM_PARAM_X87_FIP_WIDTH:
+        if ( a.value != 0 && a.value != 4 && a.value != 8 )
+        {
+            rc = -EINVAL;
+            break;
+        }
+        d->arch.x87_fip_width = a.value;
+        break;
     }
 
     if ( rc != 0 )
@@ -6258,6 +6267,7 @@ static int hvm_allow_get_param(struct domain *d,
     case HVM_PARAM_CONSOLE_PFN:
     case HVM_PARAM_CONSOLE_EVTCHN:
     case HVM_PARAM_ALTP2M:
+    case HVM_PARAM_X87_FIP_WIDTH:
         break;
     /*
      * The following parameters must not be read by the guest
@@ -6307,6 +6317,9 @@ static int hvmop_get_param(
     case HVM_PARAM_ACPI_S_STATE:
         a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0;
         break;
+    case HVM_PARAM_X87_FIP_WIDTH:
+        a.value = d->arch.x87_fip_width;
+        break;
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 81f9451..73d4718 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -210,6 +210,28 @@
 /* Boolean: Enable altp2m */
 #define HVM_PARAM_ALTP2M       35
 
-#define HVM_NR_PARAMS          36
+/*
+ * Size of the x87 FPU FIP/FDP registers that the hypervisor needs to
+ * save/restore.  This is a workaround for a hardware limitation that
+ * does not allow the full FIP/FDP and FCS/FDS to be restored.
+ *
+ * Valid values are:
+ *
+ * 8: save/restore 64-bit FIP/FDP and clear FCS/FDS (default if CPU
+ *    has FPCSDS feature).
+ *
+ * 4: save/restore 32-bit FIP/FDP, FCS/FDS, and clear upper 32-bits of
+ *    FIP/FDP.
+ *
+ * 0: allow hypervisor to choose based on the value of FIP/FDP
+ *    (default if CPU does not have FPCSDS).
+ *
+ * If FPCSDS (bit 13 in CPUID leaf 0x7, subleaf 0x0) is set, the CPU
+ * never saves FCS/FDS and this parameter should be left at the
+ * default of 8.
+ */
+#define HVM_PARAM_X87_FIP_WIDTH 36
+
+#define HVM_NR_PARAMS 37
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */
-- 
2.1.4

Re: [PATCHv2 3/3] x86/hvm: add HVM_PARAM_X87_FIP_WIDTH

[Andrew Cooper]

On 23/02/16 11:05, David Vrabel wrote:
> The HVM parameter HVM_PARAM_X87_FIP_WIDTH to allow tools and the guest
> to adjust the width of the FIP/FDP registers to be saved/restored by
> the hypervisor.  This is in case the hypervisor hueristics do not do
> the right thing.
>
> Add this parameter to the set saved during domain save/migrate.
>
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

CC'ing toolstack.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCHv2 3/3] x86/hvm: add HVM_PARAM_X87_FIP_WIDTH

[Wei Liu]

On Tue, Feb 23, 2016 at 11:20:37AM +0000, Andrew Cooper wrote:
> On 23/02/16 11:05, David Vrabel wrote:
> > The HVM parameter HVM_PARAM_X87_FIP_WIDTH to allow tools and the guest
> > to adjust the width of the FIP/FDP registers to be saved/restored by
> > the hypervisor.  This is in case the hypervisor hueristics do not do
> > the right thing.
> >
> > Add this parameter to the set saved during domain save/migrate.
> >
> > Signed-off-by: David Vrabel <david.vrabel@citrix.com>
> 
> CC'ing toolstack.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>