[PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3227 bytes --]

Said commit ("libxl_set_memory_target: retain the same maxmem offset on
top of the current target") caused a regression for "xl mem-set"
against Dom0: While prior to creation of the first domain this works,
the first domain creation involving ballooning breaks. Due to "enforce"
not being set in the domain creation case, and due to Dom0's initial
->max_pages (in the hypervisor) being UINT_MAX, the calculation of
"memorykb" in the first "xl mem-set" adusting the target upwards
subsequent to domain creation and termination may cause an overflow,
resulting in Dom0's maximum getting to a very small value. This small
maximum will the make the subsequent setting of the PoD target fail.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note that this only fixes the immediate problem - there appear to be
further issues lurking here:
- libxl_set_memory_target()'s *_memkb variables all being 32-bit,
- libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
- other similar code living elsewhere?
Note also that this requires
http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg02485.html 
(or some other change avoiding truncation) to also be in place in order
to address the observed problem.
Note further that xc_domain_setmaxmem() is being used by upstream qemu
and hence the libxc interface change here may represent a compatibility
issue.
Finally the setting of a PoD target for non-HVM domains seems bogus too
(even if it's expected to just be a no-op in that case).

--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1256,7 +1256,7 @@ int xc_getcpuinfo(xc_interface *xch, int
 
 int xc_domain_setmaxmem(xc_interface *xch,
                         uint32_t domid,
-                        unsigned int max_memkb);
+                        uint64_t max_memkb);
 
 int xc_domain_set_memmap_limit(xc_interface *xch,
                                uint32_t domid,
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -634,7 +634,7 @@ int xc_shadow_control(xc_interface *xch,
 
 int xc_domain_setmaxmem(xc_interface *xch,
                         uint32_t domid,
-                        unsigned int max_memkb)
+                        uint64_t max_memkb)
 {
     DECLARE_DOMCTL;
     domctl.cmd = XEN_DOMCTL_max_mem;
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4726,7 +4726,8 @@ int libxl_set_memory_target(libxl_ctx *c
 {
     GC_INIT(ctx);
     int rc = 1, abort_transaction = 0;
-    uint32_t memorykb = 0, videoram = 0;
+    uint64_t memorykb;
+    uint32_t videoram = 0;
     uint32_t current_target_memkb = 0, new_target_memkb = 0;
     uint32_t current_max_memkb = 0;
     char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
@@ -4820,7 +4821,7 @@ retry_transaction:
         rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
         if (rc != 0) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
-                    "xc_domain_setmaxmem domid=%d memkb=%d failed "
+                    "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed "
                     "rc=%d\n", domid, memorykb, rc);
             abort_transaction = 1;
             goto out;




[-- Attachment #2: libxl-mem-set-regression.patch --]
[-- Type: text/plain, Size: 3274 bytes --]

libxl: fix "xl mem-set" regression from 0c029c4da2

Said commit ("libxl_set_memory_target: retain the same maxmem offset on
top of the current target") caused a regression for "xl mem-set"
against Dom0: While prior to creation of the first domain this works,
the first domain creation involving ballooning breaks. Due to "enforce"
not being set in the domain creation case, and due to Dom0's initial
->max_pages (in the hypervisor) being UINT_MAX, the calculation of
"memorykb" in the first "xl mem-set" adusting the target upwards
subsequent to domain creation and termination may cause an overflow,
resulting in Dom0's maximum getting to a very small value. This small
maximum will the make the subsequent setting of the PoD target fail.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Note that this only fixes the immediate problem - there appear to be
further issues lurking here:
- libxl_set_memory_target()'s *_memkb variables all being 32-bit,
- libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
- other similar code living elsewhere?
Note also that this requires
http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg02485.html
(or some other change avoiding truncation) to also be in place in order
to address the observed problem.
Note further that xc_domain_setmaxmem() is being used by upstream qemu
and hence the libxc interface change here may represent a compatibility
issue.
Finally the setting of a PoD target for non-HVM domains seems bogus too
(even if it's expected to just be a no-op in that case).

--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1256,7 +1256,7 @@ int xc_getcpuinfo(xc_interface *xch, int
 
 int xc_domain_setmaxmem(xc_interface *xch,
                         uint32_t domid,
-                        unsigned int max_memkb);
+                        uint64_t max_memkb);
 
 int xc_domain_set_memmap_limit(xc_interface *xch,
                                uint32_t domid,
--- a/tools/libxc/xc_domain.c
+++ b/tools/libxc/xc_domain.c
@@ -634,7 +634,7 @@ int xc_shadow_control(xc_interface *xch,
 
 int xc_domain_setmaxmem(xc_interface *xch,
                         uint32_t domid,
-                        unsigned int max_memkb)
+                        uint64_t max_memkb)
 {
     DECLARE_DOMCTL;
     domctl.cmd = XEN_DOMCTL_max_mem;
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4726,7 +4726,8 @@ int libxl_set_memory_target(libxl_ctx *c
 {
     GC_INIT(ctx);
     int rc = 1, abort_transaction = 0;
-    uint32_t memorykb = 0, videoram = 0;
+    uint64_t memorykb;
+    uint32_t videoram = 0;
     uint32_t current_target_memkb = 0, new_target_memkb = 0;
     uint32_t current_max_memkb = 0;
     char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
@@ -4820,7 +4821,7 @@ retry_transaction:
         rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
         if (rc != 0) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
-                    "xc_domain_setmaxmem domid=%d memkb=%d failed "
+                    "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed "
                     "rc=%d\n", domid, memorykb, rc);
             abort_transaction = 1;
             goto out;

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

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

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Stefano Stabellini]

On Wed, 22 Apr 2015, Jan Beulich wrote:
> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
> top of the current target") caused a regression for "xl mem-set"
> against Dom0: While prior to creation of the first domain this works,
> the first domain creation involving ballooning breaks. Due to "enforce"
> not being set in the domain creation case, and due to Dom0's initial
> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
> "memorykb" in the first "xl mem-set" adusting the target upwards
> subsequent to domain creation and termination may cause an overflow,
> resulting in Dom0's maximum getting to a very small value. This small
> maximum will the make the subsequent setting of the PoD target fail.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Note that this only fixes the immediate problem - there appear to be
> further issues lurking here:
> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
> - other similar code living elsewhere?
> Note also that this requires
> http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg02485.html 
> (or some other change avoiding truncation) to also be in place in order
> to address the observed problem.
> Note further that xc_domain_setmaxmem() is being used by upstream qemu
> and hence the libxc interface change here may represent a compatibility
> issue.
> Finally the setting of a PoD target for non-HVM domains seems bogus too
> (even if it's expected to just be a no-op in that case).
> 
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1256,7 +1256,7 @@ int xc_getcpuinfo(xc_interface *xch, int
>  
>  int xc_domain_setmaxmem(xc_interface *xch,
>                          uint32_t domid,
> -                        unsigned int max_memkb);
> +                        uint64_t max_memkb);
>  
>  int xc_domain_set_memmap_limit(xc_interface *xch,
>                                 uint32_t domid,
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -634,7 +634,7 @@ int xc_shadow_control(xc_interface *xch,
>  
>  int xc_domain_setmaxmem(xc_interface *xch,
>                          uint32_t domid,
> -                        unsigned int max_memkb)
> +                        uint64_t max_memkb)
>  {
>      DECLARE_DOMCTL;
>      domctl.cmd = XEN_DOMCTL_max_mem;
>
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4726,7 +4726,8 @@ int libxl_set_memory_target(libxl_ctx *c
>  {
>      GC_INIT(ctx);
>      int rc = 1, abort_transaction = 0;
> -    uint32_t memorykb = 0, videoram = 0;
> +    uint64_t memorykb;
> +    uint32_t videoram = 0;
>      uint32_t current_target_memkb = 0, new_target_memkb = 0;
>      uint32_t current_max_memkb = 0;
>      char *memmax, *endptr, *videoram_s = NULL, *target = NULL;

>From the description of the problem above, we have two issues:

1) we don't detect that maxmem is already UINT_MAX*4, so we shouldn't try
   to increase it

2) unsigned int / uint64_t mismatch


1) is pretty easy and might just come down to one more if statement in
libxl_set_memory_target. Something like:


#define MAXMEM_MAX_KB ((uint64_t)UINT32_MAX * 4)
if ((new_target_memkb - current_target_memkb) > 0 && 
    (ptr.max_memkb - new_target_memkb + current_target_memkb) < ptr.max_memkb)
{
    /* avoid overflow */
   rc = xc_domain_setmaxmem(ctx->xch, domid, MAXMEM_MAX_KB);
} else {
   rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
}


2) is difficult as we have unsigned int in many many places.  Not only
we need to fix the libxc interface, but as far as I can tell we also
need to fix at least libxl_set_memory_target,
libxl__fill_dom0_memory_info, libxl__get_memory_target, all the callers.
This is the minimum set of changes I think are required:


diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 511eef1..3e91203 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -4641,8 +4641,8 @@ out:
     return rc;
 }
 
-static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint32_t *target_memkb,
-                                        uint32_t *max_memkb)
+static int libxl__fill_dom0_memory_info(libxl__gc *gc, uint64_t *target_memkb,
+                                        uint64_t *max_memkb)
 {
     int rc;
     libxl_dominfo info;
@@ -4666,7 +4666,7 @@ retry_transaction:
     }
 
     if (target) {
-        *target_memkb = strtoul(target, &endptr, 10);
+        *target_memkb = strtoull(target, &endptr, 10);
         if (*endptr != '\0') {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
                     "invalid memory target %s from %s\n", target, target_path);
@@ -4676,7 +4676,7 @@ retry_transaction:
     }
 
     if (staticmax) {
-        *max_memkb = strtoul(staticmax, &endptr, 10);
+        *max_memkb = strtoull(staticmax, &endptr, 10);
         if (*endptr != '\0') {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
                              "invalid memory static-max %s from %s\n",
@@ -4697,14 +4697,14 @@ retry_transaction:
         goto out;
 
     if (target == NULL) {
-        libxl__xs_write(gc, t, target_path, "%"PRIu32,
-                (uint32_t) info.current_memkb);
-        *target_memkb = (uint32_t) info.current_memkb;
+        libxl__xs_write(gc, t, target_path, "%"PRIu64,
+                info.current_memkb);
+        *target_memkb = info.current_memkb;
     }
     if (staticmax == NULL) {
-        libxl__xs_write(gc, t, max_path, "%"PRIu32,
-                        (uint32_t) info.max_memkb);
-        *max_memkb = (uint32_t) info.max_memkb;
+        libxl__xs_write(gc, t, max_path, "%"PRIu64,
+                        info.max_memkb);
+        *max_memkb = info.max_memkb;
     }
 
     rc = 0;
@@ -4726,9 +4726,9 @@ int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid,
 {
     GC_INIT(ctx);
     int rc = 1, abort_transaction = 0;
-    uint32_t memorykb = 0, videoram = 0;
-    uint32_t current_target_memkb = 0, new_target_memkb = 0;
-    uint32_t current_max_memkb = 0;
+    uint64_t memorykb = 0, videoram = 0;
+    uint64_t current_target_memkb = 0, new_target_memkb = 0;
+    uint64_t current_max_memkb = 0;
     char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
     char *dompath = libxl__xs_get_dompath(gc, domid);
     libxl_dominfo ptr;
@@ -4761,7 +4761,7 @@ retry_transaction:
         abort_transaction = 1;
         goto out;
     } else {
-        current_target_memkb = strtoul(target, &endptr, 10);
+        current_target_memkb = strtoull(target, &endptr, 10);
         if (*endptr != '\0') {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
                     "invalid memory target %s from %s/memory/target\n",
@@ -4779,7 +4779,7 @@ retry_transaction:
         abort_transaction = 1;
         goto out;
     }
-    memorykb = strtoul(memmax, &endptr, 10);
+    memorykb = strtoull(memmax, &endptr, 10);
     if (*endptr != '\0') {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
                 "invalid max memory %s from %s/memory/static-max\n",
@@ -4790,7 +4790,17 @@ retry_transaction:
 
     videoram_s = libxl__xs_read(gc, t, libxl__sprintf(gc,
                 "%s/memory/videoram", dompath));
-    videoram = videoram_s ? atoi(videoram_s) : 0;
+    if (!videoram_s) {
+        videoram = 0;
+    } else {
+        videoram = strtoull(videoram_s, &endptr, 10);
+        if (*endptr != '\0') {
+            LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
+                    "invalid videoram %s\n", videoram_s);
+            abort_transaction = 1;
+            goto out;
+        }
+    }
 
     if (relative) {
         if (target_memkb < 0 && abs(target_memkb) > current_target_memkb)
@@ -4809,7 +4819,7 @@ retry_transaction:
 
     if (!domid && new_target_memkb < LIBXL_MIN_DOM0_MEM) {
         LIBXL__LOG(ctx, LIBXL__LOG_ERROR,
-                "new target %d for dom0 is below the minimum threshold\n",
+                "new target %"PRIu64" for dom0 is below the minimum threshold\n",
                  new_target_memkb);
         abort_transaction = 1;
         goto out;
@@ -4820,7 +4830,7 @@ retry_transaction:
         rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
         if (rc != 0) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
-                    "xc_domain_setmaxmem domid=%d memkb=%d failed "
+                    "xc_domain_setmaxmem domid=%d memkb=%"PRIu64" failed "
                     "rc=%d\n", domid, memorykb, rc);
             abort_transaction = 1;
             goto out;
@@ -4831,7 +4841,7 @@ retry_transaction:
             new_target_memkb / 4, NULL, NULL, NULL);
     if (rc != 0) {
         LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
-                "xc_domain_set_pod_target domid=%d, memkb=%d "
+                "xc_domain_set_pod_target domid=%d, memkb=%"PRIu64" "
                 "failed rc=%d\n", domid, new_target_memkb / 4,
                 rc);
         abort_transaction = 1;
@@ -4839,10 +4849,10 @@ retry_transaction:
     }
 
     libxl__xs_write(gc, t, libxl__sprintf(gc, "%s/memory/target",
-                dompath), "%"PRIu32, new_target_memkb);
+                dompath), "%"PRIu64, new_target_memkb);
 
     libxl__xs_write(gc, t, libxl__sprintf(gc, "/vm/%s/memory", uuid),
-            "%"PRIu32, new_target_memkb / 1024);
+            "%"PRIu64, new_target_memkb / 1024);
 
 out:
     if (!xs_transaction_end(ctx->xsh, t, abort_transaction)
@@ -4858,13 +4868,13 @@ out_no_transaction:
 
 /* out_target_memkb and out_max_memkb can be NULL */
 static int libxl__get_memory_target(libxl__gc *gc, uint32_t domid,
-                                    uint32_t *out_target_memkb,
-                                    uint32_t *out_max_memkb)
+                                    uint64_t *out_target_memkb,
+                                    uint64_t *out_max_memkb)
 {
     int rc;
     char *target = NULL, *static_max = NULL, *endptr = NULL;
     char *dompath = libxl__xs_get_dompath(gc, domid);
-    uint32_t target_memkb, max_memkb;
+    uint64_t target_memkb, max_memkb;
 
     target = libxl__xs_read(gc, XBT_NULL, libxl__sprintf(gc,
                     "%s/memory/target", dompath));
@@ -4888,14 +4898,14 @@ static int libxl__get_memory_target(libxl__gc *gc, uint32_t domid,
                 dompath);
         goto out;
     } else {
-        target_memkb = strtoul(target, &endptr, 10);
+        target_memkb = strtoull(target, &endptr, 10);
         if (*endptr != '\0') {
             LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
                     "invalid memory target %s from %s/memory/target\n",
                     target, dompath);
             goto out;
         }
-        max_memkb = strtoul(static_max, &endptr, 10);
+        max_memkb = strtoull(static_max, &endptr, 10);
         if (*endptr != '\0') {
             LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
                     "invalid memory target %s from %s/memory/static-max\n",
@@ -4918,7 +4928,7 @@ out:
 }
 
 int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid,
-                            uint32_t *out_target)
+                            uint64_t *out_target)
 {
     GC_INIT(ctx);
     int rc;
@@ -5006,7 +5016,7 @@ out:
 int libxl_wait_for_memory_target(libxl_ctx *ctx, uint32_t domid, int wait_secs)
 {
     int rc = 0;
-    uint32_t target_memkb = 0;
+    uint64_t target_memkb = 0;
     uint64_t current_memkb, prev_memkb;
     libxl_dominfo info;
 
@@ -6652,7 +6662,7 @@ int libxl_retrieve_domain_configuration(libxl_ctx *ctx, uint32_t domid,
      * "target" and "static-max".
      */
     {
-        uint32_t target_memkb = 0, max_memkb = 0;
+        uint64_t target_memkb = 0, max_memkb = 0;
 
         /* "target" */
         rc = libxl__get_memory_target(gc, domid, &target_memkb, &max_memkb);
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 6bc75c5..3b2daa8 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1047,7 +1047,7 @@ int libxl_domain_core_dump(libxl_ctx *ctx, uint32_t domid,
 
 int libxl_domain_setmaxmem(libxl_ctx *ctx, uint32_t domid, uint32_t target_memkb);
 int libxl_set_memory_target(libxl_ctx *ctx, uint32_t domid, int32_t target_memkb, int relative, int enforce);
-int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint32_t *out_target);
+int libxl_get_memory_target(libxl_ctx *ctx, uint32_t domid, uint64_t *out_target);
 
 
 /*

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Jan Beulich]

>>> On 22.04.15 at 15:57, <stefano.stabellini@eu.citrix.com> wrote:
> From the description of the problem above, we have two issues:
> 
> 1) we don't detect that maxmem is already UINT_MAX*4, so we shouldn't try
>    to increase it
> 
> 2) unsigned int / uint64_t mismatch
> 
> 
> 1) is pretty easy and might just come down to one more if statement in
> libxl_set_memory_target. Something like:
> 
> 
> #define MAXMEM_MAX_KB ((uint64_t)UINT32_MAX * 4)
> if ((new_target_memkb - current_target_memkb) > 0 && 
>     (ptr.max_memkb - new_target_memkb + current_target_memkb) < ptr.max_memkb)
> {
>     /* avoid overflow */
>    rc = xc_domain_setmaxmem(ctx->xch, domid, MAXMEM_MAX_KB);
> } else {
>    rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
> }

Which would build into the tool stack an assumption that the
hypervisor representation is a 32-bit value worth of pages. Not
really desirable imo. If anything we'd need to do actual overflow
checking (and saturation, similar to what the hypervisor patch
does) here too.

> 2) is difficult as we have unsigned int in many many places.  Not only
> we need to fix the libxc interface, but as far as I can tell we also
> need to fix at least libxl_set_memory_target,
> libxl__fill_dom0_memory_info, libxl__get_memory_target, all the callers.
> This is the minimum set of changes I think are required:

I'll leave that to the tool stack maintainers to decide upon. All I
really need immediately is that "xl mem-set" works again.

Jan

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Stefano Stabellini]

On Wed, 22 Apr 2015, Jan Beulich wrote:
> >>> On 22.04.15 at 15:57, <stefano.stabellini@eu.citrix.com> wrote:
> > From the description of the problem above, we have two issues:
> > 
> > 1) we don't detect that maxmem is already UINT_MAX*4, so we shouldn't try
> >    to increase it
> > 
> > 2) unsigned int / uint64_t mismatch
> > 
> > 
> > 1) is pretty easy and might just come down to one more if statement in
> > libxl_set_memory_target. Something like:
> > 
> > 
> > #define MAXMEM_MAX_KB ((uint64_t)UINT32_MAX * 4)
> > if ((new_target_memkb - current_target_memkb) > 0 && 
> >     (ptr.max_memkb - new_target_memkb + current_target_memkb) < ptr.max_memkb)
> > {
> >     /* avoid overflow */
> >    rc = xc_domain_setmaxmem(ctx->xch, domid, MAXMEM_MAX_KB);
> > } else {
> >    rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
> > }
> 
> Which would build into the tool stack an assumption that the
> hypervisor representation is a 32-bit value worth of pages. Not
> really desirable imo. If anything we'd need to do actual overflow
> checking (and saturation, similar to what the hypervisor patch
> does) here too.

Sure, as long as Xen is able to handle maxmem arguments > UINT32_MAX *
4, that it doesn't look like is the case at the moment.



> > 2) is difficult as we have unsigned int in many many places.  Not only
> > we need to fix the libxc interface, but as far as I can tell we also
> > need to fix at least libxl_set_memory_target,
> > libxl__fill_dom0_memory_info, libxl__get_memory_target, all the callers.
> > This is the minimum set of changes I think are required:
> 
> I'll leave that to the tool stack maintainers to decide upon. All I
> really need immediately is that "xl mem-set" works again.

What I am saying is that, as you probably know, this patch is not enough
to get it right.

I have now seen your hypervisor side patch (it would have been better
to have them in a series so that it would have been obvious that they
need each other).

Even after your hypervisor patch and this small libxl change, xl mem-set
might work in your scenario, but libxl still doesn't have the right
behaviour: libxl is unintentionally causing overflows.
xc_domain_setmaxmem should rightfully return errors in those cases, and
libxl would abort the operation. This is not what we want, right?

I think that at the very least you need to add overflow checking in
libxl_set_memory_target.


The other changes I suggested come from:

memorykb = ptr.max_memkb - current_target_memkb + new_target_memkb;

memorykb and ptr.max_memkb might both be uint64_t with your change, but
current_target_memkb and new_target_memkb are not. There could still be
overflow there. However I do understand that they actually refer to the
memory target, not maxmem, so they could be fixed separately. If you
don't want to handle that in your series, that's fine by me.

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Ian Campbell]

On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote:
> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
> top of the current target") caused a regression for "xl mem-set"
> against Dom0: While prior to creation of the first domain this works,
> the first domain creation involving ballooning breaks. Due to "enforce"
> not being set in the domain creation case, and due to Dom0's initial
> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
> "memorykb" in the first "xl mem-set" adusting the target upwards
> subsequent to domain creation and termination may cause an overflow,
> resulting in Dom0's maximum getting to a very small value. This small
> maximum will the make the subsequent setting of the PoD target fail.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Note that this only fixes the immediate problem - there appear to be
> further issues lurking here:
> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,

I think that increasing the width of these variables wouldn't break the
API guarantee which we make, at least not in a practical way, since any
existing 32-bit arguments passed will just get promoted.

It breaks ABI but we don't guarantee that.

> - other similar code living elsewhere?
> Note also that this requires
> http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg02485.html 
> (or some other change avoiding truncation) to also be in place in order
> to address the observed problem.
> Note further that xc_domain_setmaxmem() is being used by upstream qemu
> and hence the libxc interface change here may represent a compatibility
> issue.

This might have been a problem wrt getting through the respective push
gates in the right order, but actually doesn't automatic type promotion
from unsigned int to uint64_t save us here too?

> Finally the setting of a PoD target for non-HVM domains seems bogus too
> (even if it's expected to just be a no-op in that case).
> 
> --- a/tools/libxc/include/xenctrl.h
> +++ b/tools/libxc/include/xenctrl.h
> @@ -1256,7 +1256,7 @@ int xc_getcpuinfo(xc_interface *xch, int
>  
>  int xc_domain_setmaxmem(xc_interface *xch,
>                          uint32_t domid,
> -                        unsigned int max_memkb);
> +                        uint64_t max_memkb);
>  
>  int xc_domain_set_memmap_limit(xc_interface *xch,
>                                 uint32_t domid,
> --- a/tools/libxc/xc_domain.c
> +++ b/tools/libxc/xc_domain.c
> @@ -634,7 +634,7 @@ int xc_shadow_control(xc_interface *xch,
>  
>  int xc_domain_setmaxmem(xc_interface *xch,
>                          uint32_t domid,
> -                        unsigned int max_memkb)
> +                        uint64_t max_memkb)
>  {
>      DECLARE_DOMCTL;
>      domctl.cmd = XEN_DOMCTL_max_mem;
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4726,7 +4726,8 @@ int libxl_set_memory_target(libxl_ctx *c
>  {
>      GC_INIT(ctx);
>      int rc = 1, abort_transaction = 0;
> -    uint32_t memorykb = 0, videoram = 0;
> +    uint64_t memorykb;
> +    uint32_t videoram = 0;
>      uint32_t current_target_memkb = 0, new_target_memkb = 0;
>      uint32_t current_max_memkb = 0;
>      char *memmax, *endptr, *videoram_s = NULL, *target = NULL;
> @@ -4820,7 +4821,7 @@ retry_transaction:
>          rc = xc_domain_setmaxmem(ctx->xch, domid, memorykb);
>          if (rc != 0) {
>              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR,
> -                    "xc_domain_setmaxmem domid=%d memkb=%d failed "
> +                    "xc_domain_setmaxmem domid=%u memkb=%"PRIu64" failed "
>                      "rc=%d\n", domid, memorykb, rc);
>              abort_transaction = 1;
>              goto out;
> 
> 
> 

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Jan Beulich]

>>> On 22.04.15 at 16:01, <ian.campbell@citrix.com> wrote:
> On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote:
>> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
>> top of the current target") caused a regression for "xl mem-set"
>> against Dom0: While prior to creation of the first domain this works,
>> the first domain creation involving ballooning breaks. Due to "enforce"
>> not being set in the domain creation case, and due to Dom0's initial
>> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
>> "memorykb" in the first "xl mem-set" adusting the target upwards
>> subsequent to domain creation and termination may cause an overflow,
>> resulting in Dom0's maximum getting to a very small value. This small
>> maximum will the make the subsequent setting of the PoD target fail.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Note that this only fixes the immediate problem - there appear to be
>> further issues lurking here:
>> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
>> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
> 
> I think that increasing the width of these variables wouldn't break the
> API guarantee which we make, at least not in a practical way, since any
> existing 32-bit arguments passed will just get promoted.

No, not even on 64-bit. On 32-bit, two arguments slots are needed
for what so far requires only one. On 64-bit (at least x86-64), the
calling code isn't required to zero-extend a value calculated in a
register (e.g. a result of earlier calculations which had more than
32 significant bits could be passed unchanged to the called function);
it just so happens that 32-bit arithmetic on registers would always
implicitly zero the upper halves (and iirc that's the same on ARM64).

>> - other similar code living elsewhere?
>> Note also that this requires
>> http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg02485.html 
>> (or some other change avoiding truncation) to also be in place in order
>> to address the observed problem.
>> Note further that xc_domain_setmaxmem() is being used by upstream qemu
>> and hence the libxc interface change here may represent a compatibility
>> issue.
> 
> This might have been a problem wrt getting through the respective push
> gates in the right order, but actually doesn't automatic type promotion
> from unsigned int to uint64_t save us here too?

Same as above - sadly, no.

Jan

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Ian Campbell]

On Wed, 2015-04-22 at 15:41 +0100, Jan Beulich wrote:
> >>> On 22.04.15 at 16:01, <ian.campbell@citrix.com> wrote:
> > On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote:
> >> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
> >> top of the current target") caused a regression for "xl mem-set"
> >> against Dom0: While prior to creation of the first domain this works,
> >> the first domain creation involving ballooning breaks. Due to "enforce"
> >> not being set in the domain creation case, and due to Dom0's initial
> >> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
> >> "memorykb" in the first "xl mem-set" adusting the target upwards
> >> subsequent to domain creation and termination may cause an overflow,
> >> resulting in Dom0's maximum getting to a very small value. This small
> >> maximum will the make the subsequent setting of the PoD target fail.
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> Note that this only fixes the immediate problem - there appear to be
> >> further issues lurking here:
> >> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
> >> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
> > 
> > I think that increasing the width of these variables wouldn't break the
> > API guarantee which we make, at least not in a practical way, since any
> > existing 32-bit arguments passed will just get promoted.
> 
> No, not even on 64-bit. On 32-bit, two arguments slots are needed
> for what so far requires only one. On 64-bit (at least x86-64), the
> calling code isn't required to zero-extend a value calculated in a
> register (e.g. a result of earlier calculations which had more than
> 32 significant bits could be passed unchanged to the called function);
> it just so happens that 32-bit arithmetic on registers would always
> implicitly zero the upper halves (and iirc that's the same on ARM64).

You seem to be talking about ABI? As I tried to note in my response for
libxl we only make guarantees about the API (P not B in the middle).

Ian.

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Jan Beulich]

>>> On 22.04.15 at 17:36, <ian.campbell@citrix.com> wrote:
> On Wed, 2015-04-22 at 15:41 +0100, Jan Beulich wrote:
>> >>> On 22.04.15 at 16:01, <ian.campbell@citrix.com> wrote:
>> > On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote:
>> >> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
>> >> top of the current target") caused a regression for "xl mem-set"
>> >> against Dom0: While prior to creation of the first domain this works,
>> >> the first domain creation involving ballooning breaks. Due to "enforce"
>> >> not being set in the domain creation case, and due to Dom0's initial
>> >> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
>> >> "memorykb" in the first "xl mem-set" adusting the target upwards
>> >> subsequent to domain creation and termination may cause an overflow,
>> >> resulting in Dom0's maximum getting to a very small value. This small
>> >> maximum will the make the subsequent setting of the PoD target fail.
>> >> 
>> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> >> ---
>> >> Note that this only fixes the immediate problem - there appear to be
>> >> further issues lurking here:
>> >> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
>> >> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
>> > 
>> > I think that increasing the width of these variables wouldn't break the
>> > API guarantee which we make, at least not in a practical way, since any
>> > existing 32-bit arguments passed will just get promoted.
>> 
>> No, not even on 64-bit. On 32-bit, two arguments slots are needed
>> for what so far requires only one. On 64-bit (at least x86-64), the
>> calling code isn't required to zero-extend a value calculated in a
>> register (e.g. a result of earlier calculations which had more than
>> 32 significant bits could be passed unchanged to the called function);
>> it just so happens that 32-bit arithmetic on registers would always
>> implicitly zero the upper halves (and iirc that's the same on ARM64).
> 
> You seem to be talking about ABI? As I tried to note in my response for
> libxl we only make guarantees about the API (P not B in the middle).

Oh, okay. That would mean the libxl side is fine. The change to the
libxc interface might however still be a problem for qemu (Stefano
was telling me something to the effect of a compatibility layer in
upstream qemu to deal with such version differences, but of course
the implication - without versioned libxc interfaces - would still be
that existing qemu binaries on top of updated libxc would no
longer work).

Jan

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Ian Campbell]

On Wed, 2015-04-22 at 17:33 +0100, Jan Beulich wrote:
> >>> On 22.04.15 at 17:36, <ian.campbell@citrix.com> wrote:
> > On Wed, 2015-04-22 at 15:41 +0100, Jan Beulich wrote:
> >> >>> On 22.04.15 at 16:01, <ian.campbell@citrix.com> wrote:
> >> > On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote:
> >> >> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
> >> >> top of the current target") caused a regression for "xl mem-set"
> >> >> against Dom0: While prior to creation of the first domain this works,
> >> >> the first domain creation involving ballooning breaks. Due to "enforce"
> >> >> not being set in the domain creation case, and due to Dom0's initial
> >> >> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
> >> >> "memorykb" in the first "xl mem-set" adusting the target upwards
> >> >> subsequent to domain creation and termination may cause an overflow,
> >> >> resulting in Dom0's maximum getting to a very small value. This small
> >> >> maximum will the make the subsequent setting of the PoD target fail.
> >> >> 
> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> >> ---
> >> >> Note that this only fixes the immediate problem - there appear to be
> >> >> further issues lurking here:
> >> >> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
> >> >> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
> >> > 
> >> > I think that increasing the width of these variables wouldn't break the
> >> > API guarantee which we make, at least not in a practical way, since any
> >> > existing 32-bit arguments passed will just get promoted.
> >> 
> >> No, not even on 64-bit. On 32-bit, two arguments slots are needed
> >> for what so far requires only one. On 64-bit (at least x86-64), the
> >> calling code isn't required to zero-extend a value calculated in a
> >> register (e.g. a result of earlier calculations which had more than
> >> 32 significant bits could be passed unchanged to the called function);
> >> it just so happens that 32-bit arithmetic on registers would always
> >> implicitly zero the upper halves (and iirc that's the same on ARM64).
> > 
> > You seem to be talking about ABI? As I tried to note in my response for
> > libxl we only make guarantees about the API (P not B in the middle).
> 
> Oh, okay. That would mean the libxl side is fine. The change to the
> libxc interface might however still be a problem for qemu (Stefano
> was telling me something to the effect of a compatibility layer in
> upstream qemu to deal with such version differences, but of course
> the implication - without versioned libxc interfaces - would still be
> that existing qemu binaries on top of updated libxc would no
> longer work).

Correct, but we would surely bump the SONAME for a change such as this,
so it wouldn't matter in practice.

Ian.

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Jan Beulich]

>>> On 22.04.15 at 19:55, <Ian.Campbell@citrix.com> wrote:
> On Wed, 2015-04-22 at 17:33 +0100, Jan Beulich wrote:
>> >>> On 22.04.15 at 17:36, <ian.campbell@citrix.com> wrote:
>> > On Wed, 2015-04-22 at 15:41 +0100, Jan Beulich wrote:
>> >> >>> On 22.04.15 at 16:01, <ian.campbell@citrix.com> wrote:
>> >> > On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote:
>> >> >> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
>> >> >> top of the current target") caused a regression for "xl mem-set"
>> >> >> against Dom0: While prior to creation of the first domain this works,
>> >> >> the first domain creation involving ballooning breaks. Due to "enforce"
>> >> >> not being set in the domain creation case, and due to Dom0's initial
>> >> >> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
>> >> >> "memorykb" in the first "xl mem-set" adusting the target upwards
>> >> >> subsequent to domain creation and termination may cause an overflow,
>> >> >> resulting in Dom0's maximum getting to a very small value. This small
>> >> >> maximum will the make the subsequent setting of the PoD target fail.
>> >> >> 
>> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> >> >> ---
>> >> >> Note that this only fixes the immediate problem - there appear to be
>> >> >> further issues lurking here:
>> >> >> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
>> >> >> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
>> >> > 
>> >> > I think that increasing the width of these variables wouldn't break the
>> >> > API guarantee which we make, at least not in a practical way, since any
>> >> > existing 32-bit arguments passed will just get promoted.
>> >> 
>> >> No, not even on 64-bit. On 32-bit, two arguments slots are needed
>> >> for what so far requires only one. On 64-bit (at least x86-64), the
>> >> calling code isn't required to zero-extend a value calculated in a
>> >> register (e.g. a result of earlier calculations which had more than
>> >> 32 significant bits could be passed unchanged to the called function);
>> >> it just so happens that 32-bit arithmetic on registers would always
>> >> implicitly zero the upper halves (and iirc that's the same on ARM64).
>> > 
>> > You seem to be talking about ABI? As I tried to note in my response for
>> > libxl we only make guarantees about the API (P not B in the middle).
>> 
>> Oh, okay. That would mean the libxl side is fine. The change to the
>> libxc interface might however still be a problem for qemu (Stefano
>> was telling me something to the effect of a compatibility layer in
>> upstream qemu to deal with such version differences, but of course
>> the implication - without versioned libxc interfaces - would still be
>> that existing qemu binaries on top of updated libxc would no
>> longer work).
> 
> Correct, but we would surely bump the SONAME for a change such as this,
> so it wouldn't matter in practice.

So what's the situation with this patch? Can it go in? Is someone
working on a better fix for the described problem?

Thanks, Jan

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Ian Campbell]

On Wed, 2015-05-13 at 08:18 +0100, Jan Beulich wrote:
> >>> On 22.04.15 at 19:55, <Ian.Campbell@citrix.com> wrote:
> > On Wed, 2015-04-22 at 17:33 +0100, Jan Beulich wrote:
> >> >>> On 22.04.15 at 17:36, <ian.campbell@citrix.com> wrote:
> >> > On Wed, 2015-04-22 at 15:41 +0100, Jan Beulich wrote:
> >> >> >>> On 22.04.15 at 16:01, <ian.campbell@citrix.com> wrote:
> >> >> > On Wed, 2015-04-22 at 13:02 +0100, Jan Beulich wrote:
> >> >> >> Said commit ("libxl_set_memory_target: retain the same maxmem offset on
> >> >> >> top of the current target") caused a regression for "xl mem-set"
> >> >> >> against Dom0: While prior to creation of the first domain this works,
> >> >> >> the first domain creation involving ballooning breaks. Due to "enforce"
> >> >> >> not being set in the domain creation case, and due to Dom0's initial
> >> >> >> ->max_pages (in the hypervisor) being UINT_MAX, the calculation of
> >> >> >> "memorykb" in the first "xl mem-set" adusting the target upwards
> >> >> >> subsequent to domain creation and termination may cause an overflow,
> >> >> >> resulting in Dom0's maximum getting to a very small value. This small
> >> >> >> maximum will the make the subsequent setting of the PoD target fail.
> >> >> >> 
> >> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> >> >> ---
> >> >> >> Note that this only fixes the immediate problem - there appear to be
> >> >> >> further issues lurking here:
> >> >> >> - libxl_set_memory_target()'s *_memkb variables all being 32-bit,
> >> >> >> - libxl_domain_setmaxmem()'s max_memkb parameter being 32-bit,
> >> >> > 
> >> >> > I think that increasing the width of these variables wouldn't break the
> >> >> > API guarantee which we make, at least not in a practical way, since any
> >> >> > existing 32-bit arguments passed will just get promoted.
> >> >> 
> >> >> No, not even on 64-bit. On 32-bit, two arguments slots are needed
> >> >> for what so far requires only one. On 64-bit (at least x86-64), the
> >> >> calling code isn't required to zero-extend a value calculated in a
> >> >> register (e.g. a result of earlier calculations which had more than
> >> >> 32 significant bits could be passed unchanged to the called function);
> >> >> it just so happens that 32-bit arithmetic on registers would always
> >> >> implicitly zero the upper halves (and iirc that's the same on ARM64).
> >> > 
> >> > You seem to be talking about ABI? As I tried to note in my response for
> >> > libxl we only make guarantees about the API (P not B in the middle).
> >> 
> >> Oh, okay. That would mean the libxl side is fine. The change to the
> >> libxc interface might however still be a problem for qemu (Stefano
> >> was telling me something to the effect of a compatibility layer in
> >> upstream qemu to deal with such version differences, but of course
> >> the implication - without versioned libxc interfaces - would still be
> >> that existing qemu binaries on top of updated libxc would no
> >> longer work).
> > 
> > Correct, but we would surely bump the SONAME for a change such as this,
> > so it wouldn't matter in practice.
> 
> So what's the situation with this patch? Can it go in? Is someone
> working on a better fix for the described problem?

Stefano, Are you?

Regardless it seems to me that this patch is correct in its own right,
having maxmem_kb be a 64-bit type is the correct type for it to have.

Ian.

Re: [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2

[Ian Campbell]

On Wed, 2015-05-13 at 14:46 +0100, Ian Campbell wrote:
> > So what's the situation with this patch? Can it go in? Is someone
> > working on a better fix for the described problem?
> 
> Stefano, Are you?
> 
> Regardless it seems to me that this patch is correct in its own right,
> having maxmem_kb be a 64-bit type is the correct type for it to have.

No one objected so I've now applied.

Ian.