Re: [libvirt] [Xen-devel] [PATCH] libxl: Correctly initialize vcpu bitmap

[Jim Fehlig <jfehlig@suse.com>]

Stefan Bader wrote:
> On 23.07.2013 23:20, Jim Fehlig wrote:
>   
[...]
>> You are using the driver-wide libxl_ctx in libxlDriverPrivate here, but
>> should be using the per-domain libxl_ctx in libxlDomainObjPrivate
>> structure IMO.
>>
>> It looks like libxlBuildDomainConfig, which calls libxlMakeDomBuildInfo,
>> should have just taken virDomainObjPtr instead of virDomainDefPtr. 
>> libxlBuildDomainConfig would then have access to the per-domain
>> libxl_ctx, and no longer need the libxlDriverPrivatePtr parameter as well.
>>
>>     
>
> So more like attached v2. I am not sure libxlDriverPrivatePtr can really be
> dropped (and maybe should not as part of a fix to this issue). Maybe calling
> libxl_flask_context_to_sid also should use the per domain context.

Yeah, I think so, but as a separate patch.

>  But at least
> libxlMakeVfbList->libxlMakeVfb->virPortAllocatorAcquire sounds a bit like it
> might need the driver context.
>   

Ah, right, neglected that call chain when suggesting to remove
libxlDriverPrivatePtr.

> From 2d4b7e5ae2644e6f7a2ea930d0899ea426cb9c84 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Fri, 19 Jul 2013 15:20:00 +0200
> Subject: [PATCH] libxl: Correctly initialize vcpu bitmap
>
> The avail_vcpu bitmap has to be allocated before it can be used (using
> the maximum allowed value for that). Then for each available VCPU the
> bit in the mask has to be set (libxl_bitmap_set takes a bit position
> as an argument, not the number of bits to set).
>
> Without this, I would always only get one VCPU for guests created
> through libvirt/libxl.
>
> [v2] Use private ctx from virDomainObjPtr->privateData
>   

No need for history of patch revisions in the commit message.  They can
be added with 'git send-email --annotate ...'.

> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  src/libxl/libxl_conf.c   |   19 +++++++++++++++----
>  src/libxl/libxl_conf.h   |    2 +-
>  src/libxl/libxl_driver.c |    2 +-
>  3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 4a0fba9..f732135 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -331,8 +331,10 @@ error:
>  }
>  
>  static int
> -libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
> +libxlMakeDomBuildInfo(virDomainObjPtr vm, libxl_domain_config *d_config)
>  {
> +    virDomainDefPtr def = vm->def;
> +    libxlDomainObjPrivatePtr priv = vm->privateData;
>      libxl_domain_build_info *b_info = &d_config->b_info;
>      int hvm = STREQ(def->os.type, "hvm");
>      size_t i;
> @@ -343,8 +345,15 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config)
>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_HVM);
>      else
>          libxl_domain_build_info_init_type(b_info, LIBXL_DOMAIN_TYPE_PV);
> +
>      b_info->max_vcpus = def->maxvcpus;
> -    libxl_bitmap_set((&b_info->avail_vcpus), def->vcpus);
> +    if (libxl_cpu_bitmap_alloc(priv->ctx, &b_info->avail_vcpus,
> +                               def->maxvcpus))
>   

Fits on one line.

ACK to the patch though.  I fixed these minor nits and pushed.

Regards,
Jim

> +        goto error;
> +    libxl_bitmap_set_none(&b_info->avail_vcpus);
> +    for (i = 0; i < def->vcpus; i++)
> +        libxl_bitmap_set((&b_info->avail_vcpus), i);
> +
>      if (def->clock.ntimers > 0 &&
>          def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) {
>          switch (def->clock.timers[0]->mode) {
> @@ -795,14 +804,16 @@ libxlMakeCapabilities(libxl_ctx *ctx)
>  
>  int
>  libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> -                       virDomainDefPtr def, libxl_domain_config *d_config)
> +                       virDomainObjPtr vm, libxl_domain_config *d_config)
>  {
> +    virDomainDefPtr def = vm->def;
> +
>      libxl_domain_config_init(d_config);
>  
>      if (libxlMakeDomCreateInfo(driver, def, &d_config->c_info) < 0)
>          return -1;
>  
> -    if (libxlMakeDomBuildInfo(def, d_config) < 0) {
> +    if (libxlMakeDomBuildInfo(vm, d_config) < 0) {
>          return -1;
>      }
>  
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 2b4a281..942cdd5 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -126,6 +126,6 @@ libxlMakeVfb(libxlDriverPrivatePtr driver,
>  
>  int
>  libxlBuildDomainConfig(libxlDriverPrivatePtr driver,
> -                       virDomainDefPtr def, libxl_domain_config *d_config);
> +                       virDomainObjPtr vm, libxl_domain_config *d_config);
>  
>  #endif /* LIBXL_CONF_H */
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 358d387..98b1985 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -968,7 +968,7 @@ libxlVmStart(libxlDriverPrivatePtr driver, virDomainObjPtr vm,
>  
>      libxl_domain_config_init(&d_config);
>  
> -    if (libxlBuildDomainConfig(driver, vm->def, &d_config) < 0)
> +    if (libxlBuildDomainConfig(driver, vm, &d_config) < 0)
>          goto error;
>  
>      if (driver->autoballoon && libxlFreeMem(priv, &d_config) < 0) {
> -- 1.7.9.5