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

[Peter Krempa]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20161110/f5211ddc/attachment-0001.sig>