[Top][All Lists]

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

Re: [PATCH 2/5] spapr.c: check unplug_request flag in spapr_memory_unplu

From: David Gibson
Subject: Re: [PATCH 2/5] spapr.c: check unplug_request flag in spapr_memory_unplug_request()
Date: Tue, 2 Mar 2021 13:03:57 +1100

On Fri, Feb 26, 2021 at 01:32:58PM -0300, Daniel Henrique Barboza wrote:
> Now that we're asserting the first DRC LMB earlier, use it to query if
> the DRC is already pending unplug and, in this case, issue the same
> error we already do.
> The previous check was introduced in commit 2a129767ebb1 and it works,
> but it's easier to check the unplug_requested  flag instead of looking
> for the existence of the sPAPRDIMMState. It's also compliant with what
> is already done in other unplug_request functions for other devices.
> Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>

I'm having some trouble completely convincing myself this is right.

What about this situation:
     1. We initiate a DIMM unplug
        - unplug_request is set on all the LMBs
        - all the LMBs go on the pending_unplug list
     2. The guest encounters no problems, and starts issuing set
        indicator calls to mark the LMBs unusable, starting from the
        lowest address
     3. On drc_set_unusable() for the first LMB, we see that unplug is
        requested and call spapr_drc_release()
     4. spapr_drc_release() on the first LMB clears unplug_requested
     5. At this point, but before this is done on *all* of the DIMM's
        LMBs, the user attempts another unplug triggering the code

AFAICT this will now skip the error, since the first LMB is no longer
in unplug_requested state, but there *are* still pending unplugs for
some of the remaining LMBs, so the old code would have tripped the

> ---
>  hw/ppc/spapr.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 74e046b522..149dc2113f 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -3681,13 +3681,7 @@ static void spapr_memory_unplug_request(HotplugHandler 
> *hotplug_dev,
>                                  addr_start / SPAPR_MEMORY_BLOCK_SIZE);
>      g_assert(drc_start);
> -    /*
> -     * An existing pending dimm state for this DIMM means that there is an
> -     * unplug operation in progress, waiting for the spapr_lmb_release
> -     * callback to complete the job (BQL can't cover that far). In this case,
> -     * bail out to avoid detaching DRCs that were already released.
> -     */
> -    if (spapr_pending_dimm_unplugs_find(spapr, dimm)) {
> +    if (spapr_drc_unplug_requested(drc_start)) {
>          error_setg(errp, "Memory unplug already in progress for device %s",
>                     dev->id);
>          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_!

Attachment: signature.asc
Description: PGP signature

reply via email to

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