From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCHv1 3/5] x86/fpu: Add a per-domain field to set the width of FIP/FDP Date: Fri, 19 Feb 2016 14:49:03 +0000 Message-ID: <56C72B5F.6030305@citrix.com> References: <1455821530-4263-1-git-send-email-david.vrabel@citrix.com> <1455821530-4263-4-git-send-email-david.vrabel@citrix.com> <56C72FD602000078000D422E@prv-mh.provo.novell.com> <56C723A6.40300@citrix.com> <56C7368702000078000D4297@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aWmMm-000249-5G for xen-devel@lists.xenproject.org; Fri, 19 Feb 2016 14:49:08 +0000 In-Reply-To: <56C7368702000078000D4297@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Andrew Cooper , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 19/02/16 14:36, Jan Beulich wrote: >>>> On 19.02.16 at 15:16, wrote: >> On 19/02/16 14:08, Jan Beulich wrote: >>>>>> On 18.02.16 at 19:52, 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