xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [V4] x86/xsaves: calculate the xstate_comp_offsets base on xstate_bv
@ 2016-03-16 12:12 Shuai Ruan
  2016-03-22 10:53 ` Jan Beulich
  0 siblings, 1 reply; 4+ messages in thread
From: Shuai Ruan @ 2016-03-16 12:12 UTC (permalink / raw)
  To: xen-devel; +Cc: andrew.cooper3, keir, jbeulich

Previous patch using all available features calculate xstate_comp_offsets.
This is wrong.This patch fix this bug by calculating the xstate_comp_offset
based on xstate_bv of current guest.
Also, the xstate_comp_offset should take alignment into consideration.

Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
Reported-by: Jan Beulich <jbeulich@suse.com>
---
V4: Address comments form jan:
1. use xstate_comp_offsets as on-stack array.
 
V3: Address comments from Jan:
1. fix xstate_comp_offsets used as static array problem.
2. change xstate_align from array to u64 and used as bitmap.
3. change calculating xstate_comp_offsets into three step.
        1) whether component is set in xsavearea
        2) whether component need align
        3) add xstate_size[i-1]

V2: Address comments from Jan:
1. code style fix.
2. setup_xstate_comp take xcomp_bv as param.

 xen/arch/x86/xstate.c        | 53 ++++++++++++++++++++++++++++----------------
 xen/include/asm-x86/xstate.h |  2 ++
 2 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index a5ed9e5..ef2c54d 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -26,8 +26,8 @@ u64 __read_mostly xfeature_mask;
 
 static unsigned int *__read_mostly xstate_offsets;
 unsigned int *__read_mostly xstate_sizes;
+static u64 __read_mostly xstate_align;
 static unsigned int __read_mostly xstate_features;
-static unsigned int __read_mostly xstate_comp_offsets[sizeof(xfeature_mask)*8];
 
 static uint32_t __read_mostly mxcsr_mask = 0x0000ffbf;
 
@@ -94,7 +94,7 @@ static bool_t xsave_area_compressed(const struct xsave_struct *xsave_area)
 
 static int setup_xstate_features(bool_t bsp)
 {
-    unsigned int leaf, tmp, eax, ebx;
+    unsigned int leaf, eax, ebx, ecx, edx;
 
     if ( bsp )
     {
@@ -111,57 +111,70 @@ static int setup_xstate_features(bool_t bsp)
     for ( leaf = 2; leaf < xstate_features; leaf++ )
     {
         if ( bsp )
+        {
             cpuid_count(XSTATE_CPUID, leaf, &xstate_sizes[leaf],
-                        &xstate_offsets[leaf], &tmp, &tmp);
+                        &xstate_offsets[leaf], &ecx, &edx);
+            if ( ecx & XSTATE_ALIGN64 )
+                __set_bit(leaf, &xstate_align);
+        }
         else
         {
             cpuid_count(XSTATE_CPUID, leaf, &eax,
-                        &ebx, &tmp, &tmp);
+                        &ebx, &ecx, &edx);
             BUG_ON(eax != xstate_sizes[leaf]);
             BUG_ON(ebx != xstate_offsets[leaf]);
+            BUG_ON((ecx & XSTATE_ALIGN64) != test_bit(leaf, &xstate_align));
         }
     }
 
     return 0;
 }
 
-static void __init setup_xstate_comp(void)
+static void setup_xstate_comp(uint16_t *xstate_comp_offsets,
+                              const u64 xstate_bv)
 {
     unsigned int i;
+    uint16_t offset;
 
     /*
      * The FP xstates and SSE xstates are legacy states. They are always
      * in the fixed offsets in the xsave area in either compacted form
      * or standard form.
      */
-    xstate_comp_offsets[0] = 0;
     xstate_comp_offsets[1] = XSAVE_SSE_OFFSET;
 
     xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
 
-    for ( i = 3; i < xstate_features; i++ )
+    offset = xstate_comp_offsets[2];
+    for ( i = 2; i < xstate_features; i++ )
     {
-        xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] +
-                                 (((1ul << i) & xfeature_mask)
-                                  ? xstate_sizes[i - 1] : 0);
-        ASSERT(xstate_comp_offsets[i] + xstate_sizes[i] <= xsave_cntxt_size);
+        if ( (1ul << i) & xstate_bv )
+        {
+            if ( test_bit(i, &xstate_align) )
+                offset = ROUNDUP(offset, 64);
+            xstate_comp_offsets[i] = offset;
+            offset += xstate_sizes[i];
+            ASSERT(offset <= xsave_cntxt_size);
+        }
     }
 }
 
 static void *get_xsave_addr(struct xsave_struct *xsave,
-        unsigned int xfeature_idx)
+                            const uint16_t *xstate_comp_offsets,
+                            unsigned int xfeature_idx)
 {
     if ( !((1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv) )
         return NULL;
 
-    return (void *)xsave + (xsave_area_compressed(xsave)
-            ? xstate_comp_offsets
-            : xstate_offsets)[xfeature_idx];
+    return (void *)xsave + ( xsave_area_compressed(xsave) ?
+                             xstate_comp_offsets[xfeature_idx] :
+                             xstate_offsets[xfeature_idx] );
 }
 
 void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
 {
     struct xsave_struct *xsave = v->arch.xsave_area;
+    uint16_t xstate_comp_offsets[sizeof(xfeature_mask)*8];
     u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
     u64 valid;
 
@@ -172,6 +185,8 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
     }
 
     ASSERT(xsave_area_compressed(xsave));
+    setup_xstate_comp(xstate_comp_offsets, xstate_bv);
+
     /*
      * Copy legacy XSAVE area and XSAVE hdr area.
      */
@@ -188,7 +203,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
     {
         u64 feature = valid & -valid;
         unsigned int index = fls(feature) - 1;
-        const void *src = get_xsave_addr(xsave, index);
+        const void *src = get_xsave_addr(xsave, xstate_comp_offsets, index);
 
         if ( src )
         {
@@ -203,6 +218,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
 void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
 {
     struct xsave_struct *xsave = v->arch.xsave_area;
+    uint16_t xstate_comp_offsets[sizeof(xfeature_mask)*8];
     u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
     u64 valid;
 
@@ -222,6 +238,7 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
     /* Set XSTATE_BV and XCOMP_BV.  */
     xsave->xsave_hdr.xstate_bv = xstate_bv;
     xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
+    setup_xstate_comp(xstate_comp_offsets, xstate_bv);
 
     /*
      * Copy each region from the non-compacted offset to the
@@ -232,7 +249,7 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
     {
         u64 feature = valid & -valid;
         unsigned int index = fls(feature) - 1;
-        void *dest = get_xsave_addr(xsave, index);
+        void *dest = get_xsave_addr(xsave, xstate_comp_offsets, index);
 
         if ( dest )
         {
@@ -575,8 +592,6 @@ void xstate_init(struct cpuinfo_x86 *c)
 
     if ( setup_xstate_features(bsp) && bsp )
         BUG();
-    if ( bsp && (cpu_has_xsaves || cpu_has_xsavec) )
-        setup_xstate_comp();
 }
 
 static bool_t valid_xcr0(u64 xcr0)
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index c28cea5..a488688 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -46,6 +46,8 @@
 #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
 #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
 
+#define XSTATE_ALIGN64 (1U << 1)
+
 extern u64 xfeature_mask;
 extern unsigned int *xstate_sizes;
 
-- 
1.9.1


_______________________________________________
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: [V4] x86/xsaves: calculate the xstate_comp_offsets base on xstate_bv
  2016-03-16 12:12 [V4] x86/xsaves: calculate the xstate_comp_offsets base on xstate_bv Shuai Ruan
@ 2016-03-22 10:53 ` Jan Beulich
  2016-03-23  1:28   ` Shuai Ruan
       [not found]   ` <20160323012803.GA4131@shuai.ruan@linux.intel.com>
  0 siblings, 2 replies; 4+ messages in thread
From: Jan Beulich @ 2016-03-22 10:53 UTC (permalink / raw)
  To: Shuai Ruan; +Cc: andrew.cooper3, keir, xen-devel

>>> On 16.03.16 at 13:12, <shuai.ruan@linux.intel.com> wrote:

Please have patch subjects have [PATCH at their beginning.

> @@ -111,57 +111,70 @@ static int setup_xstate_features(bool_t bsp)
>      for ( leaf = 2; leaf < xstate_features; leaf++ )
>      {
>          if ( bsp )
> +        {
>              cpuid_count(XSTATE_CPUID, leaf, &xstate_sizes[leaf],
> -                        &xstate_offsets[leaf], &tmp, &tmp);
> +                        &xstate_offsets[leaf], &ecx, &edx);
> +            if ( ecx & XSTATE_ALIGN64 )
> +                __set_bit(leaf, &xstate_align);
> +        }
>          else
>          {
>              cpuid_count(XSTATE_CPUID, leaf, &eax,
> -                        &ebx, &tmp, &tmp);
> +                        &ebx, &ecx, &edx);
>              BUG_ON(eax != xstate_sizes[leaf]);
>              BUG_ON(ebx != xstate_offsets[leaf]);
> +            BUG_ON((ecx & XSTATE_ALIGN64) != test_bit(leaf, &xstate_align));

Neither side of the != seems correct: The left side would produce
0 or 2 (instead of 0 or 1), while the right side may produce any
non-zero value for truth.

> -static void __init setup_xstate_comp(void)
> +static void setup_xstate_comp(uint16_t *xstate_comp_offsets,
> +                              const u64 xstate_bv)
>  {
>      unsigned int i;
> +    uint16_t offset;
>  
>      /*
>       * The FP xstates and SSE xstates are legacy states. They are always
>       * in the fixed offsets in the xsave area in either compacted form
>       * or standard form.
>       */
> -    xstate_comp_offsets[0] = 0;

With the array now being uninitialized again you should no
longer delete this.

>      xstate_comp_offsets[1] = XSAVE_SSE_OFFSET;
>  
>      xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
>  
> -    for ( i = 3; i < xstate_features; i++ )
> +    offset = xstate_comp_offsets[2];
> +    for ( i = 2; i < xstate_features; i++ )
>      {
> -        xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] +
> -                                 (((1ul << i) & xfeature_mask)
> -                                  ? xstate_sizes[i - 1] : 0);
> -        ASSERT(xstate_comp_offsets[i] + xstate_sizes[i] <= xsave_cntxt_size);
> +        if ( (1ul << i) & xstate_bv )
> +        {
> +            if ( test_bit(i, &xstate_align) )
> +                offset = ROUNDUP(offset, 64);
> +            xstate_comp_offsets[i] = offset;
> +            offset += xstate_sizes[i];
> +            ASSERT(offset <= xsave_cntxt_size);

This would seem to better go after the loop now that it's
independent of the loop variable. Also at least for this purpose
I think it would be better is "offset" was "unsigned int".

>  static void *get_xsave_addr(struct xsave_struct *xsave,
> -        unsigned int xfeature_idx)
> +                            const uint16_t *xstate_comp_offsets,
> +                            unsigned int xfeature_idx)
>  {
>      if ( !((1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv) )
>          return NULL;
>  
> -    return (void *)xsave + (xsave_area_compressed(xsave)
> -            ? xstate_comp_offsets
> -            : xstate_offsets)[xfeature_idx];
> +    return (void *)xsave + ( xsave_area_compressed(xsave) ?
> +                             xstate_comp_offsets[xfeature_idx] :
> +                             xstate_offsets[xfeature_idx] );

Stray blanks inside the parentheses.

>  void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>  {
>      struct xsave_struct *xsave = v->arch.xsave_area;
> +    uint16_t xstate_comp_offsets[sizeof(xfeature_mask)*8];

There's no point in prefixing a local variable in this file with xstate_.
And the same goes for the function parameters earlier on.

> @@ -172,6 +185,8 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>      }
>  
>      ASSERT(xsave_area_compressed(xsave));
> +    setup_xstate_comp(xstate_comp_offsets, xstate_bv);

Don't you need to use xcomp_bv here? That's what "Extended
Region of an XSAVE Area" in SDM Vol 1 suggests to me.

> @@ -222,6 +238,7 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
>      /* Set XSTATE_BV and XCOMP_BV.  */
>      xsave->xsave_hdr.xstate_bv = xstate_bv;
>      xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
> +    setup_xstate_comp(xstate_comp_offsets, xstate_bv);

Same here then I think.

Jan

_______________________________________________
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: [V4] x86/xsaves: calculate the xstate_comp_offsets base on xstate_bv
  2016-03-22 10:53 ` Jan Beulich
@ 2016-03-23  1:28   ` Shuai Ruan
       [not found]   ` <20160323012803.GA4131@shuai.ruan@linux.intel.com>
  1 sibling, 0 replies; 4+ messages in thread
From: Shuai Ruan @ 2016-03-23  1:28 UTC (permalink / raw)
  To: Jan Beulich; +Cc: andrew.cooper3, keir, xen-devel

On Tue, Mar 22, 2016 at 04:53:02AM -0600, Jan Beulich wrote:
> >>> On 16.03.16 at 13:12, <shuai.ruan@linux.intel.com> wrote:
> 
> Don't you need to use xcomp_bv here? That's what "Extended
> Region of an XSAVE Area" in SDM Vol 1 suggests to me.
> 
"OPERATION OF XRSTORS" and "OPERATION OF XSAVES" in SDM Vol1.

For xsaves:
"Execution of XSAVES performs the init optimization to reduce the amount
of data written to memory."

For xrstors:
"XRSTORS updates state component i based on the value of bit i in the
XSTATE_BV field of the XSAVE header".

> >      /* Set XSTATE_BV and XCOMP_BV.  */
> >      xsave->xsave_hdr.xstate_bv = xstate_bv;
> >      xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
> > +    setup_xstate_comp(xstate_comp_offsets, xstate_bv);
> 
> Same here then I think.
> 
> Jan
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
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: [V4] x86/xsaves: calculate the xstate_comp_offsets base on xstate_bv
       [not found]   ` <20160323012803.GA4131@shuai.ruan@linux.intel.com>
@ 2016-03-23  3:28     ` Shuai Ruan
  0 siblings, 0 replies; 4+ messages in thread
From: Shuai Ruan @ 2016-03-23  3:28 UTC (permalink / raw)
  To: Jan Beulich; +Cc: andrew.cooper3, keir, xen-devel

On Wed, Mar 23, 2016 at 09:28:03AM +0800, Shuai Ruan wrote:
> On Tue, Mar 22, 2016 at 04:53:02AM -0600, Jan Beulich wrote:
> > >>> On 16.03.16 at 13:12, <shuai.ruan@linux.intel.com> wrote:
> > 
> > Don't you need to use xcomp_bv here? That's what "Extended
> > Region of an XSAVE Area" in SDM Vol 1 suggests to me.
> > 
> "OPERATION OF XRSTORS" and "OPERATION OF XSAVES" in SDM Vol1.
> 
> For xsaves:
> "Execution of XSAVES performs the init optimization to reduce the amount
> of data written to memory."
> 
> For xrstors:
> "XRSTORS updates state component i based on the value of bit i in the
> XSTATE_BV field of the XSAVE header".
> 
Ignore this. You are right.
I misunderstand some XSAVES/XRSTORS behaviors in SDM . In the previous versions,
I use xcom_bv to caculate comp_offset (but changing to xstate_bv in this
version:( ).
Thanks.
> > >      /* Set XSTATE_BV and XCOMP_BV.  */
> > >      xsave->xsave_hdr.xstate_bv = xstate_bv;
> > >      xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
> > > +    setup_xstate_comp(xstate_comp_offsets, xstate_bv);
> > 
> > Same here then I think.
> > 
> > Jan
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-03-23  3:28 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-16 12:12 [V4] x86/xsaves: calculate the xstate_comp_offsets base on xstate_bv Shuai Ruan
2016-03-22 10:53 ` Jan Beulich
2016-03-23  1:28   ` Shuai Ruan
     [not found]   ` <20160323012803.GA4131@shuai.ruan@linux.intel.com>
2016-03-23  3:28     ` Shuai Ruan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).