From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCHv2 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields Date: Tue, 23 Feb 2016 07:59:08 -0700 Message-ID: <56CC81CC02000078000D5431@prv-mh.provo.novell.com> References: <1456225539-9162-1-git-send-email-david.vrabel@citrix.com> <1456225539-9162-2-git-send-email-david.vrabel@citrix.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 1aYEQk-00054X-JV for xen-devel@lists.xenproject.org; Tue, 23 Feb 2016 14:59:14 +0000 In-Reply-To: <1456225539-9162-2-git-send-email-david.vrabel@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel Cc: Kevin Tian , Suravee Suthikulpanit , Andrew Cooper , Donald D Dugger , Aravind Gopalakrishnan , Jun Nakajima , Sherry Hurwitz , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org >>> On 23.02.16 at 12:05, wrote: > --- a/xen/arch/x86/xstate.c > +++ b/xen/arch/x86/xstate.c > @@ -263,41 +263,24 @@ void xsave(struct vcpu *v, uint64_t mask) > > if ( word_size <= 0 || !is_pv_32bit_vcpu(v) ) > { > - typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel; > - typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel; > + uint64_t bad_fip; > > - 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; > - } > - } > + /* > + * FIP/FDP may not be written in some cases (e.g., if > + * XSAVEOPT/XSAVES is used, or on AMD CPUs if an exception > + * isn't pending). > + * > + * To tell if the hardware writes these fields, make the FIP > + * field non-canonical by flipping the top bit. > + */ > + bad_fip = ptr->fpu_sse.fip.addr ^= 1ull << 63; > > XSAVE("0x48,"); > > - if ( !(mask & ptr->xsave_hdr.xstate_bv & XSTATE_FP) || > - /* > - * AMD CPUs don't save/restore FDP/FIP/FOP unless an exception > - * is pending. > - */ > - (!(ptr->fpu_sse.fsw & 0x0080) && > - boot_cpu_data.x86_vendor == X86_VENDOR_AMD) ) > + /* FIP/FDP not updated? Restore the old FIP value. */ > + if ( ptr->fpu_sse.fip.addr == bad_fip ) > { > - if ( (cpu_has_xsaveopt || cpu_has_xsaves) && word_size > 0 ) > - { > - ptr->fpu_sse.fip.sel = fcs; > - ptr->fpu_sse.fdp.sel = fds; > - } > + ptr->fpu_sse.fip.addr ^= 1ull << 63; > return; > } While indeed this is a lot more simple, it puts us on thin ice, utilizing undocumented behavior: You make us depend on FIP actually being a 48-bit register which gets sign-extended to 64 bits upon saving, and truncated during restore. While all CPUs I've tested so far match this requirement, Intel ones (other than AMD's) do not match this in behavior for FDP. Since this already makes clear that AMD's are buggy (losing relevant state, since FPU operations using FS: or GS: may use non- canonical virtual addresses, becoming canonical once converted to linear ones) and hence need fixing, it would remain to be seen whether they wouldn't at once extend both FDP and FIP to 64 bits. Furthermore neither vendor's documentation describes such truncating behavior, i.e. purely based on that one might assume that these fields can be used as odd storage for 64 bits of arbitrary data each. Without a statement by both vendors that FIP is going to retain the currently observed behavior I don't think we can reasonably commit this change. I'm copying a number of people from both AMD and Intel, in the hope that they may provide clarification here. Jan