Re: [PATCH] X86: Fix vcpu xsave bug

[Andrew Cooper <andrew.cooper3@citrix.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2542 bytes --]

On 15/11/13 16:55, Liu, Jinsong wrote:
> commit 420bacd209e31917fd732ef3c1aeae03d6d14d18
> Author: Liu Jinsong <jinsong.liu@intel.com>
> Date:   Sat Nov 16 06:15:11 2013 +0800
>
>     X86: Fix vcpu xsave bug
>     
>     When nonlazy xstates used, it should be xsaved though lazy xstates are not dirty.
>     
>     Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>

Do you mean "...xsaved as though lazy...", are you stating that
currently, lazy states are not actually dirty?

Furthermore, can you describe why, and what goes wrong if you dont?  It
is not obvious why this change is needed.

>
> diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
> index 7649274..f1d2ccc 100644
> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -134,7 +134,7 @@ static inline void fpu_frstor(struct vcpu *v)
>  /*      FPU Save Functions     */
>  /*******************************/
>  /* Save x87 extended state */
> -static inline void fpu_xsave(struct vcpu *v)
> +static inline void fpu_xsave(struct vcpu *v, uint64_t mask)
>  {
>      bool_t ok;
>  
> @@ -145,7 +145,7 @@ static inline void fpu_xsave(struct vcpu *v)
>       */
>      ok = set_xcr0(v->arch.xcr0_accum | XSTATE_FP_SSE);
>      ASSERT(ok);
> -    xsave(v, v->arch.nonlazy_xstate_used ? XSTATE_ALL : XSTATE_LAZY);
> +    xsave(v, mask);
>      ok = set_xcr0(v->arch.xcr0 ?: XSTATE_FP_SSE);
>      ASSERT(ok);
>  }
> @@ -257,22 +257,29 @@ void vcpu_restore_fpu_lazy(struct vcpu *v)
>   */
>  void vcpu_save_fpu(struct vcpu *v)
>  {
> -    if ( !v->fpu_dirtied )
> -        return;
> -
>      ASSERT(!is_idle_vcpu(v));
>  
> -    /* This can happen, if a paravirtualised guest OS has set its CR0.TS. */
> +    /* Avoid recursion */

Why are you changing this comment?

~Andrew

>      clts();
> -
> -    if ( cpu_has_xsave )
> -        fpu_xsave(v);
> -    else if ( cpu_has_fxsr )
> -        fpu_fxsave(v);
> +    if ( !v->fpu_dirtied )
> +    {
> +        if ( v->arch.nonlazy_xstate_used )
> +        {
> +            ASSERT(cpu_has_xsave);
> +            fpu_xsave(v, XSTATE_NONLAZY);
> +        }
> +    }
>      else
> -        fpu_fsave(v);
> +    {
> +        if ( cpu_has_xsave )
> +            fpu_xsave(v, XSTATE_ALL);
> +        else if ( cpu_has_fxsr )
> +            fpu_fxsave(v);
> +        else
> +            fpu_fsave(v);
>  
> -    v->fpu_dirtied = 0;
> +        v->fpu_dirtied = 0;
> +    }
>      stts();
>  }
>  
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel


[-- Attachment #1.2: Type: text/html, Size: 3623 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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