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

[David Gibson]

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

signature.asc
Description: PGP signature