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:16:06 +0000 Message-ID: <56C723A6.40300@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aWlqu-0005yt-01 for xen-devel@lists.xenproject.org; Fri, 19 Feb 2016 14:16:12 +0000 In-Reply-To: <56C72FD602000078000D422E@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:08, Jan Beulich wrote: >>>> On 18.02.16 at 19:52, 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