Re: [Qemu-devel] [PATCH] vfio: free dynamically-allocated data in instance_finalize

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

On Tue, 2015-02-03 at 17:25 +0100, Paolo Bonzini wrote:
> 
> On 03/02/2015 16:20, Alex Williamson wrote:
> > On Tue, 2015-02-03 at 13:48 +0100, Paolo Bonzini wrote:
> >> In order to enable out-of-BQL address space lookup, destruction of
> >> devices needs to be split in two phases.
> >>
> >> Unrealize is the first phase; once it complete no new accesses will
> >> be started, but there may still be pending memory accesses can still
> >> be completed.
> >>
> >> The second part is freeing the device, which only happens once all memory
> >> accesses are complete.  At this point the reference count has dropped to
> >> zero, an RCU grace period must have completed (because the RCU-protected
> >> FlatViews hold a reference to the device via memory_region_ref).  This is
> >> when instance_finalize is called.
> >>
> >> Freeing data belongs in an instance_finalize callback, because the
> >> dynamically allocated memory can still be used after unrealize by the
> >> pending memory accesses.
> >>
> >> In the case of VFIO, the unrealize callback is too early to munmap the
> >> BARs.  The munmap must be delayed until memory accesses are complete.
> >> To do this, split vfio_unmap_bars in two.  The removal step, now called
> >> vfio_unregister_bars, remains in vfio_exitfn.  The reclamation step
> >> is vfio_unmap_bars and is moved to the instance_finalize callback.
> >>
> >> Similarly, quirk MemoryRegions have to be removed during
> >> vfio_unregister_bars, but freeing the data structure must be delayed
> >> to vfio_unmap_bars.
> >>
> >> Cc: Alex Williamson <alex.williamson@redhat.com>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >> 	This patch is part of the third installment 3 of the RCU work.
> >> 	Sending it out separately for Alex to review it.
> >>
> >>  hw/vfio/pci.c |   78 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---------
> >>  1 file changed, 68 insertions(+), 10 deletions(-)
> > 
> > Looks good to me.  I don't see any external dependencies, so do you want
> > me to pull this in through my branch?  Thanks,
> 
> Yes, please.

Hmm, except qemu segfaults in whatever sanity test/capabilities probing
happens when the VM is first opened.  I haven't figured out how to
capture that instance in gdb yet.