Re: [PATCH v3 7/7] spapr_drc.c: use DRC reconfiguration to cleanup DIMM unplug state

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

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

On Fri, Feb 19, 2021 at 06:31:46PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 2/16/21 11:31 PM, David Gibson wrote:
> > On Thu, Feb 11, 2021 at 07:52:46PM -0300, Daniel Henrique Barboza wrote:
> > > Handling errors in memory hotunplug in the pSeries machine is more complex
> > > than any other device type, because there are all the complications that other
> > > devices has, and more.
> 
> [...]
> 
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index ecce8abf14..4bcded4a1a 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -3575,6 +3575,36 @@ static SpaprDimmState *spapr_recover_pending_dimm_state(SpaprMachineState *ms,
> > >       return spapr_pending_dimm_unplugs_add(ms, avail_lmbs, dimm);
> > >   }
> > > +void spapr_clear_pending_dimm_unplug_state(SpaprMachineState *spapr,
> > > +                                           PCDIMMDevice *dimm)
> > > +{
> > > +    SpaprDimmState *ds = spapr_pending_dimm_unplugs_find(spapr, dimm);
> > > +    SpaprDrc *drc;
> > > +    uint32_t nr_lmbs;
> > > +    uint64_t size, addr_start, addr;
> > > +    int i;
> > > +
> > > +    if (ds) {
> > > +        spapr_pending_dimm_unplugs_remove(spapr, ds);
> > > +    }
> > 
> > Hrm... how would !ds arise?  Could this just be an assert?
> 
> !ds would appear if we do not assert g_assert(drc->dev) down there, where you
> suggested down below that a malicious/buggy code would trigger it, for example.
> With that assert in place then this less likely to occcur.
> 
> I guess what I can do here is:
> 
> - remove the g_assert(drc->dev) from down below, since it's more related to the
> logic of this function;
> 
> - here, check if drc->dev is NULL. Return doing nothing if that's the case (all the
> function relies on drc->dev being valid);
> 
> - if drc->dev is not NULL, then we can g_assert(ds) and proceed with the rest of
> the function
> 
> This way we become a little more tolerant on drc->dev being NULL, but if drc->dev
> is valid we will expect a unplug dimm state to always exist and
> assert it.

Seems reasonable.

-- 
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 --]