| Message ID | 5672F7CE02000078000C0D8F@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 17/12/15 16:58, Jan Beulich wrote: > This eliminates a number of pointless conditionals: Bits 0 and 1 of > xcomp_bv don't matter anyway, and as long as none of bits 2..62 are > set, setting bit 63 is pointless too. xcomp_bv[63] is the indication of whether the area is compressed or not. It will be set by the processor when an XSAVEC instruction is issued, and until that point, it doesn't particularly matter as no bits are set. However, it is an ABI indication of the size of the xsave area. It turns out that we are redundantly allocating an uncompressed xsave, even when we will only used the compressed form. For now, I don't think there is any harm, as the lack of XSTATE_COMPACTION_ENABLED causes the size indication to actually be correct, but it will become necessary when we start making use of the optimisations that compaction allows. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> (On that note, I really should make some patches for my planned cleanup for XSA-165. I have just been too busy recently.)
On Thu, Dec 17, 2015 at 09:58:38AM -0700, Jan Beulich wrote: > This eliminates a number of pointless conditionals: Bits 0 and 1 of > xcomp_bv don't matter anyway, and as long as none of bits 2..62 are > set, setting bit 63 is pointless too. > We should set bit 63 of the xcomp_bv when initialization. From the initialization log of xen, I got that xrstors will excute first(before xsaves). xrstors will check bit 63 of xcopm_bv. If it is not set, on the xsaves-support machine xen will hung. (I test the patch on skylake machine base on the master branch) > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Noticed while working on XSA-165. > > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -912,9 +912,7 @@ int arch_set_info_guest( > if ( v->arch.xsave_area ) > { > v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE; > - if ( cpu_has_xsaves || cpu_has_xsavec ) > - v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE | > - XSTATE_COMPACTION_ENABLED; > + v->arch.xsave_area->xsave_hdr.xcomp_bv = 0; > } > } > else if ( v->arch.xsave_area ) > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2092,9 +2092,7 @@ static int hvm_load_cpu_ctxt(struct doma > > memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs)); > xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE; > - if ( cpu_has_xsaves || cpu_has_xsavec ) > - xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE | > - XSTATE_COMPACTION_ENABLED; > + xsave_area->xsave_hdr.xcomp_bv = 0; > } > else > memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs)); > @@ -5562,9 +5560,7 @@ void hvm_vcpu_reset_state(struct vcpu *v > if ( v->arch.xsave_area ) > { > v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP; > - if ( cpu_has_xsaves || cpu_has_xsavec ) > - v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP | > - XSTATE_COMPACTION_ENABLED; > + v->arch.xsave_area->xsave_hdr.xcomp_bv = 0; > } > > v->arch.vgc_flags = VGCF_online; > --- a/xen/arch/x86/i387.c > +++ b/xen/arch/x86/i387.c > @@ -270,11 +270,7 @@ int vcpu_init_fpu(struct vcpu *v) > return rc; > > if ( v->arch.xsave_area ) > - { > v->arch.fpu_ctxt = &v->arch.xsave_area->fpu_sse; > - if ( cpu_has_xsaves || cpu_has_xsavec ) > - v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_COMPACTION_ENABLED; > - } > else > { > v->arch.fpu_ctxt = _xzalloc(sizeof(v->arch.xsave_area->fpu_sse), 16); > > > > x86/xsave: simplify xcomp_bv initialization > > This eliminates a number of pointless conditionals: Bits 0 and 1 of > xcomp_bv don't matter anyway, and as long as none of bits 2..62 are > set, setting bit 63 is pointless too. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > Noticed while working on XSA-165. > > --- a/xen/arch/x86/domain.c > +++ b/xen/arch/x86/domain.c > @@ -912,9 +912,7 @@ int arch_set_info_guest( > if ( v->arch.xsave_area ) > { > v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE; > - if ( cpu_has_xsaves || cpu_has_xsavec ) > - v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE | > - XSTATE_COMPACTION_ENABLED; > + v->arch.xsave_area->xsave_hdr.xcomp_bv = 0; > } > } > else if ( v->arch.xsave_area ) > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2092,9 +2092,7 @@ static int hvm_load_cpu_ctxt(struct doma > > memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs)); > xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE; > - if ( cpu_has_xsaves || cpu_has_xsavec ) > - xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE | > - XSTATE_COMPACTION_ENABLED; > + xsave_area->xsave_hdr.xcomp_bv = 0; > } > else > memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs)); > @@ -5562,9 +5560,7 @@ void hvm_vcpu_reset_state(struct vcpu *v > if ( v->arch.xsave_area ) > { > v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP; > - if ( cpu_has_xsaves || cpu_has_xsavec ) > - v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP | > - XSTATE_COMPACTION_ENABLED; > + v->arch.xsave_area->xsave_hdr.xcomp_bv = 0; > } > > v->arch.vgc_flags = VGCF_online; > --- a/xen/arch/x86/i387.c > +++ b/xen/arch/x86/i387.c > @@ -270,11 +270,7 @@ int vcpu_init_fpu(struct vcpu *v) > return rc; > > if ( v->arch.xsave_area ) > - { > v->arch.fpu_ctxt = &v->arch.xsave_area->fpu_sse; > - if ( cpu_has_xsaves || cpu_has_xsavec ) > - v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_COMPACTION_ENABLED; > - } > else > { > v->arch.fpu_ctxt = _xzalloc(sizeof(v->arch.xsave_area->fpu_sse), 16);
>>> On 18.12.15 at 11:53, <shuai.ruan@linux.intel.com> wrote: > On Thu, Dec 17, 2015 at 09:58:38AM -0700, Jan Beulich wrote: >> This eliminates a number of pointless conditionals: Bits 0 and 1 of >> xcomp_bv don't matter anyway, and as long as none of bits 2..62 are >> set, setting bit 63 is pointless too. >> > We should set bit 63 of the xcomp_bv when initialization. > From the initialization log of xen, I got that xrstors will > excute first(before xsaves). xrstors will check bit 63 of xcopm_bv. > If it is not set, on the xsaves-support machine xen will hung. > (I test the patch on skylake machine base on the master branch) Oh, indeed (assuming you mean crash instead of hang) - the insn page says "#GP(0) ... If bit 63 of the XCOMP_BV field of the XSAVE header is 0." I had only looked at the XRSTOR insn page. Still I think we shouldn't drop the patch entirely, it just shouldn't go as far as it does. Jan
--- a/xen/arch/x86/domain.c +++ b/xen/arch/x86/domain.c @@ -912,9 +912,7 @@ int arch_set_info_guest( if ( v->arch.xsave_area ) { v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE; - if ( cpu_has_xsaves || cpu_has_xsavec ) - v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE | - XSTATE_COMPACTION_ENABLED; + v->arch.xsave_area->xsave_hdr.xcomp_bv = 0; } } else if ( v->arch.xsave_area ) --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2092,9 +2092,7 @@ static int hvm_load_cpu_ctxt(struct doma memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs)); xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE; - if ( cpu_has_xsaves || cpu_has_xsavec ) - xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE | - XSTATE_COMPACTION_ENABLED; + xsave_area->xsave_hdr.xcomp_bv = 0; } else memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs)); @@ -5562,9 +5560,7 @@ void hvm_vcpu_reset_state(struct vcpu *v if ( v->arch.xsave_area ) { v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP; - if ( cpu_has_xsaves || cpu_has_xsavec ) - v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP | - XSTATE_COMPACTION_ENABLED; + v->arch.xsave_area->xsave_hdr.xcomp_bv = 0; } v->arch.vgc_flags = VGCF_online; --- a/xen/arch/x86/i387.c +++ b/xen/arch/x86/i387.c @@ -270,11 +270,7 @@ int vcpu_init_fpu(struct vcpu *v) return rc; if ( v->arch.xsave_area ) - { v->arch.fpu_ctxt = &v->arch.xsave_area->fpu_sse; - if ( cpu_has_xsaves || cpu_has_xsavec ) - v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_COMPACTION_ENABLED; - } else { v->arch.fpu_ctxt = _xzalloc(sizeof(v->arch.xsave_area->fpu_sse), 16);