| Message ID | 20180517081527.14410-7-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | MemoryDevice: use multi stage hotplug handlers | expand |
On Thu, 17 May 2018 10:15:19 +0200 David Hildenbrand <david@redhat.com> wrote: > For multi stage hotplug handlers, we'll have to do some error handling > in some hotplug functions, so let's use a local error variable (except > for unplug requests). > > Also, add code to pass control to the final stage hotplug handler at the > parent bus. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 43 insertions(+), 11 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index ebf30dd60b..b7c5c95f7a 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > { > MachineState *ms = MACHINE(hotplug_dev); > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); > + Error *local_err = NULL; > > + /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > int node; > > if (!smc->dr_lmb_enabled) { > - error_setg(errp, "Memory hotplug not supported for this machine"); > - return; > + error_setg(&local_err, > + "Memory hotplug not supported for this machine"); > + goto out; > } > - node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp); > - if (*errp) { Heh ! This is even a fix since errp could theoretically be NULL. Reviewed-by: Greg Kurz <groug@kaod.org> > - return; > + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, > + &local_err); > + if (local_err) { > + goto out; > } > if (node < 0 || node >= MAX_NODES) { > - error_setg(errp, "Invaild node %d", node); > - return; > + error_setg(&local_err, "Invaild node %d", node); > + goto out; > } > > - spapr_memory_plug(hotplug_dev, dev, node, errp); > + spapr_memory_plug(hotplug_dev, dev, node, &local_err); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > - spapr_core_plug(hotplug_dev, dev, errp); > + spapr_core_plug(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); > + } > +out: > + error_propagate(errp, local_err); > +} > + > +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > + DeviceState *dev, Error **errp) > +{ > + Error *local_err = NULL; > + > + /* final stage hotplug handler */ > + if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, > + &local_err); > } > + error_propagate(errp, local_err); > } > > static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, > @@ -3618,17 +3639,27 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, > return; > } > spapr_core_unplug_request(hotplug_dev, dev, errp); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev, > + errp); > } > } > > static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > + Error *local_err = NULL; > + > + /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > - spapr_memory_pre_plug(hotplug_dev, dev, errp); > + spapr_memory_pre_plug(hotplug_dev, dev, &local_err); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > - spapr_core_pre_plug(hotplug_dev, dev, errp); > + spapr_core_pre_plug(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev, > + &local_err); > } > + error_propagate(errp, local_err); > } > > static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine, > @@ -3988,6 +4019,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id; > mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids; > hc->unplug_request = spapr_machine_device_unplug_request; > + hc->unplug = spapr_machine_device_unplug; > > smc->dr_lmb_enabled = true; > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
On Thu, 17 May 2018 10:15:19 +0200 David Hildenbrand <david@redhat.com> wrote: maybe subj: make hotplug handlers use local_error > For multi stage hotplug handlers, we'll have to do some error handling > in some hotplug functions, so let's use a local error variable (except > for unplug requests). > > Also, add code to pass control to the final stage hotplug handler at the > parent bus. doing several not related things in one patch doesn't help reviewing it. Also as explained 04/14 it's not needed at all. Could you try to keep patches minimal, we can add more complexity in later revisions if it really necessary. > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 43 insertions(+), 11 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index ebf30dd60b..b7c5c95f7a 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > { > MachineState *ms = MACHINE(hotplug_dev); > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); > + Error *local_err = NULL; > > + /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > int node; > > if (!smc->dr_lmb_enabled) { > - error_setg(errp, "Memory hotplug not supported for this machine"); > - return; > + error_setg(&local_err, > + "Memory hotplug not supported for this machine"); > + goto out; > } > - node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp); > - if (*errp) { > - return; > + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, > + &local_err); > + if (local_err) { > + goto out; > } > if (node < 0 || node >= MAX_NODES) { > - error_setg(errp, "Invaild node %d", node); > - return; > + error_setg(&local_err, "Invaild node %d", node); > + goto out; > } > > - spapr_memory_plug(hotplug_dev, dev, node, errp); > + spapr_memory_plug(hotplug_dev, dev, node, &local_err); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > - spapr_core_plug(hotplug_dev, dev, errp); > + spapr_core_plug(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); > + } > +out: > + error_propagate(errp, local_err); > +} > + > +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > + DeviceState *dev, Error **errp) > +{ > + Error *local_err = NULL; > + > + /* final stage hotplug handler */ > + if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, > + &local_err); > } > + error_propagate(errp, local_err); > } > > static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, > @@ -3618,17 +3639,27 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, > return; > } > spapr_core_unplug_request(hotplug_dev, dev, errp); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev, > + errp); > } > } > > static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > + Error *local_err = NULL; > + > + /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > - spapr_memory_pre_plug(hotplug_dev, dev, errp); > + spapr_memory_pre_plug(hotplug_dev, dev, &local_err); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > - spapr_core_pre_plug(hotplug_dev, dev, errp); > + spapr_core_pre_plug(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev, > + &local_err); > } > + error_propagate(errp, local_err); > } > > static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine, > @@ -3988,6 +4019,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id; > mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids; > hc->unplug_request = spapr_machine_device_unplug_request; > + hc->unplug = spapr_machine_device_unplug; > > smc->dr_lmb_enabled = true; > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
On Thu, May 17, 2018 at 10:15:19AM +0200, David Hildenbrand wrote: > For multi stage hotplug handlers, we'll have to do some error handling > in some hotplug functions, so let's use a local error variable (except > for unplug requests). > > Also, add code to pass control to the final stage hotplug handler at the > parent bus. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 43 insertions(+), 11 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index ebf30dd60b..b7c5c95f7a 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > { > MachineState *ms = MACHINE(hotplug_dev); > sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); > + Error *local_err = NULL; > > + /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > int node; > > if (!smc->dr_lmb_enabled) { > - error_setg(errp, "Memory hotplug not supported for this machine"); > - return; > + error_setg(&local_err, > + "Memory hotplug not supported for this machine"); > + goto out; > } > - node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp); > - if (*errp) { > - return; > + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, > + &local_err); > + if (local_err) { > + goto out; > } > if (node < 0 || node >= MAX_NODES) { > - error_setg(errp, "Invaild node %d", node); > - return; > + error_setg(&local_err, "Invaild node %d", node); > + goto out; > } > > - spapr_memory_plug(hotplug_dev, dev, node, errp); > + spapr_memory_plug(hotplug_dev, dev, node, &local_err); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > - spapr_core_plug(hotplug_dev, dev, errp); > + spapr_core_plug(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); > + } > +out: > + error_propagate(errp, local_err); > +} > + > +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > + DeviceState *dev, Error **errp) > +{ > + Error *local_err = NULL; > + > + /* final stage hotplug handler */ > + if (dev->parent_bus && dev->parent_bus->hotplug_handler) { As I think Igor said on the equivalent PC patch, I don't quite get this. Isn't this already handled by the generic hotplug code picking up the bus's hotplug handler if the machine doesn't supply one? > + hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, > + &local_err); > } > + error_propagate(errp, local_err); > } > > static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, > @@ -3618,17 +3639,27 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, > return; > } > spapr_core_unplug_request(hotplug_dev, dev, errp); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev, > + errp); > } > } > > static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > + Error *local_err = NULL; > + > + /* final stage hotplug handler */ > if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > - spapr_memory_pre_plug(hotplug_dev, dev, errp); > + spapr_memory_pre_plug(hotplug_dev, dev, &local_err); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > - spapr_core_pre_plug(hotplug_dev, dev, errp); > + spapr_core_pre_plug(hotplug_dev, dev, &local_err); > + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > + hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev, > + &local_err); > } > + error_propagate(errp, local_err); > } > > static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine, > @@ -3988,6 +4019,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) > mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id; > mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids; > hc->unplug_request = spapr_machine_device_unplug_request; > + hc->unplug = spapr_machine_device_unplug; > > smc->dr_lmb_enabled = true; > mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
On 05.06.2018 03:08, David Gibson wrote: > On Thu, May 17, 2018 at 10:15:19AM +0200, David Hildenbrand wrote: >> For multi stage hotplug handlers, we'll have to do some error handling >> in some hotplug functions, so let's use a local error variable (except >> for unplug requests). >> >> Also, add code to pass control to the final stage hotplug handler at the >> parent bus. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++----------- >> 1 file changed, 43 insertions(+), 11 deletions(-) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index ebf30dd60b..b7c5c95f7a 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, >> { >> MachineState *ms = MACHINE(hotplug_dev); >> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); >> + Error *local_err = NULL; >> >> + /* final stage hotplug handler */ >> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { >> int node; >> >> if (!smc->dr_lmb_enabled) { >> - error_setg(errp, "Memory hotplug not supported for this machine"); >> - return; >> + error_setg(&local_err, >> + "Memory hotplug not supported for this machine"); >> + goto out; >> } >> - node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp); >> - if (*errp) { >> - return; >> + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, >> + &local_err); >> + if (local_err) { >> + goto out; >> } >> if (node < 0 || node >= MAX_NODES) { >> - error_setg(errp, "Invaild node %d", node); >> - return; >> + error_setg(&local_err, "Invaild node %d", node); >> + goto out; >> } >> >> - spapr_memory_plug(hotplug_dev, dev, node, errp); >> + spapr_memory_plug(hotplug_dev, dev, node, &local_err); >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { >> - spapr_core_plug(hotplug_dev, dev, errp); >> + spapr_core_plug(hotplug_dev, dev, &local_err); >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { >> + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); >> + } >> +out: >> + error_propagate(errp, local_err); >> +} >> + >> +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, >> + DeviceState *dev, Error **errp) >> +{ >> + Error *local_err = NULL; >> + >> + /* final stage hotplug handler */ >> + if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > > As I think Igor said on the equivalent PC patch, I don't quite get > this. Isn't this already handled by the generic hotplug code picking > up the bus's hotplug handler if the machine doesn't supply one? See my reply to patch nr 4. What we do is, we install the machine hotplug handler as an "intermediate" hotplug handler. E.g. if we have a VIRTIO based MemoryDevice, we have to initialize the MemoryDevice specific stuff in the machine hotplug handler, but then pass the device onto the last stage hotplug handler (which will eventually attach it to a bus and notify the guest). For PC_DIMM, the machine hotplug handler is already the last stage hotplug handler. Everything is fine. If it is not a PC_DIMM, we have to pass it on to the right last stage hotplug handler (e.g. using the bus). So the generic hotplug code will alway pick up the machine hotplug handler for MemoryDevices first, do the MemoryDevice specific stuff and then pass on control to the "actual" hotplug handler. Please note that this handling only applies to device types we "route" through the machine hotplug handler, what we will initially only do for MemoryDevices. Just like for PC, I factored out the introduction of the local error variable and will add a better description to the patch descriptions on how this plays together.
On Tue, 5 Jun 2018 09:51:26 +0200 David Hildenbrand <david@redhat.com> wrote: > On 05.06.2018 03:08, David Gibson wrote: > > On Thu, May 17, 2018 at 10:15:19AM +0200, David Hildenbrand wrote: > >> For multi stage hotplug handlers, we'll have to do some error handling > >> in some hotplug functions, so let's use a local error variable (except > >> for unplug requests). > >> > >> Also, add code to pass control to the final stage hotplug handler at the > >> parent bus. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++----------- > >> 1 file changed, 43 insertions(+), 11 deletions(-) > >> > >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >> index ebf30dd60b..b7c5c95f7a 100644 > >> --- a/hw/ppc/spapr.c > >> +++ b/hw/ppc/spapr.c > >> @@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > >> { > >> MachineState *ms = MACHINE(hotplug_dev); > >> sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); > >> + Error *local_err = NULL; > >> > >> + /* final stage hotplug handler */ > >> if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > >> int node; > >> > >> if (!smc->dr_lmb_enabled) { > >> - error_setg(errp, "Memory hotplug not supported for this machine"); > >> - return; > >> + error_setg(&local_err, > >> + "Memory hotplug not supported for this machine"); > >> + goto out; > >> } > >> - node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp); > >> - if (*errp) { > >> - return; > >> + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, > >> + &local_err); > >> + if (local_err) { > >> + goto out; > >> } > >> if (node < 0 || node >= MAX_NODES) { > >> - error_setg(errp, "Invaild node %d", node); > >> - return; > >> + error_setg(&local_err, "Invaild node %d", node); > >> + goto out; > >> } > >> > >> - spapr_memory_plug(hotplug_dev, dev, node, errp); > >> + spapr_memory_plug(hotplug_dev, dev, node, &local_err); > >> } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { > >> - spapr_core_plug(hotplug_dev, dev, errp); > >> + spapr_core_plug(hotplug_dev, dev, &local_err); > >> + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > >> + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); > >> + } > >> +out: > >> + error_propagate(errp, local_err); > >> +} > >> + > >> +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, > >> + DeviceState *dev, Error **errp) > >> +{ > >> + Error *local_err = NULL; > >> + > >> + /* final stage hotplug handler */ > >> + if (dev->parent_bus && dev->parent_bus->hotplug_handler) { > > > > As I think Igor said on the equivalent PC patch, I don't quite get > > this. Isn't this already handled by the generic hotplug code picking > > up the bus's hotplug handler if the machine doesn't supply one? > > See my reply to patch nr 4. > > What we do is, we install the machine hotplug handler as an > "intermediate" hotplug handler. > > E.g. if we have a VIRTIO based MemoryDevice, we have to initialize the > MemoryDevice specific stuff in the machine hotplug handler, but then > pass the device onto the last stage hotplug handler (which will > eventually attach it to a bus and notify the guest). as said in v4, pls don't do this implicit routing as it's hard to read and maintain. Do explicit routing within concrete device helper (virtio_mem_[un|pre]plug()) keeping un/pre/plug handlers simple. And then you won't need if check as well, just call to bus handler directly. [...]
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index ebf30dd60b..b7c5c95f7a 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -3571,27 +3571,48 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, { MachineState *ms = MACHINE(hotplug_dev); sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms); + Error *local_err = NULL; + /* final stage hotplug handler */ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { int node; if (!smc->dr_lmb_enabled) { - error_setg(errp, "Memory hotplug not supported for this machine"); - return; + error_setg(&local_err, + "Memory hotplug not supported for this machine"); + goto out; } - node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, errp); - if (*errp) { - return; + node = object_property_get_uint(OBJECT(dev), PC_DIMM_NODE_PROP, + &local_err); + if (local_err) { + goto out; } if (node < 0 || node >= MAX_NODES) { - error_setg(errp, "Invaild node %d", node); - return; + error_setg(&local_err, "Invaild node %d", node); + goto out; } - spapr_memory_plug(hotplug_dev, dev, node, errp); + spapr_memory_plug(hotplug_dev, dev, node, &local_err); } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { - spapr_core_plug(hotplug_dev, dev, errp); + spapr_core_plug(hotplug_dev, dev, &local_err); + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { + hotplug_handler_plug(dev->parent_bus->hotplug_handler, dev, &local_err); + } +out: + error_propagate(errp, local_err); +} + +static void spapr_machine_device_unplug(HotplugHandler *hotplug_dev, + DeviceState *dev, Error **errp) +{ + Error *local_err = NULL; + + /* final stage hotplug handler */ + if (dev->parent_bus && dev->parent_bus->hotplug_handler) { + hotplug_handler_unplug(dev->parent_bus->hotplug_handler, dev, + &local_err); } + error_propagate(errp, local_err); } static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, @@ -3618,17 +3639,27 @@ static void spapr_machine_device_unplug_request(HotplugHandler *hotplug_dev, return; } spapr_core_unplug_request(hotplug_dev, dev, errp); + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { + hotplug_handler_unplug_request(dev->parent_bus->hotplug_handler, dev, + errp); } } static void spapr_machine_device_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { + Error *local_err = NULL; + + /* final stage hotplug handler */ if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { - spapr_memory_pre_plug(hotplug_dev, dev, errp); + spapr_memory_pre_plug(hotplug_dev, dev, &local_err); } else if (object_dynamic_cast(OBJECT(dev), TYPE_SPAPR_CPU_CORE)) { - spapr_core_pre_plug(hotplug_dev, dev, errp); + spapr_core_pre_plug(hotplug_dev, dev, &local_err); + } else if (dev->parent_bus && dev->parent_bus->hotplug_handler) { + hotplug_handler_pre_plug(dev->parent_bus->hotplug_handler, dev, + &local_err); } + error_propagate(errp, local_err); } static HotplugHandler *spapr_get_hotplug_handler(MachineState *machine, @@ -3988,6 +4019,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data) mc->get_default_cpu_node_id = spapr_get_default_cpu_node_id; mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids; hc->unplug_request = spapr_machine_device_unplug_request; + hc->unplug = spapr_machine_device_unplug; smc->dr_lmb_enabled = true; mc->default_cpu_type = POWERPC_CPU_TYPE_NAME("power8_v2.0");
For multi stage hotplug handlers, we'll have to do some error handling in some hotplug functions, so let's use a local error variable (except for unplug requests). Also, add code to pass control to the final stage hotplug handler at the parent bus. Signed-off-by: David Hildenbrand <david@redhat.com> --- hw/ppc/spapr.c | 54 +++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 43 insertions(+), 11 deletions(-)