| Message ID | 1455821530-4263-4-git-send-email-david.vrabel@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/02/16 18:52, David Vrabel 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). ^ of FIP/FDP > diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h > index 4fad638..362fd8f 100644 > --- a/xen/include/asm-x86/domain.h > +++ b/xen/include/asm-x86/domain.h > @@ -394,6 +394,21 @@ struct arch_domain > > /* Emulated devices enabled bitmap. */ > uint32_t emulation_flags; > + > + /* > + * The width of the FIP/FDP register in the FPU that needs to be > + * saved/restored during a context switch. This is needed because > + * the FPU can either: a) restore the 64-bit FIP/FDP and clear FCS > + * and FDS; or b) retstore the 32-bit FIP/FDP (clearing the upper > + * 32-bits) and restore FCS/FDS. > + * > + * Which one is needed depends on the guest. > + * > + * This can be either: 8, 4 or 0. 0 means auto-detect the size > + * based on the width of FIP/FDP values that are written by by the > + * guest. > + */ > + uint8_t x87_fip_width; Can we get away with always using fpu_sse.x[FPU_WORD_SIZE_OFFSET], instead if duplicating the information in arch_domain and risking them getting out of sync? VMs which have migrated in will already have some policy latched, and we should preserve their old behaviour if possible. ~Andrew
On 18/02/16 19:13, Andrew Cooper wrote: > On 18/02/16 18:52, David Vrabel wrote: >> --- a/xen/include/asm-x86/domain.h >> +++ b/xen/include/asm-x86/domain.h >> @@ -394,6 +394,21 @@ struct arch_domain >> >> /* Emulated devices enabled bitmap. */ >> uint32_t emulation_flags; >> + >> + /* >> + * The width of the FIP/FDP register in the FPU that needs to be >> + * saved/restored during a context switch. This is needed because >> + * the FPU can either: a) restore the 64-bit FIP/FDP and clear FCS >> + * and FDS; or b) retstore the 32-bit FIP/FDP (clearing the upper >> + * 32-bits) and restore FCS/FDS. >> + * >> + * Which one is needed depends on the guest. >> + * >> + * This can be either: 8, 4 or 0. 0 means auto-detect the size >> + * based on the width of FIP/FDP values that are written by by the >> + * guest. >> + */ >> + uint8_t x87_fip_width; > > Can we get away with always using fpu_sse.x[FPU_WORD_SIZE_OFFSET], > instead if duplicating the information in arch_domain and risking them > getting out of sync? No. Because if x87_fip_width == 0 (auto-mode) we check every save for the correct width to use, storing the result at FPU_WORD_SIZE_OFFSET. > VMs which have migrated in will already have some policy latched, and we > should preserve their old behaviour if possible. This does. x87_fip_width is only used on save. The restore behaviour is unchanged and is conditional on the word size written to the save state. David
>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> 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. > @@ -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? Jan
On 19/02/16 14:08, Jan Beulich wrote: >>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> 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
>>> On 19.02.16 at 15:16, <david.vrabel@citrix.com> wrote: > On 19/02/16 14:08, Jan Beulich wrote: >>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> 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: + /* + * 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? Jan
On 19/02/16 14:36, Jan Beulich wrote: >>>> On 19.02.16 at 15:16, <david.vrabel@citrix.com> wrote: >> On 19/02/16 14:08, Jan Beulich wrote: >>>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> 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
>>> On 19.02.16 at 15:49, <david.vrabel@citrix.com> wrote: > On 19/02/16 14:36, Jan Beulich wrote: >>>>> On 19.02.16 at 15:16, <david.vrabel@citrix.com> wrote: >>> On 19/02/16 14:08, Jan Beulich wrote: >>>>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> 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. My initial comment referred to all of the still quoted code, while you now seem to think it was only about the second of those hunks. >> + /* >> + * 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]. Iirc the issue was with a 64-bit XSAVE having got the selector values stuck in via FNSTENV (forcing word size to 4), and a subsequent XSAVEOPT not further touching the fields (because no change to the FPU state had occurred), leading to (without the code you're deleting) the word size for the restore wrongly getting set to 8, and the selector values then lost. I cannot see how this situation would be handled with this patch in place. Jan
On 19/02/16 15:14, Jan Beulich wrote: >>>> On 19.02.16 at 15:49, <david.vrabel@citrix.com> wrote: >> On 19/02/16 14:36, Jan Beulich wrote: >>>>>> On 19.02.16 at 15:16, <david.vrabel@citrix.com> wrote: >>>> On 19/02/16 14:08, Jan Beulich wrote: >>>>>>>> On 18.02.16 at 19:52, <david.vrabel@citrix.com> 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. > > My initial comment referred to all of the still quoted code, while > you now seem to think it was only about the second of those > hunks. > >>> + /* >>> + * 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]. > > Iirc the issue was with a 64-bit XSAVE having got the selector > values stuck in via FNSTENV (forcing word size to 4), and a > subsequent XSAVEOPT not further touching the fields > (because no change to the FPU state had occurred), leading > to (without the code you're deleting) the word size for the > restore wrongly getting set to 8, and the selector values then > lost. I cannot see how this situation would be handled with > this patch in place. Er, yes. You're right. Sorry. David
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index 9d43f7b..a15cdf7 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -343,6 +343,8 @@ int switch_native(struct domain *d) hvm_set_mode(v, 8); } + d->arch.x87_fip_width = 8; + return 0; } @@ -377,6 +379,8 @@ int switch_compat(struct domain *d) domain_set_alloc_bitsize(d); + d->arch.x87_fip_width = 4; + return 0; undo_and_fail: @@ -653,6 +657,12 @@ int arch_domain_create(struct domain *d, unsigned int domcr_flags, /* PV/PVH guests get an emulated PIT too for video BIOSes to use. */ pit_init(d, cpu_khz); + /* + * If the FPU not to save FCS/FDS then we can always save/restore + * the 64-bit FIP/FDP and ignore the selectors. + */ + d->arch.x87_fip_width = cpu_has_fpu_sel ? 0 : 8; + return 0; fail: diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c index 67016c9..ef08a52 100644 --- a/xen/arch/x86/i387.c +++ b/xen/arch/x86/i387.c @@ -144,9 +144,9 @@ static inline void fpu_xsave(struct vcpu *v) static inline void fpu_fxsave(struct vcpu *v) { typeof(v->arch.xsave_area->fpu_sse) *fpu_ctxt = v->arch.fpu_ctxt; - int word_size = cpu_has_fpu_sel ? 8 : 0; + unsigned int fip_width = v->domain->arch.x87_fip_width; - if ( !is_pv_32bit_vcpu(v) ) + if ( fip_width != 4 ) { /* * The only way to force fxsaveq on a wide range of gas versions. @@ -164,7 +164,7 @@ static inline void fpu_fxsave(struct vcpu *v) boot_cpu_data.x86_vendor == X86_VENDOR_AMD ) return; - if ( word_size > 0 && + if ( !fip_width && !((fpu_ctxt->fip.addr | fpu_ctxt->fdp.addr) >> 32) ) { struct ix87_env fpu_env; @@ -172,17 +172,18 @@ static inline void fpu_fxsave(struct vcpu *v) asm volatile ( "fnstenv %0" : "=m" (fpu_env) ); fpu_ctxt->fip.sel = fpu_env.fcs; fpu_ctxt->fdp.sel = fpu_env.fds; - word_size = 4; + fip_width = 4; } + else + fip_width = 8; } else { asm volatile ( "fxsave %0" : "=m" (*fpu_ctxt) ); - word_size = 4; + fip_width = 4; } - if ( word_size >= 0 ) - fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = word_size; + fpu_ctxt->x[FPU_WORD_SIZE_OFFSET] = fip_width; } /*******************************/ diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c index 4f2fb8e..c4cc071 100644 --- a/xen/arch/x86/xstate.c +++ b/xen/arch/x86/xstate.c @@ -249,7 +249,7 @@ void xsave(struct vcpu *v, uint64_t mask) struct xsave_struct *ptr = v->arch.xsave_area; uint32_t hmask = mask >> 32; uint32_t lmask = mask; - int word_size = mask & XSTATE_FP ? (cpu_has_fpu_sel ? 8 : 0) : -1; + unsigned int fip_width = v->domain->arch.x87_fip_width; #define XSAVE(pfx) \ alternative_io_3(".byte " pfx "0x0f,0xae,0x27\n", /* xsave */ \ ".byte " pfx "0x0f,0xae,0x37\n", /* xsaveopt */ \ @@ -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; } - if ( word_size > 0 && + /* + * 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; @@ -309,17 +288,17 @@ void xsave(struct vcpu *v, uint64_t mask) asm volatile ( "fnstenv %0" : "=m" (fpu_env) ); ptr->fpu_sse.fip.sel = fpu_env.fcs; ptr->fpu_sse.fdp.sel = fpu_env.fds; - word_size = 4; + fip_width = 4; } + else + fip_width = 8; } else { XSAVE(""); - word_size = 4; } #undef XSAVE - if ( word_size >= 0 ) - ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size; + ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width; } void xrstor(struct vcpu *v, uint64_t mask) diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h index 4fad638..362fd8f 100644 --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -394,6 +394,21 @@ struct arch_domain /* Emulated devices enabled bitmap. */ uint32_t emulation_flags; + + /* + * The width of the FIP/FDP register in the FPU that needs to be + * saved/restored during a context switch. This is needed because + * the FPU can either: a) restore the 64-bit FIP/FDP and clear FCS + * and FDS; or b) retstore the 32-bit FIP/FDP (clearing the upper + * 32-bits) and restore FCS/FDS. + * + * Which one is needed depends on the guest. + * + * This can be either: 8, 4 or 0. 0 means auto-detect the size + * based on the width of FIP/FDP values that are written by by the + * guest. + */ + uint8_t x87_fip_width; } __cacheline_aligned; #define has_vlapic(d) (!!((d)->arch.emulation_flags & XEN_X86_EMU_LAPIC))
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 - Newer CPUs that do not save FCS/FDS: 8 xsave() has been simplified to not require clearing the FCS/FDS fields -- either these are updated by the XSAVE* instruction or they are not written. Signed-off-by: David Vrabel <david.vrabel@citrix.com> --- xen/arch/x86/domain.c | 10 ++++++++++ xen/arch/x86/i387.c | 15 ++++++++------- xen/arch/x86/xstate.c | 43 +++++++++++-------------------------------- xen/include/asm-x86/domain.h | 15 +++++++++++++++ 4 files changed, 44 insertions(+), 39 deletions(-)