Re: [Qemu-devel] [PATCH] spapr: manage hotplugged devices while the VM is not started

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4325 bytes --]

On Fri, Jun 09, 2017 at 10:27:33AM +0200, Igor Mammedov wrote:
> On Thu, 08 Jun 2017 15:00:53 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > Quoting David Gibson (2017-05-30 23:35:57)
> > > On Tue, May 30, 2017 at 06:04:45PM +0200, Laurent Vivier wrote:  
> > > > For QEMU, a hotlugged device is a device added using the HMP/QMP
> > > > interface.
> > > > For SPAPR, a hotplugged device is a device added while the
> > > > machine is running. In this case QEMU doesn't update internal
> > > > state but relies on the OS for this part
> > > > 
> > > > In the case of migration, when we (libvirt) hotplug a device
> > > > on the source guest, we (libvirt) generally hotplug the same
> > > > device on the destination guest. But in this case, the machine
> > > > is stopped (RUN_STATE_INMIGRATE) and QEMU must not expect
> > > > the OS will manage it as an hotplugged device as it will
> > > > be "imported" by the migration.
> > > > 
> > > > This patch changes the meaning of "hotplugged" in spapr.c
> > > > to manage a QEMU hotplugged device like a "coldplugged" one
> > > > when the machine is awaiting an incoming migration.
> > > > 
> > > > Signed-off-by: Laurent Vivier <lvivier@redhat.com>  
> > > 
> > > So, I think this is a reasonable concept, at least in terms of
> > > cleanliness and not doing unnecessary work.  However, if it's fixing
> > > bugs, I suspect that means we still have problems elsewhere.  
> > 
> > I was hoping a lot of these issues would go away once we default
> > the initial/reset DRC states to "coldplugged". I think your pending
> > patch:
> > 
> >   "spapr: Make DRC reset force DRC into known state"
> > 
> > But I didn't consider the fact that libvirt will be issuing these
> > hotplugs *after* reset, so those states would indeed need to
> > be fixed up again to reflect boot-time,attached as opposed to
> > boot-time,unattached before starting the target.
> > 
> > So I do think this patch addresses a specific bug that isn't
> > obviously fixable elsewhere.
> > 
> > To me it seems like the only way to avoid doing something like
> > what this patch does is to migrate all attached DRCs from the
> > source in all cases.
> > 
> > This would break backward-migration though, unless we switch from
> > using subregions for DRCs to explicitly disabling DRC migration
> > based on machine type.
> we could leave old machines broken and fix only new machine types,
> then it would be easy ot migrate 'additional' DRC state as subsection
> only on new for new machines.
> 
> > 
> > That approach seems to similar to what x86 does, e.g.
> > hw/acpi/ich9.c and hw/acpi/piix.c migrate vmstate_memhp_state
> > (corresponding to all DIMMs' slot status) in all cases where
> > memory hotplug is enabled. If they were to do this using
> > subregions for DIMMs in a transitional state I think similar
> > issues would pop up in that code as well.
> > 
> > Even if we take this route, we still need to explicitly suppress
> > hotplug events during INMIGRATE to avoid extra events going on
> > the queue. *Unless* we similarly rely purely on the ones sent by
> > the source.
> pc/q35 might also lose events if device is hotplugged during migration,
> in addition migration would fail anyway since dst qemu
> should be launched with all devices that are present on src.
> 
> ex: consider if one hotplugs DIMM during migration, it creates
> RAM region mapped into guest and that region might be transferred
> as part of VMState (not sure if it even works)
> and considering dst qemu has no idea about hotplugged memory mapping,
> the migration would fail on receiving unknown VMState.
> 
> Hotplug generally doesn't work during migration, so it should be disabled
> in a generic way on migration start and re-enabled on target
> on migration completion. How about blocking device_add when
> INMIGRATE state and unblocking it when switching to runnig on dst?

Yeah, that sounds like a good idea.  We need to cover the case of
migration during an incomplete hot (un)plug as well as hotplug during
a migration.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]