Re: [Qemu-trivial] [Qemu-devel] [PATCH] serial-pci: remove memory regions from BAR before destroying them

[Peter Crosthwaite]

On Tue, Jul 15, 2014 at 12:20 AM, Paolo Bonzini <address@hidden> wrote:
> Il 14/07/2014 14:36, Peter Crosthwaite ha scritto:
>
>> On Thu, Jun 26, 2014 at 5:32 PM, Markus Armbruster <address@hidden>
>> wrote:
>>>
>>> Paolo Bonzini <address@hidden> writes:
>>>
>>>> Otherwise, hot-unplug of pci-serial-2x trips the assertion
>>>> in memory_region_destroy:
>>>>
>>>>     (qemu) device_del gg
>>>>     (qemu) qemu-system-x86_64: /work/armbru/tmp/qemu/memory.c:1021:
>>>> memory_region_destroy: Assertion `((&mr->subregions)->tqh_first == ((void
>>>> *)0))' failed.
>>>>     Aborted (core dumped)
>>>>
>>>> Reported-by: Markus Armbruster <address@hidden>
>>>> Signed-off-by: Paolo Bonzini <address@hidden>
>>>
>>>
>>> Reviewed-by: Markus Armbruster <address@hidden>
>>>
>>
>> Reviewed-by: Peter Crosthwaite <address@hidden>
>>
>>> Would it make sense to add a "must not contain subregions" to
>>> memory_region_destroy()'s function comment?
>>>
>>
>> Any reason to just not patch the memory region finaliser to unparent
>
>
> Note that unparent for memory regions is _not_ memory_region_del_subregion.
> It is memory_region_destroy.
>

Yes, bad choice of words by me. What I was going for was "un-contain" I guess.

Regards,
Peter

> The parent object of a memory region is a device; the _container_ of a
> memory region is another memory region.
>
>
>> all contained subregions automatically rather than assert? Destroying
>> a container should imply removing the subregion relationship and
>> simply orphan the subregion.
>
>
> This makes sense since we will soon make memory_region_destroy optional
> (devices will automatically destroy their memory regions).  Before
> QOMification, however, I think the assert was a useful debugging tool,
> guaranteeing that owners of memory regions were destroyed in the right
> order.  So we could indeed revisit this in 2.2 and make
> memory_region_del_subregion also optional.
>
> Paolo
>