Re: [Qemu-devel] [PATCH V4 3/8] hw/cirrus_vga.c: replace register_ioport*

[Anthony Liguori]

On 04/12/2012 06:45 AM, Julien Grall wrote:
> This patch replaces all register_ioport* with portio_*. It permits to
> use the new Memory stuff like listener.
>
> Signed-off-by: Julien Grall<address@hidden>
> ---
>   hw/cirrus_vga.c |   38 ++++++++++++++++++++++----------------
>   1 files changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index afedaa4..43731ac 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -198,6 +198,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s,
>   typedef struct CirrusVGAState {
>       VGACommonState vga;
>
> +    PortioList cirrus_port_list;
>       MemoryRegion cirrus_linear_io;
>       MemoryRegion cirrus_linear_bitblt_io;
>       MemoryRegion cirrus_mmio_io;
> @@ -2781,8 +2782,23 @@ static const MemoryRegionOps cirrus_linear_io_ops = {
>       },
>   };
>
> +static const MemoryRegionPortio cirrus_portio_list[] = {
> +    { 0x04, 2, 1, .write = cirrus_vga_ioport_write,
> +    .read = cirrus_vga_ioport_read, }, /* 0x3b4 */
> +    { 0x0a, 1, 1, .write = cirrus_vga_ioport_write,
> +    .read = cirrus_vga_ioport_read, }, /* 0x3ba */
> +    { 0x10, 16, 1, .write = cirrus_vga_ioport_write,
> +    .read = cirrus_vga_ioport_read, }, /* 0x3c0 */
> +    { 0x24, 2, 1, .write = cirrus_vga_ioport_write,
> +    .read = cirrus_vga_ioport_read, }, /* 0x3d4 */
> +    { 0x2a, 1, 1, .write = cirrus_vga_ioport_write,
> +    .read = cirrus_vga_ioport_read, }, /* 0x3da */
> +    PORTIO_END_OF_LIST(),
> +};
> +
>   static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci,
> -                               MemoryRegion *system_memory)
> +                               MemoryRegion *system_memory,
> +                               MemoryRegion *system_io)
>   {
>       int i;
>       static int inited;
> @@ -2814,19 +2830,8 @@ static void cirrus_init_common(CirrusVGAState * s, int 
> device_id, int is_pci,
>               s->bustype = CIRRUS_BUSTYPE_ISA;
>       }
>
> -    register_ioport_write(0x3c0, 16, 1, cirrus_vga_ioport_write, s);
> -
> -    register_ioport_write(0x3b4, 2, 1, cirrus_vga_ioport_write, s);
> -    register_ioport_write(0x3d4, 2, 1, cirrus_vga_ioport_write, s);
> -    register_ioport_write(0x3ba, 1, 1, cirrus_vga_ioport_write, s);
> -    register_ioport_write(0x3da, 1, 1, cirrus_vga_ioport_write, s);
> -
> -    register_ioport_read(0x3c0, 16, 1, cirrus_vga_ioport_read, s);
> -
> -    register_ioport_read(0x3b4, 2, 1, cirrus_vga_ioport_read, s);
> -    register_ioport_read(0x3d4, 2, 1, cirrus_vga_ioport_read, s);
> -    register_ioport_read(0x3ba, 1, 1, cirrus_vga_ioport_read, s);
> -    register_ioport_read(0x3da, 1, 1, cirrus_vga_ioport_read, s);
> +    portio_list_init(&s->cirrus_port_list, cirrus_portio_list, s, "cirrus-io");
> +    portio_list_add(&s->cirrus_port_list, system_io, 0x3b0);

I think it's a bit better to just register the full 0x3b0..0x3df.  I think we're 
a tad overspecific in registering specific registers with specific widths.

Regards,

Anthony Liguori

>       vga_common_init(s, VGA_RAM_SIZE);
>       cirrus_init_common(&d->cirrus_vga, CIRRUS_ID_CLGD5430, 0,
> -                       isa_address_space(dev));
> +                       isa_address_space(dev), isa_address_space_io(dev));

ISA only has a single address space so this doesn't make very much sense to me.

Regards,

Anthony Liguori

>       s->ds = graphic_console_init(s->update, s->invalidate,
>                                    s->screen_dump, s->text_update,
>                                    s);
> @@ -2934,7 +2939,8 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
>
>        /* setup VGA */
>        vga_common_init(&s->vga, VGA_RAM_SIZE);
> -     cirrus_init_common(s, device_id, 1, pci_address_space(dev));
> +     cirrus_init_common(s, device_id, 1, pci_address_space(dev),
> +                        pci_address_space_io(dev));
>        s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
>                                         s->vga.screen_dump, s->vga.text_update,
>                                         &s->vga);