From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCHv2 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields Date: Tue, 23 Feb 2016 17:42:19 +0000 Message-ID: <56CC99FB.9010805@citrix.com> References: <1456225539-9162-1-git-send-email-david.vrabel@citrix.com> <1456225539-9162-2-git-send-email-david.vrabel@citrix.com> <56CC81CC02000078000D5431@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.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1aYGyg-0008UC-3V for xen-devel@lists.xenproject.org; Tue, 23 Feb 2016 17:42:26 +0000 In-Reply-To: <56CC81CC02000078000D5431@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 , 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 14:59, Jan Beulich wrote: >>>> 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. I'm not sure what you're concerned about: a) Executing a FP instruction might load FIP with a non-canonical RIP? b) All 2^64 addresses might be canonical if the valid virtual address is 64-bits wide? c) A guest might load arbitrary data into a 64-bit wide FIP register (which may look like a non-canonical address)? But whatever, I'll drop this patch. (I would hope that new AMD processors go down the Intel route and remove the FCS/FDS registers.) David