[PATCH 0/3] x86/xsaves: bugs fix

[PATCH 0/3] x86/xsaves: bugs fix

[Shuai Ruan]

This patch fix bugs caused by previous patch:
1. xstate_comp_offset should be caculated based on xcomp_bv
2. non-lazy/lazy xsave[sc] may overwiting xsave area
3. ebx may return wrong value using CPUID eax=0xdh,ecx =1

The fist and second bugs is found by Jan. Thanks to Jan.

Shuai Ruan (3):
  x86/xsaves: caculate the xstate_comp_offsets base on xcomp_bv
  x86/xsaves: fix overwriting between non-lazy/lazy xsave[sc]
  x86/xsaves: ebx may return wrong value using CPUID eax=0xdh,ecx =1

 tools/libxc/xc_cpuid_x86.c   | 10 ++++++++--
 xen/arch/x86/hvm/hvm.c       | 12 ++++++++++++
 xen/arch/x86/i387.c          |  2 +-
 xen/arch/x86/xstate.c        | 29 +++++++++++++++++++++--------
 xen/include/asm-x86/xstate.h |  1 +
 5 files changed, 43 insertions(+), 11 deletions(-)

-- 
1.9.1

[PATCH 1/3] x86/xsaves: caculate the xstate_comp_offsets base on xcomp_bv

[Shuai Ruan]

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

Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
---
 xen/arch/x86/xstate.c        | 29 +++++++++++++++++++++--------
 xen/include/asm-x86/xstate.h |  1 +
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 4f2fb8e..0e7643b 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -26,6 +26,7 @@ u64 __read_mostly xfeature_mask;
 
 static unsigned int *__read_mostly xstate_offsets;
 unsigned int *__read_mostly xstate_sizes;
+static unsigned int *__read_mostly xstate_align;
 static unsigned int __read_mostly xstate_features;
 static unsigned int __read_mostly xstate_comp_offsets[sizeof(xfeature_mask)*8];
 
@@ -94,7 +95,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 )
     {
@@ -106,34 +107,44 @@ static int setup_xstate_features(bool_t bsp)
         xstate_sizes = xzalloc_array(unsigned int, xstate_features);
         if ( !xstate_sizes )
             return -ENOMEM;
+
+        xstate_align = xzalloc_array(unsigned int, xstate_features);
+        if ( !xstate_align )
+            return -ENOMEM;
     }
 
     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);
+            xstate_align[leaf] = ecx & XSTATE_ALIGN64;
+	}
         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) != xstate_align[leaf]);
         }
     }
 
     return 0;
 }
 
-static void __init setup_xstate_comp(void)
+static void setup_xstate_comp(const struct xsave_struct *xsave)
 {
     unsigned int i;
+    u64 xcomp_bv =  xsave->xsave_hdr.xcomp_bv;
 
     /*
      * 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.
      */
+    memset(xstate_comp_offsets, 0, sizeof(xstate_comp_offsets));
     xstate_comp_offsets[0] = 0;
     xstate_comp_offsets[1] = XSAVE_SSE_OFFSET;
 
@@ -141,8 +152,10 @@ static void __init setup_xstate_comp(void)
 
     for ( i = 3; i < xstate_features; i++ )
     {
-        xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] +
-                                 (((1ul << i) & xfeature_mask)
+        xstate_comp_offsets[i] = (xstate_align[i] ?
+                                  ROUNDUP(xstate_comp_offsets[i-1], 64) :
+                                  xstate_comp_offsets[i - 1]) +
+                                 (((1ul << i) & xcomp_bv)
                                   ? xstate_sizes[i - 1] : 0);
         ASSERT(xstate_comp_offsets[i] + xstate_sizes[i] <= xsave_cntxt_size);
     }
@@ -172,6 +185,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
     }
 
     ASSERT(xsave_area_compressed(xsave));
+    setup_xstate_comp(xsave);
     /*
      * Copy legacy XSAVE area and XSAVE hdr area.
      */
@@ -223,6 +237,7 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
     xsave->xsave_hdr.xstate_bv = xstate_bv;
     xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
 
+    setup_xstate_comp(xsave);
     /*
      * Copy each region from the non-compacted offset to the
      * possibly compacted offset.
@@ -568,8 +583,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 84f0af9..0215070 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -44,6 +44,7 @@
 #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
 #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
 
+#define XSTATE_ALIGN64 (1ULL << 1)
 extern u64 xfeature_mask;
 extern unsigned int *xstate_sizes;
 
-- 
1.9.1

Re: [PATCH 1/3] x86/xsaves: caculate the xstate_comp_offsets base on xcomp_bv

[Jan Beulich]

>>> On 22.02.16 at 06:35, <shuai.ruan@linux.intel.com> wrote:
> Previous patch using all available features caculate xstate_comp_offsets.
> This is wrong.This patch fix this bug by caculating the xstate_comp_offset

In the title and above: calculate (and alike).

> based on xcomp_bv of current guest.
> Also, the xstate_comp_offset should take alignment into consideration.

Reported-by: Jan Beulich <jbeulich@suse.com>

> Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
[...]
> @@ -106,34 +107,44 @@ static int setup_xstate_features(bool_t bsp)
>          xstate_sizes = xzalloc_array(unsigned int, xstate_features);
>          if ( !xstate_sizes )
>              return -ENOMEM;
> +
> +        xstate_align = xzalloc_array(unsigned int, xstate_features);
> +        if ( !xstate_align )
> +            return -ENOMEM;
>      }
>  
>      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);
> +            xstate_align[leaf] = ecx & XSTATE_ALIGN64;
> +	}

Bogus hard tabs.

> -static void __init setup_xstate_comp(void)
> +static void setup_xstate_comp(const struct xsave_struct *xsave)
>  {
>      unsigned int i;
> +    u64 xcomp_bv =  xsave->xsave_hdr.xcomp_bv;

It looks like it would suffice if the caller passed xcomp_bv into
this function.

>      /*
>       * 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.
>       */
> +    memset(xstate_comp_offsets, 0, sizeof(xstate_comp_offsets));
>      xstate_comp_offsets[0] = 0;

The addition makes this line redundant. However, you act on
static data here, and I don't think there's any serialization
between the various callers.

> @@ -141,8 +152,10 @@ static void __init setup_xstate_comp(void)
>  
>      for ( i = 3; i < xstate_features; i++ )
>      {
> -        xstate_comp_offsets[i] = xstate_comp_offsets[i - 1] +
> -                                 (((1ul << i) & xfeature_mask)
> +        xstate_comp_offsets[i] = (xstate_align[i] ?
> +                                  ROUNDUP(xstate_comp_offsets[i-1], 64) :

Coding style.

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

Please add a blank line before the comment.

> @@ -223,6 +237,7 @@ void compress_xsave_states(struct vcpu *v, const void 
> *src, unsigned int size)
>      xsave->xsave_hdr.xstate_bv = xstate_bv;
>      xsave->xsave_hdr.xcomp_bv = v->arch.xcr0_accum | XSTATE_COMPACTION_ENABLED;
>  
> +    setup_xstate_comp(xsave);
>      /*

Please retain a blank line before the comment.

> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -44,6 +44,7 @@
>  #define XSTATE_LAZY    (XSTATE_ALL & ~XSTATE_NONLAZY)
>  #define XSTATE_COMPACTION_ENABLED  (1ULL << 63)
>  
> +#define XSTATE_ALIGN64 (1ULL << 1)
>  extern u64 xfeature_mask;

Missing blank line again.

Jan

[PATCH 2/3] x86/xsaves: fix overwriting between non-lazy/lazy xsave[sc]

[Shuai Ruan]

The offset at which components xsaved by xsave[sc] are not fixed.
So when when a save with v->fpu_dirtied set is followed by one
with v->fpu_dirtied clear, non-lazy xsave[sc] may overwriting data
written by the lazy one.

When xsave[sc] is enable, vcpu_xsave_mask will return XSTATE_ALL when
v->fpu_dirtied clear and v->arch.nonlazy_xstate_used is set.

Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
---
 xen/arch/x86/i387.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 67016c9..e3a7bc0 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -118,7 +118,7 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
     if ( v->fpu_dirtied )
         return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
 
-    return v->arch.nonlazy_xstate_used ? XSTATE_NONLAZY : 0;
+    return ( cpu_has_xsaves || cpu_has_xsavec ) ? XSTATE_ALL : XSTATE_NONLAZY;
 }
 
 /* Save x87 extended state */
-- 
1.9.1

Re: [PATCH 2/3] x86/xsaves: fix overwriting between non-lazy/lazy xsave[sc]

[Jan Beulich]

>>> On 22.02.16 at 06:35, <shuai.ruan@linux.intel.com> wrote:

First of all I wonder on what basis you collect your Cc lists on
patches.

> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -118,7 +118,7 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
>      if ( v->fpu_dirtied )
>          return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
>  
> -    return v->arch.nonlazy_xstate_used ? XSTATE_NONLAZY : 0;
> +    return ( cpu_has_xsaves || cpu_has_xsavec ) ? XSTATE_ALL : XSTATE_NONLAZY;
>  }

The description lacks any mention of the performance impact,
and what investigation was done to find ways to perhaps
overcome this. For example, regardless of cpu_has_xsaves,
do we really always need to _use_ XSAVES?

Also - coding style (stray spaces inside parentheses).

Jan

Re: [PATCH 2/3] x86/xsaves: fix overwriting between non-lazy/lazy xsave[sc]

[Shuai Ruan]

On Mon, Feb 22, 2016 at 10:08:52AM -0700, Jan Beulich wrote:
> >>> On 22.02.16 at 06:35, <shuai.ruan@linux.intel.com> wrote:
> 
> First of all I wonder on what basis you collect your Cc lists on
> patches.
> 
I send the bugs-fix patch as whole. I just get the Cc lists using the
script based on the whole patchset. May be I will send the patch
seperately.
> > --- a/xen/arch/x86/i387.c
> > +++ b/xen/arch/x86/i387.c
> > @@ -118,7 +118,7 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
> >      if ( v->fpu_dirtied )
> >          return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
> >  
> > -    return v->arch.nonlazy_xstate_used ? XSTATE_NONLAZY : 0;
> > +    return ( cpu_has_xsaves || cpu_has_xsavec ) ? XSTATE_ALL : XSTATE_NONLAZY;
> >  }
> 
> The description lacks any mention of the performance impact,
> and what investigation was done to find ways to perhaps
> overcome this. For example, regardless of cpu_has_xsaves,
> do we really always need to _use_ XSAVES?
> 
Currently no supervisor xstates is enabled in xen or even in
guest os. Using xsaves is a little ahead (xsavec may used).  
xsavec may also cause overwriting problem like xsaves.
I will add performance impact in the description. 
I am still thinking of a better way to overcome the overhead xsave
(But have not get a better solution yet).

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

Re: [PATCH 2/3] x86/xsaves: fix overwriting between non-lazy/lazy xsave[sc]

[Jan Beulich]

>>> On 24.02.16 at 08:05, <shuai.ruan@linux.intel.com> wrote:
> On Mon, Feb 22, 2016 at 10:08:52AM -0700, Jan Beulich wrote:
>> >>> On 22.02.16 at 06:35, <shuai.ruan@linux.intel.com> wrote:
>> 
>> First of all I wonder on what basis you collect your Cc lists on
>> patches.
>> 
> I send the bugs-fix patch as whole. I just get the Cc lists using the
> script based on the whole patchset. May be I will send the patch
> seperately.

Thank you. Please also see
http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches
and in particular also Konrad's recent suggested adjustments
http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg02877.html
(perhaps including the other parts of the thread).

>> > --- a/xen/arch/x86/i387.c
>> > +++ b/xen/arch/x86/i387.c
>> > @@ -118,7 +118,7 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
>> >      if ( v->fpu_dirtied )
>> >          return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
>> >  
>> > -    return v->arch.nonlazy_xstate_used ? XSTATE_NONLAZY : 0;
>> > +    return ( cpu_has_xsaves || cpu_has_xsavec ) ? XSTATE_ALL : XSTATE_NONLAZY;
>> >  }
>> 
>> The description lacks any mention of the performance impact,
>> and what investigation was done to find ways to perhaps
>> overcome this. For example, regardless of cpu_has_xsaves,
>> do we really always need to _use_ XSAVES?
>> 
> Currently no supervisor xstates is enabled in xen or even in
> guest os. Using xsaves is a little ahead (xsavec may used).  
> xsavec may also cause overwriting problem like xsaves.
> I will add performance impact in the description. 
> I am still thinking of a better way to overcome the overhead xsave
> (But have not get a better solution yet).

I was thinking that taking into consideration the features a guest
has ever used (i.e. v->arch.xcr0_accum) to decide which variant
to use may be a worthwhile avenue to explore.

Jan

Re: [PATCH 2/3] x86/xsaves: fix overwriting between non-lazy/lazy xsave[sc]

[Shuai Ruan]

On Wed, Feb 24, 2016 at 02:16:38AM -0700, Jan Beulich wrote:
> > I send the bugs-fix patch as whole. I just get the Cc lists using the
> > script based on the whole patchset. May be I will send the patch
> > seperately.
> 
> Thank you. Please also see
> http://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches
> and in particular also Konrad's recent suggested adjustments
> http://lists.xenproject.org/archives/html/xen-devel/2016-02/msg02877.html
> (perhaps including the other parts of the thread).
Thanks.
> 
> >> > --- a/xen/arch/x86/i387.c
> >> > +++ b/xen/arch/x86/i387.c
> >> > @@ -118,7 +118,7 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
> >> >      if ( v->fpu_dirtied )
> >> >          return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
> >> >  
> >> > -    return v->arch.nonlazy_xstate_used ? XSTATE_NONLAZY : 0;
> >> > +    return ( cpu_has_xsaves || cpu_has_xsavec ) ? XSTATE_ALL : XSTATE_NONLAZY;
> >> >  }
> >> 
> >> The description lacks any mention of the performance impact,
> >> and what investigation was done to find ways to perhaps
> >> overcome this. For example, regardless of cpu_has_xsaves,
> >> do we really always need to _use_ XSAVES?
> >> 
> > Currently no supervisor xstates is enabled in xen or even in
> > guest os. Using xsaves is a little ahead (xsavec may used).  
> > xsavec may also cause overwriting problem like xsaves.
> > I will add performance impact in the description. 
> > I am still thinking of a better way to overcome the overhead xsave
> > (But have not get a better solution yet).
> 
> I was thinking that taking into consideration the features a guest
> has ever used (i.e. v->arch.xcr0_accum) to decide which variant
> to use may be a worthwhile avenue to explore.
> 
Oh, Thanks for your suggestion.
You mean when (v->arch.xcr0_accum & (XSTATE_LAZY & ~XSTATE_FP_SSE)) return false,
we can return XSTATE_NONLAZY in vcpu_xsave_mask when using xsave[cs]
otherwise return XSTATE_ALL.
It means we can save the area safely, if the guest has ever use 
XSTATE_NONLAZY | XSTATE_FP_SSE only. 
> 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

Re: [PATCH 2/3] x86/xsaves: fix overwriting between non-lazy/lazy xsave[sc]

[Jan Beulich]

>>> On 26.02.16 at 08:41, <shuai.ruan@linux.intel.com> wrote:
> On Wed, Feb 24, 2016 at 02:16:38AM -0700, Jan Beulich wrote:
>> >> > --- a/xen/arch/x86/i387.c
>> >> > +++ b/xen/arch/x86/i387.c
>> >> > @@ -118,7 +118,7 @@ static inline uint64_t vcpu_xsave_mask(const struct vcpu *v)
>> >> >      if ( v->fpu_dirtied )
>> >> >          return v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY;
>> >> >  
>> >> > -    return v->arch.nonlazy_xstate_used ? XSTATE_NONLAZY : 0;
>> >> > +    return ( cpu_has_xsaves || cpu_has_xsavec ) ? XSTATE_ALL : XSTATE_NONLAZY;
>> >> >  }
>> >> 
>> >> The description lacks any mention of the performance impact,
>> >> and what investigation was done to find ways to perhaps
>> >> overcome this. For example, regardless of cpu_has_xsaves,
>> >> do we really always need to _use_ XSAVES?
>> >> 
>> > Currently no supervisor xstates is enabled in xen or even in
>> > guest os. Using xsaves is a little ahead (xsavec may used).  
>> > xsavec may also cause overwriting problem like xsaves.
>> > I will add performance impact in the description. 
>> > I am still thinking of a better way to overcome the overhead xsave
>> > (But have not get a better solution yet).
>> 
>> I was thinking that taking into consideration the features a guest
>> has ever used (i.e. v->arch.xcr0_accum) to decide which variant
>> to use may be a worthwhile avenue to explore.
>> 
> Oh, Thanks for your suggestion.
> You mean when (v->arch.xcr0_accum & (XSTATE_LAZY & ~XSTATE_FP_SSE)) return 
> false,
> we can return XSTATE_NONLAZY in vcpu_xsave_mask when using xsave[cs]
> otherwise return XSTATE_ALL.
> It means we can save the area safely, if the guest has ever use 
> XSTATE_NONLAZY | XSTATE_FP_SSE only. 

That's one step in the right direction. But the main difference
between XSAVE/XSAVEOPT and XSAVEC/XSAVES is that the former
can be used incrementally, while the latter can't. And I highly
doubt the modified optimization the latter two support will kick in
very often, since there's quite good a chance that the guest
itself executed another one of these between two of our instances.
Which to me means it should at least be investigated whether using
XSAVEOPT in favor of XSAVEC wouldn't produce better performance,
and whether XSAVES wouldn't better be used only if the guest uses
a component which can only be saved that way.

Jan


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

Re: [PATCH 2/3] x86/xsaves: fix overwriting between non-lazy/lazy xsave[sc]

[Shuai Ruan]

On Fri, Feb 26, 2016 at 01:42:35AM -0700, Jan Beulich wrote:
> >>> On 26.02.16 at 08:41, <shuai.ruan@linux.intel.com> wrote:
> > On Wed, Feb 24, 2016 at 02:16:38AM -0700, Jan Beulich wrote:
> >> >> The description lacks any mention of the performance impact,
> >> >> and what investigation was done to find ways to perhaps
> >> >> overcome this. For example, regardless of cpu_has_xsaves,
> >> >> do we really always need to _use_ XSAVES?
> >> >> 
> >> > Currently no supervisor xstates is enabled in xen or even in
> >> > guest os. Using xsaves is a little ahead (xsavec may used).  
> >> > xsavec may also cause overwriting problem like xsaves.
> >> > I will add performance impact in the description. 
> >> > I am still thinking of a better way to overcome the overhead xsave
> >> > (But have not get a better solution yet).
> >> 
> >> I was thinking that taking into consideration the features a guest
> >> has ever used (i.e. v->arch.xcr0_accum) to decide which variant
> >> to use may be a worthwhile avenue to explore.
> >> 
> > Oh, Thanks for your suggestion.
> > You mean when (v->arch.xcr0_accum & (XSTATE_LAZY & ~XSTATE_FP_SSE)) return 
> > false,
> > we can return XSTATE_NONLAZY in vcpu_xsave_mask when using xsave[cs]
> > otherwise return XSTATE_ALL.
> > It means we can save the area safely, if the guest has ever use 
> > XSTATE_NONLAZY | XSTATE_FP_SSE only. 
> 
> That's one step in the right direction. But the main difference
> between XSAVE/XSAVEOPT and XSAVEC/XSAVES is that the former
> can be used incrementally, while the latter can't. And I highly
> doubt the modified optimization the latter two support will kick in
> very often, since there's quite good a chance that the guest
> itself executed another one of these between two of our instances.
> Which to me means it should at least be investigated whether using
> XSAVEOPT in favor of XSAVEC wouldn't produce better performance,
> and whether XSAVES wouldn't better be used only if the guest uses
> a component which can only be saved that way.
> 
Thanks. 

Ok , I will do the performace test. And can you suggest me some workload/benchmark 
can be used here to the xsave related performance test ?


Other thing is from the text above, I guess that the best way to solve xsave[cs]
problem is:
1. use xsaveopt instead of xsave[cs] nowdays.
2. use xsaves whenever a component can only be saved that way( when some
   supervised components are supported in xen).
3. no xsavec support.
4. xsavec/xsaves feature will expose guest os if point 2 is ok.

If the above 4 points are ok to you, then I will write a patch to do
this.

> 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

Re: [PATCH 2/3] x86/xsaves: fix overwriting between non-lazy/lazy xsave[sc]

[Jan Beulich]

>>> On 29.02.16 at 10:06, <shuai.ruan@linux.intel.com> wrote:
> On Fri, Feb 26, 2016 at 01:42:35AM -0700, Jan Beulich wrote:
>> >>> On 26.02.16 at 08:41, <shuai.ruan@linux.intel.com> wrote:
>> > On Wed, Feb 24, 2016 at 02:16:38AM -0700, Jan Beulich wrote:
>> >> >> The description lacks any mention of the performance impact,
>> >> >> and what investigation was done to find ways to perhaps
>> >> >> overcome this. For example, regardless of cpu_has_xsaves,
>> >> >> do we really always need to _use_ XSAVES?
>> >> >> 
>> >> > Currently no supervisor xstates is enabled in xen or even in
>> >> > guest os. Using xsaves is a little ahead (xsavec may used).  
>> >> > xsavec may also cause overwriting problem like xsaves.
>> >> > I will add performance impact in the description. 
>> >> > I am still thinking of a better way to overcome the overhead xsave
>> >> > (But have not get a better solution yet).
>> >> 
>> >> I was thinking that taking into consideration the features a guest
>> >> has ever used (i.e. v->arch.xcr0_accum) to decide which variant
>> >> to use may be a worthwhile avenue to explore.
>> >> 
>> > Oh, Thanks for your suggestion.
>> > You mean when (v->arch.xcr0_accum & (XSTATE_LAZY & ~XSTATE_FP_SSE)) return 
>> > false,
>> > we can return XSTATE_NONLAZY in vcpu_xsave_mask when using xsave[cs]
>> > otherwise return XSTATE_ALL.
>> > It means we can save the area safely, if the guest has ever use 
>> > XSTATE_NONLAZY | XSTATE_FP_SSE only. 
>> 
>> That's one step in the right direction. But the main difference
>> between XSAVE/XSAVEOPT and XSAVEC/XSAVES is that the former
>> can be used incrementally, while the latter can't. And I highly
>> doubt the modified optimization the latter two support will kick in
>> very often, since there's quite good a chance that the guest
>> itself executed another one of these between two of our instances.
>> Which to me means it should at least be investigated whether using
>> XSAVEOPT in favor of XSAVEC wouldn't produce better performance,
>> and whether XSAVES wouldn't better be used only if the guest uses
>> a component which can only be saved that way.
>> 
> Thanks. 
> 
> Ok , I will do the performace test. And can you suggest me some 
> workload/benchmark 
> can be used here to the xsave related performance test ?

Measuring just instruction execution time should be fine for the
purpose here, I think.

> Other thing is from the text above, I guess that the best way to solve 
> xsave[cs]
> problem is:
> 1. use xsaveopt instead of xsave[cs] nowdays.
> 2. use xsaves whenever a component can only be saved that way( when some
>    supervised components are supported in xen).
> 3. no xsavec support.
> 4. xsavec/xsaves feature will expose guest os if point 2 is ok.

Provided this results in better performance than the alternative(s),
yes.

Jan


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

Re: [PATCH 2/3] x86/xsaves: fix overwriting between non-lazy/lazy xsave[sc]

[Shuai Ruan]

On Mon, Feb 29, 2016 at 02:33:49AM -0700, Jan Beulich wrote:
> > Thanks. 
> > 
> > Ok , I will do the performace test. And can you suggest me some 
> > workload/benchmark 
> > can be used here to the xsave related performance test ?
> 
> Measuring just instruction execution time should be fine for the
> purpose here, I think.
> 
I do the test as follow:

Xsave time measure method:
use function get_s_time, before/after xsave to get the difference. 

1. only enable xsaveopt in xen, start two guest(no workload running in
guest). 

xsave time land into two ranges [28 - 40] and [110 - 140](most in
110-120)

2. only enable xsavec in xen
xsave time land into two ranges [30 - 50] and [120 - 140]

I use a fragment of the test result and do a rough estimation. 

And I also try to get the average execution time (calculate very 10 xsave
times), the result is alomst the same as above.

The result can show xsaveopt has better performance than xsavec ( if the
test/measure method is corret ).

> > Other thing is from the text above, I guess that the best way to solve 
> > xsave[cs]
> > problem is:
> > 1. use xsaveopt instead of xsave[cs] nowdays.
> > 2. use xsaves whenever a component can only be saved that way( when some
> >    supervised components are supported in xen).
> > 3. no xsavec support.
> > 4. xsavec/xsaves feature will expose guest os if point 2 is ok.
> 
> Provided this results in better performance than the alternative(s),
> yes.
> 
> 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

[PATCH 3/3] x86/xsaves: ebx may return wrong value using CPUID eax=0xdh, ecx =1

[Shuai Ruan]

Refer to SDM 13.4.3 Extended Region of an XSAVE Area. The value return
by ecx[1] with cpuid function 0xdh and sub-fucntion i (i>1) indicates
the alignment of the state component i when the compacted format of the
extended region of an xsave area is used.

So when hvm guest using CPUID eax=0xdh,ecx=1 to get the size of area
used for compacted format, we need to take alignment into consideration.

Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
---
 tools/libxc/xc_cpuid_x86.c | 10 ++++++++--
 xen/arch/x86/hvm/hvm.c     | 12 ++++++++++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index c142595..d36f20b 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -285,6 +285,7 @@ static void intel_xc_cpuid_policy(xc_interface *xch,
 #define XSAVEC          (1 << 1)
 #define XGETBV1         (1 << 2)
 #define XSAVES          (1 << 3)
+#define XSTATE_ALIGN64  (1 << 1)
 /* Configure extended state enumeration leaves (0x0000000D for xsave) */
 static void xc_cpuid_config_xsave(xc_interface *xch,
                                   const struct cpuid_domain_info *info,
@@ -333,8 +334,13 @@ static void xc_cpuid_config_xsave(xc_interface *xch,
             regs[0] = regs[1] = regs[2] = regs[3] = 0;
             break;
         }
-        /* Don't touch EAX, EBX. Also cleanup ECX and EDX */
-        regs[2] = regs[3] = 0;
+        /* Don't touch EAX, EBX.
+         * ECX[1] indicates whether the state component
+         * needs alignment when compacted format is used.
+         */
+        regs[2] &= XSTATE_ALIGN64;
+        /* Clean up EDX */
+        regs[3] = 0;
         break;
     }
 }
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index fe382ce..4ccd392 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4703,7 +4703,19 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
                     for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
                         if ( (v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss) &
                              (1ULL << sub_leaf) )
+                        {
+                            domain_cpuid(d, input, sub_leaf, &_eax, &_ebx,
+                                         &_ecx, &_edx);
+                            /*
+                             * The value return by _ecx[1] indicates the
+                             * alignment of the state component i when the
+                             * compacted format of the extended region of
+                             *  an xsave area is used.
+                             */
+                            if (_ecx & XSTATE_ALIGN64)
+                                *ebx = ROUNDUP(*ebx, 64);
                             *ebx += xstate_sizes[sub_leaf];
+			}
             }
             else
                 *ebx = *ecx = *edx = 0;
-- 
1.9.1

Re: [PATCH 3/3] x86/xsaves: ebx may return wrong value using CPUID eax=0xdh, ecx =1

[Jan Beulich]

>>> On 22.02.16 at 06:35, <shuai.ruan@linux.intel.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4703,7 +4703,19 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
>                      for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
>                          if ( (v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss) &
>                               (1ULL << sub_leaf) )
> +                        {
> +                            domain_cpuid(d, input, sub_leaf, &_eax, &_ebx,
> +                                         &_ecx, &_edx);
> +                            /*
> +                             * The value return by _ecx[1] indicates the
> +                             * alignment of the state component i when the
> +                             * compacted format of the extended region of
> +                             *  an xsave area is used.
> +                             */
> +                            if (_ecx & XSTATE_ALIGN64)
> +                                *ebx = ROUNDUP(*ebx, 64);
>                              *ebx += xstate_sizes[sub_leaf];
> +			}
>              }

Besides the various coding style issues I wonder how you get
away without any similar adjustment to pv_cpuid().

Jan

Re: [PATCH 3/3] x86/xsaves: ebx may return wrong value using CPUID eax=0xdh, ecx =1

[Shuai Ruan]

On Mon, Feb 22, 2016 at 10:18:42AM -0700, Jan Beulich wrote:
> > @@ -4703,7 +4703,19 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx,
> >                      for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
> >                          if ( (v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss) &
> >                               (1ULL << sub_leaf) )
> > +                        {
> > +                            domain_cpuid(d, input, sub_leaf, &_eax, &_ebx,
> > +                                         &_ecx, &_edx);
> > +                            /*
> > +                             * The value return by _ecx[1] indicates the
> > +                             * alignment of the state component i when the
> > +                             * compacted format of the extended region of
> > +                             *  an xsave area is used.
> > +                             */
> > +                            if (_ecx & XSTATE_ALIGN64)
> > +                                *ebx = ROUNDUP(*ebx, 64);
> >                              *ebx += xstate_sizes[sub_leaf];
> > +			}
> >              }
> 
> Besides the various coding style issues I wonder how you get
> away without any similar adjustment to pv_cpuid().
Current pv does not support xsaves , and ebx return by CPUID (eax= 0xd,
ecx =1 ) is depend on xcr0 | msr_xss. msr_xss is only support in hvm.

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

Re: [PATCH 3/3] x86/xsaves: ebx may return wrong value using CPUID eax=0xdh, ecx =1

[Shuai Ruan]

On Mon, Feb 22, 2016 at 10:18:42AM -0700, Jan Beulich wrote:
> >                               (1ULL << sub_leaf) )
> > +                        {
> > +                            domain_cpuid(d, input, sub_leaf, &_eax, &_ebx,
> > +                                         &_ecx, &_edx);
> > +                            /*
> > +                             * The value return by _ecx[1] indicates the
> > +                             * alignment of the state component i when the
> > +                             * compacted format of the extended region of
> > +                             *  an xsave area is used.
> > +                             */
> > +                            if (_ecx & XSTATE_ALIGN64)
> > +                                *ebx = ROUNDUP(*ebx, 64);
> >                              *ebx += xstate_sizes[sub_leaf];
> > +			}
> >              }
> 
> Besides the various coding style issues I wonder how you get
> away without any similar adjustment to pv_cpuid().
> 
Please ignore the email reponse to your question earlier.
I will add pv_cpuid() support too.

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

Re: [PATCH 3/3] x86/xsaves: ebx may return wrong value using CPUID eax=0xdh, ecx =1

[Jan Beulich]

>>> On 24.02.16 at 06:42, <shuai.ruan@linux.intel.com> wrote:
> On Mon, Feb 22, 2016 at 10:18:42AM -0700, Jan Beulich wrote:
>> > @@ -4703,7 +4703,19 @@ void hvm_cpuid(unsigned int input, unsigned int *eax, 
> unsigned int *ebx,
>> >                      for ( sub_leaf = 2; sub_leaf < 63; sub_leaf++ )
>> >                          if ( (v->arch.xcr0 | v->arch.hvm_vcpu.msr_xss) &
>> >                               (1ULL << sub_leaf) )
>> > +                        {
>> > +                            domain_cpuid(d, input, sub_leaf, &_eax, &_ebx,
>> > +                                         &_ecx, &_edx);
>> > +                            /*
>> > +                             * The value return by _ecx[1] indicates the
>> > +                             * alignment of the state component i when the
>> > +                             * compacted format of the extended region of
>> > +                             *  an xsave area is used.
>> > +                             */
>> > +                            if (_ecx & XSTATE_ALIGN64)
>> > +                                *ebx = ROUNDUP(*ebx, 64);
>> >                              *ebx += xstate_sizes[sub_leaf];
>> > +			}
>> >              }
>> 
>> Besides the various coding style issues I wonder how you get
>> away without any similar adjustment to pv_cpuid().
> Current pv does not support xsaves , and ebx return by CPUID (eax= 0xd,
> ecx =1 ) is depend on xcr0 | msr_xss. msr_xss is only support in hvm.

My reading of the manual didn't result in such a connection. Mind
pointing out where this is being stated? (From an abstract
perspective I also can't see why the alignment flag would be tied
to XSAVES.)

Jan