[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> On 20.10.15 at 10:21, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2088,6 +2088,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>  
>          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;

Is this really |= ? Not just for similarity with the assignment above I
would expect this to be =, as we don't know what value the field
had before.

Also, are you intentionally using | instead of || in the if() (other than
you do elsewhere in the patch)?

> @@ -2257,10 +2259,9 @@ 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;

Bad removal of a blank line.

> --- a/xen/arch/x86/i387.c
> +++ b/xen/arch/x86/i387.c
> @@ -282,7 +282,11 @@ 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;

Again - |= or just = ?

> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -935,9 +935,9 @@ 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 &= (boot_cpu_data.x86_capability[X86_FEATURE_XSAVEOPT / 32] &
> +                 ~cpufeat_mask(X86_FEATURE_XSAVES));
> +            b = c = d = 0;

Considering that now we again seem to be black listing features here,
shouldn't we take the opportunity and convert this tho white listing now?

> +static void __init setup_xstate_comp(void)
> +{
> +    unsigned int i;
> +
> +    /*
> +     * The FP xstates and SSE xstates are legacy states. They are always
> +     * in the fixed offsets in the xsave area in either compacted form
> +     * or standard form.
> +     */
> +    xstate_comp_offsets[0] = 0;
> +    xstate_comp_offsets[1] = XSAVE_SSE_OFFSET;
> +
> +    xstate_comp_offsets[2] = FXSAVE_SIZE + XSAVE_HDR_SIZE;
> +
> +    for ( i = 3; i < xstate_features; i++ )
> +    {
> +        xstate_comp_offsets[i] = xstate_comp_offsets[i-1] +
> +                                 (((1ul << i) & xfeature_mask)
> +                                 ? xstate_sizes[i-1] : 0);

One off indentation. Also in both places "[i - 1]" please.

> +        ASSERT(xstate_comp_offsets[i] + xstate_sizes[i] <= xsave_cntxt_size);
> +    }
> +}
> +
> +static void *get_xsave_addr(const struct xsave_struct *xsave, unsigned int 
> xfeature_idx)
> +{
> +    if ( !((1ul << xfeature_idx) & xfeature_mask) )
> +        return NULL;
> +
> +    return (void *)xsave + xstate_comp_offsets[xfeature_idx];

You should never cast away constness like this. I actually don't see
why the parameter's type can't be (const) void*, avoiding any cast.

> +void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
> +{
> +    const 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;
> +
> +    /*
> +     * 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;
> +        void *src = get_xsave_addr(xsave, index);

const

> +        if ( src )
> +        {
> +            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
> +            memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
> +        }
> +
> +        valid &= ~feature;
> +    }
> +
> +}

Stray blank line.

> @@ -91,7 +253,15 @@ void xsave(struct vcpu *v, uint64_t mask)
>          typeof(ptr->fpu_sse.fip.sel) fcs = ptr->fpu_sse.fip.sel;
>          typeof(ptr->fpu_sse.fdp.sel) fds = ptr->fpu_sse.fdp.sel;
>  
> -        if ( cpu_has_xsaveopt )
> +        if ( cpu_has_xsaves )
> +            asm volatile ( ".byte 0x48,0x0f,0xc7,0x2f"
> +                           : "=m" (*ptr)
> +                           : "a" (lmask), "d" (hmask), "D" (ptr) );
> +        else if ( cpu_has_xsavec )
> +            asm volatile ( ".byte 0x48,0x0f,0xc7,0x27"
> +                           : "=m" (*ptr)
> +                           : "a" (lmask), "d" (hmask), "D" (ptr) );
> +        else if ( cpu_has_xsaveopt )

While the REX.W prefixes are needed here, ...

> @@ -144,7 +314,15 @@ void xsave(struct vcpu *v, uint64_t mask)
>      }
>      else
>      {
> -        if ( cpu_has_xsaveopt )
> +        if ( cpu_has_xsaves )
> +            asm volatile ( ".byte 0x48,0x0f,0xc7,0x2f"
> +                           : "=m" (*ptr)
> +                           : "a" (lmask), "d" (hmask), "D" (ptr) );
> +        else if ( cpu_has_xsavec )
> +            asm volatile ( ".byte 0x48,0x0f,0xc7,0x27"
> +                           : "=m" (*ptr)
> +                           : "a" (lmask), "d" (hmask), "D" (ptr) );
> +        else if ( cpu_has_xsaveopt )
>              asm volatile ( ".byte 0x0f,0xae,0x37"

... they don't belong here (as also visible from the pre-existing
xsaveopt).

> @@ -187,36 +379,20 @@ 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 0x48,0x0f,0xc7,0x1f\n"
> +                           XSTATE_FIXUP );

Same here.

Also, #undef XSTATE_FIXUP missing at the end of the function.

Jan

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.