|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH V4 2/4] x86/xsaves: enable xsaves/xrstors in xen
On 25/08/15 11:54, Shuai Ruan 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.
>
> Signed-off-by: Shuai Ruan <shuai.ruan@xxxxxxxxxxxxxxx>
> ---
> xen/arch/x86/domain.c | 2 +
> xen/arch/x86/domctl.c | 34 ++++++++++++---
> xen/arch/x86/hvm/hvm.c | 19 ++++++---
> xen/arch/x86/i387.c | 4 ++
> xen/arch/x86/traps.c | 7 ++--
> xen/arch/x86/xstate.c | 112
> ++++++++++++++++++++++++++++++++++---------------
> 6 files changed, 132 insertions(+), 46 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 045f6ff..7b8f649 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1529,6 +1529,8 @@ static void __context_switch(void)
> if ( xcr0 != get_xcr0() && !set_xcr0(xcr0) )
> BUG();
> }
> + if ( cpu_has_xsaves )
> + wrmsrl(MSR_IA32_XSS, n->arch.msr_ia32_xss);
This is still not appropriate. You need a per cpu variable and only
perform lazy writes of the MSR.
> vcpu_restore_fpu_eager(n);
> n->arch.ctxt_switch_to(n);
> }
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index bf62a88..da136876 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -867,6 +867,7 @@ long arch_do_domctl(
> if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
> {
> unsigned int size;
> + void * xsave_area;
Unnecessary space.
>
> ret = 0;
> vcpu_pause(v);
> @@ -896,9 +897,28 @@ 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 ( cpu_has_xsaves )
I think this would be more correct to gate on v->arch.xsave_area
actually being compressed.
> + {
> + xsave_area = xmalloc_bytes(size);
> + if ( !xsave_area )
> + {
> + ret = -ENOMEM;
> + goto vcpuextstate_out;
> + }
> +
> + save_xsave_states(v, xsave_area,
> + evc->size - 2*sizeof(uint64_t));
And on a similar note, save_xsave_states() is really
uncompress_xsave_area(). On further consideration, it should ASSERT()
that the source is currently compressed.
> +
> + if ( !ret && copy_to_guest_offset(evc->buffer, offset,
> + xsave_area, size -
> + 2 * sizeof(uint64_t)) )
> + ret = -EFAULT;
> + xfree(xsave_area);
> + }
> + else if ( !ret && copy_to_guest_offset(evc->buffer, offset,
> + (void *)v->arch.xsave_area,
> + size - 2 *
> sizeof(uint64_t)) )
> ret = -EFAULT;
>
> vcpu_unpause(v);
> @@ -954,8 +974,12 @@ 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 )
> + load_xsave_states(v, (void *)_xsave_area,
> + evc->size - 2*sizeof(uint64_t));
Similarly, load_xsave_states() is really compress_xsave_area().
You should check to see whether the area is already compressed, and just
memcpy() in that case.
> + else
> + memcpy(v->arch.xsave_area, (void *)_xsave_area,
> + evc->size - 2 * sizeof(uint64_t));
> vcpu_unpause(v);
> }
> else
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index c957610..dc444ac 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2148,8 +2148,12 @@ static int hvm_save_cpu_xsave_states(struct domain *d,
> hvm_domain_context_t *h)
> ctxt->xfeature_mask = xfeature_mask;
> ctxt->xcr0 = v->arch.xcr0;
> ctxt->xcr0_accum = v->arch.xcr0_accum;
> - memcpy(&ctxt->save_area, v->arch.xsave_area,
> - size - offsetof(struct hvm_hw_cpu_xsave, save_area));
> + if ( cpu_has_xsaves )
Stray hard tab.
> + save_xsave_states(v, (void *)&ctxt->save_area,
> + size - offsetof(struct
> hvm_hw_cpu_xsave,save_area));
These offsetof()s can become far shorter by using offsetof(*ctxt,
save_area).
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 9f5a6c6..e9beec1 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -936,9 +936,10 @@ void pv_cpuid(struct cpu_user_regs *regs)
> 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);
> + XSTATE_FEATURE_XSAVEC;
> + /* PV guest will not support xsaves. */
> + /* (cpu_has_xgetbv1 ? XSTATE_FEATURE_XGETBV1 : 0) |
> + (cpu_has_xsaves ? XSTATE_FEATURE_XSAVES : 0); */
Don't leave this code commented out like this. Just delete it.
> if ( !cpu_has_xsaves )
> b = c = d = 0;
> }
> diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
> index 3986515..9050607 100644
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -214,6 +214,11 @@ 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_xsaves )
> + asm volatile ( ".byte 0x48,0x0f,0xc7,0x2f"
> + : "=m" (*ptr)
> + : "a" (lmask), "d" (hmask), "D" (ptr) );
> + else
> if ( cpu_has_xsaveopt )
Please join the if() onto the previous line.
> {
> /*
> @@ -267,6 +272,11 @@ void xsave(struct vcpu *v, uint64_t mask)
> }
> else
> {
> + if ( cpu_has_xsaves )
> + asm volatile ( ".byte 0x48,0x0f,0xc7,0x2f"
> + : "=m" (*ptr)
> + : "a" (lmask), "d" (hmask), "D" (ptr) );
> + else
> if ( cpu_has_xsaveopt )
And here.
> @@ -466,16 +508,20 @@ void xstate_init(bool_t 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_xgetbv1 = !!(eax & XSTATE_FEATURE_XGETBV1);
> + cpu_has_xsaves = !!(eax & XSTATE_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_xgetbv1 != !(eax & XSTATE_FEATURE_XGETBV1));
> + BUG_ON(!cpu_has_xsaves != !(eax & XSTATE_FEATURE_XSAVES));
> }
> +
> + setup_xstate_features();
> + if ( cpu_has_xsaves )
> + setup_xstate_comp();
setup_state_comp() should only be called on the bsp, and
setup_xstate_features() should have a similar bsp/ap split with
generating or validating state.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |