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

[Paul Durrant <xadimgnik@gmail.com>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 May 2020 16:37
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
> <ian.jackson@eu.citrix.com>; 'Julien Grall' <julien@xen.org>; 'Stefano Stabellini'
> <sstabellini@kernel.org>; 'Wei Liu' <wl@xen.org>; 'Volodymyr Babchuk' <Volodymyr_Babchuk@epam.com>;
> 'Roger Pau Monné' <roger.pau@citrix.com>
> Subject: Re: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
> 
> On 19.05.2020 17:32, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 19 May 2020 16:18
> >> To: paul@xen.org
> >> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> >> <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
> >> <ian.jackson@eu.citrix.com>; 'Julien Grall' <julien@xen.org>; 'Stefano Stabellini'
> >> <sstabellini@kernel.org>; 'Wei Liu' <wl@xen.org>; 'Volodymyr Babchuk' <Volodymyr_Babchuk@epam.com>;
> >> 'Roger Pau Monné' <roger.pau@citrix.com>
> >> 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@suse.com>
> >>>> Sent: 19 May 2020 15:24
> >>>>
> >>>> On 19.05.2020 16:04, Paul Durrant wrote:
> >>>>>> From: Jan Beulich <jbeulich@suse.com>
> >>>>>> 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.

Ok, I'll add some words to that effect.

  Paul

> 
> Jan