[v2,1/5] xen/common: introduce a new framework for save/restore of 'domain' context

Commit Message

Paul Durrant April 7, 2020, 5:38 p.m. UTC

To allow enlightened HVM guests (i.e. those that have PV drivers) to be
migrated without their co-operation it will be necessary to transfer 'PV'
state such as event channel state, grant entry state, etc.

Currently there is a framework (entered via the hvm_save/load() functions)
that allows a domain's 'HVM' (architectural) state to be transferred but
'PV' state is also common with pure PV guests and so this framework is not
really suitable.

This patch adds the new public header and low level implementation of a new
common framework, entered via the domain_save/load() functions. Subsequent
patches will introduce other parts of the framework, and code that will
make use of it within the current version of the libxc migration stream.

This patch also marks the HVM-only framework as deprecated in favour of the
new framework.

Signed-off-by: Paul Durrant <[e-mail]>
---
Cc: Andrew Cooper <[e-mail]>
Cc: George Dunlap <[e-mail]>
Cc: Ian Jackson <[e-mail]>
Cc: Jan Beulich <[e-mail]>
Cc: Julien Grall <[e-mail]>
Cc: Stefano Stabellini <[e-mail]>
Cc: Wei Liu <[e-mail]>
Cc: Volodymyr Babchuk <[e-mail]>
Cc: "Roger Pau Monné" <[e-mail]>

v2:
 - Allow multi-stage save/load to avoid the need to double-buffer
 - Get rid of the masks and add an 'ignore' flag instead
 - Create copy function union to preserve const save buffer
 - Deprecate HVM-only framework
---
 xen/common/Makefile                    |   1 +
 xen/common/save.c                      | 329 +++++++++++++++++++++++++
 xen/include/public/arch-arm/hvm/save.h |   5 +
 xen/include/public/arch-x86/hvm/save.h |   5 +
 xen/include/public/save.h              |  84 +++++++
 xen/include/xen/save.h                 | 152 ++++++++++++
 6 files changed, 576 insertions(+)
 create mode 100644 xen/common/save.c
 create mode 100644 xen/include/public/save.h
 create mode 100644 xen/include/xen/save.h

Comments

Julien Grall April 20, 2020, 5:20 p.m. UTC | #1

Hi Paul,

On 07/04/2020 18:38, Paul Durrant wrote:
> To allow enlightened HVM guests (i.e. those that have PV drivers) to be
> migrated without their co-operation it will be necessary to transfer 'PV'
> state such as event channel state, grant entry state, etc.
> 
> Currently there is a framework (entered via the hvm_save/load() functions)
> that allows a domain's 'HVM' (architectural) state to be transferred but
> 'PV' state is also common with pure PV guests and so this framework is not
> really suitable.
> 
> This patch adds the new public header and low level implementation of a new
> common framework, entered via the domain_save/load() functions. Subsequent
> patches will introduce other parts of the framework, and code that will
> make use of it within the current version of the libxc migration stream.
> 
> This patch also marks the HVM-only framework as deprecated in favour of the
> new framework.
> 
> Signed-off-by: Paul Durrant <[e-mail]>
> ---
> Cc: Andrew Cooper <[e-mail]>
> Cc: George Dunlap <[e-mail]>
> Cc: Ian Jackson <[e-mail]>
> Cc: Jan Beulich <[e-mail]>
> Cc: Julien Grall <[e-mail]>
> Cc: Stefano Stabellini <[e-mail]>
> Cc: Wei Liu <[e-mail]>
> Cc: Volodymyr Babchuk <[e-mail]>
> Cc: "Roger Pau Monné" <[e-mail]>
> 
> v2:
>   - Allow multi-stage save/load to avoid the need to double-buffer
>   - Get rid of the masks and add an 'ignore' flag instead
>   - Create copy function union to preserve const save buffer
>   - Deprecate HVM-only framework
> ---
>   xen/common/Makefile                    |   1 +
>   xen/common/save.c                      | 329 +++++++++++++++++++++++++
>   xen/include/public/arch-arm/hvm/save.h |   5 +
>   xen/include/public/arch-x86/hvm/save.h |   5 +
>   xen/include/public/save.h              |  84 +++++++
>   xen/include/xen/save.h                 | 152 ++++++++++++
>   6 files changed, 576 insertions(+)
>   create mode 100644 xen/common/save.c
>   create mode 100644 xen/include/public/save.h
>   create mode 100644 xen/include/xen/save.h
> 
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index e8cde65370..90553ba5d7 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -37,6 +37,7 @@ obj-y += radix-tree.o
>   obj-y += rbtree.o
>   obj-y += rcupdate.o
>   obj-y += rwlock.o
> +obj-y += save.o
>   obj-y += shutdown.o
>   obj-y += softirq.o
>   obj-y += sort.o
> diff --git a/xen/common/save.c b/xen/common/save.c
> new file mode 100644
> index 0000000000..6cdac3785b
> --- /dev/null
> +++ b/xen/common/save.c
> @@ -0,0 +1,329 @@
> +/*
> + * 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/save.h>
> +
> +union domain_copy_entry {
> +    domain_write_entry write;
> +    domain_read_entry read;
> +};
> +
> +struct domain_context {
> +    bool log;
> +    struct domain_save_descriptor desc;
> +    size_t data_len;

What is data_len?

> +    union domain_copy_entry copy;
> +    void *priv;
> +};
> +
> +static struct {
> +    const char *name;
> +    bool per_vcpu;
> +    domain_save_handler save;
> +    domain_load_handler load;
> +} handlers[DOMAIN_SAVE_CODE_MAX + 1];
> +
> +void __init domain_register_save_type(unsigned int tc, const char *name,
> +                                      bool per_vcpu,
> +                                      domain_save_handler save,
> +                                      domain_load_handler load)
> +{
> +    BUG_ON(tc > ARRAY_SIZE(handlers));
> +
> +    ASSERT(!handlers[tc].save);
> +    ASSERT(!handlers[tc].load);
> +
> +    handlers[tc].name = name;
> +    handlers[tc].per_vcpu = per_vcpu;
> +    handlers[tc].save = save;
> +    handlers[tc].load = load;
> +}
> +
> +int domain_save_begin(struct domain_context *c, unsigned int tc,
> +                      const char *name, const struct vcpu *v, size_t len)
> +{
> +    int rc;
> +
> +    if ( c->log )
> +        gdprintk(XENLOG_INFO, "%pv save: %s (%lu)\n", v, name,
> +                 (unsigned long)len);

How about using %zu rather than a cast here?

> +
> +    BUG_ON(tc != c->desc.typecode);
> +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);

I can't find any answer on my question about this part. I understand the 
code would be buggy if this happen, but is it warrant to crash the host? 
Couldn't you just return an error and continue to run?

> +
> +    ASSERT(!c->data_len);
> +    c->data_len = c->desc.length = len;
> +
> +    rc = c->copy.write(c->priv, &c->desc, sizeof(c->desc));
> +    if ( rc )
> +        return rc;
> +
> +    c->desc.length = 0;

This is confusing, why would you need to reset c->desc.length but not 
the rest of the fields?

> +
> +    return 0;
> +}
> +
> +int domain_save_data(struct domain_context *c, const void *src, size_t len)
> +{
> +    if ( c->desc.length + len > c->data_len )
> +        return -ENOSPC;
> +
> +    c->desc.length += len;
> +
> +    return c->copy.write(c->priv, src, len);
> +}
> +
> +int domain_save_end(struct domain_context *c)
> +{
> +    /*
> +     * If desc.length does not match the length specified in
> +     * domain_save_begin(), there should have been more data.
> +     */
> +    if ( c->desc.length != c->data_len )

This suggests you know in advance the size of the record. I have seen 
some cases where we don't know the size in advance. A good example if 
when saving grants. You know the maximum of grant used by the guest but 
you don't know yet the number of grants used. You might need to walk the 
"list" twice or allocate a temporary buffer. None of them are ideal.

Another example is when saving memory, we may want to compact page 
informations to save space.

This problem is going to be more relevant for LiveUpdate where we need 
to be able to create the stream in a very short amount of time. 
Allocating any temporary buffer and/or walking the list twice is going 
to kill the performance.

I would suggest to consider a different approach where you update the 
record length at the end.

FWIW, this below the current approach for the LU stream (IIRC David sent 
a prototype recently). A record is opened using lu_stream_open_record(), 
you then have two way to add data:
     - lu_stream_append() -> This takes a buffer and write to the stream.
     - lu_stream_reserve() -> Pre-allocate space in the stream and 
return a pointer to the beginning of the reserved region.
     - lu_stream_end_reservation() -> Takes the actual size in parameter 
and update the stream size.
     - lu_stream_close_record() -> Update the header with the total 
length and update the position in the stream.

> +        return -EIO;

I noticed that all the pasding check have been dropped. I still think we 
need implicit padding to harden the code.

> +
> +    c->data_len = 0;
> +
> +    return 0;
> +}
> +
> +int domain_save(struct domain *d, domain_write_entry write, void *priv,
> +                bool dry_run)
> +{
> +    struct domain_context c = {
> +        .copy.write = write,
> +        .priv = priv,
> +        .log = !dry_run,
> +    };
> +    struct domain_save_header h = {
> +        .magic = DOMAIN_SAVE_MAGIC,
> +        .version = DOMAIN_SAVE_VERSION,
> +    };
> +    struct domain_save_header e;
> +    unsigned int i;
> +    int rc;
> +
> +    ASSERT(d != current->domain);
> +
> +    if ( d->is_dying )

I can't find an answer to my question about d->is_dying. What if the 
guest die afterwards? What does protect us against domain_kill()?

[...]

> +int domain_load(struct domain *d, domain_read_entry read, void *priv)
> +{
> +    struct domain_context c = {
> +        .copy.read = read,
> +        .priv = priv,
> +        .log = true,
> +    };
> +    struct domain_save_header h;
> +    int rc;
> +
> +    ASSERT(d != current->domain);
> +
> +    if ( d->is_dying )
> +        return -EINVAL;

Same here.


> diff --git a/xen/include/public/save.h b/xen/include/public/save.h
> new file mode 100644
> index 0000000000..7e5f8752bd
> --- /dev/null
> +++ b/xen/include/public/save.h
> @@ -0,0 +1,84 @@
> +/*
> + * 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__

Does this header need to be exposed outside of Xen and the tools?

> +
> +#include "xen.h"
> +
> +/* Each entry is preceded by a descriptor */
> +struct domain_save_descriptor {
> +    uint16_t typecode;
> +    /*
> +     * Each entry will contain either to global or per-vcpu domain state.
> +     * Entries relating to global state should have zero in this field.
> +     */
> +    uint16_t vcpu_id;
> +    uint32_t flags;
> +    /*
> +     * When restoring state this flag can be set in a descriptor to cause
> +     * its content to be ignored.

Could you give examples where you would want to ignore a descriptor?

> +     *
> +     * NOTE: It is invalid to set this flag for HEADER or END records (see
> +     *       below).
> +     */
> +#define _DOMAIN_SAVE_FLAG_IGNORE 0
> +#define DOMAIN_SAVE_FLAG_IGNORE (1u << _DOMAIN_SAVE_FLAG_IGNORE)
> +
> +    /* Entry length not including this descriptor */
> +    uint64_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; };
> +
> +#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)
> +
> +/* Terminating entry */
> +struct domain_save_end {};
> +DECLARE_DOMAIN_SAVE_TYPE(END, 0, struct domain_save_end);
> +
> +#define DOMAIN_SAVE_MAGIC   0x53415645
> +#define DOMAIN_SAVE_VERSION 0x00000001
> +
> +/* Initial entry */
> +struct domain_save_header {
> +    uint32_t magic;             /* Must be DOMAIN_SAVE_MAGIC */
> +    uint32_t version;           /* Save format version */


I haven't seen any answer about xen version here. For the record:

"Let's imagine in 4.14 we introduced a bug in the saving part. This is
solved in 4.15 but somehow the version is not bumped. How would you
differentiate the streams saved by Xen 4.14 so you can still migrate?

If you record the version of Xen in the record automatically, then you
at least have a way to differentiate the two versions."

Cheers,

Paul Durrant April 28, 2020, 3:35 p.m. UTC | #2

> -----Original Message-----
> From: Julien Grall <[e-mail]>
> Sent: 20 April 2020 18:21
> To: Paul Durrant <[e-mail]>; [e-mail]
> Cc: Paul Durrant <[e-mail]>; Andrew Cooper <[e-mail]>; George Dunlap
> <[e-mail]>; Ian Jackson <[e-mail]>; Jan Beulich <[e-mail]>;
> Stefano Stabellini <[e-mail]>; Wei Liu <[e-mail]>; Volodymyr Babchuk
> <[e-mail]>; Roger Pau Monné <[e-mail]>
> Subject: Re: [PATCH v2 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
> 
> Hi Paul,
> 
> On 07/04/2020 18:38, Paul Durrant wrote:
> > To allow enlightened HVM guests (i.e. those that have PV drivers) to be
> > migrated without their co-operation it will be necessary to transfer 'PV'
> > state such as event channel state, grant entry state, etc.
> >
> > Currently there is a framework (entered via the hvm_save/load() functions)
> > that allows a domain's 'HVM' (architectural) state to be transferred but
> > 'PV' state is also common with pure PV guests and so this framework is not
> > really suitable.
> >
> > This patch adds the new public header and low level implementation of a new
> > common framework, entered via the domain_save/load() functions. Subsequent
> > patches will introduce other parts of the framework, and code that will
> > make use of it within the current version of the libxc migration stream.
> >
> > This patch also marks the HVM-only framework as deprecated in favour of the
> > new framework.
> >
> > Signed-off-by: Paul Durrant <[e-mail]>
> > ---
> > Cc: Andrew Cooper <[e-mail]>
> > Cc: George Dunlap <[e-mail]>
> > Cc: Ian Jackson <[e-mail]>
> > Cc: Jan Beulich <[e-mail]>
> > Cc: Julien Grall <[e-mail]>
> > Cc: Stefano Stabellini <[e-mail]>
> > Cc: Wei Liu <[e-mail]>
> > Cc: Volodymyr Babchuk <[e-mail]>
> > Cc: "Roger Pau Monné" <[e-mail]>
> >
> > v2:
> >   - Allow multi-stage save/load to avoid the need to double-buffer
> >   - Get rid of the masks and add an 'ignore' flag instead
> >   - Create copy function union to preserve const save buffer
> >   - Deprecate HVM-only framework
> > ---
> >   xen/common/Makefile                    |   1 +
> >   xen/common/save.c                      | 329 +++++++++++++++++++++++++
> >   xen/include/public/arch-arm/hvm/save.h |   5 +
> >   xen/include/public/arch-x86/hvm/save.h |   5 +
> >   xen/include/public/save.h              |  84 +++++++
> >   xen/include/xen/save.h                 | 152 ++++++++++++
> >   6 files changed, 576 insertions(+)
> >   create mode 100644 xen/common/save.c
> >   create mode 100644 xen/include/public/save.h
> >   create mode 100644 xen/include/xen/save.h
> >
> > diff --git a/xen/common/Makefile b/xen/common/Makefile
> > index e8cde65370..90553ba5d7 100644
> > --- a/xen/common/Makefile
> > +++ b/xen/common/Makefile
> > @@ -37,6 +37,7 @@ obj-y += radix-tree.o
> >   obj-y += rbtree.o
> >   obj-y += rcupdate.o
> >   obj-y += rwlock.o
> > +obj-y += save.o
> >   obj-y += shutdown.o
> >   obj-y += softirq.o
> >   obj-y += sort.o
> > diff --git a/xen/common/save.c b/xen/common/save.c
> > new file mode 100644
> > index 0000000000..6cdac3785b
> > --- /dev/null
> > +++ b/xen/common/save.c
> > @@ -0,0 +1,329 @@
> > +/*
> > + * 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/save.h>
> > +
> > +union domain_copy_entry {
> > +    domain_write_entry write;
> > +    domain_read_entry read;
> > +};
> > +
> > +struct domain_context {
> > +    bool log;
> > +    struct domain_save_descriptor desc;
> > +    size_t data_len;
> 
> What is data_len?
> 

It’s used for internal accounting.

> > +    union domain_copy_entry copy;
> > +    void *priv;
> > +};
> > +
> > +static struct {
> > +    const char *name;
> > +    bool per_vcpu;
> > +    domain_save_handler save;
> > +    domain_load_handler load;
> > +} handlers[DOMAIN_SAVE_CODE_MAX + 1];
> > +
> > +void __init domain_register_save_type(unsigned int tc, const char *name,
> > +                                      bool per_vcpu,
> > +                                      domain_save_handler save,
> > +                                      domain_load_handler load)
> > +{
> > +    BUG_ON(tc > ARRAY_SIZE(handlers));
> > +
> > +    ASSERT(!handlers[tc].save);
> > +    ASSERT(!handlers[tc].load);
> > +
> > +    handlers[tc].name = name;
> > +    handlers[tc].per_vcpu = per_vcpu;
> > +    handlers[tc].save = save;
> > +    handlers[tc].load = load;
> > +}
> > +
> > +int domain_save_begin(struct domain_context *c, unsigned int tc,
> > +                      const char *name, const struct vcpu *v, size_t len)
> > +{
> > +    int rc;
> > +
> > +    if ( c->log )
> > +        gdprintk(XENLOG_INFO, "%pv save: %s (%lu)\n", v, name,
> > +                 (unsigned long)len);
> 
> How about using %zu rather than a cast here?
> 

Yes, that would be better.

> > +
> > +    BUG_ON(tc != c->desc.typecode);
> > +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
> 
> I can't find any answer on my question about this part. I understand the
> code would be buggy if this happen, but is it warrant to crash the host?
> Couldn't you just return an error and continue to run?
> 

Since it means buggy code I could ASSERT and error out, yes.

> > +
> > +    ASSERT(!c->data_len);
> > +    c->data_len = c->desc.length = len;
> > +
> > +    rc = c->copy.write(c->priv, &c->desc, sizeof(c->desc));
> > +    if ( rc )
> > +        return rc;
> > +
> > +    c->desc.length = 0;
> 
> This is confusing, why would you need to reset c->desc.length but not
> the rest of the fields?
> 

Because I use it to accumulate the length of the saved data and then cross check it against data_len in domain_save_end() below.

> > +
> > +    return 0;
> > +}
> > +
> > +int domain_save_data(struct domain_context *c, const void *src, size_t len)
> > +{
> > +    if ( c->desc.length + len > c->data_len )
> > +        return -ENOSPC;
> > +
> > +    c->desc.length += len;
> > +
> > +    return c->copy.write(c->priv, src, len);
> > +}
> > +
> > +int domain_save_end(struct domain_context *c)
> > +{
> > +    /*
> > +     * If desc.length does not match the length specified in
> > +     * domain_save_begin(), there should have been more data.
> > +     */
> > +    if ( c->desc.length != c->data_len )
> 
> This suggests you know in advance the size of the record.

That depends on what you mean by 'in advance'. I'd expect the caller of domain_save_begin() to know exactly.

> I have seen
> some cases where we don't know the size in advance. A good example if
> when saving grants. You know the maximum of grant used by the guest but
> you don't know yet the number of grants used. You might need to walk the
> "list" twice or allocate a temporary buffer. None of them are ideal.
> 
> Another example is when saving memory, we may want to compact page
> informations to save space.
> 
> This problem is going to be more relevant for LiveUpdate where we need
> to be able to create the stream in a very short amount of time.
> Allocating any temporary buffer and/or walking the list twice is going
> to kill the performance.
> 
> I would suggest to consider a different approach where you update the
> record length at the end.
> 
> FWIW, this below the current approach for the LU stream (IIRC David sent
> a prototype recently). A record is opened using lu_stream_open_record(),
> you then have two way to add data:
>      - lu_stream_append() -> This takes a buffer and write to the stream.
>      - lu_stream_reserve() -> Pre-allocate space in the stream and
> return a pointer to the beginning of the reserved region.
>      - lu_stream_end_reservation() -> Takes the actual size in parameter
> and update the stream size.
>      - lu_stream_close_record() -> Update the header with the total
> length and update the position in the stream.
> 

That sounds quite LU specific. For LM we still need to know up-front the maximal size of the buffer, and I was trying to work on the basis of never having to update previously saved data but I guess there's no actual harm in doing so, so we could avoid domain_save_begin() specifying the length.

> > +        return -EIO;
> 
> I noticed that all the pasding check have been dropped. I still think we
> need implicit padding to harden the code.
> 

I'd still view that as buggy code.

> > +
> > +    c->data_len = 0;
> > +
> > +    return 0;
> > +}
> > +
> > +int domain_save(struct domain *d, domain_write_entry write, void *priv,
> > +                bool dry_run)
> > +{
> > +    struct domain_context c = {
> > +        .copy.write = write,
> > +        .priv = priv,
> > +        .log = !dry_run,
> > +    };
> > +    struct domain_save_header h = {
> > +        .magic = DOMAIN_SAVE_MAGIC,
> > +        .version = DOMAIN_SAVE_VERSION,
> > +    };
> > +    struct domain_save_header e;
> > +    unsigned int i;
> > +    int rc;
> > +
> > +    ASSERT(d != current->domain);
> > +
> > +    if ( d->is_dying )
> 
> I can't find an answer to my question about d->is_dying. What if the
> guest die afterwards? What does protect us against domain_kill()?
> 
> [...]

As I said in a previous response, nothing protects against domain_kill(), this check is just supposed to avoid doing 'unnecessary' work for a domain we know is already dying. For LU though I guess we do need to save (some) state for even a dying domain, so the individual save handlers actually need to make the decision on whether they are going to do anything.

> 
> > +int domain_load(struct domain *d, domain_read_entry read, void *priv)
> > +{
> > +    struct domain_context c = {
> > +        .copy.read = read,
> > +        .priv = priv,
> > +        .log = true,
> > +    };
> > +    struct domain_save_header h;
> > +    int rc;
> > +
> > +    ASSERT(d != current->domain);
> > +
> > +    if ( d->is_dying )
> > +        return -EINVAL;
> 
> Same here.
> 
> 
> > diff --git a/xen/include/public/save.h b/xen/include/public/save.h
> > new file mode 100644
> > index 0000000000..7e5f8752bd
> > --- /dev/null
> > +++ b/xen/include/public/save.h
> > @@ -0,0 +1,84 @@
> > +/*
> > + * 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__
> 
> Does this header need to be exposed outside of Xen and the tools?
> 

Probably not.

> > +
> > +#include "xen.h"
> > +
> > +/* Each entry is preceded by a descriptor */
> > +struct domain_save_descriptor {
> > +    uint16_t typecode;
> > +    /*
> > +     * Each entry will contain either to global or per-vcpu domain state.
> > +     * Entries relating to global state should have zero in this field.
> > +     */
> > +    uint16_t vcpu_id;
> > +    uint32_t flags;
> > +    /*
> > +     * When restoring state this flag can be set in a descriptor to cause
> > +     * its content to be ignored.
> 
> Could you give examples where you would want to ignore a descriptor?
> 

I was thinking of the case when, e.g. you want to update something in the shared_info... You save a context blob, modify the shared_info record, and then restore the context blob with all other records marked as 'ignore' since they have not been modified.

> > +     *
> > +     * NOTE: It is invalid to set this flag for HEADER or END records (see
> > +     *       below).
> > +     */
> > +#define _DOMAIN_SAVE_FLAG_IGNORE 0
> > +#define DOMAIN_SAVE_FLAG_IGNORE (1u << _DOMAIN_SAVE_FLAG_IGNORE)
> > +
> > +    /* Entry length not including this descriptor */
> > +    uint64_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; };
> > +
> > +#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)
> > +
> > +/* Terminating entry */
> > +struct domain_save_end {};
> > +DECLARE_DOMAIN_SAVE_TYPE(END, 0, struct domain_save_end);
> > +
> > +#define DOMAIN_SAVE_MAGIC   0x53415645
> > +#define DOMAIN_SAVE_VERSION 0x00000001
> > +
> > +/* Initial entry */
> > +struct domain_save_header {
> > +    uint32_t magic;             /* Must be DOMAIN_SAVE_MAGIC */
> > +    uint32_t version;           /* Save format version */
> 
> 
> I haven't seen any answer about xen version here. For the record:
> 
> "Let's imagine in 4.14 we introduced a bug in the saving part. This is
> solved in 4.15 but somehow the version is not bumped. How would you
> differentiate the streams saved by Xen 4.14 so you can still migrate?

I'd assume testing would at least save and restore on 4.14. If we then fixed a bug then why would we not bump the version?

> 
> If you record the version of Xen in the record automatically, then you
> at least have a way to differentiate the two versions."
> 

OK, I guess redundant version information is not going to be harmful.

  Paul

Jan Beulich April 29, 2020, 11:02 a.m. UTC | #3

On 07.04.2020 19:38, Paul Durrant wrote:
> +void __init domain_register_save_type(unsigned int tc, const char *name,
> +                                      bool per_vcpu,
> +                                      domain_save_handler save,
> +                                      domain_load_handler load)
> +{
> +    BUG_ON(tc > ARRAY_SIZE(handlers));

>=

> +    ASSERT(!handlers[tc].save);
> +    ASSERT(!handlers[tc].load);
> +
> +    handlers[tc].name = name;
> +    handlers[tc].per_vcpu = per_vcpu;
> +    handlers[tc].save = save;
> +    handlers[tc].load = load;
> +}
> +
> +int domain_save_begin(struct domain_context *c, unsigned int tc,
> +                      const char *name, const struct vcpu *v, size_t len)

I find it quite odd for a function like this one to take a
struct vcpu * rather than a struct domain *. See also below
comment on the vcpu_id field in the public header.

> +{
> +    int rc;
> +
> +    if ( c->log )
> +        gdprintk(XENLOG_INFO, "%pv save: %s (%lu)\n", v, name,
> +                 (unsigned long)len);
> +
> +    BUG_ON(tc != c->desc.typecode);
> +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
> +
> +    ASSERT(!c->data_len);
> +    c->data_len = c->desc.length = len;
> +
> +    rc = c->copy.write(c->priv, &c->desc, sizeof(c->desc));
> +    if ( rc )
> +        return rc;
> +
> +    c->desc.length = 0;
> +
> +    return 0;
> +}
> +
> +int domain_save_data(struct domain_context *c, const void *src, size_t len)
> +{
> +    if ( c->desc.length + len > c->data_len )
> +        return -ENOSPC;
> +
> +    c->desc.length += len;
> +
> +    return c->copy.write(c->priv, src, len);
> +}
> +
> +int domain_save_end(struct domain_context *c)

I'm sure there is a reason for the split into three load/save
functions (begin/data/end), but I can't figure it and the
description also doesn't explain it. They're all used together
only afaics, in domain_{save,load}_entry(). Or wait, there are
DOMAIN_{SAVE,LOAD}_BEGIN() macros apparently allowing separate
use of ..._begin(), but then it's still not clear why ..._end()
need to be separate from ..._data().

> +int domain_save(struct domain *d, domain_write_entry write, void *priv,
> +                bool dry_run)
> +{
> +    struct domain_context c = {
> +        .copy.write = write,
> +        .priv = priv,
> +        .log = !dry_run,
> +    };
> +    struct domain_save_header h = {

const? Perhaps even static?

> +        .magic = DOMAIN_SAVE_MAGIC,
> +        .version = DOMAIN_SAVE_VERSION,
> +    };
> +    struct domain_save_header e;
> +    unsigned int i;
> +    int rc;
> +
> +    ASSERT(d != current->domain);
> +
> +    if ( d->is_dying )
> +        return -EINVAL;

Could I talk you into using less generic an error code here, e.g.
-ESRCH or -ENXIO? There look to be further uses of EINVAL that
may want replacing.

> +    domain_pause(d);
> +
> +    c.desc.typecode = DOMAIN_SAVE_CODE(HEADER);
> +
> +    rc = DOMAIN_SAVE_ENTRY(HEADER, &c, d->vcpu[0], &h, sizeof(h));
> +    if ( rc )
> +        goto out;
> +
> +    for ( i = 0; i < ARRAY_SIZE(handlers); i++ )
> +    {
> +        domain_save_handler save = handlers[i].save;
> +
> +        if ( !save )
> +            continue;
> +
> +        memset(&c.desc, 0, sizeof(c.desc));
> +        c.desc.typecode = i;
> +
> +        if ( handlers[i].per_vcpu )
> +        {
> +            struct vcpu *v;
> +
> +            for_each_vcpu ( d, v )
> +            {
> +                c.desc.vcpu_id = v->vcpu_id;
> +
> +                rc = save(v, &c, dry_run);
> +                if ( rc )
> +                    goto out;
> +            }
> +        }
> +        else
> +        {
> +            rc = save(d->vcpu[0], &c, dry_run);
> +            if ( rc )
> +                goto out;
> +        }
> +    }
> +
> +    memset(&c.desc, 0, sizeof(c.desc));
> +    c.desc.typecode = DOMAIN_SAVE_CODE(END);
> +
> +    rc = DOMAIN_SAVE_ENTRY(END, &c, d->vcpu[0], &e, 0);

By the looks of it you're passing uninitialized e here; it's just
that the struct has no members. It would look less odd if you used
NULL here. Otherwise please don't use literal 0, but sizeof() for
the last parameter.

> +int domain_load_begin(struct domain_context *c, unsigned int tc,
> +                      const char *name, const struct vcpu *v, size_t len,
> +                      bool exact)
> +{
> +    if ( c->log )
> +        gdprintk(XENLOG_INFO, "%pv load: %s (%lu)\n", v, name,
> +                 (unsigned long)len);
> +
> +    BUG_ON(tc != c->desc.typecode);
> +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
> +
> +    if ( (exact && (len != c->desc.length)) ||
> +         (len < c->desc.length) )
> +        return -EINVAL;

How about

    if ( exact ? len != c->desc.length
               : len < c->desc.length )

? I'm also unsure about the < - don't you mean > instead? Too
little data would be compensated by zero padding, but too
much data can't be dealt with. But maybe I'm getting the sense
of len wrong ...

> +int domain_load(struct domain *d, domain_read_entry read, void *priv)
> +{
> +    struct domain_context c = {
> +        .copy.read = read,
> +        .priv = priv,
> +        .log = true,
> +    };
> +    struct domain_save_header h;
> +    int rc;
> +
> +    ASSERT(d != current->domain);
> +
> +    if ( d->is_dying )
> +        return -EINVAL;
> +
> +    rc = c.copy.read(c.priv, &c.desc, sizeof(c.desc));
> +    if ( rc )
> +        return rc;
> +
> +    if ( c.desc.typecode != DOMAIN_SAVE_CODE(HEADER) || c.desc.vcpu_id ||
> +         c.desc.flags )
> +        return -EINVAL;
> +
> +    rc = DOMAIN_LOAD_ENTRY(HEADER, &c, d->vcpu[0], &h, sizeof(h), true);
> +    if ( rc )
> +        return rc;
> +
> +    if ( h.magic != DOMAIN_SAVE_MAGIC || h.version != DOMAIN_SAVE_VERSION )
> +        return -EINVAL;
> +
> +    domain_pause(d);
> +
> +    for (;;)
> +    {
> +        unsigned int i;
> +        unsigned int flags;
> +        domain_load_handler load;
> +        struct vcpu *v;
> +
> +        rc = c.copy.read(c.priv, &c.desc, sizeof(c.desc));
> +        if ( rc )
> +            break;
> +
> +        rc = -EINVAL;
> +
> +        flags = c.desc.flags;
> +        if ( flags & ~DOMAIN_SAVE_FLAG_IGNORE )
> +            break;
> +
> +        if ( c.desc.typecode == DOMAIN_SAVE_CODE(END) ) {

Brace placement.

> +            if ( !(flags & DOMAIN_SAVE_FLAG_IGNORE) )
> +                rc = DOMAIN_LOAD_ENTRY(END, &c, d->vcpu[0], NULL, 0, true);

The public header says DOMAIN_SAVE_FLAG_IGNORE is invalid for
END.

> +            break;
> +        }
> +
> +        i = c.desc.typecode;
> +        if ( i >= ARRAY_SIZE(handlers) )
> +            break;
> +
> +        if ( (!handlers[i].per_vcpu && c.desc.vcpu_id) ||
> +             (c.desc.vcpu_id >= d->max_vcpus) )
> +            break;
> +
> +        v = d->vcpu[c.desc.vcpu_id];
> +
> +        if ( flags & DOMAIN_SAVE_FLAG_IGNORE )
> +        {
> +            /* Sink the data */
> +            rc = domain_load_entry(&c, c.desc.typecode, "IGNORED",
> +                                   v, NULL, c.desc.length, true);

IOW the read handlers need to be able to deal with a NULL dst?
Sounds a little fragile to me. Is there a reason
domain_load_data() can't skip the call to read()?

> --- /dev/null
> +++ b/xen/include/public/save.h
> @@ -0,0 +1,84 @@
> +/*
> + * 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"
> +
> +/* Each entry is preceded by a descriptor */
> +struct domain_save_descriptor {

Throughout this new public header, let's please play nice in name
space terms: Prefix global scope items with xen_ / XEN_
respectively, and don't introduce violations of the standard's
rules (e.g. _DOMAIN_SAVE_FLAG_IGNORE below). The latter point also
goes for naming outside of the public header, of course.

> +    uint16_t typecode;
> +    /*
> +     * Each entry will contain either to global or per-vcpu domain state.

s/contain/apply/ or drop "to"?

> +     * Entries relating to global state should have zero in this field.

Is there a reason to specify zero?

> +     */
> +    uint16_t vcpu_id;

Seeing (possibly) multi-instance records in HVM state (PIC, IO-APIC, HPET),
wouldn't it be better to generalize this to ID, meaning "vCPU ID" just for
per-vCPU state?

> +    uint32_t flags;
> +    /*
> +     * When restoring state this flag can be set in a descriptor to cause
> +     * its content to be ignored.
> +     *
> +     * NOTE: It is invalid to set this flag for HEADER or END records (see
> +     *       below).
> +     */
> +#define _DOMAIN_SAVE_FLAG_IGNORE 0
> +#define DOMAIN_SAVE_FLAG_IGNORE (1u << _DOMAIN_SAVE_FLAG_IGNORE)

As per your reply to Julien I think this wants further clarification on
the intentions with this flag. I'm also uncertain it is a good idea to
allow such partial restores - there may be dependencies between records,
and hence things can easily go wrong in perhaps non-obvious ways.

> +    /* Entry length not including this descriptor */
> +    uint64_t length;

Do you really envision descriptors with more than 4Gb of data? If so,
for 32-bit purposes wouldn't this want to be uint64_aligned_t?

> +};
> +
> +/*
> + * 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; };
> +
> +#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)

Just like the typeof() I dont think the sizeof() needs an outer
pair of parentheses. I also don't see why 0 gets parenthesized.

> +/* Terminating entry */
> +struct domain_save_end {};
> +DECLARE_DOMAIN_SAVE_TYPE(END, 0, struct domain_save_end);

If the header gets a __XEN__ || __XEN_TOOLS__ restriction, as
suggested by Julien, using 0 here may be fine. If not, char[0]
and typeof() aren't legal plain C and hence would need to be
avoided.

> --- /dev/null
> +++ b/xen/include/xen/save.h
> @@ -0,0 +1,152 @@
> +/*
> + * 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/sched.h>
> +#include <xen/types.h>
> +#include <xen/init.h>

Please sort these.

> +#include <public/xen.h>
> +#include <public/save.h>

The latter includes the former anyway - is the former necessary
for some reason nevertheless?

> +struct domain_context;
> +
> +int domain_save_begin(struct domain_context *c, unsigned int tc,
> +                      const char *name, const struct vcpu *v, size_t len);
> +
> +#define DOMAIN_SAVE_BEGIN(_x, _c, _v, _len) \
> +        domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len))

In new code I'd like to ask for no leading underscores on macro
parameters as well as no unnecessary parenthesization of macro
parameters (e.g. when they simply get passed on to another macro
or function without being part of a larger expression).

> +int domain_save_data(struct domain_context *c, const void *data, size_t len);
> +int domain_save_end(struct domain_context *c);
> +
> +static inline int domain_save_entry(struct domain_context *c,
> +                                    unsigned int tc, const char *name,
> +                                    const struct vcpu *v, const void *src,
> +                                    size_t len)
> +{
> +    int rc;
> +
> +    rc = domain_save_begin(c, tc, name, v, len);
> +    if ( rc )
> +        return rc;
> +
> +    rc = domain_save_data(c, src, len);
> +    if ( rc )
> +        return rc;
> +
> +    return domain_save_end(c);
> +}
> +
> +#define DOMAIN_SAVE_ENTRY(_x, _c, _v, _src, _len) \
> +    domain_save_entry((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_src), (_len))
> +
> +int domain_load_begin(struct domain_context *c, unsigned int tc,
> +                      const char *name, const struct vcpu *v, size_t len,
> +                      bool exact);
> +
> +#define DOMAIN_LOAD_BEGIN(_x, _c, _v, _len, _exact) \
> +        domain_load_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len), \
> +                          (_exact));
> +
> +int domain_load_data(struct domain_context *c, void *data, size_t len);
> +int domain_load_end(struct domain_context *c);
> +
> +static inline int domain_load_entry(struct domain_context *c,
> +                                    unsigned int tc, const char *name,
> +                                    const struct vcpu *v, void *dst,
> +                                    size_t len, bool exact)
> +{
> +    int rc;
> +
> +    rc = domain_load_begin(c, tc, name, v, len, exact);
> +    if ( rc )
> +        return rc;
> +
> +    rc = domain_load_data(c, dst, len);
> +    if ( rc )
> +        return rc;
> +
> +    return domain_load_end(c);
> +}
> +
> +#define DOMAIN_LOAD_ENTRY(_x, _c, _v, _dst, _len, _exact) \
> +    domain_load_entry((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_dst), (_len), \
> +                          (_exact))
> +
> +/*
> + * The 'dry_run' flag indicates that the caller of domain_save() (see
> + * below) is not trying to actually acquire the data, only the size
> + * of the data. The save handler can therefore limit work to only that
> + * which is necessary to call DOMAIN_SAVE_BEGIN/ENTRY() with an accurate
> + * value for '_len'.
> + */
> +typedef int (*domain_save_handler)(const struct vcpu *v,
> +                                   struct domain_context *h,
> +                                   bool dry_run);
> +typedef int (*domain_load_handler)(struct vcpu *v,
> +                                   struct domain_context *h);

Does a load handler have a need to modify struct domain_context?

Jan

Julien Grall April 29, 2020, 11:05 a.m. UTC | #4

Hi Paul,

On 28/04/2020 16:35, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <[e-mail]>
>> Sent: 20 April 2020 18:21
>> To: Paul Durrant <[e-mail]>; [e-mail]
>> Cc: Paul Durrant <[e-mail]>; Andrew Cooper <[e-mail]>; George Dunlap
>> <[e-mail]>; Ian Jackson <[e-mail]>; Jan Beulich <[e-mail]>;
>> Stefano Stabellini <[e-mail]>; Wei Liu <[e-mail]>; Volodymyr Babchuk
>> <[e-mail]>; Roger Pau Monné <[e-mail]>
>> Subject: Re: [PATCH v2 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
>>
>> Hi Paul,
>>
>> On 07/04/2020 18:38, Paul Durrant wrote:
>>> To allow enlightened HVM guests (i.e. those that have PV drivers) to be
>>> migrated without their co-operation it will be necessary to transfer 'PV'
>>> state such as event channel state, grant entry state, etc.
>>>
>>> Currently there is a framework (entered via the hvm_save/load() functions)
>>> that allows a domain's 'HVM' (architectural) state to be transferred but
>>> 'PV' state is also common with pure PV guests and so this framework is not
>>> really suitable.
>>>
>>> This patch adds the new public header and low level implementation of a new
>>> common framework, entered via the domain_save/load() functions. Subsequent
>>> patches will introduce other parts of the framework, and code that will
>>> make use of it within the current version of the libxc migration stream.
>>>
>>> This patch also marks the HVM-only framework as deprecated in favour of the
>>> new framework.
>>>
>>> Signed-off-by: Paul Durrant <[e-mail]>
>>> ---
>>> Cc: Andrew Cooper <[e-mail]>
>>> Cc: George Dunlap <[e-mail]>
>>> Cc: Ian Jackson <[e-mail]>
>>> Cc: Jan Beulich <[e-mail]>
>>> Cc: Julien Grall <[e-mail]>
>>> Cc: Stefano Stabellini <[e-mail]>
>>> Cc: Wei Liu <[e-mail]>
>>> Cc: Volodymyr Babchuk <[e-mail]>
>>> Cc: "Roger Pau Monné" <[e-mail]>
>>>
>>> v2:
>>>    - Allow multi-stage save/load to avoid the need to double-buffer
>>>    - Get rid of the masks and add an 'ignore' flag instead
>>>    - Create copy function union to preserve const save buffer
>>>    - Deprecate HVM-only framework
>>> ---
>>>    xen/common/Makefile                    |   1 +
>>>    xen/common/save.c                      | 329 +++++++++++++++++++++++++
>>>    xen/include/public/arch-arm/hvm/save.h |   5 +
>>>    xen/include/public/arch-x86/hvm/save.h |   5 +
>>>    xen/include/public/save.h              |  84 +++++++
>>>    xen/include/xen/save.h                 | 152 ++++++++++++
>>>    6 files changed, 576 insertions(+)
>>>    create mode 100644 xen/common/save.c
>>>    create mode 100644 xen/include/public/save.h
>>>    create mode 100644 xen/include/xen/save.h
>>>
>>> diff --git a/xen/common/Makefile b/xen/common/Makefile
>>> index e8cde65370..90553ba5d7 100644
>>> --- a/xen/common/Makefile
>>> +++ b/xen/common/Makefile
>>> @@ -37,6 +37,7 @@ obj-y += radix-tree.o
>>>    obj-y += rbtree.o
>>>    obj-y += rcupdate.o
>>>    obj-y += rwlock.o
>>> +obj-y += save.o
>>>    obj-y += shutdown.o
>>>    obj-y += softirq.o
>>>    obj-y += sort.o
>>> diff --git a/xen/common/save.c b/xen/common/save.c
>>> new file mode 100644
>>> index 0000000000..6cdac3785b
>>> --- /dev/null
>>> +++ b/xen/common/save.c
>>> @@ -0,0 +1,329 @@
>>> +/*
>>> + * 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/save.h>
>>> +
>>> +union domain_copy_entry {
>>> +    domain_write_entry write;
>>> +    domain_read_entry read;
>>> +};
>>> +
>>> +struct domain_context {
>>> +    bool log;
>>> +    struct domain_save_descriptor desc;
>>> +    size_t data_len;
>>
>> What is data_len?
>>
> 
> It’s used for internal accounting.

Can you add a comment explaining it?

> 
>>> +    union domain_copy_entry copy;
>>> +    void *priv;
>>> +};
>>> +
>>> +static struct {
>>> +    const char *name;
>>> +    bool per_vcpu;
>>> +    domain_save_handler save;
>>> +    domain_load_handler load;
>>> +} handlers[DOMAIN_SAVE_CODE_MAX + 1];
>>> +
>>> +void __init domain_register_save_type(unsigned int tc, const char *name,
>>> +                                      bool per_vcpu,
>>> +                                      domain_save_handler save,
>>> +                                      domain_load_handler load)
>>> +{
>>> +    BUG_ON(tc > ARRAY_SIZE(handlers));
>>> +
>>> +    ASSERT(!handlers[tc].save);
>>> +    ASSERT(!handlers[tc].load);
>>> +
>>> +    handlers[tc].name = name;
>>> +    handlers[tc].per_vcpu = per_vcpu;
>>> +    handlers[tc].save = save;
>>> +    handlers[tc].load = load;
>>> +}
>>> +
>>> +int domain_save_begin(struct domain_context *c, unsigned int tc,
>>> +                      const char *name, const struct vcpu *v, size_t len)
>>> +{
>>> +    int rc;
>>> +
>>> +    if ( c->log )
>>> +        gdprintk(XENLOG_INFO, "%pv save: %s (%lu)\n", v, name,
>>> +                 (unsigned long)len);
>>
>> How about using %zu rather than a cast here?
>>
> 
> Yes, that would be better.
> 
>>> +
>>> +    BUG_ON(tc != c->desc.typecode);
>>> +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
>>
>> I can't find any answer on my question about this part. I understand the
>> code would be buggy if this happen, but is it warrant to crash the host?
>> Couldn't you just return an error and continue to run?
>>
> 
> Since it means buggy code I could ASSERT and error out, yes.

That would be better, thanks!

> 
>>> +
>>> +    ASSERT(!c->data_len);
>>> +    c->data_len = c->desc.length = len;
>>> +
>>> +    rc = c->copy.write(c->priv, &c->desc, sizeof(c->desc));
>>> +    if ( rc )
>>> +        return rc;
>>> +
>>> +    c->desc.length = 0;
>>
>> This is confusing, why would you need to reset c->desc.length but not
>> the rest of the fields?
>>
> 
> Because I use it to accumulate the length of the saved data and then cross check it against data_len in domain_save_end() below.
> 
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int domain_save_data(struct domain_context *c, const void *src, size_t len)
>>> +{
>>> +    if ( c->desc.length + len > c->data_len )
>>> +        return -ENOSPC;
>>> +
>>> +    c->desc.length += len;
>>> +
>>> +    return c->copy.write(c->priv, src, len);
>>> +}
>>> +
>>> +int domain_save_end(struct domain_context *c)
>>> +{
>>> +    /*
>>> +     * If desc.length does not match the length specified in
>>> +     * domain_save_begin(), there should have been more data.
>>> +     */
>>> +    if ( c->desc.length != c->data_len )
>>
>> This suggests you know in advance the size of the record.
> 
> That depends on what you mean by 'in advance'. I'd expect the caller of domain_save_begin() to know exactly.
> 
>> I have seen
>> some cases where we don't know the size in advance. A good example if
>> when saving grants. You know the maximum of grant used by the guest but
>> you don't know yet the number of grants used. You might need to walk the
>> "list" twice or allocate a temporary buffer. None of them are ideal.
>>
>> Another example is when saving memory, we may want to compact page
>> informations to save space.
>>
>> This problem is going to be more relevant for LiveUpdate where we need
>> to be able to create the stream in a very short amount of time.
>> Allocating any temporary buffer and/or walking the list twice is going
>> to kill the performance.
>>
>> I would suggest to consider a different approach where you update the
>> record length at the end.
>>
>> FWIW, this below the current approach for the LU stream (IIRC David sent
>> a prototype recently). A record is opened using lu_stream_open_record(),
>> you then have two way to add data:
>>       - lu_stream_append() -> This takes a buffer and write to the stream.
>>       - lu_stream_reserve() -> Pre-allocate space in the stream and
>> return a pointer to the beginning of the reserved region.
>>       - lu_stream_end_reservation() -> Takes the actual size in parameter
>> and update the stream size.
>>       - lu_stream_close_record() -> Update the header with the total
>> length and update the position in the stream.
>>
> 
> That sounds quite LU specific. For LM we still need to know up-front the maximal size of the buffer, 

I described the function from an LU PoV, but thsi is mostly replace 
lu_stream_reserve() by a function that check you have at least N bytes 
free in your stream.

> and I was trying to work on the basis of never having to update previously saved data but I guess there's no actual harm in doing so, so we could avoid domain_save_begin() specifying the length.

It depends what we want to achieve in term of memory space and time. If 
we want to avoid modifying previously saved data, then we would need to 
walk the elements twice or allocate a temporary buffer. They will both 
consume space and time.

A good example for LM is when we need to save the event channels. The 
maximum number of event channels is known in advance but you don't know 
how many of them are used. We could save all of them (even the free 
ones) but this is a waste of space.

To avoid the temporary buffer or walking the elements twice, our 
internal tree contain open-code code to rewrite the record in the event 
channels save function.

> 
>>> +        return -EIO;
>>
>> I noticed that all the pasding check have been dropped. I still think we
>> need implicit padding to harden the code.
>>
> 
> I'd still view that as buggy code.
> 
>>> +
>>> +    c->data_len = 0;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +int domain_save(struct domain *d, domain_write_entry write, void *priv,
>>> +                bool dry_run)
>>> +{
>>> +    struct domain_context c = {
>>> +        .copy.write = write,
>>> +        .priv = priv,
>>> +        .log = !dry_run,
>>> +    };
>>> +    struct domain_save_header h = {
>>> +        .magic = DOMAIN_SAVE_MAGIC,
>>> +        .version = DOMAIN_SAVE_VERSION,
>>> +    };
>>> +    struct domain_save_header e;
>>> +    unsigned int i;
>>> +    int rc;
>>> +
>>> +    ASSERT(d != current->domain);
>>> +
>>> +    if ( d->is_dying )
>>
>> I can't find an answer to my question about d->is_dying. What if the
>> guest die afterwards? What does protect us against domain_kill()?
>>
>> [...]
> 
> As I said in a previous response, nothing protects against domain_kill(), this check is just supposed to avoid doing 'unnecessary' work for a domain we know is already dying. 

Regardless LU (see below), I would argue that if a user purposefully 
call domain_save() on a dying domain, then he/she already accepted the cost.

Beside, this give a false idea that domain_kill() cannot be called in 
parallel. So I would rather drop this check unless we can make it safe.

> For LU though I guess we do need to save (some) state for even a dying domain, so the individual save handlers actually need to make the decision on whether they are going to do anything.

That's correct we would want to preserve states for dying domain.

>>
>>> +int domain_load(struct domain *d, domain_read_entry read, void *priv)
>>> +{
>>> +    struct domain_context c = {
>>> +        .copy.read = read,
>>> +        .priv = priv,
>>> +        .log = true,
>>> +    };
>>> +    struct domain_save_header h;
>>> +    int rc;
>>> +
>>> +    ASSERT(d != current->domain);
>>> +
>>> +    if ( d->is_dying )
>>> +        return -EINVAL;
>>
>> Same here.
>>
>>
>>> diff --git a/xen/include/public/save.h b/xen/include/public/save.h
>>> new file mode 100644
>>> index 0000000000..7e5f8752bd
>>> --- /dev/null
>>> +++ b/xen/include/public/save.h
>>> @@ -0,0 +1,84 @@
>>> +/*
>>> + * 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__
>>
>> Does this header need to be exposed outside of Xen and the tools?
>>
> 
> Probably not.
> 
>>> +
>>> +#include "xen.h"
>>> +
>>> +/* Each entry is preceded by a descriptor */
>>> +struct domain_save_descriptor {
>>> +    uint16_t typecode;
>>> +    /*
>>> +     * Each entry will contain either to global or per-vcpu domain state.
>>> +     * Entries relating to global state should have zero in this field.
>>> +     */
>>> +    uint16_t vcpu_id;
>>> +    uint32_t flags;
>>> +    /*
>>> +     * When restoring state this flag can be set in a descriptor to cause
>>> +     * its content to be ignored.
>>
>> Could you give examples where you would want to ignore a descriptor?
>>
> 
> I was thinking of the case when, e.g. you want to update something in the shared_info... You save a context blob, modify the shared_info record, and then restore the context blob with all other records marked as 'ignore' since they have not been modified.

How about introducing a domctl similar to 
XEN_DOMCTL_gethvmcontext_partial? This would only give you the 
shared_info record and make easier for the user to use.

> 
>>> +     *
>>> +     * NOTE: It is invalid to set this flag for HEADER or END records (see
>>> +     *       below).
>>> +     */
>>> +#define _DOMAIN_SAVE_FLAG_IGNORE 0
>>> +#define DOMAIN_SAVE_FLAG_IGNORE (1u << _DOMAIN_SAVE_FLAG_IGNORE)
>>> +
>>> +    /* Entry length not including this descriptor */
>>> +    uint64_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; };
>>> +
>>> +#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)
>>> +
>>> +/* Terminating entry */
>>> +struct domain_save_end {};
>>> +DECLARE_DOMAIN_SAVE_TYPE(END, 0, struct domain_save_end);
>>> +
>>> +#define DOMAIN_SAVE_MAGIC   0x53415645
>>> +#define DOMAIN_SAVE_VERSION 0x00000001
>>> +
>>> +/* Initial entry */
>>> +struct domain_save_header {
>>> +    uint32_t magic;             /* Must be DOMAIN_SAVE_MAGIC */
>>> +    uint32_t version;           /* Save format version */
>>
>>
>> I haven't seen any answer about xen version here. For the record:
>>
>> "Let's imagine in 4.14 we introduced a bug in the saving part. This is
>> solved in 4.15 but somehow the version is not bumped. How would you
>> differentiate the streams saved by Xen 4.14 so you can still migrate?
> 
> I'd assume testing would at least save and restore on 4.14. If we then fixed a bug then why would we not bump the version?

Do you mean we would test save/restore between 4.14 and 4.15 (and 
vice-versa)? If so, this may work but we need this to be part of OSSTest 
so we can catch this. Otherwise there are chance we don't catch 
regression in the stream.

> 
>>
>> If you record the version of Xen in the record automatically, then you
>> at least have a way to differentiate the two versions."
>>
> 
> OK, I guess redundant version information is not going to be harmful.
> 
>    Paul
>

Paul Durrant May 6, 2020, 4:44 p.m. UTC | #5

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 29 April 2020 12:02
> To: Paul Durrant <[e-mail]>
> Cc: [e-mail]; Paul Durrant <[e-mail]>; Andrew Cooper
> <[e-mail]>; George Dunlap <[e-mail]>; Ian Jackson
> <[e-mail]>; Julien Grall <[e-mail]>; Stefano Stabellini
> <[e-mail]>; Wei Liu <[e-mail]>; Volodymyr Babchuk <[e-mail]>; Roger
> Pau Monné <[e-mail]>
> Subject: Re: [PATCH v2 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
> 
> On 07.04.2020 19:38, Paul Durrant wrote:
> > +void __init domain_register_save_type(unsigned int tc, const char *name,
> > +                                      bool per_vcpu,
> > +                                      domain_save_handler save,
> > +                                      domain_load_handler load)
> > +{
> > +    BUG_ON(tc > ARRAY_SIZE(handlers));
> 
> >=

Yes.

> 
> > +    ASSERT(!handlers[tc].save);
> > +    ASSERT(!handlers[tc].load);
> > +
> > +    handlers[tc].name = name;
> > +    handlers[tc].per_vcpu = per_vcpu;
> > +    handlers[tc].save = save;
> > +    handlers[tc].load = load;
> > +}
> > +
> > +int domain_save_begin(struct domain_context *c, unsigned int tc,
> > +                      const char *name, const struct vcpu *v, size_t len)
> 
> I find it quite odd for a function like this one to take a
> struct vcpu * rather than a struct domain *. See also below
> comment on the vcpu_id field in the public header.

I guess struct domain + vcpu_id can be used.

> 
> > +{
> > +    int rc;
> > +
> > +    if ( c->log )
> > +        gdprintk(XENLOG_INFO, "%pv save: %s (%lu)\n", v, name,
> > +                 (unsigned long)len);
> > +
> > +    BUG_ON(tc != c->desc.typecode);
> > +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
> > +
> > +    ASSERT(!c->data_len);
> > +    c->data_len = c->desc.length = len;
> > +
> > +    rc = c->copy.write(c->priv, &c->desc, sizeof(c->desc));
> > +    if ( rc )
> > +        return rc;
> > +
> > +    c->desc.length = 0;
> > +
> > +    return 0;
> > +}
> > +
> > +int domain_save_data(struct domain_context *c, const void *src, size_t len)
> > +{
> > +    if ( c->desc.length + len > c->data_len )
> > +        return -ENOSPC;
> > +
> > +    c->desc.length += len;
> > +
> > +    return c->copy.write(c->priv, src, len);
> > +}
> > +
> > +int domain_save_end(struct domain_context *c)
> 
> I'm sure there is a reason for the split into three load/save
> functions (begin/data/end), but I can't figure it and the
> description also doesn't explain it. They're all used together
> only afaics, in domain_{save,load}_entry(). Or wait, there are
> DOMAIN_{SAVE,LOAD}_BEGIN() macros apparently allowing separate
> use of ..._begin(), but then it's still not clear why ..._end()
> need to be separate from ..._data().

The split is to avoid the need to double-buffer in the save code, shared info being a case in point. If the entire save record needs to be written in one call then the shared info content would need to be copied into a newly allocated save record and then copied again into the aggregate context buffer.

> 
> > +int domain_save(struct domain *d, domain_write_entry write, void *priv,
> > +                bool dry_run)
> > +{
> > +    struct domain_context c = {
> > +        .copy.write = write,
> > +        .priv = priv,
> > +        .log = !dry_run,
> > +    };
> > +    struct domain_save_header h = {
> 
> const? Perhaps even static?
> 

Ok, static is a good idea.

> > +        .magic = DOMAIN_SAVE_MAGIC,
> > +        .version = DOMAIN_SAVE_VERSION,
> > +    };
> > +    struct domain_save_header e;
> > +    unsigned int i;
> > +    int rc;
> > +
> > +    ASSERT(d != current->domain);
> > +
> > +    if ( d->is_dying )
> > +        return -EINVAL;
> 
> Could I talk you into using less generic an error code here, e.g.
> -ESRCH or -ENXIO? There look to be further uses of EINVAL that
> may want replacing.
> 

I'm going to drop this check as it creates a problem for live update, where we actually do need to extract and restore state for dying domains.

> > +    domain_pause(d);
> > +
> > +    c.desc.typecode = DOMAIN_SAVE_CODE(HEADER);
> > +
> > +    rc = DOMAIN_SAVE_ENTRY(HEADER, &c, d->vcpu[0], &h, sizeof(h));
> > +    if ( rc )
> > +        goto out;
> > +
> > +    for ( i = 0; i < ARRAY_SIZE(handlers); i++ )
> > +    {
> > +        domain_save_handler save = handlers[i].save;
> > +
> > +        if ( !save )
> > +            continue;
> > +
> > +        memset(&c.desc, 0, sizeof(c.desc));
> > +        c.desc.typecode = i;
> > +
> > +        if ( handlers[i].per_vcpu )
> > +        {
> > +            struct vcpu *v;
> > +
> > +            for_each_vcpu ( d, v )
> > +            {
> > +                c.desc.vcpu_id = v->vcpu_id;
> > +
> > +                rc = save(v, &c, dry_run);
> > +                if ( rc )
> > +                    goto out;
> > +            }
> > +        }
> > +        else
> > +        {
> > +            rc = save(d->vcpu[0], &c, dry_run);
> > +            if ( rc )
> > +                goto out;
> > +        }
> > +    }
> > +
> > +    memset(&c.desc, 0, sizeof(c.desc));
> > +    c.desc.typecode = DOMAIN_SAVE_CODE(END);
> > +
> > +    rc = DOMAIN_SAVE_ENTRY(END, &c, d->vcpu[0], &e, 0);
> 
> By the looks of it you're passing uninitialized e here; it's just
> that the struct has no members. It would look less odd if you used
> NULL here. Otherwise please don't use literal 0, but sizeof() for
> the last parameter.

I'll init the 'e'.

> 
> > +int domain_load_begin(struct domain_context *c, unsigned int tc,
> > +                      const char *name, const struct vcpu *v, size_t len,
> > +                      bool exact)
> > +{
> > +    if ( c->log )
> > +        gdprintk(XENLOG_INFO, "%pv load: %s (%lu)\n", v, name,
> > +                 (unsigned long)len);
> > +
> > +    BUG_ON(tc != c->desc.typecode);
> > +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
> > +
> > +    if ( (exact && (len != c->desc.length)) ||
> > +         (len < c->desc.length) )
> > +        return -EINVAL;
> 
> How about
> 
>     if ( exact ? len != c->desc.length
>                : len < c->desc.length )
> 

Yes, that doesn't look too bad.

> ? I'm also unsure about the < - don't you mean > instead? Too
> little data would be compensated by zero padding, but too
> much data can't be dealt with. But maybe I'm getting the sense
> of len wrong ...

I think the < is correct. The caller needs to have at least enough space to accommodate the context record.

> > +int domain_load(struct domain *d, domain_read_entry read, void *priv)
> > +{
> > +    struct domain_context c = {
> > +        .copy.read = read,
> > +        .priv = priv,
> > +        .log = true,
> > +    };
> > +    struct domain_save_header h;
> > +    int rc;
> > +
> > +    ASSERT(d != current->domain);
> > +
> > +    if ( d->is_dying )
> > +        return -EINVAL;
> > +
> > +    rc = c.copy.read(c.priv, &c.desc, sizeof(c.desc));
> > +    if ( rc )
> > +        return rc;
> > +
> > +    if ( c.desc.typecode != DOMAIN_SAVE_CODE(HEADER) || c.desc.vcpu_id ||
> > +         c.desc.flags )
> > +        return -EINVAL;
> > +
> > +    rc = DOMAIN_LOAD_ENTRY(HEADER, &c, d->vcpu[0], &h, sizeof(h), true);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    if ( h.magic != DOMAIN_SAVE_MAGIC || h.version != DOMAIN_SAVE_VERSION )
> > +        return -EINVAL;
> > +
> > +    domain_pause(d);
> > +
> > +    for (;;)
> > +    {
> > +        unsigned int i;
> > +        unsigned int flags;
> > +        domain_load_handler load;
> > +        struct vcpu *v;
> > +
> > +        rc = c.copy.read(c.priv, &c.desc, sizeof(c.desc));
> > +        if ( rc )
> > +            break;
> > +
> > +        rc = -EINVAL;
> > +
> > +        flags = c.desc.flags;
> > +        if ( flags & ~DOMAIN_SAVE_FLAG_IGNORE )
> > +            break;
> > +
> > +        if ( c.desc.typecode == DOMAIN_SAVE_CODE(END) ) {
> 
> Brace placement.
> 

Oops, yes.

> > +            if ( !(flags & DOMAIN_SAVE_FLAG_IGNORE) )
> > +                rc = DOMAIN_LOAD_ENTRY(END, &c, d->vcpu[0], NULL, 0, true);
> 
> The public header says DOMAIN_SAVE_FLAG_IGNORE is invalid for
> END.
> 

Indeed, and it should be... don't know why I put that in there.

> > +            break;
> > +        }
> > +
> > +        i = c.desc.typecode;
> > +        if ( i >= ARRAY_SIZE(handlers) )
> > +            break;
> > +
> > +        if ( (!handlers[i].per_vcpu && c.desc.vcpu_id) ||
> > +             (c.desc.vcpu_id >= d->max_vcpus) )
> > +            break;
> > +
> > +        v = d->vcpu[c.desc.vcpu_id];
> > +
> > +        if ( flags & DOMAIN_SAVE_FLAG_IGNORE )
> > +        {
> > +            /* Sink the data */
> > +            rc = domain_load_entry(&c, c.desc.typecode, "IGNORED",
> > +                                   v, NULL, c.desc.length, true);
> 
> IOW the read handlers need to be able to deal with a NULL dst?
> Sounds a little fragile to me. Is there a reason
> domain_load_data() can't skip the call to read()?

Something has to move the cursor so sinking the data using a NULL dst seemed like the best way to avoid the need for a separate callback function.

> 
> > --- /dev/null
> > +++ b/xen/include/public/save.h
> > @@ -0,0 +1,84 @@
> > +/*
> > + * 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"
> > +
> > +/* Each entry is preceded by a descriptor */
> > +struct domain_save_descriptor {
> 
> Throughout this new public header, let's please play nice in name
> space terms: Prefix global scope items with xen_ / XEN_
> respectively, and don't introduce violations of the standard's
> rules (e.g. _DOMAIN_SAVE_FLAG_IGNORE below). The latter point also
> goes for naming outside of the public header, of course.

Sorry, I just didn't pay enough attention to the cut'n'paste from the hvm save.h; I'll fix these up.

> 
> > +    uint16_t typecode;
> > +    /*
> > +     * Each entry will contain either to global or per-vcpu domain state.
> 
> s/contain/apply/ or drop "to"?

Yes, 'to' should be dropped.

> 
> > +     * Entries relating to global state should have zero in this field.
> 
> Is there a reason to specify zero?
> 

Not particularly but I thought it best to at least specify a value in all cases.

> > +     */
> > +    uint16_t vcpu_id;
> 
> Seeing (possibly) multi-instance records in HVM state (PIC, IO-APIC, HPET),
> wouldn't it be better to generalize this to ID, meaning "vCPU ID" just for
> per-vCPU state?
> 

True, a generic 'instance' would be needed for such records. I'll so as you suggest.

> > +    uint32_t flags;
> > +    /*
> > +     * When restoring state this flag can be set in a descriptor to cause
> > +     * its content to be ignored.
> > +     *
> > +     * NOTE: It is invalid to set this flag for HEADER or END records (see
> > +     *       below).
> > +     */
> > +#define _DOMAIN_SAVE_FLAG_IGNORE 0
> > +#define DOMAIN_SAVE_FLAG_IGNORE (1u << _DOMAIN_SAVE_FLAG_IGNORE)
> 
> As per your reply to Julien I think this wants further clarification on
> the intentions with this flag. I'm also uncertain it is a good idea to
> allow such partial restores - there may be dependencies between records,
> and hence things can easily go wrong in perhaps non-obvious ways.
> 

OK, I'll drop this for now. It could be added later if it is needed.

> > +    /* Entry length not including this descriptor */
> > +    uint64_t length;
> 
> Do you really envision descriptors with more than 4Gb of data? If so,
> for 32-bit purposes wouldn't this want to be uint64_aligned_t?
> 

I don't think we'll ever see a single record that big. I'll drop to 32 bits.

> > +};
> > +
> > +/*
> > + * 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; };
> > +
> > +#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)
> 
> Just like the typeof() I dont think the sizeof() needs an outer
> pair of parentheses. I also don't see why 0 gets parenthesized.
> 

Cut'n'paste... I'll fix it.

> > +/* Terminating entry */
> > +struct domain_save_end {};
> > +DECLARE_DOMAIN_SAVE_TYPE(END, 0, struct domain_save_end);
> 
> If the header gets a __XEN__ || __XEN_TOOLS__ restriction, as
> suggested by Julien, using 0 here may be fine. If not, char[0]
> and typeof() aren't legal plain C and hence would need to be
> avoided.
> 

I'll restrict to tools.

> > --- /dev/null
> > +++ b/xen/include/xen/save.h
> > @@ -0,0 +1,152 @@
> > +/*
> > + * 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/sched.h>
> > +#include <xen/types.h>
> > +#include <xen/init.h>
> 
> Please sort these.
> 

Ok.

> > +#include <public/xen.h>
> > +#include <public/save.h>
> 
> The latter includes the former anyway - is the former necessary
> for some reason nevertheless?
> 

No. I suspect it was at one point, but it can be dropped.

> > +struct domain_context;
> > +
> > +int domain_save_begin(struct domain_context *c, unsigned int tc,
> > +                      const char *name, const struct vcpu *v, size_t len);
> > +
> > +#define DOMAIN_SAVE_BEGIN(_x, _c, _v, _len) \
> > +        domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len))
> 
> In new code I'd like to ask for no leading underscores on macro
> parameters as well as no unnecessary parenthesization of macro
> parameters (e.g. when they simply get passed on to another macro
> or function without being part of a larger expression).

Personally I think it is generally good practice to parenthesize but I can drop if you prefer.

> 
> > +int domain_save_data(struct domain_context *c, const void *data, size_t len);
> > +int domain_save_end(struct domain_context *c);
> > +
> > +static inline int domain_save_entry(struct domain_context *c,
> > +                                    unsigned int tc, const char *name,
> > +                                    const struct vcpu *v, const void *src,
> > +                                    size_t len)
> > +{
> > +    int rc;
> > +
> > +    rc = domain_save_begin(c, tc, name, v, len);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    rc = domain_save_data(c, src, len);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    return domain_save_end(c);
> > +}
> > +
> > +#define DOMAIN_SAVE_ENTRY(_x, _c, _v, _src, _len) \
> > +    domain_save_entry((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_src), (_len))
> > +
> > +int domain_load_begin(struct domain_context *c, unsigned int tc,
> > +                      const char *name, const struct vcpu *v, size_t len,
> > +                      bool exact);
> > +
> > +#define DOMAIN_LOAD_BEGIN(_x, _c, _v, _len, _exact) \
> > +        domain_load_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len), \
> > +                          (_exact));
> > +
> > +int domain_load_data(struct domain_context *c, void *data, size_t len);
> > +int domain_load_end(struct domain_context *c);
> > +
> > +static inline int domain_load_entry(struct domain_context *c,
> > +                                    unsigned int tc, const char *name,
> > +                                    const struct vcpu *v, void *dst,
> > +                                    size_t len, bool exact)
> > +{
> > +    int rc;
> > +
> > +    rc = domain_load_begin(c, tc, name, v, len, exact);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    rc = domain_load_data(c, dst, len);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    return domain_load_end(c);
> > +}
> > +
> > +#define DOMAIN_LOAD_ENTRY(_x, _c, _v, _dst, _len, _exact) \
> > +    domain_load_entry((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_dst), (_len), \
> > +                          (_exact))
> > +
> > +/*
> > + * The 'dry_run' flag indicates that the caller of domain_save() (see
> > + * below) is not trying to actually acquire the data, only the size
> > + * of the data. The save handler can therefore limit work to only that
> > + * which is necessary to call DOMAIN_SAVE_BEGIN/ENTRY() with an accurate
> > + * value for '_len'.
> > + */
> > +typedef int (*domain_save_handler)(const struct vcpu *v,
> > +                                   struct domain_context *h,
> > +                                   bool dry_run);
> > +typedef int (*domain_load_handler)(struct vcpu *v,
> > +                                   struct domain_context *h);
> 
> Does a load handler have a need to modify struct domain_context?
> 

Not sure. I'll try const-ing.

  Paul

> Jan

Jan Beulich May 7, 2020, 7:21 a.m. UTC | #6

On 06.05.2020 18:44, Paul Durrant wrote:
>> From: Jan Beulich <[e-mail]>
>> Sent: 29 April 2020 12:02
>>
>> On 07.04.2020 19:38, Paul Durrant wrote:
>>> +int domain_load_begin(struct domain_context *c, unsigned int tc,
>>> +                      const char *name, const struct vcpu *v, size_t len,
>>> +                      bool exact)
>>> +{
>>> +    if ( c->log )
>>> +        gdprintk(XENLOG_INFO, "%pv load: %s (%lu)\n", v, name,
>>> +                 (unsigned long)len);
>>> +
>>> +    BUG_ON(tc != c->desc.typecode);
>>> +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
>>> +
>>> +    if ( (exact && (len != c->desc.length)) ||
>>> +         (len < c->desc.length) )
>>> +        return -EINVAL;
>>
>> How about
>>
>>     if ( exact ? len != c->desc.length
>>                : len < c->desc.length )
>>
> 
> Yes, that doesn't look too bad.
> 
>> ? I'm also unsure about the < - don't you mean > instead? Too
>> little data would be compensated by zero padding, but too
>> much data can't be dealt with. But maybe I'm getting the sense
>> of len wrong ...
> 
> I think the < is correct. The caller needs to have at least enough
> space to accommodate the context record.

But this is load, not save - the caller supplies the data. If
there's less data than can be fit, it'll be zero-extended. If
there's too much data, the excess you don't know what to do
with (it might be okay to tolerate it being all zero).

>>> +        if ( (!handlers[i].per_vcpu && c.desc.vcpu_id) ||
>>> +             (c.desc.vcpu_id >= d->max_vcpus) )
>>> +            break;
>>> +
>>> +        v = d->vcpu[c.desc.vcpu_id];
>>> +
>>> +        if ( flags & DOMAIN_SAVE_FLAG_IGNORE )
>>> +        {
>>> +            /* Sink the data */
>>> +            rc = domain_load_entry(&c, c.desc.typecode, "IGNORED",
>>> +                                   v, NULL, c.desc.length, true);
>>
>> IOW the read handlers need to be able to deal with a NULL dst?
>> Sounds a little fragile to me. Is there a reason
>> domain_load_data() can't skip the call to read()?
> 
> Something has to move the cursor so sinking the data using a
> NULL dst seemed like the best way to avoid the need for a
> separate callback function.

And domain_load_data() can't itself advance the cursor in such
a case, with no callback involved at all?

>>> +    uint16_t typecode;
>>> +    /*
>>> +     * Each entry will contain either to global or per-vcpu domain state.
>>> +     * Entries relating to global state should have zero in this field.
>>
>> Is there a reason to specify zero?
>>
> 
> Not particularly but I thought it best to at least specify a value in all cases.

A specific value is certainly a good idea; I wonder though whether
an obviously invalid one (like ~0) wouldn't be better then,
allowing this ID to later be assigned meaning in such cases if
need be.

>>> +     */
>>> +    uint16_t vcpu_id;
>>
>> Seeing (possibly) multi-instance records in HVM state (PIC, IO-APIC, HPET),
>> wouldn't it be better to generalize this to ID, meaning "vCPU ID" just for
>> per-vCPU state?
> 
> True, a generic 'instance' would be needed for such records. I'll so as you suggest.

Which, along with my comment on domain_save_begin() taking a
struct vcpu * right now, will then indeed require changing
to a (struct domain *, unsigned int instance) tuple, I guess.

>>> +#define DOMAIN_SAVE_BEGIN(_x, _c, _v, _len) \
>>> +        domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len))
>>
>> In new code I'd like to ask for no leading underscores on macro
>> parameters as well as no unnecessary parenthesization of macro
>> parameters (e.g. when they simply get passed on to another macro
>> or function without being part of a larger expression).
> 
> Personally I think it is generally good practice to parenthesize
> but I can drop if you prefer.

To be honest - it's more than just "prefer": Excess parentheses
hamper readability. There shouldn't be too few, and since macros
already require quite a lot of them, imo we should strive to
have exactly as many as are needed.

Jan

Paul Durrant May 7, 2020, 7:34 a.m. UTC | #7

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 07 May 2020 08:22
> To: [e-mail]
> Cc: [e-mail]; 'Paul Durrant' <[e-mail]>; 'Andrew Cooper'
> <[e-mail]>; 'George Dunlap' <[e-mail]>; 'Ian Jackson'
> <[e-mail]>; 'Julien Grall' <[e-mail]>; 'Stefano Stabellini'
> <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Volodymyr Babchuk' <[e-mail]>;
> 'Roger Pau Monné' <[e-mail]>
> Subject: Re: [PATCH v2 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
> 
> On 06.05.2020 18:44, Paul Durrant wrote:
> >> From: Jan Beulich <[e-mail]>
> >> Sent: 29 April 2020 12:02
> >>
> >> On 07.04.2020 19:38, Paul Durrant wrote:
> >>> +int domain_load_begin(struct domain_context *c, unsigned int tc,
> >>> +                      const char *name, const struct vcpu *v, size_t len,
> >>> +                      bool exact)
> >>> +{
> >>> +    if ( c->log )
> >>> +        gdprintk(XENLOG_INFO, "%pv load: %s (%lu)\n", v, name,
> >>> +                 (unsigned long)len);
> >>> +
> >>> +    BUG_ON(tc != c->desc.typecode);
> >>> +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
> >>> +
> >>> +    if ( (exact && (len != c->desc.length)) ||
> >>> +         (len < c->desc.length) )
> >>> +        return -EINVAL;
> >>
> >> How about
> >>
> >>     if ( exact ? len != c->desc.length
> >>                : len < c->desc.length )
> >>
> >
> > Yes, that doesn't look too bad.
> >
> >> ? I'm also unsure about the < - don't you mean > instead? Too
> >> little data would be compensated by zero padding, but too
> >> much data can't be dealt with. But maybe I'm getting the sense
> >> of len wrong ...
> >
> > I think the < is correct. The caller needs to have at least enough
> > space to accommodate the context record.
> 
> But this is load, not save - the caller supplies the data. If
> there's less data than can be fit, it'll be zero-extended. If
> there's too much data, the excess you don't know what to do
> with (it might be okay to tolerate it being all zero).
> 

But this is a callback. The outer load function iterates over the records calling the appropriate hander for each one. Those handlers then call this function saying how much data they expect and whether they want exactly that amount, or whether they can tolerate less (i.e. zero-extend). Hence len < c->desc.length is an error, because it means the descriptor contains more data than the hander knows how to handle.

> >>> +        if ( (!handlers[i].per_vcpu && c.desc.vcpu_id) ||
> >>> +             (c.desc.vcpu_id >= d->max_vcpus) )
> >>> +            break;
> >>> +
> >>> +        v = d->vcpu[c.desc.vcpu_id];
> >>> +
> >>> +        if ( flags & DOMAIN_SAVE_FLAG_IGNORE )
> >>> +        {
> >>> +            /* Sink the data */
> >>> +            rc = domain_load_entry(&c, c.desc.typecode, "IGNORED",
> >>> +                                   v, NULL, c.desc.length, true);
> >>
> >> IOW the read handlers need to be able to deal with a NULL dst?
> >> Sounds a little fragile to me. Is there a reason
> >> domain_load_data() can't skip the call to read()?
> >
> > Something has to move the cursor so sinking the data using a
> > NULL dst seemed like the best way to avoid the need for a
> > separate callback function.
> 
> And domain_load_data() can't itself advance the cursor in such
> a case, with no callback involved at all?
> 

How could it do that without a callback? Doing such a thing negates the utility of the ignore flag anyway since it implies the caller is going to edit the load stream in advance. Since I'm going to drop the ignore flag in v3 anyway, this is all a bit academic.

> >>> +    uint16_t typecode;
> >>> +    /*
> >>> +     * Each entry will contain either to global or per-vcpu domain state.
> >>> +     * Entries relating to global state should have zero in this field.
> >>
> >> Is there a reason to specify zero?
> >>
> >
> > Not particularly but I thought it best to at least specify a value in all cases.
> 
> A specific value is certainly a good idea; I wonder though whether
> an obviously invalid one (like ~0) wouldn't be better then,
> allowing this ID to later be assigned meaning in such cases if
> need be.
> 

Ok, that sounds reasonable. I'll #define something for convenience.

> >>> +     */
> >>> +    uint16_t vcpu_id;
> >>
> >> Seeing (possibly) multi-instance records in HVM state (PIC, IO-APIC, HPET),
> >> wouldn't it be better to generalize this to ID, meaning "vCPU ID" just for
> >> per-vCPU state?
> >
> > True, a generic 'instance' would be needed for such records. I'll so as you suggest.
> 
> Which, along with my comment on domain_save_begin() taking a
> struct vcpu * right now, will then indeed require changing
> to a (struct domain *, unsigned int instance) tuple, I guess.
> 

Yes.

  Paul

Jan Beulich May 7, 2020, 7:39 a.m. UTC | #8

On 07.05.2020 09:34, Paul Durrant wrote:
>> From: Jan Beulich <[e-mail]>
>> Sent: 07 May 2020 08:22
>>
>> On 06.05.2020 18:44, Paul Durrant wrote:
>>>> From: Jan Beulich <[e-mail]>
>>>> Sent: 29 April 2020 12:02
>>>>
>>>> On 07.04.2020 19:38, Paul Durrant wrote:
>>>>> +int domain_load_begin(struct domain_context *c, unsigned int tc,
>>>>> +                      const char *name, const struct vcpu *v, size_t len,
>>>>> +                      bool exact)
>>>>> +{
>>>>> +    if ( c->log )
>>>>> +        gdprintk(XENLOG_INFO, "%pv load: %s (%lu)\n", v, name,
>>>>> +                 (unsigned long)len);
>>>>> +
>>>>> +    BUG_ON(tc != c->desc.typecode);
>>>>> +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
>>>>> +
>>>>> +    if ( (exact && (len != c->desc.length)) ||
>>>>> +         (len < c->desc.length) )
>>>>> +        return -EINVAL;
>>>>
>>>> How about
>>>>
>>>>     if ( exact ? len != c->desc.length
>>>>                : len < c->desc.length )
>>>>
>>>
>>> Yes, that doesn't look too bad.
>>>
>>>> ? I'm also unsure about the < - don't you mean > instead? Too
>>>> little data would be compensated by zero padding, but too
>>>> much data can't be dealt with. But maybe I'm getting the sense
>>>> of len wrong ...
>>>
>>> I think the < is correct. The caller needs to have at least enough
>>> space to accommodate the context record.
>>
>> But this is load, not save - the caller supplies the data. If
>> there's less data than can be fit, it'll be zero-extended. If
>> there's too much data, the excess you don't know what to do
>> with (it might be okay to tolerate it being all zero).
>>
> 
> But this is a callback. The outer load function iterates over
> the records calling the appropriate hander for each one. Those
> handlers then call this function saying how much data they
> expect and whether they want exactly that amount, or whether
> they can tolerate less (i.e. zero-extend). Hence
> len < c->desc.length is an error, because it means the
> descriptor contains more data than the hander knows how to
> handle.

Oh, I see - "But maybe I'm getting the sense of len wrong ..."
then indeed applies.

Any thoughts on tolerating the excess data being zero?

Jan

Paul Durrant May 7, 2020, 7:45 a.m. UTC | #9

> -----Original Message-----
> From: Jan Beulich <[e-mail]>
> Sent: 07 May 2020 08:40
> To: [e-mail]
> Cc: [e-mail]; 'Paul Durrant' <[e-mail]>; 'Andrew Cooper'
> <[e-mail]>; 'George Dunlap' <[e-mail]>; 'Ian Jackson'
> <[e-mail]>; 'Julien Grall' <[e-mail]>; 'Stefano Stabellini'
> <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Volodymyr Babchuk' <[e-mail]>;
> 'Roger Pau Monné' <[e-mail]>
> Subject: Re: [PATCH v2 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
> 
> On 07.05.2020 09:34, Paul Durrant wrote:
> >> From: Jan Beulich <[e-mail]>
> >> Sent: 07 May 2020 08:22
> >>
> >> On 06.05.2020 18:44, Paul Durrant wrote:
> >>>> From: Jan Beulich <[e-mail]>
> >>>> Sent: 29 April 2020 12:02
> >>>>
> >>>> On 07.04.2020 19:38, Paul Durrant wrote:
> >>>>> +int domain_load_begin(struct domain_context *c, unsigned int tc,
> >>>>> +                      const char *name, const struct vcpu *v, size_t len,
> >>>>> +                      bool exact)
> >>>>> +{
> >>>>> +    if ( c->log )
> >>>>> +        gdprintk(XENLOG_INFO, "%pv load: %s (%lu)\n", v, name,
> >>>>> +                 (unsigned long)len);
> >>>>> +
> >>>>> +    BUG_ON(tc != c->desc.typecode);
> >>>>> +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
> >>>>> +
> >>>>> +    if ( (exact && (len != c->desc.length)) ||
> >>>>> +         (len < c->desc.length) )
> >>>>> +        return -EINVAL;
> >>>>
> >>>> How about
> >>>>
> >>>>     if ( exact ? len != c->desc.length
> >>>>                : len < c->desc.length )
> >>>>
> >>>
> >>> Yes, that doesn't look too bad.
> >>>
> >>>> ? I'm also unsure about the < - don't you mean > instead? Too
> >>>> little data would be compensated by zero padding, but too
> >>>> much data can't be dealt with. But maybe I'm getting the sense
> >>>> of len wrong ...
> >>>
> >>> I think the < is correct. The caller needs to have at least enough
> >>> space to accommodate the context record.
> >>
> >> But this is load, not save - the caller supplies the data. If
> >> there's less data than can be fit, it'll be zero-extended. If
> >> there's too much data, the excess you don't know what to do
> >> with (it might be okay to tolerate it being all zero).
> >>
> >
> > But this is a callback. The outer load function iterates over
> > the records calling the appropriate hander for each one. Those
> > handlers then call this function saying how much data they
> > expect and whether they want exactly that amount, or whether
> > they can tolerate less (i.e. zero-extend). Hence
> > len < c->desc.length is an error, because it means the
> > descriptor contains more data than the hander knows how to
> > handle.
> 
> Oh, I see - "But maybe I'm getting the sense of len wrong ..."
> then indeed applies.
> 
> Any thoughts on tolerating the excess data being zero?
> 

Well the point of the check here is to not tolerate excess data... Are you suggesting that it might be a reasonable idea? If so, then yes, insisting it is all zero would be an alternative.

  Paul

Jan Beulich May 7, 2020, 8:17 a.m. UTC | #10

On 07.05.2020 09:45, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <[e-mail]>
>> Sent: 07 May 2020 08:40
>> To: [e-mail]
>> Cc: [e-mail]; 'Paul Durrant' <[e-mail]>; 'Andrew Cooper'
>> <[e-mail]>; 'George Dunlap' <[e-mail]>; 'Ian Jackson'
>> <[e-mail]>; 'Julien Grall' <[e-mail]>; 'Stefano Stabellini'
>> <[e-mail]>; 'Wei Liu' <[e-mail]>; 'Volodymyr Babchuk' <[e-mail]>;
>> 'Roger Pau Monné' <[e-mail]>
>> Subject: Re: [PATCH v2 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
>>
>> On 07.05.2020 09:34, Paul Durrant wrote:
>>>> From: Jan Beulich <[e-mail]>
>>>> Sent: 07 May 2020 08:22
>>>>
>>>> On 06.05.2020 18:44, Paul Durrant wrote:
>>>>>> From: Jan Beulich <[e-mail]>
>>>>>> Sent: 29 April 2020 12:02
>>>>>>
>>>>>> On 07.04.2020 19:38, Paul Durrant wrote:
>>>>>>> +int domain_load_begin(struct domain_context *c, unsigned int tc,
>>>>>>> +                      const char *name, const struct vcpu *v, size_t len,
>>>>>>> +                      bool exact)
>>>>>>> +{
>>>>>>> +    if ( c->log )
>>>>>>> +        gdprintk(XENLOG_INFO, "%pv load: %s (%lu)\n", v, name,
>>>>>>> +                 (unsigned long)len);
>>>>>>> +
>>>>>>> +    BUG_ON(tc != c->desc.typecode);
>>>>>>> +    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
>>>>>>> +
>>>>>>> +    if ( (exact && (len != c->desc.length)) ||
>>>>>>> +         (len < c->desc.length) )
>>>>>>> +        return -EINVAL;
>>>>>>
>>>>>> How about
>>>>>>
>>>>>>     if ( exact ? len != c->desc.length
>>>>>>                : len < c->desc.length )
>>>>>>
>>>>>
>>>>> Yes, that doesn't look too bad.
>>>>>
>>>>>> ? I'm also unsure about the < - don't you mean > instead? Too
>>>>>> little data would be compensated by zero padding, but too
>>>>>> much data can't be dealt with. But maybe I'm getting the sense
>>>>>> of len wrong ...
>>>>>
>>>>> I think the < is correct. The caller needs to have at least enough
>>>>> space to accommodate the context record.
>>>>
>>>> But this is load, not save - the caller supplies the data. If
>>>> there's less data than can be fit, it'll be zero-extended. If
>>>> there's too much data, the excess you don't know what to do
>>>> with (it might be okay to tolerate it being all zero).
>>>>
>>>
>>> But this is a callback. The outer load function iterates over
>>> the records calling the appropriate hander for each one. Those
>>> handlers then call this function saying how much data they
>>> expect and whether they want exactly that amount, or whether
>>> they can tolerate less (i.e. zero-extend). Hence
>>> len < c->desc.length is an error, because it means the
>>> descriptor contains more data than the hander knows how to
>>> handle.
>>
>> Oh, I see - "But maybe I'm getting the sense of len wrong ..."
>> then indeed applies.
>>
>> Any thoughts on tolerating the excess data being zero?
>>
> 
> Well the point of the check here is to not tolerate excess data...
> Are you suggesting that it might be a reasonable idea?

Well - it looks to be the obvious counterpart to zero-extending.
I'm not going to assert though that I've thought through all
possible consequences...

Jan

Julien Grall May 7, 2020, 8:35 a.m. UTC | #11

On 07/05/2020 08:21, Jan Beulich wrote:
> On 06.05.2020 18:44, Paul Durrant wrote:
>>>> +#define DOMAIN_SAVE_BEGIN(_x, _c, _v, _len) \
>>>> +        domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len))
>>>
>>> In new code I'd like to ask for no leading underscores on macro
>>> parameters as well as no unnecessary parenthesization of macro
>>> parameters (e.g. when they simply get passed on to another macro
>>> or function without being part of a larger expression).
>>
>> Personally I think it is generally good practice to parenthesize
>> but I can drop if you prefer.
> 
> To be honest - it's more than just "prefer": Excess parentheses
> hamper readability. There shouldn't be too few, and since macros
> already require quite a lot of them, imo we should strive to
> have exactly as many as are needed.

While I understand that too many parentheses may make the code worse, in 
the case of the macros, adding them for each argument is a good 
practice. This pretty simple to follow and avoid the mistake to forget 
to protect an argument correctly.

So I would let the contributor decides whether he wants to protect all 
the macro arguments or just as a need basis.

Cheers,

Jan Beulich May 7, 2020, 8:58 a.m. UTC | #12

On 07.05.2020 10:35, Julien Grall wrote:
> 
> 
> On 07/05/2020 08:21, Jan Beulich wrote:
>> On 06.05.2020 18:44, Paul Durrant wrote:
>>>>> +#define DOMAIN_SAVE_BEGIN(_x, _c, _v, _len) \
>>>>> +        domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len))
>>>>
>>>> In new code I'd like to ask for no leading underscores on macro
>>>> parameters as well as no unnecessary parenthesization of macro
>>>> parameters (e.g. when they simply get passed on to another macro
>>>> or function without being part of a larger expression).
>>>
>>> Personally I think it is generally good practice to parenthesize
>>> but I can drop if you prefer.
>>
>> To be honest - it's more than just "prefer": Excess parentheses
>> hamper readability. There shouldn't be too few, and since macros
>> already require quite a lot of them, imo we should strive to
>> have exactly as many as are needed.
> 
> While I understand that too many parentheses may make the code
> worse, in the case of the macros, adding them for each argument
> is a good practice. This pretty simple to follow and avoid the
> mistake to forget to protect an argument correctly.
> 
> So I would let the contributor decides whether he wants to
> protect all the macro arguments or just as a need basis.

This is a possible alternative position to take, accepting the
worse readability. But then this would please need applying in
full (as far as it's possible - operands to # or ## of course
can't be parenthesized):

#define DOMAIN_SAVE_BEGIN(x, c, v, len) \
        domain_save_begin((c), DOMAIN_SAVE_CODE((x)), #x, (v), (len))

which might look a little odd even to you?

As to readability - I'm sure you realize that from time to time
one needs to look at preprocessor output, where parentheses used
like this plus parentheses then also applied inside the invoked
macro add to one another. All in all I don't really buy the
"avoid the mistake to forget to protect an argument correctly"
argument to outweigh the downsides. We're doing this work not on
an occasional basis, but as a job. There should be a minimum
level of care everyone applies.

Jan

Julien Grall May 7, 2020, 9:31 a.m. UTC | #13

Hi,

On 07/05/2020 09:58, Jan Beulich wrote:
> On 07.05.2020 10:35, Julien Grall wrote:
>>
>>
>> On 07/05/2020 08:21, Jan Beulich wrote:
>>> On 06.05.2020 18:44, Paul Durrant wrote:
>>>>>> +#define DOMAIN_SAVE_BEGIN(_x, _c, _v, _len) \
>>>>>> +        domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len))
>>>>>
>>>>> In new code I'd like to ask for no leading underscores on macro
>>>>> parameters as well as no unnecessary parenthesization of macro
>>>>> parameters (e.g. when they simply get passed on to another macro
>>>>> or function without being part of a larger expression).
>>>>
>>>> Personally I think it is generally good practice to parenthesize
>>>> but I can drop if you prefer.
>>>
>>> To be honest - it's more than just "prefer": Excess parentheses
>>> hamper readability. There shouldn't be too few, and since macros
>>> already require quite a lot of them, imo we should strive to
>>> have exactly as many as are needed.
>>
>> While I understand that too many parentheses may make the code
>> worse, in the case of the macros, adding them for each argument
>> is a good practice. This pretty simple to follow and avoid the
>> mistake to forget to protect an argument correctly.
>>
>> So I would let the contributor decides whether he wants to
>> protect all the macro arguments or just as a need basis.
> 
> This is a possible alternative position to take, accepting the
> worse readability. But then this would please need applying in
> full (as far as it's possible - operands to # or ## of course
> can't be parenthesized):
> 
> #define DOMAIN_SAVE_BEGIN(x, c, v, len) \
>          domain_save_begin((c), DOMAIN_SAVE_CODE((x)), #x, (v), (len))
> 
> which might look a little odd even to you?

One pair of parenthesis looks wasteful but it doesn't bother that much.

> 
> As to readability - I'm sure you realize that from time to time
> one needs to look at preprocessor output, where parentheses used
> like this plus parentheses then also applied inside the invoked
> macro add to one another. All in all I don't really buy the
> "avoid the mistake to forget to protect an argument correctly"
> argument to outweigh the downsides. We're doing this work not on
> an occasional basis, but as a job. There should be a minimum
> level of care everyone applies.

I am sure you heard about "human error" in the past. Even if you do 
something all day along, you will make a mistake one day. By requesting 
a contributor to limit the number of parenthesis, then you increase the 
chance he/she will screw up one day.

You might think reviewers will catch such error, but XSA-316 proved that 
it is possible to miss it. I was the author of the patch and you were 
one the reviewer. As you can see even an experienced contributor and 
reviewer can make mistake... we are all human after all ;).

I don't ask to be over-proctective, but we should not lay trap to other 
contributors for them to fall into accidentally. In this case, Paul 
thinks the parentheses will help him. So I would not impose him to 
remove them and possibly make a mistake.

Cheers,

Patch

diff --git a/xen/common/Makefile b/xen/common/Makefile
index e8cde65370..90553ba5d7 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -37,6 +37,7 @@  obj-y += radix-tree.o
 obj-y += rbtree.o
 obj-y += rcupdate.o
 obj-y += rwlock.o
+obj-y += save.o
 obj-y += shutdown.o
 obj-y += softirq.o
 obj-y += sort.o
diff --git a/xen/common/save.c b/xen/common/save.c
new file mode 100644
index 0000000000..6cdac3785b
--- /dev/null
+++ b/xen/common/save.c
@@ -0,0 +1,329 @@ 
+/*
+ * 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/save.h>
+
+union domain_copy_entry {
+    domain_write_entry write;
+    domain_read_entry read;
+};
+
+struct domain_context {
+    bool log;
+    struct domain_save_descriptor desc;
+    size_t data_len;
+    union domain_copy_entry copy;
+    void *priv;
+};
+
+static struct {
+    const char *name;
+    bool per_vcpu;
+    domain_save_handler save;
+    domain_load_handler load;
+} handlers[DOMAIN_SAVE_CODE_MAX + 1];
+
+void __init domain_register_save_type(unsigned int tc, const char *name,
+                                      bool per_vcpu,
+                                      domain_save_handler save,
+                                      domain_load_handler load)
+{
+    BUG_ON(tc > ARRAY_SIZE(handlers));
+
+    ASSERT(!handlers[tc].save);
+    ASSERT(!handlers[tc].load);
+
+    handlers[tc].name = name;
+    handlers[tc].per_vcpu = per_vcpu;
+    handlers[tc].save = save;
+    handlers[tc].load = load;
+}
+
+int domain_save_begin(struct domain_context *c, unsigned int tc,
+                      const char *name, const struct vcpu *v, size_t len)
+{
+    int rc;
+
+    if ( c->log )
+        gdprintk(XENLOG_INFO, "%pv save: %s (%lu)\n", v, name,
+                 (unsigned long)len);
+
+    BUG_ON(tc != c->desc.typecode);
+    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
+
+    ASSERT(!c->data_len);
+    c->data_len = c->desc.length = len;
+
+    rc = c->copy.write(c->priv, &c->desc, sizeof(c->desc));
+    if ( rc )
+        return rc;
+
+    c->desc.length = 0;
+
+    return 0;
+}
+
+int domain_save_data(struct domain_context *c, const void *src, size_t len)
+{
+    if ( c->desc.length + len > c->data_len )
+        return -ENOSPC;
+
+    c->desc.length += len;
+
+    return c->copy.write(c->priv, src, len);
+}
+
+int domain_save_end(struct domain_context *c)
+{
+    /*
+     * If desc.length does not match the length specified in
+     * domain_save_begin(), there should have been more data.
+     */
+    if ( c->desc.length != c->data_len )
+        return -EIO;
+
+    c->data_len = 0;
+
+    return 0;
+}
+
+int domain_save(struct domain *d, domain_write_entry write, void *priv,
+                bool dry_run)
+{
+    struct domain_context c = {
+        .copy.write = write,
+        .priv = priv,
+        .log = !dry_run,
+    };
+    struct domain_save_header h = {
+        .magic = DOMAIN_SAVE_MAGIC,
+        .version = DOMAIN_SAVE_VERSION,
+    };
+    struct domain_save_header e;
+    unsigned int i;
+    int rc;
+
+    ASSERT(d != current->domain);
+
+    if ( d->is_dying )
+        return -EINVAL;
+
+    domain_pause(d);
+
+    c.desc.typecode = DOMAIN_SAVE_CODE(HEADER);
+
+    rc = DOMAIN_SAVE_ENTRY(HEADER, &c, d->vcpu[0], &h, sizeof(h));
+    if ( rc )
+        goto out;
+
+    for ( i = 0; i < ARRAY_SIZE(handlers); i++ )
+    {
+        domain_save_handler save = handlers[i].save;
+
+        if ( !save )
+            continue;
+
+        memset(&c.desc, 0, sizeof(c.desc));
+        c.desc.typecode = i;
+
+        if ( handlers[i].per_vcpu )
+        {
+            struct vcpu *v;
+
+            for_each_vcpu ( d, v )
+            {
+                c.desc.vcpu_id = v->vcpu_id;
+
+                rc = save(v, &c, dry_run);
+                if ( rc )
+                    goto out;
+            }
+        }
+        else
+        {
+            rc = save(d->vcpu[0], &c, dry_run);
+            if ( rc )
+                goto out;
+        }
+    }
+
+    memset(&c.desc, 0, sizeof(c.desc));
+    c.desc.typecode = DOMAIN_SAVE_CODE(END);
+
+    rc = DOMAIN_SAVE_ENTRY(END, &c, d->vcpu[0], &e, 0);
+
+ out:
+    domain_unpause(d);
+
+    return rc;
+}
+
+int domain_load_begin(struct domain_context *c, unsigned int tc,
+                      const char *name, const struct vcpu *v, size_t len,
+                      bool exact)
+{
+    if ( c->log )
+        gdprintk(XENLOG_INFO, "%pv load: %s (%lu)\n", v, name,
+                 (unsigned long)len);
+
+    BUG_ON(tc != c->desc.typecode);
+    BUG_ON(v->vcpu_id != c->desc.vcpu_id);
+
+    if ( (exact && (len != c->desc.length)) ||
+         (len < c->desc.length) )
+        return -EINVAL;
+
+    ASSERT(!c->data_len);
+    c->data_len = len;
+
+    return 0;
+}
+
+int domain_load_data(struct domain_context *c, void *dst, size_t len)
+{
+    size_t copy_len = min_t(size_t, len, c->desc.length);
+    int rc;
+
+    if ( c->data_len < len )
+        return -ENODATA;
+
+    c->data_len -= len;
+    c->desc.length -= copy_len;
+
+    rc = c->copy.read(c->priv, dst, copy_len);
+    if ( rc )
+        return rc;
+
+    /* Zero extend if the descriptor is exhausted */
+    len -= copy_len;
+    if ( len )
+    {
+        dst += copy_len;
+        memset(dst, 0, len);
+    }
+
+    return 0;
+}
+
+int domain_load_end(struct domain_context *c)
+{
+    /* If data_len is non-zero there is unread data */
+    if ( c->data_len )
+        return -EIO;
+
+    return 0;
+}
+
+int domain_load(struct domain *d, domain_read_entry read, void *priv)
+{
+    struct domain_context c = {
+        .copy.read = read,
+        .priv = priv,
+        .log = true,
+    };
+    struct domain_save_header h;
+    int rc;
+
+    ASSERT(d != current->domain);
+
+    if ( d->is_dying )
+        return -EINVAL;
+
+    rc = c.copy.read(c.priv, &c.desc, sizeof(c.desc));
+    if ( rc )
+        return rc;
+
+    if ( c.desc.typecode != DOMAIN_SAVE_CODE(HEADER) || c.desc.vcpu_id ||
+         c.desc.flags )
+        return -EINVAL;
+
+    rc = DOMAIN_LOAD_ENTRY(HEADER, &c, d->vcpu[0], &h, sizeof(h), true);
+    if ( rc )
+        return rc;
+
+    if ( h.magic != DOMAIN_SAVE_MAGIC || h.version != DOMAIN_SAVE_VERSION )
+        return -EINVAL;
+
+    domain_pause(d);
+
+    for (;;)
+    {
+        unsigned int i;
+        unsigned int flags;
+        domain_load_handler load;
+        struct vcpu *v;
+
+        rc = c.copy.read(c.priv, &c.desc, sizeof(c.desc));
+        if ( rc )
+            break;
+
+        rc = -EINVAL;
+
+        flags = c.desc.flags;
+        if ( flags & ~DOMAIN_SAVE_FLAG_IGNORE )
+            break;
+
+        if ( c.desc.typecode == DOMAIN_SAVE_CODE(END) ) {
+            if ( !(flags & DOMAIN_SAVE_FLAG_IGNORE) )
+                rc = DOMAIN_LOAD_ENTRY(END, &c, d->vcpu[0], NULL, 0, true);
+
+            break;
+        }
+
+        i = c.desc.typecode;
+        if ( i >= ARRAY_SIZE(handlers) )
+            break;
+
+        if ( (!handlers[i].per_vcpu && c.desc.vcpu_id) ||
+             (c.desc.vcpu_id >= d->max_vcpus) )
+            break;
+
+        v = d->vcpu[c.desc.vcpu_id];
+
+        if ( flags & DOMAIN_SAVE_FLAG_IGNORE )
+        {
+            /* Sink the data */
+            rc = domain_load_entry(&c, c.desc.typecode, "IGNORED",
+                                   v, NULL, c.desc.length, true);
+            if ( rc )
+                break;
+
+            continue;
+        }
+
+        load = handlers[i].load;
+
+        rc = load ? load(v, &c) : -EOPNOTSUPP;
+        if ( rc )
+            break;
+    }
+
+    domain_unpause(d);
+
+    return rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/public/arch-arm/hvm/save.h b/xen/include/public/arch-arm/hvm/save.h
index 75b8e65bcb..d5b0c15203 100644
--- a/xen/include/public/arch-arm/hvm/save.h
+++ b/xen/include/public/arch-arm/hvm/save.h
@@ -26,6 +26,11 @@ 
 #ifndef __XEN_PUBLIC_HVM_SAVE_ARM_H__
 #define __XEN_PUBLIC_HVM_SAVE_ARM_H__
 
+/*
+ * Further use of HVM state is deprecated. New state records should only
+ * be added to the domain state header: public/save.h
+ */
+
 #endif
 
 /*
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 773a380bc2..e61e2dbcd7 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -648,6 +648,11 @@  struct hvm_msr {
  */
 #define HVM_SAVE_CODE_MAX 20
 
+/*
+ * Further use of HVM state is deprecated. New state records should only
+ * be added to the domain state header: public/save.h
+ */
+
 #endif /* __XEN_PUBLIC_HVM_SAVE_X86_H__ */
 
 /*
diff --git a/xen/include/public/save.h b/xen/include/public/save.h
new file mode 100644
index 0000000000..7e5f8752bd
--- /dev/null
+++ b/xen/include/public/save.h
@@ -0,0 +1,84 @@ 
+/*
+ * 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"
+
+/* Each entry is preceded by a descriptor */
+struct domain_save_descriptor {
+    uint16_t typecode;
+    /*
+     * Each entry will contain either to global or per-vcpu domain state.
+     * Entries relating to global state should have zero in this field.
+     */
+    uint16_t vcpu_id;
+    uint32_t flags;
+    /*
+     * When restoring state this flag can be set in a descriptor to cause
+     * its content to be ignored.
+     *
+     * NOTE: It is invalid to set this flag for HEADER or END records (see
+     *       below).
+     */
+#define _DOMAIN_SAVE_FLAG_IGNORE 0
+#define DOMAIN_SAVE_FLAG_IGNORE (1u << _DOMAIN_SAVE_FLAG_IGNORE)
+
+    /* Entry length not including this descriptor */
+    uint64_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; };
+
+#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)
+
+/* Terminating entry */
+struct domain_save_end {};
+DECLARE_DOMAIN_SAVE_TYPE(END, 0, struct domain_save_end);
+
+#define DOMAIN_SAVE_MAGIC   0x53415645
+#define DOMAIN_SAVE_VERSION 0x00000001
+
+/* Initial entry */
+struct domain_save_header {
+    uint32_t magic;             /* Must be DOMAIN_SAVE_MAGIC */
+    uint32_t version;           /* Save format version */
+};
+DECLARE_DOMAIN_SAVE_TYPE(HEADER, 1, struct domain_save_header);
+
+#define DOMAIN_SAVE_CODE_MAX 1
+
+#endif /* __XEN_PUBLIC_SAVE_H__ */
diff --git a/xen/include/xen/save.h b/xen/include/xen/save.h
new file mode 100644
index 0000000000..879bbb4390
--- /dev/null
+++ b/xen/include/xen/save.h
@@ -0,0 +1,152 @@ 
+/*
+ * 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/sched.h>
+#include <xen/types.h>
+#include <xen/init.h>
+
+#include <public/xen.h>
+#include <public/save.h>
+
+struct domain_context;
+
+int domain_save_begin(struct domain_context *c, unsigned int tc,
+                      const char *name, const struct vcpu *v, size_t len);
+
+#define DOMAIN_SAVE_BEGIN(_x, _c, _v, _len) \
+        domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len))
+
+int domain_save_data(struct domain_context *c, const void *data, size_t len);
+int domain_save_end(struct domain_context *c);
+
+static inline int domain_save_entry(struct domain_context *c,
+                                    unsigned int tc, const char *name,
+                                    const struct vcpu *v, const void *src,
+                                    size_t len)
+{
+    int rc;
+
+    rc = domain_save_begin(c, tc, name, v, len);
+    if ( rc )
+        return rc;
+
+    rc = domain_save_data(c, src, len);
+    if ( rc )
+        return rc;
+
+    return domain_save_end(c);
+}
+
+#define DOMAIN_SAVE_ENTRY(_x, _c, _v, _src, _len) \
+    domain_save_entry((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_src), (_len))
+
+int domain_load_begin(struct domain_context *c, unsigned int tc,
+                      const char *name, const struct vcpu *v, size_t len,
+                      bool exact);
+
+#define DOMAIN_LOAD_BEGIN(_x, _c, _v, _len, _exact) \
+        domain_load_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_len), \
+                          (_exact));
+
+int domain_load_data(struct domain_context *c, void *data, size_t len);
+int domain_load_end(struct domain_context *c);
+
+static inline int domain_load_entry(struct domain_context *c,
+                                    unsigned int tc, const char *name,
+                                    const struct vcpu *v, void *dst,
+                                    size_t len, bool exact)
+{
+    int rc;
+
+    rc = domain_load_begin(c, tc, name, v, len, exact);
+    if ( rc )
+        return rc;
+
+    rc = domain_load_data(c, dst, len);
+    if ( rc )
+        return rc;
+
+    return domain_load_end(c);
+}
+
+#define DOMAIN_LOAD_ENTRY(_x, _c, _v, _dst, _len, _exact) \
+    domain_load_entry((_c), DOMAIN_SAVE_CODE(_x), #_x, (_v), (_dst), (_len), \
+                          (_exact))
+
+/*
+ * The 'dry_run' flag indicates that the caller of domain_save() (see
+ * below) is not trying to actually acquire the data, only the size
+ * of the data. The save handler can therefore limit work to only that
+ * which is necessary to call DOMAIN_SAVE_BEGIN/ENTRY() with an accurate
+ * value for '_len'.
+ */
+typedef int (*domain_save_handler)(const struct vcpu *v,
+                                   struct domain_context *h,
+                                   bool dry_run);
+typedef int (*domain_load_handler)(struct vcpu *v,
+                                   struct domain_context *h);
+
+void domain_register_save_type(unsigned int tc, const char *name,
+                               bool per_vcpu,
+                               domain_save_handler save,
+                               domain_load_handler load);
+
+/*
+ * Register save and restore handlers. Save handlers will be invoked
+ * in order of DOMAIN_SAVE_CODE().
+ */
+#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _per_vcpu, _save, _load) \
+static int __init __domain_register_##_x##_save_restore(void)     \
+{                                                                 \
+    domain_register_save_type(                                    \
+        DOMAIN_SAVE_CODE(_x),                                     \
+        #_x,                                                      \
+        (_per_vcpu),                                              \
+        &(_save),                                                 \
+        &(_load));                                                \
+                                                                  \
+    return 0;                                                     \
+}                                                                 \
+__initcall(__domain_register_##_x##_save_restore);
+
+/* Copy callback functions */
+typedef int (*domain_write_entry)(void *priv, const void *data, size_t len);
+typedef int (*domain_read_entry)(void *priv, void *data, size_t len);
+
+/*
+ * Entry points:
+ *
+ * int domain_save(struct domain *d, domain_copy_entry copy, void *priv,
+ *                 bool dry_run);
+ * int domain_load(struct domain *d, domain_copy_entry copy, void *priv);
+ *
+ * write/read: This is a callback function 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.
+ * 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.
+ */
+int domain_save(struct domain *d, domain_write_entry write, void *priv,
+                bool dry_run);
+int domain_load(struct domain *d, domain_read_entry read, void *priv);
+
+#endif /* __XEN_SAVE_H__ */