[PATCH for 4.6 v4 0/3] More vNUMA patches

[PATCH for 4.6 v4 0/3] More vNUMA patches

[Wei Liu]

Wei Liu (3):
  libxc: introduce xc_domain_getvnuma
  xl/libxl: disallow saving a guest with vNUMA configured
  xl: handle empty vnuma configuration

 docs/man/xl.cfg.pod.5         |  2 ++
 tools/libxc/include/xenctrl.h | 18 +++++++++++++++
 tools/libxc/xc_domain.c       | 53 +++++++++++++++++++++++++++++++++++++++++++
 tools/libxl/libxl_dom.c       | 16 +++++++++++++
 tools/libxl/xl_cmdimpl.c      |  3 +++
 5 files changed, 92 insertions(+)

-- 
2.1.4

[PATCH for 4.6 v4 1/3] libxc: introduce xc_domain_getvnuma

[Wei Liu]

A simple wrapper for XENMEM_get_vnumainfo.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
v4: rebase on top of staging
---
 tools/libxc/include/xenctrl.h | 18 +++++++++++++++
 tools/libxc/xc_domain.c       | 53 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index e019474..3482544 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1287,6 +1287,24 @@ int xc_domain_setvnuma(xc_interface *xch,
                         unsigned int *vdistance,
                         unsigned int *vcpu_to_vnode,
                         unsigned int *vnode_to_pnode);
+/*
+ * Retrieve vnuma configuration
+ * domid: IN, target domid
+ * nr_vnodes: IN/OUT, number of vnodes, not NULL
+ * nr_vmemranges: IN/OUT, number of vmemranges, not NULL
+ * nr_vcpus: IN/OUT, number of vcpus, not NULL
+ * vmemranges: OUT, an array which has length of nr_vmemranges
+ * vdistance: OUT, an array which has length of nr_vnodes * nr_vnodes
+ * vcpu_to_vnode: OUT, an array which has length of nr_vcpus
+ */
+int xc_domain_getvnuma(xc_interface *xch,
+                       uint32_t domid,
+                       uint32_t *nr_vnodes,
+                       uint32_t *nr_vmemranges,
+                       uint32_t *nr_vcpus,
+                       xen_vmemrange_t *vmemrange,
+                       unsigned int *vdistance,
+                       unsigned int *vcpu_to_vnode);
 
 int xc_domain_soft_reset(xc_interface *xch,
                          uint32_t domid);
diff --git a/tools/libxc/xc_domain.c b/tools/libxc/xc_domain.c
index 62b2e45..e7278dd 100644
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -2493,6 +2493,59 @@ int xc_domain_setvnuma(xc_interface *xch,
     return rc;
 }
 
+int xc_domain_getvnuma(xc_interface *xch,
+                       uint32_t domid,
+                       uint32_t *nr_vnodes,
+                       uint32_t *nr_vmemranges,
+                       uint32_t *nr_vcpus,
+                       xen_vmemrange_t *vmemrange,
+                       unsigned int *vdistance,
+                       unsigned int *vcpu_to_vnode)
+{
+    int rc;
+    DECLARE_HYPERCALL_BOUNCE(vmemrange, sizeof(*vmemrange) * *nr_vmemranges,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_HYPERCALL_BOUNCE(vdistance, sizeof(*vdistance) *
+                             *nr_vnodes * *nr_vnodes,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+    DECLARE_HYPERCALL_BOUNCE(vcpu_to_vnode, sizeof(*vcpu_to_vnode) * *nr_vcpus,
+                             XC_HYPERCALL_BUFFER_BOUNCE_OUT);
+
+    struct xen_vnuma_topology_info vnuma_topo;
+
+    if ( xc_hypercall_bounce_pre(xch, vmemrange)      ||
+         xc_hypercall_bounce_pre(xch, vdistance)      ||
+         xc_hypercall_bounce_pre(xch, vcpu_to_vnode) )
+    {
+        rc = -1;
+        errno = ENOMEM;
+        goto vnumaget_fail;
+    }
+
+    set_xen_guest_handle(vnuma_topo.vmemrange.h, vmemrange);
+    set_xen_guest_handle(vnuma_topo.vdistance.h, vdistance);
+    set_xen_guest_handle(vnuma_topo.vcpu_to_vnode.h, vcpu_to_vnode);
+
+    vnuma_topo.nr_vnodes = *nr_vnodes;
+    vnuma_topo.nr_vcpus = *nr_vcpus;
+    vnuma_topo.nr_vmemranges = *nr_vmemranges;
+    vnuma_topo.domid = domid;
+    vnuma_topo.pad = 0;
+
+    rc = do_memory_op(xch, XENMEM_get_vnumainfo, &vnuma_topo,
+                      sizeof(vnuma_topo));
+
+    *nr_vnodes = vnuma_topo.nr_vnodes;
+    *nr_vcpus = vnuma_topo.nr_vcpus;
+    *nr_vmemranges = vnuma_topo.nr_vmemranges;
+
+ vnumaget_fail:
+    xc_hypercall_bounce_post(xch, vmemrange);
+    xc_hypercall_bounce_post(xch, vdistance);
+    xc_hypercall_bounce_post(xch, vcpu_to_vnode);
+
+    return rc;
+}
 
 int xc_domain_soft_reset(xc_interface *xch,
                          uint32_t domid)
-- 
2.1.4

Re: [PATCH for 4.6 v4 1/3] libxc: introduce xc_domain_getvnuma

[Wei Liu]

On Fri, Sep 11, 2015 at 02:50:07PM +0100, Wei Liu wrote:
> A simple wrapper for XENMEM_get_vnumainfo.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> ---
> v4: rebase on top of staging

Note that this patch needs some trivial contextual adjustment when being
applied to staging-4.6.

If you need a patch for staging-4.6 I can also provide one.

Wei.

[PATCH for 4.6 v4 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

This is because the migration stream does not preserve node information.

Note this is not a regression for migration v2 vs legacy migration
because neither of them preserve node information.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: andrew.cooper3@citrix.com

v4:
1. Don't differentiate "no vnuma" from "empty vnuma".

v3:
1. Update manpage, code comment and commit message.
2. *Don't* check if nomigrate is set.
---
 docs/man/xl.cfg.pod.5   |  2 ++
 tools/libxl/libxl_dom.c | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index c6345b8..157c855 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -263,6 +263,8 @@ virtual node.
 
 Note that virtual NUMA for PV guest is not yet supported, because
 there is an issue with cpuid handling that affects PV virtual NUMA.
+Further more, guest with virtual NUMA cannot be saved or migrated
+because migration stream does not preserve node information.
 
 Each B<VNODE_SPEC> is a list, which has a form of
 "[VNODE_CONFIG_OPTION,VNODE_CONFIG_OPTION, ... ]"  (without quotes).
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c2518a3..7227f35 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -24,6 +24,7 @@
 #include <xen/hvm/hvm_info_table.h>
 #include <xen/hvm/hvm_xs_strings.h>
 #include <xen/hvm/e820.h>
+#include <xen/errno.h>
 
 libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
 {
@@ -1612,6 +1613,7 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss)
     const libxl_domain_remus_info *const r_info = dss->remus;
     libxl__srm_save_autogen_callbacks *const callbacks =
         &dss->sws.shs.callbacks.save.a;
+    unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
 
     dss->rc = 0;
     logdirty_init(&dss->logdirty);
@@ -1636,6 +1638,20 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss)
           | (debug ? XCFLAGS_DEBUG : 0)
           | (dss->hvm ? XCFLAGS_HVM : 0);
 
+    /* Disallow saving a guest with vNUMA configured because migration
+     * stream does not preserve node information.
+     *
+     * Do not differentiate "no vnuma configuration" from "empty vnuma
+     * configuration".
+     */
+    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, &nr_vmemranges,
+                            &nr_vcpus, NULL, NULL, NULL);
+    if (rc != -1 || errno == XEN_ENOBUFS) {
+        LOG(ERROR, "Cannot save a guest with vNUMA configured");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     dss->guest_evtchn.port = -1;
     dss->guest_evtchn_lockfd = -1;
     dss->guest_responded = 0;
-- 
2.1.4

Re: [PATCH for 4.6 v4 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Ian Campbell]

On Fri, 2015-09-11 at 14:50 +0100, Wei Liu wrote:
> +    /* Disallow saving a guest with vNUMA configured because migration
> +     * stream does not preserve node information.
> +     *
> +     * Do not differentiate "no vnuma configuration" from "empty vnuma
> +     * configuration".
> +     */
> +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, &nr_vmemranges,
> +                            &nr_vcpus, NULL, NULL, NULL);
> +    if (rc != -1 || errno == XEN_ENOBUFS) {

This is not as we discussed, it is neither what I proposed nor what you
(incorrectly IMHO) corrected it to be in the previous thread.

Ian

Re: [PATCH for 4.6 v4 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

On Fri, Sep 11, 2015 at 03:00:35PM +0100, Ian Campbell wrote:
> On Fri, 2015-09-11 at 14:50 +0100, Wei Liu wrote:
> > +    /* Disallow saving a guest with vNUMA configured because migration
> > +     * stream does not preserve node information.
> > +     *
> > +     * Do not differentiate "no vnuma configuration" from "empty vnuma
> > +     * configuration".
> > +     */
> > +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, &nr_vmemranges,
> > +                            &nr_vcpus, NULL, NULL, NULL);
> > +    if (rc != -1 || errno == XEN_ENOBUFS) {
> 
> This is not as we discussed, it is neither what I proposed nor what you
> (incorrectly IMHO) corrected it to be in the previous thread.
> 

To avoid spamming the list I attach the updated version here.

Tested with saving guests with / without vnuma configuration.

---8<---
>From 0c286535e998b62dbe0e35be4c9366fb8dc0934d Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Wed, 9 Sep 2015 17:11:24 +0100
Subject: [PATCH] xl/libxl: disallow saving a guest with vNUMA configured

This is because the migration stream does not preserve node information.

Note this is not a regression for migration v2 vs legacy migration
because neither of them preserve node information.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: andrew.cooper3@citrix.com

v4:
1. Don't differentiate "no vnuma" from "empty vnuma".

v3:
1. Update manpage, code comment and commit message.
2. *Don't* check if nomigrate is set.
---
 docs/man/xl.cfg.pod.5   |  2 ++
 tools/libxl/libxl_dom.c | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index c6345b8..157c855 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -263,6 +263,8 @@ virtual node.
 
 Note that virtual NUMA for PV guest is not yet supported, because
 there is an issue with cpuid handling that affects PV virtual NUMA.
+Further more, guest with virtual NUMA cannot be saved or migrated
+because migration stream does not preserve node information.
 
 Each B<VNODE_SPEC> is a list, which has a form of
 "[VNODE_CONFIG_OPTION,VNODE_CONFIG_OPTION, ... ]"  (without quotes).
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c2518a3..72fce21 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -24,6 +24,7 @@
 #include <xen/hvm/hvm_info_table.h>
 #include <xen/hvm/hvm_xs_strings.h>
 #include <xen/hvm/e820.h>
+#include <xen/errno.h>
 
 libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
 {
@@ -1612,6 +1613,7 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss)
     const libxl_domain_remus_info *const r_info = dss->remus;
     libxl__srm_save_autogen_callbacks *const callbacks =
         &dss->sws.shs.callbacks.save.a;
+    unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
 
     dss->rc = 0;
     logdirty_init(&dss->logdirty);
@@ -1636,6 +1638,20 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss)
           | (debug ? XCFLAGS_DEBUG : 0)
           | (dss->hvm ? XCFLAGS_HVM : 0);
 
+    /* Disallow saving a guest with vNUMA configured because migration
+     * stream does not preserve node information.
+     *
+     * Do not differentiate "no vnuma configuration" from "empty vnuma
+     * configuration".
+     */
+    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, &nr_vmemranges,
+                            &nr_vcpus, NULL, NULL, NULL);
+    if (rc != -1 || errno != XEN_EOPNOTSUPP) {
+        LOG(ERROR, "Cannot save a guest with vNUMA configured");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     dss->guest_evtchn.port = -1;
     dss->guest_evtchn_lockfd = -1;
     dss->guest_responded = 0;
-- 
2.1.4

Re: [PATCH for 4.6 v4 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Ian Campbell]

On Fri, 2015-09-11 at 15:14 +0100, Wei Liu wrote:
@@ -1636,6 +1638,20 @@ void libxl__domain_save(libxl__egc *egc,
> libxl__domain_suspend_state *dss)
>            | (debug ? XCFLAGS_DEBUG : 0)
>            | (dss->hvm ? XCFLAGS_HVM : 0);
>  
> +    /* Disallow saving a guest with vNUMA configured because migration
> +     * stream does not preserve node information.
> +     *
> +     * Do not differentiate "no vnuma configuration" from "empty vnuma
> +     * configuration".
> +     */
> +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, &nr_vmemranges,
> +                            &nr_vcpus, NULL, NULL, NULL);

Sorry for not noticing this before but this is putting a non-libxl error
code in a variable named rc, which is verboten in coding style.

Not least because I think it is now possible to get through this function
successfully without changing it from the rc == -1 which might be assigned
here (in the case where xs_suspend_evtchn_port returns < 0).

Ian.

> +    if (rc != -1 || errno != XEN_EOPNOTSUPP) {
> +        LOG(ERROR, "Cannot save a guest with vNUMA configured");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
>      dss->guest_evtchn.port = -1;
>      dss->guest_evtchn_lockfd = -1;
>      dss->guest_responded = 0;

Re: [PATCH for 4.6 v4 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

On Fri, Sep 11, 2015 at 03:24:21PM +0100, Ian Campbell wrote:
> On Fri, 2015-09-11 at 15:14 +0100, Wei Liu wrote:
> @@ -1636,6 +1638,20 @@ void libxl__domain_save(libxl__egc *egc,
> > libxl__domain_suspend_state *dss)
> >            | (debug ? XCFLAGS_DEBUG : 0)
> >            | (dss->hvm ? XCFLAGS_HVM : 0);
> >  
> > +    /* Disallow saving a guest with vNUMA configured because migration
> > +     * stream does not preserve node information.
> > +     *
> > +     * Do not differentiate "no vnuma configuration" from "empty vnuma
> > +     * configuration".
> > +     */
> > +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, &nr_vmemranges,
> > +                            &nr_vcpus, NULL, NULL, NULL);
> 
> Sorry for not noticing this before but this is putting a non-libxl error
> code in a variable named rc, which is verboten in coding style.
> 

My bad. Should have noticed that earlier.

> Not least because I think it is now possible to get through this function
> successfully without changing it from the rc == -1 which might be assigned
> here (in the case where xs_suspend_evtchn_port returns < 0).
> 
> Ian.

Add a new variable called ret to store return value from xc function
call. Here is the patch.

---8<---
>From c2e9567fa0c5a00405d3759321c9eefb8ec049fc Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Wed, 9 Sep 2015 17:11:24 +0100
Subject: [PATCH] xl/libxl: disallow saving a guest with vNUMA configured

This is because the migration stream does not preserve node information.

Note this is not a regression for migration v2 vs legacy migration
because neither of them preserves node information.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: andrew.cooper3@citrix.com

v4:
1. Don't differentiate "no vnuma" from "empty vnuma".
2. Use ret to store xc function call return value.

v3:
1. Update manpage, code comment and commit message.
2. *Don't* check if nomigrate is set.
---
 docs/man/xl.cfg.pod.5   |  2 ++
 tools/libxl/libxl_dom.c | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index c6345b8..157c855 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -263,6 +263,8 @@ virtual node.
 
 Note that virtual NUMA for PV guest is not yet supported, because
 there is an issue with cpuid handling that affects PV virtual NUMA.
+Further more, guest with virtual NUMA cannot be saved or migrated
+because migration stream does not preserve node information.
 
 Each B<VNODE_SPEC> is a list, which has a form of
 "[VNODE_CONFIG_OPTION,VNODE_CONFIG_OPTION, ... ]"  (without quotes).
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c2518a3..905dc5a 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -24,6 +24,7 @@
 #include <xen/hvm/hvm_info_table.h>
 #include <xen/hvm/hvm_xs_strings.h>
 #include <xen/hvm/e820.h>
+#include <xen/errno.h>
 
 libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid)
 {
@@ -1602,7 +1603,7 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss)
 {
     STATE_AO_GC(dss->ao);
     int port;
-    int rc;
+    int rc, ret;
 
     /* Convenience aliases */
     const uint32_t domid = dss->domid;
@@ -1612,6 +1613,7 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss)
     const libxl_domain_remus_info *const r_info = dss->remus;
     libxl__srm_save_autogen_callbacks *const callbacks =
         &dss->sws.shs.callbacks.save.a;
+    unsigned int nr_vnodes = 0, nr_vmemranges = 0, nr_vcpus = 0;
 
     dss->rc = 0;
     logdirty_init(&dss->logdirty);
@@ -1636,6 +1638,20 @@ void libxl__domain_save(libxl__egc *egc, libxl__domain_suspend_state *dss)
           | (debug ? XCFLAGS_DEBUG : 0)
           | (dss->hvm ? XCFLAGS_HVM : 0);
 
+    /* Disallow saving a guest with vNUMA configured because migration
+     * stream does not preserve node information.
+     *
+     * Do not differentiate "no vnuma configuration" from "empty vnuma
+     * configuration".
+     */
+    ret = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes, &nr_vmemranges,
+                             &nr_vcpus, NULL, NULL, NULL);
+    if (ret != -1 || errno != XEN_EOPNOTSUPP) {
+        LOG(ERROR, "Cannot save a guest with vNUMA configured");
+        rc = ERROR_FAIL;
+        goto out;
+    }
+
     dss->guest_evtchn.port = -1;
     dss->guest_evtchn_lockfd = -1;
     dss->guest_responded = 0;
-- 
2.1.4

Re: [PATCH for 4.6 v4 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Ian Campbell]

On Fri, 2015-09-11 at 15:31 +0100, Wei Liu wrote:
> On Fri, Sep 11, 2015 at 03:24:21PM +0100, Ian Campbell wrote:
> > On Fri, 2015-09-11 at 15:14 +0100, Wei Liu wrote:
> > @@ -1636,6 +1638,20 @@ void libxl__domain_save(libxl__egc *egc,
> > > libxl__domain_suspend_state *dss)
> > >            | (debug ? XCFLAGS_DEBUG : 0)
> > >            | (dss->hvm ? XCFLAGS_HVM : 0);
> > >  
> > > +    /* Disallow saving a guest with vNUMA configured because
> > > migration
> > > +     * stream does not preserve node information.
> > > +     *
> > > +     * Do not differentiate "no vnuma configuration" from "empty
> > > vnuma
> > > +     * configuration".
> > > +     */
> > > +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes,
> > > &nr_vmemranges,
> > > +                            &nr_vcpus, NULL, NULL, NULL);
> > 
> > Sorry for not noticing this before but this is putting a non-libxl
> > error
> > code in a variable named rc, which is verboten in coding style.
> > 
> 
> My bad. Should have noticed that earlier.
> 
> > Not least because I think it is now possible to get through this
> > function
> > successfully without changing it from the rc == -1 which might be
> > assigned
> > here (in the case where xs_suspend_evtchn_port returns < 0).
> > 
> > Ian.
> 
> Add a new variable called ret to store return value from xc function
> call. Here is the patch.
> 
> ---8<---
> From c2e9567fa0c5a00405d3759321c9eefb8ec049fc Mon Sep 17 00:00:00 2001
> From: Wei Liu <wei.liu2@citrix.com>
> Date: Wed, 9 Sep 2015 17:11:24 +0100
> Subject: [PATCH] xl/libxl: disallow saving a guest with vNUMA configured
> 
> This is because the migration stream does not preserve node information.
> 
> Note this is not a regression for migration v2 vs legacy migration
> because neither of them preserves node information.
> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>

> ---
> Cc: andrew.cooper3@citrix.com
> 
> v4:
> 1. Don't differentiate "no vnuma" from "empty vnuma".
> 2. Use ret to store xc function call return value.
> 
> v3:
> 1. Update manpage, code comment and commit message.
> 2. *Don't* check if nomigrate is set.
> ---
>  docs/man/xl.cfg.pod.5   |  2 ++
>  tools/libxl/libxl_dom.c | 18 +++++++++++++++++-
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index c6345b8..157c855 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -263,6 +263,8 @@ virtual node.
>  
>  Note that virtual NUMA for PV guest is not yet supported, because
>  there is an issue with cpuid handling that affects PV virtual NUMA.
> +Further more, guest with virtual NUMA cannot be saved or migrated

I _think_ (but am not 100% sure) that in the sense you mean it is
"Furthermore". I don't think "Further more," actually means anything.

I can fix as I commit.

@@ -1636,6 +1638,20 @@ void libxl__domain_save(libxl__egc *egc,
> libxl__domain_suspend_state *dss)
>            | (debug ? XCFLAGS_DEBUG : 0)
>            | (dss->hvm ? XCFLAGS_HVM : 0);
>  
> +    /* Disallow saving a guest with vNUMA configured because migration
> +     * stream does not preserve node information.
> +     *
> +     * Do not differentiate "no vnuma configuration" from "empty vnuma
> +     * configuration".

Actually, we do differentiate, since we are checking for one explicitly.
What we are not differentiating is "vnuma enabled and configured" vs "numa
enabled but not configured", or something.

How about:

* Reject any domain which has vnuma enabled, even if the configuration is 
* empty. Only domains which have no vnuma configuration at all are 
* supported.
*/

as the second paragraph of the comment?

I can do that on commit too.

Ian.

Re: [PATCH for 4.6 v4 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

On Fri, Sep 11, 2015 at 04:05:13PM +0100, Ian Campbell wrote:
> On Fri, 2015-09-11 at 15:31 +0100, Wei Liu wrote:
> > On Fri, Sep 11, 2015 at 03:24:21PM +0100, Ian Campbell wrote:
> > > On Fri, 2015-09-11 at 15:14 +0100, Wei Liu wrote:
> > > @@ -1636,6 +1638,20 @@ void libxl__domain_save(libxl__egc *egc,
> > > > libxl__domain_suspend_state *dss)
> > > >            | (debug ? XCFLAGS_DEBUG : 0)
> > > >            | (dss->hvm ? XCFLAGS_HVM : 0);
> > > >  
> > > > +    /* Disallow saving a guest with vNUMA configured because
> > > > migration
> > > > +     * stream does not preserve node information.
> > > > +     *
> > > > +     * Do not differentiate "no vnuma configuration" from "empty
> > > > vnuma
> > > > +     * configuration".
> > > > +     */
> > > > +    rc = xc_domain_getvnuma(CTX->xch, domid, &nr_vnodes,
> > > > &nr_vmemranges,
> > > > +                            &nr_vcpus, NULL, NULL, NULL);
> > > 
> > > Sorry for not noticing this before but this is putting a non-libxl
> > > error
> > > code in a variable named rc, which is verboten in coding style.
> > > 
> > 
> > My bad. Should have noticed that earlier.
> > 
> > > Not least because I think it is now possible to get through this
> > > function
> > > successfully without changing it from the rc == -1 which might be
> > > assigned
> > > here (in the case where xs_suspend_evtchn_port returns < 0).
> > > 
> > > Ian.
> > 
> > Add a new variable called ret to store return value from xc function
> > call. Here is the patch.
> > 
> > ---8<---
> > From c2e9567fa0c5a00405d3759321c9eefb8ec049fc Mon Sep 17 00:00:00 2001
> > From: Wei Liu <wei.liu2@citrix.com>
> > Date: Wed, 9 Sep 2015 17:11:24 +0100
> > Subject: [PATCH] xl/libxl: disallow saving a guest with vNUMA configured
> > 
> > This is because the migration stream does not preserve node information.
> > 
> > Note this is not a regression for migration v2 vs legacy migration
> > because neither of them preserves node information.
> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> > ---
> > Cc: andrew.cooper3@citrix.com
> > 
> > v4:
> > 1. Don't differentiate "no vnuma" from "empty vnuma".
> > 2. Use ret to store xc function call return value.
> > 
> > v3:
> > 1. Update manpage, code comment and commit message.
> > 2. *Don't* check if nomigrate is set.
> > ---
> >  docs/man/xl.cfg.pod.5   |  2 ++
> >  tools/libxl/libxl_dom.c | 18 +++++++++++++++++-
> >  2 files changed, 19 insertions(+), 1 deletion(-)
> > 
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index c6345b8..157c855 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -263,6 +263,8 @@ virtual node.
> >  
> >  Note that virtual NUMA for PV guest is not yet supported, because
> >  there is an issue with cpuid handling that affects PV virtual NUMA.
> > +Further more, guest with virtual NUMA cannot be saved or migrated
> 
> I _think_ (but am not 100% sure) that in the sense you mean it is
> "Furthermore". I don't think "Further more," actually means anything.
> 
> I can fix as I commit.
> 

Yes please.

> @@ -1636,6 +1638,20 @@ void libxl__domain_save(libxl__egc *egc,
> > libxl__domain_suspend_state *dss)
> >            | (debug ? XCFLAGS_DEBUG : 0)
> >            | (dss->hvm ? XCFLAGS_HVM : 0);
> >  
> > +    /* Disallow saving a guest with vNUMA configured because migration
> > +     * stream does not preserve node information.
> > +     *
> > +     * Do not differentiate "no vnuma configuration" from "empty vnuma
> > +     * configuration".
> 
> Actually, we do differentiate, since we are checking for one explicitly.
> What we are not differentiating is "vnuma enabled and configured" vs "numa
> enabled but not configured", or something.
> 
> How about:
> 
> * Reject any domain which has vnuma enabled, even if the configuration is 
> * empty. Only domains which have no vnuma configuration at all are 
> * supported.
> */
> 
> as the second paragraph of the comment?
> 

OK.

> I can do that on commit too.
> 

That would be good.

Wei.

> Ian.

Re: [PATCH for 4.6 v4 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Ian Campbell]

On Fri, 2015-09-11 at 16:09 +0100, Wei Liu wrote:

> > >  Note that virtual NUMA for PV guest is not yet supported, because
> > >  there is an issue with cpuid handling that affects PV virtual NUMA.
> > > +Further more, guest with virtual NUMA cannot be saved or migrated
> > 
> > I _think_ (but am not 100% sure) that in the sense you mean it is
> > "Furthermore". I don't think "Further more," actually means anything.
> > 
> > I can fix as I commit.
> > 
> 
> Yes please.

I went with:
+Furthermore, guests with virtual NUMA cannot be saved or migrated
+because the migration stream does not preserve node information.

I've applied all three patches in this series to staging and 4.6. The
conflict you mentioned elsewhere (patch #1) was just the presence of
 xc_domain_soft_reset in staging but not staging-4.6. I resolved it, I
don't think I can have gotten it wrong, but do check ;-)

Ian.

Re: [PATCH for 4.6 v4 2/3] xl/libxl: disallow saving a guest with vNUMA configured

[Wei Liu]

On Fri, Sep 11, 2015 at 04:53:57PM +0100, Ian Campbell wrote:
> On Fri, 2015-09-11 at 16:09 +0100, Wei Liu wrote:
> 
> > > >  Note that virtual NUMA for PV guest is not yet supported, because
> > > >  there is an issue with cpuid handling that affects PV virtual NUMA.
> > > > +Further more, guest with virtual NUMA cannot be saved or migrated
> > > 
> > > I _think_ (but am not 100% sure) that in the sense you mean it is
> > > "Furthermore". I don't think "Further more," actually means anything.
> > > 
> > > I can fix as I commit.
> > > 
> > 
> > Yes please.
> 
> I went with:
> +Furthermore, guests with virtual NUMA cannot be saved or migrated
> +because the migration stream does not preserve node information.
> 
> I've applied all three patches in this series to staging and 4.6. The
> conflict you mentioned elsewhere (patch #1) was just the presence of
>  xc_domain_soft_reset in staging but not staging-4.6. I resolved it, I
> don't think I can have gotten it wrong, but do check ;-)
> 

The backported patch looks correct. Thanks.

> Ian.

[PATCH for 4.6 v4 3/3] xl: handle empty vnuma configuration

[Wei Liu]

When user specifies vnuma = [], we need to skip the whole parser
function, otherwise the parser sets b_info->max_memkb to garbage value.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/xl_cmdimpl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index c9bd839..bfbd421 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -1093,6 +1093,9 @@ static void parse_vnuma_config(const XLU_Config *config,
     if (xlu_cfg_get_list(config, "vnuma", &vnuma, &num_vnuma, 1))
         return;
 
+    if (!num_vnuma)
+        return;
+
     b_info->num_vnuma_nodes = num_vnuma;
     b_info->vnuma_nodes = xcalloc(num_vnuma, sizeof(libxl_vnode_info));
     vcpu_parsed = xcalloc(num_vnuma, sizeof(libxl_bitmap));
-- 
2.1.4