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

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

[Paolo Bonzini]

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 <armbru@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial-pci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/char/serial-pci.c b/hw/char/serial-pci.c
index 6c25296..7340c8d 100644
--- a/hw/char/serial-pci.c
+++ b/hw/char/serial-pci.c
@@ -148,6 +148,7 @@ static void multi_serial_pci_exit(PCIDevice *dev)
     for (i = 0; i < pci->ports; i++) {
         s = pci->state + i;
         serial_exit_core(s);
+        memory_region_del_subregion(&pci->iobar, &s->io);
         memory_region_destroy(&s->io);
         g_free(pci->name[i]);
     }
-- 
1.9.3

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> 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 <armbru@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Would it make sense to add a "must not contain subregions" to
memory_region_destroy()'s function comment?

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

[Paolo Bonzini]

Il 26/06/2014 09:32, Markus Armbruster ha scritto:
>> > Reported-by: Markus Armbruster <armbru@redhat.com>
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Would it make sense to add a "must not contain subregions" to
> memory_region_destroy()'s function comment?

I don't know, in the end the assertion is pretty self-explanatory.  If 
we add hotplug support to libqos, we can test it.

Paolo

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

[Peter Crosthwaite]

On Thu, Jun 26, 2014 at 5:32 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Paolo Bonzini <pbonzini@redhat.com> 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 <armbru@redhat.com>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>

Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

> 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
all contained subregions automatically rather than assert? Destroying
a container should imply removing the subregion relationship and
simply orphan the subregion.

Regards,
Peter

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

[Paolo Bonzini]

Il 14/07/2014 14:36, Peter Crosthwaite ha scritto:
> On Thu, Jun 26, 2014 at 5:32 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Paolo Bonzini <pbonzini@redhat.com> 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 <armbru@redhat.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>
>> 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.

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

Re: [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 <pbonzini@redhat.com> wrote:
> Il 14/07/2014 14:36, Peter Crosthwaite ha scritto:
>
>> On Thu, Jun 26, 2014 at 5:32 PM, Markus Armbruster <armbru@redhat.com>
>> wrote:
>>>
>>> Paolo Bonzini <pbonzini@redhat.com> 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 <armbru@redhat.com>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>
>>>
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>>
>>
>> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>>> 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
>

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

[Markus Armbruster]

Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> On Thu, Jun 26, 2014 at 5:32 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Paolo Bonzini <pbonzini@redhat.com> 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 <armbru@redhat.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>
>
> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>

Nominating for 2.1, just to be explicit.

[...]

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

[Gerd Hoffmann]

On Mo, 2014-07-14 at 17:25 +0200, Markus Armbruster wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
> 
> > On Thu, Jun 26, 2014 at 5:32 PM, Markus Armbruster <armbru@redhat.com> wrote:
> >> Paolo Bonzini <pbonzini@redhat.com> 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 <armbru@redhat.com>
> >>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >>
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >>
> >
> > Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> 
> Nominating for 2.1, just to be explicit.
> 
> [...]

Hmm, is trivial out of service?  I remember thinking "Ok, trivial patch,
qemu-trivial is cc'ed, that'll pick it up, nothing to do for me."

cheers,
  Gerd

PS: Saw paolo sticked the patch into one of his pull requests meanwhile.

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

[Paolo Bonzini]

Il 15/07/2014 10:35, Gerd Hoffmann ha scritto:
> On Mo, 2014-07-14 at 17:25 +0200, Markus Armbruster wrote:
>> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>>
>>> On Thu, Jun 26, 2014 at 5:32 PM, Markus Armbruster <armbru@redhat.com> wrote:
>>>> Paolo Bonzini <pbonzini@redhat.com> 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 <armbru@redhat.com>
>>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>
>>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>>>>
>>>
>>> Reviewed-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
>>
>> Nominating for 2.1, just to be explicit.
>>
>> [...]
>
> Hmm, is trivial out of service?  I remember thinking "Ok, trivial patch,
> qemu-trivial is cc'ed, that'll pick it up, nothing to do for me."

I think it's more likely that Michael went on vacation, from a quick 
look at his posting history.

Paolo