[PATCH v3 0/5] libxl: libxl: update available vcpus map in retrieve configuration function

[PATCH v3 0/5] libxl: libxl: update available vcpus map in retrieve configuration function

[Wei Liu]

Patch 1 and 2 are new in v3.

Patch 3 is changed to fix a minor bug.

Patch 4 is changed to fix a bug.

Patch 5 is unchanged.

See individual patch for detalied changelog.

Wei.

Wei Liu (5):
  libxl: libxl_domain_need_memory shouldn't modify b_info
  libxl: factor out libxl__get_device_modlel_version
  libxl: introduce libxl__qmp_query_cpus
  libxl: update vcpus bitmap in retrieved guest config
  libxl: only issue cpu-add call to QEMU for not present CPU

 tools/libxl/libxl.c          | 152 ++++++++++++++++++++++++++++++++++++++-----
 tools/libxl/libxl.h          |   7 ++
 tools/libxl/libxl_create.c   |  35 ++++++----
 tools/libxl/libxl_internal.h |   8 +++
 tools/libxl/libxl_qmp.c      |  38 +++++++++++
 5 files changed, 213 insertions(+), 27 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't modify b_info

[Wei Liu]

This function is used to return the memory needed for a guest. It's not
in a position to modify the b_info passed in (note the _setdefault
function).

Use a copy of b_info to do the calculation. Define a macro to mark the
change in API.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>

v3: new
Any suggestion on the macro name?

This patch should not be backported because it changes API behaviour.
---
 tools/libxl/libxl.c | 17 +++++++++++------
 tools/libxl/libxl.h |  7 +++++++
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 5ec4c80..65af9ee 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5123,20 +5123,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx, libxl_domain_build_info *b_info,
                              uint32_t *need_memkb)
 {
     GC_INIT(ctx);
+    libxl_domain_build_info tmp;
     int rc;
 
-    rc = libxl__domain_build_info_setdefault(gc, b_info);
+    libxl_domain_build_info_init(&tmp);
+    libxl_domain_build_info_copy(ctx, &tmp, b_info);
+
+    rc = libxl__domain_build_info_setdefault(gc, &tmp);
     if (rc) goto out;
 
-    *need_memkb = b_info->target_memkb;
-    switch (b_info->type) {
+    *need_memkb = tmp.target_memkb;
+    switch (tmp.type) {
     case LIBXL_DOMAIN_TYPE_HVM:
-        *need_memkb += b_info->shadow_memkb + LIBXL_HVM_EXTRA_MEMORY;
-        if (libxl_defbool_val(b_info->device_model_stubdomain))
+        *need_memkb += tmp.shadow_memkb + LIBXL_HVM_EXTRA_MEMORY;
+        if (libxl_defbool_val(tmp.device_model_stubdomain))
             *need_memkb += 32 * 1024;
         break;
     case LIBXL_DOMAIN_TYPE_PV:
-        *need_memkb += b_info->shadow_memkb + LIBXL_PV_EXTRA_MEMORY;
+        *need_memkb += tmp.shadow_memkb + LIBXL_PV_EXTRA_MEMORY;
         break;
     default:
         rc = ERROR_INVAL;
@@ -5146,6 +5150,7 @@ int libxl_domain_need_memory(libxl_ctx *ctx, libxl_domain_build_info *b_info,
         *need_memkb += (2 * 1024) - (*need_memkb % (2 * 1024));
     rc = 0;
 out:
+    libxl_domain_build_info_dispose(&tmp);
     GC_FREE;
     return rc;
 
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 2c0f868..905852d 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -67,6 +67,13 @@
  * the same $(XEN_VERSION) (e.g. throughout a major release).
  */
 
+/* LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2
+ *
+ * If this is defined, libxl_domain_need_memory no longer modifies
+ * passed in b_info.
+ */
+#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2
+
 /* LIBXL_HAVE_VNUMA
  *
  * If this is defined the type libxl_vnode_info exists, and a
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 2/5] libxl: factor out libxl__get_device_modlel_version

[Wei Liu]

Factor out the logic to determine device model version to a function. It
will be used later. Note that code now also checks if the stbudom field
is actually set before trying to extract a value from it.

No functional change.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v3: new patch
---
 tools/libxl/libxl_create.c   | 35 ++++++++++++++++++++++++-----------
 tools/libxl/libxl_internal.h |  5 +++++
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 1b99472..45fd7bc 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -62,6 +62,28 @@ void libxl__rdm_setdefault(libxl__gc *gc, libxl_domain_build_info *b_info)
                             LIBXL_RDM_MEM_BOUNDARY_MEMKB_DEFAULT;
 }
 
+libxl_device_model_version
+libxl__get_device_model_version(libxl__gc *gc,
+                                const libxl_domain_build_info *b_info)
+{
+    libxl_device_model_version version = LIBXL_DEVICE_MODEL_VERSION_UNKNOWN;
+
+    if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
+        if (!libxl_defbool_is_default(b_info->device_model_stubdomain) &&
+            libxl_defbool_val(b_info->device_model_stubdomain)) {
+            version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
+        } else {
+            version = libxl__default_device_model(gc);
+        }
+    } else {
+        version = LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
+    }
+
+    assert(version != LIBXL_DEVICE_MODEL_VERSION_UNKNOWN);
+
+    return version;
+}
+
 int libxl__domain_build_info_setdefault(libxl__gc *gc,
                                         libxl_domain_build_info *b_info)
 {
@@ -80,17 +102,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
         b_info->device_model_ssidref = SECINITSID_DOMDM;
 
     if (!b_info->device_model_version) {
-        if (b_info->type == LIBXL_DOMAIN_TYPE_HVM) {
-            if (libxl_defbool_val(b_info->device_model_stubdomain)) {
-                b_info->device_model_version =
-                    LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL;
-            } else {
-                b_info->device_model_version = libxl__default_device_model(gc);
-            }
-        } else {
-            b_info->device_model_version =
-                LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN;
-        }
+        b_info->device_model_version =
+            libxl__get_device_model_version(gc, b_info);
         if (b_info->device_model_version
                 == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
             const char *dm;
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index e7ab85d..3958313 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1999,6 +1999,11 @@ _hidden int libxl__device_model_version_running(libxl__gc *gc, uint32_t domid);
   /* Return the system-wide default device model */
 _hidden libxl_device_model_version libxl__default_device_model(libxl__gc *gc);
 
+/* Derive device model version from build info */
+_hidden libxl_device_model_version
+libxl__get_device_model_version(libxl__gc *gc,
+                                const libxl_domain_build_info *b_info);
+
 #define DEVICE_MODEL_XS_PATH(gc, dm_domid, domid, fmt, _a...)              \
     libxl__sprintf(gc, "/local/domain/%u/device-model/%u" fmt, dm_domid,   \
                    domid, ##_a)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 3/5] libxl: introduce libxl__qmp_query_cpus

[Wei Liu]

It interrogates QEMU for CPUs and update the bitmap accordingly.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v3:
1. Initialise rc in error path.
2. Fix comment in header and a typo in log message.
---
 tools/libxl/libxl_internal.h |  3 +++
 tools/libxl/libxl_qmp.c      | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 3958313..cad35fb 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -1794,6 +1794,9 @@ _hidden int libxl__qmp_set_global_dirty_log(libxl__gc *gc, int domid, bool enabl
 _hidden int libxl__qmp_insert_cdrom(libxl__gc *gc, int domid, const libxl_device_disk *disk);
 /* Add a virtual CPU */
 _hidden int libxl__qmp_cpu_add(libxl__gc *gc, int domid, int index);
+/* Query the bitmap of CPUs */
+_hidden int libxl__qmp_query_cpus(libxl__gc *gc, int domid,
+                                  libxl_bitmap *map);
 /* Start NBD server */
 _hidden int libxl__qmp_nbd_server_start(libxl__gc *gc, int domid,
                                         const char *host, const char *port);
diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
index 3eb279a..63c49c5 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -979,6 +979,44 @@ int libxl__qmp_cpu_add(libxl__gc *gc, int domid, int idx)
     return qmp_run_command(gc, domid, "cpu-add", args, NULL, NULL);
 }
 
+static int query_cpus_callback(libxl__qmp_handler *qmp,
+                               const libxl__json_object *response,
+                               void *opaque)
+{
+    libxl_bitmap *map = opaque;
+    unsigned int i;
+    const libxl__json_object *cpu = NULL;
+    int rc;
+    GC_INIT(qmp->ctx);
+
+    libxl_bitmap_set_none(map);
+    for (i = 0; (cpu = libxl__json_array_get(response, i)); i++) {
+        unsigned int idx;
+        const libxl__json_object *o;
+
+        o = libxl__json_map_get("CPU", cpu, JSON_INTEGER);
+        if (!o) {
+            LOG(ERROR, "Failed to retrieve CPU index.");
+            rc = ERROR_FAIL;
+            goto out;
+        }
+
+        idx = libxl__json_object_get_integer(o);
+        libxl_bitmap_set(map, idx);
+    }
+
+    rc = 0;
+out:
+    GC_FREE;
+    return rc;
+}
+
+int libxl__qmp_query_cpus(libxl__gc *gc, int domid, libxl_bitmap *map)
+{
+    return qmp_run_command(gc, domid, "query-cpus", NULL,
+                           query_cpus_callback, map);
+}
+
 int libxl__qmp_nbd_server_start(libxl__gc *gc, int domid,
                                 const char *host, const char *port)
 {
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Wei Liu]

... because the available vcpu bitmap can change during domain life time
due to cpu hotplug and unplug.

For QEMU upstream, we interrogate QEMU for the number of vcpus. For
others, we look directly into xenstore for information.

Reported-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v3:
1. Fix indentation of abort.
2. Use strcmp instead of strncmp.
---
 tools/libxl/libxl.c | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 96 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 65af9ee..b748bf1 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -7246,6 +7246,53 @@ void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src)
         (*dst)[i] = (*src)[i];
 }
 
+static int libxl__update_avail_vcpus_qmp(libxl__gc *gc, uint32_t domid,
+                                         unsigned int max_vcpus,
+                                         libxl_bitmap *map)
+{
+    int rc;
+
+    /* For QEMU upstream we always need to return the number
+     * of cpus present to QEMU whether they are online or not;
+     * otherwise QEMU won't accept the saved state.
+     */
+    rc = libxl__qmp_query_cpus(gc, domid, map);
+    if (rc) {
+        LOG(ERROR, "fail to get number of cpus for domain %d", domid);
+        goto out;
+    }
+
+    rc = 0;
+out:
+    return rc;
+}
+
+static int libxl__update_avail_vcpus_xenstore(libxl__gc *gc, uint32_t domid,
+                                              unsigned int max_vcpus,
+                                              libxl_bitmap *map)
+{
+    int rc;
+    unsigned int i;
+    const char *dompath;
+
+    dompath = libxl__xs_get_dompath(gc, domid);
+    if (!dompath) {
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
+    for (i = 0; i < max_vcpus; i++) {
+        const char *path = GCSPRINTF("%s/cpu/%u/availability", dompath, i);
+        const char *content = libxl__xs_read(gc, XBT_NULL, path);
+        if (!strcmp(content, "online"))
+            libxl_bitmap_set(map, i);
+    }
+
+    rc = 0;
+out:
+    return rc;
+}
+
 int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
                                         libxl_domain_config *d_config)
 {
@@ -7294,6 +7341,55 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
         libxl_dominfo_dispose(&info);
     }
 
+    /* VCPUs */
+    {
+        libxl_bitmap *map = &d_config->b_info.avail_vcpus;
+        unsigned int max_vcpus = d_config->b_info.max_vcpus;
+        libxl_device_model_version version;
+
+        libxl_bitmap_dispose(map);
+        libxl_bitmap_init(map);
+        libxl_bitmap_alloc(CTX, map, max_vcpus);
+        libxl_bitmap_set_none(map);
+
+        /* If the user did not explicitly specify a device model
+         * version, we need to use the same logic used during domain
+         * creation to derive the device model version.
+         */
+        version = d_config->b_info.device_model_version;
+        if (version == LIBXL_DEVICE_MODEL_VERSION_UNKNOWN)
+            version = libxl__get_device_model_version(gc, &d_config->b_info);
+
+        switch (d_config->b_info.type) {
+        case LIBXL_DOMAIN_TYPE_HVM:
+            switch (version) {
+            case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
+                rc = libxl__update_avail_vcpus_qmp(gc, domid,
+                                                   max_vcpus, map);
+                break;
+            case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN_TRADITIONAL:
+            case LIBXL_DEVICE_MODEL_VERSION_NONE:
+                rc = libxl__update_avail_vcpus_xenstore(gc, domid,
+                                                        max_vcpus, map);
+                break;
+            default:
+                abort();
+            }
+            break;
+        case LIBXL_DOMAIN_TYPE_PV:
+            rc = libxl__update_avail_vcpus_xenstore(gc, domid,
+                                                    max_vcpus, map);
+            break;
+        default:
+            abort();
+        }
+
+        if (rc) {
+            LOG(ERROR, "fail to update available cpu map for domain %d", domid);
+            goto out;
+        }
+    }
+
     /* Memory limits:
      *
      * Currently there are three memory limits:
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH v3 5/5] libxl: only issue cpu-add call to QEMU for not present CPU

[Wei Liu]

Calculate the final bitmap for CPUs to add to avoid having annoying
error messages complaining those CPUs are already present.

We can also properly handle error from QMP now.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>

v3:
1. Add Anthony's Reviewed-by tag.
---
 tools/libxl/libxl.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index b748bf1..0e2c15a 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5753,19 +5753,38 @@ static int libxl__set_vcpuonline_qmp(libxl__gc *gc, uint32_t domid,
                                      libxl_bitmap *cpumap,
                                      const libxl_dominfo *info)
 {
-    int i;
+    int i, rc;
+    libxl_bitmap current_map, final_map;
+
+    libxl_bitmap_init(&current_map);
+    libxl_bitmap_init(&final_map);
+
+    libxl_bitmap_alloc(CTX, &current_map, info->vcpu_max_id + 1);
+    libxl_bitmap_set_none(&current_map);
+    rc = libxl__qmp_query_cpus(gc, domid, &current_map);
+    if (rc) {
+        LOG(ERROR, "failed to query cpus for domain %d", domid);
+        goto out;
+    }
+
+    libxl_bitmap_copy_alloc(CTX, &final_map, cpumap);
 
-    for (i = 0; i <= info->vcpu_max_id; i++) {
-        if (libxl_bitmap_test(cpumap, i)) {
-            /* Return value is ignore because it does not tell anything useful
-             * on the completion of the command.
-             * (For instance, "CPU already plugged-in" give the same return
-             * value as "command not supported".)
-             */
-            libxl__qmp_cpu_add(gc, domid, i);
+    libxl_for_each_set_bit(i, current_map)
+        libxl_bitmap_reset(&final_map, i);
+
+    libxl_for_each_set_bit(i, final_map) {
+        rc = libxl__qmp_cpu_add(gc, domid, i);
+        if (rc) {
+            LOG(ERROR, "failed to add cpu %d to domain %d", i, domid);
+            goto out;
         }
     }
-    return 0;
+
+    rc = 0;
+out:
+    libxl_bitmap_dispose(&current_map);
+    libxl_bitmap_dispose(&final_map);
+    return rc;
 }
 
 int libxl_set_vcpuonline(libxl_ctx *ctx, uint32_t domid, libxl_bitmap *cpumap)
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 3/5] libxl: introduce libxl__qmp_query_cpus

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 617 bytes --]

On Wed, 2016-06-15 at 10:31 +0100, Wei Liu wrote:
> It interrogates QEMU for CPUs and update the bitmap accordingly.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> 
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/5] libxl: factor out libxl__get_device_modlel_version

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 795 bytes --]

On Wed, 2016-06-15 at 10:31 +0100, Wei Liu wrote:
> Factor out the logic to determine device model version to a function.
> It
> will be used later. Note that code now also checks if the stbudom
> field
> is actually set before trying to extract a value from it.
> 
> No functional change.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Anthony PERARD <anthony.perard@citrix.com>
>
Reviewed-by: Dario Faggioli <dario.faggioli@citrix.com>

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't modify b_info

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1390 bytes --]

On Wed, 2016-06-15 at 10:31 +0100, Wei Liu wrote:
> This function is used to return the memory needed for a guest. It's
> not
> in a position to modify the b_info passed in (note the _setdefault
> function).
> 
> Use a copy of b_info to do the calculation. Define a macro to mark
> the
> change in API.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> v3: new
> Any suggestion on the macro name?
> 
Maybe LIBXL_HAVE_DOMAIN_NEED_MEMORY_BINFO_CONST

(a bit long, I know...)

> 
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 2c0f868..905852d 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -67,6 +67,13 @@
>   * the same $(XEN_VERSION) (e.g. throughout a major release).
>   */
>  
> +/* LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2
> + *
> + * If this is defined, libxl_domain_need_memory no longer modifies
> + * passed in b_info.
> + */
> +#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2
> +
>
So, out of curiosity (or I should say "ignorance" :-)) how is one
supposed to use this macro?

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't modify b_info

[Wei Liu]

On Wed, Jun 15, 2016 at 03:31:46PM +0200, Dario Faggioli wrote:
> On Wed, 2016-06-15 at 10:31 +0100, Wei Liu wrote:
> > This function is used to return the memory needed for a guest. It's
> > not
> > in a position to modify the b_info passed in (note the _setdefault
> > function).
> > 
> > Use a copy of b_info to do the calculation. Define a macro to mark
> > the
> > change in API.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > 
> > v3: new
> > Any suggestion on the macro name?
> > 
> Maybe LIBXL_HAVE_DOMAIN_NEED_MEMORY_BINFO_CONST
> 
> (a bit long, I know...)

Heh...

> 
> > 
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 2c0f868..905852d 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -67,6 +67,13 @@
> >   * the same $(XEN_VERSION) (e.g. throughout a major release).
> >   */
> >  
> > +/* LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2
> > + *
> > + * If this is defined, libxl_domain_need_memory no longer modifies
> > + * passed in b_info.
> > + */
> > +#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2
> > +
> >
> So, out of curiosity (or I should say "ignorance" :-)) how is one
> supposed to use this macro?

#ifdef MACRO
  /* new API */
#else
  /* old API */
  /* probably want to pass in a scratch copy of b_info */
#endif

> 
> Regards,
> Dario
> -- 
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 3/5] libxl: introduce libxl__qmp_query_cpus

[Anthony PERARD]

On Wed, Jun 15, 2016 at 10:31:40AM +0100, Wei Liu wrote:
> It interrogates QEMU for CPUs and update the bitmap accordingly.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 2/5] libxl: factor out libxl__get_device_modlel_version

[Anthony PERARD]

On Wed, Jun 15, 2016 at 10:31:39AM +0100, Wei Liu wrote:
> Factor out the logic to determine device model version to a function. It
> will be used later. Note that code now also checks if the stbudom field
> is actually set before trying to extract a value from it.
> 
> No functional change.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Anthony PERARD]

On Wed, Jun 15, 2016 at 10:31:41AM +0100, Wei Liu wrote:
> ... because the available vcpu bitmap can change during domain life time
> due to cpu hotplug and unplug.
> 
> For QEMU upstream, we interrogate QEMU for the number of vcpus. For
> others, we look directly into xenstore for information.
> 
> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

-- 
Anthony PERARD

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 0/5] libxl: libxl: update available vcpus map in retrieve configuration function

[Wei Liu]

Ian, ping?

On Wed, Jun 15, 2016 at 10:31:37AM +0100, Wei Liu wrote:
> Patch 1 and 2 are new in v3.
> 
> Patch 3 is changed to fix a minor bug.
> 
> Patch 4 is changed to fix a bug.
> 
> Patch 5 is unchanged.
> 
> See individual patch for detalied changelog.
> 
> Wei.
> 
> Wei Liu (5):
>   libxl: libxl_domain_need_memory shouldn't modify b_info
>   libxl: factor out libxl__get_device_modlel_version
>   libxl: introduce libxl__qmp_query_cpus
>   libxl: update vcpus bitmap in retrieved guest config
>   libxl: only issue cpu-add call to QEMU for not present CPU
> 
>  tools/libxl/libxl.c          | 152 ++++++++++++++++++++++++++++++++++++++-----
>  tools/libxl/libxl.h          |   7 ++
>  tools/libxl/libxl_create.c   |  35 ++++++----
>  tools/libxl/libxl_internal.h |   8 +++
>  tools/libxl/libxl_qmp.c      |  38 +++++++++++
>  5 files changed, 213 insertions(+), 27 deletions(-)
> 
> -- 
> 2.1.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't modify b_info

[Ian Jackson]

Wei Liu writes ("[PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't modify b_info"):
> This function is used to return the memory needed for a guest. It's not
> in a position to modify the b_info passed in (note the _setdefault
> function).
> 
> Use a copy of b_info to do the calculation. Define a macro to mark the
> change in API.

Urgh, how unpleasant.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 5ec4c80..65af9ee 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5123,20 +5123,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx, libxl_domain_build_info *b_info,
>                               uint32_t *need_memkb)
>  {
>      GC_INIT(ctx);
> +    libxl_domain_build_info tmp;

I would suggest, instead:

    int libxl_domain_need_memory(libxl_ctx *ctx,
 -                               libxl_domain_build_info *b_info,
 +                               const libxl_domain_build_info *b_info_in,
...
 +     libxl_domain_build_info b_info[1];

If you do this then you do not need to change the bulk of the body of
the function at all.

> +/* LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2
> + *
> + * If this is defined, libxl_domain_need_memory no longer modifies
> + * passed in b_info.
> + */
> +#define LIBXL_HAVE_DOMAIN_NEED_MEMORY_V2

I suggest constifying it and

 LIBXL_HAVE_DOMAIN_NEED_MEMORY_CONST_B_INFO

Ian.

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

Re: [PATCH v3 2/5] libxl: factor out libxl__get_device_modlel_version

[Ian Jackson]

Wei Liu writes ("[PATCH v3 2/5] libxl: factor out libxl__get_device_modlel_version"):

Typo in subject line.

> Factor out the logic to determine device model version to a function. It
> will be used later. Note that code now also checks if the stbudom field
> is actually set before trying to extract a value from it.

With the typo fixed,

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Ian Jackson]

Wei Liu writes ("[PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config"):
> ... because the available vcpu bitmap can change during domain life time
> due to cpu hotplug and unplug.
> 
> For QEMU upstream, we interrogate QEMU for the number of vcpus. For
> others, we look directly into xenstore for information.
...
> +static int libxl__update_avail_vcpus_qmp(libxl__gc *gc, uint32_t domid,
> +                                         unsigned int max_vcpus,
> +                                         libxl_bitmap *map)
> +{
> +    int rc;
> +
> +    /* For QEMU upstream we always need to return the number
> +     * of cpus present to QEMU whether they are online or not;
> +     * otherwise QEMU won't accept the saved state.

This comment is confusing to me.  Perhaps `return' should read
`provide' ?  But the connection between the body of this function and
the comment is still obscure.

> +static int libxl__update_avail_vcpus_xenstore(libxl__gc *gc, uint32_t domid,
> +                                              unsigned int max_vcpus,
> +                                              libxl_bitmap *map)
> +{
> +    int rc;
> +    unsigned int i;
> +    const char *dompath;
...
> +    for (i = 0; i < max_vcpus; i++) {
> +        const char *path = GCSPRINTF("%s/cpu/%u/availability", dompath, i);
> +        const char *content = libxl__xs_read(gc, XBT_NULL, path);
> +        if (!strcmp(content, "online"))
> +            libxl_bitmap_set(map, i);

Firstly, this should be libxl__xs_read_checked.  Secondly if "content"
is NULL, this will crash.

> @@ -7294,6 +7341,55 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
>          libxl_dominfo_dispose(&info);
>      }
>  
> +    /* VCPUs */
> +    {
> +        libxl_bitmap *map = &d_config->b_info.avail_vcpus;
> +        unsigned int max_vcpus = d_config->b_info.max_vcpus;
> +        libxl_device_model_version version;
> +
> +        libxl_bitmap_dispose(map);
> +        libxl_bitmap_init(map);
> +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> +        libxl_bitmap_set_none(map);

I see that this function already trashes the domain config when it
fails (leaving it up to the caller to free the partial config) but
that this is not documented :-/.

> +        /* If the user did not explicitly specify a device model
> +         * version, we need to use the same logic used during domain
> +         * creation to derive the device model version.
> +         */
> +        version = d_config->b_info.device_model_version;
> +        if (version == LIBXL_DEVICE_MODEL_VERSION_UNKNOWN)
> +            version = libxl__get_device_model_version(gc, &d_config->b_info);

I think this is rather unfortunate.  Won't changing the default device
model while the domain is running will cause this function to give
wrong answers ?

Thanks,
Ian.

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

Re: [PATCH v3 3/5] libxl: introduce libxl__qmp_query_cpus

[Ian Jackson]

Wei Liu writes ("[PATCH v3 3/5] libxl: introduce libxl__qmp_query_cpus"):
> It interrogates QEMU for CPUs and update the bitmap accordingly.

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

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

Re: [PATCH v3 5/5] libxl: only issue cpu-add call to QEMU for not present CPU

[Ian Jackson]

Wei Liu writes ("[PATCH v3 5/5] libxl: only issue cpu-add call to QEMU for not present CPU"):
> Calculate the final bitmap for CPUs to add to avoid having annoying
> error messages complaining those CPUs are already present.

Can this be expanded a bit ?  I think perhaps it would benefit from
"When X and Y and Z, qemu prints an annoying message about Q".

I think I have understood it by reverse engineering the code but I
would still like a better commit message.

Thanks,
Ian.

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

Re: [PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't modify b_info

[Wei Liu]

On Fri, Jul 08, 2016 at 06:35:59PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v3 1/5] libxl: libxl_domain_need_memory shouldn't modify b_info"):
> > This function is used to return the memory needed for a guest. It's not
> > in a position to modify the b_info passed in (note the _setdefault
> > function).
> > 
> > Use a copy of b_info to do the calculation. Define a macro to mark the
> > change in API.
> 
> Urgh, how unpleasant.
> 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 5ec4c80..65af9ee 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -5123,20 +5123,24 @@ int libxl_domain_need_memory(libxl_ctx *ctx, libxl_domain_build_info *b_info,
> >                               uint32_t *need_memkb)
> >  {
> >      GC_INIT(ctx);
> > +    libxl_domain_build_info tmp;
> 
> I would suggest, instead:
> 
>     int libxl_domain_need_memory(libxl_ctx *ctx,
>  -                               libxl_domain_build_info *b_info,
>  +                               const libxl_domain_build_info *b_info_in,
> ...
>  +     libxl_domain_build_info b_info[1];
> 
> If you do this then you do not need to change the bulk of the body of
> the function at all.
> 

This is a good idea.

Just that I discover there is another unpleasant fact: all the generated
copy function doesn't constify their source parameter. I will need to
write a patch to fix that first.

Wei.

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

Re: [PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Wei Liu]

On Fri, Jul 08, 2016 at 06:44:28PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config"):
> > ... because the available vcpu bitmap can change during domain life time
> > due to cpu hotplug and unplug.
> > 
> > For QEMU upstream, we interrogate QEMU for the number of vcpus. For
> > others, we look directly into xenstore for information.
> ...
> > +static int libxl__update_avail_vcpus_qmp(libxl__gc *gc, uint32_t domid,
> > +                                         unsigned int max_vcpus,
> > +                                         libxl_bitmap *map)
> > +{
> > +    int rc;
> > +
> > +    /* For QEMU upstream we always need to return the number
> > +     * of cpus present to QEMU whether they are online or not;
> > +     * otherwise QEMU won't accept the saved state.
> 
> This comment is confusing to me.  Perhaps `return' should read
> `provide' ?  But the connection between the body of this function and
> the comment is still obscure.
> 

"provide" is ok.

To avoid confusion the comment can be moved a few line above to describe
the function, not the code snippet. Does that work better?

> > +static int libxl__update_avail_vcpus_xenstore(libxl__gc *gc, uint32_t domid,
> > +                                              unsigned int max_vcpus,
> > +                                              libxl_bitmap *map)
> > +{
> > +    int rc;
> > +    unsigned int i;
> > +    const char *dompath;
> ...
> > +    for (i = 0; i < max_vcpus; i++) {
> > +        const char *path = GCSPRINTF("%s/cpu/%u/availability", dompath, i);
> > +        const char *content = libxl__xs_read(gc, XBT_NULL, path);
> > +        if (!strcmp(content, "online"))
> > +            libxl_bitmap_set(map, i);
> 
> Firstly, this should be libxl__xs_read_checked.  Secondly if "content"
> is NULL, this will crash.
> 

Will fix.

> > @@ -7294,6 +7341,55 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> >          libxl_dominfo_dispose(&info);
> >      }
> >  
> > +    /* VCPUs */
> > +    {
> > +        libxl_bitmap *map = &d_config->b_info.avail_vcpus;
> > +        unsigned int max_vcpus = d_config->b_info.max_vcpus;
> > +        libxl_device_model_version version;
> > +
> > +        libxl_bitmap_dispose(map);
> > +        libxl_bitmap_init(map);
> > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > +        libxl_bitmap_set_none(map);
> 
> I see that this function already trashes the domain config when it
> fails (leaving it up to the caller to free the partial config) but
> that this is not documented :-/.

d_config is an out parameter. User can't expect the returned d_config to
be valid if this API returns error state. And user is still responsible
for calling _init before calling this API and _dispose afterwards. So
this still fits into how we expect libxl types to be used. Nothing new
needs to be documented.

All other fields are already handled in this way.

> 
> > +        /* If the user did not explicitly specify a device model
> > +         * version, we need to use the same logic used during domain
> > +         * creation to derive the device model version.
> > +         */
> > +        version = d_config->b_info.device_model_version;
> > +        if (version == LIBXL_DEVICE_MODEL_VERSION_UNKNOWN)
> > +            version = libxl__get_device_model_version(gc, &d_config->b_info);
> 
> I think this is rather unfortunate.  Won't changing the default device
> model while the domain is running will cause this function to give
> wrong answers ?

I think saving the device model into the template should work. No need
to derive it during runtime.

Wei.

> 
> Thanks,
> Ian.

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

Re: [PATCH v3 5/5] libxl: only issue cpu-add call to QEMU for not present CPU

[Wei Liu]

On Fri, Jul 08, 2016 at 06:48:27PM +0100, Ian Jackson wrote:
> Wei Liu writes ("[PATCH v3 5/5] libxl: only issue cpu-add call to QEMU for not present CPU"):
> > Calculate the final bitmap for CPUs to add to avoid having annoying
> > error messages complaining those CPUs are already present.
> 
> Can this be expanded a bit ?  I think perhaps it would benefit from
> "When X and Y and Z, qemu prints an annoying message about Q".
> 
> I think I have understood it by reverse engineering the code but I
> would still like a better commit message.

Sure. I can paste in the error message into commit log.

Wei.

> 
> Thanks,
> Ian.

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

Re: [PATCH v3 4/5] libxl: update vcpus bitmap in retrieved guest config

[Wei Liu]

On Mon, Jul 11, 2016 at 12:31:43PM +0100, Wei Liu wrote:
[...]
> 
> > 
> > > +        /* If the user did not explicitly specify a device model
> > > +         * version, we need to use the same logic used during domain
> > > +         * creation to derive the device model version.
> > > +         */
> > > +        version = d_config->b_info.device_model_version;
> > > +        if (version == LIBXL_DEVICE_MODEL_VERSION_UNKNOWN)
> > > +            version = libxl__get_device_model_version(gc, &d_config->b_info);
> > 
> > I think this is rather unfortunate.  Won't changing the default device
> > model while the domain is running will cause this function to give
> > wrong answers ?
> 
> I think saving the device model into the template should work. No need
> to derive it during runtime.
> 

Actually this information is already available via
libxl__device_model_version_running.

Wei.

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