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 14:04
> To: Paul Durrant <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 14.05.2020 12:44, Paul Durrant wrote:
> > --- /dev/null
> > +++ b/xen/common/save.c
> > @@ -0,0 +1,313 @@
> > +/*
> > + * save.c: Save and restore PV guest state common to all domain types.
> > + *
> > + * 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/>.
> > + */
> > +
> > +#include <xen/compile.h>
> > +#include <xen/save.h>
> > +
> > +struct domain_context {
> > +    struct domain *domain;
> > +    const char *name; /* for logging purposes */
> 
> With this comment, why ...
> 
> > +    struct domain_save_descriptor desc;
> > +    size_t len; /* for internal accounting */
> > +    union {
> > +        struct domain_save_ops *save;
> > +        struct domain_load_ops *load;
> > +    } ops;
> > +    void *priv;
> > +    bool log;
> 
> ... this separate field? Couldn't "no logging" simply be expressed by
> name being NULL?
> 

Ok. The log bool predated the name pointer so, yes these could be combined.

> > +int domain_save_end(struct domain_context *c)
> > +{
> > +    struct domain *d = c->domain;
> > +    uint8_t pad[DOMAIN_SAVE_ALIGN] = {};
> 
> Preferably moved into the more narrow scope, and probably wants making
> "static const".
> 

Ok.

> > +    size_t len = ROUNDUP(c->len, DOMAIN_SAVE_ALIGN) - c->len; /* padding */
> > +    int rc;
> > +
> > +    if ( len )
> > +    {
> > +        rc = domain_save_data(c, pad, len);
> > +
> > +        if ( rc )
> > +            return rc;
> > +    }
> > +    ASSERT(IS_ALIGNED(c->len, DOMAIN_SAVE_ALIGN));
> 
> While you mention the auto-padding as a change in v3, it's not really
> clear to me why it was introduced. Could you add half a sentence to
> the description clarifying the motivation?
> 

Julien favoured it and it does seem like a good idea as it avoids the need to 
put explicit trailing 'pad' fields in entries. I'll add to the commit comment 
to explain.

> > +int domain_save(struct domain *d, struct domain_save_ops *ops, void *priv,
> > +                bool dry_run)
> > +{
> > +    struct domain_context c = {
> > +        .domain = d,
> > +        .ops.save = ops,
> > +        .priv = priv,
> > +        .log = !dry_run,
> > +    };
> > +    static struct domain_save_header h = {
> 
> const?
> 

Yes, it can be.

> > +        .magic = DOMAIN_SAVE_MAGIC,
> > +        .xen_major = XEN_VERSION,
> > +        .xen_minor = XEN_SUBVERSION,
> > +        .version = DOMAIN_SAVE_VERSION,
> > +    };
> > +    struct domain_save_end e = {};
> 
> const? (static would likely be quite pointless here)
> 

Ok.

> > +int domain_load(struct domain *d, struct domain_load_ops *ops, void *priv)
> > +{
> > +    struct domain_context c = {
> > +        .domain = d,
> > +        .ops.load = ops,
> > +        .priv = priv,
> > +        .log = true,
> > +    };
> > +    unsigned int instance;
> > +    struct domain_save_header h;
> > +    int rc;
> > +
> > +    ASSERT(d != current->domain);
> > +
> > +    rc = c.ops.load->read(c.priv, &c.desc, sizeof(c.desc));
> > +    if ( rc )
> > +        return rc;
> > +
> > +    rc = DOMAIN_LOAD_ENTRY(HEADER, &c, &instance, &h, sizeof(h));
> > +    if ( rc )
> > +        return rc;
> > +
> > +    if ( instance || h.magic != DOMAIN_SAVE_MAGIC ||
> > +         h.version != DOMAIN_SAVE_VERSION )
> > +        return -EINVAL;
> > +
> > +    domain_pause(d);
> > +
> > +    for (;;)
> > +    {
> > +        unsigned int i;
> > +        domain_load_handler load;
> > +
> > +        rc = c.ops.load->read(c.priv, &c.desc, sizeof(c.desc));
> > +        if ( rc )
> > +            return rc;
> > +
> > +        rc = -EINVAL;
> > +
> > +        if ( c.desc.typecode == DOMAIN_SAVE_CODE(END) )
> > +        {
> > +            struct domain_save_end e;
> > +
> > +            rc = DOMAIN_LOAD_ENTRY(END, &c, &instance, NULL, sizeof(e));
> 
> Without using &e here I don't see how you can get away without an
> "unused variable" warning.

Hmm. I definitely don't get a warning, but yes this ought to be changed.

> 
> > --- /dev/null
> > +++ b/xen/include/public/save.h
> > @@ -0,0 +1,80 @@
> > +/*
> > + * save.h
> > + *
> > + * Structure definitions for common PV/HVM domain state that is held by
> > + * Xen and must be saved along with the domain's memory.
> > + *
> > + * Copyright Amazon.com Inc. or its affiliates.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a 
> > copy
> > + * of this software and associated documentation files (the "Software"), to
> > + * deal in the Software without restriction, including without limitation 
> > the
> > + * rights to use, copy, modify, merge, publish, distribute, sublicense, 
> > and/or
> > + * sell copies of the Software, and to permit persons to whom the Software 
> > is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included 
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL 
> > THE
> > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef XEN_PUBLIC_SAVE_H
> > +#define XEN_PUBLIC_SAVE_H
> > +
> > +#include "xen.h"
> > +
> > +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> 
> Move #include down below here?
> 

Sure.

> > +/* Entry data is preceded by a descriptor */
> > +struct domain_save_descriptor {
> > +    uint16_t typecode;
> > +
> > +    /*
> > +     * Instance number of the entry (since there may by multiple of some
> > +     * types of entry).
> 
> With a German bias I'm inclined to ask: "entries"?
> 

Not sure. Still understandable so I'm happy to change. Also s/by/be.

> > +     */
> > +    uint16_t instance;
> > +
> > +    /* Entry length not including this descriptor */
> > +    uint32_t length;
> > +};
> > +
> > +/*
> > + * Each entry has a type associated with it. DECLARE_DOMAIN_SAVE_TYPE
> > + * binds these things together.
> > + */
> > +#define DECLARE_DOMAIN_SAVE_TYPE(_x, _code, _type) \
> > +    struct DOMAIN_SAVE_TYPE_##_x { char c[_code]; _type t; };
> 
> Perhaps point out in the comment that this type is not meant to
> have instantiations?

Ok.

> 
> > +#define DOMAIN_SAVE_CODE(_x) \
> > +    (sizeof(((struct DOMAIN_SAVE_TYPE_##_x *)(0))->c))
> > +#define DOMAIN_SAVE_TYPE(_x) \
> > +    typeof(((struct DOMAIN_SAVE_TYPE_##_x *)(0))->t)
> 
> Feels like I already mentioned the oddity of having parentheses
> around a single token that's not a macro parameter name.
> 

Ok. Missed that.

> > --- /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. Also, I prefer to keep the parentheses for arguments.

> 
> > +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.

> > +/*
> > + * 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.

> > +/*
> > + * Entry points:
> > + *
> > + * ops:     These are callback functions provided by the caller that will
> > + *          be used to write to (in the save case) or read from (in the
> > + *          load case) the context buffer. See above for more detail.
> > + * priv:    This is a pointer that will be passed to the copy function to
> > + *          allow it to identify the context buffer and the current state
> > + *          of the save or load operation.
> > + * dry_run: If this is set then the caller of domain_save() is only trying
> > + *          to acquire the total size of the data, not the data itself.
> > + *          In this case the caller may supply different ops to avoid doing
> > + *          unnecessary work.
> > + */
> > +int domain_save(struct domain *d, struct domain_save_ops *ops, void *priv,
> > +                bool dry_run);
> > +int domain_load(struct domain *d, struct domain_load_ops *ops, void *priv);
> 
> I guess ops want to be pointer to const in both cases?
> 

Yes, it can be.

  Paul

> Jan