* [Xen-devel] [PATCH v3 1/6] libxl: add definition of INVALID_DOMID to the API
2020-01-16 9:35 [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Paul Durrant
@ 2020-01-16 9:35 ` Paul Durrant
2020-01-16 9:35 ` [Xen-devel] [PATCH v3 2/6] libxl_create: make 'soft reset' explicit Paul Durrant
` (5 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Paul Durrant @ 2020-01-16 9:35 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Paul Durrant, Ian Jackson, Wei Liu
Currently both xl and libxl have internal definitions of INVALID_DOMID
which happen to be identical. However, for the purposes of describing the
behaviour of libxl_domain_create_new/restore() it is useful to have a
specified invalid value for a domain id.
This patch therefore moves the libxl definition from libxl_internal.h to
libxl.h and removes the internal definition from xl_utils.h. The hardcoded
'-1' passed back via domcreate_complete() is then updated to INVALID_DOMID
and comment above libxl_domain_create_new/restore() is accordingly
modified.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
tools/libxl/libxl.h | 4 +++-
tools/libxl/libxl_create.c | 2 +-
tools/libxl/libxl_internal.h | 1 -
tools/xl/xl_utils.h | 2 --
4 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 54abb9db1f..18c1a2d6bf 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1527,9 +1527,11 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
/* domain related functions */
+#define INVALID_DOMID ~0
+
/* If the result is ERROR_ABORTED, the domain may or may not exist
* (in a half-created state). *domid will be valid and will be the
- * domain id, or -1, as appropriate */
+ * domain id, or INVALID_DOMID, as appropriate */
int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config,
uint32_t *domid,
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 32d45dcef0..bc425fee32 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1773,7 +1773,7 @@ static void domcreate_complete(libxl__egc *egc,
libxl__domain_destroy(egc, &dcs->dds);
return;
}
- dcs->guest_domid = -1;
+ dcs->guest_domid = INVALID_DOMID;
}
dcs->callback(egc, dcs, rc, dcs->guest_domid);
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ba8c9b41ab..3b708fba8f 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -121,7 +121,6 @@
#define STUBDOM_SPECIAL_CONSOLES 3
#define TAP_DEVICE_SUFFIX "-emu"
#define DOMID_XS_PATH "domid"
-#define INVALID_DOMID ~0
#define PVSHIM_BASENAME "xen-shim"
#define PVSHIM_CMDLINE "pv-shim console=xen,pv"
diff --git a/tools/xl/xl_utils.h b/tools/xl/xl_utils.h
index 7b9ccca30a..d98b419f10 100644
--- a/tools/xl/xl_utils.h
+++ b/tools/xl/xl_utils.h
@@ -52,8 +52,6 @@
#define STR_SKIP_PREFIX( a, b ) \
( STR_HAS_PREFIX(a, b) ? ((a) += strlen(b), 1) : 0 )
-#define INVALID_DOMID ~0
-
#define LOG(_f, _a...) dolog(__FILE__, __LINE__, __func__, _f "\n", ##_a)
/*
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Xen-devel] [PATCH v3 2/6] libxl_create: make 'soft reset' explicit
2020-01-16 9:35 [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Paul Durrant
2020-01-16 9:35 ` [Xen-devel] [PATCH v3 1/6] libxl: add definition of INVALID_DOMID to the API Paul Durrant
@ 2020-01-16 9:35 ` Paul Durrant
2020-01-16 9:35 ` [Xen-devel] [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids Paul Durrant
` (4 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Paul Durrant @ 2020-01-16 9:35 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Paul Durrant, Ian Jackson, Wei Liu
The 'soft reset' code path in libxl__domain_make() is currently taken if a
valid domid is passed into the function. A subsequent patch will enable
higher levels of the toolstack to determine the domid of newly created or
restored domains and therefore this criteria for choosing 'soft reset'
will no longer be usable.
This patch adds an extra boolean option to libxl__domain_make() to specify
whether it is being invoked in soft reset context and appropriately
modifies callers to choose the right value. To facilitate this, a new
'soft_reset' boolean field is added to struct libxl__domain_create_state
and the 'domid_soft_reset' field is renamed to 'domid' in anticipation of
its wider remit. For the moment do_domain_create() will always set
domid to INVALID_DOMID and hence we can add an assertion into
libxl__domain_create() that, if it is not called in soft reset context,
the passed in domid is exactly that value.
Whilst in the neighbourhood, some checks of 'restore_fd > -1' have been
replaced by 'restore_fd >= 0' to be more conventional and consistent with
checks of 'restore_fd < 0'.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
---
tools/libxl/libxl_create.c | 56 ++++++++++++++++++++++--------------
tools/libxl/libxl_dm.c | 2 +-
tools/libxl/libxl_internal.h | 5 ++--
3 files changed, 38 insertions(+), 25 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index bc425fee32..1835a5502c 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -538,7 +538,7 @@ out:
int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
libxl__domain_build_state *state,
- uint32_t *domid)
+ uint32_t *domid, bool soft_reset)
{
libxl_ctx *ctx = libxl__gc_owner(gc);
int ret, rc, nb_vm;
@@ -555,14 +555,15 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
libxl_domain_create_info *info = &d_config->c_info;
libxl_domain_build_info *b_info = &d_config->b_info;
+ assert(soft_reset || *domid == INVALID_DOMID);
+
uuid_string = libxl__uuid2string(gc, info->uuid);
if (!uuid_string) {
rc = ERROR_NOMEM;
goto out;
}
- /* Valid domid here means we're soft resetting. */
- if (!libxl_domid_valid_guest(*domid)) {
+ if (!soft_reset) {
struct xen_domctl_createdomain create = {
.ssidref = info->ssidref,
.max_vcpus = b_info->max_vcpus,
@@ -611,6 +612,14 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
goto out;
}
+ /*
+ * If soft_reset is set the the domid will have been valid on entry.
+ * If it was not set then xc_domain_create() should have assigned a
+ * valid value. Either way, if we reach this point, domid should be
+ * valid.
+ */
+ assert(libxl_domid_valid_guest(*domid));
+
ret = xc_cpupool_movedomain(ctx->xch, info->poolid, *domid);
if (ret < 0) {
LOGED(ERROR, *domid, "domain move fail");
@@ -1091,13 +1100,14 @@ static void initiate_domain_create(libxl__egc *egc,
libxl_domain_config *const d_config = dcs->guest_config;
const int restore_fd = dcs->restore_fd;
- domid = dcs->domid_soft_reset;
+ domid = dcs->domid;
libxl__domain_build_state_init(&dcs->build_state);
ret = libxl__domain_config_setdefault(gc,d_config,domid);
if (ret) goto error_out;
- ret = libxl__domain_make(gc, d_config, &dcs->build_state, &domid);
+ ret = libxl__domain_make(gc, d_config, &dcs->build_state, &domid,
+ dcs->soft_reset);
if (ret) {
LOGD(ERROR, domid, "cannot make domain: %d", ret);
dcs->guest_domid = domid;
@@ -1141,7 +1151,7 @@ static void initiate_domain_create(libxl__egc *egc,
if (ret)
goto error_out;
- if (restore_fd >= 0 || dcs->domid_soft_reset != INVALID_DOMID) {
+ if (restore_fd >= 0 || dcs->soft_reset) {
LOGD(DEBUG, domid, "restoring, not running bootloader");
domcreate_bootloader_done(egc, &dcs->bl, 0);
} else {
@@ -1217,7 +1227,7 @@ static void domcreate_bootloader_done(libxl__egc *egc,
dcs->sdss.dm.callback = domcreate_devmodel_started;
dcs->sdss.callback = domcreate_devmodel_started;
- if (restore_fd < 0 && dcs->domid_soft_reset == INVALID_DOMID) {
+ if (restore_fd < 0 && !dcs->soft_reset) {
rc = libxl__domain_build(gc, d_config, domid, state);
domcreate_rebuild_done(egc, dcs, rc);
return;
@@ -1827,7 +1837,7 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
libxl_domain_config_copy(ctx, &cdcs->dcs.guest_config_saved, d_config);
cdcs->dcs.restore_fd = cdcs->dcs.libxc_fd = restore_fd;
cdcs->dcs.send_back_fd = send_back_fd;
- if (restore_fd > -1) {
+ if (restore_fd >= 0) {
cdcs->dcs.restore_params = *params;
rc = libxl__fd_flags_modify_save(gc, cdcs->dcs.restore_fd,
~(O_NONBLOCK|O_NDELAY), 0,
@@ -1835,7 +1845,8 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config,
if (rc < 0) goto out_err;
}
cdcs->dcs.callback = domain_create_cb;
- cdcs->dcs.domid_soft_reset = INVALID_DOMID;
+ cdcs->dcs.domid = INVALID_DOMID;
+ cdcs->dcs.soft_reset = false;
if (cdcs->dcs.restore_params.checkpointed_stream ==
LIBXL_CHECKPOINTED_STREAM_COLO) {
@@ -1905,7 +1916,7 @@ static void soft_reset_dm_suspended(libxl__egc *egc,
int rc);
static int do_domain_soft_reset(libxl_ctx *ctx,
libxl_domain_config *d_config,
- uint32_t domid_soft_reset,
+ uint32_t domid,
const libxl_asyncop_how *ao_how,
const libxl_asyncprogress_how
*aop_console_how)
@@ -1933,15 +1944,16 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
libxl_domain_config_copy(ctx, &srs->cdcs.dcs.guest_config_saved,
d_config);
cdcs->dcs.restore_fd = -1;
- cdcs->dcs.domid_soft_reset = domid_soft_reset;
+ cdcs->dcs.domid = domid;
+ cdcs->dcs.soft_reset = true;
cdcs->dcs.callback = domain_create_cb;
libxl__ao_progress_gethow(&srs->cdcs.dcs.aop_console_how,
aop_console_how);
cdcs->domid_out = &domid_out;
- dom_path = libxl__xs_get_dompath(gc, domid_soft_reset);
+ dom_path = libxl__xs_get_dompath(gc, domid);
if (!dom_path) {
- LOGD(ERROR, domid_soft_reset, "failed to read domain path");
+ LOGD(ERROR, domid, "failed to read domain path");
rc = ERROR_FAIL;
goto out;
}
@@ -1950,7 +1962,7 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
GCSPRINTF("%s/store/ring-ref", dom_path),
&xs_store_mfn);
if (rc) {
- LOGD(ERROR, domid_soft_reset, "failed to read store/ring-ref.");
+ LOGD(ERROR, domid, "failed to read store/ring-ref.");
goto out;
}
state->store_mfn = xs_store_mfn ? atol(xs_store_mfn): 0;
@@ -1959,7 +1971,7 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
GCSPRINTF("%s/console/ring-ref", dom_path),
&xs_console_mfn);
if (rc) {
- LOGD(ERROR, domid_soft_reset, "failed to read console/ring-ref.");
+ LOGD(ERROR, domid, "failed to read console/ring-ref.");
goto out;
}
state->console_mfn = xs_console_mfn ? atol(xs_console_mfn): 0;
@@ -1968,20 +1980,20 @@ static int do_domain_soft_reset(libxl_ctx *ctx,
GCSPRINTF("%s/console/tty", dom_path),
&console_tty);
if (rc) {
- LOGD(ERROR, domid_soft_reset, "failed to read console/tty.");
+ LOGD(ERROR, domid, "failed to read console/tty.");
goto out;
}
state->console_tty = libxl__strdup(gc, console_tty);
dss->ao = ao;
- dss->domid = dss->dsps.domid = domid_soft_reset;
+ dss->domid = dss->dsps.domid = domid;
dss->dsps.dm_savefile = GCSPRINTF(LIBXL_DEVICE_MODEL_SAVE_FILE".%d",
- domid_soft_reset);
+ domid);
rc = libxl__save_emulator_xenstore_data(dss, &srs->toolstack_buf,
&srs->toolstack_len);
if (rc) {
- LOGD(ERROR, domid_soft_reset, "failed to save toolstack record.");
+ LOGD(ERROR, domid, "failed to save toolstack record.");
goto out;
}
@@ -2010,10 +2022,10 @@ static void soft_reset_dm_suspended(libxl__egc *egc,
* xenstore again with probably different store/console/...
* channels.
*/
- xs_release_domain(CTX->xsh, cdcs->dcs.domid_soft_reset);
+ xs_release_domain(CTX->xsh, cdcs->dcs.domid);
srs->dds.ao = ao;
- srs->dds.domid = cdcs->dcs.domid_soft_reset;
+ srs->dds.domid = cdcs->dcs.domid;
srs->dds.callback = domain_soft_reset_cb;
srs->dds.soft_reset = true;
libxl__domain_destroy(egc, &srs->dds);
@@ -2029,7 +2041,7 @@ static void domain_create_cb(libxl__egc *egc,
*cdcs->domid_out = domid;
- if (dcs->restore_fd > -1) {
+ if (dcs->restore_fd >= 0) {
flrc = libxl__fd_flags_restore(gc,
dcs->restore_fd, dcs->restore_fdfl);
/*
diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
index e92e412c1b..f758daf3b6 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2193,7 +2193,7 @@ void libxl__spawn_stub_dm(libxl__egc *egc, libxl__stub_dm_spawn_state *sdss)
/* fixme: this function can leak the stubdom if it fails */
ret = libxl__domain_make(gc, dm_config, stubdom_state,
- &sdss->pvqemu.guest_domid);
+ &sdss->pvqemu.guest_domid, false);
if (ret)
goto out;
uint32_t dm_domid = sdss->pvqemu.guest_domid;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3b708fba8f..cb23490c59 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1957,7 +1957,7 @@ _hidden void libxl__exec(libxl__gc *gc, int stdinfd, int stdoutfd,
_hidden int libxl__domain_make(libxl__gc *gc,
libxl_domain_config *d_config,
libxl__domain_build_state *state,
- uint32_t *domid);
+ uint32_t *domid, bool soft_reset);
_hidden int libxl__domain_build(libxl__gc *gc,
libxl_domain_config *d_config,
@@ -4134,7 +4134,8 @@ struct libxl__domain_create_state {
int restore_fdfl; /* original flags of restore_fd */
int send_back_fd;
libxl_domain_restore_params restore_params;
- uint32_t domid_soft_reset;
+ uint32_t domid;
+ bool soft_reset;
libxl__domain_create_cb *callback;
libxl_asyncprogress_how aop_console_how;
/* private to domain_create */
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread* [Xen-devel] [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids
2020-01-16 9:35 [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Paul Durrant
2020-01-16 9:35 ` [Xen-devel] [PATCH v3 1/6] libxl: add definition of INVALID_DOMID to the API Paul Durrant
2020-01-16 9:35 ` [Xen-devel] [PATCH v3 2/6] libxl_create: make 'soft reset' explicit Paul Durrant
@ 2020-01-16 9:35 ` Paul Durrant
2020-01-16 18:27 ` Ian Jackson
2020-01-16 9:36 ` [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid Paul Durrant
` (3 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Paul Durrant @ 2020-01-16 9:35 UTC (permalink / raw)
To: xen-devel; +Cc: Anthony PERARD, Paul Durrant, Ian Jackson, Wei Liu
A domid is considered retired if the domain it represents was destroyed
less than a specified number of seconds ago. The number can be set using
the environment variable LIBXL_DOMID_MAX_RETIREMENT. If the variable does
not exist then a default value of 60s is used.
Whenever a domain is destroyed, a time-stamped record will be written into
a history file (/var/run/xen/domid-history). To avoid the history file
growing too large, any records with time-stamps that indicate that the
domid has exceeded maximum retirement will also be purged.
A new utility function, libxl__is_retired_domid(), has been added. This
function reads the same history file checking whether a specified domid
has a record that does not exceed maximum retirement. Since this utility
function does not write to the file, no records are actually purged by it.
NOTE: Since the history file is hosted by a tmpfs file system, it is
automatically purged on boot thus allowing safe use of
CLOCK_MONOTONIC as a time source.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
v2:
- New in v2
---
tools/libxl/libxl_domain.c | 132 +++++++++++++++++++++++++++++++++++
tools/libxl/libxl_internal.h | 10 +++
2 files changed, 142 insertions(+)
diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
index 5714501778..7f255f184c 100644
--- a/tools/libxl/libxl_domain.c
+++ b/tools/libxl/libxl_domain.c
@@ -1268,6 +1268,137 @@ static void dm_destroy_cb(libxl__egc *egc,
libxl__devices_destroy(egc, &dis->drs);
}
+static unsigned int libxl__get_max_retirement(void)
+{
+ const char *env_max_retirement = getenv("LIBXL_DOMID_MAX_RETIREMENT");
+
+ return env_max_retirement ? strtol(env_max_retirement, NULL, 0) :
+ LIBXL_DOMID_MAX_RETIREMENT;
+}
+
+static int libxl__open_domid_history(libxl__gc *gc)
+{
+ const char *name;
+ int fd;
+ int ret;
+
+ name = GCSPRINTF("%s/domid-history", libxl__run_dir_path());
+
+ fd = open(name, O_RDWR|O_CREAT, 0644);
+ if (fd < 0) {
+ LOGE(ERROR, "unexpected error while trying open %s, errno=%d",
+ name, errno);
+ goto fail;
+ }
+
+ for (;;) {
+ ret = flock(fd, LOCK_EX);
+ if (!ret)
+ break;
+ if (errno != EINTR) {
+ /* All other errno: EBADF, EINVAL, ENOLCK, EWOULDBLOCK */
+ LOGE(ERROR,
+ "unexpected error while trying to lock %s, fd=%d, errno=%d",
+ name, fd, errno);
+ goto fail;
+ }
+ }
+
+ return fd;
+
+fail:
+ if (fd >= 0)
+ close(fd);
+
+ return -1;
+}
+
+/* Write a domid retirement record */
+static void libxl__retire_domid(libxl__gc *gc, uint32_t domid)
+{
+ long max_retirement = libxl__get_max_retirement();
+ int fd;
+ FILE *f;
+ long roff, woff;
+ char line[64];
+ struct timespec ts;
+
+ fd = libxl__open_domid_history(gc);
+ if (fd < 0)
+ return;
+
+ clock_gettime(CLOCK_MONOTONIC, &ts);
+
+ /* Purge old retirement records */
+
+ f = fdopen(fd, "r+");
+ woff = ftell(f);
+
+ while (fgets(line, sizeof(line), f)) {
+ unsigned long sec;
+ unsigned int ignored;
+
+ roff = ftell(f);
+
+ if (sscanf(line, "%lu %u", &sec, &ignored) != 2)
+ continue; /* Purge malformed lines */
+
+ if (ts.tv_sec - sec > max_retirement)
+ continue;
+
+ fseek(f, woff, SEEK_SET);
+ fputs(line, f);
+ woff = ftell(f);
+
+ fseek(f, roff, SEEK_SET);
+ }
+
+ fseek(f, woff, SEEK_SET);
+ fprintf(f, "%lu %u\n", ts.tv_sec, domid);
+ woff = ftell(f);
+ fflush(f);
+
+ ftruncate(fd, woff); /* may now be fewer records */
+
+ close(fd);
+}
+
+bool libxl__is_retired_domid(libxl__gc *gc, uint32_t domid)
+{
+ long max_retirement = libxl__get_max_retirement();
+ bool retired = false;
+ int fd;
+ FILE *f;
+ char line[64];
+ struct timespec ts;
+
+ fd = libxl__open_domid_history(gc);
+ if (fd < 0)
+ return false;
+
+ clock_gettime(CLOCK_MONOTONIC, &ts);
+
+ f = fdopen(fd, "r");
+
+ while (fgets(line, sizeof(line), f)) {
+ unsigned long sec;
+ unsigned int check;
+
+ if (sscanf(line, "%lu %u", &sec, &check) != 2)
+ continue;
+
+ if (check == domid &&
+ ts.tv_sec - sec <= max_retirement) {
+ retired = true;
+ break;
+ }
+ }
+
+ close(fd);
+
+ return retired;
+}
+
static void devices_destroy_cb(libxl__egc *egc,
libxl__devices_remove_state *drs,
int rc)
@@ -1331,6 +1462,7 @@ static void devices_destroy_cb(libxl__egc *egc,
if (!ctx->xch) goto badchild;
if (!dis->soft_reset) {
+ libxl__retire_domid(gc, domid);
rc = xc_domain_destroy(ctx->xch, domid);
} else {
rc = xc_domain_pause(ctx->xch, domid);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index cb23490c59..fcac8a93c5 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4770,6 +4770,16 @@ _hidden int libxl__domain_pvcontrol(libxl__egc *egc,
libxl__xswait_state *pvcontrol,
domid_t domid, const char *cmd);
+/*
+ * Maximum number of seconds a domid remains in retirement after domain
+ * destruction. This can be overidden by the environment variable of the
+ * same name.
+ */
+#define LIBXL_DOMID_MAX_RETIREMENT 60
+
+/* Check whether a domid is in retirement */
+bool libxl__is_retired_domid(libxl__gc *gc, uint32_t domid);
+
#endif
/*
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids
2020-01-16 9:35 ` [Xen-devel] [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids Paul Durrant
@ 2020-01-16 18:27 ` Ian Jackson
2020-01-17 9:26 ` Durrant, Paul
0 siblings, 1 reply; 24+ messages in thread
From: Ian Jackson @ 2020-01-16 18:27 UTC (permalink / raw)
To: Paul Durrant; +Cc: Anthony Perard, xen-devel, Wei Liu
Thanks. I think this is the algorithm as we discussed, thanks.
I have some comments about the implementation...
Paul Durrant writes ("[PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids"):
> A domid is considered retired if the domain it represents was destroyed
> less than a specified number of seconds ago. The number can be set using
> the environment variable LIBXL_DOMID_MAX_RETIREMENT. If the variable does
> not exist then a default value of 60s is used.
...
I'm afraid I think your update protocol for this file is wrong. In
general it is a bad idea to try to write over a file in-place. Doing
so is full of gotchas. (In this particular case for example I think
an interrupted attempt at cleaning the file can produce a corrupted
file containing nonsense.)
Can we please use the standard write-to-new-file-and-rename ?
Ie, to launder
flock(open("domid-history.lock"))
fopen("domid-history","r")
fopen("domid-history.new","w")
fgets/fputs
fclose
rename
close
(And no uses of ftell, fopen(,"r+"), etc.)
Reading can be done without taking the lock, if you so fancy.
I think there are a lot of missing error checks in this patch, but
since I'm asking for a different approach I won't point them out
individually.
> + fd = open(name, O_RDWR|O_CREAT, 0644);
> + if (fd < 0) {
> + LOGE(ERROR, "unexpected error while trying open %s, errno=%d",
> + name, errno);
> + goto fail;
> + }
> +
> + for (;;) {
> + ret = flock(fd, LOCK_EX);
I looked for a utility function to do this but didn't find one.
I think this is complicated because it needs to be a `carefd' in libxl
terms because of concurrent forking by other threads in the
application.
I suggest generalising libxl__lock_domain_userdata, which has all the
necessary code (and which also would permit removing the file in the
future).
I feel responsible for this inconvenience. If this is too tiresome
for you, I could do that part for you...
> +/* Write a domid retirement record */
> +static void libxl__retire_domid(libxl__gc *gc, uint32_t domid)
> +{
...
> + while (fgets(line, sizeof(line), f)) {
> + unsigned long sec;
> + unsigned int ignored;
> +
> + roff = ftell(f);
> +
> + if (sscanf(line, "%lu %u", &sec, &ignored) != 2)
> + continue; /* Purge malformed lines */
I'm not sure why you bother with fgets into a buffer, when you could
just use fscanf rather than sscanf. Your code doesn't need to take
much care about weird syntax which might occur (and indeed your code
here doesn't take such care).
> @@ -1331,6 +1462,7 @@ static void devices_destroy_cb(libxl__egc *egc,
> if (!ctx->xch) goto badchild;
>
> if (!dis->soft_reset) {
> + libxl__retire_domid(gc, domid);
I wonder if a better term than "retired" would be possible. I
initially found this patch a bit confusing because I thought a retired
domid would be one which had *not* been used recently. Maybe
"recent", "mark recent", etc. ? Ultimately this is a bikeshed issue
which I will leave this up to you, though.
I don't much like the environment variable to configure this. I don't
object to keeping it but can we have a comment saying this is not
intended for use in production ? Personally I would rather it was
hardcoded, or failing that, written to some config file.
Finally, I think this patch needs an addition to xen-init-dom0 to
remove or empty the record file. This is because while /run is
usually a tmpfs, this is not *necessarily* true.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids
2020-01-16 18:27 ` Ian Jackson
@ 2020-01-17 9:26 ` Durrant, Paul
2020-01-17 11:31 ` Ian Jackson
0 siblings, 1 reply; 24+ messages in thread
From: Durrant, Paul @ 2020-01-17 9:26 UTC (permalink / raw)
To: Ian Jackson; +Cc: Anthony Perard, xen-devel, Wei Liu
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 16 January 2020 19:28
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>
> Subject: Re: [PATCH v3 3/6] libxl: add infrastructure to track and query
> 'retired' domids
>
> Thanks. I think this is the algorithm as we discussed, thanks.
> I have some comments about the implementation...
>
> Paul Durrant writes ("[PATCH v3 3/6] libxl: add infrastructure to track
> and query 'retired' domids"):
> > A domid is considered retired if the domain it represents was destroyed
> > less than a specified number of seconds ago. The number can be set using
> > the environment variable LIBXL_DOMID_MAX_RETIREMENT. If the variable
> does
> > not exist then a default value of 60s is used.
> ...
>
> I'm afraid I think your update protocol for this file is wrong. In
> general it is a bad idea to try to write over a file in-place. Doing
> so is full of gotchas. (In this particular case for example I think
> an interrupted attempt at cleaning the file can produce a corrupted
> file containing nonsense.)
>
> Can we please use the standard write-to-new-file-and-rename ?
Ok, fair enough. I'd not really considered interruption as being too much of a risk but I guess I should.
> Ie, to launder
> flock(open("domid-history.lock"))
> fopen("domid-history","r")
> fopen("domid-history.new","w")
> fgets/fputs
> fclose
> rename
> close
>
> (And no uses of ftell, fopen(,"r+"), etc.)
>
> Reading can be done without taking the lock, if you so fancy.
>
> I think there are a lot of missing error checks in this patch, but
> since I'm asking for a different approach I won't point them out
> individually.
>
Ok.
> > + fd = open(name, O_RDWR|O_CREAT, 0644);
> > + if (fd < 0) {
> > + LOGE(ERROR, "unexpected error while trying open %s, errno=%d",
> > + name, errno);
> > + goto fail;
> > + }
> > +
> > + for (;;) {
> > + ret = flock(fd, LOCK_EX);
>
> I looked for a utility function to do this but didn't find one.
> I think this is complicated because it needs to be a `carefd' in libxl
> terms because of concurrent forking by other threads in the
> application.
>
> I suggest generalising libxl__lock_domain_userdata, which has all the
> necessary code (and which also would permit removing the file in the
> future).
>
> I feel responsible for this inconvenience. If this is too tiresome
> for you, I could do that part for you...
>
That's ok; I'll insert a preceding generalization patch, unless it turns into a total can of worms... which I doubt it will.
> > +/* Write a domid retirement record */
> > +static void libxl__retire_domid(libxl__gc *gc, uint32_t domid)
> > +{
> ...
> > + while (fgets(line, sizeof(line), f)) {
> > + unsigned long sec;
> > + unsigned int ignored;
> > +
> > + roff = ftell(f);
> > +
> > + if (sscanf(line, "%lu %u", &sec, &ignored) != 2)
> > + continue; /* Purge malformed lines */
>
> I'm not sure why you bother with fgets into a buffer, when you could
> just use fscanf rather than sscanf. Your code doesn't need to take
> much care about weird syntax which might occur (and indeed your code
> here doesn't take such care).
Well, I need to pull the line into a buffer if I'm going to write it out again, but otherwise I could indeed use fscanf().
>
> > @@ -1331,6 +1462,7 @@ static void devices_destroy_cb(libxl__egc *egc,
> > if (!ctx->xch) goto badchild;
> >
> > if (!dis->soft_reset) {
> > + libxl__retire_domid(gc, domid);
>
> I wonder if a better term than "retired" would be possible. I
> initially found this patch a bit confusing because I thought a retired
> domid would be one which had *not* been used recently. Maybe
> "recent", "mark recent", etc. ? Ultimately this is a bikeshed issue
> which I will leave this up to you, though.
>
Ok, 'recent' is probably clearer. I'll s/retired/recent/g.
>
> I don't much like the environment variable to configure this. I don't
> object to keeping it but can we have a comment saying this is not
> intended for use in production ? Personally I would rather it was
> hardcoded, or failing that, written to some config file.
>
The problem is that libxl has no config file. Env variables seem to be used for other things so I followed suit. I'd rather keep the override for debug purposes; I'll stick a comment in the header saying that's what it's for though, as you suggest.
>
> Finally, I think this patch needs an addition to xen-init-dom0 to
> remove or empty the record file. This is because while /run is
> usually a tmpfs, this is not *necessarily* true.
>
Ok, if we cannot rely on it being tmpfs then I will do that.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids
2020-01-17 9:26 ` Durrant, Paul
@ 2020-01-17 11:31 ` Ian Jackson
0 siblings, 0 replies; 24+ messages in thread
From: Ian Jackson @ 2020-01-17 11:31 UTC (permalink / raw)
To: Durrant, Paul; +Cc: Anthony Perard, xen-devel, Wei Liu
Durrant, Paul writes ("RE: [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids"):
> [Ian;]
> > I'm not sure why you bother with fgets into a buffer, when you could
> > just use fscanf rather than sscanf. Your code doesn't need to take
> > much care about weird syntax which might occur (and indeed your code
> > here doesn't take such care).
>
> Well, I need to pull the line into a buffer if I'm going to write it out again, but otherwise I could indeed use fscanf().
Well, you could just fprintf the information.
> Ok, 'recent' is probably clearer. I'll s/retired/recent/g.
Thanks.
> > I don't much like the environment variable to configure this. I don't
> > object to keeping it but can we have a comment saying this is not
> > intended for use in production ? Personally I would rather it was
> > hardcoded, or failing that, written to some config file.
>
> The problem is that libxl has no config file. Env variables seem to be used for other things so I followed suit. I'd rather keep the override for debug purposes; I'll stick a comment in the header saying that's what it's for though, as you suggest.
OK. You are right about the lack of a config file being a problem.
> > Finally, I think this patch needs an addition to xen-init-dom0 to
> > remove or empty the record file. This is because while /run is
> > usually a tmpfs, this is not *necessarily* true.
>
> Ok, if we cannot rely on it being tmpfs then I will do that.
Thanks.
Thanks for the rest of your reply, too, which I snipped as I had
nothing more to say than `thanks'.
Regards,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-16 9:35 [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Paul Durrant
` (2 preceding siblings ...)
2020-01-16 9:35 ` [Xen-devel] [PATCH v3 3/6] libxl: add infrastructure to track and query 'retired' domids Paul Durrant
@ 2020-01-16 9:36 ` Paul Durrant
2020-01-16 9:40 ` Jan Beulich
` (2 more replies)
2020-01-16 9:36 ` [Xen-devel] [PATCH v3 5/6] xl.conf: introduce 'domid_policy' Paul Durrant
` (2 subsequent siblings)
6 siblings, 3 replies; 24+ messages in thread
From: Paul Durrant @ 2020-01-16 9:36 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Paul Durrant, jandryuk,
Ian Jackson, Jan Beulich, Anthony PERARD
This patch adds a 'domid' field to libxl_domain_create_info and then
modifies do_domain_create() to use that value if it is valid. Any valid
domid will be checked against the retired domid list before being passed
to libxl__domain_make().
If the domid value is invalid then Xen will choose the domid, as before,
unless the value is the new special RANDOM_DOMID value added to the API.
This value instructs libxl__domain_make() to select a random domid value,
check it for validity, verify it does not match a retired domain, and then
pass it to Xen's XEN_DOMCTL_createdomain operation. If Xen determines that
it co-incides with an existing domain, a new random value will be
selected and the operation will be re-tried.
NOTE: libxl__logv() is also modified to only log valid domid values in
messages rather than any domid, valid or otherwise, that is not
INVALID_DOMID.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: jandryuk@gmail.com
v3:
- Added DOMID_MASK definition used to mask randomized values
- Use stack variable to avoid assuming endianness
v2:
- Re-worked to use a value from libxl_domain_create_info
---
tools/libxl/libxl.h | 9 +++++++++
tools/libxl/libxl_create.c | 36 +++++++++++++++++++++++++++++++++++-
tools/libxl/libxl_internal.c | 2 +-
tools/libxl/libxl_types.idl | 1 +
xen/include/public/xen.h | 3 +++
5 files changed, 49 insertions(+), 2 deletions(-)
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 18c1a2d6bf..7e60ee1c8b 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1268,6 +1268,14 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, const libxl_mac *src);
*/
#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONFIG
+/*
+ * LIBXL_HAVE_CREATEINFO_DOMID
+ *
+ * libxl_domain_create_new() and libxl_domain_create_restore() will use
+ * a domid specified in libxl_domain_create_info().
+ */
+#define LIBXL_HAVE_CREATEINFO_DOMID
+
typedef char **libxl_string_list;
void libxl_string_list_dispose(libxl_string_list *sl);
int libxl_string_list_length(const libxl_string_list *sl);
@@ -1528,6 +1536,7 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
/* domain related functions */
#define INVALID_DOMID ~0
+#define RANDOM_DOMID (INVALID_DOMID - 1)
/* If the result is ERROR_ABORTED, the domain may or may not exist
* (in a half-created state). *domid will be valid and will be the
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1835a5502c..a80d4f3755 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -600,9 +600,43 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
goto out;
}
- ret = xc_domain_create(ctx->xch, domid, &create);
+ if (libxl_domid_valid_guest(info->domid)) {
+ *domid = info->domid;
+
+ if (libxl__is_retired_domid(gc, *domid)) {
+ LOGED(ERROR, *domid, "domain id is retired");
+ rc = ERROR_FAIL;
+ goto out;
+ }
+ } else if (info->domid == RANDOM_DOMID) {
+ *domid = 0; /* Zero-out initial value */
+ }
+
+ for (;;) {
+ if (info->domid == RANDOM_DOMID) {
+ uint16_t v;
+
+ /* Randomize lower order bytes */
+ ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
+ if (ret < 0)
+ break;
+
+ v &= DOMID_MASK;
+ if (!libxl_domid_valid_guest(v) ||
+ libxl__is_retired_domid(gc, v))
+ continue;
+
+ *domid = v;
+ }
+
+ ret = xc_domain_create(ctx->xch, domid, &create);
+ if (ret == 0 || errno != EEXIST || info->domid != RANDOM_DOMID)
+ break;
+ }
+
if (ret < 0) {
LOGED(ERROR, *domid, "domain creation fail");
+ *domid = INVALID_DOMID;
rc = ERROR_FAIL;
goto out;
}
diff --git a/tools/libxl/libxl_internal.c b/tools/libxl/libxl_internal.c
index ba5637358e..dc6aaa9c9f 100644
--- a/tools/libxl/libxl_internal.c
+++ b/tools/libxl/libxl_internal.c
@@ -234,7 +234,7 @@ void libxl__logv(libxl_ctx *ctx, xentoollog_level msglevel, int errnoval,
fileline[sizeof(fileline)-1] = 0;
domain[0] = 0;
- if (domid != INVALID_DOMID)
+ if (libxl_domid_valid_guest(domid))
snprintf(domain, sizeof(domain), "Domain %"PRIu32":", domid);
x:
xtl_log(ctx->lg, msglevel, errnoval, "libxl",
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 7921950f6a..d0d431614f 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -409,6 +409,7 @@ libxl_domain_create_info = Struct("domain_create_info",[
("ssidref", uint32),
("ssid_label", string),
("name", string),
+ ("domid", libxl_domid),
("uuid", libxl_uuid),
("xsdata", libxl_key_value_list),
("platformdata", libxl_key_value_list),
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index d2198dffad..fade089a7b 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
/* Idle domain. */
#define DOMID_IDLE xen_mk_uint(0x7FFF)
+/* Mask for valid domain id values */
+#define DOMID_MASK 0x7FFF
+
#ifndef __ASSEMBLY__
typedef uint16_t domid_t;
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-16 9:36 ` [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid Paul Durrant
@ 2020-01-16 9:40 ` Jan Beulich
2020-01-16 9:46 ` Durrant, Paul
2020-01-16 15:53 ` Jason Andryuk
2020-01-16 18:36 ` Ian Jackson
2 siblings, 1 reply; 24+ messages in thread
From: Jan Beulich @ 2020-01-16 9:40 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, jandryuk, Ian Jackson,
Anthony PERARD, xen-devel
On 16.01.2020 10:36, Paul Durrant wrote:
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> /* Idle domain. */
> #define DOMID_IDLE xen_mk_uint(0x7FFF)
>
> +/* Mask for valid domain id values */
> +#define DOMID_MASK 0x7FFF
Seeing it used in context, any reason not to use xen_mk_uint()
here as well?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-16 9:40 ` Jan Beulich
@ 2020-01-16 9:46 ` Durrant, Paul
2020-01-16 9:59 ` Jan Beulich
0 siblings, 1 reply; 24+ messages in thread
From: Durrant, Paul @ 2020-01-16 9:46 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, jandryuk, Ian Jackson,
Anthony PERARD, xen-devel
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 16 January 2020 10:40
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Ian Jackson
> <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; Anthony PERARD
> <anthony.perard@citrix.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
> George Dunlap <George.Dunlap@eu.citrix.com>; Julien Grall
> <julien@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano
> Stabellini <sstabellini@kernel.org>; jandryuk@gmail.com
> Subject: Re: [PATCH v3 4/6] libxl: allow creation of domains with a
> specified or random domid
>
> On 16.01.2020 10:36, Paul Durrant wrote:
> > --- a/xen/include/public/xen.h
> > +++ b/xen/include/public/xen.h
> > @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
> > /* Idle domain. */
> > #define DOMID_IDLE xen_mk_uint(0x7FFF)
> >
> > +/* Mask for valid domain id values */
> > +#define DOMID_MASK 0x7FFF
>
> Seeing it used in context, any reason not to use xen_mk_uint()
> here as well?
>
I did wonder but I thought it best not to impose a type on a mask.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-16 9:46 ` Durrant, Paul
@ 2020-01-16 9:59 ` Jan Beulich
0 siblings, 0 replies; 24+ messages in thread
From: Jan Beulich @ 2020-01-16 9:59 UTC (permalink / raw)
To: Durrant, Paul
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, jandryuk, Ian Jackson,
Anthony PERARD, xen-devel
On 16.01.2020 10:46, Durrant, Paul wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 16 January 2020 10:40
>> To: Durrant, Paul <pdurrant@amazon.co.uk>
>> Cc: xen-devel@lists.xenproject.org; Ian Jackson
>> <ian.jackson@eu.citrix.com>; Wei Liu <wl@xen.org>; Anthony PERARD
>> <anthony.perard@citrix.com>; Andrew Cooper <andrew.cooper3@citrix.com>;
>> George Dunlap <George.Dunlap@eu.citrix.com>; Julien Grall
>> <julien@xen.org>; Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; jandryuk@gmail.com
>> Subject: Re: [PATCH v3 4/6] libxl: allow creation of domains with a
>> specified or random domid
>>
>> On 16.01.2020 10:36, Paul Durrant wrote:
>>> --- a/xen/include/public/xen.h
>>> +++ b/xen/include/public/xen.h
>>> @@ -614,6 +614,9 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t);
>>> /* Idle domain. */
>>> #define DOMID_IDLE xen_mk_uint(0x7FFF)
>>>
>>> +/* Mask for valid domain id values */
>>> +#define DOMID_MASK 0x7FFF
>>
>> Seeing it used in context, any reason not to use xen_mk_uint()
>> here as well?
>>
>
> I did wonder but I thought it best not to impose a type on a mask.
I'd be happy to see the other DOMID_* uses dropped (I don't see
whey they had a U suffix originally, which was then converted
to xen_mk_uint()), but I'd prefer the entire set to be consistent.
I can see in general why a mask might better not be explicitly
(or implicitly) unsigned, but here I don't really see plausible
uses of ~ on the mask.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-16 9:36 ` [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid Paul Durrant
2020-01-16 9:40 ` Jan Beulich
@ 2020-01-16 15:53 ` Jason Andryuk
2020-01-16 18:36 ` Ian Jackson
2 siblings, 0 replies; 24+ messages in thread
From: Jason Andryuk @ 2020-01-16 15:53 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Jan Beulich,
Anthony PERARD, xen-devel
On Thu, Jan 16, 2020 at 4:36 AM Paul Durrant <pdurrant@amazon.com> wrote:
>
> This patch adds a 'domid' field to libxl_domain_create_info and then
> modifies do_domain_create() to use that value if it is valid. Any valid
> domid will be checked against the retired domid list before being passed
> to libxl__domain_make().
> If the domid value is invalid then Xen will choose the domid, as before,
> unless the value is the new special RANDOM_DOMID value added to the API.
> This value instructs libxl__domain_make() to select a random domid value,
> check it for validity, verify it does not match a retired domain, and then
> pass it to Xen's XEN_DOMCTL_createdomain operation. If Xen determines that
> it co-incides with an existing domain, a new random value will be
> selected and the operation will be re-tried.
>
> NOTE: libxl__logv() is also modified to only log valid domid values in
> messages rather than any domid, valid or otherwise, that is not
> INVALID_DOMID.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Looks good.
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-16 9:36 ` [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid Paul Durrant
2020-01-16 9:40 ` Jan Beulich
2020-01-16 15:53 ` Jason Andryuk
@ 2020-01-16 18:36 ` Ian Jackson
2020-01-17 9:37 ` Durrant, Paul
2 siblings, 1 reply; 24+ messages in thread
From: Ian Jackson @ 2020-01-16 18:36 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, jandryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
Hi. This broadly contains what I expected, but:
Paul Durrant writes ("[PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid"):
> + for (;;) {
> + if (info->domid == RANDOM_DOMID) {
> + uint16_t v;
> +
> + /* Randomize lower order bytes */
> + ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
> + if (ret < 0)
> + break;
> +
> + v &= DOMID_MASK;
> + if (!libxl_domid_valid_guest(v) ||
> + libxl__is_retired_domid(gc, v))
> + continue;
> +
> + *domid = v;
> + }
> +
> + ret = xc_domain_create(ctx->xch, domid, &create);
> + if (ret == 0 || errno != EEXIST || info->domid != RANDOM_DOMID)
> + break;
> + }
I think this has a race.
Thread A, in domain destroy Thread B, in code above
choose domid V
check V in recent domid list
add V to recent domid list
destroy domain V in Xen
create domain V in Xen
continue constructing V
Thread B improperly constructs a new guest using V, exposing anyone
who was talking about V a moment ago to bugs. Some code might even
fail to spot the interval where V does not exist and carry on talking
to the new V as if it were the old one...
I think there are only two possible solutions:
- Check the domain's entry in the recent list *after* creating
the domain in Xen. This involves accepting that we will
reuse the domid but only for a domain we are in the early
stages of constructing, so hopefully without bad consequence?
- Take the recent domid lock.
Also, it seems to me that we should check the recent domid list if we
let Xen choose the domid. Maybe that can be in a subsequent patch...
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-16 18:36 ` Ian Jackson
@ 2020-01-17 9:37 ` Durrant, Paul
2020-01-17 11:35 ` Ian Jackson
0 siblings, 1 reply; 24+ messages in thread
From: Durrant, Paul @ 2020-01-17 9:37 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, jandryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 16 January 2020 19:36
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>;
> jandryuk@gmail.com
> Subject: Re: [PATCH v3 4/6] libxl: allow creation of domains with a
> specified or random domid
>
> Hi. This broadly contains what I expected, but:
>
> Paul Durrant writes ("[PATCH v3 4/6] libxl: allow creation of domains with
> a specified or random domid"):
>
> > + for (;;) {
> > + if (info->domid == RANDOM_DOMID) {
> > + uint16_t v;
> > +
> > + /* Randomize lower order bytes */
> > + ret = libxl__random_bytes(gc, (void *)&v, sizeof(v));
> > + if (ret < 0)
> > + break;
> > +
> > + v &= DOMID_MASK;
> > + if (!libxl_domid_valid_guest(v) ||
> > + libxl__is_retired_domid(gc, v))
> > + continue;
> > +
> > + *domid = v;
> > + }
> > +
> > + ret = xc_domain_create(ctx->xch, domid, &create);
> > + if (ret == 0 || errno != EEXIST || info->domid !=
> RANDOM_DOMID)
> > + break;
> > + }
>
> I think this has a race.
>
> Thread A, in domain destroy Thread B, in code above
>
> choose domid V
> check V in recent domid list
>
> add V to recent domid list
> destroy domain V in Xen
>
> create domain V in Xen
> continue constructing V
>
> Thread B improperly constructs a new guest using V, exposing anyone
> who was talking about V a moment ago to bugs. Some code might even
> fail to spot the interval where V does not exist and carry on talking
> to the new V as if it were the old one...
>
> I think there are only two possible solutions:
>
> - Check the domain's entry in the recent list *after* creating
> the domain in Xen. This involves accepting that we will
> reuse the domid but only for a domain we are in the early
> stages of constructing, so hopefully without bad consequence?
>
> - Take the recent domid lock.
>
Or take a global file lock in libxl around domain creation and destruction?
> Also, it seems to me that we should check the recent domid list if we
> let Xen choose the domid. Maybe that can be in a subsequent patch...
>
Well, we could solve all this, remove the need for a file and all the associated complexity by simply keeping history inside the hypervisor. I don't know how the Xen maintainers will feel about that though, as Xen itself shouldn't have a problem with eager domid re-use.
Paul
> Thanks,
> Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-17 9:37 ` Durrant, Paul
@ 2020-01-17 11:35 ` Ian Jackson
2020-01-17 12:06 ` Durrant, Paul
0 siblings, 1 reply; 24+ messages in thread
From: Ian Jackson @ 2020-01-17 11:35 UTC (permalink / raw)
To: Durrant, Paul
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, jandryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
Durrant, Paul writes ("RE: [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid"):
> [Ian:]
> > I think there are only two possible solutions:
> >
> > - Check the domain's entry in the recent list *after* creating
> > the domain in Xen. This involves accepting that we will
> > reuse the domid but only for a domain we are in the early
> > stages of constructing, so hopefully without bad consequence?
> >
> > - Take the recent domid lock.
> >
>
> Or take a global file lock in libxl around domain creation and destruction?
We want domain construction to be concurrent, when it can be. So I
think a lock around just xc_domain_create is OK but a lock around the
whole operation is not.
> > Also, it seems to me that we should check the recent domid list if we
> > let Xen choose the domid. Maybe that can be in a subsequent patch...
>
> Well, we could solve all this, remove the need for a file and all the associated complexity by simply keeping history inside the hypervisor. I don't know how the Xen maintainers will feel about that though, as Xen itself shouldn't have a problem with eager domid re-use.
I think this doesn't need to be done in the hypervisor so I am
inclined to say it shouldn't be. Also, there is a lot of policy here...
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-17 11:35 ` Ian Jackson
@ 2020-01-17 12:06 ` Durrant, Paul
2020-01-17 15:30 ` Ian Jackson
0 siblings, 1 reply; 24+ messages in thread
From: Durrant, Paul @ 2020-01-17 12:06 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, jandryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 17 January 2020 12:36
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>;
> jandryuk@gmail.com
> Subject: RE: [PATCH v3 4/6] libxl: allow creation of domains with a
> specified or random domid
>
> Durrant, Paul writes ("RE: [PATCH v3 4/6] libxl: allow creation of domains
> with a specified or random domid"):
> > [Ian:]
> > > I think there are only two possible solutions:
> > >
> > > - Check the domain's entry in the recent list *after* creating
> > > the domain in Xen. This involves accepting that we will
> > > reuse the domid but only for a domain we are in the early
> > > stages of constructing, so hopefully without bad consequence?
> > >
> > > - Take the recent domid lock.
> > >
> >
> > Or take a global file lock in libxl around domain creation and
> destruction?
>
> We want domain construction to be concurrent, when it can be. So I
> think a lock around just xc_domain_create is OK but a lock around the
> whole operation is not.
>
> > > Also, it seems to me that we should check the recent domid list if we
> > > let Xen choose the domid. Maybe that can be in a subsequent patch...
> >
> > Well, we could solve all this, remove the need for a file and all the
> associated complexity by simply keeping history inside the hypervisor. I
> don't know how the Xen maintainers will feel about that though, as Xen
> itself shouldn't have a problem with eager domid re-use.
>
> I think this doesn't need to be done in the hypervisor so I am
> inclined to say it shouldn't be. Also, there is a lot of policy here...
>
Ok, to cover all bases then it seems like checking the domid after creation and then destroying if it is too recent is the better option.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-17 12:06 ` Durrant, Paul
@ 2020-01-17 15:30 ` Ian Jackson
2020-01-20 8:18 ` Durrant, Paul
0 siblings, 1 reply; 24+ messages in thread
From: Ian Jackson @ 2020-01-17 15:30 UTC (permalink / raw)
To: Durrant, Paul
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, jandryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
Durrant, Paul writes ("RE: [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid"):
> Ok, to cover all bases then it seems like checking the domid after creation and then destroying if it is too recent is the better option.
I think so, yes. I think the recent timestamp should be updated in
this case. (Faff!)
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid
2020-01-17 15:30 ` Ian Jackson
@ 2020-01-20 8:18 ` Durrant, Paul
0 siblings, 0 replies; 24+ messages in thread
From: Durrant, Paul @ 2020-01-20 8:18 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
Andrew Cooper, jandryuk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 17 January 2020 15:31
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>;
> jandryuk@gmail.com
> Subject: RE: [PATCH v3 4/6] libxl: allow creation of domains with a
> specified or random domid
>
> Durrant, Paul writes ("RE: [PATCH v3 4/6] libxl: allow creation of domains
> with a specified or random domid"):
> > Ok, to cover all bases then it seems like checking the domid after
> creation and then destroying if it is too recent is the better option.
>
> I think so, yes. I think the recent timestamp should be updated in
> this case. (Faff!)
>
I don't think we need to mess with the time-stamp in this case. The domain will be killed very quickly, before any PV backends are built and IIUC that's what we care about when it comes to re-using domids too quickly.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Xen-devel] [PATCH v3 5/6] xl.conf: introduce 'domid_policy'
2020-01-16 9:35 [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Paul Durrant
` (3 preceding siblings ...)
2020-01-16 9:36 ` [Xen-devel] [PATCH v3 4/6] libxl: allow creation of domains with a specified or random domid Paul Durrant
@ 2020-01-16 9:36 ` Paul Durrant
2020-01-16 18:37 ` Ian Jackson
2020-01-16 9:36 ` [Xen-devel] [PATCH v3 6/6] xl: allow domid to be preserved on save/restore or migrate Paul Durrant
2020-01-16 18:43 ` [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Ian Jackson
6 siblings, 1 reply; 24+ messages in thread
From: Paul Durrant @ 2020-01-16 9:36 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu
This patch adds a new global 'domid_policy' configuration option to decide
how domain id values are allocated for new domains. It may be set to one of
two values:
"xen", the default value, will cause an invalid domid value to be passed
to do_domain_create() preserving the existing behaviour of having Xen
choose the domid value during domain_create().
"random" will cause the special RANDOM_DOMID value to be passed to
do_domain_create() such that libxl__domain_make() will select a random
domid value.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
v2:
- New in v2
---
docs/man/xl.conf.5.pod | 10 ++++++++++
tools/examples/xl.conf | 4 ++++
tools/xl/xl.c | 10 ++++++++++
tools/xl/xl.h | 1 +
tools/xl/xl_vmcontrol.c | 2 ++
5 files changed, 27 insertions(+)
diff --git a/docs/man/xl.conf.5.pod b/docs/man/xl.conf.5.pod
index 207ab3e77a..41ee428744 100644
--- a/docs/man/xl.conf.5.pod
+++ b/docs/man/xl.conf.5.pod
@@ -45,6 +45,16 @@ The semantics of each C<KEY> defines which form of C<VALUE> is required.
=over 4
+=item B<domid_policy="xen"|"random">
+
+Determines how domain-id is set when creating a new domain.
+
+If set to "xen" then the hypervisor will allocate new domain-id values on a sequential basis.
+
+If set to "random" then a random domain-id value will be chosen.
+
+Default: "xen"
+
=item B<autoballoon="off"|"on"|"auto">
If set to "on" then C<xl> will automatically reduce the amount of
diff --git a/tools/examples/xl.conf b/tools/examples/xl.conf
index 0446deb304..95f2f442d3 100644
--- a/tools/examples/xl.conf
+++ b/tools/examples/xl.conf
@@ -1,5 +1,9 @@
## Global XL config file ##
+# Set domain-id policy. "xen" means that the hypervisor will choose the
+# id of a new domain. "random" means that a random value will be chosen.
+#domid_policy="xen"
+
# Control whether dom0 is ballooned down when xen doesn't have enough
# free memory to create a domain. "auto" means only balloon if dom0
# starts with all the host's memory.
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index 3d4390a46d..2a5ddd4390 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -54,6 +54,7 @@ int claim_mode = 1;
bool progress_use_cr = 0;
int max_grant_frames = -1;
int max_maptrack_frames = -1;
+libxl_domid domid_policy = INVALID_DOMID;
xentoollog_level minmsglevel = minmsglevel_default;
@@ -228,6 +229,15 @@ static void parse_global_config(const char *configfile,
else
libxl_bitmap_set_any(&global_pv_affinity_mask);
+ if (!xlu_cfg_get_string (config, "domid_policy", &buf, 0)) {
+ if (!strcmp(buf, "xen"))
+ domid_policy = INVALID_DOMID;
+ else if (!strcmp(buf, "random"))
+ domid_policy = RANDOM_DOMID;
+ else
+ fprintf(stderr, "invalid domid_policy option");
+ }
+
xlu_cfg_destroy(config);
}
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 60bdad8ffb..2b4709efb2 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -283,6 +283,7 @@ extern int max_maptrack_frames;
extern libxl_bitmap global_vm_affinity_mask;
extern libxl_bitmap global_hvm_affinity_mask;
extern libxl_bitmap global_pv_affinity_mask;
+extern libxl_domid domid_policy;
enum output_format {
OUTPUT_FORMAT_JSON,
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index e520b1da79..39292acfe6 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -899,6 +899,8 @@ start:
autoconnect_console_how = 0;
}
+ d_config.c_info.domid = domid_policy;
+
if ( restoring ) {
libxl_domain_restore_params params;
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 5/6] xl.conf: introduce 'domid_policy'
2020-01-16 9:36 ` [Xen-devel] [PATCH v3 5/6] xl.conf: introduce 'domid_policy' Paul Durrant
@ 2020-01-16 18:37 ` Ian Jackson
0 siblings, 0 replies; 24+ messages in thread
From: Ian Jackson @ 2020-01-16 18:37 UTC (permalink / raw)
To: Paul Durrant; +Cc: xen-devel, Wei Liu
Paul Durrant writes ("[PATCH v3 5/6] xl.conf: introduce 'domid_policy'"):
> This patch adds a new global 'domid_policy' configuration option to decide
> how domain id values are allocated for new domains. It may be set to one of
> two values:
>
> "xen", the default value, will cause an invalid domid value to be passed
> to do_domain_create() preserving the existing behaviour of having Xen
> choose the domid value during domain_create().
>
> "random" will cause the special RANDOM_DOMID value to be passed to
> do_domain_create() such that libxl__domain_make() will select a random
> domid value.
This interface LGTM. It provides space for extension.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Xen-devel] [PATCH v3 6/6] xl: allow domid to be preserved on save/restore or migrate
2020-01-16 9:35 [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Paul Durrant
` (4 preceding siblings ...)
2020-01-16 9:36 ` [Xen-devel] [PATCH v3 5/6] xl.conf: introduce 'domid_policy' Paul Durrant
@ 2020-01-16 9:36 ` Paul Durrant
2020-01-16 18:39 ` Ian Jackson
2020-01-16 18:43 ` [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Ian Jackson
6 siblings, 1 reply; 24+ messages in thread
From: Paul Durrant @ 2020-01-16 9:36 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Ian Jackson, Wei Liu
This patch adds a '-D' command line option to save and migrate to allow
the domain id to be incorporated into the saved domain configuration and
hence be preserved.
Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
v2:
- Heavily re-worked based on new libxl_domain_create_info
---
docs/man/xl.1.pod.in | 14 ++++++++++++++
tools/xl/xl.h | 1 +
tools/xl/xl_cmdtable.c | 6 ++++--
tools/xl/xl_migrate.c | 15 ++++++++++-----
tools/xl/xl_saverestore.c | 19 ++++++++++++++-----
tools/xl/xl_vmcontrol.c | 3 ++-
6 files changed, 45 insertions(+), 13 deletions(-)
diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index d4b5e8e362..937eda690f 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -490,6 +490,13 @@ Display huge (!) amount of debug information during the migration process.
Leave the domain on the receive side paused after migration.
+=item B<-D>
+
+Preserve the B<domain-id> in the domain coniguration that is transferred
+such that it will be identical on the destination host, unless that
+configuration is overridden using the B<-C> option. Note that it is not
+possible to use this option for a 'localhost' migration.
+
=back
=item B<remus> [I<OPTIONS>] I<domain-id> I<host>
@@ -692,6 +699,13 @@ Leave the domain running after creating the snapshot.
Leave the domain paused after creating the snapshot.
+=item B<-D>
+
+Preserve the B<domain-id> in the domain coniguration that is embedded in
+the state file such that it will be identical when the domain is restored,
+unless that configuration is overridden. (See the B<restore> operation
+above).
+
=back
=item B<sharing> [I<domain-id>]
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 2b4709efb2..06569c6c4a 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -99,6 +99,7 @@ struct save_file_header {
#define SAVEFILE_BYTEORDER_VALUE ((uint32_t)0x01020304UL)
void save_domain_core_begin(uint32_t domid,
+ int preserve_domid,
const char *override_config_file,
uint8_t **config_data_r,
int *config_len_r);
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 3b302b2f20..08335394e5 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -153,7 +153,8 @@ struct cmd_spec cmd_table[] = {
"[options] <Domain> <CheckpointFile> [<ConfigFile>]",
"-h Print this help.\n"
"-c Leave domain running after creating the snapshot.\n"
- "-p Leave domain paused after creating the snapshot."
+ "-p Leave domain paused after creating the snapshot.\n"
+ "-D Store the domain id in the configration."
},
{ "migrate",
&main_migrate, 0, 1,
@@ -167,7 +168,8 @@ struct cmd_spec cmd_table[] = {
"-e Do not wait in the background (on <host>) for the death\n"
" of the domain.\n"
"--debug Print huge (!) amount of debug during the migration process.\n"
- "-p Do not unpause domain after migrating it."
+ "-p Do not unpause domain after migrating it.\n"
+ "-D Preserve the domain id"
},
{ "restore",
&main_restore, 0, 1,
diff --git a/tools/xl/xl_migrate.c b/tools/xl/xl_migrate.c
index 22f0429b84..0813beb801 100644
--- a/tools/xl/xl_migrate.c
+++ b/tools/xl/xl_migrate.c
@@ -176,7 +176,8 @@ static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
}
-static void migrate_domain(uint32_t domid, const char *rune, int debug,
+static void migrate_domain(uint32_t domid, int preserve_domid,
+ const char *rune, int debug,
const char *override_config_file)
{
pid_t child = -1;
@@ -187,7 +188,7 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug,
uint8_t *config_data;
int config_len, flags = LIBXL_SUSPEND_LIVE;
- save_domain_core_begin(domid, override_config_file,
+ save_domain_core_begin(domid, preserve_domid, override_config_file,
&config_data, &config_len);
if (!config_len) {
@@ -537,13 +538,14 @@ int main_migrate(int argc, char **argv)
char *rune = NULL;
char *host;
int opt, daemonize = 1, monitor = 1, debug = 0, pause_after_migration = 0;
+ int preserve_domid = 0;
static struct option opts[] = {
{"debug", 0, 0, 0x100},
{"live", 0, 0, 0x200},
COMMON_LONG_OPTS
};
- SWITCH_FOREACH_OPT(opt, "FC:s:ep", opts, "migrate", 2) {
+ SWITCH_FOREACH_OPT(opt, "FC:s:epD", opts, "migrate", 2) {
case 'C':
config_filename = optarg;
break;
@@ -560,6 +562,9 @@ int main_migrate(int argc, char **argv)
case 'p':
pause_after_migration = 1;
break;
+ case 'D':
+ preserve_domid = 1;
+ break;
case 0x100: /* --debug */
debug = 1;
break;
@@ -596,7 +601,7 @@ int main_migrate(int argc, char **argv)
pause_after_migration ? " -p" : "");
}
- migrate_domain(domid, rune, debug, config_filename);
+ migrate_domain(domid, preserve_domid, rune, debug, config_filename);
return EXIT_SUCCESS;
}
@@ -716,7 +721,7 @@ int main_remus(int argc, char **argv)
}
}
- save_domain_core_begin(domid, NULL, &config_data, &config_len);
+ save_domain_core_begin(domid, 0, NULL, &config_data, &config_len);
if (!config_len) {
fprintf(stderr, "No config file stored for running domain and "
diff --git a/tools/xl/xl_saverestore.c b/tools/xl/xl_saverestore.c
index 9be033fe65..953d791d1a 100644
--- a/tools/xl/xl_saverestore.c
+++ b/tools/xl/xl_saverestore.c
@@ -32,6 +32,7 @@
#ifndef LIBXL_HAVE_NO_SUSPEND_RESUME
void save_domain_core_begin(uint32_t domid,
+ int preserve_domid,
const char *override_config_file,
uint8_t **config_data_r,
int *config_len_r)
@@ -62,6 +63,8 @@ void save_domain_core_begin(uint32_t domid,
fprintf(stderr, "unable to retrieve domain configuration\n");
exit(EXIT_FAILURE);
}
+
+ d_config.c_info.domid = preserve_domid ? domid : 0;
}
config_c = libxl_domain_config_to_json(ctx, &d_config);
@@ -120,14 +123,15 @@ void save_domain_core_writeconfig(int fd, const char *source,
hdr.optional_data_len);
}
-static int save_domain(uint32_t domid, const char *filename, int checkpoint,
- int leavepaused, const char *override_config_file)
+static int save_domain(uint32_t domid, int preserve_domid,
+ const char *filename, int checkpoint,
+ int leavepaused, const char *override_config_file)
{
int fd;
uint8_t *config_data;
int config_len;
- save_domain_core_begin(domid, override_config_file,
+ save_domain_core_begin(domid, preserve_domid, override_config_file,
&config_data, &config_len);
if (!config_len) {
@@ -236,15 +240,19 @@ int main_save(int argc, char **argv)
const char *config_filename = NULL;
int checkpoint = 0;
int leavepaused = 0;
+ int preserve_domid = 0;
int opt;
- SWITCH_FOREACH_OPT(opt, "cp", NULL, "save", 2) {
+ SWITCH_FOREACH_OPT(opt, "cpD", NULL, "save", 2) {
case 'c':
checkpoint = 1;
break;
case 'p':
leavepaused = 1;
break;
+ case 'D':
+ preserve_domid = 1;
+ break;
}
if (argc-optind > 3) {
@@ -257,7 +265,8 @@ int main_save(int argc, char **argv)
if ( argc - optind >= 3 )
config_filename = argv[optind + 2];
- save_domain(domid, filename, checkpoint, leavepaused, config_filename);
+ save_domain(domid, preserve_domid, filename, checkpoint, leavepaused,
+ config_filename);
return EXIT_SUCCESS;
}
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index 39292acfe6..2e2d427492 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -899,7 +899,8 @@ start:
autoconnect_console_how = 0;
}
- d_config.c_info.domid = domid_policy;
+ if (!libxl_domid_valid_guest(d_config.c_info.domid))
+ d_config.c_info.domid = domid_policy;
if ( restoring ) {
libxl_domain_restore_params params;
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes
2020-01-16 9:35 [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Paul Durrant
` (5 preceding siblings ...)
2020-01-16 9:36 ` [Xen-devel] [PATCH v3 6/6] xl: allow domid to be preserved on save/restore or migrate Paul Durrant
@ 2020-01-16 18:43 ` Ian Jackson
2020-01-17 9:11 ` Durrant, Paul
6 siblings, 1 reply; 24+ messages in thread
From: Ian Jackson @ 2020-01-16 18:43 UTC (permalink / raw)
To: Paul Durrant
Cc: Stefano Stabellini, Julien Grall, Wei Liu, jandryuk,
Andrew Cooper, Konrad Rzeszutek Wilk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
Paul Durrant writes ("[PATCH v3 0/6] xl/libxl: domid allocation/preservation changes"):
> This series was previously named "xl/libxl: allow creation of domains with
> a specified domid".
Thanks. I think Anthony ought to have been made a maintainer of
tools/xl at the same time as of tools/libxl. But that isn't so in
MAINTAINERS right now, so he wasn't CC'd on all these patches. If you
could fix that up manually for future mails, that would be great.
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes
2020-01-16 18:43 ` [Xen-devel] [PATCH v3 0/6] xl/libxl: domid allocation/preservation changes Ian Jackson
@ 2020-01-17 9:11 ` Durrant, Paul
0 siblings, 0 replies; 24+ messages in thread
From: Durrant, Paul @ 2020-01-17 9:11 UTC (permalink / raw)
To: Ian Jackson
Cc: Stefano Stabellini, Julien Grall, Wei Liu, jandryuk,
Andrew Cooper, Konrad Rzeszutek Wilk, George Dunlap, Jan Beulich,
Anthony Perard, xen-devel
> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 16 January 2020 19:43
> To: Durrant, Paul <pdurrant@amazon.co.uk>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Anthony Perard <anthony.perard@citrix.com>;
> George Dunlap <George.Dunlap@citrix.com>; Jan Beulich <jbeulich@suse.com>;
> jandryuk@gmail.com; Julien Grall <julien@xen.org>; Konrad Rzeszutek Wilk
> <konrad.wilk@oracle.com>; Stefano Stabellini <sstabellini@kernel.org>; Wei
> Liu <wl@xen.org>
> Subject: Re: [PATCH v3 0/6] xl/libxl: domid allocation/preservation
> changes
>
> Paul Durrant writes ("[PATCH v3 0/6] xl/libxl: domid
> allocation/preservation changes"):
> > This series was previously named "xl/libxl: allow creation of domains
> with
> > a specified domid".
>
> Thanks. I think Anthony ought to have been made a maintainer of
> tools/xl at the same time as of tools/libxl. But that isn't so in
> MAINTAINERS right now, so he wasn't CC'd on all these patches. If you
> could fix that up manually for future mails, that would be great.
>
Ok, I'll re-base on top of the patch you posted. That should do the trick.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 24+ messages in thread