Re: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.05.2020 17:10, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 19 May 2020 15:24
>>
>> On 19.05.2020 16:04, Paul Durrant wrote:
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: 19 May 2020 14:04
>>>>
>>>> On 14.05.2020 12:44, Paul Durrant wrote:
>>>>> +/*
>>>>> + * Register save and restore handlers. Save handlers will be invoked
>>>>> + * in order of DOMAIN_SAVE_CODE().
>>>>> + */
>>>>> +#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load)            \
>>>>> +    static int __init __domain_register_##_x##_save_restore(void) \
>>>>> +    {                                                             \
>>>>> +        domain_register_save_type(                                \
>>>>> +            DOMAIN_SAVE_CODE(_x),                                 \
>>>>> +            #_x,                                                  \
>>>>> +            &(_save),                                             \
>>>>> +            &(_load));                                            \
>>>>> +                                                                  \
>>>>> +        return 0;                                                 \
>>>>> +    }                                                             \
>>>>> +    __initcall(__domain_register_##_x##_save_restore);
>>>>
>>>> I'm puzzled by part of the comment: Invoking by save code looks
>>>> reasonable for the saving side (albeit END doesn't match this rule
>>>> afaics), but is this going to be good enough for the consuming side?
>>>
>>> No, this only relates to the save side which is why the comment
>>> says 'Save handlers'. I do note that it would be more consistent
>>> to use 'load' rather than 'restore' here though.
>>>
>>>> There may be dependencies between types, and with fixed ordering
>>>> there may be no way to insert a depended upon type ahead of an
>>>> already defined one (at least as long as the codes are meant to be
>>>> stable).
>>>>
>>>
>>> The ordering of load handlers is determined by the stream. I'll
>>> add a sentence saying that.
>>
>> I.e. the consumer of the "get" interface (and producer of the stream)
>> is supposed to take apart the output it gets, bring records into
>> suitable order (which implies it knows of all the records, and which
>> hence means this code may need updating in cases where I'd expect
>> only the hypervisor needs), and only then issue to the stream?
> 
> The intention is that the stream is always in a suitable order so the
> load side does not have to do any re-ordering.

I understood this to be the intention, but what I continue to not
understand is where / how the save side orders it suitably. "Save
handlers will be invoked in order of DOMAIN_SAVE_CODE()" does not
allow for any ordering, unless at the time of the introduction of
a particular code you already know what others it may depend on
in the future, reserving appropriate codes.

And as said - END also doesn't look to fit this comment.

Jan