qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: David Gibson
Subject: Re: [Qemu-devel] [PATCH 1/1] hw/ppc/spapr_drc.c: adding drc->dev into detach quiesce condition
Date: Fri, 6 Oct 2017 19:54:39 +1100
User-agent: Mutt/1.9.0 (2017-09-02)

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 <address@hidden>
> ---
>  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: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]