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: 5939 bytes --]

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.

Specifically, what is it we're doing before the incoming migration
that's breaking things.  Even if it's unnecessary, anything done there
should be overwritten by the incoming stream.  That should certainly
be the case (now) for the DRC state variables.  Maybe not for the
queued hotplug events - but that means we should update the queue
migration to make sure we clear anything existing on the destination
before adding migrated events.

I'm also concerned by the fact that this makes changes for memory and
cpu hotplug, but not for PCI devices.  Why aren't they also affected
by this problem?

One nit in the implementation, see below:

> ---
>  hw/ppc/spapr.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 0980d73..f1302d0 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2511,6 +2511,12 @@ static void spapr_nmi(NMIState *n, int cpu_index, Error **errp)
>      }
>  }
>  
> +static bool spapr_coldplugged(DeviceState *dev)
> +{
> +    return runstate_check(RUN_STATE_INMIGRATE) ||
> +           !dev->hotplugged;
> +}
> +
>  static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                             uint32_t node, bool dedicated_hp_event_source,
>                             Error **errp)
> @@ -2521,6 +2527,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>      int i, fdt_offset, fdt_size;
>      void *fdt;
>      uint64_t addr = addr_start;
> +    bool coldplugged = spapr_coldplugged(dev);
>  
>      for (i = 0; i < nr_lmbs; i++) {
>          drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> @@ -2532,9 +2539,9 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>                                                  SPAPR_MEMORY_BLOCK_SIZE);
>  
>          drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -        drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, errp);
> +        drck->attach(drc, dev, fdt, fdt_offset, coldplugged, errp);
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
> -        if (!dev->hotplugged) {
> +        if (coldplugged) {
>              /* guests expect coldplugged LMBs to be pre-allocated */
>              drck->set_allocation_state(drc, SPAPR_DR_ALLOCATION_STATE_USABLE);
>              drck->set_isolation_state(drc, SPAPR_DR_ISOLATION_STATE_UNISOLATED);
> @@ -2543,7 +2550,7 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size,
>      /* send hotplug notification to the
>       * guest only in case of hotplugged memory
>       */
> -    if (dev->hotplugged) {
> +    if (!coldplugged) {
>          if (dedicated_hp_event_source) {
>              drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
>                      addr_start / SPAPR_MEMORY_BLOCK_SIZE);
> @@ -2776,6 +2783,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      int smt = kvmppc_smt_threads();
>      CPUArchId *core_slot;
>      int index;
> +    bool coldplugged = spapr_coldplugged(dev);
>  
>      core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
>      if (!core_slot) {
> @@ -2797,7 +2805,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>  
>      if (drc) {
>          sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -        drck->attach(drc, dev, fdt, fdt_offset, !dev->hotplugged, &local_err);
> +        drck->attach(drc, dev, fdt, fdt_offset, coldplugged, &local_err);
>          if (local_err) {
>              g_free(fdt);
>              error_propagate(errp, local_err);
> @@ -2805,7 +2813,7 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          }
>      }
>  
> -    if (dev->hotplugged) {
> +    if (!coldplugged) {
>          /*
>           * Send hotplug notification interrupt to the guest only in case
>           * of hotplugged CPUs.
> @@ -2838,7 +2846,7 @@ static void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      int node_id;
>      int index;
>  
> -    if (dev->hotplugged && !mc->has_hotpluggable_cpus) {
> +    if (!spapr_coldplugged(dev) && !mc->has_hotpluggable_cpus) {

It probably doesn't matter in practice, but in this specific instance,
I think you want the "raw" qemu meaning of hotplugged rather than the
spapr meaning.

>          error_setg(&local_err, "CPU hotplug not supported for this machine");
>          goto out;
>      }

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