Re: [Qemu-devel] [PATCH 6/9] ioport: split deletion and destruction

[Paolo Bonzini <pbonzini@redhat.com>]

Il 31/07/2014 14:34, Peter Crosthwaite ha scritto:
> So I found the original implementation made sense to me, in that _del
> is the converse of _add and _destroy _init WRT to the MR ops.
> 
> Currently
> _init = malloc array
> _add = mr_init + mr_add_subregion
> _del = mr_del_subregion + mr_destroy
> _destory = free array
> 
> This delays mr_destory to _destroy breaking the symmetry.

Note that there is a fundamental difference between init and destroy:
when you do init, the MemoryRegion should not be providing references to
the MemoryRegion's owner.  So you can do it pretty much whenever you want.

However, after del_subregion the device models can have references to
the MemoryRegion's owner (via address_space_map/unmap) and destroy has
to be delayed to a point when these references have disappeared.  This
is why I'm keen on making memory_region_destroy happen automatically at
finalize time, as I do later in this series: last year I had tried doing
it manually with instance_finalize, and it was an unreviewable mess.

With this patch, you still would need manual portio_list_destroy calls
in instance_finalize, but PortioLists are used sparingly so it's much
simpler to manage them.

This difference is why in this patch I focused only on
portio_list_del/destroy.  However, I can try and move init to
portio_list_add; then the symmetry is restored.


> With this change is is still possible to:
> 
> _init()
> _add()
> _del()
> _add()
> _del()
> _destrory()
> 
> And not leak a ref, with the reinited memory region on second add?
> 
> Then again I may be misunderstanding, as apparently neither of these
> API have any users so I'm having trouble getting a handle on intended
> usage:

Yes, that's because these patches are mostly used by ISA devices or
VGAs, and currently neither is hot-unpluggable.

Paolo