|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |