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

[Michael Roth]

Quoting Igor Mammedov (2017-06-09 03:27:33)
> On Thu, 08 Jun 2017 15:00:53 -0500
> Michael Roth <address@hidden> 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 <address@hidden>  
> > > 
> > > 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's an option, but subsections were only really used for backward
compatibility. Not sure how much we have to gain from using both.

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

Maybe I'm misunderstanding the intent of this patch, but in our own
testing we've seen that even for CPUs hotplugged *before* migration
starts, libvirt will add them to the dest via device_add instead of
via the command-line.

If the CPUs were all specified via command-line, I don't think these
patches would be needed, since the coldplug hooks would be executed
without the need to make any special considerations for INMIGRATE.

This libvirt commit seems to confirm that the CPUs are added via
device_add, and we've seen similar behavior in our testing:


commit 9eb9106ea51b43102ee51132f69780b2c86ccbca
Author: Peter Krempa <address@hidden>
Date:   Thu Aug 4 14:36:24 2016 +0200

    qemu: command: Add support for sparse vcpu topologies
    
    Add support for using the new approach to hotplug vcpus using device_add
    during startup of qemu to allow sparse vcpu topologies.
    
    There are a few limitations imposed by qemu on the supported
    configuration:
    - vcpu0 needs to be always present and not hotpluggable
    - non-hotpluggable cpus need to be ordered at the beginning
    - order of the vcpus needs to be unique for every single hotpluggable
      entity
    
    Qemu also doesn't really allow to query the information necessary to
    start a VM with the vcpus directly on the commandline. Fortunately they
    can be hotplugged during startup.
    
    The new hotplug code uses the following approach:
    - non-hotpluggable vcpus are counted and put to the -smp option
    - qemu is started
    - qemu is queried for the necessary information
    - the configuration is checked
    - the hotpluggable vcpus are hotplugged
    - vcpus are started
    
    This patch adds a lot of checking code and enables the support to
    specify the individual vcpu element with qemu.


So I don't think disabling migration during inmigrate is a possible
alternative unless we rework how libvirt handles this. The only
alternative to this patch that I'm aware of would be to always
migrate DRCs when dev->hotplugged == true.