On Sat, 2015-01-31 at 21:47 +0100, Paolo Bonzini wrote:
>
> On 31/01/2015 16:10, Alex Williamson wrote:
> >> 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.
> >
> > Thanks Paolo, so if I look more closely at where you added
> > object_unparent() calls in d8d95814609e, I can see that they're
> > associated with dynamically allocated objects that are freed as part of
> > the vfio device exitfn. vdev->msix is also such a structure and is the
> > property causing us the segfaults.
>
> Yeah, this is a different case than the one I mentioned above, and
> you're right that this is also not exactly the common convention---so it
> is a second case of needing an explicit object_unparent.
>
> > So, I think the second
> > object_unparent() call is correct and that the guiding principle is that
> > any MemoryRegion associated with a dynamically allocated structure and
> > freed as part of the class exit callback needs to be explicitly
> > unparented. Does that sound right?
>
> The pedant in me asks to do the object_unparent in vfio_put_device, just
> before freeing vdev->msix. This way you already abide by RCU's
> principle of separating removal and reclamation (and I won't have to
> move it in part 3 of the RCU patches, which is the next to be posted;
> that part will move the vfio_put_device call to instance_finalize).
Ok.
> Another possible change would be to make vdev->msix statically allocated
> (checking vdev->msix.entries != 0 instead of vdev->msix != NULL,
> possibly hidden beneath an inline function vfio_has_msix). Then you'd
> be in the exact same case as the other memory regions and wouldn't need
> an unparent at all. But I am not certain myself of the relative beauty
> of this, and it is obviously less suitable for qemu-stable, so do go
> ahead with the one-liner.
The pendant in me prefers that the MSI-X structure is only allocated for
devices that actually have MSI-X ;) v2 posted.
> I'll improve the documentation as soon as the code actually follows the
> principles that have to be documented. But now that the ball is rolling
> on RCU and multithreading, documentation is indeed getting more and more
> important.
Thanks, it's definitely not obvious how this works in the current docs.
Alex
> >>> 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));
> >>> }
> >>> }
> >>>
> >>>
> >>>
> >>>
> >
> >
> >
> >
> >
