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

[Paolo Bonzini]

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).

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.

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,

Paolo

> Alex
> 
>>> Signed-off-by: Alex Williamson <address@hidden>
>>> Cc: Paolo Bonzini <address@hidden>
>>> Cc: address@hidden
>>> ---
>>>
>>>  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));
>>>      }
>>>  }
>>>  
>>>
>>>
>>>
> 
> 
> 
> 
>