> -----Original Message-----
> From: Andrew Cooper <andrew.coop...@citrix.com>
> Sent: 02 October 2020 22:20
> To: Paul Durrant <p...@xen.org>; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurr...@amazon.com>; Julien Grall <jul...@xen.org>; Jan
> Beulich
> <jbeul...@suse.com>; George Dunlap <george.dun...@citrix.com>; Ian Jackson
> <ian.jack...@eu.citrix.com>; Stefano Stabellini <sstabell...@kernel.org>; Wei
> Liu <w...@xen.org>;
> Volodymyr Babchuk <volodymyr_babc...@epam.com>; Roger Pau Monné
> <roger....@citrix.com>
> 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
