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:08, Jan Beulich wrote:
>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> wrote:
>> 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
>> - 64-bit PV guest: 8
> 
> The latter is wrong: 64-bit OSes may, for the benefit of compat
> mode processes, use 32-bit save/restore operations.

Ok. I'll leave PV guests as auto (unless FPCSDS feature is set).

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

David