On Tue, Mar 15, 2016 at 12:05:06PM +0100, Igor Mammedov wrote:
> On Tue, 15 Mar 2016 17:10:27 +1100
> David Gibson <da...@gibson.dropbear.id.au> wrote:
>
> > On Thu, Mar 10, 2016 at 11:39:46AM +0100, Igor Mammedov wrote:
> > > On Thu, 10 Mar 2016 11:32:44 +0530
> > > Bharata B Rao <bhar...@linux.vnet.ibm.com> wrote:
> > >
> > > > On Thu, Mar 10, 2016 at 04:22:43PM +1100, David Gibson wrote:
> > > > > 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 <bhar...@linux.vnet.ibm.com> wrote:
> > > > > >
> > > > > > > On Tue, Mar 08, 2016 at 02:18:14PM +0100, Igor Mammedov wrote:
> > > > > > > > Signed-off-by: Igor Mammedov <imamm...@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?
> > > realize shouldn't complete without error if object properties are
> > > wrong /for ex: i.e. you create kvm vcpu thread, configure it
> > > as already existing vcpu and have a lot fun afterwards/.
> (*1 ^^^)
>
> >
> > It seems to me there are two sorts of checks. (1) properties that are
> > wrong simply with reference to the CPU core itself (e.g. unsupported
> > CPU model, impossible number of threads). (2) properties that are
> > wrong only in the context of other CPUs or devices (e.g. core id
> > already populated, too many cores, impossible core id).
> >
> > Is it really a problem for realize() to complete if (1) is checked,
> > but not (2)?
> skipping 2 would do *1, (it's hard to tell what complications would
> be if CPU object with incorrect properties are created)
Hm, ok.
> > If it's so essential, I'm surprised we haven't hit this already. What
> > happens if you try to device_add two PCI devices in the same slot?
> > Where is that checked?
>
> PCI device has 2 'address' properties, 'addr' and 'bus'
> checking for valid address /including busy slot/
> happens as the first step in:
>
> pci_qdev_realize()->
> do_pci_register_device()
Ah...!
So the trick here is that the PCI device registers with its bus during
realize(). So now I'm wondering if we should be doing an equivalent
thing for CPUs: e.g. calling spapr_register_core() or something from
realize().
Or is there a fundamental difference between the cases which means
pre_plug() is a better choice here.
> > > For example: now on x86 we do duplicate CPU check wrong way
> > > by checking for duplicate of apic property from CPU code by
> > > looping through existing CPUs. Instead it would be much cleaner
> > > to move that check to machine which owns apic id assignment
> > > and make it check for duplicate in pre_plug() handler.
> > >
> > >
> > > > Since plug() handler is post-realize, rolling back involves
> > > > deleting the threads of the core we created and finally deleting the
> > > > core
> > > > itself.
> > > Even rolling back will leave some after effects, like created
> > > KVM VCPU thread which can't be deleted and who know what else.
> > >
> > > >We aleady do this kind of roll back when core hotplug is attemptedi
> > > > on machine type version that don't support hotplug.
> > > that's seems to be wrong, it shouldn't even come to cpu.realize()
> > > if hotplug is not supported.
> >
> > To be clear here, I'm not saying I think pre_plug() is a bad idea.
> > I'm just wondering if we can treat that change to the core hotplug
> > APIs as a clean up for later, rather than a prereq for CPU hotplug.
> I's too late for core hotplug being merged into 2.6
> (it's still RFC and QEMU is in soft-freeze).
> It would be better to fix series so that hotplug would be
> done in a clean way and be ready for merging by 2.7 dev cycle opens.
Yes, I know. But I'm worried that even in the 2.7 timeframe that
adding callbacks to the core hotplug model could cause long arguments
and delays.
--
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
