[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 7/7] spapr_drc.c: use DRC reconfiguration to cleanup DIMM
From: |
David Gibson |
Subject: |
Re: [PATCH v3 7/7] spapr_drc.c: use DRC reconfiguration to cleanup DIMM unplug state |
Date: |
Mon, 22 Feb 2021 16:54:54 +1100 |
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
- [PATCH v3 6/7] spapr_drc.c: add hotunplug timeout for CPUs, (continued)
Re: [PATCH v3 0/7] CPU unplug timeout/LMB unplug cleanup in DRC reconfiguration, David Gibson, 2021/02/16