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

[David Vrabel <david.vrabel@citrix.com>]

On 19/02/16 14:36, Jan Beulich wrote:
>>>> On 19.02.16 at 15:16, <david.vrabel@citrix.com> wrote:
>> On 19/02/16 14:08, Jan Beulich wrote:
>>>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> wrote:
>>>> @@ -261,28 +261,8 @@ 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 )
>>>>      {
>>>> -        typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>>>> -        typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
>>>> -
>>>> -        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;
>>>> -            }
>>>> -        }
>>>> -
>>>>          XSAVE("0x48,");
>>>>  
>>>>          if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) ||
>>>> @@ -293,15 +273,14 @@ void xsave(struct vcpu *v, uint64_t mask)
>>>>               (!(ptr->fpu_sse.fsw & 0x0080) &&
>>>>                boot_cpu_data.x86_vendor == X86_VENDOR_AMD) )
>>>>          {
>>>> -            if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 )
>>>> -            {
>>>> -                ptr->fpu_sse.fip.sel = fcs;
>>>> -                ptr->fpu_sse.fdp.sel = fds;
>>>> -            }
>>>>              return;
>>>
>>> I don't see how you can validly delete all of the above code without
>>> any replacement. Can you explain the rationale behind this?
>>
>> I think it is unnecessary.
>>
>> If XSAVEOPT/XSAVES doesn't save the FP state, it hasn't changed since
>> last time and we don't need to clear and rewrite the FCS/FDS fields
>> since the old values are still valid.
> 
> Well, this logic fixed a particular issue, and I'd like to be sure the
> deletion won't re-introduce that issue. In particular your patch
> doesn't touch the place where the zero values are actually being
> looked at:

This was added because FCS/FDS were being cleared.  Without initially
clearing these fields, they do not need to be restored if the hardware
did not write them.

> +        /*
> +         * 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;
> 
> So how is this check going to fulfill its purpose now?

This is checking the values just written by the XSAVE* instruction.
Note that the FCS/FDS in the state overlap with the FIP/FDP[47:32].

David