Re: [libvirt] [PATCH v3 1/3] numatune: add check for numatune nodeset range

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