[5/6] libxl: allow creation of domains with specified or random domid
diff mbox series

Message ID 20191224130416.3570-6-pdurrant@amazon.com
State New
Headers show
Series
  • xl/libxl: allow creation of domains with a specified domid
Related show

Commit Message

Paul Durrant Dec. 24, 2019, 1:04 p.m. UTC
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(-)

Comments

Ian Jackson Jan. 2, 2020, 5:27 p.m. UTC | #1
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.
Paul Durrant Jan. 3, 2020, 9:19 a.m. UTC | #2
> -----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.
Jason Andryuk Jan. 3, 2020, 7:24 p.m. UTC | #3
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;
> +        }
> +

Patch
diff mbox series

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",