For memnode in numatune element, the range of attribute 'nodeset' was not validated. on my host maxnodes was 1, but when setting nodeset to '0-2' or more, guest also started succuss. there probably was qemu's bug too. Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com&gt; --- src/conf/numatune_conf.c | 28 +++++++++++++ src/conf/numatune_conf.h | 1 + src/libvirt_private.syms | 2 + src/qemu/qemu_command.c | 3 ++ src/qemu/qemu_command.h | 1 + src/util/virbitmap.c | 49 ++++++++++++++++++++++ src/util/virbitmap.h | 3 ++ src/util/virnuma.c | 33 +++++++++++++++ src/util/virnuma.h | 2 + ...rgv-numatune-static-nodeset-exceed-hostnode.xml | 35 ++++++++++++++++ tests/qemuxml2argvmock.c | 9 ++++ tests/qemuxml2argvtest.c | 1 + tests/virbitmaptest.c | 26 +++++++++++- 13 files changed, 192 insertions(+), 1 deletion(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml diff --git a/src/conf/numatune_conf.c b/src/conf/numatune_conf.c index 21d9a64..54f309a 100644 --- a/src/conf/numatune_conf.c +++ b/src/conf/numatune_conf.c @@ -612,3 +612,31 @@ virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune) return false; } + +int +virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune) +{ + int ret = -1; + virBitmapPtr nodemask = NULL; + size_t i; + + if (!numatune) + return ret; + + nodemask = virDomainNumatuneGetNodeset(numatune, NULL, -1); + if (nodemask) { + ret = virBitmapLastSetBit(nodemask, -1); + } + for (i = 0; i < numatune->nmem_nodes; i++) { + int bit = -1; + nodemask = virDomainNumatuneGetNodeset(numatune, NULL, i); + if (!nodemask) + continue; + + bit = virBitmapLastSetBit(nodemask, -1); + if (bit > ret) + ret = bit; + } + + return ret; +} diff --git a/src/conf/numatune_conf.h b/src/conf/numatune_conf.h index 5254629..15dc0d6 100644 --- a/src/conf/numatune_conf.h +++ b/src/conf/numatune_conf.h @@ -102,4 +102,5 @@ bool virDomainNumatuneHasPlacementAuto(virDomainNumatunePtr numatune); bool virDomainNumatuneHasPerNodeBinding(virDomainNumatunePtr numatune); +int virDomainNumatuneSpecifiedMaxNode(virDomainNumatunePtr numatune); #endif /* __NUMATUNE_CONF_H__ */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d63a8f0..4a30ad7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1011,6 +1011,7 @@ virBitmapFree; virBitmapGetBit; virBitmapIsAllClear; virBitmapIsAllSet; +virBitmapLastSetBit; virBitmapNew; virBitmapNewCopy; virBitmapNewData; @@ -1728,6 +1729,7 @@ virNumaGetPageInfo; virNumaGetPages; virNumaIsAvailable; virNumaNodeIsAvailable; +virNumaNodesetIsAvailable; virNumaSetPagePoolSize; virNumaSetupMemoryPolicy; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2e5af4f..9757d3e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6663,6 +6663,9 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, goto cleanup; } + if (!virNumaNodesetIsAvailable(def->numatune)) + goto cleanup; + for (i = 0; i < def->mem.nhugepages; i++) { ssize_t next_bit, pos = 0; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index aa40c9e..f263665 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -27,6 +27,7 @@ # include "domain_addr.h" # include "domain_conf.h" # include "vircommand.h" +# include "virnuma.h" # include "capabilities.h" # include "qemu_conf.h" # include "qemu_domain.h" diff --git a/src/util/virbitmap.c b/src/util/virbitmap.c index b6bd074..aed525a 100644 --- a/src/util/virbitmap.c +++ b/src/util/virbitmap.c @@ -651,6 +651,55 @@ virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos) } /** + * virBitmapLastSetBit: + * @bitmap: the bitmap + * @pos: the position after which to search for a set bit + * + * Search for the last set bit before position @pos in bitmap @bitmap. + * @pos can be -1 to search for the last set bit. Position starts + * at max_bit. + * + * Returns the position of the found bit, or -1 if no bit found. + */ +ssize_t +virBitmapLastSetBit(virBitmapPtr bitmap, ssize_t pos) +{ + size_t nl; + size_t nb; + unsigned long bits; + size_t i; + + if (pos < 0) + pos = bitmap->max_bit; + + pos--; + + if (pos >= bitmap->max_bit) + return -1; + + nl = pos / VIR_BITMAP_BITS_PER_UNIT; + nb = pos % VIR_BITMAP_BITS_PER_UNIT; + + bits = bitmap->map[nl] & ((1UL << (nb + 1)) - 1); + + while (bits == 0 && --nl < bitmap->map_len) { + bits = bitmap->map[nl]; + } + + if (bits == 0) + return -1; + + i = VIR_BITMAP_BITS_PER_UNIT - 1; + for (; i < VIR_BITMAP_BITS_PER_UNIT; i--) { + if (bits & 1UL << i) { + return i + nl * VIR_BITMAP_BITS_PER_UNIT; + } + } + + return -1; +} + +/** * virBitmapNextClearBit: * @bitmap: the bitmap * @pos: the position after which to search for a clear bit diff --git a/src/util/virbitmap.h b/src/util/virbitmap.h index 4493cc9..c0fad55 100644 --- a/src/util/virbitmap.h +++ b/src/util/virbitmap.h @@ -105,6 +105,9 @@ bool virBitmapIsAllClear(virBitmapPtr bitmap) ssize_t virBitmapNextSetBit(virBitmapPtr bitmap, ssize_t pos) ATTRIBUTE_NONNULL(1); +ssize_t virBitmapLastSetBit(virBitmapPtr bitmap, ssize_t pos) + ATTRIBUTE_NONNULL(1); + ssize_t virBitmapNextClearBit(virBitmapPtr bitmap, ssize_t pos) ATTRIBUTE_NONNULL(1); diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 690615f..fbe8fd1 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -312,6 +312,33 @@ virNumaGetNodeCPUs(int node, return ret; } + +bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + int maxnode; + int bit; + + if (!numatune) + return true; + + if ((maxnode = virNumaGetMaxNode()) < 0) + return false; + + maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; + + bit = virDomainNumatuneSpecifiedMaxNode(numatune); + if (bit > maxnode) + goto error; + + return true; + + error: + virReportError(VIR_ERR_INTERNAL_ERROR, + _("NUMA node %d is out of range"), bit); + return false; +} + # undef MASK_CPU_ISSET # undef n_bits @@ -373,6 +400,12 @@ virNumaGetNodeCPUs(int node ATTRIBUTE_UNUSED, _("NUMA isn't available on this host")); return -1; } + +bool +virNumaNodesetIsAvailable(virDomainNumatunePtr numatune) +{ + return true; +} #endif diff --git a/src/util/virnuma.h b/src/util/virnuma.h index 04b6b8c..e129bb2 100644 --- a/src/util/virnuma.h +++ b/src/util/virnuma.h @@ -34,6 +34,8 @@ char *virNumaGetAutoPlacementAdvice(unsigned short vcups, int virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, virBitmapPtr nodemask); +bool virNumaNodesetIsAvailable(virDomainNumatunePtr numatune); + bool virNumaIsAvailable(void); int virNumaGetMaxNode(void); bool virNumaNodeIsAvailable(int node); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml new file mode 100644 index 0000000..84a560a --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-numatune-static-nodeset-exceed-hostnode.xml @@ -0,0 +1,35 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>1048576</currentMemory> + <vcpu placement='static'>4</vcpu> + <numatune> + <memnode cellid='0' mode='strict' nodeset='0-8'/> + <memnode cellid='1' mode='strict' nodeset='0'/> + </numatune> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <cpu> + <numa> + <cell id='0' cpus='0-1' memory='524288'/> + <cell id='1' cpus='2-3' memory='524288'/> + </numa> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index bff3b0f..316bf0b 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -21,6 +21,7 @@ #include <config.h> #include "internal.h" +#include "virnuma.h" #include <time.h> time_t time(time_t *t) @@ -30,3 +31,11 @@ time_t time(time_t *t) *t = ret; return ret; } + +int +virNumaGetMaxNode(void) +{ + const int maxnodes = 7; + + return maxnodes; +} diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0e9fab9..bab6d0d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1250,6 +1250,7 @@ mymain(void) DO_TEST_FAILURE("numatune-memnode-no-memory", NONE); DO_TEST("numatune-auto-nodeset-invalid", NONE); + DO_TEST_FAILURE("numatune-static-nodeset-exceed-hostnode", QEMU_CAPS_OBJECT_MEMORY_RAM); DO_TEST_PARSE_ERROR("numatune-memnode-nocpu", NONE); DO_TEST_PARSE_ERROR("numatune-memnodes-problematic", NONE); DO_TEST("numad", NONE); diff --git a/tests/virbitmaptest.c b/tests/virbitmaptest.c index ea832ad..21e8611 100644 --- a/tests/virbitmaptest.c +++ b/tests/virbitmaptest.c @@ -171,7 +171,7 @@ test3(const void *data ATTRIBUTE_UNUSED) return ret; } -/* test for virBitmapNextSetBit, virBitmapNextClearBit */ +/* test for virBitmapNextSetBit, virBitmapLastSetBit, virBitmapNextClearBit */ static int test4(const void *data ATTRIBUTE_UNUSED) { @@ -200,6 +200,9 @@ test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextSetBit(bitmap, -1) != -1) goto error; + if (virBitmapLastSetBit(bitmap, -1) != -1) + goto error; + for (i = 0; i < size; i++) { if (virBitmapNextClearBit(bitmap, i - 1) != i) goto error; @@ -232,6 +235,18 @@ test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextSetBit(bitmap, i) != -1) goto error; + j = sizeof(bitsPos)/sizeof(int) - 1; + i = -1; + + while (j < ARRAY_CARDINALITY(bitsPos)) { + i = virBitmapLastSetBit(bitmap, i); + if (i != bitsPos[j--]) + goto error; + } + + if (virBitmapLastSetBit(bitmap, i) != -1) + goto error; + j = 0; i = -1; @@ -255,6 +270,15 @@ test4(const void *data ATTRIBUTE_UNUSED) if (virBitmapNextSetBit(bitmap, i) != -1) goto error; + for (i = size; i > 0; i--) { + if (virBitmapLastSetBit(bitmap, i) != i - 1) + goto error; + } + + if (virBitmapLastSetBit(bitmap, i) != -1) + goto error; + + if (virBitmapNextClearBit(bitmap, -1) != -1) goto error; -- 1.9.3

On Wed, 2014-10-29 at 14:20 +0100, Martin Kletzander wrote: Look good to me. right, I should use numatune->mem_nodes[i].nodeset directly. I will delete the "pos" attribute. Sure. when libvirt does not support numa, we can not check it. maybe we should return false if setting nodeset. Thanks, Chen

On Thu, 2014-10-30 at 07:55 +0100, Martin Kletzander wrote: Yes, I had made a v4 and sent it, please help to review. Thanks, Chen

Signed-off-by: Chen Fan <chen.fan.fnst(a)cn.fujitsu.com&gt; --- src/util/virnuma.c | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/util/virnuma.c b/src/util/virnuma.c index fbe8fd1..5a08049 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -95,31 +95,16 @@ virNumaSetupMemoryPolicy(virDomainNumatunePtr numatune, int ret = -1; int bit = 0; size_t i; - int maxnode = 0; virBitmapPtr tmp_nodemask = NULL; tmp_nodemask = virDomainNumatuneGetNodeset(numatune, nodemask, -1); if (!tmp_nodemask) return 0; - if (numa_available() < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Host kernel is not aware of NUMA.")); - return -1; - } - - maxnode = numa_max_node(); - maxnode = maxnode < NUMA_NUM_NODES ? maxnode : NUMA_NUM_NODES; - /* Convert nodemask to NUMA bitmask. */ nodemask_zero(&mask); bit = -1; while ((bit = virBitmapNextSetBit(tmp_nodemask, bit)) >= 0) { - if (bit > maxnode) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("NUMA node %d is out of range"), bit); - return -1; - } nodemask_set(&mask, bit); } -- 1.9.3

On Wed, 2014-10-29 at 14:23 +0100, Martin Kletzander wrote: call virNumaNodesetIsAvailable() in the start of the function will be fine. Thanks, Chen