Re: [Qemu-ppc] [PATCH v2 4/5] spapr: check if cpu core is already present

[Igor Mammedov]

On Tue, 8 Mar 2016 20:04:12 +0530
Bharata B Rao <address@hidden> wrote:

> On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > replaced link set check removed in previous patch
> > ---
> >  hw/ppc/spapr.c | 26 ++++++++++++++++++++++----
> >  1 file changed, 22 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 6890a44..db33c29 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -2297,6 +2297,27 @@ void *spapr_populate_hotplug_cpu_dt(DeviceState 
> > *dev, CPUState *cs,
> >      return fdt;
> >  }
> > 
> > +static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev,
> > +                                          DeviceState *dev, Error **errp)
> > +{
> > +    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(hotplug_dev);
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(hotplug_dev);
> > +
> > +    if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) {
> > +        int core = object_property_get_int(OBJECT(dev), CPU_CORE_ID_PROP,
> > +                                           &error_abort);
> > +
> > +        if (!smc->dr_cpu_enabled && dev->hotplugged) {
> > +            error_setg(errp, "CPU hotplug not supported for this machine");
> > +            return;
> > +        }
> > +        if (spapr->cores[core]) {
> > +            error_setg(errp, "core %d is already present", core);
> > +            return;
> > +        }  
> 
> Wondering why can't we do the above check from core's realizefn and fail
> the core hotplug from realizefn ?
that's rather simple, in ideal QOM world child shouldn't
poke into parents internal if it could be helped.
So hook provides responsibility separation where
board/or something else(HotplugHandler) can do a necessary
wiring of a component which is being hotplugged, without
forcing hotplugged device being aware about it.

That's what HotplugHandler->plug callback is doing for
post realize and HotplugHandler->pre_plug will do similar
thing but allowing board to execute preliminary tasks
(like check/set properties, amend its internal state)
before object is realized.

That will make realize() cleaner as it won't have to hack
into data it shouldn't and would prevent us calling unrealize()
if we were to check it later at HotplugHandler->plug time.
(i.e. realize() won't even have a chance to introduce side
effects that should be undone with unlealize())


> 
> Regards,
> Bharata.
>