[libvirt] [PATCH] Fix initialization of current vcpus in libxl driver
The cur_vcpus member of struct libxl_domain_build_info was incorrectly initialized to the number of vcpus, when it should have been interpreted as a bitmap, where bit X corresponds to online/offline status of vcpuX. To complicate matters, cur_vcpus is an int, so only 32 vcpus can be set online. Add a check to ensure vcpus does not exceed this limit. --- src/libxl/libxl_conf.c | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 3cebf41..2e48356 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -379,11 +379,23 @@ libxlMakeDomBuildInfo(virDomainDefPtr def, libxl_domain_config *d_config) int hvm = STREQ(def->os.type, "hvm"); int i; + /* Currently, libxenlight only supports 32 vcpus per domain. + * cur_vcpus member of struct libxl_domain_build_info is defined + * as an int, but its' semantic is a bitmap of online vcpus, so + * only 32 can be represented. + */ + if (def->maxvcpus > 32 || def->vcpus > 32) { + libxlError(VIR_ERR_INTERNAL_ERROR, + _("This version of libxenlight only supports 32 " + "vcpus per domain")); + return -1; + } + libxl_init_build_info(b_info, &d_config->c_info); b_info->hvm = hvm; b_info->max_vcpus = def->maxvcpus; - b_info->cur_vcpus = def->vcpus; + b_info->cur_vcpus = (1 << def->vcpus) - 1; if (def->clock.ntimers > 0 && def->clock.timers[0]->name == VIR_DOMAIN_TIMER_NAME_TSC) { switch (def->clock.timers[0]->mode) { -- 1.7.3.1
On 05/23/2011 04:26 PM, Jim Fehlig wrote:
b_info->hvm = hvm; b_info->max_vcpus = def->maxvcpus; - b_info->cur_vcpus = def->vcpus; + b_info->cur_vcpus = (1 << def->vcpus) - 1;
That's a compilation pitfall if def->vcpus is exactly 32. Shoot; xenFormatSxpr in xenxs/xen_sxpr.c has the same bug. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Mon, May 23, 2011 at 04:43:19PM -0600, Eric Blake wrote:
On 05/23/2011 04:26 PM, Jim Fehlig wrote:
b_info->hvm = hvm; b_info->max_vcpus = def->maxvcpus; - b_info->cur_vcpus = def->vcpus; + b_info->cur_vcpus = (1 << def->vcpus) - 1;
That's a compilation pitfall if def->vcpus is exactly 32.
Shoot; xenFormatSxpr in xenxs/xen_sxpr.c has the same bug.
What would be the best way to fix ? cast to a larger int and recast the result or provide some kind of helper function for bitmaps ? Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 05/24/2011 09:14 AM, Daniel Veillard wrote:
On Mon, May 23, 2011 at 04:43:19PM -0600, Eric Blake wrote:
On 05/23/2011 04:26 PM, Jim Fehlig wrote:
b_info->hvm = hvm; b_info->max_vcpus = def->maxvcpus; - b_info->cur_vcpus = def->vcpus; + b_info->cur_vcpus = (1 << def->vcpus) - 1;
That's a compilation pitfall if def->vcpus is exactly 32.
Shoot; xenFormatSxpr in xenxs/xen_sxpr.c has the same bug.
What would be the best way to fix ? cast to a larger int and recast the result or provide some kind of helper function for bitmaps ?
Maybe special-case: if (def->vcpus == 32) b_info->cur_vcpus = (uint32_t) -1; else b_info->cur_vcpus = (1 << def->vcpus) - 1; Or build it up differently (although it gets harder to read): b_info->cur_vcpus = ((1 << (def->vcpus - 1)) - 1) * 2 + 1; As long as we rework it to avoid the undefined overflow of 1 << 32, when we really just want 32 bits set. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/24/2011 09:14 AM, Daniel Veillard wrote:
On Mon, May 23, 2011 at 04:43:19PM -0600, Eric Blake wrote:
On 05/23/2011 04:26 PM, Jim Fehlig wrote:
b_info->hvm = hvm; b_info->max_vcpus = def->maxvcpus; - b_info->cur_vcpus = def->vcpus; + b_info->cur_vcpus = (1 << def->vcpus) - 1;
That's a compilation pitfall if def->vcpus is exactly 32.
Shoot; xenFormatSxpr in xenxs/xen_sxpr.c has the same bug.
What would be the best way to fix ? cast to a larger int and recast the result or provide some kind of helper function for bitmaps ?
Maybe special-case:
if (def->vcpus == 32) b_info->cur_vcpus = (uint32_t) -1; else b_info->cur_vcpus = (1 << def->vcpus) - 1;
This works in the libxl driver since cur_vcpus is an int. But vcpu_avail is a long in xenxs/xen_sxpr.c and I'm hoping there is something better than the following approach diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 4cebb21..dc000a8 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -2006,9 +2006,14 @@ xenFormatSxpr(virConnectPtr conn, virBufferAsprintf(&buf, "(vcpus %u)", def->maxvcpus); /* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is either 32, or 64 on a platform where long is big enough. */ - if (def->vcpus < def->maxvcpus) - virBufferAsprintf(&buf, "(vcpu_avail %lu)", - def->vcpus == 32 ? 1 << 31 : (1UL << def->vcpus) - 1); + if (def->vcpus < def->maxvcpus) { + if (sizeof(long) == 4 && def->vcpus == 32) + virBufferAsprintf(&buf, "(vcpu_avail %lu)", 1UL << 31); + else if (sizeof(long) == 8 && def->vcpus == 64) + virBufferAsprintf(&buf, "(vcpu_avail %lu)", 1UL << 63); + else + virBufferAsprintf(&buf, "(vcpu_avail %lu)", (1UL << def->vcpus) - 1); + } if (def->cpumask) { char *ranges = virDomainCpuSetFormat(def->cpumask, def->cpumasklen); Suggestions? Thanks, Jim
On 05/24/2011 10:33 AM, Jim Fehlig wrote:
This works in the libxl driver since cur_vcpus is an int. But vcpu_avail is a long in xenxs/xen_sxpr.c and I'm hoping there is something better than the following approach
diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 4cebb21..dc000a8 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -2006,9 +2006,14 @@ xenFormatSxpr(virConnectPtr conn, virBufferAsprintf(&buf, "(vcpus %u)", def->maxvcpus); /* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is either 32, or 64 on a platform where long is big enough. */ - if (def->vcpus < def->maxvcpus) - virBufferAsprintf(&buf, "(vcpu_avail %lu)", - def->vcpus == 32 ? 1 << 31 : (1UL << def->vcpus) - 1);
+ if (def->vcpus < def->maxvcpus) {
If def->maxvcpus is 32, then:
+ if (sizeof(long) == 4 && def->vcpus == 32)
this conditional will never be reached (def->vcpus is less than def->maxvcpus). So the existing ((1UL << def->vcpus) -1) never calls the non-portable 1UL<<32 (or 1UL<<64); so it never overflows. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 05/24/2011 10:33 AM, Jim Fehlig wrote:
This works in the libxl driver since cur_vcpus is an int. But vcpu_avail is a long in xenxs/xen_sxpr.c and I'm hoping there is something better than the following approach
diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c index 4cebb21..dc000a8 100644 --- a/src/xenxs/xen_sxpr.c +++ b/src/xenxs/xen_sxpr.c @@ -2006,9 +2006,14 @@ xenFormatSxpr(virConnectPtr conn, virBufferAsprintf(&buf, "(vcpus %u)", def->maxvcpus); /* Computing the vcpu_avail bitmask works because MAX_VIRT_CPUS is either 32, or 64 on a platform where long is big enough. */ - if (def->vcpus < def->maxvcpus) - virBufferAsprintf(&buf, "(vcpu_avail %lu)", - def->vcpus == 32 ? 1 << 31 : (1UL << def->vcpus) - 1);
+ if (def->vcpus < def->maxvcpus) {
If def->maxvcpus is 32, then:
+ if (sizeof(long) == 4 && def->vcpus == 32)
this conditional will never be reached (def->vcpus is less than def->maxvcpus).
So the existing ((1UL << def->vcpus) -1) never calls the non-portable 1UL<<32 (or 1UL<<64); so it never overflows.
Ok, so I just need to handle this in the libxl driver. I'll post a V2. Regards, Jim
participants (3)
-
Daniel Veillard -
Eric Blake -
Jim Fehlig