* [PATCH v3 0/5] domain context infrastructure
@ 2020-05-14 10:44 Paul Durrant
2020-05-14 10:44 ` [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context Paul Durrant
` (4 more replies)
0 siblings, 5 replies; 20+ messages in thread
From: Paul Durrant @ 2020-05-14 10:44 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant
From: Paul Durrant <pdurrant@amazon.com>
Paul Durrant (5):
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
common/domain: add a domain context record for shared_info...
tools/libxc: make use of domain context SHARED_INFO record...
.gitignore | 1 +
tools/flask/policy/modules/xen.if | 4 +-
tools/libxc/include/xenctrl.h | 5 +
tools/libxc/xc_domain.c | 54 +++++
tools/libxc/xc_sr_common.c | 58 +++++
tools/libxc/xc_sr_common.h | 11 +-
tools/libxc/xc_sr_common_x86_pv.c | 47 ++++
tools/libxc/xc_sr_common_x86_pv.h | 3 +
tools/libxc/xc_sr_restore_x86_pv.c | 40 ++--
tools/libxc/xc_sr_save_x86_pv.c | 26 +-
tools/libxc/xg_save_restore.h | 1 +
tools/misc/Makefile | 4 +
tools/misc/xen-domctx.c | 273 +++++++++++++++++++++
xen/common/Makefile | 1 +
xen/common/domain.c | 60 +++++
xen/common/domctl.c | 167 +++++++++++++
xen/common/save.c | 313 +++++++++++++++++++++++++
xen/include/public/arch-arm/hvm/save.h | 5 +
xen/include/public/arch-x86/hvm/save.h | 5 +
xen/include/public/domctl.h | 41 ++++
xen/include/public/save.h | 89 +++++++
xen/include/xen/save.h | 165 +++++++++++++
xen/xsm/flask/hooks.c | 6 +
xen/xsm/flask/policy/access_vectors | 4 +
24 files changed, 1332 insertions(+), 51 deletions(-)
create mode 100644 tools/misc/xen-domctx.c
create mode 100644 xen/common/save.c
create mode 100644 xen/include/public/save.h
create mode 100644 xen/include/xen/save.h
--
2.20.1
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
2020-05-14 10:44 [PATCH v3 0/5] domain context infrastructure Paul Durrant
@ 2020-05-14 10:44 ` Paul Durrant
2020-05-19 13:03 ` Jan Beulich
2020-05-14 10:44 ` [PATCH v3 2/5] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext Paul Durrant
` (3 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2020-05-14 10:44 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Paul Durrant,
Andrew Cooper, Paul Durrant, Ian Jackson, George Dunlap,
Jan Beulich, 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>
---
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: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
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 | 313 +++++++++++++++++++++++++
xen/include/public/arch-arm/hvm/save.h | 5 +
xen/include/public/arch-x86/hvm/save.h | 5 +
xen/include/public/save.h | 80 +++++++
xen/include/xen/save.h | 165 +++++++++++++
6 files changed, 569 insertions(+)
create mode 100644 xen/common/save.c
create mode 100644 xen/include/public/save.h
create mode 100644 xen/include/xen/save.h
diff --git a/xen/common/Makefile b/xen/common/Makefile
index e8cde65370..90553ba5d7 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -37,6 +37,7 @@ obj-y += radix-tree.o
obj-y += rbtree.o
obj-y += rcupdate.o
obj-y += rwlock.o
+obj-y += save.o
obj-y += shutdown.o
obj-y += softirq.o
obj-y += sort.o
diff --git a/xen/common/save.c b/xen/common/save.c
new file mode 100644
index 0000000000..62a2b7c5f6
--- /dev/null
+++ b/xen/common/save.c
@@ -0,0 +1,313 @@
+/*
+ * save.c: Save and restore PV guest state common to all domain types.
+ *
+ * Copyright Amazon.com Inc. or its affiliates.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/compile.h>
+#include <xen/save.h>
+
+struct domain_context {
+ struct domain *domain;
+ const char *name; /* for logging purposes */
+ struct domain_save_descriptor desc;
+ size_t len; /* for internal accounting */
+ union {
+ struct domain_save_ops *save;
+ struct domain_load_ops *load;
+ } ops;
+ void *priv;
+ bool log;
+};
+
+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,
+ const char *name, 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 */
+
+ c->desc.instance = instance;
+
+ rc = c->ops.save->begin(c->priv, &c->desc);
+ if ( rc )
+ return rc;
+
+ c->name = name;
+
+ 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;
+ uint8_t pad[DOMAIN_SAVE_ALIGN] = {};
+ size_t len = ROUNDUP(c->len, DOMAIN_SAVE_ALIGN) - c->len; /* padding */
+ int rc;
+
+ if ( len )
+ {
+ rc = domain_save_data(c, pad, len);
+
+ if ( rc )
+ return rc;
+ }
+ ASSERT(IS_ALIGNED(c->len, DOMAIN_SAVE_ALIGN));
+
+ if ( c->log )
+ 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, struct domain_save_ops *ops, void *priv,
+ bool dry_run)
+{
+ struct domain_context c = {
+ .domain = d,
+ .ops.save = ops,
+ .priv = priv,
+ .log = !dry_run,
+ };
+ static struct domain_save_header h = {
+ .magic = DOMAIN_SAVE_MAGIC,
+ .xen_major = XEN_VERSION,
+ .xen_minor = XEN_SUBVERSION,
+ .version = DOMAIN_SAVE_VERSION,
+ };
+ struct domain_save_end e = {};
+ unsigned int i;
+ int rc;
+
+ ASSERT(d != current->domain);
+ domain_pause(d);
+
+ 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;
+
+ memset(&c.desc, 0, sizeof(c.desc));
+ c.desc.typecode = i;
+
+ rc = save(d, &c, dry_run);
+ if ( rc )
+ goto out;
+ }
+
+ 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,
+ const char *name, 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;
+
+ c->name = name;
+
+ 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)
+{
+ 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 ( pad )
+ return -EINVAL;
+ }
+
+ if ( c->log )
+ 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, struct domain_load_ops *ops, void *priv)
+{
+ struct domain_context c = {
+ .domain = d,
+ .ops.load = ops,
+ .priv = priv,
+ .log = true,
+ };
+ unsigned int instance;
+ struct domain_save_header h;
+ int rc;
+
+ ASSERT(d != current->domain);
+
+ rc = c.ops.load->read(c.priv, &c.desc, sizeof(c.desc));
+ if ( rc )
+ return rc;
+
+ rc = DOMAIN_LOAD_ENTRY(HEADER, &c, &instance, &h, sizeof(h));
+ if ( rc )
+ return rc;
+
+ if ( instance || h.magic != DOMAIN_SAVE_MAGIC ||
+ h.version != DOMAIN_SAVE_VERSION )
+ return -EINVAL;
+
+ domain_pause(d);
+
+ for (;;)
+ {
+ unsigned int i;
+ domain_load_handler load;
+
+ rc = c.ops.load->read(c.priv, &c.desc, sizeof(c.desc));
+ if ( rc )
+ return rc;
+
+ rc = -EINVAL;
+
+ if ( c.desc.typecode == DOMAIN_SAVE_CODE(END) )
+ {
+ struct domain_save_end e;
+
+ rc = DOMAIN_LOAD_ENTRY(END, &c, &instance, NULL, sizeof(e));
+
+ if ( instance )
+ return -EINVAL;
+
+ break;
+ }
+
+ i = c.desc.typecode;
+ if ( i >= ARRAY_SIZE(handlers) )
+ break;
+
+ 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..834c031c51
--- /dev/null
+++ b/xen/include/public/save.h
@@ -0,0 +1,80 @@
+/*
+ * save.h
+ *
+ * Structure definitions for common PV/HVM domain state that is held by
+ * Xen and must be saved along with the domain's memory.
+ *
+ * Copyright Amazon.com Inc. or its affiliates.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+#ifndef XEN_PUBLIC_SAVE_H
+#define XEN_PUBLIC_SAVE_H
+
+#include "xen.h"
+
+#if defined(__XEN__) || defined(__XEN_TOOLS__)
+
+/* Entry data is preceded by a descriptor */
+struct domain_save_descriptor {
+ uint16_t typecode;
+
+ /*
+ * Instance number of the entry (since there may by multiple of some
+ * types of entry).
+ */
+ uint16_t instance;
+
+ /* Entry length not including this descriptor */
+ uint32_t length;
+};
+
+/*
+ * Each entry has a type associated with it. DECLARE_DOMAIN_SAVE_TYPE
+ * binds these things together.
+ */
+#define DECLARE_DOMAIN_SAVE_TYPE(_x, _code, _type) \
+ struct DOMAIN_SAVE_TYPE_##_x { char c[_code]; _type t; };
+
+#define DOMAIN_SAVE_CODE(_x) \
+ (sizeof(((struct DOMAIN_SAVE_TYPE_##_x *)(0))->c))
+#define DOMAIN_SAVE_TYPE(_x) \
+ typeof(((struct DOMAIN_SAVE_TYPE_##_x *)(0))->t)
+
+/* Terminating entry */
+struct domain_save_end {};
+DECLARE_DOMAIN_SAVE_TYPE(END, 0, struct domain_save_end);
+
+#define DOMAIN_SAVE_MAGIC 0x53415645
+#define DOMAIN_SAVE_VERSION 0x00000001
+
+/* Initial entry */
+struct domain_save_header {
+ uint32_t magic; /* Must be DOMAIN_SAVE_MAGIC */
+ 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..c5386b780c
--- /dev/null
+++ b/xen/include/xen/save.h
@@ -0,0 +1,165 @@
+/*
+ * save.h: support routines for save/restore
+ *
+ * Copyright Amazon.com Inc. or its affiliates.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef XEN_SAVE_H
+#define XEN_SAVE_H
+
+#include <xen/init.h>
+#include <xen/sched.h>
+#include <xen/types.h>
+
+#include <public/save.h>
+
+struct domain_context;
+
+int domain_save_begin(struct domain_context *c, unsigned int typecode,
+ const char *name, unsigned int instance);
+
+#define DOMAIN_SAVE_BEGIN(_x, _c, _instance) \
+ domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_instance))
+
+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, const char *name,
+ unsigned int instance, const void *src,
+ size_t len)
+{
+ int rc;
+
+ rc = domain_save_begin(c, typecode, name, 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, _instance, _src, _len) \
+ domain_save_entry((_c), DOMAIN_SAVE_CODE(_x), #_x, (_instance), \
+ (_src), (_len))
+
+int domain_load_begin(struct domain_context *c, unsigned int typecode,
+ const char *name, unsigned int *instance);
+
+#define DOMAIN_LOAD_BEGIN(_x, _c, _instance) \
+ domain_load_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_instance))
+
+int domain_load_data(struct domain_context *c, void *data, size_t len);
+int domain_load_end(struct domain_context *c);
+
+static inline int domain_load_entry(struct domain_context *c,
+ unsigned int typecode, const char *name,
+ unsigned int *instance, void *dst,
+ size_t len)
+{
+ int rc;
+
+ rc = domain_load_begin(c, typecode, name, instance);
+ if ( rc )
+ return rc;
+
+ rc = domain_load_data(c, dst, len);
+ if ( rc )
+ return rc;
+
+ return domain_load_end(c);
+}
+
+#define DOMAIN_LOAD_ENTRY(_x, _c, _instance, _dst, _len) \
+ domain_load_entry((_c), DOMAIN_SAVE_CODE(_x), #_x, (_instance), \
+ (_dst), (_len))
+
+/*
+ * 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 restore handlers. Save handlers will be invoked
+ * in order of DOMAIN_SAVE_CODE().
+ */
+#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load) \
+ static int __init __domain_register_##_x##_save_restore(void) \
+ { \
+ domain_register_save_type( \
+ DOMAIN_SAVE_CODE(_x), \
+ #_x, \
+ &(_save), \
+ &(_load)); \
+ \
+ return 0; \
+ } \
+ __initcall(__domain_register_##_x##_save_restore);
+
+/* 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, struct domain_save_ops *ops, void *priv,
+ bool dry_run);
+int domain_load(struct domain *d, struct domain_load_ops *ops, void *priv);
+
+#endif /* XEN_SAVE_H */
--
2.20.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 2/5] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
2020-05-14 10:44 [PATCH v3 0/5] domain context infrastructure Paul Durrant
2020-05-14 10:44 ` [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context Paul Durrant
@ 2020-05-14 10:44 ` Paul Durrant
2020-05-19 13:49 ` Jan Beulich
2020-05-14 10:44 ` [PATCH v3 3/5] tools/misc: add xen-domctx to present domain context Paul Durrant
` (2 subsequent siblings)
4 siblings, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2020-05-14 10:44 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Paul Durrant,
Andrew Cooper, Paul Durrant, Ian Jackson, George Dunlap,
Jan Beulich, Daniel De Graaf
These domctls provide a mechanism to get and set domain context from
the toolstack.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
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: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
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/libxc/include/xenctrl.h | 5 +
tools/libxc/xc_domain.c | 54 +++++++++
xen/common/domctl.c | 167 ++++++++++++++++++++++++++++
xen/include/public/domctl.h | 41 +++++++
xen/xsm/flask/hooks.c | 6 +
xen/xsm/flask/policy/access_vectors | 4 +
7 files changed, 279 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/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 45ff7db1e8..0ce2372e2f 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/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/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 71829c2bce..212d1489dd 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -537,6 +537,60 @@ 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;
+ DECLARE_HYPERCALL_BOUNCE(ctxt_buf, *size, XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+ if ( xc_hypercall_bounce_pre(xch, ctxt_buf) )
+ return -1;
+
+ domctl.cmd = XEN_DOMCTL_getdomaincontext;
+ domctl.domain = domid;
+ domctl.u.getdomaincontext.size = *size;
+ 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;
+ DECLARE_HYPERCALL_BOUNCE_IN(ctxt_buf, size);
+
+ if ( xc_hypercall_bounce_pre(xch, ctxt_buf) )
+ return -1;
+
+ domctl.cmd = XEN_DOMCTL_setdomaincontext;
+ domctl.domain = domid;
+ domctl.u.setdomaincontext.size = size;
+ 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 a69b3b59a8..c37d2ad366 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>
@@ -358,6 +360,162 @@ 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 ( 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;
+
+ 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;
@@ -942,6 +1100,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 1ad34c35eb..1b133bda59 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -1129,6 +1129,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
@@ -1210,6 +1247,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
@@ -1270,6 +1309,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 4649e6fd95..6f3db276ef 100644
--- a/xen/xsm/flask/hooks.c
+++ b/xen/xsm/flask/hooks.c
@@ -745,6 +745,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 c055c14c26..fccfb9de82 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] 20+ messages in thread
* [PATCH v3 3/5] tools/misc: add xen-domctx to present domain context
2020-05-14 10:44 [PATCH v3 0/5] domain context infrastructure Paul Durrant
2020-05-14 10:44 ` [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context Paul Durrant
2020-05-14 10:44 ` [PATCH v3 2/5] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext Paul Durrant
@ 2020-05-14 10:44 ` Paul Durrant
2020-05-14 10:44 ` [PATCH v3 4/5] common/domain: add a domain context record for shared_info Paul Durrant
2020-05-14 10:44 ` [PATCH v3 5/5] tools/libxc: make use of domain context SHARED_INFO record Paul Durrant
4 siblings, 0 replies; 20+ messages in thread
From: Paul Durrant @ 2020-05-14 10:44 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu, Paul Durrant
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>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
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 bfa53723b3..96fd7527bf 100644
--- a/.gitignore
+++ b/.gitignore
@@ -209,6 +209,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 63947bfadc..ef25524354 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -30,6 +30,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/
@@ -108,6 +109,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] 20+ messages in thread
* [PATCH v3 4/5] common/domain: add a domain context record for shared_info...
2020-05-14 10:44 [PATCH v3 0/5] domain context infrastructure Paul Durrant
` (2 preceding siblings ...)
2020-05-14 10:44 ` [PATCH v3 3/5] tools/misc: add xen-domctx to present domain context Paul Durrant
@ 2020-05-14 10:44 ` Paul Durrant
2020-05-19 14:07 ` Jan Beulich
2020-05-14 10:44 ` [PATCH v3 5/5] tools/libxc: make use of domain context SHARED_INFO record Paul Durrant
4 siblings, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2020-05-14 10:44 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Paul Durrant, Ian Jackson, George Dunlap, Jan Beulich
From: Paul Durrant <pdurrant@amazon.com>
... and update xen-domctx to dump some information describing the record.
NOTE: The domain may or may not be using the embedded vcpu_info array so
ultimately separate context records will be added for vcpu_info when
this becomes necessary.
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>
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 | 73 +++++++++++++++++++++++++++++++++++++++
xen/common/domain.c | 60 ++++++++++++++++++++++++++++++++
xen/include/public/save.h | 11 +++++-
3 files changed, 143 insertions(+), 1 deletion(-)
diff --git a/tools/misc/xen-domctx.c b/tools/misc/xen-domctx.c
index 243325dfce..b2fed5eae7 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,76 @@ static void dump_header(void)
}
+static void print_binary(const char *prefix, void *val, size_t size,
+ const char *suffix)
+{
+ printf("%s", prefix);
+
+ while (size--)
+ {
+ uint8_t octet = *(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;
+ shared_info_any_t *info;
+ unsigned int i;
+
+ GET_PTR(s);
+
+ printf(" SHARED_INFO: has_32bit_shinfo: %s buffer_size: %u\n",
+ s->has_32bit_shinfo ? "true" : "false", s->buffer_size);
+
+ info = (shared_info_any_t *)s->buffer;
+
+#define GET_FIELD_PTR(_f) \
+ (s->has_32bit_shinfo ? (void *)&(info->x32._f) : (void *)&(info->x64._f))
+#define GET_FIELD_SIZE(_f) \
+ (s->has_32bit_shinfo ? sizeof(info->x32._f) : sizeof(info->x64._f))
+#define GET_FIELD(_f) \
+ (s->has_32bit_shinfo ? info->x32._f : info->x64._f)
+
+ /* Array lengths are the same for 32-bit and 64-bit shared info */
+
+ for ( i = 0; i < ARRAY_SIZE(info->x64.evtchn_pending); 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 < ARRAY_SIZE(info->x64.evtchn_mask); 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;
@@ -167,12 +238,14 @@ int main(int argc, char **argv)
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(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 7cc9526139..e4518cd28d 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>
@@ -1649,6 +1650,65 @@ 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 = { .buffer_size = PAGE_SIZE };
+ size_t hdr_size = offsetof(typeof(ctxt), buffer);
+ int rc;
+
+ rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, 0);
+ if ( rc )
+ return rc;
+
+#ifdef CONFIG_COMPAT
+ if ( !dry_run )
+ ctxt.has_32bit_shinfo = has_32bit_shinfo(d);
+#endif
+
+ 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 || i ) /* expect only a single instance */
+ return rc;
+
+ rc = domain_load_data(c, &ctxt, hdr_size);
+ if ( rc )
+ return rc;
+
+ if ( ctxt.pad[0] || ctxt.pad[1] || ctxt.pad[2] ||
+ ctxt.buffer_size != PAGE_SIZE )
+ return -EINVAL;
+
+#ifdef CONFIG_COMPAT
+ d->arch.has_32bit_shinfo = ctxt.has_32bit_shinfo;
+#endif
+
+ rc = domain_load_data(c, d->shared_info, ctxt.buffer_size);
+ if ( rc )
+ return rc;
+
+ return domain_load_end(c);
+}
+
+DOMAIN_REGISTER_SAVE_RESTORE(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 834c031c51..2b633cf03d 100644
--- a/xen/include/public/save.h
+++ b/xen/include/public/save.h
@@ -73,7 +73,16 @@ 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 {
+ uint8_t has_32bit_shinfo;
+ uint8_t pad[3];
+ 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] 20+ messages in thread
* [PATCH v3 5/5] tools/libxc: make use of domain context SHARED_INFO record...
2020-05-14 10:44 [PATCH v3 0/5] domain context infrastructure Paul Durrant
` (3 preceding siblings ...)
2020-05-14 10:44 ` [PATCH v3 4/5] common/domain: add a domain context record for shared_info Paul Durrant
@ 2020-05-14 10:44 ` Paul Durrant
4 siblings, 0 replies; 20+ messages in thread
From: Paul Durrant @ 2020-05-14 10:44 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu, Paul Durrant
... in the save/restore code.
This patch replaces direct mapping of the shared_info_frame (retrieved
using XEN_DOMCTL_getdomaininfo) with save/load of the domain context
SHARED_INFO record.
No modifications are made to the definition of the migration stream at
this point. Subsequent patches will define a record in the libxc domain
image format for passing domain context and convert the save/restore code
to use that.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
v3:
- Moved basic get/set domain context functions to common code
v2:
- Re-based (now making use of DOMAIN_SAVE_FLAG_IGNORE)
---
tools/libxc/xc_sr_common.c | 58 ++++++++++++++++++++++++++++++
tools/libxc/xc_sr_common.h | 11 +++++-
tools/libxc/xc_sr_common_x86_pv.c | 47 ++++++++++++++++++++++++
tools/libxc/xc_sr_common_x86_pv.h | 3 ++
tools/libxc/xc_sr_restore_x86_pv.c | 40 ++++++++-------------
tools/libxc/xc_sr_save_x86_pv.c | 26 ++------------
tools/libxc/xg_save_restore.h | 1 +
7 files changed, 137 insertions(+), 49 deletions(-)
diff --git a/tools/libxc/xc_sr_common.c b/tools/libxc/xc_sr_common.c
index dd9a11b4b5..5b2b6944f8 100644
--- a/tools/libxc/xc_sr_common.c
+++ b/tools/libxc/xc_sr_common.c
@@ -138,6 +138,64 @@ int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec)
return 0;
};
+int get_domain_context(struct xc_sr_context *ctx)
+{
+ xc_interface *xch = ctx->xch;
+ size_t len = 0;
+ int rc;
+
+ if ( ctx->domain_context.buffer )
+ {
+ ERROR("Domain context already present");
+ return -1;
+ }
+
+ rc = xc_domain_getcontext(xch, ctx->domid, NULL, &len);
+ if ( rc < 0 )
+ {
+ PERROR("Unable to get size of domain context");
+ return -1;
+ }
+
+ ctx->domain_context.buffer = malloc(len);
+ if ( !ctx->domain_context.buffer )
+ {
+ PERROR("Unable to allocate memory for domain context");
+ return -1;
+ }
+
+ rc = xc_domain_getcontext(xch, ctx->domid, ctx->domain_context.buffer,
+ &len);
+ if ( rc < 0 )
+ {
+ PERROR("Unable to get domain context");
+ return -1;
+ }
+
+ ctx->domain_context.len = len;
+
+ return 0;
+}
+
+int set_domain_context(struct xc_sr_context *ctx)
+{
+ xc_interface *xch = ctx->xch;
+
+ if ( !ctx->domain_context.buffer )
+ {
+ ERROR("Domain context not present");
+ return -1;
+ }
+
+ return xc_domain_setcontext(xch, ctx->domid, ctx->domain_context.buffer,
+ ctx->domain_context.len);
+}
+
+void common_cleanup(struct xc_sr_context *ctx)
+{
+ free(ctx->domain_context.buffer);
+}
+
static void __attribute__((unused)) build_assertions(void)
{
BUILD_BUG_ON(sizeof(struct xc_sr_ihdr) != 24);
diff --git a/tools/libxc/xc_sr_common.h b/tools/libxc/xc_sr_common.h
index 5dd51ccb15..0d61978b08 100644
--- a/tools/libxc/xc_sr_common.h
+++ b/tools/libxc/xc_sr_common.h
@@ -208,6 +208,11 @@ struct xc_sr_context
xc_dominfo_t dominfo;
+ struct {
+ void *buffer;
+ unsigned int len;
+ } domain_context;
+
union /* Common save or restore data. */
{
struct /* Save data. */
@@ -314,7 +319,7 @@ struct xc_sr_context
/* The guest pfns containing the p2m leaves */
xen_pfn_t *p2m_pfns;
- /* Read-only mapping of guests shared info page */
+ /* Pointer to shared_info (located in context buffer) */
shared_info_any_t *shinfo;
/* p2m generation count for verifying validity of local p2m. */
@@ -425,6 +430,10 @@ int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec);
int populate_pfns(struct xc_sr_context *ctx, unsigned int count,
const xen_pfn_t *original_pfns, const uint32_t *types);
+int get_domain_context(struct xc_sr_context *ctx);
+int set_domain_context(struct xc_sr_context *ctx);
+void common_cleanup(struct xc_sr_context *ctx);
+
#endif
/*
* Local variables:
diff --git a/tools/libxc/xc_sr_common_x86_pv.c b/tools/libxc/xc_sr_common_x86_pv.c
index d3d425cb82..5d34144ce8 100644
--- a/tools/libxc/xc_sr_common_x86_pv.c
+++ b/tools/libxc/xc_sr_common_x86_pv.c
@@ -182,6 +182,53 @@ int x86_pv_map_m2p(struct xc_sr_context *ctx)
return rc;
}
+int x86_pv_get_shinfo(struct xc_sr_context *ctx)
+{
+ unsigned int off = 0;
+ struct domain_save_descriptor *desc;
+ int rc;
+
+ rc = get_domain_context(ctx);
+ if ( rc )
+ return rc;
+
+ do {
+ if ( ctx->domain_context.len - off < sizeof(*desc) )
+ return -1;
+
+ desc = ctx->domain_context.buffer + off;
+ off += sizeof(*desc);
+
+ switch (desc->typecode)
+ {
+ case DOMAIN_SAVE_CODE(SHARED_INFO):
+ {
+ DOMAIN_SAVE_TYPE(SHARED_INFO) *s;
+
+ if ( ctx->domain_context.len - off < sizeof(*s) )
+ return -1;
+
+ s = ctx->domain_context.buffer + off;
+ ctx->x86.pv.shinfo = (shared_info_any_t *)s->buffer;
+ /* fall through */
+ }
+ default:
+ off += desc->length;
+ break;
+ }
+ } while ( desc->typecode != DOMAIN_SAVE_CODE(END) );
+
+ if ( !ctx->x86.pv.shinfo )
+ return -1;
+
+ return 0;
+}
+
+int x86_pv_set_shinfo(struct xc_sr_context *ctx)
+{
+ return ctx->x86.pv.shinfo ? set_domain_context(ctx) : -1;
+}
+
/*
* Local variables:
* mode: C
diff --git a/tools/libxc/xc_sr_common_x86_pv.h b/tools/libxc/xc_sr_common_x86_pv.h
index 2ed03309af..01442f48fb 100644
--- a/tools/libxc/xc_sr_common_x86_pv.h
+++ b/tools/libxc/xc_sr_common_x86_pv.h
@@ -97,6 +97,9 @@ int x86_pv_domain_info(struct xc_sr_context *ctx);
*/
int x86_pv_map_m2p(struct xc_sr_context *ctx);
+int x86_pv_get_shinfo(struct xc_sr_context *ctx);
+int x86_pv_set_shinfo(struct xc_sr_context *ctx);
+
#endif
/*
* Local variables:
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index 904ccc462a..4dbc7f0da5 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -864,8 +864,7 @@ static int handle_shared_info(struct xc_sr_context *ctx,
{
xc_interface *xch = ctx->xch;
unsigned int i;
- int rc = -1;
- shared_info_any_t *guest_shinfo = NULL;
+ int rc;
const shared_info_any_t *old_shinfo = rec->data;
if ( !ctx->x86.pv.restore.seen_pv_info )
@@ -878,39 +877,30 @@ static int handle_shared_info(struct xc_sr_context *ctx,
{
ERROR("X86_PV_SHARED_INFO record wrong size: length %u"
", expected 4096", rec->length);
- goto err;
+ return -1;
}
- guest_shinfo = xc_map_foreign_range(
- xch, ctx->domid, PAGE_SIZE, PROT_READ | PROT_WRITE,
- ctx->dominfo.shared_info_frame);
- if ( !guest_shinfo )
- {
- PERROR("Failed to map Shared Info at mfn %#lx",
- ctx->dominfo.shared_info_frame);
- goto err;
- }
+ rc = x86_pv_get_shinfo(ctx);
+ if ( rc )
+ return rc;
- MEMCPY_FIELD(guest_shinfo, old_shinfo, vcpu_info, ctx->x86.pv.width);
- MEMCPY_FIELD(guest_shinfo, old_shinfo, arch, ctx->x86.pv.width);
+ MEMCPY_FIELD(ctx->x86.pv.shinfo, old_shinfo, vcpu_info,
+ ctx->x86.pv.width);
+ MEMCPY_FIELD(ctx->x86.pv.shinfo, old_shinfo, arch, ctx->x86.pv.width);
- SET_FIELD(guest_shinfo, arch.pfn_to_mfn_frame_list_list,
+ SET_FIELD(ctx->x86.pv.shinfo, arch.pfn_to_mfn_frame_list_list,
0, ctx->x86.pv.width);
- MEMSET_ARRAY_FIELD(guest_shinfo, evtchn_pending, 0, ctx->x86.pv.width);
+ MEMSET_ARRAY_FIELD(ctx->x86.pv.shinfo, evtchn_pending, 0,
+ ctx->x86.pv.width);
for ( i = 0; i < XEN_LEGACY_MAX_VCPUS; i++ )
- SET_FIELD(guest_shinfo, vcpu_info[i].evtchn_pending_sel,
+ SET_FIELD(ctx->x86.pv.shinfo, vcpu_info[i].evtchn_pending_sel,
0, ctx->x86.pv.width);
- MEMSET_ARRAY_FIELD(guest_shinfo, evtchn_mask, 0xff, ctx->x86.pv.width);
-
- rc = 0;
+ MEMSET_ARRAY_FIELD(ctx->x86.pv.shinfo, evtchn_mask, 0xff,
+ ctx->x86.pv.width);
- err:
- if ( guest_shinfo )
- munmap(guest_shinfo, PAGE_SIZE);
-
- return rc;
+ return x86_pv_set_shinfo(ctx);
}
/* restore_ops function. */
diff --git a/tools/libxc/xc_sr_save_x86_pv.c b/tools/libxc/xc_sr_save_x86_pv.c
index f3ccf5bb4b..0f73c30dbf 100644
--- a/tools/libxc/xc_sr_save_x86_pv.c
+++ b/tools/libxc/xc_sr_save_x86_pv.c
@@ -9,25 +9,6 @@ static inline bool is_canonical_address(xen_vaddr_t vaddr)
return ((int64_t)vaddr >> 47) == ((int64_t)vaddr >> 63);
}
-/*
- * Maps the guests shared info page.
- */
-static int map_shinfo(struct xc_sr_context *ctx)
-{
- xc_interface *xch = ctx->xch;
-
- ctx->x86.pv.shinfo = xc_map_foreign_range(
- xch, ctx->domid, PAGE_SIZE, PROT_READ, ctx->dominfo.shared_info_frame);
- if ( !ctx->x86.pv.shinfo )
- {
- PERROR("Failed to map shared info frame at mfn %#lx",
- ctx->dominfo.shared_info_frame);
- return -1;
- }
-
- return 0;
-}
-
/*
* Copy a list of mfns from a guest, accounting for differences between guest
* and toolstack width. Can fail if truncation would occur.
@@ -1041,7 +1022,7 @@ static int x86_pv_setup(struct xc_sr_context *ctx)
if ( rc )
return rc;
- rc = map_shinfo(ctx);
+ rc = x86_pv_get_shinfo(ctx);
if ( rc )
return rc;
@@ -1112,12 +1093,11 @@ static int x86_pv_cleanup(struct xc_sr_context *ctx)
if ( ctx->x86.pv.p2m )
munmap(ctx->x86.pv.p2m, ctx->x86.pv.p2m_frames * PAGE_SIZE);
- if ( ctx->x86.pv.shinfo )
- munmap(ctx->x86.pv.shinfo, PAGE_SIZE);
-
if ( ctx->x86.pv.m2p )
munmap(ctx->x86.pv.m2p, ctx->x86.pv.nr_m2p_frames * PAGE_SIZE);
+ common_cleanup(ctx);
+
return 0;
}
diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h
index 303081df0d..296b523963 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -19,6 +19,7 @@
#include <xen/foreign/x86_32.h>
#include <xen/foreign/x86_64.h>
+#include <xen/save.h>
/*
** We process save/restore/migrate in batches of pages; the below
--
2.20.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
2020-05-14 10:44 ` [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context Paul Durrant
@ 2020-05-19 13:03 ` Jan Beulich
2020-05-19 14:04 ` Paul Durrant
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2020-05-19 13:03 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Paul Durrant, Ian Jackson, George Dunlap, xen-devel,
Volodymyr Babchuk, Roger Pau Monné
On 14.05.2020 12:44, Paul Durrant wrote:
> --- /dev/null
> +++ b/xen/common/save.c
> @@ -0,0 +1,313 @@
> +/*
> + * save.c: Save and restore PV guest state common to all domain types.
> + *
> + * Copyright Amazon.com Inc. or its affiliates.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/compile.h>
> +#include <xen/save.h>
> +
> +struct domain_context {
> + struct domain *domain;
> + const char *name; /* for logging purposes */
With this comment, why ...
> + struct domain_save_descriptor desc;
> + size_t len; /* for internal accounting */
> + union {
> + struct domain_save_ops *save;
> + struct domain_load_ops *load;
> + } ops;
> + void *priv;
> + bool log;
... this separate field? Couldn't "no logging" simply be expressed by
name being NULL?
> +int domain_save_end(struct domain_context *c)
> +{
> + struct domain *d = c->domain;
> + uint8_t pad[DOMAIN_SAVE_ALIGN] = {};
Preferably moved into the more narrow scope, and probably wants making
"static const".
> + size_t len = ROUNDUP(c->len, DOMAIN_SAVE_ALIGN) - c->len; /* padding */
> + int rc;
> +
> + if ( len )
> + {
> + rc = domain_save_data(c, pad, len);
> +
> + if ( rc )
> + return rc;
> + }
> + ASSERT(IS_ALIGNED(c->len, DOMAIN_SAVE_ALIGN));
While you mention the auto-padding as a change in v3, it's not really
clear to me why it was introduced. Could you add half a sentence to
the description clarifying the motivation?
> +int domain_save(struct domain *d, struct domain_save_ops *ops, void *priv,
> + bool dry_run)
> +{
> + struct domain_context c = {
> + .domain = d,
> + .ops.save = ops,
> + .priv = priv,
> + .log = !dry_run,
> + };
> + static struct domain_save_header h = {
const?
> + .magic = DOMAIN_SAVE_MAGIC,
> + .xen_major = XEN_VERSION,
> + .xen_minor = XEN_SUBVERSION,
> + .version = DOMAIN_SAVE_VERSION,
> + };
> + struct domain_save_end e = {};
const? (static would likely be quite pointless here)
> +int domain_load(struct domain *d, struct domain_load_ops *ops, void *priv)
> +{
> + struct domain_context c = {
> + .domain = d,
> + .ops.load = ops,
> + .priv = priv,
> + .log = true,
> + };
> + unsigned int instance;
> + struct domain_save_header h;
> + int rc;
> +
> + ASSERT(d != current->domain);
> +
> + rc = c.ops.load->read(c.priv, &c.desc, sizeof(c.desc));
> + if ( rc )
> + return rc;
> +
> + rc = DOMAIN_LOAD_ENTRY(HEADER, &c, &instance, &h, sizeof(h));
> + if ( rc )
> + return rc;
> +
> + if ( instance || h.magic != DOMAIN_SAVE_MAGIC ||
> + h.version != DOMAIN_SAVE_VERSION )
> + return -EINVAL;
> +
> + domain_pause(d);
> +
> + for (;;)
> + {
> + unsigned int i;
> + domain_load_handler load;
> +
> + rc = c.ops.load->read(c.priv, &c.desc, sizeof(c.desc));
> + if ( rc )
> + return rc;
> +
> + rc = -EINVAL;
> +
> + if ( c.desc.typecode == DOMAIN_SAVE_CODE(END) )
> + {
> + struct domain_save_end e;
> +
> + rc = DOMAIN_LOAD_ENTRY(END, &c, &instance, NULL, sizeof(e));
Without using &e here I don't see how you can get away without an
"unused variable" warning.
> --- /dev/null
> +++ b/xen/include/public/save.h
> @@ -0,0 +1,80 @@
> +/*
> + * save.h
> + *
> + * Structure definitions for common PV/HVM domain state that is held by
> + * Xen and must be saved along with the domain's memory.
> + *
> + * Copyright Amazon.com Inc. or its affiliates.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + */
> +
> +#ifndef XEN_PUBLIC_SAVE_H
> +#define XEN_PUBLIC_SAVE_H
> +
> +#include "xen.h"
> +
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
Move #include down below here?
> +/* Entry data is preceded by a descriptor */
> +struct domain_save_descriptor {
> + uint16_t typecode;
> +
> + /*
> + * Instance number of the entry (since there may by multiple of some
> + * types of entry).
With a German bias I'm inclined to ask: "entries"?
> + */
> + uint16_t instance;
> +
> + /* Entry length not including this descriptor */
> + uint32_t length;
> +};
> +
> +/*
> + * Each entry has a type associated with it. DECLARE_DOMAIN_SAVE_TYPE
> + * binds these things together.
> + */
> +#define DECLARE_DOMAIN_SAVE_TYPE(_x, _code, _type) \
> + struct DOMAIN_SAVE_TYPE_##_x { char c[_code]; _type t; };
Perhaps point out in the comment that this type is not meant to
have instantiations?
> +#define DOMAIN_SAVE_CODE(_x) \
> + (sizeof(((struct DOMAIN_SAVE_TYPE_##_x *)(0))->c))
> +#define DOMAIN_SAVE_TYPE(_x) \
> + typeof(((struct DOMAIN_SAVE_TYPE_##_x *)(0))->t)
Feels like I already mentioned the oddity of having parentheses
around a single token that's not a macro parameter name.
> --- /dev/null
> +++ b/xen/include/xen/save.h
> @@ -0,0 +1,165 @@
> +/*
> + * save.h: support routines for save/restore
> + *
> + * Copyright Amazon.com Inc. or its affiliates.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#ifndef XEN_SAVE_H
> +#define XEN_SAVE_H
> +
> +#include <xen/init.h>
> +#include <xen/sched.h>
> +#include <xen/types.h>
> +
> +#include <public/save.h>
> +
> +struct domain_context;
> +
> +int domain_save_begin(struct domain_context *c, unsigned int typecode,
> + const char *name, unsigned int instance);
> +
> +#define DOMAIN_SAVE_BEGIN(_x, _c, _instance) \
> + domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_instance))
As per prior conversation I would have expected no leading underscores
here anymore, and no parenthesization of what is still _c. More of
these further down.
> +static inline int domain_load_entry(struct domain_context *c,
> + unsigned int typecode, const char *name,
> + unsigned int *instance, void *dst,
> + size_t len)
> +{
> + int rc;
> +
> + rc = domain_load_begin(c, typecode, name, instance);
For some reason I've spotted this only here: Why is instance a pointer
parameter of the function, when typecode is a value? Both live next to
one another in struct domain_save_descriptor, and hence instance could
be retrieved at the same time as typecode.
> +/*
> + * Register save and restore handlers. Save handlers will be invoked
> + * in order of DOMAIN_SAVE_CODE().
> + */
> +#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load) \
> + static int __init __domain_register_##_x##_save_restore(void) \
> + { \
> + domain_register_save_type( \
> + DOMAIN_SAVE_CODE(_x), \
> + #_x, \
> + &(_save), \
> + &(_load)); \
> + \
> + return 0; \
> + } \
> + __initcall(__domain_register_##_x##_save_restore);
I'm puzzled by part of the comment: Invoking by save code looks
reasonable for the saving side (albeit END doesn't match this rule
afaics), but is this going to be good enough for the consuming side?
There may be dependencies between types, and with fixed ordering
there may be no way to insert a depended upon type ahead of an
already defined one (at least as long as the codes are meant to be
stable).
> +/*
> + * Entry points:
> + *
> + * ops: These are callback functions provided by the caller that will
> + * be used to write to (in the save case) or read from (in the
> + * load case) the context buffer. See above for more detail.
> + * priv: This is a pointer that will be passed to the copy function to
> + * allow it to identify the context buffer and the current state
> + * of the save or load operation.
> + * dry_run: If this is set then the caller of domain_save() is only trying
> + * to acquire the total size of the data, not the data itself.
> + * In this case the caller may supply different ops to avoid doing
> + * unnecessary work.
> + */
> +int domain_save(struct domain *d, struct domain_save_ops *ops, void *priv,
> + bool dry_run);
> +int domain_load(struct domain *d, struct domain_load_ops *ops, void *priv);
I guess ops want to be pointer to const in both cases?
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/5] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
2020-05-14 10:44 ` [PATCH v3 2/5] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext Paul Durrant
@ 2020-05-19 13:49 ` Jan Beulich
2020-05-19 15:12 ` Paul Durrant
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2020-05-19 13:49 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Paul Durrant, Ian Jackson, George Dunlap, xen-devel,
Daniel De Graaf
On 14.05.2020 12:44, Paul Durrant wrote:
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -1129,6 +1129,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;
This and its counterpart don't seem to get checked to be zero.
While an option for a domctl, any desire to use the field in
the future would then require an interface version bump.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
2020-05-19 13:03 ` Jan Beulich
@ 2020-05-19 14:04 ` Paul Durrant
2020-05-19 14:23 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2020-05-19 14:04 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap', xen-devel, 'Volodymyr Babchuk',
'Roger Pau Monné'
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 May 2020 14:04
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Julien Grall <julien@xen.org>; 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 v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
>
> On 14.05.2020 12:44, Paul Durrant wrote:
> > --- /dev/null
> > +++ b/xen/common/save.c
> > @@ -0,0 +1,313 @@
> > +/*
> > + * save.c: Save and restore PV guest state common to all domain types.
> > + *
> > + * Copyright Amazon.com Inc. or its affiliates.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program; If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <xen/compile.h>
> > +#include <xen/save.h>
> > +
> > +struct domain_context {
> > + struct domain *domain;
> > + const char *name; /* for logging purposes */
>
> With this comment, why ...
>
> > + struct domain_save_descriptor desc;
> > + size_t len; /* for internal accounting */
> > + union {
> > + struct domain_save_ops *save;
> > + struct domain_load_ops *load;
> > + } ops;
> > + void *priv;
> > + bool log;
>
> ... this separate field? Couldn't "no logging" simply be expressed by
> name being NULL?
>
Ok. The log bool predated the name pointer so, yes these could be combined.
> > +int domain_save_end(struct domain_context *c)
> > +{
> > + struct domain *d = c->domain;
> > + uint8_t pad[DOMAIN_SAVE_ALIGN] = {};
>
> Preferably moved into the more narrow scope, and probably wants making
> "static const".
>
Ok.
> > + size_t len = ROUNDUP(c->len, DOMAIN_SAVE_ALIGN) - c->len; /* padding */
> > + int rc;
> > +
> > + if ( len )
> > + {
> > + rc = domain_save_data(c, pad, len);
> > +
> > + if ( rc )
> > + return rc;
> > + }
> > + ASSERT(IS_ALIGNED(c->len, DOMAIN_SAVE_ALIGN));
>
> While you mention the auto-padding as a change in v3, it's not really
> clear to me why it was introduced. Could you add half a sentence to
> the description clarifying the motivation?
>
Julien favoured it and it does seem like a good idea as it avoids the need to put explicit trailing 'pad' fields in entries. I'll add to the commit comment to explain.
> > +int domain_save(struct domain *d, struct domain_save_ops *ops, void *priv,
> > + bool dry_run)
> > +{
> > + struct domain_context c = {
> > + .domain = d,
> > + .ops.save = ops,
> > + .priv = priv,
> > + .log = !dry_run,
> > + };
> > + static struct domain_save_header h = {
>
> const?
>
Yes, it can be.
> > + .magic = DOMAIN_SAVE_MAGIC,
> > + .xen_major = XEN_VERSION,
> > + .xen_minor = XEN_SUBVERSION,
> > + .version = DOMAIN_SAVE_VERSION,
> > + };
> > + struct domain_save_end e = {};
>
> const? (static would likely be quite pointless here)
>
Ok.
> > +int domain_load(struct domain *d, struct domain_load_ops *ops, void *priv)
> > +{
> > + struct domain_context c = {
> > + .domain = d,
> > + .ops.load = ops,
> > + .priv = priv,
> > + .log = true,
> > + };
> > + unsigned int instance;
> > + struct domain_save_header h;
> > + int rc;
> > +
> > + ASSERT(d != current->domain);
> > +
> > + rc = c.ops.load->read(c.priv, &c.desc, sizeof(c.desc));
> > + if ( rc )
> > + return rc;
> > +
> > + rc = DOMAIN_LOAD_ENTRY(HEADER, &c, &instance, &h, sizeof(h));
> > + if ( rc )
> > + return rc;
> > +
> > + if ( instance || h.magic != DOMAIN_SAVE_MAGIC ||
> > + h.version != DOMAIN_SAVE_VERSION )
> > + return -EINVAL;
> > +
> > + domain_pause(d);
> > +
> > + for (;;)
> > + {
> > + unsigned int i;
> > + domain_load_handler load;
> > +
> > + rc = c.ops.load->read(c.priv, &c.desc, sizeof(c.desc));
> > + if ( rc )
> > + return rc;
> > +
> > + rc = -EINVAL;
> > +
> > + if ( c.desc.typecode == DOMAIN_SAVE_CODE(END) )
> > + {
> > + struct domain_save_end e;
> > +
> > + rc = DOMAIN_LOAD_ENTRY(END, &c, &instance, NULL, sizeof(e));
>
> Without using &e here I don't see how you can get away without an
> "unused variable" warning.
Hmm. I definitely don't get a warning, but yes this ought to be changed.
>
> > --- /dev/null
> > +++ b/xen/include/public/save.h
> > @@ -0,0 +1,80 @@
> > +/*
> > + * save.h
> > + *
> > + * Structure definitions for common PV/HVM domain state that is held by
> > + * Xen and must be saved along with the domain's memory.
> > + *
> > + * Copyright Amazon.com Inc. or its affiliates.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a copy
> > + * of this software and associated documentation files (the "Software"), to
> > + * deal in the Software without restriction, including without limitation the
> > + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> > + * sell copies of the Software, and to permit persons to whom the Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef XEN_PUBLIC_SAVE_H
> > +#define XEN_PUBLIC_SAVE_H
> > +
> > +#include "xen.h"
> > +
> > +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>
> Move #include down below here?
>
Sure.
> > +/* Entry data is preceded by a descriptor */
> > +struct domain_save_descriptor {
> > + uint16_t typecode;
> > +
> > + /*
> > + * Instance number of the entry (since there may by multiple of some
> > + * types of entry).
>
> With a German bias I'm inclined to ask: "entries"?
>
Not sure. Still understandable so I'm happy to change. Also s/by/be.
> > + */
> > + uint16_t instance;
> > +
> > + /* Entry length not including this descriptor */
> > + uint32_t length;
> > +};
> > +
> > +/*
> > + * Each entry has a type associated with it. DECLARE_DOMAIN_SAVE_TYPE
> > + * binds these things together.
> > + */
> > +#define DECLARE_DOMAIN_SAVE_TYPE(_x, _code, _type) \
> > + struct DOMAIN_SAVE_TYPE_##_x { char c[_code]; _type t; };
>
> Perhaps point out in the comment that this type is not meant to
> have instantiations?
Ok.
>
> > +#define DOMAIN_SAVE_CODE(_x) \
> > + (sizeof(((struct DOMAIN_SAVE_TYPE_##_x *)(0))->c))
> > +#define DOMAIN_SAVE_TYPE(_x) \
> > + typeof(((struct DOMAIN_SAVE_TYPE_##_x *)(0))->t)
>
> Feels like I already mentioned the oddity of having parentheses
> around a single token that's not a macro parameter name.
>
Ok. Missed that.
> > --- /dev/null
> > +++ b/xen/include/xen/save.h
> > @@ -0,0 +1,165 @@
> > +/*
> > + * save.h: support routines for save/restore
> > + *
> > + * Copyright Amazon.com Inc. or its affiliates.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> > + * more details.
> > + *
> > + * You should have received a copy of the GNU General Public License along with
> > + * this program; If not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef XEN_SAVE_H
> > +#define XEN_SAVE_H
> > +
> > +#include <xen/init.h>
> > +#include <xen/sched.h>
> > +#include <xen/types.h>
> > +
> > +#include <public/save.h>
> > +
> > +struct domain_context;
> > +
> > +int domain_save_begin(struct domain_context *c, unsigned int typecode,
> > + const char *name, unsigned int instance);
> > +
> > +#define DOMAIN_SAVE_BEGIN(_x, _c, _instance) \
> > + domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_instance))
>
> As per prior conversation I would have expected no leading underscores
> here anymore, and no parenthesization of what is still _c. More of
> these further down.
What's wrong with leading underscores in macro arguments? They can't pollute any namespace. Also, I prefer to keep the parentheses for arguments.
>
> > +static inline int domain_load_entry(struct domain_context *c,
> > + unsigned int typecode, const char *name,
> > + unsigned int *instance, void *dst,
> > + size_t len)
> > +{
> > + int rc;
> > +
> > + rc = domain_load_begin(c, typecode, name, instance);
>
> For some reason I've spotted this only here: Why is instance a pointer
> parameter of the function, when typecode is a value? Both live next to
> one another in struct domain_save_descriptor, and hence instance could
> be retrieved at the same time as typecode.
>
Because the typecode is known a priori. It has to be known for the correct handler to be invoked. It is only provided here for verification purposes. I could have provided the instance as an argument to the load handler but I prefer making the interactions for save and load more symmetric.
> > +/*
> > + * Register save and restore handlers. Save handlers will be invoked
> > + * in order of DOMAIN_SAVE_CODE().
> > + */
> > +#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load) \
> > + static int __init __domain_register_##_x##_save_restore(void) \
> > + { \
> > + domain_register_save_type( \
> > + DOMAIN_SAVE_CODE(_x), \
> > + #_x, \
> > + &(_save), \
> > + &(_load)); \
> > + \
> > + return 0; \
> > + } \
> > + __initcall(__domain_register_##_x##_save_restore);
>
> I'm puzzled by part of the comment: Invoking by save code looks
> reasonable for the saving side (albeit END doesn't match this rule
> afaics), but is this going to be good enough for the consuming side?
No, this only relates to the save side which is why the comment says 'Save handlers'. I do note that it would be more consistent to use 'load' rather than 'restore' here though.
> There may be dependencies between types, and with fixed ordering
> there may be no way to insert a depended upon type ahead of an
> already defined one (at least as long as the codes are meant to be
> stable).
>
The ordering of load handlers is determined by the stream. I'll add a sentence saying that.
> > +/*
> > + * Entry points:
> > + *
> > + * ops: These are callback functions provided by the caller that will
> > + * be used to write to (in the save case) or read from (in the
> > + * load case) the context buffer. See above for more detail.
> > + * priv: This is a pointer that will be passed to the copy function to
> > + * allow it to identify the context buffer and the current state
> > + * of the save or load operation.
> > + * dry_run: If this is set then the caller of domain_save() is only trying
> > + * to acquire the total size of the data, not the data itself.
> > + * In this case the caller may supply different ops to avoid doing
> > + * unnecessary work.
> > + */
> > +int domain_save(struct domain *d, struct domain_save_ops *ops, void *priv,
> > + bool dry_run);
> > +int domain_load(struct domain *d, struct domain_load_ops *ops, void *priv);
>
> I guess ops want to be pointer to const in both cases?
>
Yes, it can be.
Paul
> Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 4/5] common/domain: add a domain context record for shared_info...
2020-05-14 10:44 ` [PATCH v3 4/5] common/domain: add a domain context record for shared_info Paul Durrant
@ 2020-05-19 14:07 ` Jan Beulich
2020-05-19 15:21 ` Paul Durrant
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2020-05-19 14:07 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Andrew Cooper,
Paul Durrant, Ian Jackson, George Dunlap, xen-devel
On 14.05.2020 12:44, Paul Durrant wrote:
> @@ -61,6 +62,76 @@ static void dump_header(void)
>
> }
>
> +static void print_binary(const char *prefix, void *val, size_t size,
const also for val?
> + const char *suffix)
> +{
> + printf("%s", prefix);
> +
> + while (size--)
Judging from style elsewhere you look to be missing two blanks
here.
> + {
> + uint8_t octet = *(uint8_t *)val++;
Following the above then also better don't cast const away here.
> + 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;
> + shared_info_any_t *info;
> + unsigned int i;
> +
> + GET_PTR(s);
> +
> + printf(" SHARED_INFO: has_32bit_shinfo: %s buffer_size: %u\n",
> + s->has_32bit_shinfo ? "true" : "false", s->buffer_size);
> +
> + info = (shared_info_any_t *)s->buffer;
> +
> +#define GET_FIELD_PTR(_f) \
> + (s->has_32bit_shinfo ? (void *)&(info->x32._f) : (void *)&(info->x64._f))
Better cast to const void * ?
> +#define GET_FIELD_SIZE(_f) \
> + (s->has_32bit_shinfo ? sizeof(info->x32._f) : sizeof(info->x64._f))
> +#define GET_FIELD(_f) \
> + (s->has_32bit_shinfo ? info->x32._f : info->x64._f)
> +
> + /* Array lengths are the same for 32-bit and 64-bit shared info */
Not really, no:
xen_ulong_t evtchn_pending[sizeof(xen_ulong_t) * 8];
xen_ulong_t evtchn_mask[sizeof(xen_ulong_t) * 8];
> @@ -167,12 +238,14 @@ int main(int argc, char **argv)
> 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);
Stray insertion of a blank line?
> @@ -1649,6 +1650,65 @@ 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 = { .buffer_size = PAGE_SIZE };
Why not sizeof(shared_info), utilizing the zero padding on the
receiving side?
> + size_t hdr_size = offsetof(typeof(ctxt), buffer);
> + int rc;
> +
> + rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, 0);
> + if ( rc )
> + return rc;
> +
> +#ifdef CONFIG_COMPAT
> + if ( !dry_run )
> + ctxt.has_32bit_shinfo = has_32bit_shinfo(d);
> +#endif
Nothing will go wrong without the if(), I suppose? Better drop it
then? It could then also easily be part of the initializer of ctxt.
> + 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 || i ) /* expect only a single instance */
> + return rc;
> +
> + rc = domain_load_data(c, &ctxt, hdr_size);
> + if ( rc )
> + return rc;
> +
> + if ( ctxt.pad[0] || ctxt.pad[1] || ctxt.pad[2] ||
> + ctxt.buffer_size != PAGE_SIZE )
> + return -EINVAL;
> +
> +#ifdef CONFIG_COMPAT
> + d->arch.has_32bit_shinfo = ctxt.has_32bit_shinfo;
> +#endif
There's nothing wrong with using has_32bit_shinfo(d) here as well.
> --- a/xen/include/public/save.h
> +++ b/xen/include/public/save.h
> @@ -73,7 +73,16 @@ 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 {
> + uint8_t has_32bit_shinfo;
> + uint8_t pad[3];
32-(or 16-)bit flags, with just a single bit used for the purpose?
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
2020-05-19 14:04 ` Paul Durrant
@ 2020-05-19 14:23 ` Jan Beulich
2020-05-19 15:10 ` Paul Durrant
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2020-05-19 14:23 UTC (permalink / raw)
To: paul
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap', xen-devel, 'Volodymyr Babchuk',
'Roger Pau Monné'
On 19.05.2020 16:04, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 19 May 2020 14:04
>>
>> On 14.05.2020 12:44, Paul Durrant wrote:
>>> --- /dev/null
>>> +++ b/xen/include/xen/save.h
>>> @@ -0,0 +1,165 @@
>>> +/*
>>> + * save.h: support routines for save/restore
>>> + *
>>> + * Copyright Amazon.com Inc. or its affiliates.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms and conditions of the GNU General Public License,
>>> + * version 2, as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>>> + * more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * this program; If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#ifndef XEN_SAVE_H
>>> +#define XEN_SAVE_H
>>> +
>>> +#include <xen/init.h>
>>> +#include <xen/sched.h>
>>> +#include <xen/types.h>
>>> +
>>> +#include <public/save.h>
>>> +
>>> +struct domain_context;
>>> +
>>> +int domain_save_begin(struct domain_context *c, unsigned int typecode,
>>> + const char *name, unsigned int instance);
>>> +
>>> +#define DOMAIN_SAVE_BEGIN(_x, _c, _instance) \
>>> + domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_instance))
>>
>> As per prior conversation I would have expected no leading underscores
>> here anymore, and no parenthesization of what is still _c. More of
>> these further down.
>
> What's wrong with leading underscores in macro arguments? They can't
> pollute any namespace.
They can still hide file scope variables legitimately named
this way (which may get accessed by a macro without being a
macro argument). The wording of the standard is quite clear:
"All identifiers that begin with an underscore are always
reserved for use as identifiers with file scope in both the
ordinary and tag name spaces."
> Also, I prefer to keep the parentheses for arguments.
More code churn then once we hopefully standardize of the less
obfuscating variant without.
>>> +static inline int domain_load_entry(struct domain_context *c,
>>> + unsigned int typecode, const char *name,
>>> + unsigned int *instance, void *dst,
>>> + size_t len)
>>> +{
>>> + int rc;
>>> +
>>> + rc = domain_load_begin(c, typecode, name, instance);
>>
>> For some reason I've spotted this only here: Why is instance a pointer
>> parameter of the function, when typecode is a value? Both live next to
>> one another in struct domain_save_descriptor, and hence instance could
>> be retrieved at the same time as typecode.
>>
>
> Because the typecode is known a priori. It has to be known for the
> correct handler to be invoked. It is only provided here for
> verification purposes. I could have provided the instance as an
> argument to the load handler but I prefer making the interactions
> for save and load more symmetric.
Hmm, I don't see any symmetry violation in the alternative model,
but anyway.
>>> +/*
>>> + * Register save and restore handlers. Save handlers will be invoked
>>> + * in order of DOMAIN_SAVE_CODE().
>>> + */
>>> +#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load) \
>>> + static int __init __domain_register_##_x##_save_restore(void) \
>>> + { \
>>> + domain_register_save_type( \
>>> + DOMAIN_SAVE_CODE(_x), \
>>> + #_x, \
>>> + &(_save), \
>>> + &(_load)); \
>>> + \
>>> + return 0; \
>>> + } \
>>> + __initcall(__domain_register_##_x##_save_restore);
>>
>> I'm puzzled by part of the comment: Invoking by save code looks
>> reasonable for the saving side (albeit END doesn't match this rule
>> afaics), but is this going to be good enough for the consuming side?
>
> No, this only relates to the save side which is why the comment
> says 'Save handlers'. I do note that it would be more consistent
> to use 'load' rather than 'restore' here though.
>
>> There may be dependencies between types, and with fixed ordering
>> there may be no way to insert a depended upon type ahead of an
>> already defined one (at least as long as the codes are meant to be
>> stable).
>>
>
> The ordering of load handlers is determined by the stream. I'll
> add a sentence saying that.
I.e. the consumer of the "get" interface (and producer of the stream)
is supposed to take apart the output it gets, bring records into
suitable order (which implies it knows of all the records, and which
hence means this code may need updating in cases where I'd expect
only the hypervisor needs), and only then issue to the stream?
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
2020-05-19 14:23 ` Jan Beulich
@ 2020-05-19 15:10 ` Paul Durrant
2020-05-19 15:18 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2020-05-19 15:10 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap', xen-devel, 'Volodymyr Babchuk',
'Roger Pau Monné'
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 May 2020 15:24
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
> <ian.jackson@eu.citrix.com>; 'Julien Grall' <julien@xen.org>; '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 v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
>
> On 19.05.2020 16:04, Paul Durrant wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 19 May 2020 14:04
> >>
> >> On 14.05.2020 12:44, Paul Durrant wrote:
> >>> --- /dev/null
> >>> +++ b/xen/include/xen/save.h
> >>> @@ -0,0 +1,165 @@
> >>> +/*
> >>> + * save.h: support routines for save/restore
> >>> + *
> >>> + * Copyright Amazon.com Inc. or its affiliates.
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or modify it
> >>> + * under the terms and conditions of the GNU General Public License,
> >>> + * version 2, as published by the Free Software Foundation.
> >>> + *
> >>> + * This program is distributed in the hope it will be useful, but WITHOUT
> >>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> >>> + * more details.
> >>> + *
> >>> + * You should have received a copy of the GNU General Public License along with
> >>> + * this program; If not, see <http://www.gnu.org/licenses/>.
> >>> + */
> >>> +
> >>> +#ifndef XEN_SAVE_H
> >>> +#define XEN_SAVE_H
> >>> +
> >>> +#include <xen/init.h>
> >>> +#include <xen/sched.h>
> >>> +#include <xen/types.h>
> >>> +
> >>> +#include <public/save.h>
> >>> +
> >>> +struct domain_context;
> >>> +
> >>> +int domain_save_begin(struct domain_context *c, unsigned int typecode,
> >>> + const char *name, unsigned int instance);
> >>> +
> >>> +#define DOMAIN_SAVE_BEGIN(_x, _c, _instance) \
> >>> + domain_save_begin((_c), DOMAIN_SAVE_CODE(_x), #_x, (_instance))
> >>
> >> As per prior conversation I would have expected no leading underscores
> >> here anymore, and no parenthesization of what is still _c. More of
> >> these further down.
> >
> > What's wrong with leading underscores in macro arguments? They can't
> > pollute any namespace.
>
> They can still hide file scope variables legitimately named
> this way (which may get accessed by a macro without being a
> macro argument). The wording of the standard is quite clear:
> "All identifiers that begin with an underscore are always
> reserved for use as identifiers with file scope in both the
> ordinary and tag name spaces."
>
Ok.
> > Also, I prefer to keep the parentheses for arguments.
>
> More code churn then once we hopefully standardize of the less
> obfuscating variant without.
>
If we standardize that way, so be it.
> >>> +static inline int domain_load_entry(struct domain_context *c,
> >>> + unsigned int typecode, const char *name,
> >>> + unsigned int *instance, void *dst,
> >>> + size_t len)
> >>> +{
> >>> + int rc;
> >>> +
> >>> + rc = domain_load_begin(c, typecode, name, instance);
> >>
> >> For some reason I've spotted this only here: Why is instance a pointer
> >> parameter of the function, when typecode is a value? Both live next to
> >> one another in struct domain_save_descriptor, and hence instance could
> >> be retrieved at the same time as typecode.
> >>
> >
> > Because the typecode is known a priori. It has to be known for the
> > correct handler to be invoked. It is only provided here for
> > verification purposes. I could have provided the instance as an
> > argument to the load handler but I prefer making the interactions
> > for save and load more symmetric.
>
> Hmm, I don't see any symmetry violation in the alternative model,
> but anyway.
>
> >>> +/*
> >>> + * Register save and restore handlers. Save handlers will be invoked
> >>> + * in order of DOMAIN_SAVE_CODE().
> >>> + */
> >>> +#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load) \
> >>> + static int __init __domain_register_##_x##_save_restore(void) \
> >>> + { \
> >>> + domain_register_save_type( \
> >>> + DOMAIN_SAVE_CODE(_x), \
> >>> + #_x, \
> >>> + &(_save), \
> >>> + &(_load)); \
> >>> + \
> >>> + return 0; \
> >>> + } \
> >>> + __initcall(__domain_register_##_x##_save_restore);
> >>
> >> I'm puzzled by part of the comment: Invoking by save code looks
> >> reasonable for the saving side (albeit END doesn't match this rule
> >> afaics), but is this going to be good enough for the consuming side?
> >
> > No, this only relates to the save side which is why the comment
> > says 'Save handlers'. I do note that it would be more consistent
> > to use 'load' rather than 'restore' here though.
> >
> >> There may be dependencies between types, and with fixed ordering
> >> there may be no way to insert a depended upon type ahead of an
> >> already defined one (at least as long as the codes are meant to be
> >> stable).
> >>
> >
> > The ordering of load handlers is determined by the stream. I'll
> > add a sentence saying that.
>
> I.e. the consumer of the "get" interface (and producer of the stream)
> is supposed to take apart the output it gets, bring records into
> suitable order (which implies it knows of all the records, and which
> hence means this code may need updating in cases where I'd expect
> only the hypervisor needs), and only then issue to the stream?
The intention is that the stream is always in a suitable order so the load side does not have to do any re-ordering.
Paul
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 2/5] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
2020-05-19 13:49 ` Jan Beulich
@ 2020-05-19 15:12 ` Paul Durrant
0 siblings, 0 replies; 20+ messages in thread
From: Paul Durrant @ 2020-05-19 15:12 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap', xen-devel, 'Daniel De Graaf'
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 May 2020 14:49
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Daniel De Graaf
> <dgdegra@tycho.nsa.gov>; Ian Jackson <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; Andrew Cooper
> <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>;
> Stefano Stabellini <sstabellini@kernel.org>
> Subject: Re: [PATCH v3 2/5] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext
>
> On 14.05.2020 12:44, Paul Durrant wrote:
> > --- a/xen/include/public/domctl.h
> > +++ b/xen/include/public/domctl.h
> > @@ -1129,6 +1129,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;
>
> This and its counterpart don't seem to get checked to be zero.
> While an option for a domctl, any desire to use the field in
> the future would then require an interface version bump.
>
Indeed. It does need to be zero checked.
Paul
> Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
2020-05-19 15:10 ` Paul Durrant
@ 2020-05-19 15:18 ` Jan Beulich
2020-05-19 15:32 ` Paul Durrant
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2020-05-19 15:18 UTC (permalink / raw)
To: paul
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap', xen-devel, 'Volodymyr Babchuk',
'Roger Pau Monné'
On 19.05.2020 17:10, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 19 May 2020 15:24
>>
>> On 19.05.2020 16:04, Paul Durrant wrote:
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: 19 May 2020 14:04
>>>>
>>>> On 14.05.2020 12:44, Paul Durrant wrote:
>>>>> +/*
>>>>> + * Register save and restore handlers. Save handlers will be invoked
>>>>> + * in order of DOMAIN_SAVE_CODE().
>>>>> + */
>>>>> +#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load) \
>>>>> + static int __init __domain_register_##_x##_save_restore(void) \
>>>>> + { \
>>>>> + domain_register_save_type( \
>>>>> + DOMAIN_SAVE_CODE(_x), \
>>>>> + #_x, \
>>>>> + &(_save), \
>>>>> + &(_load)); \
>>>>> + \
>>>>> + return 0; \
>>>>> + } \
>>>>> + __initcall(__domain_register_##_x##_save_restore);
>>>>
>>>> I'm puzzled by part of the comment: Invoking by save code looks
>>>> reasonable for the saving side (albeit END doesn't match this rule
>>>> afaics), but is this going to be good enough for the consuming side?
>>>
>>> No, this only relates to the save side which is why the comment
>>> says 'Save handlers'. I do note that it would be more consistent
>>> to use 'load' rather than 'restore' here though.
>>>
>>>> There may be dependencies between types, and with fixed ordering
>>>> there may be no way to insert a depended upon type ahead of an
>>>> already defined one (at least as long as the codes are meant to be
>>>> stable).
>>>>
>>>
>>> The ordering of load handlers is determined by the stream. I'll
>>> add a sentence saying that.
>>
>> I.e. the consumer of the "get" interface (and producer of the stream)
>> is supposed to take apart the output it gets, bring records into
>> suitable order (which implies it knows of all the records, and which
>> hence means this code may need updating in cases where I'd expect
>> only the hypervisor needs), and only then issue to the stream?
>
> The intention is that the stream is always in a suitable order so the
> load side does not have to do any re-ordering.
I understood this to be the intention, but what I continue to not
understand is where / how the save side orders it suitably. "Save
handlers will be invoked in order of DOMAIN_SAVE_CODE()" does not
allow for any ordering, unless at the time of the introduction of
a particular code you already know what others it may depend on
in the future, reserving appropriate codes.
And as said - END also doesn't look to fit this comment.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 4/5] common/domain: add a domain context record for shared_info...
2020-05-19 14:07 ` Jan Beulich
@ 2020-05-19 15:21 ` Paul Durrant
2020-05-19 15:34 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2020-05-19 15:21 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap',
xen-devel
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 May 2020 15:08
> To: Paul Durrant <paul@xen.org>
> Cc: xen-devel@lists.xenproject.org; Paul Durrant <pdurrant@amazon.com>; Ian Jackson
> <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>; George
> Dunlap <george.dunlap@citrix.com>; Julien Grall <julien@xen.org>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: Re: [PATCH v3 4/5] common/domain: add a domain context record for shared_info...
>
> On 14.05.2020 12:44, Paul Durrant wrote:
> > @@ -61,6 +62,76 @@ static void dump_header(void)
> >
> > }
> >
> > +static void print_binary(const char *prefix, void *val, size_t size,
>
> const also for val?
Yes, it can be.
>
> > + const char *suffix)
> > +{
> > + printf("%s", prefix);
> > +
> > + while (size--)
>
> Judging from style elsewhere you look to be missing two blanks
> here.
>
Yes.
> > + {
> > + uint8_t octet = *(uint8_t *)val++;
>
> Following the above then also better don't cast const away here.
>
> > + 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;
> > + shared_info_any_t *info;
> > + unsigned int i;
> > +
> > + GET_PTR(s);
> > +
> > + printf(" SHARED_INFO: has_32bit_shinfo: %s buffer_size: %u\n",
> > + s->has_32bit_shinfo ? "true" : "false", s->buffer_size);
> > +
> > + info = (shared_info_any_t *)s->buffer;
> > +
> > +#define GET_FIELD_PTR(_f) \
> > + (s->has_32bit_shinfo ? (void *)&(info->x32._f) : (void *)&(info->x64._f))
>
> Better cast to const void * ?
>
Ok.
> > +#define GET_FIELD_SIZE(_f) \
> > + (s->has_32bit_shinfo ? sizeof(info->x32._f) : sizeof(info->x64._f))
> > +#define GET_FIELD(_f) \
> > + (s->has_32bit_shinfo ? info->x32._f : info->x64._f)
> > +
> > + /* Array lengths are the same for 32-bit and 64-bit shared info */
>
> Not really, no:
>
> xen_ulong_t evtchn_pending[sizeof(xen_ulong_t) * 8];
> xen_ulong_t evtchn_mask[sizeof(xen_ulong_t) * 8];
>
Oh, I must have misread.
> > @@ -167,12 +238,14 @@ int main(int argc, char **argv)
> > 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);
>
> Stray insertion of a blank line?
>
Yes.
> > @@ -1649,6 +1650,65 @@ 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 = { .buffer_size = PAGE_SIZE };
>
> Why not sizeof(shared_info), utilizing the zero padding on the
> receiving side?
>
Ok, yes, I guess that would work.
> > + size_t hdr_size = offsetof(typeof(ctxt), buffer);
> > + int rc;
> > +
> > + rc = DOMAIN_SAVE_BEGIN(SHARED_INFO, c, 0);
> > + if ( rc )
> > + return rc;
> > +
> > +#ifdef CONFIG_COMPAT
> > + if ( !dry_run )
> > + ctxt.has_32bit_shinfo = has_32bit_shinfo(d);
> > +#endif
>
> Nothing will go wrong without the if(), I suppose? Better drop it
> then? It could then also easily be part of the initializer of ctxt.
>
Ok. I said last time I wanted to keep it as it was illustrative but I'll drop it since it has now come up twice.
> > + 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 || i ) /* expect only a single instance */
> > + return rc;
> > +
> > + rc = domain_load_data(c, &ctxt, hdr_size);
> > + if ( rc )
> > + return rc;
> > +
> > + if ( ctxt.pad[0] || ctxt.pad[1] || ctxt.pad[2] ||
> > + ctxt.buffer_size != PAGE_SIZE )
> > + return -EINVAL;
> > +
> > +#ifdef CONFIG_COMPAT
> > + d->arch.has_32bit_shinfo = ctxt.has_32bit_shinfo;
> > +#endif
>
> There's nothing wrong with using has_32bit_shinfo(d) here as well.
>
I just thought it looked odd.
> > --- a/xen/include/public/save.h
> > +++ b/xen/include/public/save.h
> > @@ -73,7 +73,16 @@ 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 {
> > + uint8_t has_32bit_shinfo;
> > + uint8_t pad[3];
>
> 32-(or 16-)bit flags, with just a single bit used for the purpose?
>
I debated that. Given this is xen/tools-only would a bit-field be acceptable?
Paul
> Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
2020-05-19 15:18 ` Jan Beulich
@ 2020-05-19 15:32 ` Paul Durrant
2020-05-19 15:37 ` Jan Beulich
0 siblings, 1 reply; 20+ messages in thread
From: Paul Durrant @ 2020-05-19 15:32 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap', xen-devel, 'Volodymyr Babchuk',
'Roger Pau Monné'
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 May 2020 16:18
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
> <ian.jackson@eu.citrix.com>; 'Julien Grall' <julien@xen.org>; '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 v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
>
> On 19.05.2020 17:10, Paul Durrant wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 19 May 2020 15:24
> >>
> >> On 19.05.2020 16:04, Paul Durrant wrote:
> >>>> From: Jan Beulich <jbeulich@suse.com>
> >>>> Sent: 19 May 2020 14:04
> >>>>
> >>>> On 14.05.2020 12:44, Paul Durrant wrote:
> >>>>> +/*
> >>>>> + * Register save and restore handlers. Save handlers will be invoked
> >>>>> + * in order of DOMAIN_SAVE_CODE().
> >>>>> + */
> >>>>> +#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load) \
> >>>>> + static int __init __domain_register_##_x##_save_restore(void) \
> >>>>> + { \
> >>>>> + domain_register_save_type( \
> >>>>> + DOMAIN_SAVE_CODE(_x), \
> >>>>> + #_x, \
> >>>>> + &(_save), \
> >>>>> + &(_load)); \
> >>>>> + \
> >>>>> + return 0; \
> >>>>> + } \
> >>>>> + __initcall(__domain_register_##_x##_save_restore);
> >>>>
> >>>> I'm puzzled by part of the comment: Invoking by save code looks
> >>>> reasonable for the saving side (albeit END doesn't match this rule
> >>>> afaics), but is this going to be good enough for the consuming side?
> >>>
> >>> No, this only relates to the save side which is why the comment
> >>> says 'Save handlers'. I do note that it would be more consistent
> >>> to use 'load' rather than 'restore' here though.
> >>>
> >>>> There may be dependencies between types, and with fixed ordering
> >>>> there may be no way to insert a depended upon type ahead of an
> >>>> already defined one (at least as long as the codes are meant to be
> >>>> stable).
> >>>>
> >>>
> >>> The ordering of load handlers is determined by the stream. I'll
> >>> add a sentence saying that.
> >>
> >> I.e. the consumer of the "get" interface (and producer of the stream)
> >> is supposed to take apart the output it gets, bring records into
> >> suitable order (which implies it knows of all the records, and which
> >> hence means this code may need updating in cases where I'd expect
> >> only the hypervisor needs), and only then issue to the stream?
> >
> > The intention is that the stream is always in a suitable order so the
> > load side does not have to do any re-ordering.
>
> I understood this to be the intention, but what I continue to not
> understand is where / how the save side orders it suitably. "Save
> handlers will be invoked in order of DOMAIN_SAVE_CODE()" does not
> allow for any ordering, unless at the time of the introduction of
> a particular code you already know what others it may depend on
> in the future, reserving appropriate codes.
>
That's just how it is *now*. If a new code is defined that needs to be in the stream before one of the existing ones then we'll have to introduce a more elaborate scheme to deal with that at the time. Using the save code as the array index and iterating in that order is purely a convenience, and the load side does not depend on entries being in save code order.
> And as said - END also doesn't look to fit this comment.
>
Ok, I can add a comment stating that exception.
Paul
> Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 4/5] common/domain: add a domain context record for shared_info...
2020-05-19 15:21 ` Paul Durrant
@ 2020-05-19 15:34 ` Jan Beulich
2020-05-19 15:35 ` Paul Durrant
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2020-05-19 15:34 UTC (permalink / raw)
To: paul
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap',
xen-devel
On 19.05.2020 17:21, Paul Durrant wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 19 May 2020 15:08
>>
>> On 14.05.2020 12:44, Paul Durrant wrote:
>>> --- a/xen/include/public/save.h
>>> +++ b/xen/include/public/save.h
>>> @@ -73,7 +73,16 @@ 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 {
>>> + uint8_t has_32bit_shinfo;
>>> + uint8_t pad[3];
>>
>> 32-(or 16-)bit flags, with just a single bit used for the purpose?
>>
>
> I debated that. Given this is xen/tools-only would a bit-field be acceptable?
Looking at domctl.h and sysctl.h, the only instance I can find is a
live-patching struct, and I'd suppose the addition of bitfields there
was missed in the hasty review back then. While it might be
acceptable, I'd recommend against it. It'll bite us the latest with
a port to an arch where endianness is not fixed, and hence may vary
between hypercall caller and callee. The standard way of using
#define-s looks more future proof.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 4/5] common/domain: add a domain context record for shared_info...
2020-05-19 15:34 ` Jan Beulich
@ 2020-05-19 15:35 ` Paul Durrant
0 siblings, 0 replies; 20+ messages in thread
From: Paul Durrant @ 2020-05-19 15:35 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap',
xen-devel
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 May 2020 16:34
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Ian Jackson'
> <ian.jackson@eu.citrix.com>; 'Wei Liu' <wl@xen.org>; 'Andrew Cooper' <andrew.cooper3@citrix.com>;
> 'George Dunlap' <george.dunlap@citrix.com>; 'Julien Grall' <julien@xen.org>; 'Stefano Stabellini'
> <sstabellini@kernel.org>
> Subject: Re: [PATCH v3 4/5] common/domain: add a domain context record for shared_info...
>
> On 19.05.2020 17:21, Paul Durrant wrote:
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 19 May 2020 15:08
> >>
> >> On 14.05.2020 12:44, Paul Durrant wrote:
> >>> --- a/xen/include/public/save.h
> >>> +++ b/xen/include/public/save.h
> >>> @@ -73,7 +73,16 @@ 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 {
> >>> + uint8_t has_32bit_shinfo;
> >>> + uint8_t pad[3];
> >>
> >> 32-(or 16-)bit flags, with just a single bit used for the purpose?
> >>
> >
> > I debated that. Given this is xen/tools-only would a bit-field be acceptable?
>
> Looking at domctl.h and sysctl.h, the only instance I can find is a
> live-patching struct, and I'd suppose the addition of bitfields there
> was missed in the hasty review back then. While it might be
> acceptable, I'd recommend against it. It'll bite us the latest with
> a port to an arch where endianness is not fixed, and hence may vary
> between hypercall caller and callee. The standard way of using
> #define-s looks more future proof.
>
Ok, I'll go with a flag. Probably is better in the long run.
Paul
> Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
2020-05-19 15:32 ` Paul Durrant
@ 2020-05-19 15:37 ` Jan Beulich
2020-05-19 15:38 ` Paul Durrant
0 siblings, 1 reply; 20+ messages in thread
From: Jan Beulich @ 2020-05-19 15:37 UTC (permalink / raw)
To: paul
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap', xen-devel, 'Volodymyr Babchuk',
'Roger Pau Monné'
On 19.05.2020 17:32, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 19 May 2020 16:18
>> To: paul@xen.org
>> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
>> <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
>> <ian.jackson@eu.citrix.com>; 'Julien Grall' <julien@xen.org>; '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 v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
>>
>> On 19.05.2020 17:10, Paul Durrant wrote:
>>>> From: Jan Beulich <jbeulich@suse.com>
>>>> Sent: 19 May 2020 15:24
>>>>
>>>> On 19.05.2020 16:04, Paul Durrant wrote:
>>>>>> From: Jan Beulich <jbeulich@suse.com>
>>>>>> Sent: 19 May 2020 14:04
>>>>>>
>>>>>> On 14.05.2020 12:44, Paul Durrant wrote:
>>>>>>> +/*
>>>>>>> + * Register save and restore handlers. Save handlers will be invoked
>>>>>>> + * in order of DOMAIN_SAVE_CODE().
>>>>>>> + */
>>>>>>> +#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load) \
>>>>>>> + static int __init __domain_register_##_x##_save_restore(void) \
>>>>>>> + { \
>>>>>>> + domain_register_save_type( \
>>>>>>> + DOMAIN_SAVE_CODE(_x), \
>>>>>>> + #_x, \
>>>>>>> + &(_save), \
>>>>>>> + &(_load)); \
>>>>>>> + \
>>>>>>> + return 0; \
>>>>>>> + } \
>>>>>>> + __initcall(__domain_register_##_x##_save_restore);
>>>>>>
>>>>>> I'm puzzled by part of the comment: Invoking by save code looks
>>>>>> reasonable for the saving side (albeit END doesn't match this rule
>>>>>> afaics), but is this going to be good enough for the consuming side?
>>>>>
>>>>> No, this only relates to the save side which is why the comment
>>>>> says 'Save handlers'. I do note that it would be more consistent
>>>>> to use 'load' rather than 'restore' here though.
>>>>>
>>>>>> There may be dependencies between types, and with fixed ordering
>>>>>> there may be no way to insert a depended upon type ahead of an
>>>>>> already defined one (at least as long as the codes are meant to be
>>>>>> stable).
>>>>>>
>>>>>
>>>>> The ordering of load handlers is determined by the stream. I'll
>>>>> add a sentence saying that.
>>>>
>>>> I.e. the consumer of the "get" interface (and producer of the stream)
>>>> is supposed to take apart the output it gets, bring records into
>>>> suitable order (which implies it knows of all the records, and which
>>>> hence means this code may need updating in cases where I'd expect
>>>> only the hypervisor needs), and only then issue to the stream?
>>>
>>> The intention is that the stream is always in a suitable order so the
>>> load side does not have to do any re-ordering.
>>
>> I understood this to be the intention, but what I continue to not
>> understand is where / how the save side orders it suitably. "Save
>> handlers will be invoked in order of DOMAIN_SAVE_CODE()" does not
>> allow for any ordering, unless at the time of the introduction of
>> a particular code you already know what others it may depend on
>> in the future, reserving appropriate codes.
>>
>
> That's just how it is *now*. If a new code is defined that needs to
> be in the stream before one of the existing ones then we'll have to
> introduce a more elaborate scheme to deal with that at the time.
> Using the save code as the array index and iterating in that order
> is purely a convenience, and the load side does not depend on
> entries being in save code order.
Could then you make the comment indicate so? This will allow people
wanting to modify this do so more easily, without much digging in
code or mail history.
Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
* RE: [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
2020-05-19 15:37 ` Jan Beulich
@ 2020-05-19 15:38 ` Paul Durrant
0 siblings, 0 replies; 20+ messages in thread
From: Paul Durrant @ 2020-05-19 15:38 UTC (permalink / raw)
To: 'Jan Beulich'
Cc: 'Stefano Stabellini', 'Julien Grall',
'Wei Liu', 'Andrew Cooper',
'Paul Durrant', 'Ian Jackson',
'George Dunlap', xen-devel, 'Volodymyr Babchuk',
'Roger Pau Monné'
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 19 May 2020 16:37
> To: paul@xen.org
> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
> <ian.jackson@eu.citrix.com>; 'Julien Grall' <julien@xen.org>; '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 v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context
>
> On 19.05.2020 17:32, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@suse.com>
> >> Sent: 19 May 2020 16:18
> >> To: paul@xen.org
> >> Cc: xen-devel@lists.xenproject.org; 'Paul Durrant' <pdurrant@amazon.com>; 'Andrew Cooper'
> >> <andrew.cooper3@citrix.com>; 'George Dunlap' <george.dunlap@citrix.com>; 'Ian Jackson'
> >> <ian.jackson@eu.citrix.com>; 'Julien Grall' <julien@xen.org>; '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 v3 1/5] xen/common: introduce a new framework for save/restore of 'domain'
> context
> >>
> >> On 19.05.2020 17:10, Paul Durrant wrote:
> >>>> From: Jan Beulich <jbeulich@suse.com>
> >>>> Sent: 19 May 2020 15:24
> >>>>
> >>>> On 19.05.2020 16:04, Paul Durrant wrote:
> >>>>>> From: Jan Beulich <jbeulich@suse.com>
> >>>>>> Sent: 19 May 2020 14:04
> >>>>>>
> >>>>>> On 14.05.2020 12:44, Paul Durrant wrote:
> >>>>>>> +/*
> >>>>>>> + * Register save and restore handlers. Save handlers will be invoked
> >>>>>>> + * in order of DOMAIN_SAVE_CODE().
> >>>>>>> + */
> >>>>>>> +#define DOMAIN_REGISTER_SAVE_RESTORE(_x, _save, _load) \
> >>>>>>> + static int __init __domain_register_##_x##_save_restore(void) \
> >>>>>>> + { \
> >>>>>>> + domain_register_save_type( \
> >>>>>>> + DOMAIN_SAVE_CODE(_x), \
> >>>>>>> + #_x, \
> >>>>>>> + &(_save), \
> >>>>>>> + &(_load)); \
> >>>>>>> + \
> >>>>>>> + return 0; \
> >>>>>>> + } \
> >>>>>>> + __initcall(__domain_register_##_x##_save_restore);
> >>>>>>
> >>>>>> I'm puzzled by part of the comment: Invoking by save code looks
> >>>>>> reasonable for the saving side (albeit END doesn't match this rule
> >>>>>> afaics), but is this going to be good enough for the consuming side?
> >>>>>
> >>>>> No, this only relates to the save side which is why the comment
> >>>>> says 'Save handlers'. I do note that it would be more consistent
> >>>>> to use 'load' rather than 'restore' here though.
> >>>>>
> >>>>>> There may be dependencies between types, and with fixed ordering
> >>>>>> there may be no way to insert a depended upon type ahead of an
> >>>>>> already defined one (at least as long as the codes are meant to be
> >>>>>> stable).
> >>>>>>
> >>>>>
> >>>>> The ordering of load handlers is determined by the stream. I'll
> >>>>> add a sentence saying that.
> >>>>
> >>>> I.e. the consumer of the "get" interface (and producer of the stream)
> >>>> is supposed to take apart the output it gets, bring records into
> >>>> suitable order (which implies it knows of all the records, and which
> >>>> hence means this code may need updating in cases where I'd expect
> >>>> only the hypervisor needs), and only then issue to the stream?
> >>>
> >>> The intention is that the stream is always in a suitable order so the
> >>> load side does not have to do any re-ordering.
> >>
> >> I understood this to be the intention, but what I continue to not
> >> understand is where / how the save side orders it suitably. "Save
> >> handlers will be invoked in order of DOMAIN_SAVE_CODE()" does not
> >> allow for any ordering, unless at the time of the introduction of
> >> a particular code you already know what others it may depend on
> >> in the future, reserving appropriate codes.
> >>
> >
> > That's just how it is *now*. If a new code is defined that needs to
> > be in the stream before one of the existing ones then we'll have to
> > introduce a more elaborate scheme to deal with that at the time.
> > Using the save code as the array index and iterating in that order
> > is purely a convenience, and the load side does not depend on
> > entries being in save code order.
>
> Could then you make the comment indicate so? This will allow people
> wanting to modify this do so more easily, without much digging in
> code or mail history.
Ok, I'll add some words to that effect.
Paul
>
> Jan
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-05-19 15:39 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-14 10:44 [PATCH v3 0/5] domain context infrastructure Paul Durrant
2020-05-14 10:44 ` [PATCH v3 1/5] xen/common: introduce a new framework for save/restore of 'domain' context Paul Durrant
2020-05-19 13:03 ` Jan Beulich
2020-05-19 14:04 ` Paul Durrant
2020-05-19 14:23 ` Jan Beulich
2020-05-19 15:10 ` Paul Durrant
2020-05-19 15:18 ` Jan Beulich
2020-05-19 15:32 ` Paul Durrant
2020-05-19 15:37 ` Jan Beulich
2020-05-19 15:38 ` Paul Durrant
2020-05-14 10:44 ` [PATCH v3 2/5] xen/common/domctl: introduce XEN_DOMCTL_get/setdomaincontext Paul Durrant
2020-05-19 13:49 ` Jan Beulich
2020-05-19 15:12 ` Paul Durrant
2020-05-14 10:44 ` [PATCH v3 3/5] tools/misc: add xen-domctx to present domain context Paul Durrant
2020-05-14 10:44 ` [PATCH v3 4/5] common/domain: add a domain context record for shared_info Paul Durrant
2020-05-19 14:07 ` Jan Beulich
2020-05-19 15:21 ` Paul Durrant
2020-05-19 15:34 ` Jan Beulich
2020-05-19 15:35 ` Paul Durrant
2020-05-14 10:44 ` [PATCH v3 5/5] tools/libxc: make use of domain context SHARED_INFO record Paul Durrant
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.