Re: [PATCH 2/3] softmmu/ioport.c: QOMify MemoryRegionPortioList

[Mark Cave-Ayland]

On 11/05/2023 14:50, Philippe Mathieu-Daudé wrote:

> On 19/4/23 17:16, Mark Cave-Ayland wrote:
> > The aim of QOMification is so that the lifetime of the MemoryRegionPortioList
> > structure can be managed using QOM's in-built refcounting instead of having to
> > handle this manually.
> >
> > Due to the use of an opaque pointer it isn't possible to model the new
> > TYPE_MEMORY_REGION_PORTIO_LIST directly using QOM properties, however since
> > use of the new object is restricted to the portio API we can simply set the
> > opaque pointer (and the heap-allocated port list) internally.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > ---
> >   softmmu/ioport.c | 25 ++++++++++++++++++++++---
> >   1 file changed, 22 insertions(+), 3 deletions(-)
>
>
> >   static uint64_t unassigned_io_read(void *opaque, hwaddr addr, unsigned size)
> >   {
> > @@ -228,7 +233,8 @@ static void portio_list_add_1(PortioList *piolist,
> >       unsigned i;
> >       /* Copy the sub-list and null-terminate it.  */
> > -    mrpio = g_malloc0(sizeof(MemoryRegionPortioList));
> > +    mrpio = MEMORY_REGION_PORTIO_LIST(
> > +                object_new(TYPE_MEMORY_REGION_PORTIO_LIST));
>
> Shouldn't we need to replace the g_free() call by object_unref()
> in portio_list_destroy()?

Both the existing g_free() call and replacing it with object_unref() cause QEMU to 
segfault if you call portio_list_destroy() before the final patch in this series. So 
in effect we'd end up causing more code churn just to replace one crash with another ;)

The real fix is in patch 3 which alters the refcounting/ownership in order to solve 
the underlying issue, which I hope I have described in enough detail in that commit 
message.

> >       mrpio->portio_opaque = piolist->opaque;
> >       mrpio->ports = g_malloc0(sizeof(MemoryRegionPortio) * (count + 1));
> >       memcpy(mrpio->ports, pio_init, sizeof(MemoryRegionPortio) * count);
> > @@ -298,3 +304,16 @@ void portio_list_del(PortioList *piolist)
> >           memory_region_del_subregion(piolist->address_space, &mrpio->mr);
> >       }
> >   }


ATB,

Mark.