From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40947) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d49aZ-0007wd-PO for qemu-devel@nongnu.org; Fri, 28 Apr 2017 13:21:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d49aV-0000SE-OR for qemu-devel@nongnu.org; Fri, 28 Apr 2017 13:21:51 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:58532 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1d49aV-0000Rw-Hx for qemu-devel@nongnu.org; Fri, 28 Apr 2017 13:21:47 -0400 Received: from pps.filterd (m0098416.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v3SH9UOM007941 for ; Fri, 28 Apr 2017 13:21:46 -0400 Received: from e24smtp05.br.ibm.com (e24smtp05.br.ibm.com [32.104.18.26]) by mx0b-001b2d01.pphosted.com with ESMTP id 2a45uccpg0-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Fri, 28 Apr 2017 13:21:46 -0400 Received: from localhost by e24smtp05.br.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 28 Apr 2017 14:21:44 -0300 References: <20170426213124.29064-1-danielhb@linux.vnet.ibm.com> <20170426213124.29064-2-danielhb@linux.vnet.ibm.com> <149324459112.4290.2308973009808049809@loki> From: Daniel Henrique Barboza Date: Fri, 28 Apr 2017 14:21:07 -0300 MIME-Version: 1.0 In-Reply-To: <149324459112.4290.2308973009808049809@loki> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Message-Id: <02019918-625b-ba68-7b77-183faa82851c@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 1/4] hw/ppc: setting spapr_drc_detach_cb in spapr_dr_connector_new List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Michael Roth , qemu-devel@nongnu.org Cc: qemu-ppc@nongnu.org, david@gibson.dropbear.id.au On 04/26/2017 07:09 PM, Michael Roth wrote: > 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 > 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. I'm on it! Thanks! > >> --- >> 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 >> >> >