On Tue, Oct 20, 2015 at 09:57:49 -0400, John Ferlan wrote:

On 10/16/2015 08:11 AM, Peter Krempa wrote:

...

Since we already make sure before that the domain configuration is valid we may execute it always at the cost of doing 0 iterations of the for loop.

This patch will simplify later refactor as it will avoid whitespace changes. --- src/qemu/qemu_command.c | 47 +++++++++++++++++++++++------------------------ 1 file changed, 23 insertions(+), 24 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index a37a4fa..43e81c7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -9484,38 +9484,37 @@ qemuBuildCommandLine(virConnectPtr conn, } }

- if (virDomainNumaGetNodeCount(def->numa)) { - if (qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0) + if (virDomainNumaGetNodeCount(def->numa) && + qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps, nodeset) < 0) goto error;

- /* memory hotplug requires NUMA to be enabled - we already checked - * that memory devices are present only when NUMA is */ + /* memory hotplug requires NUMA to be enabled - we already checked + * that memory devices are present only when NUMA is */

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.

Since the next commit is actually moving the place where it's checked and the later commits invalidate that commit since it will work even without NUMA at all I will not bother modifying it here. Peter

On 10/16/2015 08:11 AM, Peter Krempa wrote:

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 */

Does the comment make sense any more?

- 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:

Not necessarily called only in Hotplug path, but I understand why it's named this way since *HasMemoryHotplug uses similar naming and adding a Capable on the end is an unnecessarily long name.

+ * @def: domain definition + * @qemuCaps: qemu capabilities object + * @mem: definition of memory device that is to be added to @def with hotplug

or NULL when ...

+ * + * 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;

I think you'd have to move qemuDomainMemoryDeviceAlignSize to sooner in the caller so that this calculation and future check is more correct.

+ } + + if (!virDomainDefHasMemoryHotplug(def)) { + if (nmems) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("maxMemory has to be specified when using memory " + "devices "));

extraneous space ^ Although I have to say the check and error reads strange... The *Has function returns true when memory_slots || max_memory > 0. So while it's obvious knowing the code and XML that maxMemory needs to be defined in order for hotplug to work, it just a strange message to see for someone perhaps passing memory device XML. Essentially there's a failure because someone was requesting to hotplug a memory device, but the domain doesn't support it. Perhaps, "cannot hotplug a memory device when domain 'maxMemory' is not defined" ?

+ 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);

This is one of those cases where different error messages based on operation may be useful. In one case (hotplug) one cannot add a memory device because there's no slots, while the other case (domain startup) we cannot startup the domain because we have more 'nmems' provided than 'memory_slots' available.

+ 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; +

Why wait? It's not like validate routine is checking the alias. ACK with a couple of tweaks John

if (vm->def->mem.cur_balloon == virDomainDefGetMemoryActual(vm->def)) fix_balloon = true;

On 10/16/2015 08:11 AM, Peter Krempa wrote:

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 we are in the hotplug path (qemuDomainAttachMemory), then this is duplicitous. IOW: Why check something we've already validated when we started? In this case "if (!mem &&" would reduce the extra check, but perhaps be confusing or strange to read.

+ } + + if (mem && + qemuDomainDefValidateMemoryHotplugDevice(mem, def) < 0) + return -1; +

In my mind - this would be cleaner if combined with the above first "if (mem)" condition... Your call on moving it though... at least the check is there. ACK - although I do think the tweak to not validate the already validated would be appropriate. John

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);

On 10/16/2015 08:11 AM, Peter Krempa wrote:

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(-)

Hmm.. we never had a DO_TEST_PARSE_FAILURE test on this...

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 "));

Well at least I know where you got that error message in patch 5... it almost feels as if this could be merged there to make it more obvious. ACK - regardless of whether it's moved or not. John

- goto cleanup; - } - ret = 0;

cleanup:

On 10/16/2015 08:11 AM, Peter Krempa wrote:

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(-)

It almost feels like patches 9 & 10 should precede this one. Perhaps easier with the html.in change. Consider this as a review for 8-10 as well as some other random thoughts. So if I'm reading things correctly, a PPC64 guest "can" supply NUMA guest nodes, but it doesn't have to. Is that a fair assumption? It matters mostly to help describe things. If it can have it, then perhaps another test (copy qemuxml2argv-memory-hotplug-ppc64-nonuma.xml to qemuxml2argv-memory-hotplug-ppc64.xml and add the numa node defs). If PPC64 doesn't support NUMA guest nodes, then does it make sense in post processing code to set targetNode = -1 if ARCH_IS_PPC64? It wouldn't be the first PPC64 specific code. Along the same lines, would be ignoring targetNode in patch 9 if PPC64. Furthermore, if PPC64 doesn't support NUMA guest nodes, I would have expected a check during parse - I didn't look that hard for this case. I was merely thinking what happens if PPC64 does define NUMA guest nodes and those can be used by the memory device.

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.

How about: The optional <code>node</code> subelement provides the guest <a href="#elementsCPU">cpu NUMA node or cell id</a> to attach the memory. <p> Note: The hypervisor may require the <code>node</code> to be specified for some architectures.</p> FWIW: It almost feels like we need an example of what is meant - that is "For QEMU/KVM, the PowerPC64 pseries guests do not require that the <code>node</code> subelement is defined. If not defined, then the memory device will be attached to [???]." BTW: The additional text only makes sense as of patch 10 and it wasn't clear if PPC64 "could" support numa guest nodes.

</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 */

specify This feels like a need for something like the Tristate{Bool|State} structs... Or way to force optional fields to something specific

+ 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"));

...value for memory...

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 "

specified Before an explicit ACK - a few adjustments I think would be beneficial. John

+ "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,

On 11/10/2015 08:34 AM, Peter Krempa wrote:

On Tue, Oct 20, 2015 at 17:31:57 -0400, John Ferlan wrote:

...

On 10/16/2015 08:11 AM, Peter Krempa wrote:

...

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(-)

Let's see... can I recall what I was thinking 3 weeks ago (and today was a rough one - long story).

...

It almost feels like patches 9 & 10 should precede this one. Perhaps easier with the html.in change. Consider this as a review for 8-10 as well as some other random thoughts.

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.

OK - perhaps I was thinking more along the lines of the docs and how things were flowing, but sure I see how 10 comes after 8 (perhaps I had combined them in my mind)

...

So if I'm reading things correctly, a PPC64 guest "can" supply NUMA guest nodes, but it doesn't have to. Is that a fair assumption? It

Well, I'd say that you can hotplug memory on PPC64 without needing NUMA in any way.

Maybe I was missing that distinction - I cannot recall. But as you elaborate in the following having NUMA on PPC64 is supported so one could attach using nodes or not. That makes the rest of my comments below null and void. John

...

matters mostly to help describe things. If it can have it, then perhaps another test (copy qemuxml2argv-memory-hotplug-ppc64-nonuma.xml to qemuxml2argv-memory-hotplug-ppc64.xml and add the numa node defs).

Yes that would work, but it would basically test the same code.

...

If PPC64 doesn't support NUMA guest nodes, then does it make sense in post processing code to set targetNode = -1 if ARCH_IS_PPC64? It

That statement is untrue. PPC64 does support guest NUMA nodes. It works with the current code. This is a usability improvement.

...

wouldn't be the first PPC64 specific code. Along the same lines, would be ignoring targetNode in patch 9 if PPC64. Furthermore, if PPC64 doesn't support NUMA guest nodes, I would have expected a check during parse - I didn't look that hard for this case. I was merely thinking what happens if PPC64 does define NUMA guest nodes and those can be used by the memory device.

Um, what?

...

...

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.

How about:

The optional <code>node</code> subelement provides the guest <a href="#elementsCPU">cpu NUMA node or cell id</a> to attach the memory.

In the first sentence this merely drops 'mandatory'. Rewording can be saved for later.

...

<p> Note: The hypervisor may require the <code>node</code> to be specified for some architectures.</p>

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.

...

FWIW: It almost feels like we need an example of what is meant - that is "For QEMU/KVM, the PowerPC64 pseries guests do not require that the <code>node</code> subelement is defined. If not defined, then the memory device will be attached to [???]."

... 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.

...

BTW: The additional text only makes sense as of patch 10 and it wasn't clear if PPC64 "could" support numa guest nodes.

It obviously does.

...

...

</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 */

specify

This feels like a need for something like the Tristate{Bool|State} structs... Or way to force optional fields to something specific

Not really. Sacrificing the sign bit for this is sufficient here.

Peter