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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan


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