| Message ID | 20170430172547.13415-2-danielhb@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 30/04/2017 19:25, Daniel Henrique Barboza wrote: > The idea of moving the detach callback functions to the constructor > of the dr_connector is to set them statically at init time, avoiding > any post-load hooks to restore it (after a migration, for example). > > Summary of changes: > > - hw/ppc/spapr_drc.c and include/hw/ppc/spapr_drc.h: > * spapr_dr_connector_new() now has an additional parameter, > spapr_drc_detach_cb *detach_cb > * 'spapr_drc_detach_cb *detach_cb' parameter was removed of > the detach function pointer in sPAPRDRConnectorClass > > - hw/ppc/spapr_pci.c: > * the callback 'spapr_phb_remove_pci_device_cb' is now passed > as a parameter in 'spapr_dr_connector_new' instead of 'drck->detach()' > > - hw/ppc/spapr.c: > * 'spapr_create_lmb_dr_connectors' now passes the callback > 'spapr_lmb_release' to 'spapr_dr_connector_new' instead of 'drck-detach()' > * 'spapr_init_cpus' now passes the callback 'spapr_core_release' > to 'spapr_dr_connector_new' instead of 'drck-detach()' > * moved the callback functions up in the code so they can be referenced > by 'spapr_create_lmb_dr_connectors' and 'spapr_init_cpus' > > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > --- > hw/ppc/spapr.c | 71 +++++++++++++++++++++++----------------------- > hw/ppc/spapr_drc.c | 17 ++++++----- > hw/ppc/spapr_pci.c | 5 ++-- > include/hw/ppc/spapr_drc.h | 4 +-- > 4 files changed, 49 insertions(+), 48 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 80d12d0..bc11757 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1887,6 +1887,29 @@ static void spapr_drc_reset(void *opaque) > } > } > > +typedef struct sPAPRDIMMState { > + uint32_t nr_lmbs; > +} sPAPRDIMMState; > + > +static void spapr_lmb_release(DeviceState *dev, void *opaque) > +{ > + sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque; > + HotplugHandler *hotplug_ctrl; > + > + if (--ds->nr_lmbs) { > + return; > + } > + > + g_free(ds); > + > + /* > + * Now that all the LMBs have been removed by the guest, call the > + * pc-dimm unplug handler to cleanup up the pc-dimm device. > + */ > + hotplug_ctrl = qdev_get_hotplug_handler(dev); > + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); > +} > + > static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) > { > MachineState *machine = MACHINE(spapr); > @@ -1900,7 +1923,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) > > addr = i * lmb_size + spapr->hotplug_memory.base; > drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB, > - addr/lmb_size); > + (addr / lmb_size), spapr_lmb_release); You have added useless parenthesis around "addr / lmb_size". Reviewed-by: Laurent Vivier <lvivier@redhat.com>
On Sun, Apr 30, 2017 at 02:25:43PM -0300, Daniel Henrique Barboza wrote: > The idea of moving the detach callback functions to the constructor > of the dr_connector is to set them statically at init time, avoiding > any post-load hooks to restore it (after a migration, for example). > > Summary of changes: > > - hw/ppc/spapr_drc.c and include/hw/ppc/spapr_drc.h: > * spapr_dr_connector_new() now has an additional parameter, > spapr_drc_detach_cb *detach_cb > * 'spapr_drc_detach_cb *detach_cb' parameter was removed of > the detach function pointer in sPAPRDRConnectorClass > > - hw/ppc/spapr_pci.c: > * the callback 'spapr_phb_remove_pci_device_cb' is now passed > as a parameter in 'spapr_dr_connector_new' instead of 'drck->detach()' > > - hw/ppc/spapr.c: > * 'spapr_create_lmb_dr_connectors' now passes the callback > 'spapr_lmb_release' to 'spapr_dr_connector_new' instead of 'drck-detach()' > * 'spapr_init_cpus' now passes the callback 'spapr_core_release' > to 'spapr_dr_connector_new' instead of 'drck-detach()' > * moved the callback functions up in the code so they can be referenced > by 'spapr_create_lmb_dr_connectors' and 'spapr_init_cpus' > > Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> So, the patch looks correct, but the approach bothers me a bit. The DRC type and the detach callback are essentially redundant information - the callback still gets stored in the instance, which doesn't make a whole lot of sense. Ideally we'd use QOM subtypes of the base DRC type and put the callback in the drck, but I suspect that will raise some other complications, so I'm ok with postponing that. In the meantime, I think we'd be better of doing an explicit switch on the DRC type when we want to call the detach function, rather than storing a function pointer at all. It's kind of ugly, but we already have a bunch of nasty switches on the type, so I don't think it's really any uglier than what we have. > --- > hw/ppc/spapr.c | 71 +++++++++++++++++++++++----------------------- > hw/ppc/spapr_drc.c | 17 ++++++----- > hw/ppc/spapr_pci.c | 5 ++-- > include/hw/ppc/spapr_drc.h | 4 +-- > 4 files changed, 49 insertions(+), 48 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 80d12d0..bc11757 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1887,6 +1887,29 @@ static void spapr_drc_reset(void *opaque) > } > } > > +typedef struct sPAPRDIMMState { > + uint32_t nr_lmbs; > +} sPAPRDIMMState; > + > +static void spapr_lmb_release(DeviceState *dev, void *opaque) > +{ > + sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque; > + HotplugHandler *hotplug_ctrl; > + > + if (--ds->nr_lmbs) { > + return; > + } > + > + g_free(ds); > + > + /* > + * Now that all the LMBs have been removed by the guest, call the > + * pc-dimm unplug handler to cleanup up the pc-dimm device. > + */ > + hotplug_ctrl = qdev_get_hotplug_handler(dev); > + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); > +} > + > static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) > { > MachineState *machine = MACHINE(spapr); > @@ -1900,7 +1923,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) > > addr = i * lmb_size + spapr->hotplug_memory.base; > drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB, > - addr/lmb_size); > + (addr / lmb_size), spapr_lmb_release); > qemu_register_reset(spapr_drc_reset, drc); > } > } > @@ -1956,6 +1979,14 @@ static CPUArchId *spapr_find_cpu_slot(MachineState *ms, uint32_t id, int *idx) > return &ms->possible_cpus->cpus[index]; > } > > +static void spapr_core_release(DeviceState *dev, void *opaque) > +{ > + HotplugHandler *hotplug_ctrl; > + > + hotplug_ctrl = qdev_get_hotplug_handler(dev); > + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); > +} > + > static void spapr_init_cpus(sPAPRMachineState *spapr) > { > MachineState *machine = MACHINE(spapr); > @@ -1998,7 +2029,8 @@ static void spapr_init_cpus(sPAPRMachineState *spapr) > sPAPRDRConnector *drc = > spapr_dr_connector_new(OBJECT(spapr), > SPAPR_DR_CONNECTOR_TYPE_CPU, > - (core_id / smp_threads) * smt); > + (core_id / smp_threads) * smt, > + spapr_core_release); > > qemu_register_reset(spapr_drc_reset, drc); > } > @@ -2596,29 +2628,6 @@ out: > error_propagate(errp, local_err); > } > > -typedef struct sPAPRDIMMState { > - uint32_t nr_lmbs; > -} sPAPRDIMMState; > - > -static void spapr_lmb_release(DeviceState *dev, void *opaque) > -{ > - sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque; > - HotplugHandler *hotplug_ctrl; > - > - if (--ds->nr_lmbs) { > - return; > - } > - > - g_free(ds); > - > - /* > - * Now that all the LMBs have been removed by the guest, call the > - * pc-dimm unplug handler to cleanup up the pc-dimm device. > - */ > - hotplug_ctrl = qdev_get_hotplug_handler(dev); > - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); > -} > - > static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > Error **errp) > { > @@ -2636,7 +2645,7 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, > g_assert(drc); > > drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > - drck->detach(drc, dev, spapr_lmb_release, ds, errp); > + drck->detach(drc, dev, ds, errp); > addr += SPAPR_MEMORY_BLOCK_SIZE; > } > > @@ -2712,14 +2721,6 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > object_unparent(OBJECT(dev)); > } > > -static void spapr_core_release(DeviceState *dev, void *opaque) > -{ > - HotplugHandler *hotplug_ctrl; > - > - hotplug_ctrl = qdev_get_hotplug_handler(dev); > - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); > -} > - > static > void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > @@ -2745,7 +2746,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, > g_assert(drc); > > drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > - drck->detach(drc, dev, spapr_core_release, NULL, &local_err); > + drck->detach(drc, dev, NULL, &local_err); > if (local_err) { > error_propagate(errp, local_err); > return; > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c > index a1cdc87..afe5d82 100644 > --- a/hw/ppc/spapr_drc.c > +++ b/hw/ppc/spapr_drc.c > @@ -99,8 +99,8 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc, > if (drc->awaiting_release) { > if (drc->configured) { > trace_spapr_drc_set_isolation_state_finalizing(get_index(drc)); > - drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, > - drc->detach_cb_opaque, NULL); > + drck->detach(drc, DEVICE(drc->dev), drc->detach_cb_opaque, > + NULL); > } else { > trace_spapr_drc_set_isolation_state_deferring(get_index(drc)); > } > @@ -153,8 +153,8 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc, > if (drc->awaiting_release && > drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) { > trace_spapr_drc_set_allocation_state_finalizing(get_index(drc)); > - drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, > - drc->detach_cb_opaque, NULL); > + drck->detach(drc, DEVICE(drc->dev), drc->detach_cb_opaque, > + NULL); > } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) { > drc->awaiting_allocation = false; > } > @@ -405,12 +405,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt, > } > > static void detach(sPAPRDRConnector *drc, DeviceState *d, > - spapr_drc_detach_cb *detach_cb, > void *detach_cb_opaque, Error **errp) > { > trace_spapr_drc_detach(get_index(drc)); > > - drc->detach_cb = detach_cb; > drc->detach_cb_opaque = detach_cb_opaque; > > /* if we've signalled device presence to the guest, or if the guest > @@ -498,8 +496,7 @@ static void reset(DeviceState *d) > * force removal if we are > */ > if (drc->awaiting_release) { > - drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, > - drc->detach_cb_opaque, NULL); > + drck->detach(drc, DEVICE(drc->dev), drc->detach_cb_opaque, NULL); > } > > /* non-PCI devices may be awaiting a transition to UNUSABLE */ > @@ -566,7 +563,8 @@ static void unrealize(DeviceState *d, Error **errp) > > sPAPRDRConnector *spapr_dr_connector_new(Object *owner, > sPAPRDRConnectorType type, > - uint32_t id) > + uint32_t id, > + spapr_drc_detach_cb *detach_cb) > { > sPAPRDRConnector *drc = > SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR)); > @@ -577,6 +575,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner, > drc->type = type; > drc->id = id; > drc->owner = owner; > + drc->detach_cb = detach_cb; > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", get_index(drc)); > object_property_add_child(owner, prop_name, OBJECT(drc), NULL); > object_property_set_bool(OBJECT(drc), true, "realized", NULL); > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > index e7567e2..935e65e 100644 > --- a/hw/ppc/spapr_pci.c > +++ b/hw/ppc/spapr_pci.c > @@ -1392,7 +1392,7 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc, > { > sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); > > - drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp); > + drck->detach(drc, DEVICE(pdev), phb, errp); > } > > static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb, > @@ -1764,7 +1764,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) > for (i = 0; i < PCI_SLOT_MAX * 8; i++) { > spapr_dr_connector_new(OBJECT(phb), > SPAPR_DR_CONNECTOR_TYPE_PCI, > - (sphb->index << 16) | i); > + (sphb->index << 16) | i, > + spapr_phb_remove_pci_device_cb); > } > } > > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h > index 5524247..0a2c173 100644 > --- a/include/hw/ppc/spapr_drc.h > +++ b/include/hw/ppc/spapr_drc.h > @@ -189,7 +189,6 @@ typedef struct sPAPRDRConnectorClass { > void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt, > int fdt_start_offset, bool coldplug, Error **errp); > void (*detach)(sPAPRDRConnector *drc, DeviceState *d, > - spapr_drc_detach_cb *detach_cb, > void *detach_cb_opaque, Error **errp); > bool (*release_pending)(sPAPRDRConnector *drc); > void (*set_signalled)(sPAPRDRConnector *drc); > @@ -197,7 +196,8 @@ typedef struct sPAPRDRConnectorClass { > > sPAPRDRConnector *spapr_dr_connector_new(Object *owner, > sPAPRDRConnectorType type, > - uint32_t id); > + uint32_t id, > + spapr_drc_detach_cb *detach_cb); > sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index); > sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type, > uint32_t id);
On 05/04/2017 02:33 AM, David Gibson wrote: > On Sun, Apr 30, 2017 at 02:25:43PM -0300, Daniel Henrique Barboza wrote: >> The idea of moving the detach callback functions to the constructor >> of the dr_connector is to set them statically at init time, avoiding >> any post-load hooks to restore it (after a migration, for example). >> >> Summary of changes: >> >> - hw/ppc/spapr_drc.c and include/hw/ppc/spapr_drc.h: >> * spapr_dr_connector_new() now has an additional parameter, >> spapr_drc_detach_cb *detach_cb >> * 'spapr_drc_detach_cb *detach_cb' parameter was removed of >> the detach function pointer in sPAPRDRConnectorClass >> >> - hw/ppc/spapr_pci.c: >> * the callback 'spapr_phb_remove_pci_device_cb' is now passed >> as a parameter in 'spapr_dr_connector_new' instead of 'drck->detach()' >> >> - hw/ppc/spapr.c: >> * 'spapr_create_lmb_dr_connectors' now passes the callback >> 'spapr_lmb_release' to 'spapr_dr_connector_new' instead of 'drck-detach()' >> * 'spapr_init_cpus' now passes the callback 'spapr_core_release' >> to 'spapr_dr_connector_new' instead of 'drck-detach()' >> * moved the callback functions up in the code so they can be referenced >> by 'spapr_create_lmb_dr_connectors' and 'spapr_init_cpus' >> >> Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > So, the patch looks correct, but the approach bothers me a bit. The > DRC type and the detach callback are essentially redundant information > - the callback still gets stored in the instance, which doesn't make a > whole lot of sense. > > Ideally we'd use QOM subtypes of the base DRC type and put the > callback in the drck, but I suspect that will raise some other > complications, so I'm ok with postponing that. Interesting. > > In the meantime, I think we'd be better of doing an explicit switch on > the DRC type when we want to call the detach function, rather than > storing a function pointer at all. It's kind of ugly, but we already > have a bunch of nasty switches on the type, so I don't think it's > really any uglier than what we have. That sounds reasonable to me. If no one has a strong objection against it I'll add this change in the next version. Daniel > > >> --- >> hw/ppc/spapr.c | 71 +++++++++++++++++++++++----------------------- >> hw/ppc/spapr_drc.c | 17 ++++++----- >> hw/ppc/spapr_pci.c | 5 ++-- >> include/hw/ppc/spapr_drc.h | 4 +-- >> 4 files changed, 49 insertions(+), 48 deletions(-) >> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >> index 80d12d0..bc11757 100644 >> --- a/hw/ppc/spapr.c >> +++ b/hw/ppc/spapr.c >> @@ -1887,6 +1887,29 @@ static void spapr_drc_reset(void *opaque) >> } >> } >> >> +typedef struct sPAPRDIMMState { >> + uint32_t nr_lmbs; >> +} sPAPRDIMMState; >> + >> +static void spapr_lmb_release(DeviceState *dev, void *opaque) >> +{ >> + sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque; >> + HotplugHandler *hotplug_ctrl; >> + >> + if (--ds->nr_lmbs) { >> + return; >> + } >> + >> + g_free(ds); >> + >> + /* >> + * Now that all the LMBs have been removed by the guest, call the >> + * pc-dimm unplug handler to cleanup up the pc-dimm device. >> + */ >> + hotplug_ctrl = qdev_get_hotplug_handler(dev); >> + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); >> +} >> + >> static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) >> { >> MachineState *machine = MACHINE(spapr); >> @@ -1900,7 +1923,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) >> >> addr = i * lmb_size + spapr->hotplug_memory.base; >> drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB, >> - addr/lmb_size); >> + (addr / lmb_size), spapr_lmb_release); >> qemu_register_reset(spapr_drc_reset, drc); >> } >> } >> @@ -1956,6 +1979,14 @@ static CPUArchId *spapr_find_cpu_slot(MachineState *ms, uint32_t id, int *idx) >> return &ms->possible_cpus->cpus[index]; >> } >> >> +static void spapr_core_release(DeviceState *dev, void *opaque) >> +{ >> + HotplugHandler *hotplug_ctrl; >> + >> + hotplug_ctrl = qdev_get_hotplug_handler(dev); >> + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); >> +} >> + >> static void spapr_init_cpus(sPAPRMachineState *spapr) >> { >> MachineState *machine = MACHINE(spapr); >> @@ -1998,7 +2029,8 @@ static void spapr_init_cpus(sPAPRMachineState *spapr) >> sPAPRDRConnector *drc = >> spapr_dr_connector_new(OBJECT(spapr), >> SPAPR_DR_CONNECTOR_TYPE_CPU, >> - (core_id / smp_threads) * smt); >> + (core_id / smp_threads) * smt, >> + spapr_core_release); >> >> qemu_register_reset(spapr_drc_reset, drc); >> } >> @@ -2596,29 +2628,6 @@ out: >> error_propagate(errp, local_err); >> } >> >> -typedef struct sPAPRDIMMState { >> - uint32_t nr_lmbs; >> -} sPAPRDIMMState; >> - >> -static void spapr_lmb_release(DeviceState *dev, void *opaque) >> -{ >> - sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque; >> - HotplugHandler *hotplug_ctrl; >> - >> - if (--ds->nr_lmbs) { >> - return; >> - } >> - >> - g_free(ds); >> - >> - /* >> - * Now that all the LMBs have been removed by the guest, call the >> - * pc-dimm unplug handler to cleanup up the pc-dimm device. >> - */ >> - hotplug_ctrl = qdev_get_hotplug_handler(dev); >> - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); >> -} >> - >> static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, >> Error **errp) >> { >> @@ -2636,7 +2645,7 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, >> g_assert(drc); >> >> drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); >> - drck->detach(drc, dev, spapr_lmb_release, ds, errp); >> + drck->detach(drc, dev, ds, errp); >> addr += SPAPR_MEMORY_BLOCK_SIZE; >> } >> >> @@ -2712,14 +2721,6 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, >> object_unparent(OBJECT(dev)); >> } >> >> -static void spapr_core_release(DeviceState *dev, void *opaque) >> -{ >> - HotplugHandler *hotplug_ctrl; >> - >> - hotplug_ctrl = qdev_get_hotplug_handler(dev); >> - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); >> -} >> - >> static >> void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, >> Error **errp) >> @@ -2745,7 +2746,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, >> g_assert(drc); >> >> drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); >> - drck->detach(drc, dev, spapr_core_release, NULL, &local_err); >> + drck->detach(drc, dev, NULL, &local_err); >> if (local_err) { >> error_propagate(errp, local_err); >> return; >> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c >> index a1cdc87..afe5d82 100644 >> --- a/hw/ppc/spapr_drc.c >> +++ b/hw/ppc/spapr_drc.c >> @@ -99,8 +99,8 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc, >> if (drc->awaiting_release) { >> if (drc->configured) { >> trace_spapr_drc_set_isolation_state_finalizing(get_index(drc)); >> - drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, >> - drc->detach_cb_opaque, NULL); >> + drck->detach(drc, DEVICE(drc->dev), drc->detach_cb_opaque, >> + NULL); >> } else { >> trace_spapr_drc_set_isolation_state_deferring(get_index(drc)); >> } >> @@ -153,8 +153,8 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc, >> if (drc->awaiting_release && >> drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) { >> trace_spapr_drc_set_allocation_state_finalizing(get_index(drc)); >> - drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, >> - drc->detach_cb_opaque, NULL); >> + drck->detach(drc, DEVICE(drc->dev), drc->detach_cb_opaque, >> + NULL); >> } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) { >> drc->awaiting_allocation = false; >> } >> @@ -405,12 +405,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt, >> } >> >> static void detach(sPAPRDRConnector *drc, DeviceState *d, >> - spapr_drc_detach_cb *detach_cb, >> void *detach_cb_opaque, Error **errp) >> { >> trace_spapr_drc_detach(get_index(drc)); >> >> - drc->detach_cb = detach_cb; >> drc->detach_cb_opaque = detach_cb_opaque; >> >> /* if we've signalled device presence to the guest, or if the guest >> @@ -498,8 +496,7 @@ static void reset(DeviceState *d) >> * force removal if we are >> */ >> if (drc->awaiting_release) { >> - drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, >> - drc->detach_cb_opaque, NULL); >> + drck->detach(drc, DEVICE(drc->dev), drc->detach_cb_opaque, NULL); >> } >> >> /* non-PCI devices may be awaiting a transition to UNUSABLE */ >> @@ -566,7 +563,8 @@ static void unrealize(DeviceState *d, Error **errp) >> >> sPAPRDRConnector *spapr_dr_connector_new(Object *owner, >> sPAPRDRConnectorType type, >> - uint32_t id) >> + uint32_t id, >> + spapr_drc_detach_cb *detach_cb) >> { >> sPAPRDRConnector *drc = >> SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR)); >> @@ -577,6 +575,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner, >> drc->type = type; >> drc->id = id; >> drc->owner = owner; >> + drc->detach_cb = detach_cb; >> prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", get_index(drc)); >> object_property_add_child(owner, prop_name, OBJECT(drc), NULL); >> object_property_set_bool(OBJECT(drc), true, "realized", NULL); >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c >> index e7567e2..935e65e 100644 >> --- a/hw/ppc/spapr_pci.c >> +++ b/hw/ppc/spapr_pci.c >> @@ -1392,7 +1392,7 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc, >> { >> sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); >> >> - drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp); >> + drck->detach(drc, DEVICE(pdev), phb, errp); >> } >> >> static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb, >> @@ -1764,7 +1764,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) >> for (i = 0; i < PCI_SLOT_MAX * 8; i++) { >> spapr_dr_connector_new(OBJECT(phb), >> SPAPR_DR_CONNECTOR_TYPE_PCI, >> - (sphb->index << 16) | i); >> + (sphb->index << 16) | i, >> + spapr_phb_remove_pci_device_cb); >> } >> } >> >> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h >> index 5524247..0a2c173 100644 >> --- a/include/hw/ppc/spapr_drc.h >> +++ b/include/hw/ppc/spapr_drc.h >> @@ -189,7 +189,6 @@ typedef struct sPAPRDRConnectorClass { >> void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt, >> int fdt_start_offset, bool coldplug, Error **errp); >> void (*detach)(sPAPRDRConnector *drc, DeviceState *d, >> - spapr_drc_detach_cb *detach_cb, >> void *detach_cb_opaque, Error **errp); >> bool (*release_pending)(sPAPRDRConnector *drc); >> void (*set_signalled)(sPAPRDRConnector *drc); >> @@ -197,7 +196,8 @@ typedef struct sPAPRDRConnectorClass { >> >> sPAPRDRConnector *spapr_dr_connector_new(Object *owner, >> sPAPRDRConnectorType type, >> - uint32_t id); >> + uint32_t id, >> + spapr_drc_detach_cb *detach_cb); >> sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index); >> sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type, >> uint32_t id);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 80d12d0..bc11757 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1887,6 +1887,29 @@ static void spapr_drc_reset(void *opaque) } } +typedef struct sPAPRDIMMState { + uint32_t nr_lmbs; +} sPAPRDIMMState; + +static void spapr_lmb_release(DeviceState *dev, void *opaque) +{ + sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque; + HotplugHandler *hotplug_ctrl; + + if (--ds->nr_lmbs) { + return; + } + + g_free(ds); + + /* + * Now that all the LMBs have been removed by the guest, call the + * pc-dimm unplug handler to cleanup up the pc-dimm device. + */ + hotplug_ctrl = qdev_get_hotplug_handler(dev); + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); +} + static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) { MachineState *machine = MACHINE(spapr); @@ -1900,7 +1923,7 @@ static void spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr) addr = i * lmb_size + spapr->hotplug_memory.base; drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_LMB, - addr/lmb_size); + (addr / lmb_size), spapr_lmb_release); qemu_register_reset(spapr_drc_reset, drc); } } @@ -1956,6 +1979,14 @@ static CPUArchId *spapr_find_cpu_slot(MachineState *ms, uint32_t id, int *idx) return &ms->possible_cpus->cpus[index]; } +static void spapr_core_release(DeviceState *dev, void *opaque) +{ + HotplugHandler *hotplug_ctrl; + + hotplug_ctrl = qdev_get_hotplug_handler(dev); + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); +} + static void spapr_init_cpus(sPAPRMachineState *spapr) { MachineState *machine = MACHINE(spapr); @@ -1998,7 +2029,8 @@ static void spapr_init_cpus(sPAPRMachineState *spapr) sPAPRDRConnector *drc = spapr_dr_connector_new(OBJECT(spapr), SPAPR_DR_CONNECTOR_TYPE_CPU, - (core_id / smp_threads) * smt); + (core_id / smp_threads) * smt, + spapr_core_release); qemu_register_reset(spapr_drc_reset, drc); } @@ -2596,29 +2628,6 @@ out: error_propagate(errp, local_err); } -typedef struct sPAPRDIMMState { - uint32_t nr_lmbs; -} sPAPRDIMMState; - -static void spapr_lmb_release(DeviceState *dev, void *opaque) -{ - sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque; - HotplugHandler *hotplug_ctrl; - - if (--ds->nr_lmbs) { - return; - } - - g_free(ds); - - /* - * Now that all the LMBs have been removed by the guest, call the - * pc-dimm unplug handler to cleanup up the pc-dimm device. - */ - hotplug_ctrl = qdev_get_hotplug_handler(dev); - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); -} - static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, Error **errp) { @@ -2636,7 +2645,7 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, g_assert(drc); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); - drck->detach(drc, dev, spapr_lmb_release, ds, errp); + drck->detach(drc, dev, ds, errp); addr += SPAPR_MEMORY_BLOCK_SIZE; } @@ -2712,14 +2721,6 @@ static void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, object_unparent(OBJECT(dev)); } -static void spapr_core_release(DeviceState *dev, void *opaque) -{ - HotplugHandler *hotplug_ctrl; - - hotplug_ctrl = qdev_get_hotplug_handler(dev); - hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); -} - static void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) @@ -2745,7 +2746,7 @@ void spapr_core_unplug_request(HotplugHandler *hotplug_dev, DeviceState *dev, g_assert(drc); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); - drck->detach(drc, dev, spapr_core_release, NULL, &local_err); + drck->detach(drc, dev, NULL, &local_err); if (local_err) { error_propagate(errp, local_err); return; diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c index a1cdc87..afe5d82 100644 --- a/hw/ppc/spapr_drc.c +++ b/hw/ppc/spapr_drc.c @@ -99,8 +99,8 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc, if (drc->awaiting_release) { if (drc->configured) { trace_spapr_drc_set_isolation_state_finalizing(get_index(drc)); - drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, - drc->detach_cb_opaque, NULL); + drck->detach(drc, DEVICE(drc->dev), drc->detach_cb_opaque, + NULL); } else { trace_spapr_drc_set_isolation_state_deferring(get_index(drc)); } @@ -153,8 +153,8 @@ static uint32_t set_allocation_state(sPAPRDRConnector *drc, if (drc->awaiting_release && drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) { trace_spapr_drc_set_allocation_state_finalizing(get_index(drc)); - drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, - drc->detach_cb_opaque, NULL); + drck->detach(drc, DEVICE(drc->dev), drc->detach_cb_opaque, + NULL); } else if (drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_USABLE) { drc->awaiting_allocation = false; } @@ -405,12 +405,10 @@ static void attach(sPAPRDRConnector *drc, DeviceState *d, void *fdt, } static void detach(sPAPRDRConnector *drc, DeviceState *d, - spapr_drc_detach_cb *detach_cb, void *detach_cb_opaque, Error **errp) { trace_spapr_drc_detach(get_index(drc)); - drc->detach_cb = detach_cb; drc->detach_cb_opaque = detach_cb_opaque; /* if we've signalled device presence to the guest, or if the guest @@ -498,8 +496,7 @@ static void reset(DeviceState *d) * force removal if we are */ if (drc->awaiting_release) { - drck->detach(drc, DEVICE(drc->dev), drc->detach_cb, - drc->detach_cb_opaque, NULL); + drck->detach(drc, DEVICE(drc->dev), drc->detach_cb_opaque, NULL); } /* non-PCI devices may be awaiting a transition to UNUSABLE */ @@ -566,7 +563,8 @@ static void unrealize(DeviceState *d, Error **errp) sPAPRDRConnector *spapr_dr_connector_new(Object *owner, sPAPRDRConnectorType type, - uint32_t id) + uint32_t id, + spapr_drc_detach_cb *detach_cb) { sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(object_new(TYPE_SPAPR_DR_CONNECTOR)); @@ -577,6 +575,7 @@ sPAPRDRConnector *spapr_dr_connector_new(Object *owner, drc->type = type; drc->id = id; drc->owner = owner; + drc->detach_cb = detach_cb; prop_name = g_strdup_printf("dr-connector[%"PRIu32"]", get_index(drc)); object_property_add_child(owner, prop_name, OBJECT(drc), NULL); object_property_set_bool(OBJECT(drc), true, "realized", NULL); diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index e7567e2..935e65e 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -1392,7 +1392,7 @@ static void spapr_phb_remove_pci_device(sPAPRDRConnector *drc, { sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); - drck->detach(drc, DEVICE(pdev), spapr_phb_remove_pci_device_cb, phb, errp); + drck->detach(drc, DEVICE(pdev), phb, errp); } static sPAPRDRConnector *spapr_phb_get_pci_func_drc(sPAPRPHBState *phb, @@ -1764,7 +1764,8 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp) for (i = 0; i < PCI_SLOT_MAX * 8; i++) { spapr_dr_connector_new(OBJECT(phb), SPAPR_DR_CONNECTOR_TYPE_PCI, - (sphb->index << 16) | i); + (sphb->index << 16) | i, + spapr_phb_remove_pci_device_cb); } } diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h index 5524247..0a2c173 100644 --- a/include/hw/ppc/spapr_drc.h +++ b/include/hw/ppc/spapr_drc.h @@ -189,7 +189,6 @@ typedef struct sPAPRDRConnectorClass { void (*attach)(sPAPRDRConnector *drc, DeviceState *d, void *fdt, int fdt_start_offset, bool coldplug, Error **errp); void (*detach)(sPAPRDRConnector *drc, DeviceState *d, - spapr_drc_detach_cb *detach_cb, void *detach_cb_opaque, Error **errp); bool (*release_pending)(sPAPRDRConnector *drc); void (*set_signalled)(sPAPRDRConnector *drc); @@ -197,7 +196,8 @@ typedef struct sPAPRDRConnectorClass { sPAPRDRConnector *spapr_dr_connector_new(Object *owner, sPAPRDRConnectorType type, - uint32_t id); + uint32_t id, + spapr_drc_detach_cb *detach_cb); sPAPRDRConnector *spapr_dr_connector_by_index(uint32_t index); sPAPRDRConnector *spapr_dr_connector_by_id(sPAPRDRConnectorType type, uint32_t id);
The idea of moving the detach callback functions to the constructor of the dr_connector is to set them statically at init time, avoiding any post-load hooks to restore it (after a migration, for example). Summary of changes: - hw/ppc/spapr_drc.c and include/hw/ppc/spapr_drc.h: * spapr_dr_connector_new() now has an additional parameter, spapr_drc_detach_cb *detach_cb * 'spapr_drc_detach_cb *detach_cb' parameter was removed of the detach function pointer in sPAPRDRConnectorClass - hw/ppc/spapr_pci.c: * the callback 'spapr_phb_remove_pci_device_cb' is now passed as a parameter in 'spapr_dr_connector_new' instead of 'drck->detach()' - hw/ppc/spapr.c: * 'spapr_create_lmb_dr_connectors' now passes the callback 'spapr_lmb_release' to 'spapr_dr_connector_new' instead of 'drck-detach()' * 'spapr_init_cpus' now passes the callback 'spapr_core_release' to 'spapr_dr_connector_new' instead of 'drck-detach()' * moved the callback functions up in the code so they can be referenced by 'spapr_create_lmb_dr_connectors' and 'spapr_init_cpus' Signed-off-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> --- hw/ppc/spapr.c | 71 +++++++++++++++++++++++----------------------- hw/ppc/spapr_drc.c | 17 ++++++----- hw/ppc/spapr_pci.c | 5 ++-- include/hw/ppc/spapr_drc.h | 4 +-- 4 files changed, 49 insertions(+), 48 deletions(-)