[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 Wed, Aug 26, 2015 at 11:12:02AM +0100, Andrew Cooper wrote:
> 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.
> 
Ok.I will introduce two helper
void set_xss_msr(uint64_t msr_xss);
uint64_t get_xss_msr();
to 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.
> 
Sorry.
> >  
> >              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.
> 
I will introduce a helper called 
bool_t xsave_area_compressed(struct xsave_struct *xsave_area)
to check whether the area is compressed or not.
> > +            {
> > +                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.
> 
Ok.
> > +
> > +                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.
> 
Ok.
> > +                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.
> 
Ok.
> > +            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.
> 
Sorry.
> >              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.
> 
Ok.
> >          {
> >              /*
> > @@ -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.
> 
Ok.
> ~Andrew
> 
Thanks for your review, Andrew

_______________________________________________
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®.