On Wed, Nov 09, 2016 at 18:40:28 -0500, John Ferlan wrote:

On 11/03/2016 02:12 AM, Peter Krempa wrote:

...

Detect on reconnect to a running qemu VM whether the alias of a hotpluggable memory device (dimm) does not match the dimm slot number where it's connected to. This is necessary as qemu is actually considering the alias as machine ABI used to connect the backend object to the dimm device.

This will require us to keep them consistent so that we can reliably restore them on migration. In some situations it was currently possible to create a mismatched configuration and qemu would refuse to restore the migration stream.

To avoid breaking existing VMs we'll need to keep the old algorithm though. --- src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+)

diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 2ee1829..1b7b375 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -232,6 +232,9 @@ struct _qemuDomainObjPrivate { /* private XML) - need to restore at process reconnect */ uint8_t *masterKey; size_t masterKeyLen; + + /* note whether memory device alias does not correspond to slot number */ + bool memHotplugAliasMismatch; };

# define QEMU_DOMAIN_PRIVATE(vm) \ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b67aee..15b8ec8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3205,6 +3205,29 @@ qemuDomainPerfRestart(virDomainObjPtr vm) return 0; }

+ +static void +qemuProcessReconnectCheckMemoryHotplugMismatch(virDomainObjPtr vm) +{ + size_t i; + int aliasidx; + virDomainDefPtr def = vm->def; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!virDomainDefHasMemoryHotplug(def) || def->nmems == 0) + return; + + for (i = 0; i < def->nmems; i++) { + aliasidx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, "dimm");

When/how does the info.alias get filled in during restart processing?

The live definition along with aliases is saved to the status XML and then reloaded from the disk. Otherwise if we'd not remember the aliases the whole hotunplug code would not work.

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.

The aliases are reloaded so they are valid. Re-assigning them would break all hotunplug if you generate a different alias. It works this way in other parts for quite a long time now.

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

As most other code that needs aliases after restart ...

wouldn't be defined, thus wouldn't a hotplug after libvirtd restart result in "dimm0"?

Nope. Both paragraphs don't make sense due to the fact above.

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.

No need. It's saved by libvirt.

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.

No it's explicitly stored into the live XML. The address and slot number are necessary to ensure migration compatibility.

This way - we shouldn't need all of patch4, I think... Or at least we

Patch 4 is necessary as QEMU actually makes the alias part of the qemu migration stream. This effectively makes the alias part of the machine ABI. If the aliases don't match on migration qemu rejects it.

wouldn't need the memHotplugAliasMismatch logic. Forcing the alias ID to match the slot would probably still be good goal - still doesn't mean

That is a necessary goal. The whole purpose of this series!

the mems list is ordered 0..def->mem.memory_slots. You could have 0,4,2,1,3...

Exactly. Due do the fact above the alias needs to be tied to the slot number rather than the order. This patch is to make sure that if we employed the wrong alias alocation scheme the code will not refuse to hotplug memory into a existing VM that has mismatched slots and aliases.

On Thu, Nov 10, 2016 at 08:56:48 -0500, John Ferlan wrote:

On 11/10/2016 04:11 AM, Peter Krempa wrote:

...

On Wed, Nov 09, 2016 at 18:40:28 -0500, John Ferlan wrote:

...

On 11/03/2016 02:12 AM, Peter Krempa wrote:

...

Detect on reconnect to a running qemu VM whether the alias of a hotpluggable memory device (dimm) does not match the dimm slot number where it's connected to. This is necessary as qemu is actually considering the alias as machine ABI used to connect the backend object to the dimm device.

This will require us to keep them consistent so that we can reliably restore them on migration. In some situations it was currently possible to create a mismatched configuration and qemu would refuse to restore the migration stream.

To avoid breaking existing VMs we'll need to keep the old algorithm though. --- src/qemu/qemu_domain.h | 3 +++ src/qemu/qemu_process.c | 25 +++++++++++++++++++++++++ 2 files changed, 28 insertions(+)

[...]

...

...

...

diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1b67aee..15b8ec8 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3205,6 +3205,29 @@ qemuDomainPerfRestart(virDomainObjPtr vm) return 0; }

+ +static void +qemuProcessReconnectCheckMemoryHotplugMismatch(virDomainObjPtr vm) +{ + size_t i; + int aliasidx; + virDomainDefPtr def = vm->def; + qemuDomainObjPrivatePtr priv = vm->privateData; + + if (!virDomainDefHasMemoryHotplug(def) || def->nmems == 0) + return; + + for (i = 0; i < def->nmems; i++) { + aliasidx = qemuDomainDeviceAliasIndex(&def->mems[i]->info, "dimm");

When/how does the info.alias get filled in during restart processing?

The live definition along with aliases is saved to the status XML and then reloaded from the disk.

Otherwise if we'd not remember the aliases the whole hotunplug code would not work.

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

Okay, I'll go with the former, since that describes exactly the details.

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

It should not ever happen, but I thought about this option originally. The code tolerates this for local usage since it's not really fatal, but rejects migration if the slot or base address is missing.

On 11/03/2016 02:12 AM, Peter Krempa wrote:

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

So this forces the write of the <address type='dimm' slot='#'/> to the guest config XML, but we still don't explain that in formatdomain.html - perhaps by this point we should. The code looks fine to me, but I would think you'd need the previous patches in place/adjusted before pushing this one. ACK John

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>

On 11/03/2016 02:12 AM, Peter Krempa wrote:

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)

Whether the oldAlias is necessary depends on patch 2 feedback...

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

This has assumed that info.alias is defined - perhaps not necessarily true after libvirtd restart... Luckily we haven't had any issues yet. At the very least it would be obvious as I would think qemu would fail the attach reqeust due to duplicate ID.

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

And this is probably our root cause... It also makes the assumption that whatever <address> is there is numerically ordered with dimm.slot, although as your XML from patch1 proves we can have slot='0' and slot='2' At least it's "consistent" (partially) to the theory of ordering mems and assigning aliases in increasing order

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

'ble' - cut-n-paste? or some other name used during development? Let's see how the fallout of patch2 goes and then think about this. John

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 \

On Thu, Nov 10, 2016 at 08:58:10 -0500, John Ferlan wrote:

On 11/03/2016 02:12 AM, Peter Krempa wrote:

...

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

Revisiting after my face-palm for patch2.

...

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.

s/old/use (multiple places)

I kept using oldAlias, as I think the variable should emphasize that the old approach is used. I can change it if you think otherwise. I will be following up with a patch adding docs for the address anyways. Pushed; Thanks. Peter