|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] libxl: fix "xl mem-set" regression from 0c029c4da2
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@xxxxxxxx>
> ---
> 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);
/*
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |