[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/4] hw/ppc: setting spapr_drc_detach_cb in spap
|
From: |
Michael Roth |
|
Subject: |
Re: [Qemu-devel] [PATCH 1/4] hw/ppc: setting spapr_drc_detach_cb in spapr_dr_connector_new |
|
Date: |
Wed, 26 Apr 2017 17:09:51 -0500 |
|
User-agent: |
alot/0.5.1 |
Quoting Daniel Henrique Barboza (2017-04-26 16:31:21)
> 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 <address@hidden>
Patch looks good, but we need a follow-up afterward that removes the
need for the void *opaque argument as I mentioned in my last email.
Otherwise we'd still need to restore the opaque argument in post-load
which isn't really viable.
> ---
> 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);
> --
> 2.9.3
>
>