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:32, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 19 May 2020 16:18
>> To: paul@xxxxxxx
>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxxx; 'Paul Durrant' <pdurrant@xxxxxxxxxx>; 
>> 'Andrew Cooper'
>> <andrew.cooper3@xxxxxxxxxx>; 'George Dunlap' <george.dunlap@xxxxxxxxxx>; 
>> 'Ian Jackson'
>> <ian.jackson@xxxxxxxxxxxxx>; 'Julien Grall' <julien@xxxxxxx>; 'Stefano 
>> Stabellini'
>> <sstabellini@xxxxxxxxxx>; 'Wei Liu' <wl@xxxxxxx>; 'Volodymyr Babchuk' 
>> <Volodymyr_Babchuk@xxxxxxxx>;
>> 'Roger Pau Monné' <roger.pau@xxxxxxxxxx>
>> Subject: Re: [PATCH v3 1/5] xen/common: introduce a new framework for 
>> save/restore of 'domain' context
>>
>> 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.
>>
> 
> That's just how it is *now*. If a new code is defined that needs to
> be in the stream before one of the existing ones then we'll have to
> introduce a more elaborate scheme to deal with that at the time.
> Using the save code as the array index and iterating in that order
> is purely a convenience, and the load side does not depend on
> entries being in save code order.

Could then you make the comment indicate so? This will allow people
wanting to modify this do so more easily, without much digging in
code or mail history.

Jan