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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.10.15 at 08:07, <shuai.ruan@xxxxxxxxxxxxxxx> wrote:
> --- 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)) )

Here you use size, which is fine, but why do you use evc->size
three lines up from here?

> +                     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)) )

I also think code readability would benefit from folding the two
copy_to_guest_offset()s, which differ in only the pointer used.

> @@ -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) )
> +                    compress_xsave_states(v, _xsave_area,
> +                                          evc->size - 2 * sizeof(uint64_t));
> +                else
> +                    memcpy(v->arch.xsave_area, (void *)_xsave_area,

Stray cast got added here.

> @@ -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) )
> +        compress_xsave_states(v, &ctxt->save_area,
> +                              min(desc->length, size) -
> +                              offsetof(struct hvm_hw_cpu_xsave,save_area));
> +    else
> +        memcpy(v->arch.xsave_area, &ctxt->save_area,
> +               min(desc->length, size) - offsetof(struct hvm_hw_cpu_xsave,
> +               save_area));

Each time I look at these two hunks I wonder whether the condition
and memcpy() wouldn't better move into the new called functions.

> +void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
> +{
> +    const struct xsave_struct *xsave = v->arch.xsave_area;
> +    struct xsave_struct *dest_xsave = (struct xsave_struct *)dest;

Pointless cast.

> +void compress_xsave_states(struct vcpu *v, const void *src, unsigned int 
> size)
> +{
> +    struct xsave_struct *xsave = v->arch.xsave_area;
> +    struct xsave_struct *src_xsave = (struct xsave_struct *)src;

Here the cast is even bogus, as it removes const-ness.

> +    u64 xstate_bv = src_xsave->xsave_hdr.xstate_bv;
> +    u64 valid;
> +
> +    ASSERT(!xsave_area_compressed((struct xsave_struct *)src));

Pointless cast (to same type).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel