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 14:46, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 12/09/16 13:14, Jan Beulich wrote:
>>>>> 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?
> 
> 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.

Oh, indeed.

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

Indeed I don't think there was ever any justification provided, it
just happened to be that way (perhaps having in mind that the
FXSAVE layout had unused space at the end).

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

With these two small things taken care of,
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>


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