Re: [Xen-devel] [PATCH] x86/xsave: simplify xcomp_bv initialization

[Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>]

On Thu, Dec 17, 2015 at 09:58:38AM -0700, Jan Beulich wrote:
> This eliminates a number of pointless conditionals: Bits 0 and 1 of
> xcomp_bv don't matter anyway, and as long as none of bits 2..62 are
> set, setting bit 63 is pointless too.
> 
We should set bit 63 of the xcomp_bv when initialization.
From the initialization log of xen, I got that xrstors will 
excute first(before xsaves). xrstors will check bit 63 of xcopm_bv. 
If it is not set, on the xsaves-support machine xen will hung.
(I test the patch on skylake machine base on the master branch)

> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Noticed while working on XSA-165.
> 
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -912,9 +912,7 @@ int arch_set_info_guest(
>          if ( v->arch.xsave_area )
>          {
>              v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
> -            if ( cpu_has_xsaves || cpu_has_xsavec )
> -                v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE |
> -                                                         
> XSTATE_COMPACTION_ENABLED;
> +            v->arch.xsave_area->xsave_hdr.xcomp_bv = 0;
>          }
>      }
>      else if ( v->arch.xsave_area )
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2092,9 +2092,7 @@ static int hvm_load_cpu_ctxt(struct doma
>  
>          memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
>          xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
> -        if ( cpu_has_xsaves || cpu_has_xsavec )
> -            xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE |
> -                                             XSTATE_COMPACTION_ENABLED;
> +        xsave_area->xsave_hdr.xcomp_bv = 0;
>      }
>      else
>          memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
> @@ -5562,9 +5560,7 @@ void hvm_vcpu_reset_state(struct vcpu *v
>      if ( v->arch.xsave_area )
>      {
>          v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP;
> -        if ( cpu_has_xsaves || cpu_has_xsavec )
> -            v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP |
> -                                                     
> XSTATE_COMPACTION_ENABLED;
> +        v->arch.xsave_area->xsave_hdr.xcomp_bv = 0;
>      }
>  
>      v->arch.vgc_flags = VGCF_online;
> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -270,11 +270,7 @@ int vcpu_init_fpu(struct vcpu *v)
>          return rc;
>  
>      if ( v->arch.xsave_area )
> -    {
>          v->arch.fpu_ctxt = &v->arch.xsave_area->fpu_sse;
> -        if ( cpu_has_xsaves || cpu_has_xsavec )
> -            v->arch.xsave_area->xsave_hdr.xcomp_bv = 
> XSTATE_COMPACTION_ENABLED;
> -    }
>      else
>      {
>          v->arch.fpu_ctxt = _xzalloc(sizeof(v->arch.xsave_area->fpu_sse), 16);
> 
> 
> 

> x86/xsave: simplify xcomp_bv initialization
> 
> This eliminates a number of pointless conditionals: Bits 0 and 1 of
> xcomp_bv don't matter anyway, and as long as none of bits 2..62 are
> set, setting bit 63 is pointless too.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Noticed while working on XSA-165.
> 
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -912,9 +912,7 @@ int arch_set_info_guest(
>          if ( v->arch.xsave_area )
>          {
>              v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
> -            if ( cpu_has_xsaves || cpu_has_xsavec )
> -                v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE |
> -                                                         
> XSTATE_COMPACTION_ENABLED;
> +            v->arch.xsave_area->xsave_hdr.xcomp_bv = 0;
>          }
>      }
>      else if ( v->arch.xsave_area )
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2092,9 +2092,7 @@ static int hvm_load_cpu_ctxt(struct doma
>  
>          memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
>          xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE;
> -        if ( cpu_has_xsaves || cpu_has_xsavec )
> -            xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP_SSE |
> -                                             XSTATE_COMPACTION_ENABLED;
> +        xsave_area->xsave_hdr.xcomp_bv = 0;
>      }
>      else
>          memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs));
> @@ -5562,9 +5560,7 @@ void hvm_vcpu_reset_state(struct vcpu *v
>      if ( v->arch.xsave_area )
>      {
>          v->arch.xsave_area->xsave_hdr.xstate_bv = XSTATE_FP;
> -        if ( cpu_has_xsaves || cpu_has_xsavec )
> -            v->arch.xsave_area->xsave_hdr.xcomp_bv = XSTATE_FP |
> -                                                     
> XSTATE_COMPACTION_ENABLED;
> +        v->arch.xsave_area->xsave_hdr.xcomp_bv = 0;
>      }
>  
>      v->arch.vgc_flags = VGCF_online;
> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -270,11 +270,7 @@ int vcpu_init_fpu(struct vcpu *v)
>          return rc;
>  
>      if ( v->arch.xsave_area )
> -    {
>          v->arch.fpu_ctxt = &v->arch.xsave_area->fpu_sse;
> -        if ( cpu_has_xsaves || cpu_has_xsavec )
> -            v->arch.xsave_area->xsave_hdr.xcomp_bv = 
> XSTATE_COMPACTION_ENABLED;
> -    }
>      else
>      {
>          v->arch.fpu_ctxt = _xzalloc(sizeof(v->arch.xsave_area->fpu_sse), 16);


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