On 11/03/2016 02:12 AM, Peter Krempa wrote: 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

On 11/03/2016 02:12 AM, Peter Krempa wrote: When/how does the info.alias get filled in during restart processing? I see it being defined during qemuAssignDeviceMemoryAlias and qemuAssignDeviceAliases. The former is only called in hotplug processing and the latter during qemuProcessPrepareDomain (domain startup). So I think the answer is we return -1 always here, but could be proved wrong. Thus, I think this is doomed from the start. I also wonder how existing code is affected since it's based on getting alias index - which wouldn't be defined, thus wouldn't a hotplug after libvirtd restart result in "dimm0"? The good news is we do have a way to fetch/return a 'meminfo' from qemuMonitorGetMemoryDeviceInfo which would be a hash table indexed by the ID's provided at startup. Thus we'd just need a mechanism to match 'mems' with each element of the returned meminfo hash table and "generate"/assign/steal the alias from that to mems. At the very least - whatever we set will be whatever we created or was hotplugged. It won't be stored in the config XML (yet), but it would seemingly be bug for bug compatible. This way - we shouldn't need all of patch4, I think... Or at least we wouldn't need the memHotplugAliasMismatch logic. Forcing the alias ID to match the slot would probably still be good goal - still doesn't mean the mems list is ordered 0..def->mem.memory_slots. You could have 0,4,2,1,3... It's late... been a long day... hopefully this all makes sense. John

On 11/10/2016 04:11 AM, Peter Krempa wrote: face-palm - I knew I was missing something obvious, but the brain just wasn't able to recognize it, <sigh>. Still consider the changed XML example from patch1 (without any hotplug), we have: <address type='dimm' slot='0' ...> => alias == "dimm0" <address type='dimm' slot='2' ...> => alias == "dimm1" so the bool could be "memAliasOrderMismatch" or "memAliasUnordered". Since it's not necessarily "Hotplug" related, I think changing the variable and function name should be done, but it's not a requirement for the ACK since it's understandable why Hotplug is used. John FWIW: One other oddball path that "might" disrupt things is the ignore_value(qemuDomainUpdateMemoryDeviceInfo(...) hotplug code where we're not "guaranteed" that the dimm.slot is filled in...

As with other devices assign the slot number right away when adding the device. This will make the slot numbers static as we do with other addressing elements and it will ultimately simplify allocation of the alias in a static way which does not break with qemu. --- src/qemu/qemu_domain_address.c | 91 ++++++++++++++++++++++ src/qemu/qemu_domain_address.h | 4 + src/qemu/qemu_hotplug.c | 3 + .../qemuxml2argv-hugepages-numa.args | 2 +- .../qemuxml2argv-memory-hotplug-dimm.args | 4 +- .../qemuxml2argv-memory-hotplug-ppc64-nonuma.args | 4 +- .../qemuxml2xmlout-memory-hotplug-dimm.xml | 2 + 7 files changed, 105 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index b35a95f..6ce3b1e 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1748,6 +1748,94 @@ qemuDomainUSBAddressAddHubs(virDomainDefPtr def) } +static virBitmapPtr +qemuDomainGetMemorySlotMap(const virDomainDef *def) +{ + virBitmapPtr ret; + virDomainMemoryDefPtr mem; + size_t i; + + if (!(ret = virBitmapNew(def->mem.memory_slots))) + return NULL; + + for (i = 0; i < def->nmems; i++) { + mem = def->mems[i]; + + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) + ignore_value(virBitmapSetBit(ret, mem->info.addr.dimm.slot)); + } + + return ret; +} + + +static int +qemuAssignMemoryDeviceSlot(virDomainMemoryDefPtr mem, + virBitmapPtr slotmap) +{ + ssize_t nextslot = -1; + + if (mem->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM) + return 0; + + if ((nextslot = virBitmapNextClearBit(slotmap, -1)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to find a emtpy memory slot")); + return -1; + } + + ignore_value(virBitmapSetBit(slotmap, nextslot)); + mem->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_DIMM; + mem->info.addr.dimm.slot = nextslot; + + return 0; +} + + +int +qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def, + virDomainMemoryDefPtr mem) +{ + virBitmapPtr slotmap = NULL; + int ret; + + if (!(slotmap = qemuDomainGetMemorySlotMap(def))) + return -1; + + ret = qemuAssignMemoryDeviceSlot(mem, slotmap); + + virBitmapFree(slotmap); + return ret; +} + + +static int +qemuDomainAssignMemorySlots(virDomainDefPtr def) +{ + virBitmapPtr slotmap = NULL; + int ret = -1; + size_t i; + + if (!virDomainDefHasMemoryHotplug(def)) + return 0; + + if (!(slotmap = qemuDomainGetMemorySlotMap(def))) + return -1; + + for (i = 0; i < def->nmems; i++) { + if (qemuAssignMemoryDeviceSlot(def->mems[i], slotmap) < 0) + goto cleanup; + } + + ret = 0; + + cleanup: + virBitmapFree(slotmap); + return ret; + +} + + static int qemuDomainAssignUSBAddresses(virDomainDefPtr def, virDomainObjPtr obj, @@ -1827,6 +1915,9 @@ qemuDomainAssignAddresses(virDomainDefPtr def, if (qemuDomainAssignUSBAddresses(def, obj, newDomain) < 0) return -1; + if (qemuDomainAssignMemorySlots(def) < 0) + return -1; + return 0; } diff --git a/src/qemu/qemu_domain_address.h b/src/qemu/qemu_domain_address.h index 11d6e92..ecb92b5 100644 --- a/src/qemu/qemu_domain_address.h +++ b/src/qemu/qemu_domain_address.h @@ -45,6 +45,10 @@ virDomainCCWAddressSetPtr qemuDomainCCWAddrSetCreateFromDomain(virDomainDefPtr def) ATTRIBUTE_NONNULL(1); +int qemuDomainAssignMemoryDeviceSlot(virDomainDefPtr def, + virDomainMemoryDefPtr mem); + + # define __QEMU_DOMAIN_ADDRESS_H__ #endif /* __QEMU_DOMAIN_ADDRESS_H__ */ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e06862c..75477cd 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2127,6 +2127,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (qemuDomainDefValidateMemoryHotplug(vm->def, priv->qemuCaps, mem) < 0) goto cleanup; + if (qemuDomainAssignMemoryDeviceSlot(vm->def, mem) < 0) + goto cleanup; + if (qemuAssignDeviceMemoryAlias(vm->def, mem) < 0) goto cleanup; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args index 2eb006e..7b90784 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hugepages-numa.args @@ -16,7 +16,7 @@ QEMU_AUDIO_DRV=spice \ -object memory-backend-file,id=memdimm0,prealloc=yes,\ mem-path=/dev/hugepages1G/libvirt/qemu,size=1073741824,host-nodes=1-3,\ policy=bind \ --device pc-dimm,node=0,memdev=memdimm0,id=dimm0 \ +-device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0 \ -uuid 63840878-0deb-4095-97e6-fc444d9bc9fa \ -nodefaults \ -monitor unix:/tmp/lib/domain--1-fedora/monitor.sock,server,nowait \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args index fa64fcf..1587aba 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm.args @@ -12,11 +12,11 @@ QEMU_AUDIO_DRV=none \ -smp 2,sockets=2,cores=1,threads=1 \ -numa node,nodeid=0,cpus=0-1,mem=214 \ -object memory-backend-ram,id=memdimm0,size=536870912 \ --device pc-dimm,node=0,memdev=memdimm0,id=dimm0 \ +-device pc-dimm,node=0,memdev=memdimm0,id=dimm0,slot=0 \ -object memory-backend-file,id=memdimm1,prealloc=yes,\ mem-path=/dev/hugepages2M/libvirt/qemu,size=536870912,host-nodes=1-3,\ policy=bind \ --device pc-dimm,node=0,memdev=memdimm1,id=dimm1 \ +-device pc-dimm,node=0,memdev=memdimm1,id=dimm1,slot=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ -nodefaults \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-ppc64-nonuma.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-ppc64-nonuma.args index 8a85fb1..475b721 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-ppc64-nonuma.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-ppc64-nonuma.args @@ -11,9 +11,9 @@ QEMU_AUDIO_DRV=none \ -m size=1310720k,slots=16,maxmem=4194304k \ -smp 1,sockets=1,cores=1,threads=1 \ -object memory-backend-ram,id=memdimm0,size=536870912 \ --device pc-dimm,memdev=memdimm0,id=dimm0 \ +-device pc-dimm,memdev=memdimm0,id=dimm0,slot=0 \ -object memory-backend-ram,id=memdimm1,size=536870912 \ --device pc-dimm,memdev=memdimm1,id=dimm1 \ +-device pc-dimm,memdev=memdimm1,id=dimm1,slot=1 \ -uuid 49545eb3-75e1-2d0a-acdd-f0294406c99e \ -nographic \ -nodefaults \ diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-dimm.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-dimm.xml index c5b5d75..be97e4e 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-dimm.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-memory-hotplug-dimm.xml @@ -47,6 +47,7 @@ <size unit='KiB'>523264</size> <node>0</node> </target> + <address type='dimm' slot='0'/> </memory> <memory model='dimm'> <source> @@ -57,6 +58,7 @@ <size unit='KiB'>524287</size> <node>0</node> </target> + <address type='dimm' slot='1'/> </memory> </devices> </domain> -- 2.10.2

The memory device alias needs to be treated as machine ABI as qemu is using it in the migration stream for section labels. To simplify this generate the alias from the slot number unless an existing broken configuration is detected. With this patch the aliases are predictable and even certain configurations which would not be migratable previously are fixed. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1359135 --- src/qemu/qemu_alias.c | 27 ++++++++++++++++++---- src/qemu/qemu_alias.h | 3 ++- src/qemu/qemu_hotplug.c | 4 +++- .../qemuxml2argv-memory-hotplug-dimm-addr.args | 4 ++-- 4 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index 9737158..8521a44 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -332,17 +332,34 @@ qemuAssignDeviceRNGAlias(virDomainDefPtr def, } +/** + * qemuAssignDeviceMemoryAlias: + * @def: domain definition. Necessary only if @oldAlias is true. + * @mem: memory device definition + * @oldAlias: Generate the alias according to the order of the device in @def + * rather than according to the slot number for legacy reasons. + * + * Generates alias for a memory device according to slot number if @oldAlias is + * false or according to order in @def->mems otherwise. + * + * Returns 0 on success, -1 on error. + */ int qemuAssignDeviceMemoryAlias(virDomainDefPtr def, - virDomainMemoryDefPtr mem) + virDomainMemoryDefPtr mem, + bool oldAlias) { size_t i; int maxidx = 0; int idx; - for (i = 0; i < def->nmems; i++) { - if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, "dimm")) >= maxidx) - maxidx = idx + 1; + if (oldAlias) { + for (i = 0; i < def->nmems; i++) { + if ((idx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, "dimm")) >= maxidx) + maxidx = idx + 1; + } + } else { + maxidx = mem->info.addr.dimm.slot; } if (virAsprintf(&mem->info.alias, "dimm%d", maxidx) < 0) @@ -475,7 +492,7 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return -1; } for (i = 0; i < def->nmems; i++) { - if (virAsprintf(&def->mems[i]->info.alias, "dimm%zu", i) < 0) + if (qemuAssignDeviceMemoryAlias(NULL, def->mems[i], false) < 0) return -1; } diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index d298a4d..1d93912 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -58,7 +58,8 @@ int qemuAssignDeviceRNGAlias(virDomainDefPtr def, virDomainRNGDefPtr rng); int qemuAssignDeviceMemoryAlias(virDomainDefPtr def, - virDomainMemoryDefPtr mems); + virDomainMemoryDefPtr mems, + bool ble); int qemuAssignDeviceShmemAlias(virDomainDefPtr def, virDomainShmemDefPtr shmem, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 75477cd..77176fb 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2130,7 +2130,9 @@ qemuDomainAttachMemory(virQEMUDriverPtr driver, if (qemuDomainAssignMemoryDeviceSlot(vm->def, mem) < 0) goto cleanup; - if (qemuAssignDeviceMemoryAlias(vm->def, mem) < 0) + /* in cases where we are using a VM with aliases generated according to the + * index of the memory device we need to keep continue using that scheme */ + if (qemuAssignDeviceMemoryAlias(vm->def, mem, priv->memHotplugAliasMismatch) < 0) goto cleanup; if (virAsprintf(&objalias, "mem%s", mem->info.alias) < 0) diff --git a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args index 23403df..fdbb4c3 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-memory-hotplug-dimm-addr.args @@ -15,8 +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 \ +-object memory-backend-ram,id=memdimm2,size=536870912 \ +-device pc-dimm,node=0,memdev=memdimm2,id=dimm2,slot=2 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -nographic \ -nodefaults \ -- 2.10.2

On 11/03/2016 02:12 AM, Peter Krempa wrote: Revisiting after my face-palm for patch2. s/old/use (multiple places) Since this is primarily for the startup path, then false is OK... Although I can only wonder what would happen in the QemuAttach path... s/ble/useAlias This ends up being a long line > 80 cols... ACK w/ variable name cleanup John