Re: [libvirt] [PATCH 2/4] qemu: process: detect if dimm aliases are broken on reconnect

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

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