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
