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 <da...@gibson.dropbear.id.au>
Reviewed-by: Michael Roth <mdr...@linux.vnet.ibm.com> > --- > 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 >