Re: [PATCH for-xen-4.5] libxl: Allow copying smaller bitmap into a larger one

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

On Tue, 2014-11-25 at 11:15 +0000, Wei Liu wrote:
> This will result in assertion in libxl_set_vcpuaffinity()->libxl_bitmap_copy()
> since the destination bitmap is created for maximum number of CPUs.

FYI I'm also seeing this with libvirt (on ARM, but I don't think that
matters) when the guest XML uses:
        <vcpu placement='static' cpuset='0-7'>1</vcpu>
Results in:
        libvirtd: libxl_utils.c:612: libxl_bitmap_copy: Assertion `dptr->size == sptr->size' failed.
        
        Program received signal SIGABRT, Aborted.
        [Switching to Thread 0xb67f9420 (LWP 3881)]
        0xb6ab7f96 in ?? () from /lib/arm-linux-gnueabihf/libc.so.6
        (gdb) bt
        #0  0xb6ab7f96 in ?? () from /lib/arm-linux-gnueabihf/libc.so.6
        #1  0xb6ac5f8a in raise () from /lib/arm-linux-gnueabihf/libc.so.6
        #2  0xb6ac8428 in abort () from /lib/arm-linux-gnueabihf/libc.so.6
        #3  0xb6ac101e in __assert_fail () from /lib/arm-linux-gnueabihf/libc.so.6
        #4  0xb16caeb4 in libxl_bitmap_copy (ctx=<optimized out>, dptr=<optimized out>, sptr=<optimized out>) at libxl_utils.c:612
        #5  0xb16af15c in libxl_set_vcpuaffinity (ctx=0x2, domid=3065494508, vcpuid=0, cpumap_hard=0xb67f8668, cpumap_soft=0x0) at libxl.c:5323
        #6  0xb17195ae in libxlDomainSetVcpuAffinities () from /opt/libvirt/lib/libvirt/connection-driver/libvirt_driver_libxl.so
        #7  0xb1719cf2 in libxlDomainStart () from /opt/libvirt/lib/libvirt/connection-driver/libvirt_driver_libxl.so
        #8  0xb171b7c2 in libxlDomainCreateXML () from /opt/libvirt/lib/libvirt/connection-driver/libvirt_driver_libxl.so
        #9  0xb6d28630 in virDomainCreateXML () from /opt/libvirt/lib/libvirt.so.0
        [...snip...]
        
libxlDomainSetVcpuAffinities does:
    libxl_bitmap map;
    [...]

        map.size = cpumaplen;
        map.map = cpumap;

        if (libxl_set_vcpuaffinity(priv->ctx, def->id, vcpu, &map) != 0) {

http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/libxl/libxl_domain.c;h=9c622910547ada174a3d787c76c8bb076c9a75c3;hb=HEAD#l1059

Applying this libxl patch fixes things.

I don't think we've explicitly outlawed looking "inside" a libxl_bitmap
in this way anywhere, so I think libvirtd is within its rights to do so,
but it does highlight that we need to be mindful within libxl that
people may not be using libxl_cpu_bitmap_alloc.

> 
> We could allocate that bitmap of the same size as the source, however, it is
> later passed to xc_vcpu_setaffinity() which expects it to be sized to the max
> number of CPUs
> 
> To fix this issue, introduce an internal function to allowing copying between
> bitmaps of different sizes. Note that this function is only used in
> libxl_set_vcpuaffinity at the moment. Though NUMA placement logic invoke
> libxl_bitmap_copy as well there's no need to replace those invocations.  NUMA
> placement logic comes into effect when no vcpu / node pinning is provided, so
> it always operates on bitmap of the same sizes (that is, size of maximum
> number of cpus /nodes).
> 
> Reported-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Dario Faggioli <dario.faggioli@citrix.com>
> ---
> This fixes a regression for 4.5. Can't think of obvious risk.
> ---
>  tools/libxl/libxl.c          |    4 ++--
>  tools/libxl/libxl_internal.h |   11 +++++++++++
>  tools/libxl/libxl_utils.c    |   15 +++++++++++++++
>  3 files changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index de23fec..1e9da10 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -5320,7 +5320,7 @@ int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
>          if (rc)
>              goto out;
>  
> -        libxl_bitmap_copy(ctx, &hard, cpumap_hard);
> +        libxl__bitmap_copy_best_effort(gc, &hard, cpumap_hard);
>          flags = XEN_VCPUAFFINITY_HARD;
>      }
>      if (cpumap_soft) {
> @@ -5328,7 +5328,7 @@ int libxl_set_vcpuaffinity(libxl_ctx *ctx, uint32_t domid, uint32_t vcpuid,
>          if (rc)
>              goto out;
>  
> -        libxl_bitmap_copy(ctx, &soft, cpumap_soft);
> +        libxl__bitmap_copy_best_effort(gc, &soft, cpumap_soft);
>          flags |= XEN_VCPUAFFINITY_SOFT;
>      }
>  
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 4361421..a38f695 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -3617,6 +3617,17 @@ static inline void libxl__update_config_vtpm(libxl__gc *gc,
>          libxl_device_##type##_copy(CTX, DA_p, (dev));           \
>      })
>  
> +/* This function copies X bytes from source to destination bitmap,
> + * where X is the smaller of the two sizes.
> + *
> + * If destination's size is larger than source, the extra bytes are
> + * untouched.
> + *
> + * XXX This is introduced to fix a regression for 4.5. It shall
> + * be revisited in 4.6 time frame.
> + */
> +void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
> +                                    const libxl_bitmap *sptr);
>  #endif
>  
>  /*
> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c
> index 58df4f3..3e1ba17 100644
> --- a/tools/libxl/libxl_utils.c
> +++ b/tools/libxl/libxl_utils.c
> @@ -614,6 +614,21 @@ void libxl_bitmap_copy(libxl_ctx *ctx, libxl_bitmap *dptr,
>      memcpy(dptr->map, sptr->map, sz * sizeof(*dptr->map));
>  }
>  
> +/* This function copies X bytes from source to destination bitmap,
> + * where X is the smaller of the two sizes.
> + *
> + * If destination's size is larger than source, the extra bytes are
> + * untouched.
> + */
> +void libxl__bitmap_copy_best_effort(libxl__gc *gc, libxl_bitmap *dptr,
> +                                    const libxl_bitmap *sptr)
> +{
> +    int sz;
> +
> +    sz = dptr->size < sptr->size ? dptr->size : sptr->size;
> +    memcpy(dptr->map, sptr->map, sz * sizeof(*dptr->map));
> +}
> +
>  void libxl_bitmap_copy_alloc(libxl_ctx *ctx,
>                               libxl_bitmap *dptr,
>                               const libxl_bitmap *sptr)