Re: [Xen-devel] [V3] x86/xsaves: fix overwriting between non-lazy/lazy xsaves

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.03.16 at 08:19, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> After doing some performance test on xsavec and xsaveopt(suggested by jan),
> the result show xsaveopt performs better than xsavec. This patch will clean
> up xsavec suppot code in xen.
> 
> Also xsaves will be disabled (xsaves will be used when supervised state is
> introduced). Here in this patch do not change xc_cpuid_config_xsave in
> tools/libxc/xc_cpuid_x86.c but add some check in hvm_cpuid. Next time
> xsaves is needed, only add some code in xstate_init is enough.

I think both of these are too much of a step backwards. E.g. ...

> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -922,7 +922,7 @@ long arch_do_domctl(
>                  ret = -EFAULT;
>  
>              offset += sizeof(v->arch.xcr0_accum);
> -            if ( !ret && (cpu_has_xsaves || cpu_has_xsavec) )
> +            if ( !ret && cpu_has_xsaves )

... here (and similarly elsewhere) you shouldn't make the code
continue to depend on the raw CPU feature, but you should have
a variable (or could be a #define for now) indicating whether we're
actively using compressed state areas. For the purpose of
alternative patching, the most suitable thing likely would be a
synthetic CPU feature.

In no case do I see any reason to artificially make cpu_has_* yield
false despite the hardware actually having that feature. Doing so
would only risk making future changes more cumbersome.

> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -118,7 +118,19 @@ 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;
> +    /*
> +     * The offsets of components in the extended region of xsave area xsaved 
> by
> +     * xasves are not fixed. This may cause overwriting xsave area  when
> +     * v->fpu_dirtied set is followed by one with v->fpu_dirtied clear.
> +     * The way solve this problem is taking xcro_accum into consideration.
> +     * if guest has ever used lazy states (exclude XSTATE_FP_SSE),
> +     * vcpu_xsave_mask will return XSTATE_ALL. Otherwise return 
> XSTATE_NONLAZY.
> +     * The reason XSTATE_FP_SSE should be excluded is that the offsets of
> +     * XSTATE_FP_SSE (in the legacy region of xsave area) are fixed, saving
> +     * XSTATE_FS_SSE using xsaves will not cause overwriting problem.
> +     */

Please carefully go through this comment and fix all typos,
typographical issues, and ideally also grammar. And there also is at
least one apparent factual issue: "The reason XSTATE_FP_SSE
should be excluded ..." seems wrong to me - I think you mean "may"
instead of "should", because this is an optimization aspect, not a
requirement of any sort.

> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -165,7 +165,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, 
> unsigned int size)
>      u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
>      u64 valid;
>  
> -    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> +    if ( !cpu_has_xsaves )
>      {
>          memcpy(dest, xsave, size);
>          return;
> @@ -206,7 +206,7 @@ void compress_xsave_states(struct vcpu *v, const void 
> *src, unsigned int size)
>      u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
>      u64 valid;
>  
> -    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> +    if ( !cpu_has_xsaves )
>      {
>          memcpy(xsave, src, size);
>          return;

Wouldn't both of these better simply check xcomp_bv[63] instead
of CPU features?

Jan


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