On 31/01/2015 00:55, Alex Williamson wrote: > Commit d8d95814609e replaced a number of memory_region_destroy() > calls with object_unparent() calls. The logic appears to be that > subregions need to be unparented, but the base region is destroyed > with the device object. Doing hotplug testing with vfio-pci I > occasionally get a segfault from object_finalize_child_property() > due to completely bogus class pointers on the child Object. Adding > the explicit object_unparent() for these subregions resolves the > problem, however I question the sanity of the Memory API now where > we sometimes need to destroy MemoryRegions, but the rules aren't > clear
There is no memory_region_destroy API because you cannot destroy MemoryRegions. All you do is releasing the link between the VFIO device (the parent, specified in memory_region_init*) and the MemoryRegion. The link caused the VFIO device to keep the MemoryRegion alive. There can be pending references to the VFIO device at unrealize time, and this is why the memory_region_destroy() API was not enough. For example if someone was doing I/O to a BAR and thus address_space_map is keeping the VFIO device alive. The explicit memory_region_destroy() function made it much harder to handle this case. You had to define an instance_finalize function for every class, and do memory_region_destroy() there. Not surprisingly, no one did that. Sure, it's not a common case and a well-behaving guest does not do that, but if it does it means use-after-frees and thus a possible guest->host escalation. Instead, the implicit destruction via reference counting makes this case easy to handle, because reclamation is done automatically when the VFIO device dies. Explicit object_unparent() is only needed if you recreate the memory region during the lifetime of the object. This is rarely needed, and it is simple to spot if it's needed. If you do memory_region_init* outside the realize function, most likely you need a matching object_unparent somewhere else in the device logic. This was the idea behind commit d8d95814609e. It only touched a handful of files because almost no one does memory_region_init* outside the realize function, and in particular VFIO doesn't. VFIO follows the common convention of only creating regions in realize, and thus does not need object_unparent. > and there's no longer a memory_region_destroy() function, so > we need to reach over to some other random QEMU API It's not random. Object is the parent class of MemoryRegion. object_unparent is a method for MemoryRegion. > and unparent an object that we barely know about I'm not sure about this? You certainly know the memory regions you create. > and certainly didn't explicitly parent previously. You did when you passed the VFIO device to memory_region_init*. I'm afraid this patch is incorrect. You have to find out where the region is being overwritten. Thanks, Paolo > Signed-off-by: Alex Williamson <alex.william...@redhat.com> > Cc: Paolo Bonzini <pbonz...@redhat.com> > Cc: qemu-sta...@nongnu.org > --- > > hw/vfio/pci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c > index 014a92c..c71499e 100644 > --- a/hw/vfio/pci.c > +++ b/hw/vfio/pci.c > @@ -2294,10 +2294,12 @@ static void vfio_unmap_bar(VFIOPCIDevice *vdev, int > nr) > > memory_region_del_subregion(&bar->region.mem, &bar->region.mmap_mem); > munmap(bar->region.mmap, memory_region_size(&bar->region.mmap_mem)); > + object_unparent(OBJECT(&bar->region.mmap_mem)); > > if (vdev->msix && vdev->msix->table_bar == nr) { > memory_region_del_subregion(&bar->region.mem, &vdev->msix->mmap_mem); > munmap(vdev->msix->mmap, memory_region_size(&vdev->msix->mmap_mem)); > + object_unparent(OBJECT(&vdev->msix->mmap_mem)); > } > } > > > >