Re: [Xen-devel] [V8 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.10.15 at 11:48, <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>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

There were actual bugs fixed from v7 to v8, plus there's a public
interface change, so retaining this tag is wrong.

> @@ -2025,6 +2026,11 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>  
>      v->arch.hvm_vcpu.msr_tsc_aux = ctxt.msr_tsc_aux;
>  
> +    if ( cpu_has_xsaves )
> +        v->arch.hvm_vcpu.msr_xss = ctxt.msr_xss;
> +    else
> +        v->arch.hvm_vcpu.msr_xss = 0;

Cases like this call for use of ?:.

> @@ -2257,10 +2266,10 @@ static int hvm_load_cpu_xsave_states(struct domain 
> *d, hvm_domain_context_t *h)
>      v->arch.xcr0_accum = ctxt->xcr0_accum;
>      if ( ctxt->xcr0_accum & XSTATE_NONLAZY )
>          v->arch.nonlazy_xstate_used = 1;
> -    memcpy(v->arch.xsave_area, &ctxt->save_area,
> -           min(desc->length, size) - offsetof(struct hvm_hw_cpu_xsave,
> -           save_area));
>  
> +    compress_xsave_states(v, &ctxt->save_area,
> +                          min(desc->length, size) -
> +                          offsetof(struct hvm_hw_cpu_xsave,save_area));
>      return 0;

This change now misplaces the blank line. Please - a little more care
when comments on formatting were already given.

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -935,9 +935,10 @@ void pv_cpuid(struct cpu_user_regs *regs)
>              goto unsupported;
>          if ( regs->_ecx == 1 )
>          {
> -            a &= boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32];
> -            if ( !cpu_has_xsaves )
> -                b = c = d = 0;
> +            a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> +                 cpufeat_mask(X86_FEATURE_XSAVEC) |
> +                 (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 0);

Why the cpu_has_xgetbv1 dependency? If you want to do it this
way, it will get unreadable with two or three more features. Why
don't you simply and with the known mask on top of the and with
the capability flags that was already there?

And actually I realize I may have misguided you: xstate_init()
already makes sure boot_cpu_data.x86_capability[] doesn't
contain any unsupported flags, so keeping just the masking
that's already there should be enough.

> +void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
> +{
> +    struct xsave_struct *xsave = v->arch.xsave_area;
> +    u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
> +    u64 valid;
> +
> +    if ( !cpu_has_xsaves && !cpu_has_xsavec )
> +    {
> +        memcpy(dest, xsave, size);
> +        return;
> +    }
> +
> +    ASSERT(xsave_area_compressed(xsave));
> +    /*
> +     * Copy legacy XSAVE area, to avoid complications with CPUID
> +     * leaves 0 and 1 in the loop below.
> +     */
> +    memcpy(dest, xsave, FXSAVE_SIZE);
> +
> +    ((struct xsave_struct *)dest)->xsave_hdr.xstate_bv = xstate_bv;
> +    ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;

Wouldn't it be more logical to also memcpy() the header? Which
would do away with at least one of these ugly casts, would
allow folding with the memcpy() done right before, _and_ would
eliminate an (apparent or real I'm not sure without looking in
more detail) information leak (the remainder of the header).

> +    /*
> +     * Copy each region from the possibly compacted offset to the
> +     * non-compacted offset.
> +     */
> +    valid = xstate_bv & ~XSTATE_FP_SSE;
> +    while ( valid )
> +    {
> +        u64 feature = valid & -valid;
> +        unsigned int index = fls(feature) - 1;
> +        const void *src = get_xsave_addr(xsave, index);
> +
> +        if ( src )
> +        {
> +            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
> +            memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
> +        }
> +
> +        valid &= ~feature;
> +    }
> +
> +}

Stray blank line.

> @@ -187,39 +379,24 @@ 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" );
> -        break;
> +        if ( cpu_has_xsaves )
> +            asm volatile ( "1: .byte 0x48,0x0f,0xc7,0x1f\n"
> +                           XSTATE_FIXUP );
> +        else
> +            asm volatile ( "1: .byte 0x48,0x0f,0xae,0x2f\n"
> +                           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" );
> +        if ( cpu_has_xsaves )
> +            asm volatile ( "1: .byte 0x0f,0xc7,0x1f\n"
> +                           XSTATE_FIXUP );
> +        else
> +            asm volatile ( "1: .byte 0x0f,0xae,0x2f\n"
> +                           XSTATE_FIXUP );

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.

> @@ -343,11 +520,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[X86_FEATURE_XSAVEOPT / 32] = eax;
>  
>      BUG_ON(eax != boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32]);
> +
> +    if ( setup_xstate_features(bsp) )
> +        BUG();

BUG()? On the BSP maybe, but APs should simply fail being
brought up instead of bringing down the whole system.

> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -140,6 +140,7 @@ struct hvm_hw_cpu {
>      uint64_t msr_syscall_mask;
>      uint64_t msr_efer;
>      uint64_t msr_tsc_aux;
> +    uint64_t msr_xss;

You can't extend a public interface structure like this. New MSRs
shouldn't be saved/restored this way anyway - that's what we
have struct hvm_msr for.

Jan

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