[PATCH v2 0/3] libxl: libxl: update available vcpus map in retrieve configuration function

[PATCH v2 0/3] libxl: libxl: update available vcpus map in retrieve configuration function

[Wei Liu]

Patch 3 is not really related to the bug, but something we can fix when after
having first patch.

Wei Liu (3):
  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          | 126 +++++++++++++++++++++++++++++++++++++++----
 tools/libxl/libxl_internal.h |   3 ++
 tools/libxl/libxl_qmp.c      |  37 +++++++++++++
 3 files changed, 156 insertions(+), 10 deletions(-)

-- 
2.1.4


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

[PATCH v2 1/3] 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>
---
 tools/libxl/libxl_internal.h |  3 +++
 tools/libxl/libxl_qmp.c      | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index ae16c25..e9a3cf0 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 number 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..23eac92 100644
--- a/tools/libxl/libxl_qmp.c
+++ b/tools/libxl/libxl_qmp.c
@@ -979,6 +979,43 @@ 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 retreive CPU index.");
+            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

Re: [PATCH v2 1/3] libxl: introduce libxl__qmp_query_cpus

[Dario Faggioli]

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

On Wed, 2016-06-08 at 15:28 +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: Dario Faggioli <dario.faggioli@citrix.com>

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 v2 1/3] libxl: introduce libxl__qmp_query_cpus

[Anthony PERARD]

On Wed, Jun 08, 2016 at 03:28:44PM +0100, Wei Liu wrote:
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index 3eb279a..23eac92 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -979,6 +979,43 @@ 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 retreive CPU index.");

rc is still uninitialised at this point.

Also, s/retreive/retrieve/.

> +            goto out;
> +        }
> +
> +        idx = libxl__json_object_get_integer(o);
> +        libxl_bitmap_set(map, idx);
> +    }
> +
> +    rc = 0;
> +out:
> +    GC_FREE;
> +    return rc;
> +}

-- 
Anthony PERARD

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

Re: [PATCH v2 1/3] libxl: introduce libxl__qmp_query_cpus

[Wei Liu]

On Mon, Jun 13, 2016 at 05:52:14PM +0100, Anthony PERARD wrote:
> On Wed, Jun 08, 2016 at 03:28:44PM +0100, Wei Liu wrote:
> > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> > index 3eb279a..23eac92 100644
> > --- a/tools/libxl/libxl_qmp.c
> > +++ b/tools/libxl/libxl_qmp.c
> > @@ -979,6 +979,43 @@ 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 retreive CPU index.");
> 
> rc is still uninitialised at this point.
> 
> Also, s/retreive/retrieve/.
> 

Good catch. I will fix both issues.

Wei.

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

[PATCH v2 2/3] 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>
---
 tools/libxl/libxl.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 006b83f..02706ab 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -7222,6 +7222,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 (!strncmp(content, "online", strlen("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)
 {
@@ -7270,6 +7317,46 @@ 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_bitmap_dispose(map);
+        libxl_bitmap_init(map);
+        libxl_bitmap_alloc(CTX, map, max_vcpus);
+        libxl_bitmap_set_none(map);
+
+        switch (d_config->b_info.type) {
+        case LIBXL_DOMAIN_TYPE_HVM:
+            switch (d_config->b_info.device_model_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

Re: [PATCH v2 2/3] libxl: update vcpus bitmap in retrieved guest config

[Dario Faggioli]

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

On Wed, 2016-06-08 at 15:28 +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: Dario Faggioli <dario.faggioli@citrix.com>

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 v2 2/3] libxl: update vcpus bitmap in retrieved guest config

[Anthony PERARD]

On Wed, Jun 08, 2016 at 03:28:45PM +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.

I tried to migrate a guest, and libxl abort in
libxl_retrieve_domain_configuration within the switch
(device_model_version).


> Reported-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxl/libxl.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 006b83f..02706ab 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -7222,6 +7222,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;

The value should already be 0 at this point.

> +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 (!strncmp(content, "online", strlen("online")))

I don't think strncmp is usefull here as one of the argument is a plain
string. One could just use strcmp?

> +            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)
>  {
> @@ -7270,6 +7317,46 @@ 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_bitmap_dispose(map);
> +        libxl_bitmap_init(map);
> +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> +        libxl_bitmap_set_none(map);
> +
> +        switch (d_config->b_info.type) {
> +        case LIBXL_DOMAIN_TYPE_HVM:
> +            switch (d_config->b_info.device_model_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();

Missing indentation for abort.

Also, that is where xl abort on migration.

> +            }
> +            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:



-- 
Anthony PERARD

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

Re: [PATCH v2 2/3] libxl: update vcpus bitmap in retrieved guest config

[Wei Liu]

On Mon, Jun 13, 2016 at 06:39:36PM +0100, Anthony PERARD wrote:
> On Wed, Jun 08, 2016 at 03:28:45PM +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.
> 
> I tried to migrate a guest, and libxl abort in
> libxl_retrieve_domain_configuration within the switch
> (device_model_version).
> 
> 
> > Reported-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > ---
> >  tools/libxl/libxl.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> > 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > index 006b83f..02706ab 100644
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -7222,6 +7222,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;
> 
> The value should already be 0 at this point.
> 

I would like to keep this as-is because this is an idiom that is safer
against further modification of this function.

> > +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 (!strncmp(content, "online", strlen("online")))
> 
> I don't think strncmp is usefull here as one of the argument is a plain
> string. One could just use strcmp?
> 

Fine by me of course.

> > +            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)
> >  {
> > @@ -7270,6 +7317,46 @@ 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_bitmap_dispose(map);
> > +        libxl_bitmap_init(map);
> > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > +        libxl_bitmap_set_none(map);
> > +
> > +        switch (d_config->b_info.type) {
> > +        case LIBXL_DOMAIN_TYPE_HVM:
> > +            switch (d_config->b_info.device_model_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();
> 
> Missing indentation for abort.
> 

Will fix.

> Also, that is where xl abort on migration.
> 

Hmm...  This means the device model version is not valid (unknown?).

Can you paste in your guest config?

Wei.

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

Re: [PATCH v2 2/3] libxl: update vcpus bitmap in retrieved guest config

[Anthony PERARD]

On Mon, Jun 13, 2016 at 07:21:53PM +0100, Wei Liu wrote:
> On Mon, Jun 13, 2016 at 06:39:36PM +0100, Anthony PERARD wrote:
> > On Wed, Jun 08, 2016 at 03:28:45PM +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.
> > 
> > I tried to migrate a guest, and libxl abort in
> > libxl_retrieve_domain_configuration within the switch
> > (device_model_version).
> > 
> > 
> > > Reported-by: Jan Beulich <jbeulich@suse.com>
> > > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > > ---
> > >  tools/libxl/libxl.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 87 insertions(+)
> > > 
> > > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > > index 006b83f..02706ab 100644
> > > --- a/tools/libxl/libxl.c
> > > +++ b/tools/libxl/libxl.c
> > > @@ -7222,6 +7222,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;
> > 
> > The value should already be 0 at this point.
> > 
> 
> I would like to keep this as-is because this is an idiom that is safer
> against further modification of this function.
> 
> > > +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 (!strncmp(content, "online", strlen("online")))
> > 
> > I don't think strncmp is usefull here as one of the argument is a plain
> > string. One could just use strcmp?
> > 
> 
> Fine by me of course.
> 
> > > +            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)
> > >  {
> > > @@ -7270,6 +7317,46 @@ 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_bitmap_dispose(map);
> > > +        libxl_bitmap_init(map);
> > > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > > +        libxl_bitmap_set_none(map);
> > > +
> > > +        switch (d_config->b_info.type) {
> > > +        case LIBXL_DOMAIN_TYPE_HVM:
> > > +            switch (d_config->b_info.device_model_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();
> > 
> > Missing indentation for abort.
> > 
> 
> Will fix.
> 
> > Also, that is where xl abort on migration.
> > 
> 
> Hmm...  This means the device model version is not valid (unknown?).
> 
> Can you paste in your guest config?

With all commented line removed:

builder = 'hvm'
memory = 500
vcpus = 2
maxvcpus = 6
name = "arch"
vif = [ 'type=ioemu,mac=00:16:3e:XX:XX:XX' ]
disk = [
'phy:/dev/vg42/guest_arch64,hda,w',
 'file:/root/vm/iso/archlinux-2014.04.01-dual.iso,hdc:cdrom,r',
]
device_model_args_hvm = [
]
usbdevice='tablet'
boot="cd"
serial='pty'
sdl = 0
vnc = 1
vnclisten = '0.0.0.0'
vncunused = 1
spice=0
uuid = "XXXX"


-- 
Anthony PERARD

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

Re: [PATCH v2 2/3] libxl: update vcpus bitmap in retrieved guest config

[Wei Liu]

On Tue, Jun 14, 2016 at 11:47:57AM +0100, Anthony PERARD wrote:
[...]
> > > > +
> > > >  int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > > >                                          libxl_domain_config *d_config)
> > > >  {
> > > > @@ -7270,6 +7317,46 @@ 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_bitmap_dispose(map);
> > > > +        libxl_bitmap_init(map);
> > > > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > > > +        libxl_bitmap_set_none(map);
> > > > +
> > > > +        switch (d_config->b_info.type) {
> > > > +        case LIBXL_DOMAIN_TYPE_HVM:
> > > > +            switch (d_config->b_info.device_model_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();
> > > 
> > > Missing indentation for abort.
> > > 
> > 
> > Will fix.
> > 
> > > Also, that is where xl abort on migration.
> > > 
> > 
> > Hmm...  This means the device model version is not valid (unknown?).
> > 
> > Can you paste in your guest config?
> 
> With all commented line removed:
> 
> builder = 'hvm'
> memory = 500
> vcpus = 2
> maxvcpus = 6
> name = "arch"
> vif = [ 'type=ioemu,mac=00:16:3e:XX:XX:XX' ]
> disk = [
> 'phy:/dev/vg42/guest_arch64,hda,w',
>  'file:/root/vm/iso/archlinux-2014.04.01-dual.iso,hdc:cdrom,r',
> ]
> device_model_args_hvm = [
> ]
> usbdevice='tablet'
> boot="cd"
> serial='pty'
> sdl = 0
> vnc = 1
> vnclisten = '0.0.0.0'
> vncunused = 1
> spice=0
> uuid = "XXXX"
> 

This means your device model version is LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
so it should be covered by the correct case.

I'm confused.

Wei.

> 
> -- 
> Anthony PERARD

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

Re: [PATCH v2 2/3] libxl: update vcpus bitmap in retrieved guest config

[Anthony PERARD]

On Tue, Jun 14, 2016 at 11:50:12AM +0100, Wei Liu wrote:
> On Tue, Jun 14, 2016 at 11:47:57AM +0100, Anthony PERARD wrote:
> [...]
> > > > > +
> > > > >  int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > > > >                                          libxl_domain_config *d_config)
> > > > >  {
> > > > > @@ -7270,6 +7317,46 @@ 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_bitmap_dispose(map);
> > > > > +        libxl_bitmap_init(map);
> > > > > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > > > > +        libxl_bitmap_set_none(map);
> > > > > +
> > > > > +        switch (d_config->b_info.type) {
> > > > > +        case LIBXL_DOMAIN_TYPE_HVM:
> > > > > +            switch (d_config->b_info.device_model_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();
> > > > 
> > > > Missing indentation for abort.
> > > > 
> > > 
> > > Will fix.
> > > 
> > > > Also, that is where xl abort on migration.
> > > > 
> > > 
> > > Hmm...  This means the device model version is not valid (unknown?).
> > > 
> > > Can you paste in your guest config?
> > 
> > With all commented line removed:
> > 
> > builder = 'hvm'
> > memory = 500
> > vcpus = 2
> > maxvcpus = 6
> > name = "arch"
> > vif = [ 'type=ioemu,mac=00:16:3e:XX:XX:XX' ]
> > disk = [
> > 'phy:/dev/vg42/guest_arch64,hda,w',
> >  'file:/root/vm/iso/archlinux-2014.04.01-dual.iso,hdc:cdrom,r',
> > ]
> > device_model_args_hvm = [
> > ]
> > usbdevice='tablet'
> > boot="cd"
> > serial='pty'
> > sdl = 0
> > vnc = 1
> > vnclisten = '0.0.0.0'
> > vncunused = 1
> > spice=0
> > uuid = "XXXX"
> > 
> 
> This means your device model version is LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
> so it should be covered by the correct case.
> 
> I'm confused.

If it works for you, I'll properly install Xen with your patch in,
I may miss something...
I did:
LD_LIBRARY_PATH=`pwd` ./xl -vvv migrate arch localhost

-- 
Anthony PERARD

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

Re: [PATCH v2 2/3] libxl: update vcpus bitmap in retrieved guest config

[Wei Liu]

On Tue, Jun 14, 2016 at 11:58:58AM +0100, Anthony PERARD wrote:
[...]
> > 
> > This means your device model version is LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
> > so it should be covered by the correct case.
> > 
> > I'm confused.
> 
> If it works for you, I'll properly install Xen with your patch in,
> I may miss something...

Yes, it did work for me.

> I did:
> LD_LIBRARY_PATH=`pwd` ./xl -vvv migrate arch localhost
> 

Never tried this so I'm not sure what might go wrong...

Wei.

> -- 
> Anthony PERARD

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

Re: [PATCH v2 2/3] libxl: update vcpus bitmap in retrieved guest config

[Ian Jackson]

Wei Liu writes ("Re: [PATCH v2 2/3] libxl: update vcpus bitmap in retrieved guest config"):
> On Tue, Jun 14, 2016 at 11:58:58AM +0100, Anthony PERARD wrote:
> > I did:
> > LD_LIBRARY_PATH=`pwd` ./xl -vvv migrate arch localhost
> 
> Never tried this so I'm not sure what might go wrong...

That is how I do most of my ad-hoc testing.

If you have a newer xenstore etc. too you may need something like
  LD_LIBRARY_PATH=../xenstore:.

I find that relative paths work well with xl (because libxl is not
entitled to chdir, and xl doesn't).

Ian.

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

Re: [PATCH v2 2/3] libxl: update vcpus bitmap in retrieved guest config

[Anthony PERARD]

On Tue, Jun 14, 2016 at 11:58:58AM +0100, Anthony PERARD wrote:
> On Tue, Jun 14, 2016 at 11:50:12AM +0100, Wei Liu wrote:
> > On Tue, Jun 14, 2016 at 11:47:57AM +0100, Anthony PERARD wrote:
> > [...]
> > > > > > +
> > > > > >  int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
> > > > > >                                          libxl_domain_config *d_config)
> > > > > >  {
> > > > > > @@ -7270,6 +7317,46 @@ 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_bitmap_dispose(map);
> > > > > > +        libxl_bitmap_init(map);
> > > > > > +        libxl_bitmap_alloc(CTX, map, max_vcpus);
> > > > > > +        libxl_bitmap_set_none(map);
> > > > > > +
> > > > > > +        switch (d_config->b_info.type) {
> > > > > > +        case LIBXL_DOMAIN_TYPE_HVM:
> > > > > > +            switch (d_config->b_info.device_model_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();
> > > > > 
> > > > > Missing indentation for abort.
> > > > > 
> > > > 
> > > > Will fix.
> > > > 
> > > > > Also, that is where xl abort on migration.
> > > > > 
> > > > 
> > > > Hmm...  This means the device model version is not valid (unknown?).
> > > > 
> > > > Can you paste in your guest config?
> > > 
> > > With all commented line removed:
> > > 
> > > builder = 'hvm'
> > > memory = 500
> > > vcpus = 2
> > > maxvcpus = 6
> > > name = "arch"
> > > vif = [ 'type=ioemu,mac=00:16:3e:XX:XX:XX' ]
> > > disk = [
> > > 'phy:/dev/vg42/guest_arch64,hda,w',
> > >  'file:/root/vm/iso/archlinux-2014.04.01-dual.iso,hdc:cdrom,r',
> > > ]
> > > device_model_args_hvm = [
> > > ]
> > > usbdevice='tablet'
> > > boot="cd"
> > > serial='pty'
> > > sdl = 0
> > > vnc = 1
> > > vnclisten = '0.0.0.0'
> > > vncunused = 1
> > > spice=0
> > > uuid = "XXXX"
> > > 
> > 
> > This means your device model version is LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN
> > so it should be covered by the correct case.
> > 
> > I'm confused.
> 
> If it works for you, I'll properly install Xen with your patch in,
> I may miss something...
> I did:
> LD_LIBRARY_PATH=`pwd` ./xl -vvv migrate arch localhost

I tried again, this time Xen properly installed with your patch series.

sh$ xl create arch.hvm
sh$ xl list
Name                                        ID   Mem VCPUs	State	Time(s)
Domain-0                                     0  4000     8     r-----      25.3
arch                                         1   500     2     -b----       5.4
sh$ xl vcpu-set arch 5
sh$ xl list
Name                                        ID   Mem VCPUs	State	Time(s)
Domain-0                                     0  4000     8     r-----      25.8
arch                                         1   500     5     -b----       6.4

# All fine up to here.

sh$ xl migrate arch localhost
Aborted (core dumped)
sh$  xl save arch savefile
Aborted (core dumped)
sh$ xl save arch savefile ~/arch.hvm
Saving to savefile new xl format (info 0x3/0x0/1540)
xc: info: Saving domain 1, type x86 HVM
xc: Frames: 1044482/1044482  100%
xc: End of stream: 0/0    0%

# So there is only one way to migrate or save the guest.

sh$ xl restore savefile
[...]
libxl: error: libxl_dm.c:2187:device_model_spawn_outcome: domain 2 device model: spawn failed (rc=-3)
[...]

# So restore still fail with this in QEMU log:
qemu-system-i386: Unknown savevm section or instance 'cpu_common' 2
qemu-system-i386: load of migration failed: Invalid argument

-- 
Anthony PERARD

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

[PATCH v2 3/3] 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>
---
 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 02706ab..62a7ade 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -5740,19 +5740,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 v2 3/3] libxl: only issue cpu-add call to QEMU for not present CPU

[Jan Beulich]

>>> On 08.06.16 at 16:28, <wei.liu2@citrix.com> wrote:
> Calculate the final bitmap for CPUs to add to avoid having annoying
> error messages complaining those CPUs are already present.

Ah, nice - I had noticed these odd errors too, but didn't dare to
complain about such a cosmetic issue at the same time.

Jan


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

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

[Anthony PERARD]

On Wed, Jun 08, 2016 at 03:28:46PM +0100, Wei Liu wrote:
> 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>

-- 
Anthony PERARD

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