Re: [PATCH RFC 1/3] xen/save: pass a size paramter to the HVM compat functions

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

On 15/10/15 10:59, Jan Beulich wrote:
>>>> On 15.10.15 at 11:49, <roger.pau@citrix.com> wrote:
>> El 15/10/15 a les 10.12, Jan Beulich ha escrit:
>>>>>> On 14.10.15 at 18:24, <roger.pau@citrix.com> wrote:
>>>> In order to cope with types having multiple compat versions pass a parameter
>>>> to the fixup function so we can identify which compat version Xen is dealing
>>>> with.
>>> Having peeked at patch 2, this won't help once another bit gets added
>>> to the tail of that structure. Also it doesn't seem logical that the
>>> previous compat handling got around without being passed the size
>>> explicitly. I.e. while perhaps more involved, I think the compat
>>> handling needs to be extended to allow for multiple versions.
>> Yes, patch #2 needs to be fixed by making fpu_initialised an uint32_t.
>> Adding versions would be the best option IMHO, but I don't think I have
>> enough time to do that myself right now.
> Understood. An I don't think we really need versioning here.
>
>>> Or, since we have this under control going forward, don't even declare
>>> all the various compat structures in the public header (and only ever
>>> add to the tail). Then staying with the passing of size probably makes
>>> sense, but the fixup function then should use offsetof() instead of
>>> sizeof() (and validate unused tail bits are zero, so they can be used for
>>> something later on).
>> Since we use hvm_load_entry_zeroextend I think we can assert that all
>> new tail bits are going to be 0, but I'm not sure I follow you regarding
>> the usage of offsetof instead of sizeof. What are we going to compare
>> with offsetof?
> Instead of introducing compat1 and compat2 structures, just add
> the new field to the existing structure, and use offsetof() to compare
> the passed in size with the that of the structure in its original state.
>
>> Also, we already have a compat version of hvm_hw_cpu that didn't add the
>> new field to the end.
> Right, but we can avoid making the same mistake again.

Please be aware that, in due course, I will be replacing all of this,
migration v2 style, as a prerequisite of getting cpuid policies
functioning correctly during migrate.

While we shouldn't paint ourselves into a corner, I wouldn't worry too
much about their longevity.

~Andrew