Re: [Xen-devel] [V9 1/3] x86/xsaves: enable xsaves/xrstors/xsavec in xen

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.11.15 at 07:27, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
> to perform the xsave_area switching so that xen itself
> can benefit from them when available.
> 
> For xsaves/xrstors/xsavec only use compact format. Add format conversion
> support when perform guest os migration.
> 
> Also, pv guest will not support xsaves/xrstors.
> 
> Signed-off-by: Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>
> ---
>  xen/arch/x86/domain.c        |   7 ++
>  xen/arch/x86/domctl.c        |  30 +++++-
>  xen/arch/x86/hvm/hvm.c       |  18 +++-
>  xen/arch/x86/i387.c          |   4 +
>  xen/arch/x86/traps.c         |   6 +-
>  xen/arch/x86/xstate.c        | 242 
> +++++++++++++++++++++++++++++++++++++------
>  xen/include/asm-x86/xstate.h |   2 +
>  7 files changed, 265 insertions(+), 44 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index fe3be30..108d4f8 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -883,7 +883,12 @@ int arch_set_info_guest(
>      {
>          memcpy(v->arch.fpu_ctxt, &c.nat->fpu_ctxt, sizeof(c.nat->fpu_ctxt));
>          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;
> +        }

So here you nicely extend the existing conditional.

> @@ -1568,6 +1573,8 @@ static void __context_switch(void)
>              if ( xcr0 != get_xcr0() && !set_xcr0(xcr0) )
>                  BUG();
>          }
> +        if ( cpu_has_xsaves && has_hvm_container_vcpu(n) )
> +            set_msr_xss(n->arch.hvm_vcpu.msr_xss);

Why not also here (the previous if() uses cpu_has_xsave, which
you surely depend on)? Agreed the difference is minor for modern
CPUs, but I wanted to ask anyway. I.e. an explanation will do,
no need to re-submit just because of this.

> @@ -158,6 +334,20 @@ void xsave(struct vcpu *v, uint64_t mask)
>          ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = word_size;
>  }
>  
> +#define XSTATE_FIXUP ".section .fixup,\"ax\"      \n"        \
> +                     "2: mov %5,%%ecx             \n"        \
> +                     "   xor %1,%1                \n"        \
> +                     "   rep stosb                \n"        \
> +                     "   lea %2,%0                \n"        \
> +                     "   mov %3,%1                \n"        \
> +                     "   jmp 1b                   \n"        \
> +                     ".previous                   \n"        \
> +                     _ASM_EXTABLE(1b, 2b)                    \
> +                     : "+&D" (ptr), "+&a" (lmask)            \
> +                     : "m" (*ptr), "g" (lmask), "d" (hmask), \
> +                       "m" (xsave_cntxt_size)                \
> +                     : "ecx"
> +
>  void xrstor(struct vcpu *v, uint64_t mask)
>  {
>      uint32_t hmask = mask >> 32;
> @@ -187,39 +377,22 @@ void xrstor(struct vcpu *v, uint64_t mask)
>      switch ( __builtin_expect(ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET], 8) )
>      {
>      default:
> -        asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
> -                       ".section .fixup,\"ax\"      \n"
> -                       "2: mov %5,%%ecx             \n"
> -                       "   xor %1,%1                \n"
> -                       "   rep stosb                \n"
> -                       "   lea %2,%0                \n"
> -                       "   mov %3,%1                \n"
> -                       "   jmp 1b                   \n"
> -                       ".previous                   \n"
> -                       _ASM_EXTABLE(1b, 2b)
> -                       : "+&D" (ptr), "+&a" (lmask)
> -                       : "m" (*ptr), "g" (lmask), "d" (hmask),
> -                         "m" (xsave_cntxt_size)
> -                       : "ecx" );
> +        alternative_input("1: "".byte 0x48,0x0f,0xae,0x2f",
> +                          ".byte 0x48,0x0f,0xc7,0x1f",
> +                          X86_FEATURE_XSAVES,
> +                          "D" (ptr), "m" (*ptr), "a" (lmask), "d" (hmask));
> +        asm volatile (XSTATE_FIXUP);
>          break;
>      case 4: case 2:
> -        asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
> -                       ".section .fixup,\"ax\" \n"
> -                       "2: mov %5,%%ecx        \n"
> -                       "   xor %1,%1           \n"
> -                       "   rep stosb           \n"
> -                       "   lea %2,%0           \n"
> -                       "   mov %3,%1           \n"
> -                       "   jmp 1b              \n"
> -                       ".previous              \n"
> -                       _ASM_EXTABLE(1b, 2b)
> -                       : "+&D" (ptr), "+&a" (lmask)
> -                       : "m" (*ptr), "g" (lmask), "d" (hmask),
> -                         "m" (xsave_cntxt_size)
> -                       : "ecx" );
> +        alternative_input("1: "".byte 0x0f,0xae,0x2f",
> +                          ".byte 0x0f,0xc7,0x1f",
> +                          X86_FEATURE_XSAVES,
> +                          "D" (ptr), "m" (*ptr), "a" (lmask), "d" (hmask));
> +        asm volatile (XSTATE_FIXUP);
>          break;
>      }
>  }
> +#undef XSTATE_FIXUP

Repeating my comment on v8: "I wonder whether at least for the
restore side alternative asm wouldn't result in better readable code
and at the same time in a smaller patch." Did you at least look into
that option?

> @@ -343,11 +516,18 @@ void xstate_init(struct cpuinfo_x86 *c)
>  
>      /* Mask out features not currently understood by Xen. */
>      eax &= (cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> -            cpufeat_mask(X86_FEATURE_XSAVEC));
> +            cpufeat_mask(X86_FEATURE_XSAVEC) |
> +            cpufeat_mask(X86_FEATURE_XGETBV1) |
> +            cpufeat_mask(X86_FEATURE_XSAVES));
>  
>      c->x86_capability[cpufeat_word(X86_FEATURE_XSAVEOPT)] = eax;
>  
>      BUG_ON(eax != 
> boot_cpu_data.x86_capability[cpufeat_word(X86_FEATURE_XSAVEOPT)]);
> +
> +    if ( setup_xstate_features(bsp) && bsp )
> +        BUG();

Well, not what I had hoped for, but okay.

Jan

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