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

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

On 12/09/16 13:14, Jan Beulich wrote:
>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> 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?

No.  That doesn't catch the case where _xcr0_accum is 0, which will
cause the xsave_area_compressed(_xsave_area) check to wander off the end
of the buffer.

>
> In any event the tightening to != you do here supports my desire to
> not do any relaxation of the size check in patch 2.

The two cases are different, and should not be conflated.

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

I looked back through the history and couldn't find any justification
for the check being <= as opposed to being more strict.  We absolutely
don't want to be in the case where we only restore part of an xstate
component.

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

Hmm yes.  I hadn't considered that it was actually mutating the hvm
domain context buffer.  I will switch to a shadow variable instead.

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

It is where my editor naturally indents to.  It would appear that emacs
is mistaking the __nonnull(1) as the function name, and presuming that
the following line is K&R style parameters.

It is certainly awkward that the attributes need to be that way around.

~Andrew

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