Re: [Qemu-devel] [PATCH] vfio-pci: unparent BAR subregions

[Alex Williamson <alex.williamson@redhat.com>]

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.williamson@redhat.com>
> >>> Cc: Paolo Bonzini <pbonzini@redhat.com>
> >>> Cc: qemu-stable@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));
> >>>      }
> >>>  }
> >>>  
> >>>
> >>>
> >>>
> > 
> > 
> > 
> > 
> >