Re: [Qemu-ppc] [PATCH for-2.11] spapr: reset DRCs after devices

[David Gibson]

On Fri, Nov 17, 2017 at 01:56:48PM +0100, Greg Kurz wrote:
> A DRC with a pending unplug request releases its associated device at
> machine reset time.
> 
> In the case of LMB, when all DRCs for a DIMM device have been reset,
> the DIMM gets unplugged, causing guest memory to disappear. This may
> be very confusing for anything still using this memory.
> 
> This is exactly what happens with vhost backends, and QEMU aborts
> with:
> 
> qemu-system-ppc64: used ring relocated for ring 2
> qemu-system-ppc64: qemu/hw/virtio/vhost.c:649: vhost_commit: Assertion
>  `r >= 0' failed.
> 
> The issue is that each DRC registers a QEMU reset handler, and we
> don't control the order in which these handlers are called (ie,
> a LMB DRC will unplug a DIMM before the virtio device using the
> memory on this DIMM could stop its vhost backend).
> 
> To avoid such situations, let's reset DRCs after all devices
> have been reset.
> 
> Reported-by: Mallesh N. Koti <address@hidden>
> Signed-off-by: Greg Kurz <address@hidden>

Applied to ppc-for-2.11.  I'll be looking at preparing a pull request
today.

> ---
>  hw/ppc/spapr.c     |   21 +++++++++++++++++++++
>  hw/ppc/spapr_drc.c |    7 -------
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 6841bd294b3c..6285f7211f9a 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1411,6 +1411,19 @@ static void find_unknown_sysbus_device(SysBusDevice 
> *sbdev, void *opaque)
>      }
>  }
>  
> +static int spapr_reset_drcs(Object *child, void *opaque)
> +{
> +    sPAPRDRConnector *drc =
> +        (sPAPRDRConnector *) object_dynamic_cast(child,
> +                                                 TYPE_SPAPR_DR_CONNECTOR);
> +
> +    if (drc) {
> +        spapr_drc_reset(drc);
> +    }
> +
> +    return 0;
> +}
> +
>  static void ppc_spapr_reset(void)
>  {
>      MachineState *machine = MACHINE(qdev_get_machine());
> @@ -1434,6 +1447,14 @@ static void ppc_spapr_reset(void)
>      }
>  
>      qemu_devices_reset();
> +
> +    /* DRC reset may cause a device to be unplugged. This will cause troubles
> +     * if this device is used by another device (eg, a running vhost backend
> +     * will crash QEMU if the DIMM holding the vring goes away). To avoid 
> such
> +     * situations, we reset DRCs after all devices have been reset.
> +     */
> +    object_child_foreach_recursive(object_get_root(), spapr_reset_drcs, 
> NULL);
> +
>      spapr_clear_pending_events(spapr);
>  
>      /*
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 915e9b51c40c..e3b122968e89 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -455,11 +455,6 @@ void spapr_drc_reset(sPAPRDRConnector *drc)
>      }
>  }
>  
> -static void drc_reset(void *opaque)
> -{
> -    spapr_drc_reset(SPAPR_DR_CONNECTOR(opaque));
> -}
> -
>  bool spapr_drc_needed(void *opaque)
>  {
>      sPAPRDRConnector *drc = (sPAPRDRConnector *)opaque;
> @@ -518,7 +513,6 @@ static void realize(DeviceState *d, Error **errp)
>      }
>      vmstate_register(DEVICE(drc), spapr_drc_index(drc), &vmstate_spapr_drc,
>                       drc);
> -    qemu_register_reset(drc_reset, drc);
>      trace_spapr_drc_realize_complete(spapr_drc_index(drc));
>  }
>  
> @@ -529,7 +523,6 @@ static void unrealize(DeviceState *d, Error **errp)
>      gchar *name;
>  
>      trace_spapr_drc_unrealize(spapr_drc_index(drc));
> -    qemu_unregister_reset(drc_reset, drc);
>      vmstate_unregister(DEVICE(drc), &vmstate_spapr_drc, drc);
>      root_container = container_get(object_get_root(), DRC_CONTAINER_PATH);
>      name = g_strdup_printf("%x", spapr_drc_index(drc));
> 

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

signature.asc
Description: PGP signature