Re: [PATCH v3 13/18] exec/ioport: Factor portio_list_register_flush_coalesced() out

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 02/03/2023 22:40, Philippe Mathieu-Daudé wrote:

> We always follow the same pattern when registering
> coalesced portio:
> 
>    - portio_list_init()
>    - portio_list_set_flush_coalesced()
>    - portio_list_add()
> 
> Factor these 3 operations in a single helper named
> portio_list_register_flush_coalesced().
> 
> Drop portio_list_set_flush_coalesced() which is now
> inlined.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Message-Id: <20230207234615.77300-2-philmd@linaro.org>
> ---
>   hw/display/qxl.c      |  7 +++----
>   hw/display/vga.c      |  5 ++---
>   include/exec/ioport.h |  5 ++++-
>   softmmu/ioport.c      | 27 ++++++++++++++++++++++-----
>   4 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index ec712d3ca2..2ecaa0643f 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -2224,10 +2224,9 @@ static void qxl_realize_primary(PCIDevice *dev, Error **errp)
>       }
>       vga_init(vga, OBJECT(dev),
>                pci_address_space(dev), pci_address_space_io(dev), false);
> -    portio_list_init(&qxl->vga_port_list, OBJECT(dev), qxl_vga_portio_list,
> -                     vga, "vga");
> -    portio_list_set_flush_coalesced(&qxl->vga_port_list);
> -    portio_list_add(&qxl->vga_port_list, pci_address_space_io(dev), 0x3b0);
> +    portio_list_register_flush_coalesced(&qxl->vga_port_list, OBJECT(dev),
> +                                         qxl_vga_portio_list, vga, "vga",
> +                                         pci_address_space_io(dev), 0x3b0);
>       qxl->have_vga = true;
>   
>       vga->con = graphic_console_init(DEVICE(dev), 0, &qxl_ops, qxl);
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 7a5fdff649..98d644922e 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -2309,9 +2309,8 @@ void vga_init(VGACommonState *s, Object *obj, MemoryRegion *address_space,
>                                           1);
>       memory_region_set_coalescing(vga_io_memory);
>       if (init_vga_ports) {
> -        portio_list_init(&s->vga_port_list, obj, vga_ports, s, "vga");
> -        portio_list_set_flush_coalesced(&s->vga_port_list);
> -        portio_list_add(&s->vga_port_list, address_space_io, 0x3b0);
> +        portio_list_register_flush_coalesced(&s->vga_port_list, obj, vga_ports,
> +                                             s, "vga", address_space_io, 0x3b0);
>       }
>       if (vbe_ports) {
>           portio_list_init(&s->vbe_port_list, obj, vbe_ports, s, "vbe");
> diff --git a/include/exec/ioport.h b/include/exec/ioport.h
> index e34f668998..eb9882a3ee 100644
> --- a/include/exec/ioport.h
> +++ b/include/exec/ioport.h
> @@ -65,7 +65,10 @@ typedef struct PortioList {
>   void portio_list_init(PortioList *piolist, Object *owner,
>                         const struct MemoryRegionPortio *callbacks,
>                         void *opaque, const char *name);
> -void portio_list_set_flush_coalesced(PortioList *piolist);
> +void portio_list_register_flush_coalesced(PortioList *piolist, Object *owner,
> +                                          const MemoryRegionPortio *callbacks,
> +                                          void *opaque, const char *name,
> +                                          MemoryRegion *mr, uint32_t offset);
>   void portio_list_destroy(PortioList *piolist);
>   void portio_list_add(PortioList *piolist,
>                        struct MemoryRegion *address_space,
> diff --git a/softmmu/ioport.c b/softmmu/ioport.c
> index cb8adb0b93..be0c920c5c 100644
> --- a/softmmu/ioport.c
> +++ b/softmmu/ioport.c
> @@ -124,6 +124,7 @@ void portio_list_init(PortioList *piolist,
>           ++n;
>       }
>   
> +    assert(owner);
>       piolist->ports = callbacks;
>       piolist->nr = 0;
>       piolist->regions = g_new0(MemoryRegion *, n);
> @@ -134,11 +135,6 @@ void portio_list_init(PortioList *piolist,
>       piolist->flush_coalesced_mmio = false;
>   }
>   
> -void portio_list_set_flush_coalesced(PortioList *piolist)
> -{
> -    piolist->flush_coalesced_mmio = true;
> -}
> -
>   void portio_list_destroy(PortioList *piolist)
>   {
>       MemoryRegionPortioList *mrpio;
> @@ -297,3 +293,24 @@ void portio_list_del(PortioList *piolist)
>           memory_region_del_subregion(piolist->address_space, &mrpio->mr);
>       }
>   }
> +
> +static void do_portio_list_register(PortioList *piolist, Object *owner,
> +                                    const MemoryRegionPortio *callbacks,
> +                                    void *opaque, const char *name,
> +                                    MemoryRegion *mr, uint32_t offset,
> +                                    bool flush_coalesced_mmio)
> +{
> +    assert(piolist && !piolist->owner);
> +    portio_list_init(piolist, owner, callbacks, opaque, name);
> +    piolist->flush_coalesced_mmio = flush_coalesced_mmio;
> +    portio_list_add(piolist, mr, offset);
> +}
> +
> +void portio_list_register_flush_coalesced(PortioList *piolist, Object *owner,
> +                                          const MemoryRegionPortio *callbacks,
> +                                          void *opaque, const char *name,
> +                                          MemoryRegion *mr, uint32_t offset)
> +{
> +    do_portio_list_register(piolist, owner, callbacks,
> +                            opaque, name, mr, offset, true);
> +}

Would it be better to have a portio_list_register() that calls portio_list_init() 
followed by portio_list_add() and still keep portio_list_set_flush_coalesced() 
separate? Then it seems that other portio users can benefit in a slight reduction in 
line count given that there only appear to be 2 users of 
portio_list_set_flush_coalesced().


ATB,

Mark.