Later on, we will need to know whether the user specified the target node or not. Turn the variable into a signed value. We already treat it as signed in various parts of the qemu driver. --- src/conf/domain_conf.c | 10 ++++++---- src/conf/domain_conf.h | 2 +- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 002bf13..514bd8a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12521,7 +12521,8 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, xmlNodePtr save = ctxt->node; ctxt->node = node; - if (virXPathUInt("string(./node)", ctxt, &def->targetNode) < 0) { + if (virXPathInt("string(./node)", ctxt, &def->targetNode) < 0 || + def->targetNode < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid or missing value of memory device node")); goto cleanup; @@ -17668,8 +17669,8 @@ virDomainMemoryDefCheckABIStability(virDomainMemoryDefPtr src, if (src->targetNode != dst->targetNode) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Target memory device targetNode '%u' " - "doesn't match source targetNode '%u'"), + _("Target memory device targetNode '%d' " + "doesn't match source targetNode '%d'"), dst->targetNode, src->targetNode); return false; } @@ -20759,7 +20760,8 @@ virDomainMemoryTargetDefFormat(virBufferPtr buf, virBufferAdjustIndent(buf, 2); virBufferAsprintf(buf, "<size unit='KiB'>%llu</size>\n", def->size); - virBufferAsprintf(buf, "<node>%u</node>\n", def->targetNode); + if (def->targetNode >= 0) + virBufferAsprintf(buf, "<node>%d</node>\n", def->targetNode); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</target>\n"); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index f10b534..8d43ee6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2022,7 +2022,7 @@ struct _virDomainMemoryDef { /* target */ int model; /* virDomainMemoryModel */ - unsigned int targetNode; + int targetNode; unsigned long long size; /* kibibytes */ virDomainDeviceInfo info; -- 2.4.5

On 10/16/2015 08:11 AM, Peter Krempa wrote: either Although it seems that comment is left for under the --- ... ^^ this could move ^^ Seems to me the code could be adjusted a bit to have: if (guestNode >= 0) { if (guestNode >= virDomainNumaGetNodeCount(def->numa)) ... memAccess = virDomainNumaGetNodeMemoryAccessMode(def->numa, guestNode); ... if (virDomainNumatuneGetMode(def->numa, guestNode, &mode) < 0 && ... } where 'mode' is initialized to VIR_DOMAIN_NUMATUNE_MEM_STRICT; and the props call moves either before or after the blob. So what you're say is if master_hugepage is set/found, then we don't need to find anything else? Or can there be more than one master_hugepage and it is desired to find the "last"? Is there perhaps a cause/reason to break the loop rather than continue? IOW: Move the check inside the previous if and use it as a cause to break the loop. I think this is ACK-able with a couple of adjustments, but just wanted to check what was more reasonable first - especially with this < 0 change. John

On 10/16/2015 08:11 AM, Peter Krempa wrote: The second half of this comment made me wonder where... especially with the removal of the virDomainNumaGetNodeCount call which would essentially be checking if numa was enabled (I think). The virDomainDefHasMemoryHotplug checks memory_slots or max_memory The virDomainDefPostParseMemory could have it, but it's not overt Not that the check is wrong, but it would be nice to fixup the comment to indicate that 'xxx' checked that memory devices are present only when NUMA is. That at least helps ensure any future refactor doesn't impact the assumption made here. ACK with that John

Add a function that will aggregate various checks related to memory hotplug so that they aren't scattered accross various parts of the code. --- src/qemu/qemu_command.c | 26 ++--------------- src/qemu/qemu_domain.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 4 +++ src/qemu/qemu_hotplug.c | 9 ++---- 4 files changed, 87 insertions(+), 29 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 43e81c7..4a709db 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9414,26 +9414,12 @@ qemuBuildCommandLine(virConnectPtr conn, qemuDomainAlignMemorySizes(def) < 0) goto error; + if (qemuDomainDefValidateMemoryHotplug(def, qemuCaps, NULL) < 0) + goto error; + virCommandAddArg(cmd, "-m"); if (virDomainDefHasMemoryHotplug(def)) { - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("memory hotplug isn't supported by this QEMU binary")); - goto error; - } - - /* due to guest support, qemu would silently enable NUMA with one node - * once the memory hotplug backend is enabled. To avoid possible - * confusion we will enforce user originated numa configuration along - * with memory hotplug. */ - if (virDomainNumaGetNodeCount(def->numa) == 0) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("At least one numa node has to be configured when " - "enabling memory hotplug")); - goto error; - } - /* Use the 'k' suffix to let qemu handle the units */ virCommandAddArgFormat(cmd, "size=%lluk,slots=%u,maxmem=%lluk", virDomainDefGetMemoryInitial(def), @@ -9491,12 +9477,6 @@ qemuBuildCommandLine(virConnectPtr conn, /* memory hotplug requires NUMA to be enabled - we already checked * that memory devices are present only when NUMA is */ - if (def->nmems > def->mem.memory_slots) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("memory device count '%zu' exceeds slots count '%u'"), - def->nmems, def->mem.memory_slots); - goto error; - } for (i = 0; i < def->nmems; i++) { char *backStr; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index bdc0e67..cb50754 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3553,6 +3553,83 @@ qemuDomainMachineIsS390CCW(const virDomainDef *def) /** + * qemuDomainDefValidateMemoryHotplug: + * @def: domain definition + * @qemuCaps: qemu capabilities object + * @mem: definition of memory device that is to be added to @def with hotplug + * + * Validates that the domain definition and memory modules have valid + * configuration and are possibly able to accept @mem via hotplug if it's + * passed. + * + * Returns 0 on success; -1 and a libvirt error on error. + */ +int +qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, + virQEMUCapsPtr qemuCaps, + const virDomainMemoryDef *mem) +{ + unsigned int nmems = def->nmems; + unsigned long long hotplugSpace; + unsigned long long hotplugMemory = 0; + size_t i; + + hotplugSpace = def->mem.max_memory - virDomainDefGetMemoryInitial(def); + + if (mem) { + nmems++; + hotplugMemory = mem->size; + } + + if (!virDomainDefHasMemoryHotplug(def)) { + if (nmems) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("maxMemory has to be specified when using memory " + "devices ")); + return -1; + } + + return 0; + } + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("memory hotplug isn't supported by this QEMU binary")); + return -1; + } + + /* due to guest support, qemu would silently enable NUMA with one node + * once the memory hotplug backend is enabled. To avoid possible + * confusion we will enforce user originated numa configuration along + * with memory hotplug. */ + if (virDomainNumaGetNodeCount(def->numa) == 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("At least one numa node has to be configured when " + "enabling memory hotplug")); + return -1; + } + + if (nmems > def->mem.memory_slots) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device count '%u' exceeds slots count '%u'"), + nmems, def->mem.memory_slots); + return -1; + } + + for (i = 0; i < def->nmems; i++) + hotplugMemory += def->mems[i]->size; + + if (hotplugMemory > hotplugSpace) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("memory device total size exceeds hotplug space")); + return -1; + } + + return 0; +} + + +/** * qemuDomainUpdateCurrentMemorySize: * * Updates the current balloon size from the monitor if necessary. In case when diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 64cd7e1..23f5b1f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -482,4 +482,8 @@ bool qemuDomainMachineIsS390CCW(const virDomainDef *def); int qemuDomainUpdateCurrentMemorySize(virQEMUDriverPtr driver, virDomainObjPtr vm); +int qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, + virQEMUCapsPtr qemuCaps, + const virDomainMemoryDef *mem); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index afc5408..2c7f165 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1784,18 +1784,15 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, int id; int ret = -1; - if (vm->def->nmems == vm->def->mem.memory_slots) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("no free memory device slot available")); - goto cleanup; - } - if (virAsprintf(&mem->info.alias, "dimm%zu", vm->def->nmems) < 0) goto cleanup; if (virAsprintf(&objalias, "mem%s", mem->info.alias) < 0) goto cleanup; + if (qemuDomainDefValidateMemoryHotplug(vm->def, priv->qemuCaps, mem) < 0) + goto cleanup; + if (vm->def->mem.cur_balloon == virDomainDefGetMemoryActual(vm->def)) fix_balloon = true; -- 2.4.5

Aggregate the checks of the dimm device into the verification function rather than having them in the formatter. --- src/qemu/qemu_command.c | 65 ++------------------------------------ src/qemu/qemu_command.h | 4 +-- src/qemu/qemu_domain.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_hotplug.c | 2 +- 4 files changed, 86 insertions(+), 68 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a709db..5e7b052 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5318,44 +5318,8 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem, } -static bool -qemuCheckMemoryDimmConflict(virDomainDefPtr def, - virDomainMemoryDefPtr mem) -{ - size_t i; - - for (i = 0; i < def->nmems; i++) { - virDomainMemoryDefPtr tmp = def->mems[i]; - - if (tmp == mem || - tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) - continue; - - if (mem->info.addr.dimm.slot == tmp->info.addr.dimm.slot) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("memory device slot '%u' is already being " - "used by another memory device"), - mem->info.addr.dimm.slot); - return true; - } - - if (mem->info.addr.dimm.base != 0 && - mem->info.addr.dimm.base == tmp->info.addr.dimm.base) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("memory device base '0x%llx' is already being " - "used by another memory device"), - mem->info.addr.dimm.base); - return true; - } - } - - return false; -} - char * -qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem, - virDomainDefPtr def, - virQEMUCapsPtr qemuCaps) +qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem) { virBuffer buf = VIR_BUFFER_INITIALIZER; @@ -5367,35 +5331,10 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem, switch ((virDomainMemoryModel) mem->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_PC_DIMM)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("this qemu doesn't support the pc-dimm device")); - return NULL; - } - - if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && - mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("only 'dimm' addresses are supported for the " - "pc-dimm device")); - return NULL; - } - - if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && - mem->info.addr.dimm.slot >= def->mem.memory_slots) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("memory device slot '%u' exceeds slots count '%u'"), - mem->info.addr.dimm.slot, def->mem.memory_slots); - return NULL; - } - virBufferAsprintf(&buf, "pc-dimm,node=%d,memdev=mem%s,id=%s", mem->targetNode, mem->info.alias, mem->info.alias); if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { - if (qemuCheckMemoryDimmConflict(def, mem)) - return NULL; - virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot); virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base); } @@ -9486,7 +9425,7 @@ qemuBuildCommandLine(virConnectPtr conn, qemuCaps, cfg))) goto error; - if (!(dimmStr = qemuBuildMemoryDeviceStr(def->mems[i], def, qemuCaps))) { + if (!(dimmStr = qemuBuildMemoryDeviceStr(def->mems[i]))) { VIR_FREE(backStr); goto error; } diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 4aa7f2d..c7ed300 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -173,9 +173,7 @@ int qemuBuildMemoryBackendStr(unsigned long long size, virJSONValuePtr *backendProps, bool force); -char *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem, - virDomainDefPtr def, - virQEMUCapsPtr qemuCaps); +char *qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem); /* Legacy, pre device support */ char *qemuBuildPCIHostdevPCIDevStr(virDomainHostdevDefPtr dev, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index cb50754..1474a5b 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3552,6 +3552,79 @@ qemuDomainMachineIsS390CCW(const virDomainDef *def) } +static bool +qemuCheckMemoryDimmConflict(const virDomainDef *def, + const virDomainMemoryDef *mem) +{ + size_t i; + + for (i = 0; i < def->nmems; i++) { + virDomainMemoryDefPtr tmp = def->mems[i]; + + if (tmp == mem || + tmp->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) + continue; + + if (mem->info.addr.dimm.slot == tmp->info.addr.dimm.slot) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device slot '%u' is already being " + "used by another memory device"), + mem->info.addr.dimm.slot); + return true; + } + + if (mem->info.addr.dimm.base != 0 && + mem->info.addr.dimm.base == tmp->info.addr.dimm.base) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device base '0x%llx' is already being " + "used by another memory device"), + mem->info.addr.dimm.base); + return true; + } + } + + return false; +} +static int +qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, + const virDomainDef *def) +{ + switch ((virDomainMemoryModel) mem->model) { + case VIR_DOMAIN_MEMORY_MODEL_DIMM: + if (mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM && + mem->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("only 'dimm' addresses are supported for the " + "pc-dimm device")); + return -1; + } + + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { + if (mem->info.addr.dimm.slot >= def->mem.memory_slots) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("memory device slot '%u' exceeds slots " + "count '%u'"), + mem->info.addr.dimm.slot, def->mem.memory_slots); + return -1; + } + + + if (qemuCheckMemoryDimmConflict(def, mem)) + return -1; + } + break; + + case VIR_DOMAIN_MEMORY_MODEL_NONE: + case VIR_DOMAIN_MEMORY_MODEL_LAST: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid memory device type")); + return -1; + } + + return 0; +} + + /** * qemuDomainDefValidateMemoryHotplug: * @def: domain definition @@ -3616,9 +3689,17 @@ qemuDomainDefValidateMemoryHotplug(const virDomainDef *def, return -1; } - for (i = 0; i < def->nmems; i++) + for (i = 0; i < def->nmems; i++) { hotplugMemory += def->mems[i]->size; + if (qemuDomainDefValidateMemoryHotplugDevice(def->mems[i], def) < 0) + return -1; + } + + if (mem && + qemuDomainDefValidateMemoryHotplugDevice(mem, def) < 0) + return -1; + if (hotplugMemory > hotplugSpace) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("memory device total size exceeds hotplug space")); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2c7f165..d3630d7 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -1796,7 +1796,7 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (vm->def->mem.cur_balloon == virDomainDefGetMemoryActual(vm->def)) fix_balloon = true; - if (!(devstr = qemuBuildMemoryDeviceStr(mem, vm->def, priv->qemuCaps))) + if (!(devstr = qemuBuildMemoryDeviceStr(mem))) goto cleanup; qemuDomainMemoryDeviceAlignSize(vm->def, mem); -- 2.4.5

We check that <maxMemory> is present when we have memory devices at the time we start the VM or add the device, so we can remove it from the post parse callback to be more tolerant to users. --- src/qemu/qemu_domain.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1474a5b..a22e5ad 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1347,14 +1347,6 @@ qemuDomainDeviceDefPostParse(virDomainDeviceDefPtr dev, } } - if (dev->type == VIR_DOMAIN_DEVICE_MEMORY && - !virDomainDefHasMemoryHotplug(def)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("maxMemory has to be specified when using memory " - "devices ")); - goto cleanup; - } - ret = 0; cleanup: -- 2.4.5

Adjust the config code so that it does not enforce that target memory node is specified. To avoid breakage, adjust the qemu memory hotplug config checker to disallow such config for now. --- docs/formatdomain.html.in | 5 +++-- docs/schemas/domaincommon.rng | 8 +++++--- src/conf/domain_conf.c | 10 +++++++--- src/qemu/qemu_domain.c | 7 +++++++ 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index c88b032..279424d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -6300,8 +6300,9 @@ qemu-kvm -net nic,model=? /dev/null added memory as a scaled integer. </p> <p> - The mandatory <code>node</code> subelement configures the guest NUMA - node to attach the memory to. + The <code>node</code> subelement configures the guest NUMA node to + attach the memory to. Note: Some hypervisors or specific + configurations may require that <code>node</code> is specified. </p> </dd> </dl> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index f196177..994face 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -4532,9 +4532,11 @@ <element name="size"> <ref name="scaledInteger"/> </element> - <element name="node"> - <ref name="unsignedInt"/> - </element> + <optional> + <element name="node"> + <ref name="unsignedInt"/> + </element> + </optional> </interleave> </element> </define> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 514bd8a..a5ab29c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12520,11 +12520,15 @@ virDomainMemoryTargetDefParseXML(xmlNodePtr node, int ret = -1; xmlNodePtr save = ctxt->node; ctxt->node = node; + int rv; - if (virXPathInt("string(./node)", ctxt, &def->targetNode) < 0 || - def->targetNode < 0) { + /* initialize to value which marks that the user didn't speify it */ + def->targetNode = -1; + + if ((rv = virXPathInt("string(./node)", ctxt, &def->targetNode)) == -2 || + (rv == 0 && def->targetNode < 0)) { virReportError(VIR_ERR_XML_ERROR, "%s", - _("invalid or missing value of memory device node")); + _("invalid value of memory device node")); goto cleanup; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a22e5ad..29fed1d 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -3591,6 +3591,13 @@ qemuDomainDefValidateMemoryHotplugDevice(const virDomainMemoryDef *mem, return -1; } + if (mem->targetNode == -1) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("target NUMA node needs to be specifed for memory " + "device")); + return -1; + } + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { if (mem->info.addr.dimm.slot >= def->mem.memory_slots) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, -- 2.4.5

On Tue, Oct 20, 2015 at 17:31:57 -0400, John Ferlan wrote: They can't be inverted, since the later patches rely on the fact that the node is now a signed variable. Undoing that would break the separation. You'll have to live with it. Well, I'd say that you can hotplug memory on PPC64 without needing NUMA in any way. Yes that would work, but it would basically test the same code. That statement is untrue. PPC64 does support guest NUMA nodes. It works with the current code. This is a usability improvement. Um, what? In the first sentence this merely drops 'mandatory'. Rewording can be saved for later. If you enable NUMA for your guest, you have to specify the NUMA node for the memory module, so your wording is insufficient. I'll reword it though, but differently. ... the only memory region the guest is using. Since it doesn't have NUMA. It can be ommited if and only if NUMA is not enabled. BTW there's a bug in the code in this regard. The slot wouldn't be checked on PPC if NUMA was enabled. It obviously does. Not really. Sacrificing the sign bit for this is sufficient here. Peter

Prepare the command line generator for the possibility that in some configurations the target NUMA node info will be missing. --- src/qemu/qemu_command.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5e7b052..9815732 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5331,8 +5331,13 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem) switch ((virDomainMemoryModel) mem->model) { case VIR_DOMAIN_MEMORY_MODEL_DIMM: - virBufferAsprintf(&buf, "pc-dimm,node=%d,memdev=mem%s,id=%s", - mem->targetNode, mem->info.alias, mem->info.alias); + virBufferAddLit(&buf, "pc-dimm,"); + + if (mem->targetNode >= 0) + virBufferAsprintf(&buf, "node=%d,", mem->targetNode); + + virBufferAsprintf(&buf, "memdev=mem%s,id=%s", + mem->info.alias, mem->info.alias); if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) { virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot); -- 2.4.5