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

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

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

On Wed, Mar 09, 2016 at 11:07:40AM +0100, Igor Mammedov wrote:
> On Tue, 8 Mar 2016 20:04:12 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > > 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.

Oh.. yes.  Sorry, somehow I got confused and thought you were
suggesting a 'pre_realize()' method on the *object* rather than a
pre_plug hotplughandler hook.

> 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())

Hmm.. how big a deal is it to roll back from the existing plug()
handler?

-- 
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: 819 bytes --]