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"
+OBJECT_DECLARE_SIMPLE_TYPE(MemoryRegionPortioList, MEMORY_REGION_PORTIO_LIST)
 
 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));
     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);
     }
 }
+
+static const TypeInfo memory_region_portio_list_info = {
+    .parent             = TYPE_OBJECT,
+    .name               = TYPE_MEMORY_REGION_PORTIO_LIST,
+    .instance_size      = sizeof(MemoryRegionPortioList),
+};
+
+static void ioport_register_types(void)
+{
+    type_register_static(&memory_region_portio_list_info);
+}
+
+type_init(ioport_register_types)
-- 
2.30.2

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()?

>       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);
>       }
>   }

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.

On 5/11/23 16:52, Mark Cave-Ayland wrote:
>>>
>>>       /* 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.

Here I agree with Philippe though, there is a mismatch between new and 
unref that would (for example) cause the finalize method not to be called.

I think this patch should already introduce the instance_finalize 
function that only does "g_free(mrpio->ports);", and include the full 
portio_list_destroy() hunk from patch 3.

Then patch 3 basically focuses on the reparenting trick (including 
adding the object_unref() call in memory_region_portio_list_finalize).

I can do the change myself when applying though.

Paolo

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"

Otherwise:

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

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.

On 5/11/23 16:43, Mark Cave-Ayland wrote:
>>>
>>> +#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.

I agree, using memory-region-portio would be confusing given the name of 
the struct.

Paolo

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.

In all uses this opaque pointer is a Object*. Almost all cases are
a DeviceState* and one is a BusState* (IDEBus).

Could this opaque become 'Object *owner' (simplifying the next patch)?

> 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;
> +};

On 20/04/2023 09:41, 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.
> 
> In all uses this opaque pointer is a Object*. Almost all cases are
> a DeviceState* and one is a BusState* (IDEBus).
> 
> Could this opaque become 'Object *owner' (simplifying the next patch)?

I'm not sure that it does, since the opaque is part of the portio API and not related 
to this series which is focused on MemoryRegionPortioList i.e. the glue interface 
between the portio and memory APIs. Otherwise you end up changing the portio API and 
the associated callbacks which is orthogonal to the bug this series is trying to fix.

>> 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;
>> +};


ATB,

Mark.