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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Sent: 02 October 2020 22:20
> To: Paul Durrant <paul@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Paul Durrant <pdurrant@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Jan 
> Beulich
> <jbeulich@xxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>; Ian Jackson
> <ian.jackson@xxxxxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei 
> Liu <wl@xxxxxxx>;
> Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>; Roger Pau Monné 
> <roger.pau@xxxxxxxxxx>
> Subject: Re: [PATCH v9 1/8] xen/common: introduce a new framework for 
> save/restore of 'domain' context
> 
> On 24/09/2020 14:10, Paul Durrant wrote:
> > diff --git a/xen/common/save.c b/xen/common/save.c
> > new file mode 100644
> > index 0000000000..841c4d0e4e
> > --- /dev/null
> > +++ b/xen/common/save.c
> > @@ -0,0 +1,315 @@
> > +/*
> > + * save.c: Save and restore PV guest state common to all domain types.
> 
> This description will be stale by the time your work is complete.
> 

True now, I'll just drop the 'PV'

> > +int domain_save_data(struct domain_context *c, const void *src, size_t len)
> > +{
> > +    int rc = c->ops.save->append(c->priv, src, len);
> > +
> > +    if ( !rc )
> > +        c->len += len;
> > +
> > +    return rc;
> > +}
> > +
> > +#define DOMAIN_SAVE_ALIGN 8
> 
> This is part of the stream ABI.
> 

And what's actually the problem with defining it here?

> > +
> > +int domain_save_end(struct domain_context *c)
> > +{
> > +    struct domain *d = c->domain;
> > +    size_t len = ROUNDUP(c->len, DOMAIN_SAVE_ALIGN) - c->len; /* padding */
> 
> DOMAIN_SAVE_ALIGN - (c->len & (DOMAIN_SAVE_ALIGN - 1))
> 
> isn't vulnerable to overflow.
> 

...and significantly uglier code. What's actually wrong with what I wrote?

> > +    int rc;
> > +
> > +    if ( len )
> > +    {
> > +        static const uint8_t pad[DOMAIN_SAVE_ALIGN] = {};
> > +
> > +        rc = domain_save_data(c, pad, len);
> > +
> > +        if ( rc )
> > +            return rc;
> > +    }
> > +    ASSERT(IS_ALIGNED(c->len, DOMAIN_SAVE_ALIGN));
> > +
> > +    if ( c->name )
> > +        gdprintk(XENLOG_INFO, "%pd save: %s[%u] +%zu (-%zu)\n", d, c->name,
> > +                 c->desc.instance, c->len, len);
> 
> IMO, this is unhelpful to print out.  It also appears to be the only use
> of the c->name field.
> 
> It also creates obscure and hard to follow logic based on dry_run.
> 

I'll drop it to debug. I personally find it helpful and would prefer to keep it.

> > diff --git a/xen/include/public/save.h b/xen/include/public/save.h
> > new file mode 100644
> > index 0000000000..551dbbddb8
> > --- /dev/null
> > +++ b/xen/include/public/save.h
> > @@ -0,0 +1,89 @@
> > +/*
> > + * 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
> > +
> > +#if defined(__XEN__) || defined(__XEN_TOOLS__)
> > +
> > +#include "xen.h"
> > +
> > +/* Entry data is preceded by a descriptor */
> > +struct domain_save_descriptor {
> > +    uint16_t typecode;
> > +
> > +    /*
> > +     * Instance number of the entry (since there may be multiple of some
> > +     * types of entries).
> > +     */
> > +    uint16_t instance;
> > +
> > +    /* Entry length not including this descriptor */
> > +    uint32_t length;
> > +};
> > +
> > +/*
> > + * Each entry has a type associated with it. DECLARE_DOMAIN_SAVE_TYPE
> > + * binds these things together, although it is not intended that the
> > + * resulting type is ever instantiated.
> > + */
> > +#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)
> 
> I realise this is going to make me very unpopular, but NACK.
> 
> This is straight up obfuscation with no redeeming properties.  I know
> you've copied it from the exist HVMCONTEXT infrastructure, but it is
> obnoxious to use there (particularly in the domain builder) and not an
> example wanting copying.
> 
> Furthermore, the code will be simpler and easier to follow without it.
> 

OK, I can drop it if you so vehemently object.

> Secondly, and more importantly, I do not see anything in docs/specs/
> describing the binary format of this stream,  and I'm going to insist
> that one appears, ahead of this patch in the series.
> 

I can certainly put something there if you wish.

> In doing so, you're hopefully going to discover the bug with the older
> HVMCONTEXT stream which makes the version field fairly pointless (more
> below).
> 
> It should describe how to forward compatibly extend the stream, and
> under what circumstances the version number can/should change.  It also
> needs to describe the alignment and extending rules which ...
> 
> > +
> > +/*
> > + * All entries will be zero-padded to the next 64-bit boundary when saved,
> > + * so there is no need to include trailing pad fields in structure
> > + * definitions.
> > + * When loading, entries will be zero-extended if the load handler reads
> > + * beyond the length specified in the descriptor.
> > + */
> 
> ... shouldn't be this.
> 

Auto-padding was explicitly requested by Julien and extending (with zeroes or 
otherwise) is the necessary corollary (since the save handlers are not 
explicitly padding to the alignment boundary).

> The current zero extending property was an emergency hack to fix an ABI
> breakage which had gone unnoticed for a couple of releases.  The work to
> implement it created several very hard to debug breakages in Xen.
> 
> A properly designed stream shouldn't need auto-extending behaviour, and
> the legibility of the code is improved by not having it.
> 
> It is a trick which can stay up your sleeve for an emergency, in the
> hope you'll never have to use it.
> 

The zero-extending here is different; it does not form part of the record. It 
is merely there to make sure the alignment constraint is met.

> > +
> > +/* 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 */
> > +    uint16_t xen_major, xen_minor; /* Xen version */
> > +    uint32_t version;              /* Save format version */
> > +};
> > +DECLARE_DOMAIN_SAVE_TYPE(HEADER, 1, struct domain_save_header);
> 
> The layout problem with the stream is the fact that this header doesn't
> come first.
> 

? It most certainly does some first as is evident from the load and save 
functions. But I will add a document that states it, as requested.

> In the eventual future where uint16_t won't be sufficient for instance,
> and uint32_t might not be sufficient for len, the version number is
> going to have to be bumped, in order to change the descriptor layout.
> 
> 
> Overall, this patch needs to be a minimum of two.  First a written
> document which is the authoritative stream ABI, and the second which is
> this implementation.  The header describing the stream format should not
> be substantively different from xg_sr_stream_format.h
> 

Ok.

> ~Andrew
> 
> P.S. Another good reason for having extremely simple header files is for
> the poor sole trying to write a Go/Rust/other binding for this in some
> likely not-to-distant future.

Fine. I'm happy to drop the macro/type magic if no-one feels it is necessary.

  Paul