Re: [Qemu-devel] [PATCH 08/10] isa: use memory regions instead of portio_list_* functions

[Andreas Färber]

Am 04.01.2013 22:29, schrieb Hervé Poussineau:
> Signed-off-by: Hervé Poussineau <address@hidden>
> ---
>  hw/isa-bus.c |  127 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/isa.h     |    2 +-
>  2 files changed, 125 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/isa-bus.c b/hw/isa-bus.c
> index 86b0bbd..bcf7cd4 100644
> --- a/hw/isa-bus.c
> +++ b/hw/isa-bus.c
> @@ -104,19 +104,140 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion 
> *io, uint16_t start)
>      isa_init_ioport(dev, start);
>  }
>  
> +typedef struct PortioState {
> +    const char *name; /* debug purposes */
> +    uint16_t start;
> +    uint16_t offset;
> +    const MemoryRegionPortio *pio_start;
> +    void *opaque;
> +} PortioState;
> +
> +static const MemoryRegionPortio *portio_find(const MemoryRegionPortio *mrp,
> +                                             uint64_t offset,
> +                                             unsigned int width, bool write,
> +                                             bool smaller)
> +{
> +    for (; mrp->size; ++mrp) {
> +        if (offset >= mrp->offset && offset < mrp->offset + mrp->len
> +            && (width == mrp->size || (smaller && width < mrp->size))
> +            && (write ? (bool)mrp->write : (bool)mrp->read)) {
> +            return mrp;
> +        }
> +    }
> +    return NULL;
> +}
> +
> +static uint64_t portio_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    const PortioState *s = opaque;
> +    const MemoryRegionPortio *mrp;
> +
> +    addr += s->offset;
> +    mrp = portio_find(s->pio_start, addr, size, false, false);
> +    if (mrp) {
> +        return mrp->read(s->opaque, s->start + addr);
> +    } else if (size == 2) {
> +        uint64_t data;
> +        mrp = portio_find(s->pio_start, addr, 1, false, false);
> +        assert(mrp);
> +        data = mrp->read(s->opaque, s->start + addr) |
> +              (mrp->read(s->opaque, s->start + addr + 1) << 8);
> +        return data;
> +    }
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid read from 0x%x size=%d",
> +                  s->name, s->start + (int)addr, size);

I note that patch 10/10 shows that memory.c does it similarly, assuming
Little Endian for size == 2 and ignoring assembled size == 4.

> +    return -1U;
> +}
> +
> +static void portio_write(void *opaque, hwaddr addr, uint64_t data,
> +                         unsigned int size)
> +{
> +    const PortioState *s = opaque;
> +    const MemoryRegionPortio *mrp;
> +
> +    addr += s->offset;
> +    mrp = portio_find(s->pio_start, addr, size, true, false);
> +    if (mrp) {
> +        mrp->write(s->opaque, s->start + addr, data);
> +        return;
> +    } else if (size == 2) {
> +        mrp = portio_find(s->pio_start, addr, 1, true, false);
> +        assert(mrp);
> +        mrp->write(s->opaque, s->start + addr, data & 0xff);
> +        mrp->write(s->opaque, s->start + addr + 1, data >> 8);
> +        return;
> +    }
> +    qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid write to 0x%x size=%d",
> +                  s->name, s->start + (int)addr, size);

Ditto.

> +}
> +
> +static bool portio_accepts(void *opaque, hwaddr addr, unsigned int size,
> +                           bool is_write)
> +{
> +    const PortioState *s = opaque;
> +    const MemoryRegionPortio *mrp;
> +
> +    addr += s->offset;
> +    mrp = portio_find(s->pio_start, addr, size, is_write, true);
> +    return (mrp != NULL);
> +}
> +
> +const MemoryRegionOps portio_ops = {

static const?

> +    .read = portio_read,
> +    .write = portio_write,
> +    .valid.accepts = portio_accepts,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
> +static void isa_register_portio_list_1(ISADevice *dev, uint16_t start,
> +                                       uint16_t offset, uint16_t end,
> +                                       const MemoryRegionPortio *pio_start,
> +                                       void *opaque, const char *name)
> +{
> +    MemoryRegion *mr = g_new(MemoryRegion, 1);
> +    PortioState *s = g_new(PortioState, 1);
> +
> +    s->name = name;
> +    s->start = start;
> +    s->offset = offset;
> +    s->pio_start = pio_start;
> +    s->opaque = opaque;
> +    memory_region_init_io(mr, &portio_ops, s, name, end - offset);
> +    memory_region_add_subregion(isa_address_space_io(dev),
> +                                start + offset, mr);
> +}
> +
>  void isa_register_portio_list(ISADevice *dev, uint16_t start,
>                                const MemoryRegionPortio *pio_start,
>                                void *opaque, const char *name)
>  {
> -    PortioList *piolist = g_new(PortioList, 1);
> +    const MemoryRegionPortio *pio, *first;
> +    uint16_t end;
>  
>      /* START is how we should treat DEV, regardless of the actual
>         contents of the portio array.  This is how the old code
>         actually handled e.g. the FDC device.  */
>      isa_init_ioport(dev, start);
>  
> -    portio_list_init(piolist, pio_start, opaque, name);
> -    portio_list_add(piolist, isabus->address_space_io, start);
> +    assert(pio_start->size);
> +
> +    first = pio_start;
> +    end = 0;
> +    for (pio = pio_start; pio->size; pio++) {
> +        assert(pio->offset >= first->offset);
> +        if (pio->offset > first->offset + first->len) {
> +            isa_register_portio_list_1(dev, start, first->offset, end,
> +                                       pio_start, opaque, name);
> +            first = pio;
> +            end = 0;
> +        }
> +        if (pio->offset + pio->len > end) {
> +            end = pio->offset + pio->len;
> +        }
> +    }
> +
> +    isa_register_portio_list_1(dev, start, first->offset, end,
> +                               pio_start, opaque, name);
>  }
>  
>  static int isa_qdev_init(DeviceState *qdev)
> diff --git a/hw/isa.h b/hw/isa.h
> index 62e89d3..c3b01ea 100644
> --- a/hw/isa.h
> +++ b/hw/isa.h
> @@ -73,7 +73,7 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, 
> uint16_t start);
>   * @dev: the ISADevice against which these are registered; may be NULL.
>   * @start: the base I/O port against which the portio->offset is applied.
>   * @portio: the ports, sorted by offset.
> - * @opaque: passed into the old_portio callbacks.
> + * @opaque: passed into the portio callbacks.
>   * @name: passed into memory_region_init_io.
>   */
>  void isa_register_portio_list(ISADevice *dev, uint16_t start,

Otherwise looks okay to me, but I'd appreciate another pair of eyes.

AFAIU this moves the current implementation from ioport.c (09/10) and
memory.c (10/10) into isa-bus.c, to remove MemoryRegionOps::old_portio
in 10/10. But ISA keeps using MemoryRegionPortio unlike memory.c itself.

I wonder if it were not a cleaner course of action to remove
isa_register_portio_list() completely by converting callers to use
isa_register_ioport() with a MemoryRegion owned by the caller?
Remaining MemoryRegionPortio lists after this series below.

Regards,
Andreas


dma.c:
/* IOport from page_base */
static const MemoryRegionPortio page_portio_list[] = {
    { 0x01, 3, 1, .write = write_page, .read = read_page, },
    { 0x07, 1, 1, .write = write_page, .read = read_page, },
    PORTIO_END_OF_LIST(),
};

/* IOport from pageh_base */
static const MemoryRegionPortio pageh_portio_list[] = {
    { 0x01, 3, 1, .write = write_pageh, .read = read_pageh, },
    { 0x07, 3, 1, .write = write_pageh, .read = read_pageh, },
    PORTIO_END_OF_LIST(),
};

fdc.c:
static const MemoryRegionPortio fdc_portio_list[] = {
    { 1, 5, 1, .read = fdctrl_read, .write = fdctrl_write },
    { 7, 1, 1, .read = fdctrl_read, .write = fdctrl_write },
    PORTIO_END_OF_LIST(),
};

gus.c:
static const MemoryRegionPortio gus_portio_list1[] = {
    {0x000,  1, 1, .write = gus_writeb },
    {0x000,  1, 2, .write = gus_writew },
    {0x006, 10, 1, .read = gus_readb, .write = gus_writeb },
    {0x006, 10, 2, .read = gus_readw, .write = gus_writew },
    {0x100,  8, 1, .read = gus_readb, .write = gus_writeb },
    {0x100,  8, 2, .read = gus_readw, .write = gus_writew },
    PORTIO_END_OF_LIST (),
};

static const MemoryRegionPortio gus_portio_list2[] = {
    {0, 1, 1, .read = gus_readb },
    {0, 1, 2, .read = gus_readw },
    PORTIO_END_OF_LIST (),
};

ide/core.c:
static const MemoryRegionPortio ide_portio_list[] = {
    { 0, 8, 1, .read = ide_ioport_read, .write = ide_ioport_write },
    { 0, 2, 2, .read = ide_data_readw, .write = ide_data_writew },
    { 0, 4, 4, .read = ide_data_readl, .write = ide_data_writel },
    PORTIO_END_OF_LIST(),
};

static const MemoryRegionPortio ide_portio2_list[] = {
    { 0, 1, 1, .read = ide_status_read, .write = ide_cmd_write },
    PORTIO_END_OF_LIST(),
};

parallel.c:
static const MemoryRegionPortio isa_parallel_portio_hw_list[] = {
    { 0, 8, 1,
      .read = parallel_ioport_read_hw,
      .write = parallel_ioport_write_hw },
    { 4, 1, 2,
      .read = parallel_ioport_eppdata_read_hw2,
      .write = parallel_ioport_eppdata_write_hw2 },
    { 4, 1, 4,
      .read = parallel_ioport_eppdata_read_hw4,
      .write = parallel_ioport_eppdata_write_hw4 },
    { 0x400, 8, 1,
      .read = parallel_ioport_ecp_read,
      .write = parallel_ioport_ecp_write },
    PORTIO_END_OF_LIST(),
};

static const MemoryRegionPortio isa_parallel_portio_sw_list[] = {
    { 0, 8, 1,
      .read = parallel_ioport_read_sw,
      .write = parallel_ioport_write_sw },
    PORTIO_END_OF_LIST(),
};

qxl.c:
static const MemoryRegionPortio qxl_vga_portio_list[] = {
    { 0x04,  2, 1, .read  = vga_ioport_read,
                   .write = qxl_vga_ioport_write }, /* 3b4 */
    { 0x0a,  1, 1, .read  = vga_ioport_read,
                   .write = qxl_vga_ioport_write }, /* 3ba */
    { 0x10, 16, 1, .read  = vga_ioport_read,
                   .write = qxl_vga_ioport_write }, /* 3c0 */
    { 0x24,  2, 1, .read  = vga_ioport_read,
                   .write = qxl_vga_ioport_write }, /* 3d4 */
    { 0x2a,  1, 1, .read  = vga_ioport_read,
                   .write = qxl_vga_ioport_write }, /* 3da */
    PORTIO_END_OF_LIST(),
};

sb16.c:
static const MemoryRegionPortio sb16_ioport_list[] = {
    {  4, 1, 1, .write = mixer_write_indexb },
    {  4, 1, 2, .write = mixer_write_indexw },
    {  5, 1, 1, .read = mixer_read, .write = mixer_write_datab },
    {  6, 1, 1, .read = dsp_read, .write = dsp_write },
    { 10, 1, 1, .read = dsp_read },
    { 12, 1, 1, .write = dsp_write },
    { 12, 4, 1, .read = dsp_read },
    PORTIO_END_OF_LIST (),
};

vga.c:
static const MemoryRegionPortio vga_portio_list[] = {
    { 0x04,  2, 1, .read = vga_ioport_read, .write = vga_ioport_write },
/* 3b4 */
    { 0x0a,  1, 1, .read = vga_ioport_read, .write = vga_ioport_write },
/* 3ba */
    { 0x10, 16, 1, .read = vga_ioport_read, .write = vga_ioport_write },
/* 3c0 */
    { 0x24,  2, 1, .read = vga_ioport_read, .write = vga_ioport_write },
/* 3d4 */
    { 0x2a,  1, 1, .read = vga_ioport_read, .write = vga_ioport_write },
/* 3da */
    PORTIO_END_OF_LIST(),
};

static const MemoryRegionPortio vbe_portio_list[] = {
    { 0, 1, 2, .read = vbe_ioport_read_index, .write =
vbe_ioport_write_index },
# ifdef TARGET_I386
    { 1, 1, 2, .read = vbe_ioport_read_data, .write =
vbe_ioport_write_data },
# endif
    { 2, 1, 2, .read = vbe_ioport_read_data, .write =
vbe_ioport_write_data },
    PORTIO_END_OF_LIST(),
};

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg