| Message ID | 20171005192458.610-1-danielhb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 05, 2017 at 04:24:58PM -0300, Daniel Henrique Barboza wrote: > In cases where a device is hotplugged and hot-unplugged shortly after, > there is a chance of QEMU breaking with the following message: > > hw/ppc/spapr_drc.c:417:spapr_drc_detach: assertion failed: (drc->dev) > Aborted > > spapr_drc_detach makes a g_assert(drc->dev) to ensure that the following > spapr_drc_release call is able to execute the appropriate callback > using drc->dev as a parameter. However, in a scenario where a hotplug > is quickly followed by a hot-unplug, this g_assert can be reached before > the hotplug operation sets drc->dev in spapr_drc_attach. > > This patch makes use of the awaiting quiesce mechanism inside > spapr_drc_detach to fix this scenario. Inside spapr_drc_detach there is a > quiesce condition that relies on drc->state being equal to drck->empty_state. > If this doesn't happen, it is considered that the drc is not ready to be > detached. By extending this condition to include drc->dev being non-null > we cover this situation where the drc is still being attached and drc->dev > isn't set yet during the detach. Hrm. I thought the plug and unplug operations were serialized by the BQL. So, we need some more explanation of exactly how we get here. But, more importantly, this is broken. If we get here and attach hasn't completed yet, then we abort, *leaving the attach actions in place* meaning the device *isn't* unplugged. > > Fixes: https://bugs.launchpad.net/qemu/+bug/1718118 > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > --- > hw/ppc/spapr_drc.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index 915e9b51c4..6ad8190360 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -414,11 +414,9 @@ void spapr_drc_detach(sPAPRDRConnector *drc) > > trace_spapr_drc_detach(spapr_drc_index(drc)); > > - g_assert(drc->dev); > - > drc->unplug_requested = true; > > - if (drc->state != drck->empty_state) { > + if (!drc->dev || (drc->state != drck->empty_state)) { > trace_spapr_drc_awaiting_quiesce(spapr_drc_index(drc)); > return; > }
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index 915e9b51c4..6ad8190360 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -414,11 +414,9 @@ void spapr_drc_detach(sPAPRDRConnector *drc) trace_spapr_drc_detach(spapr_drc_index(drc)); - g_assert(drc->dev); - drc->unplug_requested = true; - if (drc->state != drck->empty_state) { + if (!drc->dev || (drc->state != drck->empty_state)) { trace_spapr_drc_awaiting_quiesce(spapr_drc_index(drc)); return; }
In cases where a device is hotplugged and hot-unplugged shortly after, there is a chance of QEMU breaking with the following message: hw/ppc/spapr_drc.c:417:spapr_drc_detach: assertion failed: (drc->dev) Aborted spapr_drc_detach makes a g_assert(drc->dev) to ensure that the following spapr_drc_release call is able to execute the appropriate callback using drc->dev as a parameter. However, in a scenario where a hotplug is quickly followed by a hot-unplug, this g_assert can be reached before the hotplug operation sets drc->dev in spapr_drc_attach. This patch makes use of the awaiting quiesce mechanism inside spapr_drc_detach to fix this scenario. Inside spapr_drc_detach there is a quiesce condition that relies on drc->state being equal to drck->empty_state. If this doesn't happen, it is considered that the drc is not ready to be detached. By extending this condition to include drc->dev being non-null we cover this situation where the drc is still being attached and drc->dev isn't set yet during the detach. Fixes: https://bugs.launchpad.net/qemu/+bug/1718118 Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> --- hw/ppc/spapr_drc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)