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

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 19/02/16 15:14, Jan Beulich wrote:
>>>> On 19.02.16 at 15:49, <david.vrabel@xxxxxxxxxx> wrote:
>> On 19/02/16 14:36, Jan Beulich wrote:
>>>>>> On 19.02.16 at 15:16, <david.vrabel@xxxxxxxxxx> wrote:
>>>> On 19/02/16 14:08, Jan Beulich wrote:
>>>>>>>> On 18.02.16 at 19:52, <david.vrabel@xxxxxxxxxx> 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.
> 
> My initial comment referred to all of the still quoted code, while
> you now seem to think it was only about the second of those
> hunks.
> 
>>> +        /*
>>> +         * 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].
> 
> Iirc the issue was with a 64-bit XSAVE having got the selector
> values stuck in via FNSTENV (forcing word size to 4), and a
> subsequent XSAVEOPT not further touching the fields
> (because no change to the FPU state had occurred), leading
> to (without the code you're deleting) the word size for the
> restore wrongly getting set to 8, and the selector values then
> lost. I cannot see how this situation would be handled with
> this patch in place.

Er, yes. You're right. Sorry.

David

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