Re: [PATCH 1/5] X86: Fix xsave bug for nonlazy xstates

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

On 19/11/13 10:46, Liu, Jinsong wrote:
> From 0d6072a31977aa509d04e0b0dc1642508a1ed87b Mon Sep 17 00:00:00 2001
> From: Liu Jinsong <jinsong.liu@intel.com>
> Date: Tue, 19 Nov 2013 18:27:36 +0800
> Subject: [PATCH 1/5] X86: Fix xsave bug for nonlazy xstates
>
> Nonlazy xstates should be xsaved each time when vcpu_save_fpu.
> Operation to nonlazy xstates will not trigger #NM exception,
> so whenever vcpu scheduled in it got restored and whenever
> scheduled out it should get saved.
>
> Currently this bug affects AMD LWP feature, and later Intel
> MPX feature. With the bugfix both LWP and MPX will work fine.
>
> Signed-off-by: Liu Jinsong <jinsong.liu@intel.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/arch/x86/i387.c |   30 ++++++++++++++++++++++++++----
>  1 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
> index 7649274..e7a590b 100644
> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -133,10 +133,33 @@ static inline void fpu_frstor(struct vcpu *v)
>  /*******************************/
>  /*      FPU Save Functions     */
>  /*******************************/
> +
> +static inline uint64_t fpu_xsave_mask(struct vcpu *v)
> +{
> +    if ( v->fpu_dirtied )
> +    {
> +        if ( v->arch.nonlazy_xstate_used )
> +            return XSTATE_ALL;
> +        else
> +            return XSTATE_LAZY;
> +    }
> +    else
> +    {
> +        if ( v->arch.nonlazy_xstate_used )
> +            return XSTATE_NONLAZY;
> +        else
> +            return 0;
> +    }
> +}
> +
>  /* Save x87 extended state */
>  static inline void fpu_xsave(struct vcpu *v)
>  {
>      bool_t ok;
> +    uint64_t mask = fpu_xsave_mask(v);
> +
> +    if ( !mask )
> +        return;
>  
>      ASSERT(v->arch.xsave_area);
>      /*
> @@ -145,7 +168,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,9 +280,6 @@ 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. */
> @@ -267,6 +287,8 @@ void vcpu_save_fpu(struct vcpu *v)
>  
>      if ( cpu_has_xsave )
>          fpu_xsave(v);
> +    else if ( !v->fpu_dirtied )
> +        ; /* Nothing */
>      else if ( cpu_has_fxsr )
>          fpu_fxsave(v);
>      else