On 11/03/2016 02:12 AM, Peter Krempa wrote:
> Simplify handling of the 'dimm' address element by allowing to specify
> the slot number only. This will allow libvirt to allocate slot numbers
> before starting qemu.
> ---
> src/conf/domain_conf.c | 18 ++++++++++--------
> src/qemu/qemu_command.c | 3 ++-
> .../qemuxml2argv-memory-hotplug-dimm-addr.args | 2 ++
> .../qemuxml2argv-memory-hotplug-dimm-addr.xml | 11 +++++++++--
> tests/qemuxml2argvtest.c | 2 +-
> 5 files changed, 24 insertions(+), 12 deletions(-)
>
Interestingly both 'slot' and 'base' are optional in the
domaincommon.rng (dimmaddress). Neither are described in the
formatdomain.html.in which IIRC was a developer decision (I have a vague
recollection of doing a review and noting that)...
Still if an <address> was provided, 'slot' doesn't seem to have been
treated as optional in most places (used cscope to search on dimm.slot).
Even though it's optional, it seems qemuDomainUpdateMemoryDeviceInfo is
run in order to fill in the "running domain's" dimm.{slot,base} from the
qemuMonitorGetMemoryDeviceInfo; however, there's no "assurance" that
dimm.slot is the same as the alias dimm#. IOW: We've never guaranteed
that dimm.slot is the same as the dimm# alias number. It just happens to
work that way due to the algorithm or until someone comes along and
messes with the karma.
In any case, the code below seems fine (implied ACK), but I think that
domaincommon.rng could use a tweak to make 'slot' non optional, which
should be fine since <address> is still optional. Before you push
though, consider my comments in patch2.
John
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a233c0c..74efe8c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5007,7 +5007,8 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>
> case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM:
> virBufferAsprintf(buf, " slot='%u'", info->addr.dimm.slot);
> - virBufferAsprintf(buf, " base='0x%llx'", info->addr.dimm.base);
> + if (info->addr.dimm.base)
> + virBufferAsprintf(buf, " base='0x%llx'", info->addr.dimm.base);
>
> break;
>
> @@ -5408,14 +5409,15 @@ virDomainDeviceDimmAddressParseXML(xmlNodePtr node,
> }
> VIR_FREE(tmp);
>
> - if (!(tmp = virXMLPropString(node, "base")) ||
> - virStrToLong_ullp(tmp, NULL, 16, &addr->base) < 0) {
> - virReportError(VIR_ERR_XML_ERROR,
> - _("invalid or missing dimm base address '%s'"),
> - NULLSTR(tmp));
> - goto cleanup;
> + if ((tmp = virXMLPropString(node, "base"))) {
> + if (virStrToLong_ullp(tmp, NULL, 16, &addr->base) < 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("invalid dimm base address '%s'"), tmp);
> + goto cleanup;
> + }
> +
> + VIR_FREE(tmp);
> }
> - VIR_FREE(tmp);
>
> ret = 0;
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 96c4f31..4e530d1 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -3478,7 +3478,8 @@ qemuBuildMemoryDeviceStr(virDomainMemoryDefPtr mem)
>
> if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) {
> virBufferAsprintf(&buf, ",slot=%d", mem->info.addr.dimm.slot);
> - virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base);
> + if (mem->info.addr.dimm.base)
> + virBufferAsprintf(&buf, ",addr=%llu", mem->info.addr.dimm.base);
> }
>
> break;
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args
> index 1c881c6..23403df 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args
> @@ -15,6 +15,8 @@ QEMU_AUDIO_DRV=none \
> mem-path=/dev/hugepages2M/libvirt/qemu,size=536870912,host-nodes=1-3,\
> policy=bind \
> -device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0,addr=4294967296 \
> +-object memory-backend-ram,id=memdimm1,size=536870912 \
> +-device pc-dimm,node=0,memdev=memdimm1,id=dimm1,slot=2 \
> -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> -nographic \
> -nodefaults \
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.xml b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.xml
> index 49f2f10..fc21dc4 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.xml
> @@ -2,8 +2,8 @@
> <name>QEMUGuest1</name>
> <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> <maxMemory slots='16' unit='KiB'>1099511627776</maxMemory>
> - <memory unit='KiB'>743423</memory>
> - <currentMemory unit='KiB'>743423</currentMemory>
> + <memory unit='KiB'>7434230</memory>
> + <currentMemory unit='KiB'>7434230</currentMemory>
> <vcpu placement='static' cpuset='0-1'>2</vcpu>
> <os>
> <type arch='i686' machine='pc'>hvm</type>
> @@ -41,5 +41,12 @@
> </target>
> <address type='dimm' slot='0' base='0x100000000'/>
> </memory>
> + <memory model='dimm'>
> + <target>
> + <size unit='KiB'>524287</size>
> + <node>0</node>
> + </target>
> + <address type='dimm' slot='2'/>
> + </memory>
> </devices>
> </domain>
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index d025930..81dab9f 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -2095,7 +2095,7 @@ mymain(void)
> DO_TEST("memory-hotplug-dimm", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA,
> QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
> DO_TEST("memory-hotplug-dimm-addr", QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA,
> - QEMU_CAPS_OBJECT_MEMORY_FILE);
> + QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
> DO_TEST("memory-hotplug-ppc64-nonuma", QEMU_CAPS_KVM, QEMU_CAPS_DEVICE_PC_DIMM, QEMU_CAPS_NUMA,
> QEMU_CAPS_OBJECT_MEMORY_RAM, QEMU_CAPS_OBJECT_MEMORY_FILE);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list