Re: [Qemu-devel] [PATCH 1/1] hw/ppc/spapr_drc.c: adding drc->dev into detach quiesce condition

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2470 bytes --]

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

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