* [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context
2020-09-24 13:10 [PATCH v9 0/8] domain context infrastructure Paul Durrant
@ 2020-09-24 13:10 ` Paul Durrant
2020-10-02 21:20 ` Andrew Cooper
2020-10-02 22:00 ` Andrew Cooper
2020-09-24 13:10 ` [PATCH v9 2/8] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext Paul Durrant
` (7 subsequent siblings)
8 siblings, 2 replies; 41+ messages in thread
From: Paul Durrant @ 2020-09-24 13:10 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Julien Grall, Jan Beulich, Andrew Cooper,
George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Volodymyr Babchuk, Roger Pau Monné
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 <pdurrant@amazon.com>
Acked-by: Julien Grall <julien@xen.org>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
v7:
- Add an option to domain_load_end() to ignore unconsumed data, which will
be needed by a subsequent patch
- Kept acks since the modification is very small
v4:
- Addressed further comments from Jan
v3:
- Addressed comments from Julien and Jan
- Save handlers no longer need to state entry length up-front
- Save handlers expected to deal with multiple instances internally
- Entries are now auto-padded to 8 byte boundary
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 | 315 +++++++++++++++++++++++++
xen/include/public/arch-arm/hvm/save.h | 5 +
xen/include/public/arch-x86/hvm/save.h | 5 +
xen/include/public/save.h | 89 +++++++
xen/include/xen/save.h | 170 +++++++++++++
6 files changed, 585 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 b3b60a1ba2..3e6f21714a 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..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.
+ *
+ * Copyright Amazon.com Inc. or its affiliates.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/compile.h>
+#include <xen/save.h>
+
+struct domain_context {
+ struct domain *domain;
+ const char *name; /* for logging purposes */
+ struct domain_save_descriptor desc;
+ size_t len; /* for internal accounting */
+ union {
+ const struct domain_save_ops *save;
+ const struct domain_load_ops *load;
+ } ops;
+ void *priv;
+};
+
+static struct {
+ const char *name;
+ domain_save_handler save;
+ domain_load_handler load;
+} handlers[DOMAIN_SAVE_CODE_MAX + 1];
+
+void __init domain_register_save_type(unsigned int typecode,
+ const char *name,
+ domain_save_handler save,
+ domain_load_handler load)
+{
+ BUG_ON(typecode >= ARRAY_SIZE(handlers));
+
+ ASSERT(!handlers[typecode].save);
+ ASSERT(!handlers[typecode].load);
+
+ handlers[typecode].name = name;
+ handlers[typecode].save = save;
+ handlers[typecode].load = load;
+}
+
+int domain_save_begin(struct domain_context *c, unsigned int typecode,
+ unsigned int instance)
+{
+ int rc;
+
+ if ( typecode != c->desc.typecode )
+ {
+ ASSERT_UNREACHABLE();
+ return -EINVAL;
+ }
+ ASSERT(!c->desc.length); /* Should always be zero during domain_save() */
+ ASSERT(!c->len); /* Verify domain_save_end() was called */
+
+ rc = c->ops.save->begin(c->priv, &c->desc);
+ if ( rc )
+ return rc;
+
+ return 0;
+}
+
+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
+
+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 */
+ 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);
+
+ rc = c->ops.save->end(c->priv, c->len);
+ c->len = 0;
+
+ return rc;
+}
+
+int domain_save(struct domain *d, const struct domain_save_ops *ops,
+ void *priv, bool dry_run)
+{
+ struct domain_context c = {
+ .domain = d,
+ .ops.save = ops,
+ .priv = priv,
+ };
+ static const struct domain_save_header h = {
+ .magic = DOMAIN_SAVE_MAGIC,
+ .xen_major = XEN_VERSION,
+ .xen_minor = XEN_SUBVERSION,
+ .version = DOMAIN_SAVE_VERSION,
+ };
+ const struct domain_save_end e = {};
+ unsigned int i;
+ int rc;
+
+ ASSERT(d != current->domain);
+ domain_pause(d);
+
+ c.name = !dry_run ? "HEADER" : NULL;
+ c.desc.typecode = DOMAIN_SAVE_CODE(HEADER);
+
+ rc = DOMAIN_SAVE_ENTRY(HEADER, &c, 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;
+
+ c.name = !dry_run ? handlers[i].name : NULL;
+ memset(&c.desc, 0, sizeof(c.desc));
+ c.desc.typecode = i;
+
+ rc = save(d, &c, dry_run);
+ if ( rc )
+ goto out;
+ }
+
+ c.name = !dry_run ? "END" : NULL;
+ memset(&c.desc, 0, sizeof(c.desc));
+ c.desc.typecode = DOMAIN_SAVE_CODE(END);
+
+ rc = DOMAIN_SAVE_ENTRY(END, &c, 0, &e, sizeof(e));
+
+ out:
+ domain_unpause(d);
+
+ return rc;
+}
+
+int domain_load_begin(struct domain_context *c, unsigned int typecode,
+ unsigned int *instance)
+{
+ if ( typecode != c->desc.typecode )
+ {
+ ASSERT_UNREACHABLE();
+ return -EINVAL;
+ }
+
+ ASSERT(!c->len); /* Verify domain_load_end() was called */
+
+ *instance = c->desc.instance;
+
+ 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 - c->len);
+ int rc;
+
+ c->len += copy_len;
+ ASSERT(c->len <= c->desc.length);
+
+ rc = copy_len ? c->ops.load->read(c->priv, dst, copy_len) : 0;
+ if ( rc )
+ return rc;
+
+ /* Zero extend if the entry is exhausted */
+ len -= copy_len;
+ if ( len )
+ {
+ dst += copy_len;
+ memset(dst, 0, len);
+ }
+
+ return 0;
+}
+
+int domain_load_end(struct domain_context *c, bool ignore_data)
+{
+ struct domain *d = c->domain;
+ size_t len = c->desc.length - c->len;
+
+ while ( c->len != c->desc.length ) /* unconsumed data or pad */
+ {
+ uint8_t pad;
+ int rc = domain_load_data(c, &pad, sizeof(pad));
+
+ if ( rc )
+ return rc;
+
+ if ( !ignore_data && pad )
+ return -EINVAL;
+ }
+
+ ASSERT(c->name);
+ gdprintk(XENLOG_INFO, "%pd load: %s[%u] +%zu (-%zu)\n", d, c->name,
+ c->desc.instance, c->len, len);
+
+ c->len = 0;
+
+ return 0;
+}
+
+int domain_load(struct domain *d, const struct domain_load_ops *ops,
+ void *priv)
+{
+ struct domain_context c = {
+ .domain = d,
+ .ops.load = ops,
+ .priv = priv,
+ };
+ unsigned int instance;
+ struct domain_save_header h;
+ int rc;
+
+ ASSERT(d != current->domain);
+
+ rc = c.ops.load->read(c.priv, &c.desc, sizeof(c.desc));
+ if ( rc )
+ return rc;
+
+ c.name = "HEADER";
+
+ rc = DOMAIN_LOAD_ENTRY(HEADER, &c, &instance, &h, sizeof(h));
+ if ( rc )
+ return rc;
+
+ if ( instance || h.magic != DOMAIN_SAVE_MAGIC ||
+ h.version != DOMAIN_SAVE_VERSION )
+ return -EINVAL;
+
+ domain_pause(d);
+
+ for (;;)
+ {
+ unsigned int i;
+ domain_load_handler load;
+
+ rc = c.ops.load->read(c.priv, &c.desc, sizeof(c.desc));
+ if ( rc )
+ return rc;
+
+ rc = -EINVAL;
+
+ if ( c.desc.typecode == DOMAIN_SAVE_CODE(END) )
+ {
+ struct domain_save_end e;
+
+ c.name = "END";
+
+ rc = DOMAIN_LOAD_ENTRY(END, &c, &instance, &e, sizeof(e));
+
+ if ( instance )
+ return -EINVAL;
+
+ break;
+ }
+
+ i = c.desc.typecode;
+ if ( i >= ARRAY_SIZE(handlers) )
+ break;
+
+ c.name = handlers[i].name;
+ load = handlers[i].load;
+
+ rc = load ? load(d, &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..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)
+
+/*
+ * 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.
+ */
+
+/* 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);
+
+#define DOMAIN_SAVE_CODE_MAX 1
+
+#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
+
+#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..e631a2e85e
--- /dev/null
+++ b/xen/include/xen/save.h
@@ -0,0 +1,170 @@
+/*
+ * save.h: support routines for save/restore
+ *
+ * Copyright Amazon.com Inc. or its affiliates.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef XEN_SAVE_H
+#define XEN_SAVE_H
+
+#include <xen/init.h>
+#include <xen/sched.h>
+#include <xen/types.h>
+
+#include <public/save.h>
+
+struct domain_context;
+
+int domain_save_begin(struct domain_context *c, unsigned int typecode,
+ unsigned int instance);
+
+#define DOMAIN_SAVE_BEGIN(x, c, i) \
+ domain_save_begin((c), DOMAIN_SAVE_CODE(x), (i))
+
+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 typecode,
+ unsigned int instance, const void *src,
+ size_t len)
+{
+ int rc;
+
+ rc = domain_save_begin(c, typecode, instance);
+ 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, i, s, l) \
+ domain_save_entry((c), DOMAIN_SAVE_CODE(x), (i), (s), (l))
+
+int domain_load_begin(struct domain_context *c, unsigned int typecode,
+ unsigned int *instance);
+
+#define DOMAIN_LOAD_BEGIN(x, c, i) \
+ domain_load_begin((c), DOMAIN_SAVE_CODE(x), (i))
+
+int domain_load_data(struct domain_context *c, void *data, size_t len);
+int domain_load_end(struct domain_context *c, bool ignore_data);
+
+static inline int domain_load_entry(struct domain_context *c,
+ unsigned int typecode,
+ unsigned int *instance, void *dst,
+ size_t len)
+{
+ int rc;
+
+ rc = domain_load_begin(c, typecode, instance);
+ if ( rc )
+ return rc;
+
+ rc = domain_load_data(c, dst, len);
+ if ( rc )
+ return rc;
+
+ return domain_load_end(c, false);
+}
+
+#define DOMAIN_LOAD_ENTRY(x, c, i, d, l) \
+ domain_load_entry((c), DOMAIN_SAVE_CODE(x), (i), (d), (l))
+
+/*
+ * 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_data() the correct number of times with accurate values
+ * for 'len'.
+ */
+typedef int (*domain_save_handler)(const struct domain *d,
+ struct domain_context *c,
+ bool dry_run);
+typedef int (*domain_load_handler)(struct domain *d,
+ struct domain_context *c);
+
+void domain_register_save_type(unsigned int typecode, const char *name,
+ domain_save_handler save,
+ domain_load_handler load);
+
+/*
+ * Register save and load handlers.
+ *
+ * Save handlers will be invoked in an order which copes with any inter-
+ * entry dependencies. For now this means that HEADER will come first and
+ * END will come last, all others being invoked in order of 'typecode'.
+ *
+ * Load handlers will be invoked in the order of entries present in the
+ * buffer.
+ */
+#define DOMAIN_REGISTER_SAVE_LOAD(x, s, l) \
+ static int __init __domain_register_##x##_save_load(void) \
+ { \
+ domain_register_save_type( \
+ DOMAIN_SAVE_CODE(x), \
+ #x, \
+ &(s), \
+ &(l)); \
+ \
+ return 0; \
+ } \
+ __initcall(__domain_register_##x##_save_load);
+
+/* Callback functions */
+struct domain_save_ops {
+ /*
+ * Begin a new entry with the given descriptor (only type and instance
+ * are valid).
+ */
+ int (*begin)(void *priv, const struct domain_save_descriptor *desc);
+ /* Append data/padding to the buffer */
+ int (*append)(void *priv, const void *data, size_t len);
+ /*
+ * Complete the entry by updating the descriptor with the total
+ * length of the appended data (not including padding).
+ */
+ int (*end)(void *priv, size_t len);
+};
+
+struct domain_load_ops {
+ /* Read data/padding from the buffer */
+ int (*read)(void *priv, void *data, size_t len);
+};
+
+/*
+ * Entry points:
+ *
+ * ops: These are callback functions provided by the caller that will
+ * be used to write to (in the save case) or read from (in the
+ * load case) the context buffer. See above for more detail.
+ * priv: This is a pointer that will be passed to the copy function to
+ * allow it to identify the context buffer and the current state
+ * of the save or load operation.
+ * dry_run: If this is set then the caller of domain_save() is only trying
+ * to acquire the total size of the data, not the data itself.
+ * In this case the caller may supply different ops to avoid doing
+ * unnecessary work.
+ */
+int domain_save(struct domain *d, const struct domain_save_ops *ops,
+ void *priv, bool dry_run);
+int domain_load(struct domain *d, const struct domain_load_ops *ops,
+ void *priv);
+
+#endif /* XEN_SAVE_H */
--
2.20.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context
2020-09-24 13:10 ` [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context Paul Durrant
@ 2020-10-02 21:20 ` Andrew Cooper
2020-10-03 14:33 ` Wei Liu
2020-10-05 8:03 ` Paul Durrant
2020-10-02 22:00 ` Andrew Cooper
1 sibling, 2 replies; 41+ messages in thread
From: Andrew Cooper @ 2020-10-02 21:20 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Julien Grall, Jan Beulich, George Dunlap,
Ian Jackson, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
Roger Pau Monné
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.
> +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.
> +
> +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.
> + 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.
> 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.
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.
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.
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.
> +
> +/* 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.
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
~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.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context
2020-10-02 21:20 ` Andrew Cooper
@ 2020-10-03 14:33 ` Wei Liu
2020-10-05 8:03 ` Paul Durrant
1 sibling, 0 replies; 41+ messages in thread
From: Wei Liu @ 2020-10-03 14:33 UTC (permalink / raw)
To: Andrew Cooper
Cc: Paul Durrant, xen-devel, Paul Durrant, Julien Grall, Jan Beulich,
George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu,
Volodymyr Babchuk, Roger Pau Monné
On Fri, Oct 02, 2020 at 10:20:18PM +0100, Andrew Cooper wrote:
[...]
> 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.
For Rust the header is going to be generated by a tool called bindgen.
It doesn't like nested macros, so I would be all for a simpler C header
file if we can help it.
Wei.
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context
2020-10-02 21:20 ` Andrew Cooper
2020-10-03 14:33 ` Wei Liu
@ 2020-10-05 8:03 ` Paul Durrant
2020-10-13 11:44 ` Jan Beulich
1 sibling, 1 reply; 41+ messages in thread
From: Paul Durrant @ 2020-10-05 8:03 UTC (permalink / raw)
To: 'Andrew Cooper', xen-devel
Cc: 'Paul Durrant', 'Julien Grall',
'Jan Beulich', 'George Dunlap',
'Ian Jackson', 'Stefano Stabellini',
'Wei Liu', 'Volodymyr Babchuk',
'Roger Pau Monné'
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 02 October 2020 22:20
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>; Julien Grall <julien@xen.org>; Jan Beulich
> <jbeulich@suse.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Roger Pau Monné <roger.pau@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
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context
2020-10-05 8:03 ` Paul Durrant
@ 2020-10-13 11:44 ` Jan Beulich
0 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2020-10-13 11:44 UTC (permalink / raw)
To: paul
Cc: 'Andrew Cooper', xen-devel, 'Paul Durrant',
'Julien Grall', 'George Dunlap',
'Ian Jackson', 'Stefano Stabellini',
'Wei Liu', 'Volodymyr Babchuk',
'Roger Pau Monné'
On 05.10.2020 10:03, Paul Durrant wrote:
>> From: Andrew Cooper <andrew.cooper3@citrix.com>
>> Sent: 02 October 2020 22:20
>>
>> On 24/09/2020 14:10, Paul Durrant wrote:
>>> +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?
I don't think there's anything "wrong" or "vulnerable" here, but
I still can see Andrew's point. The "vulnerable" aspect applies
only in the (highly hypothetical I think) cases of either
sizeof(size_t) < sizeof(int) or size_t being a signed type, afaict.
But since it's easy (and imo not "significantly uglier") to write
code that is free of any wrapping or overflowing behavior, I
think it is sensible to actually write it that way.
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context
2020-09-24 13:10 ` [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context Paul Durrant
2020-10-02 21:20 ` Andrew Cooper
@ 2020-10-02 22:00 ` Andrew Cooper
1 sibling, 0 replies; 41+ messages in thread
From: Andrew Cooper @ 2020-10-02 22:00 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Julien Grall, Jan Beulich, George Dunlap,
Ian Jackson, Stefano Stabellini, Wei Liu, Volodymyr Babchuk,
Roger Pau Monné
On 24/09/2020 14:10, Paul Durrant wrote:
> +/*
> + * 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_data() the correct number of times with accurate values
> + * for 'len'.
> + */
> +typedef int (*domain_save_handler)(const struct domain *d,
> + struct domain_context *c,
> + bool dry_run);
Sorry - missed this the first time around. This cannot take a const domain.
Doing so prevents putting (amongst other things), event channel details
into the stream, because you won't be able to take the domain's event
lock, and having the domain paused isn't good enough protection.
Removing this const will reduce the churn in subsequent patches somewhat.
~Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v9 2/8] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
2020-09-24 13:10 [PATCH v9 0/8] domain context infrastructure Paul Durrant
2020-09-24 13:10 ` [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context Paul Durrant
@ 2020-09-24 13:10 ` Paul Durrant
2020-09-30 14:31 ` Wei Liu
2020-10-02 21:58 ` Andrew Cooper
2020-09-24 13:10 ` [PATCH v9 3/8] tools/misc: add xen-domctx to present domain context Paul Durrant
` (6 subsequent siblings)
8 siblings, 2 replies; 41+ messages in thread
From: Paul Durrant @ 2020-09-24 13:10 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Julien Grall, Daniel De Graaf, Ian Jackson,
Wei Liu, Andrew Cooper, George Dunlap, Jan Beulich,
Stefano Stabellini
These domctls provide a mechanism to get and set domain context from
the toolstack.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Julien Grall <julien@xen.org>
---
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
v4:
- Add missing zero pad checks
v3:
- Addressed comments from Julien and Jan
- Use vmalloc() rather than xmalloc_bytes()
v2:
- drop mask parameter
- const-ify some more buffers
---
tools/flask/policy/modules/xen.if | 4 +-
tools/libs/ctrl/include/xenctrl.h | 5 +
tools/libs/ctrl/xc_domain.c | 56 +++++++++
xen/common/domctl.c | 173 ++++++++++++++++++++++++++++
xen/include/public/domctl.h | 41 +++++++
xen/xsm/flask/hooks.c | 6 +
xen/xsm/flask/policy/access_vectors | 4 +
7 files changed, 287 insertions(+), 2 deletions(-)
diff --git a/tools/flask/policy/modules/xen.if b/tools/flask/policy/modules/xen.if
index 8eb2293a52..2bc9db4f64 100644
--- a/tools/flask/policy/modules/xen.if
+++ b/tools/flask/policy/modules/xen.if
@@ -53,7 +53,7 @@ define(`create_domain_common', `
allow $1 $2:domain2 { set_cpu_policy settsc setscheduler setclaim
set_vnumainfo get_vnumainfo cacheflush
psr_cmt_op psr_alloc soft_reset
- resource_map get_cpu_policy };
+ resource_map get_cpu_policy setcontext };
allow $1 $2:security check_context;
allow $1 $2:shadow enable;
allow $1 $2:mmu { map_read map_write adjust memorymap physmap pinpage mmuext_op updatemp };
@@ -97,7 +97,7 @@ define(`migrate_domain_out', `
allow $1 $2:hvm { gethvmc getparam };
allow $1 $2:mmu { stat pageinfo map_read };
allow $1 $2:domain { getaddrsize getvcpucontext pause destroy };
- allow $1 $2:domain2 gettsc;
+ allow $1 $2:domain2 { gettsc getcontext };
allow $1 $2:shadow { enable disable logdirty };
')
diff --git a/tools/libs/ctrl/include/xenctrl.h b/tools/libs/ctrl/include/xenctrl.h
index 73e9535fc8..f18b9b25d1 100644
--- a/tools/libs/ctrl/include/xenctrl.h
+++ b/tools/libs/ctrl/include/xenctrl.h
@@ -867,6 +867,11 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
uint8_t *hvm_ctxt,
uint32_t size);
+int xc_domain_getcontext(xc_interface *xch, uint32_t domid,
+ void *ctxt_buf, size_t *size);
+int xc_domain_setcontext(xc_interface *xch, uint32_t domid,
+ const void *ctxt_buf, size_t size);
+
/**
* This function will return guest IO ABI protocol
*
diff --git a/tools/libs/ctrl/xc_domain.c b/tools/libs/ctrl/xc_domain.c
index e7cea4a17d..e435e6e82b 100644
--- a/tools/libs/ctrl/xc_domain.c
+++ b/tools/libs/ctrl/xc_domain.c
@@ -536,6 +536,62 @@ int xc_domain_hvm_setcontext(xc_interface *xch,
return ret;
}
+int xc_domain_getcontext(xc_interface *xch, uint32_t domid,
+ void *ctxt_buf, size_t *size)
+{
+ int ret;
+ DECLARE_DOMCTL = {
+ .cmd = XEN_DOMCTL_getdomaincontext,
+ .domain = domid,
+ .u.getdomaincontext.size = *size,
+ };
+ DECLARE_HYPERCALL_BOUNCE(ctxt_buf, *size, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+ if ( xc_hypercall_bounce_pre(xch, ctxt_buf) )
+ return -1;
+
+ set_xen_guest_handle(domctl.u.setdomaincontext.buffer, ctxt_buf);
+
+ ret = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, ctxt_buf);
+
+ if ( ret )
+ return ret;
+
+ *size = domctl.u.getdomaincontext.size;
+ if ( *size != domctl.u.getdomaincontext.size )
+ {
+ errno = EOVERFLOW;
+ return -1;
+ }
+
+ return 0;
+}
+
+int xc_domain_setcontext(xc_interface *xch, uint32_t domid,
+ const void *ctxt_buf, size_t size)
+{
+ int ret;
+ DECLARE_DOMCTL = {
+ .cmd = XEN_DOMCTL_setdomaincontext,
+ .domain = domid,
+ .u.setdomaincontext.size = size,
+ };
+ DECLARE_HYPERCALL_BOUNCE_IN(ctxt_buf, size);
+
+ if ( xc_hypercall_bounce_pre(xch, ctxt_buf) )
+ return -1;
+
+ set_xen_guest_handle(domctl.u.setdomaincontext.buffer, ctxt_buf);
+
+ ret = do_domctl(xch, &domctl);
+
+ xc_hypercall_bounce_post(xch, ctxt_buf);
+
+ return ret;
+}
+
int xc_vcpu_getcontext(xc_interface *xch,
uint32_t domid,
uint32_t vcpu,
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index af044e2eda..ac76045354 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -25,6 +25,8 @@
#include <xen/hypercall.h>
#include <xen/vm_event.h>
#include <xen/monitor.h>
+#include <xen/save.h>
+#include <xen/vmap.h>
#include <asm/current.h>
#include <asm/irq.h>
#include <asm/page.h>
@@ -273,6 +275,168 @@ static struct vnuma_info *vnuma_init(const struct xen_domctl_vnuma *uinfo,
return ERR_PTR(ret);
}
+struct domctl_context
+{
+ void *buffer;
+ struct domain_save_descriptor *desc;
+ size_t len;
+ size_t cur;
+};
+
+static int dry_run_append(void *priv, const void *data, size_t len)
+{
+ struct domctl_context *c = priv;
+
+ if ( c->len + len < c->len )
+ return -EOVERFLOW;
+
+ c->len += len;
+
+ return 0;
+}
+
+static int dry_run_begin(void *priv, const struct domain_save_descriptor *desc)
+{
+ return dry_run_append(priv, NULL, sizeof(*desc));
+}
+
+static int dry_run_end(void *priv, size_t len)
+{
+ return 0;
+}
+
+static struct domain_save_ops dry_run_ops = {
+ .begin = dry_run_begin,
+ .append = dry_run_append,
+ .end = dry_run_end,
+};
+
+static int save_begin(void *priv, const struct domain_save_descriptor *desc)
+{
+ struct domctl_context *c = priv;
+
+ if ( c->len - c->cur < sizeof(*desc) )
+ return -ENOSPC;
+
+ c->desc = c->buffer + c->cur; /* stash pointer to descriptor */
+ *c->desc = *desc;
+
+ c->cur += sizeof(*desc);
+
+ return 0;
+}
+
+static int save_append(void *priv, const void *data, size_t len)
+{
+ struct domctl_context *c = priv;
+
+ if ( c->len - c->cur < len )
+ return -ENOSPC;
+
+ memcpy(c->buffer + c->cur, data, len);
+ c->cur += len;
+
+ return 0;
+}
+
+static int save_end(void *priv, size_t len)
+{
+ struct domctl_context *c = priv;
+
+ c->desc->length = len;
+
+ return 0;
+}
+
+static struct domain_save_ops save_ops = {
+ .begin = save_begin,
+ .append = save_append,
+ .end = save_end,
+};
+
+static int getdomaincontext(struct domain *d,
+ struct xen_domctl_getdomaincontext *gdc)
+{
+ struct domctl_context c = { .buffer = ZERO_BLOCK_PTR };
+ int rc;
+
+ if ( d == current->domain )
+ return -EPERM;
+
+ if ( gdc->pad )
+ return -EINVAL;
+
+ if ( guest_handle_is_null(gdc->buffer) ) /* query for buffer size */
+ {
+ if ( gdc->size )
+ return -EINVAL;
+
+ /* dry run to acquire buffer size */
+ rc = domain_save(d, &dry_run_ops, &c, true);
+ if ( rc )
+ return rc;
+
+ gdc->size = c.len;
+ return 0;
+ }
+
+ c.len = gdc->size;
+ c.buffer = vmalloc(c.len);
+ if ( !c.buffer )
+ return -ENOMEM;
+
+ rc = domain_save(d, &save_ops, &c, false);
+
+ gdc->size = c.cur;
+ if ( !rc && copy_to_guest(gdc->buffer, c.buffer, gdc->size) )
+ rc = -EFAULT;
+
+ vfree(c.buffer);
+
+ return rc;
+}
+
+static int load_read(void *priv, void *data, size_t len)
+{
+ struct domctl_context *c = priv;
+
+ if ( c->len - c->cur < len )
+ return -ENODATA;
+
+ memcpy(data, c->buffer + c->cur, len);
+ c->cur += len;
+
+ return 0;
+}
+
+static struct domain_load_ops load_ops = {
+ .read = load_read,
+};
+
+static int setdomaincontext(struct domain *d,
+ const struct xen_domctl_setdomaincontext *sdc)
+{
+ struct domctl_context c = { .buffer = ZERO_BLOCK_PTR, .len = sdc->size };
+ int rc;
+
+ if ( d == current->domain )
+ return -EPERM;
+
+ if ( sdc->pad )
+ return -EINVAL;
+
+ c.buffer = vmalloc(c.len);
+ if ( !c.buffer )
+ return -ENOMEM;
+
+ rc = !copy_from_guest(c.buffer, sdc->buffer, c.len) ?
+ domain_load(d, &load_ops, &c) : -EFAULT;
+
+ vfree(c.buffer);
+
+ return rc;
+}
+
long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
{
long ret = 0;
@@ -867,6 +1031,15 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
copyback = 1;
break;
+ case XEN_DOMCTL_getdomaincontext:
+ ret = getdomaincontext(d, &op->u.getdomaincontext);
+ copyback = !ret;
+ break;
+
+ case XEN_DOMCTL_setdomaincontext:
+ ret = setdomaincontext(d, &op->u.setdomaincontext);
+ break;
+
default:
ret = arch_do_domctl(op, d, u_domctl);
break;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 791f0a2592..743105181f 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1130,6 +1130,43 @@ struct xen_domctl_vuart_op {
*/
};
+/*
+ * XEN_DOMCTL_getdomaincontext
+ * ---------------------------
+ *
+ * buffer (IN): The buffer into which the context data should be
+ * copied, or NULL to query the buffer size that should
+ * be allocated.
+ * size (IN/OUT): If 'buffer' is NULL then the value passed in must be
+ * zero, and the value passed out will be the size of the
+ * buffer to allocate.
+ * If 'buffer' is non-NULL then the value passed in must
+ * be the size of the buffer into which data may be copied.
+ * The value passed out will be the size of data written.
+ */
+struct xen_domctl_getdomaincontext {
+ uint32_t size;
+ uint32_t pad;
+ XEN_GUEST_HANDLE_64(void) buffer;
+};
+
+/* XEN_DOMCTL_setdomaincontext
+ * ---------------------------
+ *
+ * buffer (IN): The buffer from which the context data should be
+ * copied.
+ * size (IN): The size of the buffer from which data may be copied.
+ * This data must include DOMAIN_SAVE_CODE_HEADER at the
+ * start and terminate with a DOMAIN_SAVE_CODE_END record.
+ * Any data beyond the DOMAIN_SAVE_CODE_END record will be
+ * ignored.
+ */
+struct xen_domctl_setdomaincontext {
+ uint32_t size;
+ uint32_t pad;
+ XEN_GUEST_HANDLE_64(const_void) buffer;
+};
+
struct xen_domctl {
uint32_t cmd;
#define XEN_DOMCTL_createdomain 1
@@ -1214,6 +1251,8 @@ struct xen_domctl {
#define XEN_DOMCTL_vuart_op 81
#define XEN_DOMCTL_get_cpu_policy 82
#define XEN_DOMCTL_set_cpu_policy 83
+#define XEN_DOMCTL_getdomaincontext 84
+#define XEN_DOMCTL_setdomaincontext 85
#define XEN_DOMCTL_gdbsx_guestmemio 1000
#define XEN_DOMCTL_gdbsx_pausevcpu 1001
#define XEN_DOMCTL_gdbsx_unpausevcpu 1002
@@ -1274,6 +1313,8 @@ struct xen_domctl {
struct xen_domctl_monitor_op monitor_op;
struct xen_domctl_psr_alloc psr_alloc;
struct xen_domctl_vuart_op vuart_op;
+ struct xen_domctl_getdomaincontext getdomaincontext;
+ struct xen_domctl_setdomaincontext setdomaincontext;
uint8_t pad[128];
} u;
};
diff --git a/xen/xsm/flask/hooks.c b/xen/xsm/flask/hooks.c
index fab5d30c3a..18a9f11c74 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -744,6 +744,12 @@ static int flask_domctl(struct domain *d, int cmd)
case XEN_DOMCTL_get_cpu_policy:
return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__GET_CPU_POLICY);
+ case XEN_DOMCTL_setdomaincontext:
+ return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__SETCONTEXT);
+
+ case XEN_DOMCTL_getdomaincontext:
+ return current_has_perm(d, SECCLASS_DOMAIN2, DOMAIN2__GETCONTEXT);
+
default:
return avc_unknown_permission("domctl", cmd);
}
diff --git a/xen/xsm/flask/policy/access_vectors b/xen/xsm/flask/policy/access_vectors
index fde5162c7e..a7f6f96ed3 100644
--- a/xen/xsm/flask/policy/access_vectors
+++ b/xen/xsm/flask/policy/access_vectors
@@ -245,6 +245,10 @@ class domain2
resource_map
# XEN_DOMCTL_get_cpu_policy
get_cpu_policy
+# XEN_DOMCTL_setdomaincontext
+ setcontext
+# XEN_DOMCTL_getdomaincontext
+ getcontext
}
# Similar to class domain, but primarily contains domctls related to HVM domains
--
2.20.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v9 2/8] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
2020-09-24 13:10 ` [PATCH v9 2/8] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext Paul Durrant
@ 2020-09-30 14:31 ` Wei Liu
2020-10-02 21:58 ` Andrew Cooper
1 sibling, 0 replies; 41+ messages in thread
From: Wei Liu @ 2020-09-30 14:31 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Julien Grall, Daniel De Graaf,
Ian Jackson, Wei Liu, Andrew Cooper, George Dunlap, Jan Beulich,
Stefano Stabellini
On Thu, Sep 24, 2020 at 02:10:24PM +0100, Paul Durrant wrote:
> These domctls provide a mechanism to get and set domain context from
> the toolstack.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Reviewed-by: Julien Grall <julien@xen.org>
Acked-by: Wei Liu <wl@xen.org>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v9 2/8] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
2020-09-24 13:10 ` [PATCH v9 2/8] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext Paul Durrant
2020-09-30 14:31 ` Wei Liu
@ 2020-10-02 21:58 ` Andrew Cooper
2020-10-05 9:18 ` Durrant, Paul
1 sibling, 1 reply; 41+ messages in thread
From: Andrew Cooper @ 2020-10-02 21:58 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Julien Grall, Daniel De Graaf, Ian Jackson,
Wei Liu, George Dunlap, Jan Beulich, Stefano Stabellini
On 24/09/2020 14:10, Paul Durrant wrote:
> diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> index 791f0a2592..743105181f 100644
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1130,6 +1130,43 @@ struct xen_domctl_vuart_op {
> */
> };
>
> +/*
> + * XEN_DOMCTL_getdomaincontext
> + * ---------------------------
> + *
> + * buffer (IN): The buffer into which the context data should be
> + * copied, or NULL to query the buffer size that should
> + * be allocated.
> + * size (IN/OUT): If 'buffer' is NULL then the value passed in must be
> + * zero, and the value passed out will be the size of the
> + * buffer to allocate.
> + * If 'buffer' is non-NULL then the value passed in must
> + * be the size of the buffer into which data may be copied.
> + * The value passed out will be the size of data written.
> + */
> +struct xen_domctl_getdomaincontext {
> + uint32_t size;
This series is full of mismatched 32/64bit sizes, with several
truncation bugs in the previous patch.
Just use a 64bit size here. Life is too short to go searching for all
the other truncation bug when this stream tips over 4G, and its not like
there is a shortage of space in this structure.
> + uint32_t pad;
> + XEN_GUEST_HANDLE_64(void) buffer;
> +};
> +
> +/* XEN_DOMCTL_setdomaincontext
> + * ---------------------------
> + *
> + * buffer (IN): The buffer from which the context data should be
> + * copied.
> + * size (IN): The size of the buffer from which data may be copied.
> + * This data must include DOMAIN_SAVE_CODE_HEADER at the
> + * start and terminate with a DOMAIN_SAVE_CODE_END record.
> + * Any data beyond the DOMAIN_SAVE_CODE_END record will be
> + * ignored.
> + */
> +struct xen_domctl_setdomaincontext {
> + uint32_t size;
> + uint32_t pad;
> + XEN_GUEST_HANDLE_64(const_void) buffer;
> +};
> +
> struct xen_domctl {
> uint32_t cmd;
> #define XEN_DOMCTL_createdomain 1
> @@ -1214,6 +1251,8 @@ struct xen_domctl {
> #define XEN_DOMCTL_vuart_op 81
> #define XEN_DOMCTL_get_cpu_policy 82
> #define XEN_DOMCTL_set_cpu_policy 83
> +#define XEN_DOMCTL_getdomaincontext 84
> +#define XEN_DOMCTL_setdomaincontext 85
So, we've currently got:
#define XEN_DOMCTL_setvcpucontext 12
#define XEN_DOMCTL_getvcpucontext 13
#define XEN_DOMCTL_gethvmcontext 33
#define XEN_DOMCTL_sethvmcontext 34
#define XEN_DOMCTL_set_ext_vcpucontext 42
#define XEN_DOMCTL_get_ext_vcpucontext 43
#define XEN_DOMCTL_gethvmcontext_partial 55
#define XEN_DOMCTL_setvcpuextstate 62
#define XEN_DOMCTL_getvcpuextstate 63
which are doing alarmingly related things for vcpus. (As an amusing
exercise to the reader, figure out which are PV specific and which are
HVM specific. Hint: they're not disjoint sets.)
I know breaking with tradition is sacrilege, but at the very minimum,
can we get some underscores in that name so you can at least read the
words which make it up more easily.
~Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH v9 2/8] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
2020-10-02 21:58 ` Andrew Cooper
@ 2020-10-05 9:18 ` Durrant, Paul
0 siblings, 0 replies; 41+ messages in thread
From: Durrant, Paul @ 2020-10-05 9:18 UTC (permalink / raw)
To: Andrew Cooper, Paul Durrant, xen-devel
Cc: Julien Grall, Daniel De Graaf, Ian Jackson, Wei Liu,
George Dunlap, Jan Beulich, Stefano Stabellini
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 02 October 2020 22:58
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Julien Grall <julien@xen.org>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; George Dunlap
> <george.dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: RE: [EXTERNAL] [PATCH v9 2/8] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
>
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On 24/09/2020 14:10, Paul Durrant wrote:
> > diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
> > index 791f0a2592..743105181f 100644
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -1130,6 +1130,43 @@ struct xen_domctl_vuart_op {
> > */
> > };
> >
> > +/*
> > + * XEN_DOMCTL_getdomaincontext
> > + * ---------------------------
> > + *
> > + * buffer (IN): The buffer into which the context data should be
> > + * copied, or NULL to query the buffer size that should
> > + * be allocated.
> > + * size (IN/OUT): If 'buffer' is NULL then the value passed in must be
> > + * zero, and the value passed out will be the size of the
> > + * buffer to allocate.
> > + * If 'buffer' is non-NULL then the value passed in must
> > + * be the size of the buffer into which data may be copied.
> > + * The value passed out will be the size of data written.
> > + */
> > +struct xen_domctl_getdomaincontext {
> > + uint32_t size;
>
> This series is full of mismatched 32/64bit sizes, with several
> truncation bugs in the previous patch.
>
> Just use a 64bit size here. Life is too short to go searching for all
> the other truncation bug when this stream tips over 4G, and its not like
> there is a shortage of space in this structure.
>
Ok.
> > + uint32_t pad;
> > + XEN_GUEST_HANDLE_64(void) buffer;
> > +};
> > +
> > +/* XEN_DOMCTL_setdomaincontext
> > + * ---------------------------
> > + *
> > + * buffer (IN): The buffer from which the context data should be
> > + * copied.
> > + * size (IN): The size of the buffer from which data may be copied.
> > + * This data must include DOMAIN_SAVE_CODE_HEADER at the
> > + * start and terminate with a DOMAIN_SAVE_CODE_END record.
> > + * Any data beyond the DOMAIN_SAVE_CODE_END record will be
> > + * ignored.
> > + */
> > +struct xen_domctl_setdomaincontext {
> > + uint32_t size;
> > + uint32_t pad;
> > + XEN_GUEST_HANDLE_64(const_void) buffer;
> > +};
> > +
> > struct xen_domctl {
> > uint32_t cmd;
> > #define XEN_DOMCTL_createdomain 1
> > @@ -1214,6 +1251,8 @@ struct xen_domctl {
> > #define XEN_DOMCTL_vuart_op 81
> > #define XEN_DOMCTL_get_cpu_policy 82
> > #define XEN_DOMCTL_set_cpu_policy 83
> > +#define XEN_DOMCTL_getdomaincontext 84
> > +#define XEN_DOMCTL_setdomaincontext 85
>
> So, we've currently got:
>
> #define XEN_DOMCTL_setvcpucontext 12
> #define XEN_DOMCTL_getvcpucontext 13
> #define XEN_DOMCTL_gethvmcontext 33
> #define XEN_DOMCTL_sethvmcontext 34
> #define XEN_DOMCTL_set_ext_vcpucontext 42
> #define XEN_DOMCTL_get_ext_vcpucontext 43
> #define XEN_DOMCTL_gethvmcontext_partial 55
> #define XEN_DOMCTL_setvcpuextstate 62
> #define XEN_DOMCTL_getvcpuextstate 63
>
> which are doing alarmingly related things for vcpus. (As an amusing
> exercise to the reader, figure out which are PV specific and which are
> HVM specific. Hint: they're not disjoint sets.)
>
Yes, hence the desire to come up with something common.
>
> I know breaking with tradition is sacrilege, but at the very minimum,
> can we get some underscores in that name so you can at least read the
> words which make it up more easily.
>
Sure.
Paul
> ~Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v9 3/8] tools/misc: add xen-domctx to present domain context
2020-09-24 13:10 [PATCH v9 0/8] domain context infrastructure Paul Durrant
2020-09-24 13:10 ` [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context Paul Durrant
2020-09-24 13:10 ` [PATCH v9 2/8] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext Paul Durrant
@ 2020-09-24 13:10 ` Paul Durrant
2020-09-30 14:32 ` Wei Liu
2020-10-02 22:39 ` Andrew Cooper
2020-09-24 13:10 ` [PATCH v9 4/8] docs/specs: add missing definitions to libxc-migration-stream Paul Durrant
` (5 subsequent siblings)
8 siblings, 2 replies; 41+ messages in thread
From: Paul Durrant @ 2020-09-24 13:10 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Andrew Cooper, Wei Liu
This tool is analogous to 'xen-hvmctx' which presents HVM context.
Subsequent patches will add 'dump' functions when new records are
introduced.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wl@xen.org>
NOTE: Ian requested ack from Andrew
v3:
- Re-worked to avoid copying onto stack
- Added optional typecode and instance arguments
v2:
- Change name from 'xen-ctx' to 'xen-domctx'
---
.gitignore | 1 +
tools/misc/Makefile | 4 +
tools/misc/xen-domctx.c | 200 ++++++++++++++++++++++++++++++++++++++++
3 files changed, 205 insertions(+)
create mode 100644 tools/misc/xen-domctx.c
diff --git a/.gitignore b/.gitignore
index 5e8c47e2db..efbbd46fa9 100644
--- a/.gitignore
+++ b/.gitignore
@@ -233,6 +233,7 @@ tools/misc/xen_cpuperf
tools/misc/xen-cpuid
tools/misc/xen-detect
tools/misc/xen-diag
+tools/misc/xen-domctx
tools/misc/xen-tmem-list-parse
tools/misc/xen-livepatch
tools/misc/xenperf
diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index 7d37f297a9..fb673d0ff6 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -32,6 +32,7 @@ INSTALL_SBIN += xenpm
INSTALL_SBIN += xenwatchdogd
INSTALL_SBIN += xen-livepatch
INSTALL_SBIN += xen-diag
+INSTALL_SBIN += xen-domctx
INSTALL_SBIN += $(INSTALL_SBIN-y)
# Everything to be installed in a private bin/
@@ -111,6 +112,9 @@ xen-livepatch: xen-livepatch.o
xen-diag: xen-diag.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+xen-domctx: xen-domctx.o
+ $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+
xen-lowmemd: xen-lowmemd.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenevtchn) $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore) $(APPEND_LDFLAGS)
diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
new file mode 100644
index 0000000000..243325dfce
--- /dev/null
+++ b/tools/misc/xen-domctx.c
@@ -0,0 +1,200 @@
+/*
+ * xen-domctx.c
+ *
+ * Print out domain save records in a human-readable way.
+ *
+ * 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.
+ */
+
+#include <inttypes.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <errno.h>
+
+#include <xenctrl.h>
+#include <xen/xen.h>
+#include <xen/domctl.h>
+#include <xen/save.h>
+
+static void *buf = NULL;
+static size_t len, off;
+
+#define GET_PTR(_x) \
+ do { \
+ if ( len - off < sizeof(*(_x)) ) \
+ { \
+ fprintf(stderr, \
+ "error: need another %lu bytes, only %lu available\n", \
+ sizeof(*(_x)), len - off); \
+ exit(1); \
+ } \
+ (_x) = buf + off; \
+ } while (false);
+
+static void dump_header(void)
+{
+ DOMAIN_SAVE_TYPE(HEADER) *h;
+
+ GET_PTR(h);
+
+ printf(" HEADER: magic %#x, version %u\n",
+ h->magic, h->version);
+
+}
+
+static void dump_end(void)
+{
+ DOMAIN_SAVE_TYPE(END) *e;
+
+ GET_PTR(e);
+
+ printf(" END\n");
+}
+
+static void usage(const char *prog)
+{
+ fprintf(stderr, "usage: %s <domid> [ <typecode> [ <instance> ]]\n",
+ prog);
+ exit(1);
+}
+
+int main(int argc, char **argv)
+{
+ char *s, *e;
+ long domid;
+ long typecode = -1;
+ long instance = -1;
+ unsigned int entry;
+ xc_interface *xch;
+ int rc;
+
+ if ( argc < 2 || argc > 4 )
+ usage(argv[0]);
+
+ s = e = argv[1];
+ domid = strtol(s, &e, 0);
+
+ if ( *s == '\0' || *e != '\0' ||
+ domid < 0 || domid >= DOMID_FIRST_RESERVED )
+ {
+ fprintf(stderr, "invalid domid '%s'\n", s);
+ exit(1);
+ }
+
+ if ( argc >= 3 )
+ {
+ s = e = argv[2];
+ typecode = strtol(s, &e, 0);
+
+ if ( *s == '\0' || *e != '\0' )
+ {
+ fprintf(stderr, "invalid typecode '%s'\n", s);
+ exit(1);
+ }
+ }
+
+ if ( argc == 4 )
+ {
+ s = e = argv[3];
+ instance = strtol(s, &e, 0);
+
+ if ( *s == '\0' || *e != '\0' )
+ {
+ fprintf(stderr, "invalid instance '%s'\n", s);
+ exit(1);
+ }
+ }
+
+ xch = xc_interface_open(0, 0, 0);
+ if ( !xch )
+ {
+ fprintf(stderr, "error: can't open libxc handle\n");
+ exit(1);
+ }
+
+ rc = xc_domain_getcontext(xch, domid, NULL, &len);
+ if ( rc < 0 )
+ {
+ fprintf(stderr, "error: can't get record length for dom %lu: %s\n",
+ domid, strerror(errno));
+ exit(1);
+ }
+
+ buf = malloc(len);
+ if ( !buf )
+ {
+ fprintf(stderr, "error: can't allocate %lu bytes\n", len);
+ exit(1);
+ }
+
+ rc = xc_domain_getcontext(xch, domid, buf, &len);
+ if ( rc < 0 )
+ {
+ fprintf(stderr, "error: can't get domain record for dom %lu: %s\n",
+ domid, strerror(errno));
+ exit(1);
+ }
+ off = 0;
+
+ entry = 0;
+ for ( ; ; )
+ {
+ struct domain_save_descriptor *desc;
+
+ GET_PTR(desc);
+
+ off += sizeof(*desc);
+
+ if ( (typecode < 0 || typecode == desc->typecode) &&
+ (instance < 0 || instance == desc->instance) )
+ {
+ printf("[%u] type: %u instance: %u length: %u\n", entry++,
+ desc->typecode, desc->instance, desc->length);
+
+ switch (desc->typecode)
+ {
+ case DOMAIN_SAVE_CODE(HEADER): dump_header(); break;
+ case DOMAIN_SAVE_CODE(END): dump_end(); break;
+ default:
+ printf("Unknown type %u: skipping\n", desc->typecode);
+ break;
+ }
+ }
+
+ if ( desc->typecode == DOMAIN_SAVE_CODE(END) )
+ break;
+
+ off += desc->length;
+ }
+
+ return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.20.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v9 3/8] tools/misc: add xen-domctx to present domain context
2020-09-24 13:10 ` [PATCH v9 3/8] tools/misc: add xen-domctx to present domain context Paul Durrant
@ 2020-09-30 14:32 ` Wei Liu
2020-10-02 22:39 ` Andrew Cooper
1 sibling, 0 replies; 41+ messages in thread
From: Wei Liu @ 2020-09-30 14:32 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Paul Durrant, Ian Jackson, Andrew Cooper, Wei Liu
On Thu, Sep 24, 2020 at 02:10:25PM +0100, Paul Durrant wrote:
> This tool is analogous to 'xen-hvmctx' which presents HVM context.
> Subsequent patches will add 'dump' functions when new records are
> introduced.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Acked-by: Wei Liu <wl@xen.org>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v9 3/8] tools/misc: add xen-domctx to present domain context
2020-09-24 13:10 ` [PATCH v9 3/8] tools/misc: add xen-domctx to present domain context Paul Durrant
2020-09-30 14:32 ` Wei Liu
@ 2020-10-02 22:39 ` Andrew Cooper
2020-10-05 9:16 ` Durrant, Paul
1 sibling, 1 reply; 41+ messages in thread
From: Andrew Cooper @ 2020-10-02 22:39 UTC (permalink / raw)
To: Paul Durrant, xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu
On 24/09/2020 14:10, Paul Durrant wrote:
> This tool is analogous to 'xen-hvmctx' which presents HVM context.
> Subsequent patches will add 'dump' functions when new records are
> introduced.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> Cc: Wei Liu <wl@xen.org>
>
> NOTE: Ian requested ack from Andrew
I mean - its a fairly throwaway piece of misc userspace, so ack.
However, it is largely superseded by the changes you need to make to
verify-stream-v2 so you might want to bear that in mind.
Also, I wonder if it is wise in general that we're throwing so many misc
debugging tools into sbin.
> +#include <inttypes.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <errno.h>
> +
> +#include <xenctrl.h>
> +#include <xen/xen.h>
> +#include <xen/domctl.h>
> +#include <xen/save.h>
> +
> +static void *buf = NULL;
> +static size_t len, off;
> +
> +#define GET_PTR(_x) \
> + do { \
> + if ( len - off < sizeof(*(_x)) ) \
> + { \
> + fprintf(stderr, \
> + "error: need another %lu bytes, only %lu available\n", \
%zu is the correct formatter for size_t.
> + sizeof(*(_x)), len - off); \
> + exit(1); \
Your error handling will be far more simple by using err() instead of
opencoding it.
~Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH v9 3/8] tools/misc: add xen-domctx to present domain context
2020-10-02 22:39 ` Andrew Cooper
@ 2020-10-05 9:16 ` Durrant, Paul
0 siblings, 0 replies; 41+ messages in thread
From: Durrant, Paul @ 2020-10-05 9:16 UTC (permalink / raw)
To: Andrew Cooper, Paul Durrant, xen-devel; +Cc: Ian Jackson, Wei Liu
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 02 October 2020 23:40
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu
> <wl@xen.org>
> Subject: RE: [EXTERNAL] [PATCH v9 3/8] tools/misc: add xen-domctx to present domain context
>
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On 24/09/2020 14:10, Paul Durrant wrote:
> > This tool is analogous to 'xen-hvmctx' which presents HVM context.
> > Subsequent patches will add 'dump' functions when new records are
> > introduced.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> > ---
> > Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> > Cc: Wei Liu <wl@xen.org>
> >
> > NOTE: Ian requested ack from Andrew
>
> I mean - its a fairly throwaway piece of misc userspace, so ack.
>
> However, it is largely superseded by the changes you need to make to
> verify-stream-v2 so you might want to bear that in mind.
>
> Also, I wonder if it is wise in general that we're throwing so many misc
> debugging tools into sbin.
>
The intention is to eventually replace hvm context so we'll need a parallel tool to replace xen-hvmctx, unless we want to retire it.
> > +#include <inttypes.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <errno.h>
> > +
> > +#include <xenctrl.h>
> > +#include <xen/xen.h>
> > +#include <xen/domctl.h>
> > +#include <xen/save.h>
> > +
> > +static void *buf = NULL;
> > +static size_t len, off;
> > +
> > +#define GET_PTR(_x) \
> > + do { \
> > + if ( len - off < sizeof(*(_x)) ) \
> > + { \
> > + fprintf(stderr, \
> > + "error: need another %lu bytes, only %lu available\n", \
>
> %zu is the correct formatter for size_t.
>
True, missed that.
> > + sizeof(*(_x)), len - off); \
> > + exit(1); \
>
> Your error handling will be far more simple by using err() instead of
> opencoding it.
>
Ok.
Paul
> ~Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v9 4/8] docs/specs: add missing definitions to libxc-migration-stream
2020-09-24 13:10 [PATCH v9 0/8] domain context infrastructure Paul Durrant
` (2 preceding siblings ...)
2020-09-24 13:10 ` [PATCH v9 3/8] tools/misc: add xen-domctx to present domain context Paul Durrant
@ 2020-09-24 13:10 ` Paul Durrant
2020-09-30 14:35 ` Wei Liu
2020-10-02 22:42 ` Andrew Cooper
2020-09-24 13:10 ` [PATCH v9 5/8] docs / tools: specific migration v4 to include DOMAIN_CONTEXT Paul Durrant
` (4 subsequent siblings)
8 siblings, 2 replies; 41+ messages in thread
From: Paul Durrant @ 2020-09-24 13:10 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
From: Paul Durrant <pdurrant@amazon.com>
The STATIC_DATA_END, X86_CPUID_POLICY and X86_MSR_POLICY record types have
sections explaining what they are but their values are not defined. Indeed
their values are defined as "Reserved for future mandatory records."
Also, the spec revision is adjusted to match the migration stream version
and an END record is added to the description of a 'typical save record for
and x86 HVM guest.'
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Fixes: 6f71b5b1506 ("docs/migration Specify migration v3 and STATIC_DATA_END")
Fixes: ddd273d8863 ("docs/migration: Specify X86_{CPUID,MSR}_POLICY records")
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
v7:
- New in v7
---
docs/specs/libxc-migration-stream.pandoc | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
index 6b0c49e97a..8aeab3b11b 100644
--- a/docs/specs/libxc-migration-stream.pandoc
+++ b/docs/specs/libxc-migration-stream.pandoc
@@ -3,7 +3,7 @@
Andrew Cooper <<andrew.cooper3@citrix.com>>
Wen Congyang <<wency@cn.fujitsu.com>>
Yang Hongyang <<hongyang.yang@easystack.cn>>
-% Revision 2
+% Revision 3
Introduction
============
@@ -227,7 +227,13 @@ type 0x00000000: END
0x0000000F: CHECKPOINT_DIRTY_PFN_LIST (Secondary -> Primary)
- 0x00000010 - 0x7FFFFFFF: Reserved for future _mandatory_
+ 0x00000010: STATIC_DATA_END
+
+ 0x00000011: X86_CPUID_POLICY
+
+ 0x00000012: X86_MSR_POLICY
+
+ 0x00000013 - 0x7FFFFFFF: Reserved for future _mandatory_
records.
0x80000000 - 0xFFFFFFFF: Reserved for future _optional_
@@ -732,6 +738,7 @@ A typical save record for an x86 HVM guest image would look like:
* X86_TSC_INFO
* HVM_PARAMS
* HVM_CONTEXT
+* END record
HVM_PARAMS must precede HVM_CONTEXT, as certain parameters can affect
the validity of architectural state in the context.
--
2.20.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v9 4/8] docs/specs: add missing definitions to libxc-migration-stream
2020-09-24 13:10 ` [PATCH v9 4/8] docs/specs: add missing definitions to libxc-migration-stream Paul Durrant
@ 2020-09-30 14:35 ` Wei Liu
2020-10-02 22:42 ` Andrew Cooper
1 sibling, 0 replies; 41+ messages in thread
From: Wei Liu @ 2020-09-30 14:35 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu
On Thu, Sep 24, 2020 at 02:10:26PM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> The STATIC_DATA_END, X86_CPUID_POLICY and X86_MSR_POLICY record types have
> sections explaining what they are but their values are not defined. Indeed
> their values are defined as "Reserved for future mandatory records."
>
> Also, the spec revision is adjusted to match the migration stream version
> and an END record is added to the description of a 'typical save record for
> and x86 HVM guest.'
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Fixes: 6f71b5b1506 ("docs/migration Specify migration v3 and STATIC_DATA_END")
> Fixes: ddd273d8863 ("docs/migration: Specify X86_{CPUID,MSR}_POLICY records")
Acked-by: Wei Liu <wl@xen.org>
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v9 4/8] docs/specs: add missing definitions to libxc-migration-stream
2020-09-24 13:10 ` [PATCH v9 4/8] docs/specs: add missing definitions to libxc-migration-stream Paul Durrant
2020-09-30 14:35 ` Wei Liu
@ 2020-10-02 22:42 ` Andrew Cooper
2020-10-05 9:14 ` Durrant, Paul
1 sibling, 1 reply; 41+ messages in thread
From: Andrew Cooper @ 2020-10-02 22:42 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, George Dunlap, Ian Jackson, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu
On 24/09/2020 14:10, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> The STATIC_DATA_END, X86_CPUID_POLICY and X86_MSR_POLICY record types have
> sections explaining what they are but their values are not defined. Indeed
> their values are defined as "Reserved for future mandatory records."
>
> Also, the spec revision is adjusted to match the migration stream version
> and an END record is added to the description of a 'typical save record for
> and x86 HVM guest.'
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Fixes: 6f71b5b1506 ("docs/migration Specify migration v3 and STATIC_DATA_END")
> Fixes: ddd273d8863 ("docs/migration: Specify X86_{CPUID,MSR}_POLICY records")
Oops sorry. I swear I had these at one point. I can only presume it
got swallowed by a rebase at some point.
~Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH v9 4/8] docs/specs: add missing definitions to libxc-migration-stream
2020-10-02 22:42 ` Andrew Cooper
@ 2020-10-05 9:14 ` Durrant, Paul
0 siblings, 0 replies; 41+ messages in thread
From: Durrant, Paul @ 2020-10-05 9:14 UTC (permalink / raw)
To: Andrew Cooper, Paul Durrant, xen-devel
Cc: George Dunlap, Ian Jackson, Jan Beulich, Julien Grall,
Stefano Stabellini, Wei Liu
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 02 October 2020 23:42
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall <julien@xen.org>; Stefano
> Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>
> Subject: RE: [EXTERNAL] [PATCH v9 4/8] docs/specs: add missing definitions to libxc-migration-stream
>
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On 24/09/2020 14:10, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > The STATIC_DATA_END, X86_CPUID_POLICY and X86_MSR_POLICY record types have
> > sections explaining what they are but their values are not defined. Indeed
> > their values are defined as "Reserved for future mandatory records."
> >
> > Also, the spec revision is adjusted to match the migration stream version
> > and an END record is added to the description of a 'typical save record for
> > and x86 HVM guest.'
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> > Fixes: 6f71b5b1506 ("docs/migration Specify migration v3 and STATIC_DATA_END")
> > Fixes: ddd273d8863 ("docs/migration: Specify X86_{CPUID,MSR}_POLICY records")
>
> Oops sorry. I swear I had these at one point. I can only presume it
> got swallowed by a rebase at some point.
>
Can I take that as an R-b?
Paul
> ~Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v9 5/8] docs / tools: specific migration v4 to include DOMAIN_CONTEXT
2020-09-24 13:10 [PATCH v9 0/8] domain context infrastructure Paul Durrant
` (3 preceding siblings ...)
2020-09-24 13:10 ` [PATCH v9 4/8] docs/specs: add missing definitions to libxc-migration-stream Paul Durrant
@ 2020-09-24 13:10 ` Paul Durrant
2020-09-30 14:41 ` Wei Liu
2020-10-05 10:09 ` Andrew Cooper
2020-09-24 13:10 ` [PATCH v9 6/8] common/domain: add a domain context record for shared_info Paul Durrant
` (3 subsequent siblings)
8 siblings, 2 replies; 41+ messages in thread
From: Paul Durrant @ 2020-09-24 13:10 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu,
Marek Marczykowski-Górecki
From: Paul Durrant <pdurrant@amazon.com>
A new 'domain context' framework was recently introduced to facilitate
transfer of state for both PV and HVM guests. Hence this patch mandates the
presence of a new DOMAIN_CONTEXT record in version 4 of the libxc migration
stream.
This record will incorprate the content of the domain's 'shared_info' page
and the TSC informaiton so the SHARED_INFO and TSC_INFO records are deprecated.
It is intended that, in future, this record will contain state currently
present in the HVM_CONTEXT record. However, for compatibility with earlier
migration streams, the version 4 stream format continues to specify an
HVM_CONTEXT record and XEN_DOMCTL_sethvmcontext will continue to accept all
content of that record that may be present in a version 3 stream.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
Cc: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>
v7:
- New in v7
---
docs/specs/libxc-migration-stream.pandoc | 62 ++++++++++++++++++------
tools/libs/guest/xg_sr_common.c | 1 +
tools/libs/guest/xg_sr_stream_format.h | 1 +
tools/python/xen/migration/libxc.py | 2 +
4 files changed, 52 insertions(+), 14 deletions(-)
diff --git a/docs/specs/libxc-migration-stream.pandoc b/docs/specs/libxc-migration-stream.pandoc
index 8aeab3b11b..989f2a0cb6 100644
--- a/docs/specs/libxc-migration-stream.pandoc
+++ b/docs/specs/libxc-migration-stream.pandoc
@@ -3,7 +3,7 @@
Andrew Cooper <<andrew.cooper3@citrix.com>>
Wen Congyang <<wency@cn.fujitsu.com>>
Yang Hongyang <<hongyang.yang@easystack.cn>>
-% Revision 3
+% Revision 4
Introduction
============
@@ -127,7 +127,7 @@ marker 0xFFFFFFFFFFFFFFFF.
id 0x58454E46 ("XENF" in ASCII).
-version 0x00000003. The version of this specification.
+version 0x00000004. The version of this specification.
options bit 0: Endianness. 0 = little-endian, 1 = big-endian.
@@ -209,9 +209,9 @@ type 0x00000000: END
0x00000006: X86_PV_VCPU_XSAVE
- 0x00000007: SHARED_INFO
+ 0x00000007: SHARED_INFO (deprecated)
- 0x00000008: X86_TSC_INFO
+ 0x00000008: X86_TSC_INFO (deprecated)
0x00000009: HVM_CONTEXT
@@ -233,7 +233,9 @@ type 0x00000000: END
0x00000012: X86_MSR_POLICY
- 0x00000013 - 0x7FFFFFFF: Reserved for future _mandatory_
+ 0x00000013: DOMAIN_CONTEXT
+
+ 0x00000014 - 0x7FFFFFFF: Reserved for future _mandatory_
records.
0x80000000 - 0xFFFFFFFF: Reserved for future _optional_
@@ -442,10 +444,11 @@ X86_PV_VCPU_MSRS XEN_DOMCTL_{get,set}\_vcpu_msrs
\clearpage
-SHARED_INFO
------------
+SHARED_INFO (deprecated)
+------------------------
-The content of the Shared Info page.
+The content of the Shared Info page. This is incorporated into the
+DOMAIN_CONTEXT record as of specification version 4.
0 1 2 3 4 5 6 7 octet
+-------------------------------------------------+
@@ -462,11 +465,12 @@ shared_info Contents of the shared info page. This record
\clearpage
-X86_TSC_INFO
-------------
+X86_TSC_INFO (deprecated)
+-------------------------
Domain TSC information, as accessed by the
-XEN_DOMCTL_{get,set}tscinfo hypercall sub-ops.
+XEN_DOMCTL_{get,set}tscinfo hypercall sub-ops. This is incorporated into the
+DOMAIN_CONTEXT record as of specification version 4.
0 1 2 3 4 5 6 7 octet
+------------------------+------------------------+
@@ -680,6 +684,25 @@ MSR_policy Array of xen_msr_entry_t[]'s
\clearpage
+DOMAIN_CONTEXT
+--------------
+
+Domain context, as accessed by the
+XEN_DOMCTL_{get,set}domaincontext hypercall sub-ops.
+
+ 0 1 2 3 4 5 6 7 octet
+ +-------------------------------------------------+
+ | dom_ctx |
+ ...
+ +-------------------------------------------------+
+
+--------------------------------------------------------------------
+Field Description
+----------- ---------------------------------------------------
+dom_ctx The Domain Context blob from Xen.
+--------------------------------------------------------------------
+
+\clearpage
Layout
======
@@ -706,8 +729,7 @@ A typical save record for an x86 PV guest image would look like:
* STATIC_DATA_END
* X86_PV_P2M_FRAMES record
* Many PAGE_DATA records
-* X86_TSC_INFO
-* SHARED_INFO record
+* DOMAIN_CONTEXT
* VCPU context records for each online VCPU
* X86_PV_VCPU_BASIC record
* X86_PV_VCPU_EXTENDED record
@@ -735,7 +757,7 @@ A typical save record for an x86 HVM guest image would look like:
* X86_{CPUID,MSR}_POLICY
* STATIC_DATA_END
* Many PAGE_DATA records
-* X86_TSC_INFO
+* DOMAIN_CONTEXT
* HVM_PARAMS
* HVM_CONTEXT
* END record
@@ -746,6 +768,18 @@ the validity of architectural state in the context.
Compatibility with older versions
=================================
+v4 compat with v3
+-----------------
+
+A v4 stream is compatible with a v3 stream, but mandates the presence of a
+DOMAIN_CONTEXT record. This incorporates context such as the content of
+the domain's Shared Info page and the TSC information, hence the SHARED_INFO
+and TSC_INFO records are deprecated.
+It also supercedes HVM_CONTEXT and, over time, data that is currently part of
+the HVM_CONTEXT blob will move to the DOMAIN_CONTEXT blob. Xen, however, will
+continue to accept all defined HVM_CONTEXT records so a v4-compatible
+receiver can still accept an unmodified v3 stream.
+
v3 compat with v2
-----------------
diff --git a/tools/libs/guest/xg_sr_common.c b/tools/libs/guest/xg_sr_common.c
index 17567ab133..f813320202 100644
--- a/tools/libs/guest/xg_sr_common.c
+++ b/tools/libs/guest/xg_sr_common.c
@@ -39,6 +39,7 @@ static const char *const mandatory_rec_types[] =
[REC_TYPE_STATIC_DATA_END] = "Static data end",
[REC_TYPE_X86_CPUID_POLICY] = "x86 CPUID policy",
[REC_TYPE_X86_MSR_POLICY] = "x86 MSR policy",
+ [REC_TYPE_DOMAIN_CONTEXT] = "Domain context",
};
const char *rec_type_to_str(uint32_t type)
diff --git a/tools/libs/guest/xg_sr_stream_format.h b/tools/libs/guest/xg_sr_stream_format.h
index 8a0da26f75..bc538bc192 100644
--- a/tools/libs/guest/xg_sr_stream_format.h
+++ b/tools/libs/guest/xg_sr_stream_format.h
@@ -76,6 +76,7 @@ struct xc_sr_rhdr
#define REC_TYPE_STATIC_DATA_END 0x00000010U
#define REC_TYPE_X86_CPUID_POLICY 0x00000011U
#define REC_TYPE_X86_MSR_POLICY 0x00000012U
+#define REC_TYPE_DOMAIN_CONTEXT 0x00000013U
#define REC_TYPE_OPTIONAL 0x80000000U
diff --git a/tools/python/xen/migration/libxc.py b/tools/python/xen/migration/libxc.py
index 9881f5ced4..08ac81344f 100644
--- a/tools/python/xen/migration/libxc.py
+++ b/tools/python/xen/migration/libxc.py
@@ -59,6 +59,7 @@ REC_TYPE_checkpoint_dirty_pfn_list = 0x0000000f
REC_TYPE_static_data_end = 0x00000010
REC_TYPE_x86_cpuid_policy = 0x00000011
REC_TYPE_x86_msr_policy = 0x00000012
+REC_TYPE_domain_context = 0x00000013
rec_type_to_str = {
REC_TYPE_end : "End",
@@ -80,6 +81,7 @@ rec_type_to_str = {
REC_TYPE_static_data_end : "Static data end",
REC_TYPE_x86_cpuid_policy : "x86 CPUID policy",
REC_TYPE_x86_msr_policy : "x86 MSR policy",
+ REC_TYPE_domain_context : "Domain context",
}
# page_data
--
2.20.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v9 5/8] docs / tools: specific migration v4 to include DOMAIN_CONTEXT
2020-09-24 13:10 ` [PATCH v9 5/8] docs / tools: specific migration v4 to include DOMAIN_CONTEXT Paul Durrant
@ 2020-09-30 14:41 ` Wei Liu
2020-10-05 10:09 ` Andrew Cooper
1 sibling, 0 replies; 41+ messages in thread
From: Wei Liu @ 2020-09-30 14:41 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu, Marek Marczykowski-Górecki
On Thu, Sep 24, 2020 at 02:10:27PM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> A new 'domain context' framework was recently introduced to facilitate
> transfer of state for both PV and HVM guests. Hence this patch mandates the
> presence of a new DOMAIN_CONTEXT record in version 4 of the libxc migration
> stream.
> This record will incorprate the content of the domain's 'shared_info' page
> and the TSC informaiton so the SHARED_INFO and TSC_INFO records are deprecated.
> It is intended that, in future, this record will contain state currently
> present in the HVM_CONTEXT record. However, for compatibility with earlier
> migration streams, the version 4 stream format continues to specify an
> HVM_CONTEXT record and XEN_DOMCTL_sethvmcontext will continue to accept all
> content of that record that may be present in a version 3 stream.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Wei Liu <wl@xen.org>
I would like Andrew to give an explicit ack on this patch.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v9 5/8] docs / tools: specific migration v4 to include DOMAIN_CONTEXT
2020-09-24 13:10 ` [PATCH v9 5/8] docs / tools: specific migration v4 to include DOMAIN_CONTEXT Paul Durrant
2020-09-30 14:41 ` Wei Liu
@ 2020-10-05 10:09 ` Andrew Cooper
2020-10-05 10:13 ` Paul Durrant
1 sibling, 1 reply; 41+ messages in thread
From: Andrew Cooper @ 2020-10-05 10:09 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, George Dunlap, Ian Jackson, Jan Beulich,
Julien Grall, Stefano Stabellini, Wei Liu,
Marek Marczykowski-Górecki
On 24/09/2020 14:10, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> A new 'domain context' framework was recently introduced to facilitate
> transfer of state for both PV and HVM guests. Hence this patch mandates the
> presence of a new DOMAIN_CONTEXT record in version 4 of the libxc migration
> stream.
> This record will incorprate the content of the domain's 'shared_info' page
> and the TSC informaiton so the SHARED_INFO and TSC_INFO records are deprecated.
> It is intended that, in future, this record will contain state currently
> present in the HVM_CONTEXT record. However, for compatibility with earlier
> migration streams, the version 4 stream format continues to specify an
> HVM_CONTEXT record and XEN_DOMCTL_sethvmcontext will continue to accept all
> content of that record that may be present in a version 3 stream.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
What's the plan for the remainder of the work? We don't want to burn
multiple version numbers for each bit of incremental work.
One option might be to specify the full extent of the work, and use an
environment variable to alter the behaviour of the sending side, while
it is still work-in-progress.
~Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v9 5/8] docs / tools: specific migration v4 to include DOMAIN_CONTEXT
2020-10-05 10:09 ` Andrew Cooper
@ 2020-10-05 10:13 ` Paul Durrant
0 siblings, 0 replies; 41+ messages in thread
From: Paul Durrant @ 2020-10-05 10:13 UTC (permalink / raw)
To: 'Andrew Cooper', xen-devel
Cc: 'Paul Durrant', 'George Dunlap',
'Ian Jackson', 'Jan Beulich',
'Julien Grall', 'Stefano Stabellini',
'Wei Liu', 'Marek Marczykowski-Górecki'
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 05 October 2020 11:09
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall <julien@xen.org>; Stefano
> Stabellini <sstabellini@kernel.org>; Wei Liu <wl@xen.org>; Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com>
> Subject: Re: [PATCH v9 5/8] docs / tools: specific migration v4 to include DOMAIN_CONTEXT
>
> On 24/09/2020 14:10, Paul Durrant wrote:
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > A new 'domain context' framework was recently introduced to facilitate
> > transfer of state for both PV and HVM guests. Hence this patch mandates the
> > presence of a new DOMAIN_CONTEXT record in version 4 of the libxc migration
> > stream.
> > This record will incorprate the content of the domain's 'shared_info' page
> > and the TSC informaiton so the SHARED_INFO and TSC_INFO records are deprecated.
> > It is intended that, in future, this record will contain state currently
> > present in the HVM_CONTEXT record. However, for compatibility with earlier
> > migration streams, the version 4 stream format continues to specify an
> > HVM_CONTEXT record and XEN_DOMCTL_sethvmcontext will continue to accept all
> > content of that record that may be present in a version 3 stream.
> >
> > Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>
> What's the plan for the remainder of the work? We don't want to burn
> multiple version numbers for each bit of incremental work.
>
> One option might be to specify the full extent of the work, and use an
> environment variable to alter the behaviour of the sending side, while
> it is still work-in-progress.
>
The other missing part for transparent migration is xenstore content, but I'd expect that to sit at the next level up so I'm not anticipating any more churn at this level.
Paul
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v9 6/8] common/domain: add a domain context record for shared_info...
2020-09-24 13:10 [PATCH v9 0/8] domain context infrastructure Paul Durrant
` (4 preceding siblings ...)
2020-09-24 13:10 ` [PATCH v9 5/8] docs / tools: specific migration v4 to include DOMAIN_CONTEXT Paul Durrant
@ 2020-09-24 13:10 ` Paul Durrant
2020-09-25 12:44 ` Jan Beulich
` (2 more replies)
2020-09-24 13:10 ` [PATCH v9 7/8] x86/time: add a domain context record for tsc_info Paul Durrant
` (2 subsequent siblings)
8 siblings, 3 replies; 41+ messages in thread
From: Paul Durrant @ 2020-09-24 13:10 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Ian Jackson, Wei Liu, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini
From: Paul Durrant <pdurrant@amazon.com>
... and update xen-domctx to dump some information describing the record.
NOTE: Processing of the content during restore is currently limited to
PV domains, and matches processing of the PV-only SHARED_INFO record
done by libxc. All content is, however, saved such that restore
processing can be modified in future without requiring a new record
format.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
v9:
- Use macros to make the code less verbose
- Add missing check for allocation failure
v8:
- Incorporate zero-ing out of shared info fields that would be done in
processing of SHARED_INFO from older stream versions
v7:
- Only restore vcpu_info and arch sub-structures for PV domains, to match
processing of SHARED_INFO in xc_sr_restore_x86_pv.c
- Use additional option to domain_load_end() to ignore the record for
HVM domains
v6:
- Only save compat_shared_info buffer if has_32bit_shinfo is set
- Validate flags field in load handler
v5:
- Addressed comments from Julien
v4:
- Addressed comments from Jan
v3:
- Actually dump some of the content of shared_info
v2:
- Drop the header change to define a 'Xen' page size and instead use a
variable length struct now that the framework makes this is feasible
- Guard use of 'has_32bit_shinfo' in common code with CONFIG_COMPAT
---
tools/misc/xen-domctx.c | 78 ++++++++++++++++++++++++++++
xen/common/domain.c | 105 ++++++++++++++++++++++++++++++++++++++
xen/include/public/save.h | 13 ++++-
3 files changed, 195 insertions(+), 1 deletion(-)
diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
index 243325dfce..6ead7ea89d 100644
--- a/tools/misc/xen-domctx.c
+++ b/tools/misc/xen-domctx.c
@@ -31,6 +31,7 @@
#include <errno.h>
#include <xenctrl.h>
+#include <xen-tools/libs.h>
#include <xen/xen.h>
#include <xen/domctl.h>
#include <xen/save.h>
@@ -61,6 +62,82 @@ static void dump_header(void)
}
+static void print_binary(const char *prefix, const void *val, size_t size,
+ const char *suffix)
+{
+ printf("%s", prefix);
+
+ while ( size-- )
+ {
+ uint8_t octet = *(const uint8_t *)val++;
+ unsigned int i;
+
+ for ( i = 0; i < 8; i++ )
+ {
+ printf("%u", octet & 1);
+ octet >>= 1;
+ }
+ }
+
+ printf("%s", suffix);
+}
+
+static void dump_shared_info(void)
+{
+ DOMAIN_SAVE_TYPE(SHARED_INFO) *s;
+ bool has_32bit_shinfo;
+ shared_info_any_t *info;
+ unsigned int i, n;
+
+ GET_PTR(s);
+ has_32bit_shinfo = s->flags & DOMAIN_SAVE_32BIT_SHINFO;
+
+ printf(" SHARED_INFO: has_32bit_shinfo: %s buffer_size: %u\n",
+ has_32bit_shinfo ? "true" : "false", s->buffer_size);
+
+ info = (shared_info_any_t *)s->buffer;
+
+#define GET_FIELD_PTR(_f) \
+ (has_32bit_shinfo ? \
+ (const void *)&(info->x32._f) : \
+ (const void *)&(info->x64._f))
+#define GET_FIELD_SIZE(_f) \
+ (has_32bit_shinfo ? sizeof(info->x32._f) : sizeof(info->x64._f))
+#define GET_FIELD(_f) \
+ (has_32bit_shinfo ? info->x32._f : info->x64._f)
+
+ n = has_32bit_shinfo ?
+ ARRAY_SIZE(info->x32.evtchn_pending) :
+ ARRAY_SIZE(info->x64.evtchn_pending);
+
+ for ( i = 0; i < n; i++ )
+ {
+ const char *prefix = !i ?
+ " evtchn_pending: " :
+ " ";
+
+ print_binary(prefix, GET_FIELD_PTR(evtchn_pending[0]),
+ GET_FIELD_SIZE(evtchn_pending[0]), "\n");
+ }
+
+ for ( i = 0; i < n; i++ )
+ {
+ const char *prefix = !i ?
+ " evtchn_mask: " :
+ " ";
+
+ print_binary(prefix, GET_FIELD_PTR(evtchn_mask[0]),
+ GET_FIELD_SIZE(evtchn_mask[0]), "\n");
+ }
+
+ printf(" wc: version: %u sec: %u nsec: %u\n",
+ GET_FIELD(wc_version), GET_FIELD(wc_sec), GET_FIELD(wc_nsec));
+
+#undef GET_FIELD
+#undef GET_FIELD_SIZE
+#undef GET_FIELD_PTR
+}
+
static void dump_end(void)
{
DOMAIN_SAVE_TYPE(END) *e;
@@ -173,6 +250,7 @@ int main(int argc, char **argv)
switch (desc->typecode)
{
case DOMAIN_SAVE_CODE(HEADER): dump_header(); break;
+ case DOMAIN_SAVE_CODE(SHARED_INFO): dump_shared_info(); break;
case DOMAIN_SAVE_CODE(END): dump_end(); break;
default:
printf("Unknown type %u: skipping\n", desc->typecode);
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 8cfa2e0b6b..6709f9c79e 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -33,6 +33,7 @@
#include <xen/xenoprof.h>
#include <xen/irq.h>
#include <xen/argo.h>
+#include <xen/save.h>
#include <asm/debugger.h>
#include <asm/p2m.h>
#include <asm/processor.h>
@@ -1657,6 +1658,110 @@ int continue_hypercall_on_cpu(
return 0;
}
+static int save_shared_info(const struct domain *d, struct domain_context *c,
+ bool dry_run)
+{
+ struct domain_shared_info_context ctxt = {
+#ifdef CONFIG_COMPAT
+ .flags = has_32bit_shinfo(d) ? DOMAIN_SAVE_32BIT_SHINFO : 0,
+ .buffer_size = has_32bit_shinfo(d) ?
+ sizeof(struct compat_shared_info) :
+ sizeof(struct shared_info),
+#else
+ .buffer_size = sizeof(struct shared_info),
+#endif
+ };
+ size_t hdr_size = offsetof(typeof(ctxt), buffer);
+ int rc;
+
+ rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, 0);
+ if ( rc )
+ return rc;
+
+ rc = domain_save_data(c, &ctxt, hdr_size);
+ if ( rc )
+ return rc;
+
+ rc = domain_save_data(c, d->shared_info, ctxt.buffer_size);
+ if ( rc )
+ return rc;
+
+ return domain_save_end(c);
+}
+
+static int load_shared_info(struct domain *d, struct domain_context *c)
+{
+ struct domain_shared_info_context ctxt;
+ size_t hdr_size = offsetof(typeof(ctxt), buffer);
+ unsigned int i;
+ int rc;
+
+ rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, &i);
+ if ( rc )
+ return rc;
+
+ if ( i ) /* expect only a single instance */
+ return -ENXIO;
+
+ rc = domain_load_data(c, &ctxt, hdr_size);
+ if ( rc )
+ return rc;
+
+ if ( ctxt.buffer_size > sizeof(shared_info_t) ||
+ (ctxt.flags & ~DOMAIN_SAVE_32BIT_SHINFO) )
+ return -EINVAL;
+
+ if ( ctxt.flags & DOMAIN_SAVE_32BIT_SHINFO )
+ {
+#ifdef CONFIG_COMPAT
+ has_32bit_shinfo(d) = true;
+#else
+ return -EINVAL;
+#endif
+ }
+
+ if ( is_pv_domain(d) )
+ {
+ shared_info_t *shinfo = xmalloc(shared_info_t);
+
+ if ( !shinfo )
+ return -ENOMEM;
+
+ rc = domain_load_data(c, shinfo, sizeof(*shinfo));
+ if ( rc )
+ goto out;
+
+ memcpy(&shared_info(d, vcpu_info), &__shared_info(d, shinfo, vcpu_info),
+ sizeof(shared_info(d, vcpu_info)));
+ memcpy(&shared_info(d, arch), &__shared_info(d, shinfo, arch),
+ sizeof(shared_info(d, arch)));
+
+ memset(&shared_info(d, evtchn_pending), 0,
+ sizeof(shared_info(d, evtchn_pending)));
+ memset(&shared_info(d, evtchn_mask), 0xff,
+ sizeof(shared_info(d, evtchn_mask)));
+
+ shared_info(d, arch.pfn_to_mfn_frame_list_list) = 0;
+ for ( i = 0; i < XEN_LEGACY_MAX_VCPUS; i++ )
+ shared_info(d, vcpu_info[i].evtchn_pending_sel) = 0;
+
+ rc = domain_load_end(c, false);
+
+ out:
+ xfree(shinfo);
+ }
+ else
+ /*
+ * No modifications to shared_info are required for restoring non-PV
+ * domains.
+ */
+ rc = domain_load_end(c, true);
+
+ return rc;
+}
+
+DOMAIN_REGISTER_SAVE_LOAD(SHARED_INFO, save_shared_info, load_shared_info);
+
/*
* Local variables:
* mode: C
diff --git a/xen/include/public/save.h b/xen/include/public/save.h
index 551dbbddb8..0e855a4b97 100644
--- a/xen/include/public/save.h
+++ b/xen/include/public/save.h
@@ -82,7 +82,18 @@ struct domain_save_header {
};
DECLARE_DOMAIN_SAVE_TYPE(HEADER, 1, struct domain_save_header);
-#define DOMAIN_SAVE_CODE_MAX 1
+struct domain_shared_info_context {
+ uint32_t flags;
+
+#define DOMAIN_SAVE_32BIT_SHINFO 0x00000001
+
+ uint32_t buffer_size;
+ uint8_t buffer[XEN_FLEX_ARRAY_DIM]; /* Implementation specific size */
+};
+
+DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
+
+#define DOMAIN_SAVE_CODE_MAX 2
#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
--
2.20.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v9 6/8] common/domain: add a domain context record for shared_info...
2020-09-24 13:10 ` [PATCH v9 6/8] common/domain: add a domain context record for shared_info Paul Durrant
@ 2020-09-25 12:44 ` Jan Beulich
2020-09-30 14:42 ` Wei Liu
2020-10-05 10:39 ` Andrew Cooper
2 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2020-09-25 12:44 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini
On 24.09.2020 15:10, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> ... and update xen-domctx to dump some information describing the record.
>
> NOTE: Processing of the content during restore is currently limited to
> PV domains, and matches processing of the PV-only SHARED_INFO record
> done by libxc. All content is, however, saved such that restore
> processing can be modified in future without requiring a new record
> format.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v9 6/8] common/domain: add a domain context record for shared_info...
2020-09-24 13:10 ` [PATCH v9 6/8] common/domain: add a domain context record for shared_info Paul Durrant
2020-09-25 12:44 ` Jan Beulich
@ 2020-09-30 14:42 ` Wei Liu
2020-10-05 10:39 ` Andrew Cooper
2 siblings, 0 replies; 41+ messages in thread
From: Wei Liu @ 2020-09-30 14:42 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu, Andrew Cooper,
George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini
On Thu, Sep 24, 2020 at 02:10:28PM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> ... and update xen-domctx to dump some information describing the record.
>
> NOTE: Processing of the content during restore is currently limited to
> PV domains, and matches processing of the PV-only SHARED_INFO record
> done by libxc. All content is, however, saved such that restore
> processing can be modified in future without requiring a new record
> format.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Wei Liu <wl@xen.org>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v9 6/8] common/domain: add a domain context record for shared_info...
2020-09-24 13:10 ` [PATCH v9 6/8] common/domain: add a domain context record for shared_info Paul Durrant
2020-09-25 12:44 ` Jan Beulich
2020-09-30 14:42 ` Wei Liu
@ 2020-10-05 10:39 ` Andrew Cooper
2020-10-07 12:03 ` Paul Durrant
2020-10-13 11:49 ` Jan Beulich
2 siblings, 2 replies; 41+ messages in thread
From: Andrew Cooper @ 2020-10-05 10:39 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Ian Jackson, Wei Liu, George Dunlap, Jan Beulich,
Julien Grall, Stefano Stabellini
On 24/09/2020 14:10, Paul Durrant wrote:
> diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
> index 243325dfce..6ead7ea89d 100644
> --- a/tools/misc/xen-domctx.c
> +++ b/tools/misc/xen-domctx.c
> @@ -31,6 +31,7 @@
> #include <errno.h>
>
> #include <xenctrl.h>
> +#include <xen-tools/libs.h>
> #include <xen/xen.h>
> #include <xen/domctl.h>
> #include <xen/save.h>
> @@ -61,6 +62,82 @@ static void dump_header(void)
>
> }
>
> +static void print_binary(const char *prefix, const void *val, size_t size,
> + const char *suffix)
> +{
> + printf("%s", prefix);
> +
> + while ( size-- )
> + {
> + uint8_t octet = *(const uint8_t *)val++;
> + unsigned int i;
> +
> + for ( i = 0; i < 8; i++ )
> + {
> + printf("%u", octet & 1);
> + octet >>= 1;
> + }
> + }
> +
> + printf("%s", suffix);
> +}
> +
> +static void dump_shared_info(void)
> +{
> + DOMAIN_SAVE_TYPE(SHARED_INFO) *s;
> + bool has_32bit_shinfo;
> + shared_info_any_t *info;
> + unsigned int i, n;
> +
> + GET_PTR(s);
> + has_32bit_shinfo = s->flags & DOMAIN_SAVE_32BIT_SHINFO;
> +
> + printf(" SHARED_INFO: has_32bit_shinfo: %s buffer_size: %u\n",
> + has_32bit_shinfo ? "true" : "false", s->buffer_size);
> +
> + info = (shared_info_any_t *)s->buffer;
> +
> +#define GET_FIELD_PTR(_f) \
> + (has_32bit_shinfo ? \
> + (const void *)&(info->x32._f) : \
> + (const void *)&(info->x64._f))
> +#define GET_FIELD_SIZE(_f) \
> + (has_32bit_shinfo ? sizeof(info->x32._f) : sizeof(info->x64._f))
> +#define GET_FIELD(_f) \
> + (has_32bit_shinfo ? info->x32._f : info->x64._f)
> +
> + n = has_32bit_shinfo ?
> + ARRAY_SIZE(info->x32.evtchn_pending) :
> + ARRAY_SIZE(info->x64.evtchn_pending);
> +
> + for ( i = 0; i < n; i++ )
> + {
> + const char *prefix = !i ?
> + " evtchn_pending: " :
> + " ";
> +
> + print_binary(prefix, GET_FIELD_PTR(evtchn_pending[0]),
> + GET_FIELD_SIZE(evtchn_pending[0]), "\n");
> + }
> +
> + for ( i = 0; i < n; i++ )
> + {
> + const char *prefix = !i ?
> + " evtchn_mask: " :
> + " ";
> +
> + print_binary(prefix, GET_FIELD_PTR(evtchn_mask[0]),
> + GET_FIELD_SIZE(evtchn_mask[0]), "\n");
> + }
What about domains using FIFO? This is meaningless for them.
> +
> + printf(" wc: version: %u sec: %u nsec: %u\n",
> + GET_FIELD(wc_version), GET_FIELD(wc_sec), GET_FIELD(wc_nsec));
wc_sec_hi is also a rather critical field in this calculation.
> +
> +#undef GET_FIELD
> +#undef GET_FIELD_SIZE
> +#undef GET_FIELD_PTR
> +}
> +
> static void dump_end(void)
> {
> DOMAIN_SAVE_TYPE(END) *e;
> @@ -173,6 +250,7 @@ int main(int argc, char **argv)
> switch (desc->typecode)
> {
> case DOMAIN_SAVE_CODE(HEADER): dump_header(); break;
> + case DOMAIN_SAVE_CODE(SHARED_INFO): dump_shared_info(); break;
> case DOMAIN_SAVE_CODE(END): dump_end(); break;
> default:
> printf("Unknown type %u: skipping\n", desc->typecode);
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 8cfa2e0b6b..6709f9c79e 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -33,6 +33,7 @@
> #include <xen/xenoprof.h>
> #include <xen/irq.h>
> #include <xen/argo.h>
> +#include <xen/save.h>
> #include <asm/debugger.h>
> #include <asm/p2m.h>
> #include <asm/processor.h>
> @@ -1657,6 +1658,110 @@ int continue_hypercall_on_cpu(
> return 0;
> }
>
> +static int save_shared_info(const struct domain *d, struct domain_context *c,
> + bool dry_run)
> +{
> + struct domain_shared_info_context ctxt = {
> +#ifdef CONFIG_COMPAT
> + .flags = has_32bit_shinfo(d) ? DOMAIN_SAVE_32BIT_SHINFO : 0,
> + .buffer_size = has_32bit_shinfo(d) ?
> + sizeof(struct compat_shared_info) :
> + sizeof(struct shared_info),
> +#else
> + .buffer_size = sizeof(struct shared_info),
> +#endif
> + };
> + size_t hdr_size = offsetof(typeof(ctxt), buffer);
> + int rc;
> +
> + rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, 0);
> + if ( rc )
> + return rc;
> +
> + rc = domain_save_data(c, &ctxt, hdr_size);
> + if ( rc )
> + return rc;
> +
> + rc = domain_save_data(c, d->shared_info, ctxt.buffer_size);
> + if ( rc )
> + return rc;
> +
> + return domain_save_end(c);
> +}
> +
> +static int load_shared_info(struct domain *d, struct domain_context *c)
> +{
> + struct domain_shared_info_context ctxt;
> + size_t hdr_size = offsetof(typeof(ctxt), buffer);
> + unsigned int i;
> + int rc;
> +
> + rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, &i);
> + if ( rc )
> + return rc;
> +
> + if ( i ) /* expect only a single instance */
> + return -ENXIO;
> +
> + rc = domain_load_data(c, &ctxt, hdr_size);
> + if ( rc )
> + return rc;
> +
> + if ( ctxt.buffer_size > sizeof(shared_info_t) ||
> + (ctxt.flags & ~DOMAIN_SAVE_32BIT_SHINFO) )
> + return -EINVAL;
> +
> + if ( ctxt.flags & DOMAIN_SAVE_32BIT_SHINFO )
> + {
> +#ifdef CONFIG_COMPAT
> + has_32bit_shinfo(d) = true;
d->arch.has_32bit_shinfo
> +#else
> + return -EINVAL;
> +#endif
> + }
> +
> + if ( is_pv_domain(d) )
> + {
> + shared_info_t *shinfo = xmalloc(shared_info_t);
> +
> + if ( !shinfo )
> + return -ENOMEM;
> +
> + rc = domain_load_data(c, shinfo, sizeof(*shinfo));
> + if ( rc )
> + goto out;
There's no need for a memory allocation, or to double buffer this data.
You can memcpy() straight out of the context record.
> +
> + memcpy(&shared_info(d, vcpu_info), &__shared_info(d, shinfo, vcpu_info),
> + sizeof(shared_info(d, vcpu_info)));
> + memcpy(&shared_info(d, arch), &__shared_info(d, shinfo, arch),
> + sizeof(shared_info(d, arch)));
> +
> + memset(&shared_info(d, evtchn_pending), 0,
> + sizeof(shared_info(d, evtchn_pending)));
> + memset(&shared_info(d, evtchn_mask), 0xff,
> + sizeof(shared_info(d, evtchn_mask)));
> +
> + shared_info(d, arch.pfn_to_mfn_frame_list_list) = 0;
> + for ( i = 0; i < XEN_LEGACY_MAX_VCPUS; i++ )
> + shared_info(d, vcpu_info[i].evtchn_pending_sel) = 0;
What is the plan for transparent migrate here? While this is ok for
regular migrate, its definitely not for transparent.
> +
> + rc = domain_load_end(c, false);
> +
> + out:
> + xfree(shinfo);
> + }
> + else
> + /*
> + * No modifications to shared_info are required for restoring non-PV
> + * domains.
> + */
> + rc = domain_load_end(c, true);
> +
> + return rc;
> +}
> +
> +DOMAIN_REGISTER_SAVE_LOAD(SHARED_INFO, save_shared_info, load_shared_info);
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/include/public/save.h b/xen/include/public/save.h
> index 551dbbddb8..0e855a4b97 100644
> --- a/xen/include/public/save.h
> +++ b/xen/include/public/save.h
> @@ -82,7 +82,18 @@ struct domain_save_header {
> };
> DECLARE_DOMAIN_SAVE_TYPE(HEADER, 1, struct domain_save_header);
>
> -#define DOMAIN_SAVE_CODE_MAX 1
> +struct domain_shared_info_context {
> + uint32_t flags;
> +
> +#define DOMAIN_SAVE_32BIT_SHINFO 0x00000001
> +
> + uint32_t buffer_size;
This struct is already wrapped with a header including a size which
encompasses buffer.
Multiple overlapping size fields is an easy way to memory corruption,
because it causes ambiguity as to which one is right.
~Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH v9 6/8] common/domain: add a domain context record for shared_info...
2020-10-05 10:39 ` Andrew Cooper
@ 2020-10-07 12:03 ` Paul Durrant
2020-10-13 11:49 ` Jan Beulich
1 sibling, 0 replies; 41+ messages in thread
From: Paul Durrant @ 2020-10-07 12:03 UTC (permalink / raw)
To: 'Andrew Cooper', xen-devel
Cc: 'Paul Durrant', 'Ian Jackson', 'Wei Liu',
'George Dunlap', 'Jan Beulich',
'Julien Grall', 'Stefano Stabellini'
> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> Sent: 05 October 2020 11:40
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>;
> George Dunlap <george.dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall
> <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v9 6/8] common/domain: add a domain context record for shared_info...
>
> On 24/09/2020 14:10, Paul Durrant wrote:
> > diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
> > index 243325dfce..6ead7ea89d 100644
> > --- a/tools/misc/xen-domctx.c
> > +++ b/tools/misc/xen-domctx.c
> > @@ -31,6 +31,7 @@
> > #include <errno.h>
> >
> > #include <xenctrl.h>
> > +#include <xen-tools/libs.h>
> > #include <xen/xen.h>
> > #include <xen/domctl.h>
> > #include <xen/save.h>
> > @@ -61,6 +62,82 @@ static void dump_header(void)
> >
> > }
> >
> > +static void print_binary(const char *prefix, const void *val, size_t size,
> > + const char *suffix)
> > +{
> > + printf("%s", prefix);
> > +
> > + while ( size-- )
> > + {
> > + uint8_t octet = *(const uint8_t *)val++;
> > + unsigned int i;
> > +
> > + for ( i = 0; i < 8; i++ )
> > + {
> > + printf("%u", octet & 1);
> > + octet >>= 1;
> > + }
> > + }
> > +
> > + printf("%s", suffix);
> > +}
> > +
> > +static void dump_shared_info(void)
> > +{
> > + DOMAIN_SAVE_TYPE(SHARED_INFO) *s;
> > + bool has_32bit_shinfo;
> > + shared_info_any_t *info;
> > + unsigned int i, n;
> > +
> > + GET_PTR(s);
> > + has_32bit_shinfo = s->flags & DOMAIN_SAVE_32BIT_SHINFO;
> > +
> > + printf(" SHARED_INFO: has_32bit_shinfo: %s buffer_size: %u\n",
> > + has_32bit_shinfo ? "true" : "false", s->buffer_size);
> > +
> > + info = (shared_info_any_t *)s->buffer;
> > +
> > +#define GET_FIELD_PTR(_f) \
> > + (has_32bit_shinfo ? \
> > + (const void *)&(info->x32._f) : \
> > + (const void *)&(info->x64._f))
> > +#define GET_FIELD_SIZE(_f) \
> > + (has_32bit_shinfo ? sizeof(info->x32._f) : sizeof(info->x64._f))
> > +#define GET_FIELD(_f) \
> > + (has_32bit_shinfo ? info->x32._f : info->x64._f)
> > +
> > + n = has_32bit_shinfo ?
> > + ARRAY_SIZE(info->x32.evtchn_pending) :
> > + ARRAY_SIZE(info->x64.evtchn_pending);
> > +
> > + for ( i = 0; i < n; i++ )
> > + {
> > + const char *prefix = !i ?
> > + " evtchn_pending: " :
> > + " ";
> > +
> > + print_binary(prefix, GET_FIELD_PTR(evtchn_pending[0]),
> > + GET_FIELD_SIZE(evtchn_pending[0]), "\n");
> > + }
> > +
> > + for ( i = 0; i < n; i++ )
> > + {
> > + const char *prefix = !i ?
> > + " evtchn_mask: " :
> > + " ";
> > +
> > + print_binary(prefix, GET_FIELD_PTR(evtchn_mask[0]),
> > + GET_FIELD_SIZE(evtchn_mask[0]), "\n");
> > + }
>
> What about domains using FIFO? This is meaningless for them.
>
Indeed, but this is essentially a debug tool so I'd rather it just dumped everything that might be useful.
> > +
> > + printf(" wc: version: %u sec: %u nsec: %u\n",
> > + GET_FIELD(wc_version), GET_FIELD(wc_sec), GET_FIELD(wc_nsec));
>
> wc_sec_hi is also a rather critical field in this calculation.
>
Ok.
> > +
> > +#undef GET_FIELD
> > +#undef GET_FIELD_SIZE
> > +#undef GET_FIELD_PTR
> > +}
> > +
> > static void dump_end(void)
> > {
> > DOMAIN_SAVE_TYPE(END) *e;
> > @@ -173,6 +250,7 @@ int main(int argc, char **argv)
> > switch (desc->typecode)
> > {
> > case DOMAIN_SAVE_CODE(HEADER): dump_header(); break;
> > + case DOMAIN_SAVE_CODE(SHARED_INFO): dump_shared_info(); break;
> > case DOMAIN_SAVE_CODE(END): dump_end(); break;
> > default:
> > printf("Unknown type %u: skipping\n", desc->typecode);
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index 8cfa2e0b6b..6709f9c79e 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -33,6 +33,7 @@
> > #include <xen/xenoprof.h>
> > #include <xen/irq.h>
> > #include <xen/argo.h>
> > +#include <xen/save.h>
> > #include <asm/debugger.h>
> > #include <asm/p2m.h>
> > #include <asm/processor.h>
> > @@ -1657,6 +1658,110 @@ int continue_hypercall_on_cpu(
> > return 0;
> > }
> >
> > +static int save_shared_info(const struct domain *d, struct domain_context *c,
> > + bool dry_run)
> > +{
> > + struct domain_shared_info_context ctxt = {
> > +#ifdef CONFIG_COMPAT
> > + .flags = has_32bit_shinfo(d) ? DOMAIN_SAVE_32BIT_SHINFO : 0,
> > + .buffer_size = has_32bit_shinfo(d) ?
> > + sizeof(struct compat_shared_info) :
> > + sizeof(struct shared_info),
> > +#else
> > + .buffer_size = sizeof(struct shared_info),
> > +#endif
> > + };
> > + size_t hdr_size = offsetof(typeof(ctxt), buffer);
> > + int rc;
> > +
> > + rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, 0);
> > + if ( rc )
> > + return rc;
> > +
> > + rc = domain_save_data(c, &ctxt, hdr_size);
> > + if ( rc )
> > + return rc;
> > +
> > + rc = domain_save_data(c, d->shared_info, ctxt.buffer_size);
> > + if ( rc )
> > + return rc;
> > +
> > + return domain_save_end(c);
> > +}
> > +
> > +static int load_shared_info(struct domain *d, struct domain_context *c)
> > +{
> > + struct domain_shared_info_context ctxt;
> > + size_t hdr_size = offsetof(typeof(ctxt), buffer);
> > + unsigned int i;
> > + int rc;
> > +
> > + rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, &i);
> > + if ( rc )
> > + return rc;
> > +
> > + if ( i ) /* expect only a single instance */
> > + return -ENXIO;
> > +
> > + rc = domain_load_data(c, &ctxt, hdr_size);
> > + if ( rc )
> > + return rc;
> > +
> > + if ( ctxt.buffer_size > sizeof(shared_info_t) ||
> > + (ctxt.flags & ~DOMAIN_SAVE_32BIT_SHINFO) )
> > + return -EINVAL;
> > +
> > + if ( ctxt.flags & DOMAIN_SAVE_32BIT_SHINFO )
> > + {
> > +#ifdef CONFIG_COMPAT
> > + has_32bit_shinfo(d) = true;
>
> d->arch.has_32bit_shinfo
>
If you'd prefer, ok.
> > +#else
> > + return -EINVAL;
> > +#endif
> > + }
> > +
> > + if ( is_pv_domain(d) )
> > + {
> > + shared_info_t *shinfo = xmalloc(shared_info_t);
> > +
> > + if ( !shinfo )
> > + return -ENOMEM;
> > +
> > + rc = domain_load_data(c, shinfo, sizeof(*shinfo));
> > + if ( rc )
> > + goto out;
>
> There's no need for a memory allocation, or to double buffer this data.
> You can memcpy() straight out of the context record.
>
That would mean re-working the way that domain_load_data() works. I'd really rather not.
> > +
> > + memcpy(&shared_info(d, vcpu_info), &__shared_info(d, shinfo, vcpu_info),
> > + sizeof(shared_info(d, vcpu_info)));
> > + memcpy(&shared_info(d, arch), &__shared_info(d, shinfo, arch),
> > + sizeof(shared_info(d, arch)));
> > +
> > + memset(&shared_info(d, evtchn_pending), 0,
> > + sizeof(shared_info(d, evtchn_pending)));
> > + memset(&shared_info(d, evtchn_mask), 0xff,
> > + sizeof(shared_info(d, evtchn_mask)));
> > +
> > + shared_info(d, arch.pfn_to_mfn_frame_list_list) = 0;
> > + for ( i = 0; i < XEN_LEGACY_MAX_VCPUS; i++ )
> > + shared_info(d, vcpu_info[i].evtchn_pending_sel) = 0;
>
> What is the plan for transparent migrate here? While this is ok for
> regular migrate, its definitely not for transparent.
>
Quite true, as evidenced that this is inside 'if ( is_pv_domain(d) )'. It is not yet clear how much of the shared info we need for transparent migrate. It may be nothing.
> > +
> > + rc = domain_load_end(c, false);
> > +
> > + out:
> > + xfree(shinfo);
> > + }
> > + else
> > + /*
> > + * No modifications to shared_info are required for restoring non-PV
> > + * domains.
> > + */
> > + rc = domain_load_end(c, true);
> > +
> > + return rc;
> > +}
> > +
> > +DOMAIN_REGISTER_SAVE_LOAD(SHARED_INFO, save_shared_info, load_shared_info);
> > +
> > /*
> > * Local variables:
> > * mode: C
> > diff --git a/xen/include/public/save.h b/xen/include/public/save.h
> > index 551dbbddb8..0e855a4b97 100644
> > --- a/xen/include/public/save.h
> > +++ b/xen/include/public/save.h
> > @@ -82,7 +82,18 @@ struct domain_save_header {
> > };
> > DECLARE_DOMAIN_SAVE_TYPE(HEADER, 1, struct domain_save_header);
> >
> > -#define DOMAIN_SAVE_CODE_MAX 1
> > +struct domain_shared_info_context {
> > + uint32_t flags;
> > +
> > +#define DOMAIN_SAVE_32BIT_SHINFO 0x00000001
> > +
> > + uint32_t buffer_size;
>
> This struct is already wrapped with a header including a size which
> encompasses buffer.
>
> Multiple overlapping size fields is an easy way to memory corruption,
> because it causes ambiguity as to which one is right.
>
The record size currently includes padding. I'm re-working that in v10 and so this size can be dropped.
Paul
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v9 6/8] common/domain: add a domain context record for shared_info...
2020-10-05 10:39 ` Andrew Cooper
2020-10-07 12:03 ` Paul Durrant
@ 2020-10-13 11:49 ` Jan Beulich
1 sibling, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2020-10-13 11:49 UTC (permalink / raw)
To: Andrew Cooper
Cc: Paul Durrant, xen-devel, Paul Durrant, Ian Jackson, Wei Liu,
George Dunlap, Julien Grall, Stefano Stabellini
On 05.10.2020 12:39, Andrew Cooper wrote:
> On 24/09/2020 14:10, Paul Durrant wrote:
>> +static int load_shared_info(struct domain *d, struct domain_context *c)
>> +{
>> + struct domain_shared_info_context ctxt;
>> + size_t hdr_size = offsetof(typeof(ctxt), buffer);
>> + unsigned int i;
>> + int rc;
>> +
>> + rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, &i);
>> + if ( rc )
>> + return rc;
>> +
>> + if ( i ) /* expect only a single instance */
>> + return -ENXIO;
>> +
>> + rc = domain_load_data(c, &ctxt, hdr_size);
>> + if ( rc )
>> + return rc;
>> +
>> + if ( ctxt.buffer_size > sizeof(shared_info_t) ||
>> + (ctxt.flags & ~DOMAIN_SAVE_32BIT_SHINFO) )
>> + return -EINVAL;
>> +
>> + if ( ctxt.flags & DOMAIN_SAVE_32BIT_SHINFO )
>> + {
>> +#ifdef CONFIG_COMPAT
>> + has_32bit_shinfo(d) = true;
>
> d->arch.has_32bit_shinfo
But this is common code, i.e. using d->arch directly is a layering
violation. I know your dislike of lvalues disguised by function-
like macros, but what do you do?
Jan
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v9 7/8] x86/time: add a domain context record for tsc_info...
2020-09-24 13:10 [PATCH v9 0/8] domain context infrastructure Paul Durrant
` (5 preceding siblings ...)
2020-09-24 13:10 ` [PATCH v9 6/8] common/domain: add a domain context record for shared_info Paul Durrant
@ 2020-09-24 13:10 ` Paul Durrant
2020-09-30 14:43 ` Wei Liu
2020-09-24 13:10 ` [PATCH v9 8/8] tools/libxc: add DOMAIN_CONTEXT records to the migration stream Paul Durrant
2020-09-24 19:36 ` [PATCH v9 0/8] domain context infrastructure Lengyel, Tamas
8 siblings, 1 reply; 41+ messages in thread
From: Paul Durrant @ 2020-09-24 13:10 UTC (permalink / raw)
To: xen-devel
Cc: Paul Durrant, Jan Beulich, Ian Jackson, Wei Liu, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini,
Roger Pau Monné
From: Paul Durrant <pdurrant@amazon.com>
... and update xen-domctx to dump some information describing the record.
NOTE: Whilst the record definition is x86 specific, it is visible directly
in the common header as context record numbers should be unique across
all architectures.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
v8:
- Removed stray blank line
v7:
- New in v7
---
tools/misc/xen-domctx.c | 11 +++++++++++
xen/arch/x86/time.c | 34 +++++++++++++++++++++++++++++++++-
xen/include/asm-x86/time.h | 5 +++--
xen/include/public/save.h | 13 ++++++++++++-
4 files changed, 59 insertions(+), 4 deletions(-)
diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
index 6ead7ea89d..e582a79678 100644
--- a/tools/misc/xen-domctx.c
+++ b/tools/misc/xen-domctx.c
@@ -59,7 +59,17 @@ static void dump_header(void)
printf(" HEADER: magic %#x, version %u\n",
h->magic, h->version);
+}
+
+static void dump_tsc_info(void)
+{
+ DOMAIN_SAVE_TYPE(TSC_INFO) *t;
+
+ GET_PTR(t);
+ printf(" TSC_INFO: mode: %u incarnation: %u\n"
+ " khz %u elapsed_nsec: %"PRIu64"\n",
+ t->mode, t->incarnation, t->khz, t->elapsed_nsec);
}
static void print_binary(const char *prefix, const void *val, size_t size,
@@ -251,6 +261,7 @@ int main(int argc, char **argv)
{
case DOMAIN_SAVE_CODE(HEADER): dump_header(); break;
case DOMAIN_SAVE_CODE(SHARED_INFO): dump_shared_info(); break;
+ case DOMAIN_SAVE_CODE(TSC_INFO): dump_tsc_info(); break;
case DOMAIN_SAVE_CODE(END): dump_end(); break;
default:
printf("Unknown type %u: skipping\n", desc->typecode);
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 8938c0f435..25731f7df4 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -26,6 +26,7 @@
#include <xen/symbols.h>
#include <xen/keyhandler.h>
#include <xen/guest_access.h>
+#include <xen/save.h>
#include <asm/io.h>
#include <asm/iocap.h>
#include <asm/msr.h>
@@ -2334,7 +2335,7 @@ int host_tsc_is_safe(void)
* called to collect tsc-related data only for save file or live
* migrate; called after last rdtsc is done on this incarnation
*/
-void tsc_get_info(struct domain *d, uint32_t *tsc_mode,
+void tsc_get_info(const struct domain *d, uint32_t *tsc_mode,
uint64_t *elapsed_nsec, uint32_t *gtsc_khz,
uint32_t *incarnation)
{
@@ -2451,6 +2452,37 @@ int tsc_set_info(struct domain *d,
return 0;
}
+static int save_tsc_info(const struct domain *d, struct domain_context *c,
+ bool dry_run)
+{
+ struct domain_tsc_info_context ctxt;
+
+ if ( !dry_run )
+ tsc_get_info(d, &ctxt.mode, &ctxt.elapsed_nsec, &ctxt.khz,
+ &ctxt.incarnation);
+
+ return DOMAIN_SAVE_ENTRY(TSC_INFO, c, 0, &ctxt, sizeof(ctxt));
+}
+
+static int load_tsc_info(struct domain *d, struct domain_context *c)
+{
+ struct domain_tsc_info_context ctxt;
+ unsigned int i;
+ int rc;
+
+ rc = DOMAIN_LOAD_ENTRY(TSC_INFO, c, &i, &ctxt, sizeof(ctxt));
+ if ( rc )
+ return rc;
+
+ if ( i ) /* expect only a single instance */
+ return -ENXIO;
+
+ return tsc_set_info(d, ctxt.mode, ctxt.elapsed_nsec, ctxt.khz,
+ ctxt.incarnation);
+}
+
+DOMAIN_REGISTER_SAVE_LOAD(TSC_INFO, save_tsc_info, load_tsc_info);
+
/* vtsc may incur measurable performance degradation, diagnose with this */
static void dump_softtsc(unsigned char key)
{
diff --git a/xen/include/asm-x86/time.h b/xen/include/asm-x86/time.h
index f347311cc4..7f2ce6226a 100644
--- a/xen/include/asm-x86/time.h
+++ b/xen/include/asm-x86/time.h
@@ -59,8 +59,9 @@ u64 gtsc_to_gtime(struct domain *d, u64 tsc);
int tsc_set_info(struct domain *d, uint32_t tsc_mode, uint64_t elapsed_nsec,
uint32_t gtsc_khz, uint32_t incarnation);
-void tsc_get_info(struct domain *d, uint32_t *tsc_mode, uint64_t *elapsed_nsec,
- uint32_t *gtsc_khz, uint32_t *incarnation);
+void tsc_get_info(const struct domain *d, uint32_t *tsc_mode,
+ uint64_t *elapsed_nsec, uint32_t *gtsc_khz,
+ uint32_t *incarnation);
void force_update_vcpu_system_time(struct vcpu *v);
diff --git a/xen/include/public/save.h b/xen/include/public/save.h
index 0e855a4b97..aeb17298eb 100644
--- a/xen/include/public/save.h
+++ b/xen/include/public/save.h
@@ -93,7 +93,18 @@ struct domain_shared_info_context {
DECLARE_DOMAIN_SAVE_TYPE(SHARED_INFO, 2, struct domain_shared_info_context);
-#define DOMAIN_SAVE_CODE_MAX 2
+#if defined(__i386__) || defined(__x86_64__)
+struct domain_tsc_info_context {
+ uint32_t mode;
+ uint32_t incarnation;
+ uint64_t elapsed_nsec;
+ uint32_t khz;
+};
+
+DECLARE_DOMAIN_SAVE_TYPE(TSC_INFO, 3, struct domain_tsc_info_context);
+#endif
+
+#define DOMAIN_SAVE_CODE_MAX 3
#endif /* defined(__XEN__) || defined(__XEN_TOOLS__) */
--
2.20.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v9 7/8] x86/time: add a domain context record for tsc_info...
2020-09-24 13:10 ` [PATCH v9 7/8] x86/time: add a domain context record for tsc_info Paul Durrant
@ 2020-09-30 14:43 ` Wei Liu
0 siblings, 0 replies; 41+ messages in thread
From: Wei Liu @ 2020-09-30 14:43 UTC (permalink / raw)
To: Paul Durrant
Cc: xen-devel, Paul Durrant, Jan Beulich, Ian Jackson, Wei Liu,
Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Roger Pau Monné
On Thu, Sep 24, 2020 at 02:10:29PM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> ... and update xen-domctx to dump some information describing the record.
>
> NOTE: Whilst the record definition is x86 specific, it is visible directly
> in the common header as context record numbers should be unique across
> all architectures.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Wei Liu <wl@xen.org>
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v9 8/8] tools/libxc: add DOMAIN_CONTEXT records to the migration stream...
2020-09-24 13:10 [PATCH v9 0/8] domain context infrastructure Paul Durrant
` (6 preceding siblings ...)
2020-09-24 13:10 ` [PATCH v9 7/8] x86/time: add a domain context record for tsc_info Paul Durrant
@ 2020-09-24 13:10 ` Paul Durrant
2020-09-30 14:46 ` Wei Liu
2020-10-01 15:17 ` Andrew Cooper
2020-09-24 19:36 ` [PATCH v9 0/8] domain context infrastructure Lengyel, Tamas
8 siblings, 2 replies; 41+ messages in thread
From: Paul Durrant @ 2020-09-24 13:10 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu
From: Paul Durrant <pdurrant@amazon.com>
... and bump the version.
This patch implements version 4 of the migration stream by adding the code
necessary to save and restore DOMAIN_CONTEXT records, and removing the code
to save the SHARED_INFO and TSC_INFO records (as these are deprecated in
version 4).
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
v7:
- New in v7
---
tools/libs/guest/xg_sr_common.h | 3 ++
tools/libs/guest/xg_sr_common_x86.c | 20 -----------
tools/libs/guest/xg_sr_common_x86.h | 6 ----
tools/libs/guest/xg_sr_restore.c | 45 +++++++++++++++++++++--
tools/libs/guest/xg_sr_save.c | 52 ++++++++++++++++++++++++++-
tools/libs/guest/xg_sr_save_x86_hvm.c | 5 ---
tools/libs/guest/xg_sr_save_x86_pv.c | 22 ------------
7 files changed, 97 insertions(+), 56 deletions(-)
diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
index 13fcc47420..d440281cc1 100644
--- a/tools/libs/guest/xg_sr_common.h
+++ b/tools/libs/guest/xg_sr_common.h
@@ -298,6 +298,9 @@ struct xc_sr_context
/* Sender has invoked verify mode on the stream. */
bool verify;
+
+ /* Domain context blob. */
+ struct xc_sr_blob context;
} restore;
};
diff --git a/tools/libs/guest/xg_sr_common_x86.c b/tools/libs/guest/xg_sr_common_x86.c
index 6f12483907..10a35b998e 100644
--- a/tools/libs/guest/xg_sr_common_x86.c
+++ b/tools/libs/guest/xg_sr_common_x86.c
@@ -1,25 +1,5 @@
#include "xg_sr_common_x86.h"
-int write_x86_tsc_info(struct xc_sr_context *ctx)
-{
- xc_interface *xch = ctx->xch;
- struct xc_sr_rec_x86_tsc_info tsc = {};
- struct xc_sr_record rec = {
- .type = REC_TYPE_X86_TSC_INFO,
- .length = sizeof(tsc),
- .data = &tsc,
- };
-
- if ( xc_domain_get_tsc_info(xch, ctx->domid, &tsc.mode,
- &tsc.nsec, &tsc.khz, &tsc.incarnation) < 0 )
- {
- PERROR("Unable to obtain TSC information");
- return -1;
- }
-
- return write_record(ctx, &rec);
-}
-
int handle_x86_tsc_info(struct xc_sr_context *ctx, struct xc_sr_record *rec)
{
xc_interface *xch = ctx->xch;
diff --git a/tools/libs/guest/xg_sr_common_x86.h b/tools/libs/guest/xg_sr_common_x86.h
index b55758c96d..e504169705 100644
--- a/tools/libs/guest/xg_sr_common_x86.h
+++ b/tools/libs/guest/xg_sr_common_x86.h
@@ -3,12 +3,6 @@
#include "xg_sr_common.h"
-/*
- * Obtains a domains TSC information from Xen and writes a X86_TSC_INFO record
- * into the stream.
- */
-int write_x86_tsc_info(struct xc_sr_context *ctx);
-
/*
* Parses a X86_TSC_INFO record and applies the result to the domain.
*/
diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c
index b57a787519..453a383ba4 100644
--- a/tools/libs/guest/xg_sr_restore.c
+++ b/tools/libs/guest/xg_sr_restore.c
@@ -35,9 +35,9 @@ static int read_headers(struct xc_sr_context *ctx)
return -1;
}
- if ( ihdr.version < 2 || ihdr.version > 3 )
+ if ( ihdr.version < 2 || ihdr.version > 4 )
{
- ERROR("Invalid Version: Expected 2 <= ver <= 3, Got %d",
+ ERROR("Invalid Version: Expected 2 <= ver <= 4, Got %d",
ihdr.version);
return -1;
}
@@ -529,6 +529,20 @@ static int send_checkpoint_dirty_pfn_list(struct xc_sr_context *ctx)
return rc;
}
+static int stream_complete(struct xc_sr_context *ctx)
+{
+ xc_interface *xch = ctx->xch;
+ int rc;
+
+ rc = xc_domain_setcontext(xch, ctx->domid,
+ ctx->restore.context.ptr,
+ ctx->restore.context.size);
+ if ( rc < 0 )
+ PERROR("Unable to restore domain context");
+
+ return rc;
+}
+
static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec);
static int handle_checkpoint(struct xc_sr_context *ctx)
{
@@ -597,6 +611,10 @@ static int handle_checkpoint(struct xc_sr_context *ctx)
/* COLO */
/* We need to resume guest */
+ rc = stream_complete(ctx);
+ if ( rc )
+ goto err;
+
rc = ctx->restore.ops.stream_complete(ctx);
if ( rc )
goto err;
@@ -682,6 +700,21 @@ int handle_static_data_end(struct xc_sr_context *ctx)
return rc;
}
+/*
+ * Process a DOMAIN_CONTEXT record from the stream.
+ */
+static int handle_domain_context(struct xc_sr_context *ctx,
+ struct xc_sr_record *rec)
+{
+ xc_interface *xch = ctx->xch;
+ int rc = update_blob(&ctx->restore.context, rec->data, rec->length);
+
+ if ( rc )
+ ERROR("Unable to allocate %u bytes for domain context", rec->length);
+
+ return rc;
+}
+
static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
{
xc_interface *xch = ctx->xch;
@@ -709,6 +742,10 @@ static int process_record(struct xc_sr_context *ctx, struct xc_sr_record *rec)
rc = handle_static_data_end(ctx);
break;
+ case REC_TYPE_DOMAIN_CONTEXT:
+ rc = handle_domain_context(ctx, rec);
+ break;
+
default:
rc = ctx->restore.ops.process_record(ctx, rec);
break;
@@ -860,6 +897,10 @@ static int restore(struct xc_sr_context *ctx)
* With Remus, if we reach here, there must be some error on primary,
* failover from the last checkpoint state.
*/
+ rc = stream_complete(ctx);
+ if ( rc )
+ goto err;
+
rc = ctx->restore.ops.stream_complete(ctx);
if ( rc )
goto err;
diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index 2ba7c3200c..a6718c9ee4 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -13,7 +13,7 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
struct xc_sr_ihdr ihdr = {
.marker = IHDR_MARKER,
.id = htonl(IHDR_ID),
- .version = htonl(3),
+ .version = htonl(4),
.options = htons(IHDR_OPT_LITTLE_ENDIAN),
};
struct xc_sr_dhdr dhdr = {
@@ -44,6 +44,52 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type)
return 0;
}
+/*
+ * Writes a DOMAIN_CONTEXT record into the stream.
+ */
+static int write_domain_context_record(struct xc_sr_context *ctx)
+{
+ xc_interface *xch = ctx->xch;
+ struct xc_sr_record rec = {
+ .type = REC_TYPE_DOMAIN_CONTEXT,
+ };
+ size_t len = 0;
+ int rc;
+
+ rc = xc_domain_getcontext(xch, ctx->domid, NULL, &len);
+ if ( rc < 0 )
+ {
+ PERROR("can't get record length for dom %u\n", ctx->domid);
+ goto out;
+ }
+
+ rec.data = malloc(len);
+
+ rc = -1;
+ if ( !rec.data )
+ {
+ PERROR("can't allocate %lu bytes\n", len);
+ goto out;
+ }
+
+ rc = xc_domain_getcontext(xch, ctx->domid, rec.data, &len);
+ if ( rc < 0 )
+ {
+ PERROR("can't get domain record for dom %u\n", ctx->domid);
+ goto out;
+ }
+
+ rec.length = len;
+ rc = write_record(ctx, &rec);
+ if ( rc < 0 )
+ PERROR("failed to write DOMAIN_CONTEXT record");
+
+ out:
+ free(rec.data);
+
+ return rc;
+}
+
/*
* Writes an END record into the stream.
*/
@@ -905,6 +951,10 @@ static int save(struct xc_sr_context *ctx, uint16_t guest_type)
goto err;
}
+ rc = write_domain_context_record(ctx);
+ if ( rc )
+ goto err;
+
rc = ctx->save.ops.end_of_checkpoint(ctx);
if ( rc )
goto err;
diff --git a/tools/libs/guest/xg_sr_save_x86_hvm.c b/tools/libs/guest/xg_sr_save_x86_hvm.c
index 1634a7bc43..d44fb3fc4f 100644
--- a/tools/libs/guest/xg_sr_save_x86_hvm.c
+++ b/tools/libs/guest/xg_sr_save_x86_hvm.c
@@ -193,11 +193,6 @@ static int x86_hvm_end_of_checkpoint(struct xc_sr_context *ctx)
{
int rc;
- /* Write the TSC record. */
- rc = write_x86_tsc_info(ctx);
- if ( rc )
- return rc;
-
/* Write the HVM_CONTEXT record. */
rc = write_hvm_context(ctx);
if ( rc )
diff --git a/tools/libs/guest/xg_sr_save_x86_pv.c b/tools/libs/guest/xg_sr_save_x86_pv.c
index 4964f1f7b8..3de7b19f54 100644
--- a/tools/libs/guest/xg_sr_save_x86_pv.c
+++ b/tools/libs/guest/xg_sr_save_x86_pv.c
@@ -849,20 +849,6 @@ static int write_x86_pv_p2m_frames(struct xc_sr_context *ctx)
return rc;
}
-/*
- * Writes an SHARED_INFO record into the stream.
- */
-static int write_shared_info(struct xc_sr_context *ctx)
-{
- struct xc_sr_record rec = {
- .type = REC_TYPE_SHARED_INFO,
- .length = PAGE_SIZE,
- .data = ctx->x86.pv.shinfo,
- };
-
- return write_record(ctx, &rec);
-}
-
/*
* Normalise a pagetable for the migration stream. Performs mfn->pfn
* conversions on the ptes.
@@ -1093,14 +1079,6 @@ static int x86_pv_end_of_checkpoint(struct xc_sr_context *ctx)
{
int rc;
- rc = write_x86_tsc_info(ctx);
- if ( rc )
- return rc;
-
- rc = write_shared_info(ctx);
- if ( rc )
- return rc;
-
rc = write_all_vcpu_information(ctx);
if ( rc )
return rc;
--
2.20.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v9 8/8] tools/libxc: add DOMAIN_CONTEXT records to the migration stream...
2020-09-24 13:10 ` [PATCH v9 8/8] tools/libxc: add DOMAIN_CONTEXT records to the migration stream Paul Durrant
@ 2020-09-30 14:46 ` Wei Liu
2020-10-01 15:17 ` Andrew Cooper
1 sibling, 0 replies; 41+ messages in thread
From: Wei Liu @ 2020-09-30 14:46 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Paul Durrant, Ian Jackson, Wei Liu
On Thu, Sep 24, 2020 at 02:10:30PM +0100, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> ... and bump the version.
>
> This patch implements version 4 of the migration stream by adding the code
> necessary to save and restore DOMAIN_CONTEXT records, and removing the code
> to save the SHARED_INFO and TSC_INFO records (as these are deprecated in
> version 4).
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Wei Liu <wl@xen.org>
Again, I think Andrew should have a look at this as well.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v9 8/8] tools/libxc: add DOMAIN_CONTEXT records to the migration stream...
2020-09-24 13:10 ` [PATCH v9 8/8] tools/libxc: add DOMAIN_CONTEXT records to the migration stream Paul Durrant
2020-09-30 14:46 ` Wei Liu
@ 2020-10-01 15:17 ` Andrew Cooper
1 sibling, 0 replies; 41+ messages in thread
From: Andrew Cooper @ 2020-10-01 15:17 UTC (permalink / raw)
To: Paul Durrant, xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu
On 24/09/2020 14:10, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
>
> ... and bump the version.
>
> This patch implements version 4 of the migration stream by adding the code
> necessary to save and restore DOMAIN_CONTEXT records, and removing the code
> to save the SHARED_INFO and TSC_INFO records (as these are deprecated in
> version 4).
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
This really needs to be at least 3 patches.
First to adjust tools/python/scripts/verify-stream-v2 to understand the
new changes in the stream.
My testing tends to include running the script over the result of `xl
save`, and using a script in place of libxl-save-helper which tee's the
stream through the script, which lets you test in-line migrate. (I
wonder if it would be better to add a pass-through mode to the script
and give libxl a way of running it in the same way as it currently runs
covert-legacy-stream.)
Next, a patch updating the receive side only to understand the new
changes in the stream. In particular, this makes it far easier to
confirm that backwards compatibility is maintained.
Finally, a patch updating the sending side, if applicable. (I've got an
alternative suggestion to avoid burning a load of major version numbers,
but will follow up on a different patch with that).
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wl@xen.org>
>
> v7:
> - New in v7
> ---
> tools/libs/guest/xg_sr_common.h | 3 ++
> tools/libs/guest/xg_sr_common_x86.c | 20 -----------
> tools/libs/guest/xg_sr_common_x86.h | 6 ----
> tools/libs/guest/xg_sr_restore.c | 45 +++++++++++++++++++++--
> tools/libs/guest/xg_sr_save.c | 52 ++++++++++++++++++++++++++-
> tools/libs/guest/xg_sr_save_x86_hvm.c | 5 ---
> tools/libs/guest/xg_sr_save_x86_pv.c | 22 ------------
> 7 files changed, 97 insertions(+), 56 deletions(-)
>
> diff --git a/tools/libs/guest/xg_sr_common.h b/tools/libs/guest/xg_sr_common.h
> index 13fcc47420..d440281cc1 100644
> --- a/tools/libs/guest/xg_sr_common.h
> +++ b/tools/libs/guest/xg_sr_common.h
> @@ -298,6 +298,9 @@ struct xc_sr_context
>
> /* Sender has invoked verify mode on the stream. */
> bool verify;
> +
> + /* Domain context blob. */
> + struct xc_sr_blob context;
We already have
ctx->x86.hvm.restore.context
and are now gaining
ctx->restore.context
This is concerning close to being ambiguous. How about dom_context ?
Also, you leak the memory allocation. Free it in xg_sr_restore.c:cleanup().
> diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c
> index b57a787519..453a383ba4 100644
> --- a/tools/libs/guest/xg_sr_restore.c
> +++ b/tools/libs/guest/xg_sr_restore.c
> @@ -529,6 +529,20 @@ static int send_checkpoint_dirty_pfn_list(struct xc_sr_context *ctx)
> return rc;
> }
>
> +static int stream_complete(struct xc_sr_context *ctx)
> +{
> + xc_interface *xch = ctx->xch;
> + int rc;
> +
> + rc = xc_domain_setcontext(xch, ctx->domid,
> + ctx->restore.context.ptr,
> + ctx->restore.context.size);
> + if ( rc < 0 )
> + PERROR("Unable to restore domain context");
> +
> + return rc;
> +}
Please put this in the PV and HVM stream_complete() hooks.
This is somewhat of a layering violation and enforcing an order which
might not be appropriate in all cases.
~Andrew
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v9 0/8] domain context infrastructure
2020-09-24 13:10 [PATCH v9 0/8] domain context infrastructure Paul Durrant
` (7 preceding siblings ...)
2020-09-24 13:10 ` [PATCH v9 8/8] tools/libxc: add DOMAIN_CONTEXT records to the migration stream Paul Durrant
@ 2020-09-24 19:36 ` Lengyel, Tamas
2020-09-25 12:49 ` Paul Durrant
8 siblings, 1 reply; 41+ messages in thread
From: Lengyel, Tamas @ 2020-09-24 19:36 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Paul Durrant, Andrew Cooper, Daniel De Graaf, George Dunlap,
Ian Jackson, Jan Beulich, Julien Grall,
Marek Marczykowski-Górecki, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Wei Liu
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Paul
> Durrant
> Sent: Thursday, September 24, 2020 9:10 AM
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>;
> George Dunlap <george.dunlap@citrix.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall
> <julien@xen.org>; Marek Marczykowski-Górecki
> <marmarek@invisiblethingslab.com>; Roger Pau Monné
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Wei Liu
> <wl@xen.org>
> Subject: [PATCH v9 0/8] domain context infrastructure
>
> From: Paul Durrant <pdurrant@amazon.com>
>
> Paul Durrant (8):
> xen/common: introduce a new framework for save/restore of 'domain'
> context
> xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
> tools/misc: add xen-domctx to present domain context
> docs/specs: add missing definitions to libxc-migration-stream
> docs / tools: specific migration v4 to include DOMAIN_CONTEXT
> common/domain: add a domain context record for shared_info...
> x86/time: add a domain context record for tsc_info...
> tools/libxc: add DOMAIN_CONTEXT records to the migration stream...
Hi Paul,
Could you push a git branch somewhere for this series? I would like to see this being integrated with VM forking and if its not too much effort just create the patch for that so that it could be appended to the series.
Thanks,
Tamas
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH v9 0/8] domain context infrastructure
2020-09-24 19:36 ` [PATCH v9 0/8] domain context infrastructure Lengyel, Tamas
@ 2020-09-25 12:49 ` Paul Durrant
2020-09-28 14:16 ` Lengyel, Tamas
0 siblings, 1 reply; 41+ messages in thread
From: Paul Durrant @ 2020-09-25 12:49 UTC (permalink / raw)
To: 'Lengyel, Tamas', xen-devel
Cc: 'Paul Durrant', 'Andrew Cooper',
'Daniel De Graaf', 'George Dunlap',
'Ian Jackson', 'Jan Beulich',
'Julien Grall', 'Marek Marczykowski-Górecki',
'Roger Pau Monné', 'Stefano Stabellini',
'Volodymyr Babchuk', 'Wei Liu'
> -----Original Message-----
> From: Lengyel, Tamas <tamas.lengyel@intel.com>
> Sent: 24 September 2020 20:36
> To: Paul Durrant <paul@xen.org>; xen-devel@lists.xenproject.org
> Cc: Paul Durrant <pdurrant@amazon.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall <julien@xen.org>; Marek
> Marczykowski-Górecki <marmarek@invisiblethingslab.com>; Roger Pau Monné <roger.pau@citrix.com>;
> Stefano Stabellini <sstabellini@kernel.org>; Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Wei Liu
> <wl@xen.org>
> Subject: RE: [PATCH v9 0/8] domain context infrastructure
>
>
>
> > -----Original Message-----
> > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Paul
> > Durrant
> > Sent: Thursday, September 24, 2020 9:10 AM
> > To: xen-devel@lists.xenproject.org
> > Cc: Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> > <andrew.cooper3@citrix.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>;
> > George Dunlap <george.dunlap@citrix.com>; Ian Jackson
> > <ian.jackson@eu.citrix.com>; Jan Beulich <jbeulich@suse.com>; Julien Grall
> > <julien@xen.org>; Marek Marczykowski-Górecki
> > <marmarek@invisiblethingslab.com>; Roger Pau Monné
> > <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Wei Liu
> > <wl@xen.org>
> > Subject: [PATCH v9 0/8] domain context infrastructure
> >
> > From: Paul Durrant <pdurrant@amazon.com>
> >
> > Paul Durrant (8):
> > xen/common: introduce a new framework for save/restore of 'domain'
> > context
> > xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
> > tools/misc: add xen-domctx to present domain context
> > docs/specs: add missing definitions to libxc-migration-stream
> > docs / tools: specific migration v4 to include DOMAIN_CONTEXT
> > common/domain: add a domain context record for shared_info...
> > x86/time: add a domain context record for tsc_info...
> > tools/libxc: add DOMAIN_CONTEXT records to the migration stream...
>
>
> Hi Paul,
> Could you push a git branch somewhere for this series? I would like to see this being integrated with
> VM forking and if its not too much effort just create the patch for that so that it could be appended
> to the series.
>
Hi Tamas,
Done. See https://xenbits.xen.org/gitweb/?p=people/pauldu/xen.git;a=shortlog;h=refs/heads/domain-save14
Cheers,
Paul
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [PATCH v9 0/8] domain context infrastructure
2020-09-25 12:49 ` Paul Durrant
@ 2020-09-28 14:16 ` Lengyel, Tamas
2020-09-29 11:53 ` Durrant, Paul
0 siblings, 1 reply; 41+ messages in thread
From: Lengyel, Tamas @ 2020-09-28 14:16 UTC (permalink / raw)
To: paul, xen-devel
Cc: 'Paul Durrant', 'Andrew Cooper',
'Daniel De Graaf', 'George Dunlap',
'Ian Jackson', 'Jan Beulich',
'Julien Grall', 'Marek Marczykowski-Górecki',
'Roger Pau Monné', 'Stefano Stabellini',
'Volodymyr Babchuk', 'Wei Liu'
> > Hi Paul,
> > Could you push a git branch somewhere for this series? I would like to
> > see this being integrated with VM forking and if its not too much
> > effort just create the patch for that so that it could be appended to the
> series.
> >
>
> Hi Tamas,
>
> Done. See
> https://xenbits.xen.org/gitweb/?p=people/pauldu/xen.git;a=shortlog;h=refs/h
> eads/domain-save14
>
> Cheers,
>
> Paul
Hi Paul,
I added a small patch that would save & load the PV context from one domain to another that would be called during VM forking. Please take a look at https://xenbits.xen.org/gitweb/?p=people/tklengyel/xen.git;a=commitdiff;h=1843ca7302e415317fdb9a63b3a4d29a385dc766;hp=8149296fdf80c73727e61cea6fe3251aecf8b333. I called the function copy_pv_domaincontext for now as that seemed like the most appropriate description for it. Please let me know if this looks good to you. I'm still testing it but if everything checks out it would be nice to just append this patch to your series.
Thanks,
Tamas
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v9 0/8] domain context infrastructure
2020-09-28 14:16 ` Lengyel, Tamas
@ 2020-09-29 11:53 ` Durrant, Paul
2020-09-29 12:05 ` Tamas K Lengyel
0 siblings, 1 reply; 41+ messages in thread
From: Durrant, Paul @ 2020-09-29 11:53 UTC (permalink / raw)
To: Lengyel, Tamas, paul, xen-devel
Cc: 'Andrew Cooper', 'Daniel De Graaf',
'George Dunlap', 'Ian Jackson',
'Jan Beulich', 'Julien Grall',
'Marek Marczykowski-Górecki',
'Roger Pau Monné', 'Stefano Stabellini',
'Volodymyr Babchuk', 'Wei Liu'
> -----Original Message-----
> From: Lengyel, Tamas <tamas.lengyel@intel.com>
> Sent: 28 September 2020 15:17
> To: paul@xen.org; xen-devel@lists.xenproject.org
> Cc: Durrant, Paul <pdurrant@amazon.co.uk>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'Daniel De
> Graaf' <dgdegra@tycho.nsa.gov>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
> <ian.jackson@eu.citrix.com>; 'Jan Beulich' <jbeulich@suse.com>; 'Julien Grall' <julien@xen.org>;
> 'Marek Marczykowski-Górecki' <marmarek@invisiblethingslab.com>; 'Roger Pau Monné'
> <roger.pau@citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Volodymyr Babchuk'
> <Volodymyr_Babchuk@epam.com>; 'Wei Liu' <wl@xen.org>
> Subject: RE: [EXTERNAL] [PATCH v9 0/8] domain context infrastructure
>
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
>
>
>
> > > Hi Paul,
> > > Could you push a git branch somewhere for this series? I would like to
> > > see this being integrated with VM forking and if its not too much
> > > effort just create the patch for that so that it could be appended to the
> > series.
> > >
> >
> > Hi Tamas,
> >
> > Done. See
> > https://xenbits.xen.org/gitweb/?p=people/pauldu/xen.git;a=shortlog;h=refs/h
> > eads/domain-save14
> >
> > Cheers,
> >
> > Paul
>
> Hi Paul,
> I added a small patch that would save & load the PV context from one domain to another that would be
> called during VM forking. Please take a look at
> https://xenbits.xen.org/gitweb/?p=people/tklengyel/xen.git;a=commitdiff;h=1843ca7302e415317fdb9a63b3a4
> d29a385dc766;hp=8149296fdf80c73727e61cea6fe3251aecf8b333. I called the function copy_pv_domaincontext
> for now as that seemed like the most appropriate description for it. Please let me know if this looks
> good to you. I'm still testing it but if everything checks out it would be nice to just append this
> patch to your series.
Hi Tamas,
The code structure appears to be ok... just some cosmetic tweaks:
- I think you should call the function simply 'copy_domaincontext' as the idea is that all state (including what is now in hvm context) will be consolidated
- The prevailing style in domctl.c AFAICS is that assignments are mostly not done inside if statements. Personally I think this is a good thing.
Once you have something ready to go then I'd be happy to tag it onto my series if I need to do a v10... but I'm currently hoping that won't be necessary.
Cheers,
Paul
>
> Thanks,
> Tamas
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v9 0/8] domain context infrastructure
2020-09-29 11:53 ` Durrant, Paul
@ 2020-09-29 12:05 ` Tamas K Lengyel
2020-09-29 12:13 ` Durrant, Paul
0 siblings, 1 reply; 41+ messages in thread
From: Tamas K Lengyel @ 2020-09-29 12:05 UTC (permalink / raw)
To: Durrant, Paul
Cc: Lengyel, Tamas, paul, xen-devel, Andrew Cooper, Daniel De Graaf,
George Dunlap, Ian Jackson, Jan Beulich, Julien Grall,
Marek Marczykowski-Górecki, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Wei Liu
On Tue, Sep 29, 2020 at 7:54 AM Durrant, Paul <pdurrant@amazon.co.uk> wrote:
>
> > -----Original Message-----
> > From: Lengyel, Tamas <tamas.lengyel@intel.com>
> > Sent: 28 September 2020 15:17
> > To: paul@xen.org; xen-devel@lists.xenproject.org
> > Cc: Durrant, Paul <pdurrant@amazon.co.uk>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'Daniel De
> > Graaf' <dgdegra@tycho.nsa.gov>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
> > <ian.jackson@eu.citrix.com>; 'Jan Beulich' <jbeulich@suse.com>; 'Julien Grall' <julien@xen.org>;
> > 'Marek Marczykowski-Górecki' <marmarek@invisiblethingslab.com>; 'Roger Pau Monné'
> > <roger.pau@citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Volodymyr Babchuk'
> > <Volodymyr_Babchuk@epam.com>; 'Wei Liu' <wl@xen.org>
> > Subject: RE: [EXTERNAL] [PATCH v9 0/8] domain context infrastructure
> >
> > CAUTION: This email originated from outside of the organization. Do not click links or open
> > attachments unless you can confirm the sender and know the content is safe.
> >
> >
> >
> > > > Hi Paul,
> > > > Could you push a git branch somewhere for this series? I would like to
> > > > see this being integrated with VM forking and if its not too much
> > > > effort just create the patch for that so that it could be appended to the
> > > series.
> > > >
> > >
> > > Hi Tamas,
> > >
> > > Done. See
> > > https://xenbits.xen.org/gitweb/?p=people/pauldu/xen.git;a=shortlog;h=refs/h
> > > eads/domain-save14
> > >
> > > Cheers,
> > >
> > > Paul
> >
> > Hi Paul,
> > I added a small patch that would save & load the PV context from one domain to another that would be
> > called during VM forking. Please take a look at
> > https://xenbits.xen.org/gitweb/?p=people/tklengyel/xen.git;a=commitdiff;h=1843ca7302e415317fdb9a63b3a4
> > d29a385dc766;hp=8149296fdf80c73727e61cea6fe3251aecf8b333. I called the function copy_pv_domaincontext
> > for now as that seemed like the most appropriate description for it. Please let me know if this looks
> > good to you. I'm still testing it but if everything checks out it would be nice to just append this
> > patch to your series.
>
> Hi Tamas,
>
> The code structure appears to be ok... just some cosmetic tweaks:
>
> - I think you should call the function simply 'copy_domaincontext' as the idea is that all state (including what is now in hvm context) will be consolidated
Sure, I wasn't entirely clear about whether this will be limited to PV
context or if it will eventually add the hvm stuff too. Right now I
still would have to do that separately.
> - The prevailing style in domctl.c AFAICS is that assignments are mostly not done inside if statements. Personally I think this is a good thing.
I think it cuts down on function sizes when all that is being done
after an assigment is a NULL-check. No need for a separate line for it
but I also don't care that much. So if it's more important to whoever
maintains this to keep the style consistent in this regard I can
change it.
>
> Once you have something ready to go then I'd be happy to tag it onto my series if I need to do a v10... but I'm currently hoping that won't be necessary.
I think I'll wait until HVM context is included in the framework as
well so that we can just switch over everything at once.
Tamas
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v9 0/8] domain context infrastructure
2020-09-29 12:05 ` Tamas K Lengyel
@ 2020-09-29 12:13 ` Durrant, Paul
2020-09-29 14:19 ` Lengyel, Tamas
0 siblings, 1 reply; 41+ messages in thread
From: Durrant, Paul @ 2020-09-29 12:13 UTC (permalink / raw)
To: Tamas K Lengyel
Cc: Lengyel, Tamas, paul, xen-devel, Andrew Cooper, Daniel De Graaf,
George Dunlap, Ian Jackson, Jan Beulich, Julien Grall,
Marek Marczykowski-Górecki, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Wei Liu
> -----Original Message-----
> From: Tamas K Lengyel <tamas.k.lengyel@gmail.com>
> Sent: 29 September 2020 13:06
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: Lengyel, Tamas <tamas.lengyel@intel.com>; paul@xen.org; xen-devel@lists.xenproject.org; Andrew
> Cooper <andrew.cooper3@citrix.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>; George Dunlap
> <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>; Roger Pau
> Monné <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>; Volodymyr Babchuk
> <Volodymyr_Babchuk@epam.com>; Wei Liu <wl@xen.org>
> Subject: RE: [EXTERNAL] [PATCH v9 0/8] domain context infrastructure
>
> CAUTION: This email originated from outside of the organization. Do not click links or open
> attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Tue, Sep 29, 2020 at 7:54 AM Durrant, Paul <pdurrant@amazon.co.uk> wrote:
> >
> > > -----Original Message-----
> > > From: Lengyel, Tamas <tamas.lengyel@intel.com>
> > > Sent: 28 September 2020 15:17
> > > To: paul@xen.org; xen-devel@lists.xenproject.org
> > > Cc: Durrant, Paul <pdurrant@amazon.co.uk>; 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'Daniel De
> > > Graaf' <dgdegra@tycho.nsa.gov>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
> > > <ian.jackson@eu.citrix.com>; 'Jan Beulich' <jbeulich@suse.com>; 'Julien Grall' <julien@xen.org>;
> > > 'Marek Marczykowski-Górecki' <marmarek@invisiblethingslab.com>; 'Roger Pau Monné'
> > > <roger.pau@citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>; 'Volodymyr Babchuk'
> > > <Volodymyr_Babchuk@epam.com>; 'Wei Liu' <wl@xen.org>
> > > Subject: RE: [EXTERNAL] [PATCH v9 0/8] domain context infrastructure
> > >
> > > CAUTION: This email originated from outside of the organization. Do not click links or open
> > > attachments unless you can confirm the sender and know the content is safe.
> > >
> > >
> > >
> > > > > Hi Paul,
> > > > > Could you push a git branch somewhere for this series? I would like to
> > > > > see this being integrated with VM forking and if its not too much
> > > > > effort just create the patch for that so that it could be appended to the
> > > > series.
> > > > >
> > > >
> > > > Hi Tamas,
> > > >
> > > > Done. See
> > > > https://xenbits.xen.org/gitweb/?p=people/pauldu/xen.git;a=shortlog;h=refs/h
> > > > eads/domain-save14
> > > >
> > > > Cheers,
> > > >
> > > > Paul
> > >
> > > Hi Paul,
> > > I added a small patch that would save & load the PV context from one domain to another that would
> be
> > > called during VM forking. Please take a look at
> > >
> https://xenbits.xen.org/gitweb/?p=people/tklengyel/xen.git;a=commitdiff;h=1843ca7302e415317fdb9a63b3a4
> > > d29a385dc766;hp=8149296fdf80c73727e61cea6fe3251aecf8b333. I called the function
> copy_pv_domaincontext
> > > for now as that seemed like the most appropriate description for it. Please let me know if this
> looks
> > > good to you. I'm still testing it but if everything checks out it would be nice to just append
> this
> > > patch to your series.
> >
> > Hi Tamas,
> >
> > The code structure appears to be ok... just some cosmetic tweaks:
> >
> > - I think you should call the function simply 'copy_domaincontext' as the idea is that all state
> (including what is now in hvm context) will be consolidated
>
> Sure, I wasn't entirely clear about whether this will be limited to PV
> context or if it will eventually add the hvm stuff too. Right now I
> still would have to do that separately.
>
> > - The prevailing style in domctl.c AFAICS is that assignments are mostly not done inside if
> statements. Personally I think this is a good thing.
>
> I think it cuts down on function sizes when all that is being done
> after an assigment is a NULL-check. No need for a separate line for it
> but I also don't care that much. So if it's more important to whoever
> maintains this to keep the style consistent in this regard I can
> change it.
>
> >
> > Once you have something ready to go then I'd be happy to tag it onto my series if I need to do a
> v10... but I'm currently hoping that won't be necessary.
>
> I think I'll wait until HVM context is included in the framework as
> well so that we can just switch over everything at once.
>
It may be a while before I have everything moved over so you may still want to go ahead with this patch if the delay is likely to block things. Also, without this I assume any records I port over from HVM context (and hence remove the save code) are going to cause breakage for VM forking?
Paul
> Tamas
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [PATCH v9 0/8] domain context infrastructure
2020-09-29 12:13 ` Durrant, Paul
@ 2020-09-29 14:19 ` Lengyel, Tamas
0 siblings, 0 replies; 41+ messages in thread
From: Lengyel, Tamas @ 2020-09-29 14:19 UTC (permalink / raw)
To: Durrant, Paul
Cc: paul, xen-devel, Andrew Cooper, Daniel De Graaf, George Dunlap,
Ian Jackson, Jan Beulich, Julien Grall,
Marek Marczykowski-Górecki, Roger Pau Monné,
Stefano Stabellini, Volodymyr Babchuk, Wei Liu
> -----Original Message-----
> From: Durrant, Paul <pdurrant@amazon.co.uk>
> Sent: Tuesday, September 29, 2020 8:14 AM
> To: Tamas K Lengyel <tamas.k.lengyel@gmail.com>
> Cc: Lengyel, Tamas <tamas.lengyel@intel.com>; paul@xen.org; xen-
> devel@lists.xenproject.org; Andrew Cooper <andrew.cooper3@citrix.com>;
> Daniel De Graaf <dgdegra@tycho.nsa.gov>; George Dunlap
> <george.dunlap@citrix.com>; Ian Jackson <ian.jackson@eu.citrix.com>; Jan
> Beulich <jbeulich@suse.com>; Julien Grall <julien@xen.org>; Marek
> Marczykowski-Górecki <marmarek@invisiblethingslab.com>; Roger Pau Monné
> <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Wei Liu
> <wl@xen.org>
> Subject: RE: [PATCH v9 0/8] domain context infrastructure
>
> > -----Original Message-----
> > From: Tamas K Lengyel <tamas.k.lengyel@gmail.com>
> > Sent: 29 September 2020 13:06
> > To: Durrant, Paul <pdurrant@amazon.co.uk>
> > Cc: Lengyel, Tamas <tamas.lengyel@intel.com>; paul@xen.org;
> > xen-devel@lists.xenproject.org; Andrew Cooper
> > <andrew.cooper3@citrix.com>; Daniel De Graaf <dgdegra@tycho.nsa.gov>;
> > George Dunlap <george.dunlap@citrix.com>; Ian Jackson
> > <ian.jackson@eu.citrix.com>; Jan Beulich <jbeulich@suse.com>; Julien
> > Grall <julien@xen.org>; Marek Marczykowski-Górecki
> > <marmarek@invisiblethingslab.com>; Roger Pau Monné
> > <roger.pau@citrix.com>; Stefano Stabellini <sstabellini@kernel.org>;
> > Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; Wei Liu
> <wl@xen.org>
> > Subject: RE: [EXTERNAL] [PATCH v9 0/8] domain context infrastructure
> >
> > CAUTION: This email originated from outside of the organization. Do
> > not click links or open attachments unless you can confirm the sender and
> know the content is safe.
> >
> >
> >
> > On Tue, Sep 29, 2020 at 7:54 AM Durrant, Paul <pdurrant@amazon.co.uk>
> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Lengyel, Tamas <tamas.lengyel@intel.com>
> > > > Sent: 28 September 2020 15:17
> > > > To: paul@xen.org; xen-devel@lists.xenproject.org
> > > > Cc: Durrant, Paul <pdurrant@amazon.co.uk>; 'Andrew Cooper'
> > > > <andrew.cooper3@citrix.com>; 'Daniel De Graaf'
> <dgdegra@tycho.nsa.gov>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian
> Jackson'
> > > > <ian.jackson@eu.citrix.com>; 'Jan Beulich' <jbeulich@suse.com>;
> > > > 'Julien Grall' <julien@xen.org>; 'Marek Marczykowski-Górecki'
> <marmarek@invisiblethingslab.com>; 'Roger Pau Monné'
> > > > <roger.pau@citrix.com>; 'Stefano Stabellini' <sstabellini@kernel.org>;
> 'Volodymyr Babchuk'
> > > > <Volodymyr_Babchuk@epam.com>; 'Wei Liu' <wl@xen.org>
> > > > Subject: RE: [EXTERNAL] [PATCH v9 0/8] domain context
> > > > infrastructure
> > > >
> > > > CAUTION: This email originated from outside of the organization.
> > > > Do not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> > > >
> > > >
> > > >
> > > > > > Hi Paul,
> > > > > > Could you push a git branch somewhere for this series? I would
> > > > > > like to see this being integrated with VM forking and if its
> > > > > > not too much effort just create the patch for that so that it
> > > > > > could be appended to the
> > > > > series.
> > > > > >
> > > > >
> > > > > Hi Tamas,
> > > > >
> > > > > Done. See
> > > > > https://xenbits.xen.org/gitweb/?p=people/pauldu/xen.git;a=shortl
> > > > > og;h=refs/h
> > > > > eads/domain-save14
> > > > >
> > > > > Cheers,
> > > > >
> > > > > Paul
> > > >
> > > > Hi Paul,
> > > > I added a small patch that would save & load the PV context from
> > > > one domain to another that would
> > be
> > > > called during VM forking. Please take a look at
> > > >
> > https://xenbits.xen.org/gitweb/?p=people/tklengyel/xen.git;a=commitdif
> > f;h=1843ca7302e415317fdb9a63b3a4
> > > > d29a385dc766;hp=8149296fdf80c73727e61cea6fe3251aecf8b333. I called
> > > > the function
> > copy_pv_domaincontext
> > > > for now as that seemed like the most appropriate description for
> > > > it. Please let me know if this
> > looks
> > > > good to you. I'm still testing it but if everything checks out it
> > > > would be nice to just append
> > this
> > > > patch to your series.
> > >
> > > Hi Tamas,
> > >
> > > The code structure appears to be ok... just some cosmetic tweaks:
> > >
> > > - I think you should call the function simply 'copy_domaincontext'
> > > as the idea is that all state
> > (including what is now in hvm context) will be consolidated
> >
> > Sure, I wasn't entirely clear about whether this will be limited to PV
> > context or if it will eventually add the hvm stuff too. Right now I
> > still would have to do that separately.
> >
> > > - The prevailing style in domctl.c AFAICS is that assignments are
> > > mostly not done inside if
> > statements. Personally I think this is a good thing.
> >
> > I think it cuts down on function sizes when all that is being done
> > after an assigment is a NULL-check. No need for a separate line for it
> > but I also don't care that much. So if it's more important to whoever
> > maintains this to keep the style consistent in this regard I can
> > change it.
> >
> > >
> > > Once you have something ready to go then I'd be happy to tag it
> > > onto my series if I need to do a
> > v10... but I'm currently hoping that won't be necessary.
> >
> > I think I'll wait until HVM context is included in the framework as
> > well so that we can just switch over everything at once.
> >
>
> It may be a while before I have everything moved over so you may still want to
> go ahead with this patch if the delay is likely to block things. Also, without this I
> assume any records I port over from HVM context (and hence remove the save
> code) are going to cause breakage for VM forking?
If you do remove the existing hvm_save/hvm_load functions then yes, that would pretty much break VM forking right away. We use those in hvm_copy_context_and_params. So if you do make significant changes in there I would ask that you at least compile test with CONFIG_MEM_SHARING enabled. That would be the point where we would want to start using this new copy_domaincontext function. Right now just for the PV stuff it's not critical as we do copy those internally ourself, it would just be nicer to use this plumbing you add that will be shared with domain save/restore/migrate.
Tamas
^ permalink raw reply [flat|nested] 41+ messages in thread