[Xen-devel] [PATCH 0/6] xl/libxl: allow creation of domains with a specified domid

[Xen-devel] [PATCH 0/6] xl/libxl: allow creation of domains with a specified domid

[Paul Durrant]

Paul Durrant (6):
  libxl: add definition of INVALID_DOMID to the API
  libxl_create: make 'soft reset' explicit
  domctl: return EEXIST from XEN_DOMCTL_createdomain...
  domctl: set XEN_DOMCTL_createdomain 'rover' if valid domid is
    specified
  libxl: allow creation of domains with specified or random domid
  xl: allow specified domain id to be used for create, restore and
    migrate

 docs/man/xl.1.pod.in         | 34 +++++++++++--
 tools/libxl/libxl.h          | 16 +++++-
 tools/libxl/libxl_create.c   | 95 +++++++++++++++++++++++++-----------
 tools/libxl/libxl_dm.c       |  2 +-
 tools/libxl/libxl_internal.c |  2 +-
 tools/libxl/libxl_internal.h |  6 +--
 tools/xl/xl.h                |  1 +
 tools/xl/xl_cmdtable.c       | 22 +++++++--
 tools/xl/xl_migrate.c        | 42 ++++++++++++----
 tools/xl/xl_parse.c          | 33 +++++++++++++
 tools/xl/xl_parse.h          |  2 +
 tools/xl/xl_saverestore.c    |  9 +++-
 tools/xl/xl_utils.h          |  2 -
 tools/xl/xl_vmcontrol.c      | 23 ++++++---
 xen/common/domctl.c          |  6 +--
 15 files changed, 230 insertions(+), 65 deletions(-)
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@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: Wei Liu <wl@xen.org>
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/6] libxl: add definition of INVALID_DOMID to the API

[Paul Durrant]

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>
---
Cc: 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 b5adbfe4b7..a15778952c 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

Re: [Xen-devel] [PATCH 1/6] libxl: add definition of INVALID_DOMID to the API

[Ian Jackson]

Paul Durrant writes ("[PATCH 1/6] libxl: add definition of INVALID_DOMID to the API"):
> 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.

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

[Xen-devel] [PATCH 2/6] libxl_create: make 'soft reset' explicit

[Paul Durrant]

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>
---
Cc: 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 dac1b8ddb8..ac7806604a 100644
--- a/tools/libxl/libxl_dm.c
+++ b/tools/libxl/libxl_dm.c
@@ -2190,7 +2190,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 a15778952c..15f167a6c4 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,
@@ -4135,7 +4135,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

Re: [Xen-devel] [PATCH 2/6] libxl_create: make 'soft reset' explicit

[Ian Jackson]

Paul Durrant writes ("[PATCH 2/6] libxl_create: make 'soft reset' explicit"):
> 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'.

Thanks, nice work.  (In general my preference would be to split stuff
like that last bit out into a pre-patch but this is a trivial
observation.)

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

[Xen-devel] [PATCH 3/6] domctl: return EEXIST from XEN_DOMCTL_createdomain...

[Paul Durrant]

...if a specified domid is already in use.

XEN_DOMCTL_createdomain allows a domid to be specified by its caller and
will correctly fail if that domid is already in use. However the errno
returned in this case will be EINVAL, making it indistinguishable from
several other failures. Also a value of EINVAL does not seem appropriate
as the specified domid is valid [1] but just not (transiently) available.

[1] any invalid value passed in is ignored and causes Xen to choose an
    unused and valid value.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@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: Wei Liu <wl@xen.org>
---
 xen/common/domctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 03d0226039..650310e874 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -504,7 +504,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         dom = op->domain;
         if ( (dom > 0) && (dom < DOMID_FIRST_RESERVED) )
         {
-            ret = -EINVAL;
+            ret = -EEXIST;
             if ( !is_free_domid(dom) )
                 break;
         }
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 3/6] domctl: return EEXIST from XEN_DOMCTL_createdomain...

[Ian Jackson]

Paul Durrant writes ("[PATCH 3/6] domctl: return EEXIST from XEN_DOMCTL_createdomain..."):
> ...if a specified domid is already in use.
> 
> XEN_DOMCTL_createdomain allows a domid to be specified by its caller and
> will correctly fail if that domid is already in use. However the errno
> returned in this case will be EINVAL, making it indistinguishable from
> several other failures. Also a value of EINVAL does not seem appropriate
> as the specified domid is valid [1] but just not (transiently) available.
> 
> [1] any invalid value passed in is ignored and causes Xen to choose an
>     unused and valid value.

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

[Xen-devel] [PATCH 4/6] domctl: set XEN_DOMCTL_createdomain 'rover' if valid domid is specified

[Paul Durrant]

The value of 'rover' is the value at which Xen will start searching for an
unused domid if none is specified. Currently it is only updated when a
domid is automatically chosen, rather than specified by the caller, which
makes it very hard to describe Xen's rationale in choosing domids in an
environment where some domain creations have specified domids and some
don't.
This patch always updates 'rover' after a successful creation, even in the
case that domid is specified by the caller. This ensures that, if Xen
automatically chooses a domid for a subsequent domain creation it will
always be the next available value after the domid of the most recently
created domain.

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@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: Wei Liu <wl@xen.org>
---
 xen/common/domctl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 650310e874..5268f3967b 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -521,8 +521,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
             ret = -ENOMEM;
             if ( dom == rover )
                 break;
-
-            rover = dom;
         }
 
         d = domain_create(dom, &op->u.createdomain, false);
@@ -534,7 +532,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
         }
 
         ret = 0;
-        op->domain = d->domain_id;
+        rover = op->domain = d->domain_id;
         copyback = 1;
         d = NULL;
         break;
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/6] domctl: set XEN_DOMCTL_createdomain 'rover' if valid domid is specified

[Ian Jackson]

Paul Durrant writes ("[PATCH 4/6] domctl: set XEN_DOMCTL_createdomain 'rover' if valid domid is specified"):
> The value of 'rover' is the value at which Xen will start searching for an
> unused domid if none is specified. Currently it is only updated when a
> domid is automatically chosen, rather than specified by the caller, which
> makes it very hard to describe Xen's rationale in choosing domids in an
> environment where some domain creations have specified domids and some
> don't.
> This patch always updates 'rover' after a successful creation, even in the
> case that domid is specified by the caller. This ensures that, if Xen
> automatically chooses a domid for a subsequent domain creation it will
> always be the next available value after the domid of the most recently
> created domain.

I'm not yet convinced this behaviour is better, but I'm open to
persuasion.

The existing allocation system falls down in any case if the domid
gets near to wrapping round.  If it doesn't, then without this patch
it is possible to have two ranges of domids: automatically allocated
ones, and statically allocated high ones.

With this patch, statically allocating a domid resets rover and causes
the remaining bits of static space to be polluted.

What am I missing ?  What are the use cases here ?

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/6] domctl: set XEN_DOMCTL_createdomain 'rover' if valid domid is specified

[Durrant, Paul]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 02 January 2020 17:25
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: xen-devel@lists.xenproject.org; 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>; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH 4/6] domctl: set XEN_DOMCTL_createdomain 'rover' if
> valid domid is specified
> 
> Paul Durrant writes ("[PATCH 4/6] domctl: set XEN_DOMCTL_createdomain
> 'rover' if valid domid is specified"):
> > The value of 'rover' is the value at which Xen will start searching for
> an
> > unused domid if none is specified. Currently it is only updated when a
> > domid is automatically chosen, rather than specified by the caller,
> which
> > makes it very hard to describe Xen's rationale in choosing domids in an
> > environment where some domain creations have specified domids and some
> > don't.
> > This patch always updates 'rover' after a successful creation, even in
> the
> > case that domid is specified by the caller. This ensures that, if Xen
> > automatically chooses a domid for a subsequent domain creation it will
> > always be the next available value after the domid of the most recently
> > created domain.
> 
> I'm not yet convinced this behaviour is better, but I'm open to
> persuasion.
> 
> The existing allocation system falls down in any case if the domid
> gets near to wrapping round.  If it doesn't, then without this patch
> it is possible to have two ranges of domids: automatically allocated
> ones, and statically allocated high ones.
> 
> With this patch, statically allocating a domid resets rover and causes
> the remaining bits of static space to be polluted.
> 
> What am I missing ?  What are the use cases here ?

The problem I was tackling was trying to document how Xen chooses a domid. E.g. it is not correct to say that it chooses the lowest numbered available domid. The best I could come up with was 'the next available domid after the last one it thought of' which is pretty poor... and also becomes harder to explain if some domids are not actually chosen by Xen, because the toolstack specified them.

The end game which this series is working towards is transparent migration which, because of the 'design' of PV protocols and certain hypercalls in the guest ABI, requires that the domid is persisted on migration. This patch is simply trying to lay groundwork for the co-existence of domains with specified domids and those with automatically allocated domids. 

  Paul

> 
> Thanks,
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 4/6] domctl: set XEN_DOMCTL_createdomain 'rover' if valid domid is specified

[Andrew Cooper]

On 24/12/2019 13:04, Paul Durrant wrote:
> The value of 'rover' is the value at which Xen will start searching for an
> unused domid if none is specified. Currently it is only updated when a
> domid is automatically chosen, rather than specified by the caller, which
> makes it very hard to describe Xen's rationale in choosing domids in an
> environment where some domain creations have specified domids and some
> don't.
> This patch always updates 'rover' after a successful creation, even in the
> case that domid is specified by the caller. This ensures that, if Xen
> automatically chooses a domid for a subsequent domain creation it will
> always be the next available value after the domid of the most recently
> created domain.
>
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>

This behaviour is deliberate, if not obvious.

Lots of things break when domid's overflow.  The current behaviour means
that an overflow won't occur until all domids have been used.

Domid reuse is a massive swamp which someone is going to fix at some
point...

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 5/6] libxl: allow creation of domains with specified or random domid

[Paul Durrant]

This patch modifies do_domain_create() to use the value of domid that is
passed in. A new 'special value' - RANDOM_DOMID - is added into the API
and this, INVALID_DOMID or any valid domid is passed unmodified to
libxl__domain_make(). Any other invalid domid value will cause an error.

If RANDOM_DOMID is passed in then libxl__domain_make() will use
libxl__random_bytes() to choose a domid. If the chosen value is not a
valid domid then this step will be repeated. Once a valid value is chosen
it will be passed to xc_domain_create() but if this fails with an errno
value of EEXIST, a new random value will be chosen and the operation will
be retried.

If a valid domid is passed in and xc_domain_create() fails with errno
set to EEXIST then this will result in a new error value of
ERROR_DEVICE_EXISTS being returned from libxl__domain_make(). This is
done so that domcreate_complete() can be adjusted so as not to tear down
the existing domain that the attempted creation happened to collide with.

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>
---
 tools/libxl/libxl.h          | 12 ++++++++++
 tools/libxl/libxl_create.c   | 43 +++++++++++++++++++++++++++++-------
 tools/libxl/libxl_internal.c |  2 +-
 3 files changed, 48 insertions(+), 9 deletions(-)

diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 18c1a2d6bf..6e7f5a0241 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_SPECIFIED_DOMID
+ *
+ * libxl_domain_create_new() and libxl_domain_create_restore() will use
+ * a caller specified domid value.
+ */
+#define LIBXL_HAVE_SPECIFIED_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,7 +1536,11 @@ int libxl_ctx_free(libxl_ctx *ctx /* 0 is OK */);
 /* domain related functions */
 
 #define INVALID_DOMID ~0
+#define RANDOM_DOMID  (INVALID_DOMID - 1)
 
+/* On entry a domid == RANDOM_DOMID, a valid random domain id will be
+ * chosen, otherwise if domid is a valid value then that will be used as
+ * the domain id */
 /* 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 INVALID_DOMID, as appropriate */
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1835a5502c..1d98567f59 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -555,8 +555,6 @@ 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;
@@ -571,6 +569,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
             .max_grant_frames = b_info->max_grant_frames,
             .max_maptrack_frames = b_info->max_maptrack_frames,
         };
+        uint32_t in_domid = *domid;
 
         if (info->type != LIBXL_DOMAIN_TYPE_PV) {
             create.flags |= XEN_DOMCTL_CDF_hvm;
@@ -600,10 +599,24 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
             goto out;
         }
 
-        ret = xc_domain_create(ctx->xch, domid, &create);
+        for (;;) {
+            if (in_domid == RANDOM_DOMID) {
+                ret = libxl__random_bytes(gc, (void *)domid, sizeof(*domid));
+                if (ret < 0)
+                    break;
+
+                if (!libxl_domid_valid_guest(*domid))
+                    continue;
+            }
+
+            ret = xc_domain_create(ctx->xch, domid, &create);
+            if (ret == 0 || errno != EEXIST || in_domid != RANDOM_DOMID)
+                break;
+        }
+
         if (ret < 0) {
-            LOGED(ERROR, *domid, "domain creation fail");
-            rc = ERROR_FAIL;
+            LOGED(ERROR, in_domid, "domain creation fail");
+            rc = (errno == EEXIST) ? ERROR_DEVICE_EXISTS : ERROR_FAIL;
             goto out;
         }
 
@@ -1111,7 +1124,6 @@ static void initiate_domain_create(libxl__egc *egc,
     if (ret) {
         LOGD(ERROR, domid, "cannot make domain: %d", ret);
         dcs->guest_domid = domid;
-        ret = ERROR_FAIL;
         goto error_out;
     }
 
@@ -1752,7 +1764,8 @@ static void domcreate_complete(libxl__egc *egc,
     if (!rc && d_config->b_info.exec_ssidref)
         rc = xc_flask_relabel_domain(CTX->xch, dcs->guest_domid, d_config->b_info.exec_ssidref);
 
-    bool retain_domain = !rc || rc == ERROR_ABORTED;
+    bool retain_domain = !rc || rc == ERROR_ABORTED ||
+        rc == ERROR_DEVICE_EXISTS;
 
     if (retain_domain) {
         libxl__domain_userdata_lock *lock;
@@ -1845,7 +1858,21 @@ 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 = INVALID_DOMID;
+
+    /* Allow valid and special values */
+    switch (*domid) {
+    case INVALID_DOMID:
+    case RANDOM_DOMID:
+        break;
+    default:
+        if (libxl_domid_valid_guest(*domid))
+            break;
+
+        rc = ERROR_FAIL;
+        goto out_err;
+    }
+
+    cdcs->dcs.domid = *domid;
     cdcs->dcs.soft_reset = false;
 
     if (cdcs->dcs.restore_params.checkpointed_stream ==
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",
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/6] libxl: allow creation of domains with specified or random domid

[Ian Jackson]

Paul Durrant writes ("[PATCH 5/6] libxl: allow creation of domains with specified or random domid"):
> This patch modifies do_domain_create() to use the value of domid that is
> passed in. A new 'special value' - RANDOM_DOMID - is added into the API
> and this, INVALID_DOMID or any valid domid is passed unmodified to
> libxl__domain_make(). Any other invalid domid value will cause an error.
> 
> If RANDOM_DOMID is passed in then libxl__domain_make() will use
> libxl__random_bytes() to choose a domid. If the chosen value is not a
> valid domid then this step will be repeated. Once a valid value is chosen
> it will be passed to xc_domain_create() but if this fails with an errno
> value of EEXIST, a new random value will be chosen and the operation will
> be retried.

What is the use case for this ?

I think using this is hazardous if you ever destroy domains, because
it will lead to reuse of domids in circumstances[1] where right now
that can't happen.

[1] Fewer than ~2^16 creations per Xen boot.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/6] libxl: allow creation of domains with specified or random domid

[Durrant, Paul]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 02 January 2020 17:27
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>; Anthony Perard
> <anthony.perard@citrix.com>
> Subject: Re: [PATCH 5/6] libxl: allow creation of domains with specified
> or random domid
> 
> Paul Durrant writes ("[PATCH 5/6] libxl: allow creation of domains with
> specified or random domid"):
> > This patch modifies do_domain_create() to use the value of domid that is
> > passed in. A new 'special value' - RANDOM_DOMID - is added into the API
> > and this, INVALID_DOMID or any valid domid is passed unmodified to
> > libxl__domain_make(). Any other invalid domid value will cause an error.
> >
> > If RANDOM_DOMID is passed in then libxl__domain_make() will use
> > libxl__random_bytes() to choose a domid. If the chosen value is not a
> > valid domid then this step will be repeated. Once a valid value is
> chosen
> > it will be passed to xc_domain_create() but if this fails with an errno
> > value of EEXIST, a new random value will be chosen and the operation
> will
> > be retried.
> 
> What is the use case for this ?
> 

The use-case is trying to make sure that, across a pool of hosts, the likelihood of creating two domains with the same domid is small. Because a transparent migration requires that domid is persisted, migrations are more likely to fail (due to a domid already being in use) if a sequential scheme is used.

> I think using this is hazardous if you ever destroy domains, because
> it will lead to reuse of domids in circumstances[1] where right now
> that can't happen.
> 
> [1] Fewer than ~2^16 creations per Xen boot.

Agreed, but is that actually a problem? A domain must be fully destroyed (i.e. all page refs dropped) before its id will become available for re-use. What are we actually trying to avoid by not immediately recycling? I'd certainly be open to adding some sort of retirement list for domids that would prevent re-use within a specified time, if that would help.

  Paul

> 
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 5/6] libxl: allow creation of domains with specified or random domid

[Jason Andryuk]

On Tue, Dec 24, 2019 at 8:06 AM Paul Durrant <pdurrant@amazon.com> wrote:
>
> This patch modifies do_domain_create() to use the value of domid that is
> passed in. A new 'special value' - RANDOM_DOMID - is added into the API
> and this, INVALID_DOMID or any valid domid is passed unmodified to
> libxl__domain_make(). Any other invalid domid value will cause an error.
>
> If RANDOM_DOMID is passed in then libxl__domain_make() will use
> libxl__random_bytes() to choose a domid. If the chosen value is not a
> valid domid then this step will be repeated. Once a valid value is chosen
> it will be passed to xc_domain_create() but if this fails with an errno
> value of EEXIST, a new random value will be chosen and the operation will
> be retried.
>
> If a valid domid is passed in and xc_domain_create() fails with errno
> set to EEXIST then this will result in a new error value of
> ERROR_DEVICE_EXISTS being returned from libxl__domain_make(). This is
> done so that domcreate_complete() can be adjusted so as not to tear down
> the existing domain that the attempted creation happened to collide with.
>
> 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>
> ---

<snip>

> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c

<snip>

> @@ -571,6 +569,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>              .max_grant_frames = b_info->max_grant_frames,
>              .max_maptrack_frames = b_info->max_maptrack_frames,
>          };
> +        uint32_t in_domid = *domid;
>
>          if (info->type != LIBXL_DOMAIN_TYPE_PV) {
>              create.flags |= XEN_DOMCTL_CDF_hvm;
> @@ -600,10 +599,24 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>              goto out;
>          }
>
> -        ret = xc_domain_create(ctx->xch, domid, &create);
> +        for (;;) {
> +            if (in_domid == RANDOM_DOMID) {
> +                ret = libxl__random_bytes(gc, (void *)domid, sizeof(*domid));

Since valid domids are ~0-2^15, you may want to used a temporary
uint16_t instead of the uint32_t domid to tighten up the range.

Regards,
Jason

> +                if (ret < 0)
> +                    break;
> +
> +                if (!libxl_domid_valid_guest(*domid))
> +                    continue;
> +            }
> +
> +            ret = xc_domain_create(ctx->xch, domid, &create);
> +            if (ret == 0 || errno != EEXIST || in_domid != RANDOM_DOMID)
> +                break;
> +        }
> +

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 6/6] xl: allow specified domain id to be used for create, restore and migrate

[Paul Durrant]

This patch adds the option to use a specified domain id to be used for
the create, restore and migrate lifecycle operations and documentation
thereof.

The specified id may be numeric or, in all cases, one of two special
values. The value 'random' will cause libxl to use a randomly chosen
domain id and the value 'next' will cause Xen to automatically choose
the next available domain id (which is the default and legacy behaviour).
In the case of the migrate operation a third special value may be
specified: 'preserve'. If this value is chosen then the current id of
the domain being migrated will be used to restore the domain on the
destination host (which clearly precludes 'localhost' migrations).

NOTE: Whilst modifing xl_cmdtable.c, several formatting errors were
      corrected. Also erroneous documentation of the '-f' option in
      xl.1.pod.in was corrected (to remove the '=').

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wl@xen.org>
---
 docs/man/xl.1.pod.in      | 34 +++++++++++++++++++++++++++----
 tools/xl/xl.h             |  1 +
 tools/xl/xl_cmdtable.c    | 22 +++++++++++++++-----
 tools/xl/xl_migrate.c     | 42 ++++++++++++++++++++++++++++++---------
 tools/xl/xl_parse.c       | 33 ++++++++++++++++++++++++++++++
 tools/xl/xl_parse.h       |  2 ++
 tools/xl/xl_saverestore.c |  9 ++++++++-
 tools/xl/xl_vmcontrol.c   | 23 +++++++++++++++------
 8 files changed, 141 insertions(+), 25 deletions(-)

diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index d4b5e8e362..2def32664b 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -122,11 +122,19 @@ B<OPTIONS>
 
 =over 4
 
+=item B<-D DOMID>, B<--domid=DOMID>
+
+If DOMID is numeric then create the new domain with this domain id. If
+DOMID is I<random> then use a randomly generated value for domain id
+otherwise, if DOMID is I<next> (the default value for this option) then
+use the next available domain id following on from the previous domain to
+be created, restored or migrated in.
+
 =item B<-q>, B<--quiet>
 
 No console output.
 
-=item B<-f=FILE>, B<--defconfig=FILE>
+=item B<-f FILE>, B<--defconfig=FILE>
 
 Use the given configuration file.
 
@@ -205,8 +213,7 @@ B<OPTIONS>
 
 =over 4
 
-=item B<-f=FILE>, B<--defconfig=FILE>
-
+=item B<-f FILE>, B<--defconfig=FILE>
 Use the given configuration file.
 
 =item B<key=value>
@@ -467,6 +474,17 @@ B<OPTIONS>
 
 =over 4
 
+=item B<-D DOMID>, B<--domid=DOMID>
+
+If DOMID is numeric then create the migrated domain with this domain id. If
+DOMID is I<preserve> then use the same numeric domain id as the domain
+being migrated has on the current host. Note that migration will fail in
+the case that a specified or preserved domain id is already in use on the
+destination host. If DOMID is I<random> then use a randomly generated
+value for domain id otherwise, if DOMID is I<next> (the default value for
+this option) then use the next available domain id following on from the
+previous domain to be created, restored or migrated in.
+
 =item B<-s> I<sshcommand>
 
 Use <sshcommand> instead of ssh.  String will be passed to sh. If empty, run
@@ -648,6 +666,14 @@ B<OPTIONS>
 
 =over 4
 
+=item B<-D DOMID>, B<--domid=DOMID>
+
+If DOMID is numeric then create the restored domain with this domain id. If
+DOMID is I<random> then use a randomly generated value for domain id
+otherwise, if DOMID is I<next> (the default value for this option) then
+use the next available domain id following on from the previous domain to
+be created, restored or migrated in.
+
 =item B<-p>
 
 Do not unpause the domain after restoring it.
@@ -1287,7 +1313,7 @@ B<OPTIONS>
 
 =over 4
 
-=item B<-f=FILE>, B<--defconfig=FILE>
+=item B<-f FILE>, B<--defconfig=FILE>
 
 Use the given configuration file.
 
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 60bdad8ffb..f2500f36e0 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -31,6 +31,7 @@ struct cmd_spec {
 };
 
 struct domain_create {
+    int domid;
     int debug;
     int daemonize;
     int monitor; /* handle guest reboots etc */
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 5baa6023aa..b244b6a243 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -26,17 +26,22 @@ struct cmd_spec cmd_table[] = {
       "-h                      Print this help.\n"
       "-p                      Leave the domain paused after it is created.\n"
       "-c                      Connect to the console after the domain is created.\n"
-      "-f FILE, --defconfig=FILE\n                     Use the given configuration file.\n"
+      "-f FILE, --defconfig=FILE\n"
+      "                        Use the given configuration file.\n"
       "-q, --quiet             Quiet.\n"
       "-n, --dryrun            Dry run - prints the resulting configuration\n"
-      "                         (deprecated in favour of global -N option).\n"
+      "                        (deprecated in favour of global -N option).\n"
       "-d                      Enable debug messages.\n"
       "-F                      Run in foreground until death of the domain.\n"
       "-e                      Do not wait in the background for the death of the domain.\n"
       "-V, --vncviewer         Connect to the VNC display after the domain is created.\n"
       "-A, --vncviewer-autopass\n"
       "                        Pass VNC password to viewer via stdin.\n"
-      "--ignore-global-affinity-masks Ignore global masks in xl.conf."
+      "--ignore-global-affinity-masks\n"
+      "                        Ignore global masks in xl.conf.\n"
+      "-D, --domid=DOMID|next|random\n"
+      "                        Use the specified domain id, the next available (default)\n"
+      "                        or choose one at random."
     },
     { "config-update",
       &main_config_update, 1, 1,
@@ -167,7 +172,11 @@ 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, --domid=DOMID|next|random|preserve\n"
+      "                Use the specified domain id, the next available (default),\n"
+      "                choose one at random, or preserve the existing domain's id\n"
+      "                (hence precluding localhost migrate)."
     },
     { "restore",
       &main_restore, 0, 1,
@@ -178,7 +187,10 @@ struct cmd_spec cmd_table[] = {
       "-e                       Do not wait in the background for the death of the domain.\n"
       "-d                       Enable debug messages.\n"
       "-V, --vncviewer          Connect to the VNC display after the domain is created.\n"
-      "-A, --vncviewer-autopass Pass VNC password to viewer via stdin."
+      "-A, --vncviewer-autopass Pass VNC password to viewer via stdin.\n"
+      "-D, --domid=DOMID|next|random\n"
+      "                         Use the specified domain id, the next available (default)\n"
+      "                         or choose one at random."
     },
     { "migrate-receive",
       &main_migrate_receive, 0, 1,
diff --git a/tools/xl/xl_migrate.c b/tools/xl/xl_migrate.c
index 22f0429b84..b0d8f12d95 100644
--- a/tools/xl/xl_migrate.c
+++ b/tools/xl/xl_migrate.c
@@ -315,14 +315,13 @@ static void migrate_domain(uint32_t domid, const char *rune, int debug,
     exit(EXIT_FAILURE);
 }
 
-static void migrate_receive(int debug, int daemonize, int monitor,
-                            int pause_after_migration,
+static void migrate_receive(int domid, int debug, int daemonize,
+                            int monitor, int pause_after_migration,
                             int send_fd, int recv_fd,
                             libxl_checkpointed_stream checkpointed,
                             char *colo_proxy_script,
                             bool userspace_colo_proxy)
 {
-    uint32_t domid;
     int rc, rc2;
     char rc_buf;
     char *migration_domname;
@@ -339,6 +338,7 @@ static void migrate_receive(int debug, int daemonize, int monitor,
                      "migration ack stream", "banner") );
 
     memset(&dom_info, 0, sizeof(dom_info));
+    dom_info.domid = domid;
     dom_info.debug = debug;
     dom_info.daemonize = daemonize;
     dom_info.monitor = monitor;
@@ -477,6 +477,7 @@ static void migrate_receive(int debug, int daemonize, int monitor,
 
 int main_migrate_receive(int argc, char **argv)
 {
+    const char *domid = NULL;
     int debug = 0, daemonize = 1, monitor = 1, pause_after_migration = 0;
     libxl_checkpointed_stream checkpointed = LIBXL_CHECKPOINTED_STREAM_NONE;
     int opt;
@@ -490,7 +491,10 @@ int main_migrate_receive(int argc, char **argv)
         COMMON_LONG_OPTS
     };
 
-    SWITCH_FOREACH_OPT(opt, "Fedrp", opts, "migrate-receive", 0) {
+    SWITCH_FOREACH_OPT(opt, "D:Fedrp", opts, "migrate-receive", 0) {
+    case 'D':
+        domid = optarg;
+        break;
     case 'F':
         daemonize = 0;
         break;
@@ -522,7 +526,9 @@ int main_migrate_receive(int argc, char **argv)
         help("migrate-receive");
         return EXIT_FAILURE;
     }
-    migrate_receive(debug, daemonize, monitor, pause_after_migration,
+
+    migrate_receive(parse_domid(domid), debug, daemonize,
+                    monitor, pause_after_migration,
                     STDOUT_FILENO, STDIN_FILENO,
                     checkpointed, script, userspace_colo_proxy);
 
@@ -531,7 +537,8 @@ int main_migrate_receive(int argc, char **argv)
 
 int main_migrate(int argc, char **argv)
 {
-    uint32_t domid;
+    uint32_t src_domid;
+    const char *dst_domid = NULL;
     const char *config_filename = NULL;
     const char *ssh_command = "ssh";
     char *rune = NULL;
@@ -540,10 +547,14 @@ int main_migrate(int argc, char **argv)
     static struct option opts[] = {
         {"debug", 0, 0, 0x100},
         {"live", 0, 0, 0x200},
+        {"domid", 1, 0, 'D'},
         COMMON_LONG_OPTS
     };
 
-    SWITCH_FOREACH_OPT(opt, "FC:s:ep", opts, "migrate", 2) {
+    SWITCH_FOREACH_OPT(opt, "D:FC:s:ep", opts, "migrate", 2) {
+    case 'D':
+        dst_domid = optarg;
+        break;
     case 'C':
         config_filename = optarg;
         break;
@@ -568,7 +579,7 @@ int main_migrate(int argc, char **argv)
         break;
     }
 
-    domid = find_domain(argv[optind]);
+    src_domid = find_domain(argv[optind]);
     host = argv[optind + 1];
 
     bool pass_tty_arg = progress_use_cr || (isatty(2) > 0);
@@ -578,6 +589,8 @@ int main_migrate(int argc, char **argv)
     } else {
         char verbose_buf[minmsglevel_default+3];
         int verbose_len;
+        char *extra = NULL;
+
         verbose_buf[0] = ' ';
         verbose_buf[1] = '-';
         memset(verbose_buf+2, 'v', minmsglevel_default);
@@ -594,9 +607,20 @@ int main_migrate(int argc, char **argv)
                   daemonize ? "" : " -e",
                   debug ? " -d" : "",
                   pause_after_migration ? " -p" : "");
+
+        if (dst_domid) {
+            if (!strcmp(dst_domid, "preserve"))
+                xasprintf(&extra, " -D %u", src_domid);
+            else
+                xasprintf(&extra, " -D %s", dst_domid);
+        }
+        if (extra) {
+            string_realloc_append(&rune, extra);
+            free(extra);
+        }
     }
 
-    migrate_domain(domid, rune, debug, config_filename);
+    migrate_domain(src_domid, rune, debug, config_filename);
     return EXIT_SUCCESS;
 }
 
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index b881184804..58b1aeea8c 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -399,6 +399,30 @@ static unsigned long parse_ulong(const char *str)
     return val;
 }
 
+static unsigned long parse_long(const char *str)
+{
+    char *endptr;
+    long val;
+
+    val = strtol(str, &endptr, 10);
+    if (endptr == str || val == LONG_MIN || val == LONG_MAX) {
+        fprintf(stderr, "xl: failed to convert \"%s\" to number\n", str);
+        exit(EXIT_FAILURE);
+    }
+    return val;
+}
+
+static int parse_int(const char *str)
+{
+    long val = parse_long(str);
+
+    if (val < INT_MIN || val > INT_MAX) {
+        fprintf(stderr, "xl: \"%s\" is out of range\n", str);
+        exit(EXIT_FAILURE);
+    }
+    return val;
+}
+
 void replace_string(char **str, const char *val)
 {
     free(*str);
@@ -2865,6 +2889,15 @@ out:
     return rc;
 }
 
+int parse_domid(const char *arg)
+{
+    if (!arg || !strcmp(arg, "next"))
+        return INVALID_DOMID; /* Xen will use the next available */
+    else if (!strcmp(arg, "random"))
+        return RANDOM_DOMID; /* libxl will choose a random value */
+
+    return parse_int(arg);
+}
 
 /*
  * Local variables:
diff --git a/tools/xl/xl_parse.h b/tools/xl/xl_parse.h
index bab2861f8c..3a82722f92 100644
--- a/tools/xl/xl_parse.h
+++ b/tools/xl/xl_parse.h
@@ -56,6 +56,8 @@ void trim(char_predicate_t predicate, const char *input, char **output);
 
 const char *get_action_on_shutdown_name(libxl_action_on_shutdown a);
 
+int parse_domid(const char *str);
+
 #endif	/* XL_PARSE_H */
 
 /*
diff --git a/tools/xl/xl_saverestore.c b/tools/xl/xl_saverestore.c
index 9be033fe65..4be0afa04c 100644
--- a/tools/xl/xl_saverestore.c
+++ b/tools/xl/xl_saverestore.c
@@ -164,17 +164,22 @@ int main_restore(int argc, char **argv)
 {
     const char *checkpoint_file = NULL;
     const char *config_file = NULL;
+    const char *domid = NULL;
     struct domain_create dom_info;
     int paused = 0, debug = 0, daemonize = 1, monitor = 1,
         console_autoconnect = 0, vnc = 0, vncautopass = 0;
     int opt, rc;
     static struct option opts[] = {
+        {"domid", 1, 0, 'D'},
         {"vncviewer", 0, 0, 'V'},
         {"vncviewer-autopass", 0, 0, 'A'},
         COMMON_LONG_OPTS
     };
 
-    SWITCH_FOREACH_OPT(opt, "FcpdeVA", opts, "restore", 1) {
+    SWITCH_FOREACH_OPT(opt, "D:FcpdeVA", opts, "restore", 1) {
+    case 'D':
+        domid = optarg;
+        break;
     case 'c':
         console_autoconnect = 1;
         break;
@@ -210,6 +215,8 @@ int main_restore(int argc, char **argv)
     }
 
     memset(&dom_info, 0, sizeof(dom_info));
+
+    dom_info.domid = parse_domid(domid);
     dom_info.debug = debug;
     dom_info.daemonize = daemonize;
     dom_info.monitor = monitor;
diff --git a/tools/xl/xl_vmcontrol.c b/tools/xl/xl_vmcontrol.c
index e520b1da79..d81a629a5a 100644
--- a/tools/xl/xl_vmcontrol.c
+++ b/tools/xl/xl_vmcontrol.c
@@ -641,7 +641,7 @@ static void autoconnect_console(libxl_ctx *ctx_ignored,
 
 int create_domain(struct domain_create *dom_info)
 {
-    uint32_t domid = INVALID_DOMID;
+    uint32_t domid = dom_info->domid;
 
     libxl_domain_config d_config;
 
@@ -660,6 +660,7 @@ int create_domain(struct domain_create *dom_info)
 
     int i;
     int need_daemon = daemonize;
+    const char *op;
     int ret, rc;
     libxl_evgen_domain_death *deathw = NULL;
     libxl_evgen_disk_eject **diskws = NULL; /* one per disk */
@@ -872,8 +873,6 @@ int create_domain(struct domain_create *dom_info)
         goto out;
 
 start:
-    assert(domid == INVALID_DOMID);
-
     rc = acquire_lock();
     if (rc < 0)
         goto error_out;
@@ -911,6 +910,7 @@ start:
         libxl_defbool_set(&params.userspace_colo_proxy,
                           dom_info->userspace_colo_proxy);
 
+        op = "restore";
         ret = libxl_domain_create_restore(ctx, &d_config,
                                           &domid, restore_fd,
                                           send_back_fd, &params,
@@ -925,16 +925,21 @@ start:
         restoring = 0;
     } else if (domid_soft_reset != INVALID_DOMID) {
         /* Do soft reset. */
+        op = "soft reset";
         ret = libxl_domain_soft_reset(ctx, &d_config, domid_soft_reset,
                                       0, autoconnect_console_how);
         domid = domid_soft_reset;
         domid_soft_reset = INVALID_DOMID;
     } else {
+        op = "create";
         ret = libxl_domain_create_new(ctx, &d_config, &domid,
                                       0, autoconnect_console_how);
     }
-    if ( ret )
+    if (ret) {
+        fprintf(stderr, "%s operation failed: %s\n", op,
+                libxl_error_to_string(ret));
         goto error_out;
+    }
 
     release_lock();
 
@@ -1111,7 +1116,7 @@ start:
 
 error_out:
     release_lock();
-    if (libxl_domid_valid_guest(domid)) {
+    if (ret != ERROR_DEVICE_EXISTS && libxl_domid_valid_guest(domid)) {
         libxl_domain_destroy(ctx, domid, 0);
         domid = INVALID_DOMID;
     }
@@ -1153,11 +1158,13 @@ out:
 int main_create(int argc, char **argv)
 {
     const char *filename = NULL;
+    const char *domid = NULL;
     struct domain_create dom_info;
     int paused = 0, debug = 0, daemonize = 1, console_autoconnect = 0,
         quiet = 0, monitor = 1, vnc = 0, vncautopass = 0, ignore_masks = 0;
     int opt, rc;
     static struct option opts[] = {
+        {"domid", 1, 0, 'D'},
         {"dryrun", 0, 0, 'n'},
         {"quiet", 0, 0, 'q'},
         {"defconfig", 1, 0, 'f'},
@@ -1174,7 +1181,10 @@ int main_create(int argc, char **argv)
         argc--; argv++;
     }
 
-    SWITCH_FOREACH_OPT(opt, "Fnqf:pcdeVAi", opts, "create", 0) {
+    SWITCH_FOREACH_OPT(opt, "D:Fnqf:pcdeVAi", opts, "create", 0) {
+    case 'D':
+        domid = optarg;
+        break;
     case 'f':
         filename = optarg;
         break;
@@ -1226,6 +1236,7 @@ int main_create(int argc, char **argv)
         }
     }
 
+    dom_info.domid = parse_domid(domid);
     dom_info.debug = debug;
     dom_info.daemonize = daemonize;
     dom_info.monitor = monitor;
-- 
2.20.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 6/6] xl: allow specified domain id to be used for create, restore and migrate

[Ian Jackson]

Paul Durrant writes ("[PATCH 6/6] xl: allow specified domain id to be used for create, restore and migrate"):
> This patch adds the option to use a specified domain id to be used for
> the create, restore and migrate lifecycle operations and documentation
> thereof.

I approve of the idea that the xl user can specify the domid.  But:

Why should this be a command line argument to xl, rather than a domain
config parameter ?  Obviously there needs to be a way to override the
choice, especially to make localhost migration work, but there is
already a way to specify extra config on domain create, at least.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 6/6] xl: allow specified domain id to be used for create, restore and migrate

[Durrant, Paul]

> -----Original Message-----
> From: Ian Jackson <ian.jackson@citrix.com>
> Sent: 02 January 2020 17:30
> To: Durrant, Paul <pdurrant@amazon.com>
> Cc: xen-devel@lists.xenproject.org; Wei Liu <wl@xen.org>
> Subject: Re: [PATCH 6/6] xl: allow specified domain id to be used for
> create, restore and migrate
> 
> Paul Durrant writes ("[PATCH 6/6] xl: allow specified domain id to be used
> for create, restore and migrate"):
> > This patch adds the option to use a specified domain id to be used for
> > the create, restore and migrate lifecycle operations and documentation
> > thereof.
> 
> I approve of the idea that the xl user can specify the domid.  But:
> 
> Why should this be a command line argument to xl, rather than a domain
> config parameter ?  Obviously there needs to be a way to override the
> choice, especially to make localhost migration work, but there is
> already a way to specify extra config on domain create, at least.
> 

I debated which was best and came down on the side of a command line parameter becase I thought that chances of an admin wanting to tie a cfg to specified domid was small. But I guess the cfg can already specify a name, which needs to be overridden on a per-creation basis if an admin wants to use a common 'template' cfg... so maybe that option is indeed more consistent.

  Paul

> Thanks,
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel