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,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list