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

[Ian Campbell <ian.campbell@citrix.com>]

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;
> 
> 
>