| Message ID | 1408097555-28126-7-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 15, 2014 at 08:12:28PM +1000, Alexey Kardashevskiy wrote: > Every sPAPRTCETable object holds an IOMMU memory region which holds > a referenced to the sPAPRTCETable instance. So if we want to free > an sPAPRTCETable instance, calling object_unref() will not be enough > as embedded memory region will hold the reference and we need to break > the loop. > > This adds a spapr_tce_free_table() helper which destroys the embedded > memory region and then calls object_unref() on the sPAPRTCETable instance > which will succeed now. The helper adds a new child under unique name > derived from LIOBN. > > As we are here, fix spapr_tce_new_table() callers. > At the moment spapr_tce_new_table() references sPAPRTCETable twice - > once in object_new() and second time in object_property_add_child(). > The callers of spapr_tce_new_table() should take care of correct > referencing. So I've been trying to determine if there's any way to avoid this by not constructing the reference loop in the first place, but all the qom is breaking my brain.
On 08/26/2014 04:16 PM, David Gibson wrote: > On Fri, Aug 15, 2014 at 08:12:28PM +1000, Alexey Kardashevskiy wrote: >> Every sPAPRTCETable object holds an IOMMU memory region which holds >> a referenced to the sPAPRTCETable instance. So if we want to free >> an sPAPRTCETable instance, calling object_unref() will not be enough >> as embedded memory region will hold the reference and we need to break >> the loop. >> >> This adds a spapr_tce_free_table() helper which destroys the embedded >> memory region and then calls object_unref() on the sPAPRTCETable instance >> which will succeed now. The helper adds a new child under unique name >> derived from LIOBN. >> >> As we are here, fix spapr_tce_new_table() callers. >> At the moment spapr_tce_new_table() references sPAPRTCETable twice - >> once in object_new() and second time in object_property_add_child(). >> The callers of spapr_tce_new_table() should take care of correct >> referencing. > > So I've been trying to determine if there's any way to avoid this by > not constructing the reference loop in the first place, but all the > qom is breaking my brain. Well. I could have added an additional object to sPAPRTCETable (just a pointer in the struct but not a QOM-child!) and use it as an owner for MemoryRegion. It would not hold reference to the table. But since I do not grasp the idea of having an owner for a MemoryRegion, this can break something which I cannot even imagine :)
diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index 588d442..74949f4 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -147,6 +147,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, bool vfio_accel) { sPAPRTCETable *tcet; + char buf[32]; if (spapr_tce_find_by_liobn(liobn)) { fprintf(stderr, "Attempted to create TCE table with duplicate" @@ -165,13 +166,21 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, tcet->nb_table = nb_table; tcet->vfio_accel = vfio_accel; - object_property_add_child(OBJECT(owner), "tce-table", OBJECT(tcet), NULL); + snprintf(buf, sizeof(buf) - 1, "tce-table-%08X", tcet->liobn); + object_property_add_child(OBJECT(owner), buf, OBJECT(tcet), NULL); object_property_set_bool(OBJECT(tcet), true, "realized", NULL); return tcet; } +void spapr_tce_free_table(sPAPRTCETable *tcet) +{ + memory_region_destroy(&tcet->iommu); + + object_unref(OBJECT(tcet)); +} + static void spapr_tce_table_finalize(Object *obj) { sPAPRTCETable *tcet = SPAPR_TCE_TABLE(obj); diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c index 17eb0d8..aa20c36 100644 --- a/hw/ppc/spapr_pci.c +++ b/hw/ppc/spapr_pci.c @@ -654,6 +654,8 @@ static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp) /* Register default 32bit DMA window */ memory_region_add_subregion(&sphb->iommu_root, 0, spapr_tce_get_iommu(tcet)); + + object_unref(OBJECT(tcet)); } static int spapr_phb_children_reset(Object *child, void *opaque) diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c index d3bddf2..51b4314 100644 --- a/hw/ppc/spapr_pci_vfio.c +++ b/hw/ppc/spapr_pci_vfio.c @@ -69,6 +69,8 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp) /* Register default 32bit DMA window */ memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset, spapr_tce_get_iommu(tcet)); + + object_unref(OBJECT(tcet)); } static void spapr_phb_vfio_reset(DeviceState *qdev) diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index 782519a..537072f 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -477,6 +477,7 @@ sPAPRTCETable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn, uint32_t page_shift, uint32_t nb_table, bool vfio_accel); +void spapr_tce_free_table(sPAPRTCETable *tcet); MemoryRegion *spapr_tce_get_iommu(sPAPRTCETable *tcet); void spapr_tce_set_bypass(sPAPRTCETable *tcet, bool bypass); int spapr_dma_dt(void *fdt, int node_off, const char *propname,
Every sPAPRTCETable object holds an IOMMU memory region which holds a referenced to the sPAPRTCETable instance. So if we want to free an sPAPRTCETable instance, calling object_unref() will not be enough as embedded memory region will hold the reference and we need to break the loop. This adds a spapr_tce_free_table() helper which destroys the embedded memory region and then calls object_unref() on the sPAPRTCETable instance which will succeed now. The helper adds a new child under unique name derived from LIOBN. As we are here, fix spapr_tce_new_table() callers. At the moment spapr_tce_new_table() references sPAPRTCETable twice - once in object_new() and second time in object_property_add_child(). The callers of spapr_tce_new_table() should take care of correct referencing. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- hw/ppc/spapr_iommu.c | 11 ++++++++++- hw/ppc/spapr_pci.c | 2 ++ hw/ppc/spapr_pci_vfio.c | 2 ++ include/hw/ppc/spapr.h | 1 + 4 files changed, 15 insertions(+), 1 deletion(-)