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

[Mark Cave-Ayland]

On 11/05/2023 14:46, 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(-)
> >
> > diff --git a/softmmu/ioport.c b/softmmu/ioport.c
> > index d0d5b0bcaa..238625a36f 100644
> > --- a/softmmu/ioport.c
> > +++ b/softmmu/ioport.c
> > @@ -32,11 +32,16 @@
> >   #include "exec/address-spaces.h"
> >   #include "trace.h"
> > -typedef struct MemoryRegionPortioList {
> > +struct MemoryRegionPortioList {
> > +    Object obj;
> > +
> >       MemoryRegion mr;
> >       void *portio_opaque;
> >       MemoryRegionPortio *ports;
> > -} MemoryRegionPortioList;
> > +};
> > +
> > +#define TYPE_MEMORY_REGION_PORTIO_LIST "memory-region-portio-list"
>
> Possibly simpler as: TYPE_MEMORY_REGION_PORTIO "memory-region-portio"

I'm a little undecided about this one: the ports field contains an array of 
MemoryRegionPortio entries e.g. 
https://gitlab.com/qemu-project/qemu/-/blob/master/hw/ide/ioport.c#L31 so I 
considered that the QOM object contains a list of MemoryRegionPortios. 
TYPE_MEMORY_REGION_PORTIO_LIST feels a better fit here since it reflects this whilst 
also matching the existing MemoryRegionPortioList struct name.

> Otherwise:
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>


ATB,

Mark.