[libvirt] [PATCH 0/2] qemu: Enforce vCPU hotplug granularity constraints
return -ENOBLURB; Andrea Bolognani (2): qemu: Invert condition nesting in qemuDomainDefValidate() qemu: Enforce vCPU hotplug granularity constraints src/qemu/qemu_domain.c | 56 +++++++++++++++++++--- tests/qemuxml2argvdata/cpu-hotplug-granularity.xml | 18 +++++++ tests/qemuxml2argvtest.c | 3 ++ 3 files changed, 71 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/cpu-hotplug-granularity.xml -- 2.14.3
While at the moment we're only performing a single check that is connected to vCPU hotplugging, we're going to introduce a second one soon. Move the topology check underneath the capability check to make that easier; as a bonus, doing so allows us to reduce the scope of the 'topologycpus' variable. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 74b82450b..e93333669 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3299,7 +3299,6 @@ qemuDomainDefValidate(const virDomainDef *def, { virQEMUDriverPtr driver = opaque; virQEMUCapsPtr qemuCaps = NULL; - unsigned int topologycpus; int ret = -1; if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, @@ -3359,11 +3358,15 @@ qemuDomainDefValidate(const virDomainDef *def, } } - /* qemu as of 2.5.0 rejects SMP topologies that don't match the cpu count */ - if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 && - topologycpus != virDomainDefGetVcpusMax(def)) { - /* presence of query-hotpluggable-cpus should be a good enough witness */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) { + /* QEMU 2.7 (detected via the availability of query-hotpluggable-cpus) + * enforces stricter rules than previous versions when it comes to guest + * CPU topology. Verify known constraints are respected */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) { + unsigned int topologycpus; + + /* Max vCPU count and overall vCPU topology must agree */ + if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 && + topologycpus != virDomainDefGetVcpusMax(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("CPU topology doesn't match maximum vcpu count")); goto cleanup; -- 2.14.3
On Thu, Dec 14, 2017 at 17:29:50 +0100, Andrea Bolognani wrote:
While at the moment we're only performing a single check that is connected to vCPU hotplugging, we're going to introduce a second one soon. Move the topology check underneath the capability check to make that easier; as a bonus, doing so allows us to reduce the scope of the 'topologycpus' variable.
You know that generally we prefer variables declared at the top scope? Also you change the version of qemu in the comment without explanation here. Rather than bragging how you reduced scope, please document that change.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 74b82450b..e93333669 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3299,7 +3299,6 @@ qemuDomainDefValidate(const virDomainDef *def, { virQEMUDriverPtr driver = opaque; virQEMUCapsPtr qemuCaps = NULL; - unsigned int topologycpus; int ret = -1;
if (!(qemuCaps = virQEMUCapsCacheLookup(driver->qemuCapsCache, @@ -3359,11 +3358,15 @@ qemuDomainDefValidate(const virDomainDef *def, } }
- /* qemu as of 2.5.0 rejects SMP topologies that don't match the cpu count */ - if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 && - topologycpus != virDomainDefGetVcpusMax(def)) { - /* presence of query-hotpluggable-cpus should be a good enough witness */ - if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) { + /* QEMU 2.7 (detected via the availability of query-hotpluggable-cpus) + * enforces stricter rules than previous versions when it comes to guest + * CPU topology. Verify known constraints are respected */ + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) { + unsigned int topologycpus; + + /* Max vCPU count and overall vCPU topology must agree */ + if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 && + topologycpus != virDomainDefGetVcpusMax(def)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("CPU topology doesn't match maximum vcpu count")); goto cleanup;
The code looks okay, but I want to see a justification on the changed version.
On Fri, 2017-12-15 at 13:18 +0100, Peter Krempa wrote:
On Thu, Dec 14, 2017 at 17:29:50 +0100, Andrea Bolognani wrote:
While at the moment we're only performing a single check that is connected to vCPU hotplugging, we're going to introduce a second one soon. Move the topology check underneath the capability check to make that easier; as a bonus, doing so allows us to reduce the scope of the 'topologycpus' variable.
You know that generally we prefer variables declared at the top scope?
Is that so? I've seen several instances of the opposite, and it makes to me not to pollute the function scope with one-use variables when there are usually enough variables that actually need to be accessed throughout the function. But I can leave the declaration where it is if you like it better that way.
Also you change the version of qemu in the comment without explanation here. Rather than bragging how you reduced scope, please document that change.
Right, I meant to include the explanation but forgot :) It's very simple, anyway: the version number was wrong, since QEMU introduced query-hotpluggable-cpus in 2.7 rather than 2.5. -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Dec 15, 2017 at 14:11:41 +0100, Andrea Bolognani wrote:
On Fri, 2017-12-15 at 13:18 +0100, Peter Krempa wrote:
On Thu, Dec 14, 2017 at 17:29:50 +0100, Andrea Bolognani wrote:
While at the moment we're only performing a single check that is connected to vCPU hotplugging, we're going to introduce a second one soon. Move the topology check underneath the capability check to make that easier; as a bonus, doing so allows us to reduce the scope of the 'topologycpus' variable.
You know that generally we prefer variables declared at the top scope?
Is that so? I've seen several instances of the opposite, and it makes to me not to pollute the function scope with one-use variables when there are usually enough variables that actually need to be accessed throughout the function. But I can leave the declaration where it is if you like it better that way.
Fair enough. We only state that we should declare it at the beginning of a scope, so while generally most are declared at function scoep, this does not violate the contributor guidelines.
Also you change the version of qemu in the comment without explanation here. Rather than bragging how you reduced scope, please document that change.
Right, I meant to include the explanation but forgot :)
It's very simple, anyway: the version number was wrong, since QEMU introduced query-hotpluggable-cpus in 2.7 rather than 2.5.
While this is true for the presence of query-hotpluggable-cpus. The paragraph is specifically saying that QEMU rejects such configurations starting from 2.5., but the code checks them only since query-hotpluggable-cpus (which was introduced in 2.7). Your modification is thus incorrect. You can add statement that query-hotpluggable-cpus was added in 2.7. but mangling it as you did is not correct in my opinion.
On Fri, 2017-12-15 at 14:20 +0100, Peter Krempa wrote:
You know that generally we prefer variables declared at the top scope?
Is that so? I've seen several instances of the opposite, and it makes to me not to pollute the function scope with one-use variables when there are usually enough variables that actually need to be accessed throughout the function. But I can leave the declaration where it is if you like it better that way.
Fair enough. We only state that we should declare it at the beginning of a scope, so while generally most are declared at function scoep, this does not violate the contributor guidelines.
Cool, I'll still move it to the inner scope then.
It's very simple, anyway: the version number was wrong, since QEMU introduced query-hotpluggable-cpus in 2.7 rather than 2.5.
While this is true for the presence of query-hotpluggable-cpus. The paragraph is specifically saying that QEMU rejects such configurations starting from 2.5., but the code checks them only since query-hotpluggable-cpus (which was introduced in 2.7).
Your modification is thus incorrect. You can add statement that query-hotpluggable-cpus was added in 2.7. but mangling it as you did is not correct in my opinion.
I see, that's a nuance I was not aware of. I'll amend the comment so that it tells the whole story. -- Andrea Bolognani / Red Hat / Virtualization
QEMU 2.7 and newer don't allow guests to start unless the initial vCPUs count is a multiple of the vCPU hotplug granularity, so validate it and report an error if needed. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283700 Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 41 ++++++++++++++++++++++ tests/qemuxml2argvdata/cpu-hotplug-granularity.xml | 18 ++++++++++ tests/qemuxml2argvtest.c | 3 ++ 3 files changed, 62 insertions(+) create mode 100644 tests/qemuxml2argvdata/cpu-hotplug-granularity.xml diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e93333669..632f330f7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3289,6 +3289,36 @@ qemuDomainDefValidateVideo(const virDomainDef *def) } +/** + * qemuDomainDefGetVcpuHotplugGranularity: + * @def: domain definition + * @granularity: return location for vCPU hotplug granularity + * + * With QEMU 2.7 and newer, vCPUs can only be hotplugged @granularity at + * a time; because of that, QEMU will not allow guests to start unless the + * initial number of vCPUs is a multiple of @granularity. + * + * Returns 0 on success, 1 if topology is not configured and -1 on error. + */ +static int +qemuDomainDefGetVcpuHotplugGranularity(const virDomainDef *def, + unsigned int *granularity) +{ + if (!granularity) + return -1; + + if (!def->cpu || def->cpu->sockets == 0) + return 1; + + if (qemuDomainIsPSeries(def)) + *granularity = def->cpu->threads; + else + *granularity = 1; + + return 0; +} + + #define QEMU_MAX_VCPUS_WITHOUT_EIM 255 @@ -3363,6 +3393,7 @@ qemuDomainDefValidate(const virDomainDef *def, * CPU topology. Verify known constraints are respected */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS)) { unsigned int topologycpus; + unsigned int granularity; /* Max vCPU count and overall vCPU topology must agree */ if (virDomainDefGetVcpusTopology(def, &topologycpus) == 0 && @@ -3371,6 +3402,16 @@ qemuDomainDefValidate(const virDomainDef *def, _("CPU topology doesn't match maximum vcpu count")); goto cleanup; } + + /* vCPU hotplug granularity must be respected */ + if (qemuDomainDefGetVcpuHotplugGranularity(def, &granularity) == 0 && + (virDomainDefGetVcpus(def) % granularity) != 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("vCPUs count must be a multiple of the vCPU " + "hotplug granularity (%u)"), + granularity); + goto cleanup; + } } if (ARCH_IS_X86(def->os.arch) && diff --git a/tests/qemuxml2argvdata/cpu-hotplug-granularity.xml b/tests/qemuxml2argvdata/cpu-hotplug-granularity.xml new file mode 100644 index 000000000..a94f41e46 --- /dev/null +++ b/tests/qemuxml2argvdata/cpu-hotplug-granularity.xml @@ -0,0 +1,18 @@ +<domain type='qemu'> + <name>guest</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <vcpu placement='static' current='2'>8</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + </os> + <cpu> + <topology sockets='1' cores='2' threads='4'/> + </cpu> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='pci' model='pci-root'/> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ca24e0bbb..10ab8d787 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2900,6 +2900,9 @@ mymain(void) QEMU_CAPS_DEVICE_INTEL_IOMMU); DO_TEST("cpu-hotplug-startup", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS); + DO_TEST_PARSE_ERROR("cpu-hotplug-granularity", + QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS); + DO_TEST("virtio-options", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_KEYBOARD, QEMU_CAPS_VIRTIO_MOUSE, QEMU_CAPS_VIRTIO_TABLET, QEMU_CAPS_VIRTIO_INPUT_HOST, -- 2.14.3
On Thu, Dec 14, 2017 at 17:29:51 +0100, Andrea Bolognani wrote:
QEMU 2.7 and newer don't allow guests to start unless the initial vCPUs count is a multiple of the vCPU hotplug granularity, so validate it and report an error if needed.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1283700
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/qemu/qemu_domain.c | 41 ++++++++++++++++++++++ tests/qemuxml2argvdata/cpu-hotplug-granularity.xml | 18 ++++++++++ tests/qemuxml2argvtest.c | 3 ++ 3 files changed, 62 insertions(+) create mode 100644 tests/qemuxml2argvdata/cpu-hotplug-granularity.xml
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index e93333669..632f330f7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3289,6 +3289,36 @@ qemuDomainDefValidateVideo(const virDomainDef *def) }
+/** + * qemuDomainDefGetVcpuHotplugGranularity: + * @def: domain definition + * @granularity: return location for vCPU hotplug granularity + * + * With QEMU 2.7 and newer, vCPUs can only be hotplugged @granularity at + * a time; because of that, QEMU will not allow guests to start unless the + * initial number of vCPUs is a multiple of @granularity. + * + * Returns 0 on success, 1 if topology is not configured and -1 on error. + */ +static int +qemuDomainDefGetVcpuHotplugGranularity(const virDomainDef *def, + unsigned int *granularity) +{ + if (!granularity) + return -1;
Why?
+ + if (!def->cpu || def->cpu->sockets == 0) + return 1;
I think we assume '1' as threads here if it's not specified.
+ + if (qemuDomainIsPSeries(def)) + *granularity = def->cpu->threads; + else + *granularity = 1;
Why not just return the granularity rather than this weirdness?
+ + return 0; +} + +
On Fri, 2017-12-15 at 13:22 +0100, Peter Krempa wrote:
+ + if (!def->cpu || def->cpu->sockets == 0) + return 1;
I think we assume '1' as threads here if it's not specified.
Okay.
+ + if (qemuDomainIsPSeries(def)) + *granularity = def->cpu->threads; + else + *granularity = 1;
Why not just return the granularity rather than this weirdness?
Sure, I can do that. Does the rest of the patch look reasonable, or do you want to point out anything else before I respin? -- Andrea Bolognani / Red Hat / Virtualization
participants (2)
-
Andrea Bolognani -
Peter Krempa