| Message ID | 20240808134150.29927-3-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: FPU handling cleanup | expand |
On 08.08.2024 15:41, Alejandro Vallejo wrote: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -1164,10 +1164,25 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h) > seg.attr = ctxt.ldtr_arbytes; > hvm_set_segment_register(v, x86_seg_ldtr, &seg); > > - /* Cover xsave-absent save file restoration on xsave-capable host. */ > - vcpu_setup_fpu(v, xsave_enabled(v) ? NULL : v->arch.xsave_area, > - ctxt.flags & XEN_X86_FPU_INITIALISED ? ctxt.fpu_regs : NULL, > - FCW_RESET); > + /* > + * On Xen 4.1 and later the FPU state is restored on later HVM context in > + * the migrate stream, so what we're doing here is initialising the FPU > + * state for guests from even older versions of Xen. > + * > + * In particular: > + * 1. If there's an XSAVE context later in the stream what we do here for > + * the FPU doesn't matter because it'll be overriden later. > + * 2. If there isn't and the guest didn't use extended states it's still > + * fine because we have all the information we need here. > + * 3. If there isn't and the guest DID use extended states (could've > + * happened prior to Xen 4.1) then we're in a pickle because we have > + * to make up non-existing state. For this case we initialise the FPU > + * as using x87/SSE only because the rest of the state is gone. Was this really possible to happen? Guests wouldn't have been able to turn on CR4.OSXSAVE, would they? Jan
On Mon Aug 12, 2024 at 4:23 PM BST, Jan Beulich wrote: > On 08.08.2024 15:41, Alejandro Vallejo wrote: > > --- a/xen/arch/x86/hvm/hvm.c > > +++ b/xen/arch/x86/hvm/hvm.c > > @@ -1164,10 +1164,25 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h) > > seg.attr = ctxt.ldtr_arbytes; > > hvm_set_segment_register(v, x86_seg_ldtr, &seg); > > > > - /* Cover xsave-absent save file restoration on xsave-capable host. */ > > - vcpu_setup_fpu(v, xsave_enabled(v) ? NULL : v->arch.xsave_area, > > - ctxt.flags & XEN_X86_FPU_INITIALISED ? ctxt.fpu_regs : NULL, > > - FCW_RESET); > > + /* > > + * On Xen 4.1 and later the FPU state is restored on later HVM context in > > + * the migrate stream, so what we're doing here is initialising the FPU > > + * state for guests from even older versions of Xen. > > + * > > + * In particular: > > + * 1. If there's an XSAVE context later in the stream what we do here for > > + * the FPU doesn't matter because it'll be overriden later. > > + * 2. If there isn't and the guest didn't use extended states it's still > > + * fine because we have all the information we need here. > > + * 3. If there isn't and the guest DID use extended states (could've > > + * happened prior to Xen 4.1) then we're in a pickle because we have > > + * to make up non-existing state. For this case we initialise the FPU > > + * as using x87/SSE only because the rest of the state is gone. > > Was this really possible to happen? Guests wouldn't have been able to > turn on CR4.OSXSAVE, would they? > > Jan You may be right, but my reading of the comment and the code was that xsave_enabled(v) might be set and the XSAVE hvm context might be missing in the stream. The archives didn't shed a lot more light than what the code already gives away. Otherwise it would've been far simpler to unconditionally pass v->arch.xsave_area to the second parameter and let the xsave area to be overriden by the follow-up HVM context with its actual state. If my understanding is wrong, I'm happy to remove (3), as I don't think it affects the code anyway. I thought however that it was a relevant data point to leave paper trail for. Cheers, Alejandro
On 13.08.2024 14:40, Alejandro Vallejo wrote: > On Mon Aug 12, 2024 at 4:23 PM BST, Jan Beulich wrote: >> On 08.08.2024 15:41, Alejandro Vallejo wrote: >>> --- a/xen/arch/x86/hvm/hvm.c >>> +++ b/xen/arch/x86/hvm/hvm.c >>> @@ -1164,10 +1164,25 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h) >>> seg.attr = ctxt.ldtr_arbytes; >>> hvm_set_segment_register(v, x86_seg_ldtr, &seg); >>> >>> - /* Cover xsave-absent save file restoration on xsave-capable host. */ >>> - vcpu_setup_fpu(v, xsave_enabled(v) ? NULL : v->arch.xsave_area, >>> - ctxt.flags & XEN_X86_FPU_INITIALISED ? ctxt.fpu_regs : NULL, >>> - FCW_RESET); >>> + /* >>> + * On Xen 4.1 and later the FPU state is restored on later HVM context in >>> + * the migrate stream, so what we're doing here is initialising the FPU >>> + * state for guests from even older versions of Xen. >>> + * >>> + * In particular: >>> + * 1. If there's an XSAVE context later in the stream what we do here for >>> + * the FPU doesn't matter because it'll be overriden later. >>> + * 2. If there isn't and the guest didn't use extended states it's still >>> + * fine because we have all the information we need here. >>> + * 3. If there isn't and the guest DID use extended states (could've >>> + * happened prior to Xen 4.1) then we're in a pickle because we have >>> + * to make up non-existing state. For this case we initialise the FPU >>> + * as using x87/SSE only because the rest of the state is gone. >> >> Was this really possible to happen? Guests wouldn't have been able to >> turn on CR4.OSXSAVE, would they? > > You may be right, but my reading of the comment and the code was that > xsave_enabled(v) might be set and the XSAVE hvm context might be missing in the > stream. The archives didn't shed a lot more light than what the code already > gives away. > > Otherwise it would've been far simpler to unconditionally pass > v->arch.xsave_area to the second parameter and let the xsave area to be > overriden by the follow-up HVM context with its actual state. > > If my understanding is wrong, I'm happy to remove (3), as I don't think it > affects the code anyway. I thought however that it was a relevant data point > to leave paper trail for. I would certainly agree - as long as it describes (past) reality. If it doesn't, I consider it misleading. Jan
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c index d977ec71ca20..5af9e3e7a8b4 100644 --- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -1186,9 +1186,10 @@ int arch_set_info_guest( is_pv_64bit_domain(d) ) v->arch.flags &= ~TF_kernel_mode; - vcpu_setup_fpu(v, v->arch.xsave_area, - flags & VGCF_I387_VALID ? &c.nat->fpu_ctxt : NULL, - FCW_DEFAULT); + if ( flags & VGCF_I387_VALID ) + vcpu_setup_fpu(v, &c.nat->fpu_ctxt); + else + vcpu_default_fpu(v); if ( !compat ) { diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 6607dba562a4..83cb21884ce6 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1164,10 +1164,25 @@ static int cf_check hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h) seg.attr = ctxt.ldtr_arbytes; hvm_set_segment_register(v, x86_seg_ldtr, &seg); - /* Cover xsave-absent save file restoration on xsave-capable host. */ - vcpu_setup_fpu(v, xsave_enabled(v) ? NULL : v->arch.xsave_area, - ctxt.flags & XEN_X86_FPU_INITIALISED ? ctxt.fpu_regs : NULL, - FCW_RESET); + /* + * On Xen 4.1 and later the FPU state is restored on later HVM context in + * the migrate stream, so what we're doing here is initialising the FPU + * state for guests from even older versions of Xen. + * + * In particular: + * 1. If there's an XSAVE context later in the stream what we do here for + * the FPU doesn't matter because it'll be overriden later. + * 2. If there isn't and the guest didn't use extended states it's still + * fine because we have all the information we need here. + * 3. If there isn't and the guest DID use extended states (could've + * happened prior to Xen 4.1) then we're in a pickle because we have + * to make up non-existing state. For this case we initialise the FPU + * as using x87/SSE only because the rest of the state is gone. + */ + if ( ctxt.flags & XEN_X86_FPU_INITIALISED ) + vcpu_setup_fpu(v, &ctxt.fpu_regs); + else + vcpu_reset_fpu(v); v->arch.user_regs.rax = ctxt.rax; v->arch.user_regs.rbx = ctxt.rbx; @@ -4007,9 +4022,7 @@ void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip) v->arch.guest_table = pagetable_null(); } - if ( v->arch.xsave_area ) - v->arch.xsave_area->xsave_hdr.xstate_bv = 0; - vcpu_setup_fpu(v, v->arch.xsave_area, NULL, FCW_RESET); + vcpu_reset_fpu(v); arch_vcpu_regs_init(v); v->arch.user_regs.rip = ip; diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c index fbb9d3584a3d..af5ae805998a 100644 --- a/xen/arch/x86/i387.c +++ b/xen/arch/x86/i387.c @@ -303,41 +303,37 @@ int vcpu_init_fpu(struct vcpu *v) return xstate_alloc_save_area(v); } -void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area, - const void *data, unsigned int fcw_default) +void vcpu_reset_fpu(struct vcpu *v) { - fpusse_t *fpu_sse = &v->arch.xsave_area->fpu_sse; - - ASSERT(!xsave_area || xsave_area == v->arch.xsave_area); - - v->fpu_initialised = !!data; + v->fpu_initialised = false; + *v->arch.xsave_area = (struct xsave_struct) { + .fpu_sse = { + .mxcsr = MXCSR_DEFAULT, + .fcw = FCW_RESET, + .ftw = FTW_RESET, + }, + .xsave_hdr.xstate_bv = fcw == X86_XCR0_X87, + }; +} - if ( data ) - { - memcpy(fpu_sse, data, sizeof(*fpu_sse)); - if ( xsave_area ) - xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE; - } - else if ( xsave_area && fcw_default == FCW_DEFAULT ) - { - xsave_area->xsave_hdr.xstate_bv = 0; - fpu_sse->mxcsr = MXCSR_DEFAULT; - } - else - { - memset(fpu_sse, 0, sizeof(*fpu_sse)); - fpu_sse->fcw = fcw_default; - fpu_sse->mxcsr = MXCSR_DEFAULT; - if ( v->arch.xsave_area ) - { - v->arch.xsave_area->xsave_hdr.xstate_bv &= ~XSTATE_FP_SSE; - if ( fcw_default != FCW_DEFAULT ) - v->arch.xsave_area->xsave_hdr.xstate_bv |= X86_XCR0_X87; - } - } +void vcpu_default_fpu(struct vcpu *v) +{ + v->fpu_initialised = false; + *v->arch.xsave_area = (struct xsave_struct) { + .fpu_sse = { + .mxcsr = MXCSR_DEFAULT, + .fcw = FCW_DEFAULT, + }, + }; +} - if ( xsave_area ) - xsave_area->xsave_hdr.xcomp_bv = 0; +void vcpu_setup_fpu(struct vcpu *v, const void *data) +{ + v->fpu_initialised = true; + *v->arch.xsave_area = (struct xsave_struct) { + .fpu_sse = *(const fpusse_t*)data, + .xsave_hdr.xstate_bv = XSTATE_FP_SSE, + }; } /* Free FPU's context save area */ diff --git a/xen/arch/x86/include/asm/i387.h b/xen/arch/x86/include/asm/i387.h index a783549db991..7a69577de45b 100644 --- a/xen/arch/x86/include/asm/i387.h +++ b/xen/arch/x86/include/asm/i387.h @@ -31,10 +31,30 @@ void vcpu_restore_fpu_nonlazy(struct vcpu *v, bool need_stts); void vcpu_restore_fpu_lazy(struct vcpu *v); void vcpu_save_fpu(struct vcpu *v); void save_fpu_enable(void); - int vcpu_init_fpu(struct vcpu *v); -struct xsave_struct; -void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area, - const void *data, unsigned int fcw_default); void vcpu_destroy_fpu(struct vcpu *v); + +/* + * Restore v's FPU to power-on reset values + * + * @param v vCPU containing the FPU + */ +void vcpu_reset_fpu(struct vcpu *v); + +/* + * Restore v's FPU to default values + * + * @param v vCPU containing the FPU + */ +void vcpu_default_fpu(struct vcpu *v); + +/* + * Load x87/SSE state into v's FPU + * + * Overrides the XSAVE header to set the state components to be x87 and SSE. + * + * @param v vCPU containing the FPU + * @param data 512-octet blob for x87/SSE state + */ +void vcpu_setup_fpu(struct vcpu *v, const void *data); #endif /* __ASM_I386_I387_H */ diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h index ebeb2a3dcaf9..6144ed6f8551 100644 --- a/xen/arch/x86/include/asm/xstate.h +++ b/xen/arch/x86/include/asm/xstate.h @@ -14,6 +14,7 @@ #define FCW_DEFAULT 0x037f #define FCW_RESET 0x0040 +#define FTW_RESET 0xFF #define MXCSR_DEFAULT 0x1f80 extern uint32_t mxcsr_mask;
It was trying to do too many things at once and there was no clear way of defining what it was meant to do.This commit splits the function in two. 1. A reset function, parameterized by the FCW value. FCW_RESET means to reset the state to power-on reset values, while FCW_DEFAULT means to reset to the default values present during vCPU creation. 2. A x87/SSE state loader (equivalent to the old function when it took a data pointer). While at it, make sure the abridged tag is consistent with the manuals and start as 0xFF. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- v2: * Reworded comment about pre-Xen 4.1 migrations. * Reset abridged FTW to -1 (tag=0x5555), as per the manuals. * Undo const cast-away. * Split vcpu_reset_fpu() into vcpu_default_fpu() * vcpu_init_fpu() already exists. * Removed backticks from comments --- xen/arch/x86/domain.c | 7 ++-- xen/arch/x86/hvm/hvm.c | 27 ++++++++++---- xen/arch/x86/i387.c | 60 +++++++++++++++---------------- xen/arch/x86/include/asm/i387.h | 28 ++++++++++++--- xen/arch/x86/include/asm/xstate.h | 1 + 5 files changed, 77 insertions(+), 46 deletions(-)