Re: [BUGFIX][PATCH 3/4] hvm_save_one: return correct data.

[Don Slutz <dslutz@verizon.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9401 bytes --]

On 12/15/13 13:11, Andrew Cooper wrote:
> On 15/12/2013 17:42, Don Slutz wrote:
>> On 12/15/13 12:22, Andrew Cooper wrote:
>>> On 15/12/2013 17:19, Don Slutz wrote:
>>>> 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
>>>
>>> Ah - so there are actually two problems.  I see now the one you are 
>>> trying to solve, and would agree that your code does solve it.
>>>
>>> However, some of the save handlers are themselves variable length, 
>>> and will write records shorter than hvm_sr_handlers[typecode].size 
>>> if they can get away with doing so.  In this case, the new logic 
>>> still wont get the correct instance.
>>>
>> Not sure which one(s) you are referring to.
>>
>> From the full dump:
>>
>> xen-hvmctx 1| grep -i entry
>> Entry 0: type 1 instance 0, length 24
>> Entry 1: type 2 instance 0, length 1024
>> Entry 2: type 2 instance 2, length 1024
>> Entry 3: type 2 instance 3, length 1024
>> Entry 4: type 2 instance 4, length 1024
>> Entry 5: type 2 instance 5, length 1024
>> Entry 6: type 2 instance 6, length 1024
>> Entry 7: type 2 instance 7, length 1024
>> Entry 8: type 3 instance 0, length 8
>> Entry 9: type 3 instance 1, length 8
>> Entry 10: type 4 instance 0, length 400
>> Entry 11: type 5 instance 0, length 24
>> Entry 12: type 5 instance 1, length 24
>> Entry 13: type 5 instance 2, length 24
>> Entry 14: type 5 instance 3, length 24
>> Entry 15: type 5 instance 4, length 24
>> Entry 16: type 5 instance 5, length 24
>> Entry 17: type 5 instance 6, length 24
>> Entry 18: type 5 instance 7, length 24
>> Entry 19: type 6 instance 0, length 1024
>> Entry 20: type 6 instance 1, length 1024
>> Entry 21: type 6 instance 2, length 1024
>> Entry 22: type 6 instance 3, length 1024
>> Entry 23: type 6 instance 4, length 1024
>> Entry 24: type 6 instance 5, length 1024
>> Entry 25: type 6 instance 6, length 1024
>> Entry 26: type 6 instance 7, length 1024
>> Entry 27: type 7 instance 0, length 16
>> Entry 28: type 8 instance 0, length 8
>> Entry 29: type 9 instance 0, length 8
>> Entry 30: type 10 instance 0, length 56
>> Entry 31: type 11 instance 0, length 16
>> Entry 32: type 12 instance 0, length 1048
>> Entry 33: type 13 instance 0, length 8
>> Entry 34: type 14 instance 0, length 240
>> Entry 35: type 14 instance 1, length 240
>> Entry 36: type 14 instance 2, length 240
>> Entry 37: type 14 instance 3, length 240
>> Entry 38: type 14 instance 4, length 240
>> Entry 39: type 14 instance 5, length 240
>> Entry 40: type 14 instance 6, length 240
>> Entry 41: type 14 instance 7, length 240
>> Entry 42: type 16 instance 0, length 856
>> Entry 43: type 16 instance 1, length 856
>> Entry 44: type 16 instance 2, length 856
>> Entry 45: type 16 instance 3, length 856
>> Entry 46: type 16 instance 4, length 856
>> Entry 47: type 16 instance 5, length 856
>> Entry 48: type 16 instance 6, length 856
>> Entry 49: type 16 instance 7, length 856
>> Entry 50: type 18 instance 0, length 24
>> Entry 51: type 18 instance 1, length 24
>> Entry 52: type 18 instance 2, length 24
>> Entry 53: type 18 instance 3, length 24
>> Entry 54: type 18 instance 4, length 24
>> Entry 55: type 18 instance 5, length 24
>> Entry 56: type 18 instance 6, length 24
>> Entry 57: type 18 instance 7, length 24
>> Entry 58: type 19 instance 0, length 8
>> Entry 59: type 19 instance 1, length 8
>> Entry 60: type 19 instance 2, length 8
>> Entry 61: type 19 instance 3, length 8
>> Entry 62: type 19 instance 4, length 8
>> Entry 63: type 19 instance 5, length 8
>> Entry 64: type 19 instance 6, length 8
>> Entry 65: type 19 instance 7, length 8
>> Entry 66: type 0 instance 0, length 0
>>
>> All typecode's appear to save the same amount per instance.
>>
>> Most use hvm_save_entry:
>>
>> ...
>>         _hvm_write_entry((_h), (_src), HVM_SAVE_LENGTH(_x)); \
>>
>> and
>>
>> /* Syntactic sugar around that function: specify the max number of
>>  * saves, and this calculates the size of buffer needed */
>> #define HVM_REGISTER_SAVE_RESTORE(_x, _save, _load, _num, 
>> _k)             \
>> static int __init 
>> __hvm_register_##_x##_save_and_restore(void)            \
>> { \
>> hvm_register_savevm(HVM_SAVE_CODE(_x), \
>> #_x,                                              \
>> &_save,                                           \
>> &_load,                                           \
>>                         (_num) * 
>> (HVM_SAVE_LENGTH(_x)                     \
>>                                   + sizeof (struct 
>> hvm_save_descriptor)), \
>> _k);                                              \
>>     return 
>> 0;                                                             \
>> } \
>>
>> I do not find any that call on _hvm_write_entry directly.
>>
>> The only special one I found: CPU_XSAVE_CODE
>>
>> Still "writes" a full sized entry:
>>
>>         if ( _hvm_init_entry(h, CPU_XSAVE_CODE, v->vcpu_id, 
>> HVM_CPU_XSAVE_SIZE) )
>>             return 1;
>>         ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
>>         h->cur += HVM_CPU_XSAVE_SIZE;
>>         memset(ctxt, 0, HVM_CPU_XSAVE_SIZE);
>>
>> It then modifies the zeros conditionaly.
>>
>>         if ( v->fpu_initialised )
>>             memcpy(&ctxt->save_area,
>>                 v->arch.xsave_area, xsave_cntxt_size);
>>
>> #define HVM_CPU_XSAVE_SIZE  (3 * sizeof(uint64_t) + xsave_cntxt_size)
>>
>> is part of this.
>>
>> /* We need variable length data chunk for xsave area, hence customized
>>  * declaration other than HVM_REGISTER_SAVE_RESTORE.
>>  */
>> static int __init __hvm_register_CPU_XSAVE_save_and_restore(void)
>> {
>>     hvm_register_savevm(CPU_XSAVE_CODE,
>>                         "CPU_XSAVE",
>>                         hvm_save_cpu_xsave_states,
>>                         hvm_load_cpu_xsave_states,
>>                         HVM_CPU_XSAVE_SIZE + sizeof (struct 
>> hvm_save_descriptor),
>>                         HVMSR_PER_VCPU);
>>     return 0;
>> }
>> __initcall(__hvm_register_CPU_XSAVE_save_and_restore);
>>
>> is the final part of this one.  So I do not find any code that does 
>> what you are wondering about.
>>
>>    -Don
>>
>
> HVM_CPU_XSAVE_SIZE() changes depending on which xsave features have 
> ever been enabled by a vcpu (size is proportional to the contents of 
> v->arch.xcr0_accum).  It is not guaranteed to be the same for each 
> vcpu in a domain, (although almost certainly will be the same for any 
> recognisable OS)
>
Ah, I see.

Well, hvm_save_one, hvm_save_size, and hvm_save all expect that 
hvm_sr_handlers[typecode].size has the max size.  I do not see that 
being true for XSAVE.
> Jan's new generic MSR save record will also write less than the 
> maximum if it can.
>
This looks to be Jan's patch:

http://lists.xen.org/archives/html/xen-devel/2013-12/msg02061.html

Does look to set hvm_sr_handlers[typecode].size to the max size.

And it looks like the code I did in patch #4 would actually fix this 
issue.  Since it now uses the length stored in the save descriptor to 
find each instance.

Jan has some questions about patch #4; so what to do about it is still 
pending.

Clearly I can merge #3 and #4 into 1 patch.

    -Don Slutz
> ~Andrew




[-- Attachment #1.2: Type: text/html, Size: 19622 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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