Re: [Qemu-devel] [PATCH 4/7] spapr: Don't misuse DR-indicator in spapr_recover_pending_dimm_state()

[Michael Roth]

Quoting David Gibson (2017-06-06 03:32:18)
> With some combinations of migration and hotplug we can lost temporary state
> indicating how many DRCs (guest side hotplug handles) are still connected
> to a DIMM object in the process of removal.  When we hit that situation
> spapr_recover_pending_dimm_state() is used to scan more extensively and
> work out the right number.
> 
> It does this using drc->indicator state to determine what state of
> disconnection the DRC is in.  However, this is not safe, because the
> indicator state is guest settable - in fact it's more-or-less a purely
> guest->host notification mechanism which should have no bearing on the
> internals of hotplug state management.
> 
> So, replace the test for this with a test on drc->dev, which is a purely
> qemu side managed variable, and updated the same BQL critical section as
> the indicator state.
> 
> This does introduce an off-by-one change, because the indicator state was
> updated before the call to spapr_lmb_release() on the current DRC, whereas
> drc->dev is updated afterwards.  That's corrected by always decrementing
> the nr_lmbs value instead of only doing so in the case where we didn't
> have to recover information.
> 
> Signed-off-by: David Gibson <address@hidden>

Reviewed-by: Michael Roth <address@hidden>

> ---
>  hw/ppc/spapr.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 671cdbb..e8cecaf 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2683,7 +2683,7 @@ static sPAPRDIMMState 
> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
>          drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
>                                addr / SPAPR_MEMORY_BLOCK_SIZE);
>          g_assert(drc);
> -        if (drc->indicator_state != SPAPR_DR_INDICATOR_STATE_INACTIVE) {
> +        if (drc->dev) {
>              avail_lmbs++;
>          }
>          addr += SPAPR_MEMORY_BLOCK_SIZE;
> @@ -2707,10 +2707,11 @@ void spapr_lmb_release(DeviceState *dev)
>       * during the unplug process. In this case recover it. */
>      if (ds == NULL) {
>          ds = spapr_recover_pending_dimm_state(spapr, PC_DIMM(dev));
> -        if (ds->nr_lmbs) {
> -            return;
> -        }
> -    } else if (--ds->nr_lmbs) {
> +        /* The DRC being examined by the caller at least must be counted */
> +        g_assert(ds->nr_lmbs);
> +    }
> +
> +    if (--ds->nr_lmbs) {
>          return;
>      }
> 
> -- 
> 2.9.4
>