On Fri, Nov 17, 2017 at 06:42:25PM -0200, Daniel Henrique Barboza wrote: > > > On 11/17/2017 04:19 PM, Michael Roth wrote: > > Quoting Daniel Henrique Barboza (2017-11-17 10:21:27) > > > > > > On 11/17/2017 10:56 AM, 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 <mall...@linux.vnet.ibm.com> > > > > Signed-off-by: Greg Kurz <gr...@kaod.org> > > > > --- > > > I believe this has to do with the problem discussed at the memory > > > unplug reboot with vhost=on, right? > > > > > > I don't see any problem with this patch and it's probably the best > > > solution > > > short-term for the problem (and potentially other related LMB release > > > problems), but that said, how hard it is to forbid LMB unplug at all if > > > the > > > memory is being used in vhost? > > > > > > The way I see it, the root problem is that a device unplug failed at the > > > guest > > > side and we don't know about it, leaving drc->unplug_requested flags set > > > around the code when it shouldn't. Reading that thread I see a comment > > > from > > I'm not sure we want to go down the road of preemptively aborting > > specific cases where we think unplug will fail in the guest. We already > > need to deal with the cases where we don't know whether they'll fail > > ahead of time, so it seems like more of a headache to add special cases > > on top of that that management would need to deal with. > > Just to be clear, I only advocate this kind of constraint if the LMB unplug, > at this specific scenario, has a 100% chance of fail. Otherwise we're > better of rolling the dice. > > > > > > And in this particular case we'd never be able to unplug the DIMM if > > the guest happens to use it for the vring each boot (unless maybe the > > unplug request occured during early boot), even though there's no > > technical reason we shouldn't be able to at least handle it on the > > next reset. > > I am ok going this route if we have our bases covered. Does this > behavior of unplugging the LMBs pending unplug at reset documented > in the PAPR specs? If not, we would need to document it somewhere else > (or ask for a PAPR revision to add it).
Well, the LMB is supposed to be released once the guest has vacated
it. At reset, the guest can't be using it any more, so..
> Otherwise, I am not sure if management will be OK with LMBs being
> "inadvertently" hot unplugged in a reset due to a failed LMB hot unplug that
> might have occurred days ago and the user doesn't even remember it
> anymore :)
>
>
> Thanks,
>
>
> Daniel
> >
> > > Michael S. Tsirkin saying that this bug is somewhat expected, that vhost
> > > backend will
> > > not behave well if memory is going away. Unlike other LMB unplug
> > > failures from
> > > the guest side that might happen for any other reason, this one sees
> > > predicable
> > > and we can avoid it.
> > >
> > > I think this is something worth considering. But for now,
> > >
> > >
> > > Reviewed-by: Daniel Henrique Barboza <danie...@linux.vnet.ibm.com>
> > >
> > > > 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
