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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 19 May 2020 15:24
> 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 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:
> >>> --- /dev/null
> >>> +++ b/xen/include/xen/save.h
> >>> @@ -0,0 +1,165 @@
> >>> +/*
> >>> + * save.h: support routines for save/restore
> >>> + *
> >>> + * Copyright Amazon.com Inc. or its affiliates.
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or modify 
> >>> it
> >>> + * under the terms and conditions of the GNU General Public License,
> >>> + * version 2, as published by the Free Software Foundation.
> >>> + *
> >>> + * This program is distributed in the hope it will be useful, but WITHOUT
> >>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
> >>> for
> >>> + * more details.
> >>> + *
> >>> + * You should have received a copy of the GNU General Public License 
> >>> along with
> >>> + * this program; If not, see <http://www.gnu.org/licenses/>.
> >>> + */
> >>> +
> >>> +#ifndef XEN_SAVE_H
> >>> +#define XEN_SAVE_H
> >>> +
> >>> +#include <xen/init.h>
> >>> +#include <xen/sched.h>
> >>> +#include <xen/types.h>
> >>> +
> >>> +#include <public/save.h>
> >>> +
> >>> +struct domain_context;
> >>> +
> >>> +int domain_save_begin(struct domain_context *c, unsigned int typecode,
> >>> +                      const char *name, unsigned int instance);
> >>> +
> >>> +#define DOMAIN_SAVE_BEGIN(_x, _c, _instance) \
> >>> +    domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_instance))
> >>
> >> As per prior conversation I would have expected no leading underscores
> >> here anymore, and no parenthesization of what is still _c. More of
> >> these further down.
> >
> > What's wrong with leading underscores in macro arguments? They can't
> > pollute any namespace.
> 
> They can still hide file scope variables legitimately named
> this way (which may get accessed by a macro without being a
> macro argument). The wording of the standard is quite clear:
> "All identifiers that begin with an underscore are always
> reserved for use as identifiers with file scope in both the
> ordinary and tag name spaces."
> 

Ok.

> > Also, I prefer to keep the parentheses for arguments.
> 
> More code churn then once we hopefully standardize of the less
> obfuscating variant without.
> 

If we standardize that way, so be it.

> >>> +static inline int domain_load_entry(struct domain_context *c,
> >>> +                                    unsigned int typecode, const char 
> >>> *name,
> >>> +                                    unsigned int *instance, void *dst,
> >>> +                                    size_t len)
> >>> +{
> >>> +    int rc;
> >>> +
> >>> +    rc = domain_load_begin(c, typecode, name, instance);
> >>
> >> For some reason I've spotted this only here: Why is instance a pointer
> >> parameter of the function, when typecode is a value? Both live next to
> >> one another in struct domain_save_descriptor, and hence instance could
> >> be retrieved at the same time as typecode.
> >>
> >
> > Because the typecode is known a priori. It has to be known for the
> > correct handler to be invoked. It is only provided here for
> > verification purposes. I could have provided the instance as an
> > argument to the load handler but I prefer making the interactions
> > for save and load more symmetric.
> 
> Hmm, I don't see any symmetry violation in the alternative model,
> but anyway.
> 
> >>> +/*
> >>> + * 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.

  Paul