| Message ID | 1457443095-213125-5-git-send-email-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ? Regards, Bharata.
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. 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. >
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?
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 <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? Since plug() handler is post-realize, rolling back involves deleting the threads of the core we created and finally deleting the core itself. We aleady do this kind of roll back when core hotplug is attemptedi on machine type version that don't support hotplug. For the present case of rejecting the hotplug for duplicate core_ids, are you in fact hinting that instead of failing the hotplug in pre_plug() lets realize then and then roll back from plug() ? Regards, Bharata.
On Thu, 10 Mar 2016 11:32:44 +0530 Bharata B Rao <bharata@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 <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? 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/. 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. > > For the present case of rejecting the hotplug for duplicate core_ids, > are you in fact hinting that instead of failing the hotplug in pre_plug() > lets realize then and then roll back from plug() ? > > Regards, > Bharata. >
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 <bharata@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 <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? > 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/. > > 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. Hmm that's how we dis-allowed memory hotplug on machine type versions that don't support memory hotplug, i,e., we failed hotplug from ->plug() handler. So ->pre_plug() seems to be the ideal place for such early failures ? Regards, Bharata.
On Thu, 10 Mar 2016 20:15:23 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> 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 <bharata@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 <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? > > 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/. > > > > 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. > > Hmm that's how we dis-allowed memory hotplug on machine type versions > that don't support memory hotplug, i,e., we failed hotplug from > ->plug() handler. So ->pre_plug() seems to be the ideal place for > such early failures ? It sure looks like it's. most of pc_dimm_memory_plug() should be done at pre_plug() except of following hunk which should stay a part of plug() pc_dimm_memory_plug(): ... memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr); vmstate_register_ram(mr, dev); numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node); ... Ok, I'll try to repost QMP series on top of the latest SPAPR today, taking in account libvirt feedback I've got this week. > > Regards, > Bharata. > >
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 <bharata@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 <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? > 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/. 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)? 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? > 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.
On Tue, 15 Mar 2016 17:10:27 +1100 David Gibson <david@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 <bharata@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 <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? > > 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) > 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() > > > 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.
On Tue, Mar 15, 2016 at 12:05:06PM +0100, Igor Mammedov wrote: > On Tue, 15 Mar 2016 17:10:27 +1100 > David Gibson <david@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 <bharata@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 <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? > > > 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.
On Wed, 16 Mar 2016 10:38:33 +1100 David Gibson <david@gibson.dropbear.id.au> wrote: > On Tue, Mar 15, 2016 at 12:05:06PM +0100, Igor Mammedov wrote: > > On Tue, 15 Mar 2016 17:10:27 +1100 > > David Gibson <david@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 <bharata@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 <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? > > > > 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(). When I introduced HotplugHandler, I've only did minor changes to bus based hotplug so it might use HotplugHandler transparently without rewriting QEMU. And since goal was to introduce missing hotplug infrastructure for bus-less devices not much of legacy bus hotplug had been touched. (patches as usual are welcome if someone has time for it) > Or is there a fundamental difference between the cases which means > pre_plug() is a better choice here. PCI device has access to its parent bus while CPUs are bus-less devices and don't have any supporting infrastructure to access its parent/owner. That's the reason why HotplugHandler has been introduced to allow hotplug of bus-less devices and still maintain clear role separation between device models. Apparent I've haven't noticed a need for pre_plug() hook back then. > > > > 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. So far no one has argued against of the idea so that should be fine, and as Bharata pointed out memory hotplug code could benefit from pre_plug() hook as well.
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; + } + } +} + static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { @@ -2338,10 +2359,6 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, spapr_memory_plug(hotplug_dev, dev, node, errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { - if (!smc->dr_cpu_enabled && dev->hotplugged) { - error_setg(errp, "CPU hotplug not supported for this machine"); - return; - } spapr_core_plug(hotplug_dev, dev, errp); } } @@ -2413,6 +2430,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) mc->has_dynamic_sysbus = true; mc->pci_allow_0_address = true; mc->get_hotplug_handler = spapr_get_hotpug_handler; + hc->pre_plug = spapr_machine_device_pre_plug; hc->plug = spapr_machine_device_plug; hc->unplug = spapr_machine_device_unplug; mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
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(-)