Re: [Xen-devel] [PATCH 5/6] x86/domctl: Fix migration of guests which are not using xsave

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.16 at 11:51, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -1158,7 +1158,15 @@ long arch_do_domctl(
>                  goto vcpuextstate_out;
>              }
>  
> -            if ( evc->size <= PV_XSAVE_SIZE(_xcr0_accum) )
> +            if ( evc->size == PV_XSAVE_HDR_SIZE )
> +                ; /* Nothing to restore. */
> +            else if ( evc->size < PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE )
> +                ret = -EINVAL; /* Can't be legitimate data. */
> +            else if ( xsave_area_compressed(_xsave_area) )
> +                ret = -EOPNOTSUPP; /* Don't support compressed data. */
> +            else if ( evc->size != PV_XSAVE_SIZE(_xcr0_accum) )
> +                ret = -EINVAL; /* Not legitimate data. */

Can't this be moved ahead of the xsave_area_compressed() check,
eliminating (as redundant) the check that's there right now?

In any event the tightening to != you do here supports my desire to
not do any relaxation of the size check in patch 2. Or alternatively
you would want to consider restoring the behavior from prior to
the xsaves change, where the <= which you now remove was still
okay.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1345,6 +1345,23 @@ static int hvm_load_cpu_xsave_states(struct domain *d, 
> hvm_domain_context_t *h)
>          printk(XENLOG_G_WARNING
>                 "HVM%d.%u restore mismatch: xsave length %#x > %#x\n",
>                 d->domain_id, vcpuid, desc->length, size);
> +        /* Rewind desc->length to ignore the extraneous zeros. */
> +        desc->length = size;

This is slightly ugly, as it will prevent eventually constifying dest (which
it really should have been from the beginning).

> --- a/xen/include/asm-x86/xstate.h
> +++ b/xen/include/asm-x86/xstate.h
> @@ -131,4 +131,10 @@ static inline bool_t xstate_all(const struct vcpu *v)
>             (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
>  }
>  
> +static inline bool __nonnull(1)
> +    xsave_area_compressed(const struct xsave_struct *xsave_area)

This is certainly odd indentation.

Jan


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