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

+        /*
+         * 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?

Jan


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