virVcpuInfo contains the vcpu number that the data refers to. Report what's returned by the daemon rather than the sequence number as with sparse vcpu topologies they won't match. --- tools/virsh-domain.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index de2a22c..90d2543 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -6315,8 +6315,8 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) } for (n = 0; n < ncpus; n++) { - vshPrint(ctl, "%-15s %d\n", _("VCPU:"), n); if (cpuinfo) { + vshPrint(ctl, "%-15s %d\n", _("VCPU:"), cpuinfo[n].number); vshPrint(ctl, "%-15s %d\n", _("CPU:"), cpuinfo[n].cpu); vshPrint(ctl, "%-15s %s\n", _("State:"), virshDomainVcpuStateToString(cpuinfo[n].state)); @@ -6328,6 +6328,7 @@ cmdVcpuinfo(vshControl *ctl, const vshCmd *cmd) vshPrint(ctl, "%-15s %.1lfs\n", _("CPU time:"), cpuUsed); } } else { + vshPrint(ctl, "%-15s %d\n", _("VCPU:"), n); vshPrint(ctl, "%-15s %s\n", _("CPU:"), _("N/A")); vshPrint(ctl, "%-15s %s\n", _("State:"), _("N/A")); vshPrint(ctl, "%-15s %s\n", _("CPU time"), _("N/A")); -- 2.8.2

ce43cca0e refactored the helper to prepare it for sparse topologies but forgot to fix the iterator used to fill the structures. This would result into a weirdly sparse populated array and possible out of bounds access and crash once sparse vcpu topologies were allowed. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1369988 --- src/qemu/qemu_driver.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 97e2ffc..671d1ff 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1477,15 +1477,17 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, for (i = 0; i < virDomainDefGetVcpusMax(vm->def) && ncpuinfo < maxinfo; i++) { virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i); pid_t vcpupid = qemuDomainGetVcpuPid(vm, i); + virVcpuInfoPtr vcpuinfo = info + ncpuinfo; if (!vcpu->online) continue; if (info) { - info[i].number = i; - info[i].state = VIR_VCPU_RUNNING; + vcpuinfo->number = i; + vcpuinfo->state = VIR_VCPU_RUNNING; - if (qemuGetProcessInfo(&(info[i].cpuTime), &(info[i].cpu), NULL, + if (qemuGetProcessInfo(&vcpuinfo->cpuTime, + &vcpuinfo->cpu, NULL, vm->pid, vcpupid) < 0) { virReportSystemError(errno, "%s", _("cannot get vCPU placement & pCPU time")); @@ -1494,7 +1496,7 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, } if (cpumaps) { - unsigned char *cpumap = VIR_GET_CPUMAP(cpumaps, maplen, i); + unsigned char *cpumap = VIR_GET_CPUMAP(cpumaps, maplen, ncpuinfo); virBitmapPtr map = NULL; if (!(map = virProcessGetAffinity(vcpupid))) @@ -1505,7 +1507,7 @@ qemuDomainHelperGetVcpus(virDomainObjPtr vm, } if (cpuwait) { - if (qemuGetSchedInfo(&(cpuwait[i]), vm->pid, vcpupid) < 0) + if (qemuGetSchedInfo(&(cpuwait[ncpuinfo]), vm->pid, vcpupid) < 0) return -1; } -- 2.8.2

Make it clear that vcpu order is valid for online vcpus only and state that it has to be specified for all vcpus or not provided at all. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1370043 --- docs/formatdomain.html.in | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f2de8e4..f1dadc6 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -564,11 +564,12 @@ all disabled vcpus must be hotpluggable. Valid values are <code>yes</code> and <code>no</code>. - <code>order</code> allows to specify the order to add the vcpus. For - hypervisors/platforms that require to insert multiple vcpus at once + <code>order</code> allows to specify the order to add the online vcpus. + For hypervisors/platforms that require to insert multiple vcpus at once the order may be be duplicated accross all vcpus that need to be enabled at once. Specifying order is not necessary, vcpus are then - added in an arbitrary order. + added in an arbitrary order. If order info is used, it must be used for + all online vcpus. Note that hypervisors may create hotpluggable vcpus differently from boot vcpus thus special initialization may be necessary. -- 2.8.2

Validating the vcpu count is more intricate and doing it in the XML parser will make previously valid configs (with older qemus) vanish. Now that we have the validation callbacks we can do it in a more appropriate place. This basically reverts commit b54de0830a. Partially resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1370066 --- src/conf/domain_conf.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 17c2f53..ae3eb14 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -16376,15 +16376,6 @@ virDomainDefParseXML(xmlDocPtr xml, if (def->cpu == NULL) goto error; - - if (def->cpu->sockets && - virDomainDefGetVcpusMax(def) > - def->cpu->sockets * def->cpu->cores * def->cpu->threads) { - virReportError(VIR_ERR_XML_DETAIL, "%s", - _("Maximum CPUs greater than topology limit")); - goto error; - } - } if (virDomainNumaDefCPUParseXML(def->numa, ctxt) < 0) -- 2.8.2

Setting vcpu count when cpu topology is specified may result into an invalid configuration. Since the topology can't be modified, reject the setting if it doesn't match the requested topology. This will allow fixing the topology in case it was broken. Partially fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1370066 --- src/qemu/qemu_driver.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 671d1ff..5f8c11c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4730,6 +4730,17 @@ qemuDomainSetVcpusMax(virQEMUDriverPtr driver, goto cleanup; } + if (persistentDef && persistentDef->cpu && persistentDef->cpu->sockets) { + /* explicitly allow correcting invalid vcpu count */ + if (nvcpus != persistentDef->cpu->sockets * + persistentDef->cpu->cores * + persistentDef->cpu->threads) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("CPU topology doesn't match the desired vcpu count")); + goto cleanup; + } + } + if (virDomainDefSetVcpusMax(persistentDef, nvcpus, driver->xmlopt) < 0) goto cleanup; -- 2.8.2

../../src/conf/domain_conf.c:4425:21: error: potential null pointer dereference [-Werror=null-dereference] switch (vcpu->hotpluggable) { ~~~~^~~~~~~~~~~~~~ --- src/conf/domain_conf.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ae3eb14..61f6dbb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4422,6 +4422,10 @@ virDomainVcpuDefPostParse(virDomainDefPtr def) for (i = 0; i < maxvcpus; i++) { vcpu = virDomainDefGetVcpu(def, i); + /* impossible but some compilers don't like it */ + if (!vcpu) + continue; + switch (vcpu->hotpluggable) { case VIR_TRISTATE_BOOL_ABSENT: if (vcpu->online) -- 2.8.2