* [PATCH] x86/xsave: simplify xcomp_bv initialization
@ 2015-12-17 16:58 Jan Beulich
2015-12-17 17:46 ` Andrew Cooper
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Jan Beulich @ 2015-12-17 16:58 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser, Shuai Ruan
[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]
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);
[-- Attachment #2: x86-xcomp_bv-simplify.patch --]
[-- Type: text/plain, Size: 2399 bytes --]
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);
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86/xsave: simplify xcomp_bv initialization
2015-12-17 16:58 [PATCH] x86/xsave: simplify xcomp_bv initialization Jan Beulich
@ 2015-12-17 17:46 ` Andrew Cooper
2015-12-18 10:53 ` Shuai Ruan
[not found] ` <20151218105339.GA24875@shuai.ruan@linux.intel.com>
2 siblings, 0 replies; 4+ messages in thread
From: Andrew Cooper @ 2015-12-17 17:46 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser, Shuai Ruan
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.)
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86/xsave: simplify xcomp_bv initialization
2015-12-17 16:58 [PATCH] x86/xsave: simplify xcomp_bv initialization Jan Beulich
2015-12-17 17:46 ` Andrew Cooper
@ 2015-12-18 10:53 ` Shuai Ruan
[not found] ` <20151218105339.GA24875@shuai.ruan@linux.intel.com>
2 siblings, 0 replies; 4+ messages in thread
From: Shuai Ruan @ 2015-12-18 10:53 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, Keir Fraser, Andrew Cooper
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);
^ permalink raw reply [flat|nested] 4+ messages in thread[parent not found: <20151218105339.GA24875@shuai.ruan@linux.intel.com>]
* Re: [PATCH] x86/xsave: simplify xcomp_bv initialization
[not found] ` <20151218105339.GA24875@shuai.ruan@linux.intel.com>
@ 2015-12-18 11:09 ` Jan Beulich
0 siblings, 0 replies; 4+ messages in thread
From: Jan Beulich @ 2015-12-18 11:09 UTC (permalink / raw)
To: Shuai Ruan; +Cc: Andrew Cooper, Keir Fraser, xen-devel
>>> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-12-18 11:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-17 16:58 [PATCH] x86/xsave: simplify xcomp_bv initialization Jan Beulich
2015-12-17 17:46 ` Andrew Cooper
2015-12-18 10:53 ` Shuai Ruan
[not found] ` <20151218105339.GA24875@shuai.ruan@linux.intel.com>
2015-12-18 11:09 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.