On 12/15/13 11:51, Andrew Cooper wrote:
> On 15/12/2013 00:29, Don Slutz wrote:
>>
>> I think I have corrected all coding errors (please check again). And 
>> done all requested changes.  I did add the reviewed by (not sure if I 
>> should since this changes a large part of the patch, but they are all 
>> what Jan said).
>>
>> I have unit tested it and it appears to work the same as the previous 
>> version (as expected).
>>
>> Here is the new version, also attached.
>>
>> From e0e8f5246ba492b153884cea93bfe753f1b0782e Mon Sep 17 00:00:00 2001
>> From: Don Slutz <dslutz@verizon.com>
>> Date: Tue, 12 Nov 2013 08:22:53 -0500
>> Subject: [PATCH v2 3/4] hvm_save_one: return correct data.
>>
>> It is possible that hvm_sr_handlers[typecode].save does not use all
>> the provided room.  In that case, using:
>>
>>    instance * hvm_sr_handlers[typecode].size
>>
>> does not select the correct instance.  Add code to search for the
>> correct instance.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> but this fairs no better at selecting the correct subset in the case 
> that less data than hvm_sr_handlers[typecode].size is written by 
> hvm_sr_handlers[typecode].save.
>
True, but the inverse is the case here; .save writes 'n' 'size' blocks.  
Form the loop above:

     if ( hvm_sr_handlers[typecode].kind == HVMSR_PER_VCPU )
         for_each_vcpu(d, v)
             sz += hvm_sr_handlers[typecode].size;
     else
         sz = hvm_sr_handlers[typecode].size;

so sz is in multiples of 'size'.  Normally sz == ctxt.cur.  With some 
offline vcpus it write fewer 'size' blocks.
> It always increments by 'size' bytes, and will only copy the data back 
> if the bytes under desc->instance happen to match the instance we are 
> looking for.
>
The only time it does not find one is for an offline vcpu.  Try out the 
unit test code in patch #1 on an unchanged xen.  It should not display 
anything.  Then offline a cpu in a domU (echo 0 > 
/sys/devices/system/cpu/cpu1/online).  And with 3 vcpus, it will report 
an error.

    -Don Slutz
> The only solution I can see is that for the per-vcpu records, the save 
> functions get refactored to take an instance ID, and only save their 
> specific instance.
>
> I shall have a go at this and see how invasive it is.
>
> ~Andrew
>
>> ---
>>  xen/common/hvm/save.c | 28 +++++++++++++++++++++-------
>>  1 file changed, 21 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/common/hvm/save.c b/xen/common/hvm/save.c
>> index de76ada..6aaea6f 100644
>> --- a/xen/common/hvm/save.c
>> +++ b/xen/common/hvm/save.c
>> @@ -112,13 +112,27 @@ int hvm_save_one(struct domain *d, uint16_t 
>> typecode, uint16_t instance,
>>                 d->domain_id, typecode);
>>          rv = -EFAULT;
>>      }
>> -    else if ( copy_to_guest(handle,
>> -                            ctxt.data
>> -                            + (instance * 
>> hvm_sr_handlers[typecode].size)
>> -                            + sizeof (struct hvm_save_descriptor),
>> -                            hvm_sr_handlers[typecode].size
>> -                            - sizeof (struct hvm_save_descriptor)) )
>> -        rv = -EFAULT;
>> +    else
>> +    {
>> +        uint32_t off;
>> +
>> +        rv = -EBADSLT;
>> +        for ( off = 0; off < ctxt.cur; off += 
>> hvm_sr_handlers[typecode].size )
>> +        {
>> +            const struct hvm_save_descriptor *desc = (void 
>> *)&ctxt.data[off];
>> +
>> +            if ( instance == desc->instance )
>> +            {
>> +                rv = 0;
>> +                if ( copy_to_guest(handle,
>> +                                   ctxt.data + off + sizeof(*desc),
>> + hvm_sr_handlers[typecode].size
>> +                                   - sizeof(*desc)) )
>> +                    rv = -EFAULT;
>> +                break;
>> +            }
>> +        }
>> +    }
>>
>>      xfree(ctxt.data);
>>      return rv;
>>
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>