Re: [V6 2/4] x86/xsaves: enable xsaves/xrstors/xsavec in xen

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/10/15 07:07, Shuai Ruan wrote:
> This patch uses xsaves/xrstors/xsavec instead of xsaveopt/xrstor
> to perform the xsave_area switching so that xen itself
> can benefit from them when available.
>
> For xsaves/xrstors/xsavec only use compact format. Add format conversion
> support when perform guest os migration.
>
> Also, pv guest will not support xsaves/xrstors.
>
> Signed-off-by: Shuai Ruan <shuai.ruan@linux.intel.com>
> ---
>  xen/arch/x86/domain.c        |   2 +
>  xen/arch/x86/domctl.c        |  35 ++++++-
>  xen/arch/x86/hvm/hvm.c       |  19 +++-
>  xen/arch/x86/i387.c          |   4 +
>  xen/arch/x86/traps.c         |   6 +-
>  xen/arch/x86/xstate.c        | 221 ++++++++++++++++++++++++++++++++++++-------
>  xen/include/asm-x86/xstate.h |   2 +
>  7 files changed, 244 insertions(+), 45 deletions(-)
>
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 7ca9b93..7b3d9f4 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1569,6 +1569,8 @@ static void __context_switch(void)
>              if ( xcr0 != get_xcr0() && !set_xcr0(xcr0) )
>                  BUG();
>          }
> +        if ( cpu_has_xsaves && is_hvm_vcpu(n) )

Because of the current implementation of PVH mode, the correct predicate
here is has_hvm_container(n)

> +            set_msr_xss(n->arch.hvm_vcpu.msr_xss);
>          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 f8a559c..9517741 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -897,9 +897,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) )
> +            {
> +                void *xsave_area;
> +
> +                xsave_area = xmalloc_bytes(size);
> +                if ( !xsave_area )
> +                {
> +                    ret = -ENOMEM;
> +                    vcpu_unpause(v);
> +                    goto vcpuextstate_out;
> +                }
> +
> +                expand_xsave_states(v, xsave_area,
> +                                    evc->size - 2 * sizeof(uint64_t));
> +
> +                if ( 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);
> @@ -955,8 +976,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 || cpu_has_xsavec) )

You can drop the inner set of brackets.

> +                    compress_xsave_states(v, _xsave_area,
> +                                          evc->size - 2 * sizeof(uint64_t));
> +                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 3fa2280..177cf5f 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -2157,8 +2157,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 || cpu_has_xsavec) )

And here

> +            expand_xsave_states(v, &ctxt->save_area,
> +                                size - offsetof(typeof(*ctxt), save_area));
> +        else
> +            memcpy(&ctxt->save_area, v->arch.xsave_area,
> +                   size - offsetof(struct hvm_hw_cpu_xsave, save_area));
>      }
>  
>      return 0;
> @@ -2257,9 +2261,14 @@ 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) )

And here.

With these 4 things fixed, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>