|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [V5 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen
>>> On 21.09.15 at 13:33, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> This patch uses xsaves/xrstors instead of xsaveopt/xrstor
> to perform the xsave_area switching so that xen itself
> can benefit from them when available.
>
> For xsaves/xrstors only use compact format. Add format conversion
> support when perform guest os migration.
Assuming the hardware has XSAVEC but no XSAVES support - why
wouldn't we want use XSAVEC in that case?
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1529,6 +1529,9 @@ static void __context_switch(void)
> if ( xcr0 != get_xcr0() && !set_xcr0(xcr0) )
> BUG();
> }
> + if ( cpu_has_xsaves )
> + if ( is_hvm_vcpu(n) )
> + set_msr_xss(n->arch.hvm_vcpu.msr_xss);
The two if()s should be combined.
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -867,7 +867,7 @@ long arch_do_domctl(
> if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
> {
> unsigned int size;
> -
> + void * xsave_area;
> ret = 0;
Stray removal of a blank line. Stray blank after *.
> @@ -896,9 +896,30 @@ long arch_do_domctl(
> ret = -EFAULT;
>
> offset += sizeof(v->arch.xcr0_accum);
> - if ( !ret && copy_to_guest_offset(evc->buffer, offset,
> - (void *)v->arch.xsave_area,
> - size - 2 * sizeof(uint64_t)) )
> +
> + if ( !ret && (cpu_has_xsaves || cpu_has_xsavec) &&
Ah, you actually use it, but don't say so in the description.
> + xsave_area_compressed(v->arch.xsave_area) )
> + {
> + xsave_area = xmalloc_bytes(size);
Perhaps this variable would better be declared here anyway.
> + if ( !xsave_area )
> + {
> + ret = -ENOMEM;
> + vcpu_unpause(v);
> + goto vcpuextstate_out;
This is unfortunate - it would be better if you could avoid failing
the request here.
> + }
> +
> + save_xsave_states(v, xsave_area,
> + evc->size - 2*sizeof(uint64_t));
Blanks around * please.
> +
> + if ( !ret && copy_to_guest_offset(evc->buffer, offset,
There's no way for ret to be non-zero at this point.
> + xsave_area, size -
Hard tabs (more elsewhere).
> @@ -954,8 +975,13 @@ long arch_do_domctl(
> v->arch.xcr0_accum = _xcr0_accum;
> if ( _xcr0_accum & XSTATE_NONLAZY )
> v->arch.nonlazy_xstate_used = 1;
> - memcpy(v->arch.xsave_area, _xsave_area,
> - evc->size - 2 * sizeof(uint64_t));
> + if ( (cpu_has_xsaves || cpu_has_xsavec) &&
> + !xsave_area_compressed(_xsave_area) )
Is it intended to support compact input here? Where would such
come from? And if so, don't you need to validate the input (e.g.
being a certain size)?
> @@ -2248,9 +2253,15 @@ 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));
> + if ( (cpu_has_xsaves || cpu_has_xsavec) &&
> + !xsave_area_compressed((struct xsave_struct *)&ctxt->save_area) )
Same questions here.
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -935,10 +935,9 @@ void pv_cpuid(struct cpu_user_regs *regs)
> goto unsupported;
> if ( regs->_ecx == 1 )
> {
> - a &= XSTATE_FEATURE_XSAVEOPT |
> - XSTATE_FEATURE_XSAVEC |
> - (cpu_has_xgetbv1 ? XSTATE_FEATURE_XGETBV1 : 0) |
> - (cpu_has_xsaves ? XSTATE_FEATURE_XSAVES : 0);
> + a &= cpufeat_mask(X86_FEATURE_XSAVEOPT) |
> + cpufeat_mask(X86_FEATURE_XSAVEC) |
> + (cpu_has_xgetbv1 ? cpufeat_mask(X86_FEATURE_XGETBV1) : 0);
I think you indeed mean to drop xsaves here, but then you should
say so (i.e. PV unsupported) in the commit message
> if ( !cpu_has_xsaves )
> b = c = d = 0;
And it would seem that this then needs to become unconditional?
> @@ -341,36 +357,68 @@ 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"
> + ".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" );
> + else
> + 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;
> 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"
> + ".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" );
> + else
> + 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" );
The amount of redundancy calls for some helper macros.
> @@ -495,18 +543,24 @@ void xstate_init(bool_t bsp)
> cpuid_count(XSTATE_CPUID, 1, &eax, &ebx, &ecx, &edx);
> if ( bsp )
> {
> - cpu_has_xsaveopt = !!(eax & XSTATE_FEATURE_XSAVEOPT);
> - cpu_has_xsavec = !!(eax & XSTATE_FEATURE_XSAVEC);
> - /* XXX cpu_has_xgetbv1 = !!(eax & XSTATE_FEATURE_XGETBV1); */
> - /* XXX cpu_has_xsaves = !!(eax & XSTATE_FEATURE_XSAVES); */
> + cpu_has_xsaveopt = !!(eax & cpufeat_mask(X86_FEATURE_XSAVEOPT));
> + cpu_has_xsavec = !!(eax & cpufeat_mask(X86_FEATURE_XSAVEC));
> + cpu_has_xgetbv1 = !!(eax & cpufeat_mask(X86_FEATURE_XGETBV1));
> + cpu_has_xsaves = !!(eax & cpufeat_mask(X86_FEATURE_XSAVES));
> }
> else
> {
> - BUG_ON(!cpu_has_xsaveopt != !(eax & XSTATE_FEATURE_XSAVEOPT));
> - BUG_ON(!cpu_has_xsavec != !(eax & XSTATE_FEATURE_XSAVEC));
> - /* XXX BUG_ON(!cpu_has_xgetbv1 != !(eax & XSTATE_FEATURE_XGETBV1));
> */
> - /* XXX BUG_ON(!cpu_has_xsaves != !(eax & XSTATE_FEATURE_XSAVES)); */
> + BUG_ON(!cpu_has_xsaveopt != !(eax &
> cpufeat_mask(X86_FEATURE_XSAVEOPT)));
> + BUG_ON(!cpu_has_xsavec != !(eax & cpufeat_mask(X86_FEATURE_XSAVEC)));
> + BUG_ON(!cpu_has_xgetbv1 != !(eax &
> cpufeat_mask(X86_FEATURE_XGETBV1)));
> + BUG_ON(!cpu_has_xsaves != !(eax & cpufeat_mask(X86_FEATURE_XSAVES)));
> }
> +
> + if( setup_xstate_features(bsp) )
> + BUG();
> +
> + if ( bsp && (cpu_has_xsaves || cpu_has_xsavec) )
> + setup_xstate_comp();
Implying that the function should be annotated __init in patch 1. With
it being static there without having a caller, it also means things wouldn't
build with just patch 1 applied. This needs to be addressed.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |