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

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

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

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)

Jan's new generic MSR save record will also write less than the maximum
if it can.

~Andrew

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

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

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