| Message ID | 1456225539-9162-2-git-send-email-david.vrabel@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23/02/16 11:05, David Vrabel wrote: > The hardware may not write the FIP/FDP fields with a XSAVE* > instruction. e.g., with XSAVEOPT/XSAVES if the state hasn't changed > or on AMD CPUs when a floating point exception is not pending. We > need to identify this case so we can correctly apply the check for > whether to save/restore FCS/FDS. > > By toggling FIP[63] we can turn the field into a non-canonical address > and check for this value after the XSAVE instruction. > > This results in smaller code with fewer branches and is more > understandable. > > Signed-off-by: David Vrabel <david.vrabel@citrix.com> For consistently, the same change in detection logic should be applied to fpu_fxsave() Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 23/02/16 11:18, Andrew Cooper wrote: > On 23/02/16 11:05, David Vrabel wrote: >> The hardware may not write the FIP/FDP fields with a XSAVE* >> instruction. e.g., with XSAVEOPT/XSAVES if the state hasn't changed >> or on AMD CPUs when a floating point exception is not pending. We >> need to identify this case so we can correctly apply the check for >> whether to save/restore FCS/FDS. >> >> By toggling FIP[63] we can turn the field into a non-canonical address >> and check for this value after the XSAVE instruction. >> >> This results in smaller code with fewer branches and is more >> understandable. >> >> Signed-off-by: David Vrabel <david.vrabel@citrix.com> > > For consistently, the same change in detection logic should be applied > to fpu_fxsave() I don't think it is necessary. fpu_fxsave() only needs to check for the AMD case, and the logic is already simple. David
>>> On 23.02.16 at 12:54, <david.vrabel@citrix.com> wrote: > On 23/02/16 11:18, Andrew Cooper wrote: >> On 23/02/16 11:05, David Vrabel wrote: >>> The hardware may not write the FIP/FDP fields with a XSAVE* >>> instruction. e.g., with XSAVEOPT/XSAVES if the state hasn't changed >>> or on AMD CPUs when a floating point exception is not pending. We >>> need to identify this case so we can correctly apply the check for >>> whether to save/restore FCS/FDS. >>> >>> By toggling FIP[63] we can turn the field into a non-canonical address >>> and check for this value after the XSAVE instruction. >>> >>> This results in smaller code with fewer branches and is more >>> understandable. >>> >>> Signed-off-by: David Vrabel <david.vrabel@citrix.com> >> >> For consistently, the same change in detection logic should be applied >> to fpu_fxsave() > > I don't think it is necessary. fpu_fxsave() only needs to check for the > AMD case, and the logic is already simple. I agree. Jan
>>> On 23.02.16 at 12:05, <david.vrabel@citrix.com> 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
On 23/02/16 14:59, Jan Beulich wrote: >>>> On 23.02.16 at 12:05, <david.vrabel@citrix.com> 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
>>> On 23.02.16 at 18:42, <david.vrabel@citrix.com> wrote: > On 23/02/16 14:59, Jan Beulich wrote: >>>>> On 23.02.16 at 12:05, <david.vrabel@citrix.com> 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? Neither of these two, ... > c) A guest might load arbitrary data into a 64-bit wide FIP register > (which may look like a non-canonical address)? ... but this one. > But whatever, I'll drop this patch. Prior to dropping, perhaps we should indeed see if we can get feedback from Intel and AMD. If the currently observed behavior would get documented (for at least FIP), the patch would be fine. However, to make progress with the actual issue, perhaps re-ordering the series might be worth considering. Jan
> From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: Wednesday, February 24, 2016 3:51 PM > > >>> On 23.02.16 at 18:42, <david.vrabel@citrix.com> wrote: > > On 23/02/16 14:59, Jan Beulich wrote: > >>>>> On 23.02.16 at 12:05, <david.vrabel@citrix.com> 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? > > Neither of these two, ... > > > c) A guest might load arbitrary data into a 64-bit wide FIP register > > (which may look like a non-canonical address)? > > ... but this one. > > > But whatever, I'll drop this patch. > > Prior to dropping, perhaps we should indeed see if we can get > feedback from Intel and AMD. If the currently observed behavior > would get documented (for at least FIP), the patch would be fine. > Sorry I didn't quite get the question here. Could anyone of you write down a standalone description of the problem then I can forward internally to confirm since my translation might be inaccurate here? Thanks Kevin
>>> On 24.02.16 at 11:37, <kevin.tian@intel.com> wrote: > Sorry I didn't quite get the question here. Could anyone of you > write down a standalone description of the problem then I can > forward internally to confirm since my translation might be > inaccurate here? What we'd like to get formally stated is whether FIP is guaranteed to be treated as 48-bit pointer, which upon loading/storing by 64-bit {F,}X{XSAVE,RSTOR} will get truncated/canonicalized. With FDP being a full 64-bit pointer on Intel CPUs (but only a 48 bit one on AMD ones), and both your and their manuals implicitly describing both as full 64-bit fields, FIP potentially also being a full 64-bit field on past, present, or future CPUs would render David's intended code improvement unsafe. Jan
Jan: We have an answer from the architects: "We can confirm that our CPUs do treat FIP as a 48-bit pointer. When loaded by one of the restore instructions, bit 47 is sign-extended into bits 63:48, making the result canonical. As a result, the save instructions will always save a canonical address for FIP." Thanks for your patience, -Paul -----Original Message----- From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Jan Beulich Sent: Wednesday, February 24, 2016 2:50 AM Subject: Re: [Xen-devel] [PATCHv2 1/3] x86/fpu: improve check for XSAVE* not writing FIP/FDP fields >>> On 24.02.16 at 11:37, <kevin.tian@intel.com> wrote: > Sorry I didn't quite get the question here. Could anyone of you write > down a standalone description of the problem then I can forward > internally to confirm since my translation might be inaccurate here? What we'd like to get formally stated is whether FIP is guaranteed to be treated as 48-bit pointer, which upon loading/storing by 64-bit {F,}X{XSAVE,RSTOR} will get truncated/canonicalized. With FDP being a full 64-bit pointer on Intel CPUs (but only a 48 bit one on AMD ones), and both your and their manuals implicitly describing both as full 64-bit fields, FIP potentially also being a full 64-bit field on past, present, or future CPUs would render David's intended code improvement unsafe. Jan
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c index 4f2fb8e..0869789 100644 --- 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; }
The hardware may not write the FIP/FDP fields with a XSAVE* instruction. e.g., with XSAVEOPT/XSAVES if the state hasn't changed or on AMD CPUs when a floating point exception is not pending. We need to identify this case so we can correctly apply the check for whether to save/restore FCS/FDS. By toggling FIP[63] we can turn the field into a non-canonical address and check for this value after the XSAVE instruction. This results in smaller code with fewer branches and is more understandable. Signed-off-by: David Vrabel <david.vrabel@citrix.com> --- xen/arch/x86/xstate.c | 43 +++++++++++++------------------------------ 1 file changed, 13 insertions(+), 30 deletions(-)