From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH for-xen-4.5] libxl: Allow copying smaller bitmap into a larger one Date: Fri, 21 Nov 2014 09:43:15 -0500 Message-ID: <546F4F83.8010704@oracle.com> References: <1416518854-5284-1-git-send-email-boris.ostrovsky@oracle.com> <20141121111244.GA18698@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20141121111244.GA18698@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: xen-devel@lists.xen.org, ian.jackson@eu.citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 11/21/2014 06:12 AM, Wei Liu wrote: > On Thu, Nov 20, 2014 at 04:27:34PM -0500, Boris Ostrovsky wrote: >> When parsing bitmap objects JSON parser will create libxl_bitmap >> map of the smallest size needed. >> >> This can cause problems when saved image file specifies CPU affinity. >> For example, if 'vcpu_hard_affinity' in the saved image has only the >> first CPU specified, just a single byte will be allocated and >> libxl_bitmap->size will be set to 1. >> >> This will result in assertion in libxl_set_vcpuaffinity()->libxl_bitmap_copy() >> since the destination bitmap is created for maximum number of CPUs. >> >> 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 >> >> Instead, we should allow copying the (smaller) bitmap read by the parser >> and keep the rest of bytes in the destination map unmodified (zero in >> this case) >> > What about copying large bitmap to a smaller one? Say, you migrate to > a host whose number of cpus is smaller than the size of the source > bitmap. Is this a valid use case? > > Should we have a "best effort" copy function? That is, > > size = min(source_size, destination_size) > copy(source, destination, size) Right, I haven't thought about it for the reversed case. -boris > > In any case I think this is a regression and should be fixed for 4.5. > > Wei. > >> Signed-off-by: Boris Ostrovsky >> --- >> tools/libxl/libxl.c | 4 ++-- >> tools/libxl/libxl_utils.c | 7 +++++++ >> tools/libxl/libxl_utils.h | 2 ++ >> 3 files changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index f7961f6..84fd2ca 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -5319,7 +5319,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_partial(ctx, &hard, cpumap_hard); >> flags = XEN_VCPUAFFINITY_HARD; >> } >> if (cpumap_soft) { >> @@ -5327,7 +5327,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_partial(ctx, &soft, cpumap_soft); >> flags |= XEN_VCPUAFFINITY_SOFT; >> } >> >> diff --git a/tools/libxl/libxl_utils.c b/tools/libxl/libxl_utils.c >> index 58df4f3..2a08bef 100644 >> --- a/tools/libxl/libxl_utils.c >> +++ b/tools/libxl/libxl_utils.c >> @@ -614,6 +614,13 @@ void libxl_bitmap_copy(libxl_ctx *ctx, libxl_bitmap *dptr, >> memcpy(dptr->map, sptr->map, sz * sizeof(*dptr->map)); >> } >> >> +void libxl_bitmap_copy_partial(libxl_ctx *ctx, libxl_bitmap *dptr, >> + const libxl_bitmap *sptr) >> +{ >> + assert(dptr->size >= sptr->size); >> + memcpy(dptr->map, sptr->map, sptr->size * sizeof(*dptr->map)); >> +} >> + >> void libxl_bitmap_copy_alloc(libxl_ctx *ctx, >> libxl_bitmap *dptr, >> const libxl_bitmap *sptr) >> diff --git a/tools/libxl/libxl_utils.h b/tools/libxl/libxl_utils.h >> index 117b229..d4d0a51 100644 >> --- a/tools/libxl/libxl_utils.h >> +++ b/tools/libxl/libxl_utils.h >> @@ -80,6 +80,8 @@ void libxl_bitmap_copy_alloc(libxl_ctx *ctx, libxl_bitmap *dptr, >> const libxl_bitmap *sptr); >> void libxl_bitmap_copy(libxl_ctx *ctx, libxl_bitmap *dptr, >> const libxl_bitmap *sptr); >> +void libxl_bitmap_copy_partial(libxl_ctx *ctx, libxl_bitmap *dptr, >> + const libxl_bitmap *sptr); >> int libxl_bitmap_is_full(const libxl_bitmap *bitmap); >> int libxl_bitmap_is_empty(const libxl_bitmap *bitmap); >> int libxl_bitmap_test(const libxl_bitmap *bitmap, int bit); >> -- >> 1.7.1