[Qemu-devel] [PATCH 00/14] Convert ISA I/O ports to Memory API

[Qemu-devel] [PATCH 00/14] Convert ISA I/O ports to Memory API

[Richard Henderson]

I.e. all users of isa_init_ioport_range and isa_init_ioport.

This implements a suggestion from Avi about having an interface
that makes it easy to register multiple MemoryRegions from a
single data structure.

I chose a concatenated list of MemoryRegionPortio arrays, since it's
easy to write and easy to drop the pieces into MemoryRegionOps.


r~


Richard Henderson (14):
  isa: Tidy support code for isabus_get_fw_dev_path.
  isa: Add isa_register_old_portio_list().
  fdc: Convert to isa_register_old_portio_list.
  gus: Convert to isa_register_old_portio_list.
  m48t59: Convert to isa_register_ioport.
  rtc: Convert to isa_register_ioport.
  ne2000: Convert to isa_register_ioport.
  parallel: Convert to isa_register_old_portio_list.
  sb16: Convert to isa_register_old_portio_list.
  vga: Convert to isa_register_old_portio_list.
  pc: Convert port92 to isa_register_ioport.
  vmport: Convert to isa_register_ioport.
  ide: Convert to isa_register_old_portio_list.
  isa: Remove isa_init_ioport_range and isa_init_ioport.

 hw/fdc.c          |   36 ++++-----------------------
 hw/gus.c          |   43 +++++++++++++++++---------------
 hw/ide/core.c     |   33 +++++++++++++++++--------
 hw/ide/internal.h |    3 +-
 hw/ide/isa.c      |    4 +--
 hw/ide/piix.c     |    7 +++--
 hw/ide/via.c      |    7 +++--
 hw/isa-bus.c      |   69 ++++++++++++++++++++++++++++++++--------------------
 hw/isa.h          |   40 +++++++++++++++++++++++++-----
 hw/m48t59.c       |   15 +++++++++--
 hw/mc146818rtc.c  |   15 +++++++++--
 hw/ne2000-isa.c   |    5 +---
 hw/parallel.c     |   50 +++++++++++++++++++++++--------------
 hw/pc.c           |   16 ++++++++++--
 hw/sb16.c         |   35 ++++++++++++--------------
 hw/vga-isa.c      |   10 -------
 hw/vga.c          |   61 ++++++++++++++++++++++++++--------------------
 hw/vmport.c       |   16 ++++++++++--
 18 files changed, 269 insertions(+), 196 deletions(-)

-- 
1.7.6

[Qemu-devel] [PATCH 01/14] isa: Tidy support code for isabus_get_fw_dev_path.

[Richard Henderson]

The only user of ISADevice.ioports is isabus_get_fw_dev_path, and it
only looks at the first entry of the array.  Which suggests that this
entire array+sort operation can be replaced by a simple minimum.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/isa-bus.c |   25 +++++--------------------
 hw/isa.h     |    5 +----
 2 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 6c15a31..e9c1712 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -83,24 +83,11 @@ void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq)
     dev->nirqs++;
 }
 
-static void isa_init_ioport_one(ISADevice *dev, uint16_t ioport)
-{
-    assert(dev->nioports < ARRAY_SIZE(dev->ioports));
-    dev->ioports[dev->nioports++] = ioport;
-}
-
-static int isa_cmp_ports(const void *p1, const void *p2)
-{
-    return *(uint16_t*)p1 - *(uint16_t*)p2;
-}
-
 void isa_init_ioport_range(ISADevice *dev, uint16_t start, uint16_t length)
 {
-    int i;
-    for (i = start; i < start + length; i++) {
-        isa_init_ioport_one(dev, i);
+    if (dev->ioport_id == 0 || start < dev->ioport_id) {
+        dev->ioport_id = start;
     }
-    qsort(dev->ioports, dev->nioports, sizeof(dev->ioports[0]), isa_cmp_ports);
 }
 
 void isa_init_ioport(ISADevice *dev, uint16_t ioport)
@@ -112,9 +99,7 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
 {
     memory_region_add_subregion(isabus->address_space_io, start, io);
     if (dev != NULL) {
-        assert(dev->nio < ARRAY_SIZE(dev->io));
-        dev->io[dev->nio++] = io;
-        isa_init_ioport_range(dev, start, memory_region_size(io));
+        isa_init_ioport(dev, start);
     }
 }
 
@@ -208,8 +193,8 @@ static char *isabus_get_fw_dev_path(DeviceState *dev)
     int off;
 
     off = snprintf(path, sizeof(path), "%s", qdev_fw_name(dev));
-    if (d->nioports) {
-        snprintf(path + off, sizeof(path) - off, "@%04x", d->ioports[0]);
+    if (d->ioport_id) {
+        snprintf(path + off, sizeof(path) - off, "@%04x", d->ioport_id);
     }
 
     return strdup(path);
diff --git a/hw/isa.h b/hw/isa.h
index 432d17a..c5c2618 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -13,12 +13,9 @@ typedef struct ISADeviceInfo ISADeviceInfo;
 
 struct ISADevice {
     DeviceState qdev;
-    MemoryRegion *io[32];
     uint32_t isairq[2];
-    uint16_t ioports[32];
     int nirqs;
-    int nioports;
-    int nio;
+    int ioport_id;
 };
 
 typedef int (*isa_qdev_initfn)(ISADevice *dev);
-- 
1.7.6

[Qemu-devel] [PATCH 02/14] isa: Add isa_register_old_portio_list().

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/isa-bus.c |   39 +++++++++++++++++++++++++++++++++++++++
 hw/isa.h     |   33 ++++++++++++++++++++++++++++++++-
 2 files changed, 71 insertions(+), 1 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index e9c1712..d8e1880 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -16,6 +16,7 @@
  * You should have received a copy of the GNU Lesser General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
+#include <glib.h>
 #include "hw.h"
 #include "monitor.h"
 #include "sysbus.h"
@@ -103,6 +104,44 @@ void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
     }
 }
 
+void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
+                                  const MemoryRegionPortio *pio_start,
+                                  void *opaque, const char *name)
+{
+    MemoryRegion *io_space = isabus->address_space_io;
+    const MemoryRegionPortio *pio_iter;
+
+    /* 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.  */
+    if (dev) {
+        isa_init_ioport(dev, start);
+    }
+
+    for (; pio_start->size != 0; pio_start = pio_iter + 1) {
+        unsigned int off_low = UINT_MAX, off_high = 0;
+        MemoryRegionOps *ops;
+        MemoryRegion *region;
+
+        for (pio_iter = pio_start; pio_iter->size; ++pio_iter) {
+            if (pio_iter->offset < off_low) {
+                off_low = pio_iter->offset;
+            }
+            if (pio_iter->offset + pio_iter->len > off_high) {
+                off_high = pio_iter->offset + pio_iter->len;
+            }
+        }
+
+        ops = g_new(MemoryRegionOps, 1);
+        ops->old_portio = pio_start;
+
+        region = g_new(MemoryRegion, 1);
+        memory_region_init_io(region, ops, opaque, name, off_high - off_low);
+        memory_region_set_offset(region, start + off_low);
+        memory_region_add_subregion(io_space, start + off_low, region);
+    }
+}
+
 static int isa_qdev_init(DeviceState *qdev, DeviceInfo *base)
 {
     ISADevice *dev = DO_UPCAST(ISADevice, qdev, qdev);
diff --git a/hw/isa.h b/hw/isa.h
index c5c2618..117d8b0 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -28,7 +28,6 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space_io);
 void isa_bus_irqs(qemu_irq *irqs);
 qemu_irq isa_get_irq(int isairq);
 void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
-void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
 void isa_init_ioport(ISADevice *dev, uint16_t ioport);
 void isa_init_ioport_range(ISADevice *dev, uint16_t start, uint16_t length);
 void isa_qdev_register(ISADeviceInfo *info);
@@ -37,6 +36,38 @@ ISADevice *isa_create(const char *name);
 ISADevice *isa_try_create(const char *name);
 ISADevice *isa_create_simple(const char *name);
 
+/**
+ * isa_register_ioport: Install an I/O port region on the ISA bus.
+ *
+ * Register an I/O port region via memory_region_add_subregion
+ * inside the ISA I/O address space.
+ *
+ * @dev: the ISADevice against which these are registered; may be NULL.
+ * @io: the #MemoryRegion being registered.
+ * @start: the base I/O port.
+ */
+void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
+
+/**
+ * isa_register_old_portio_list: Initialize a set of ISA io ports
+ *
+ * Several ISA devices have many dis-joint I/O ports.  Worse, these I/O
+ * ports can be interleaved with I/O ports from other devices.  This
+ * function makes it easy to create multiple MemoryRegions for a single
+ * device and use the legacy portio routines.
+ *
+ * @dev: the ISADevice against which these are registered; may be NULL.
+ * @start: the base I/O port against which the portio->offset is applied.
+ * @old_portio: A concatenation of several #MemoryRegionOps old_portio
+ *   parameters.  The entire list should be terminated by a double
+ *   PORTIO_END_OF_LIST().
+ * @opaque: passed into the old_portio callbacks.
+ * @name: passed into memory_region_init_io.
+ */
+void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
+                                  const MemoryRegionPortio *old_portio,
+                                  void *opaque, const char *name);
+
 extern target_phys_addr_t isa_mem_base;
 
 void isa_mmio_setup(MemoryRegion *mr, target_phys_addr_t size);
-- 
1.7.6

[Qemu-devel] [PATCH 03/14] fdc: Convert to isa_register_old_portio_list.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/fdc.c |   36 ++++++------------------------------
 1 files changed, 6 insertions(+), 30 deletions(-)

diff --git a/hw/fdc.c b/hw/fdc.c
index cba973e..099acf2 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -425,7 +425,6 @@ typedef struct FDCtrlSysBus {
 
 typedef struct FDCtrlISABus {
     ISADevice busdev;
-    MemoryRegion io_0, io_7;
     struct FDCtrl state;
     int32_t bootindexA;
     int32_t bootindexB;
@@ -1882,32 +1881,12 @@ static int fdctrl_init_common(FDCtrl *fdctrl)
     return fdctrl_connect_drives(fdctrl);
 }
 
-static uint32_t fdctrl_read_port_7(void *opaque, uint32_t reg)
-{
-    return fdctrl_read(opaque, reg + 7);
-}
-
-static void fdctrl_write_port_7(void *opaque, uint32_t reg, uint32_t value)
-{
-    fdctrl_write(opaque, reg + 7, value);
-}
-
-static const MemoryRegionPortio fdc_portio_0[] = {
+static const MemoryRegionPortio fdc_portio_list[] = {
     { 1, 5, 1, .read = fdctrl_read, .write = fdctrl_write },
-    PORTIO_END_OF_LIST()
-};
-
-static const MemoryRegionPortio fdc_portio_7[] = {
-    { 0, 1, 1, .read = fdctrl_read_port_7, .write = fdctrl_write_port_7 },
-    PORTIO_END_OF_LIST()
-};
-
-static const MemoryRegionOps fdc_ioport_0_ops = {
-    .old_portio = fdc_portio_0
-};
-
-static const MemoryRegionOps fdc_ioport_7_ops = {
-    .old_portio = fdc_portio_7
+    PORTIO_END_OF_LIST(),
+    { 7, 1, 1, .read = fdctrl_read, .write = fdctrl_write },
+    PORTIO_END_OF_LIST(),
+    PORTIO_END_OF_LIST(),
 };
 
 static int isabus_fdc_init1(ISADevice *dev)
@@ -1919,10 +1898,7 @@ static int isabus_fdc_init1(ISADevice *dev)
     int dma_chann = 2;
     int ret;
 
-    memory_region_init_io(&isa->io_0, &fdc_ioport_0_ops, fdctrl, "fdc", 6);
-    memory_region_init_io(&isa->io_7, &fdc_ioport_7_ops, fdctrl, "fdc", 1);
-    isa_register_ioport(dev, &isa->io_0, iobase);
-    isa_register_ioport(dev, &isa->io_7, iobase + 7);
+    isa_register_old_portio_list(dev, iobase, fdc_portio_list, fdctrl, "fdc");
 
     isa_init_irq(&isa->busdev, &fdctrl->irq, isairq);
     fdctrl->dma_chann = dma_chann;
-- 
1.7.6

[Qemu-devel] [PATCH 04/14] gus: Convert to isa_register_old_portio_list.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
Cc: malc <av1474@comtv.ru>
---
 hw/gus.c |   43 +++++++++++++++++++++++--------------------
 1 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/hw/gus.c b/hw/gus.c
index ff9e7c7..9531d5f 100644
--- a/hw/gus.c
+++ b/hw/gus.c
@@ -232,6 +232,26 @@ static const VMStateDescription vmstate_gus = {
     }
 };
 
+static const MemoryRegionPortio gus_portio_list1[] = {
+    {0x000,  1, 1, .write = gus_writeb },
+    {0x000,  1, 2, .write = gus_writew },
+    PORTIO_END_OF_LIST(),
+    {0x006, 10, 1, .read = gus_readb, .write = gus_writeb },
+    {0x006, 10, 2, .read = gus_readw, .write = gus_writew },
+    PORTIO_END_OF_LIST(),
+    {0x100,  8, 1, .read = gus_readb, .write = gus_writeb },
+    {0x100,  8, 2, .read = gus_readw, .write = gus_writew },
+    PORTIO_END_OF_LIST(),
+    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(),
+    PORTIO_END_OF_LIST(),
+};
+
 static int gus_initfn (ISADevice *dev)
 {
     GUSState *s = DO_UPCAST(GUSState, dev, dev);
@@ -262,26 +282,9 @@ static int gus_initfn (ISADevice *dev)
     s->samples = AUD_get_buffer_size_out (s->voice) >> s->shift;
     s->mixbuf = qemu_mallocz (s->samples << s->shift);
 
-    register_ioport_write (s->port, 1, 1, gus_writeb, s);
-    register_ioport_write (s->port, 1, 2, gus_writew, s);
-    isa_init_ioport_range(dev, s->port, 2);
-
-    register_ioport_read ((s->port + 0x100) & 0xf00, 1, 1, gus_readb, s);
-    register_ioport_read ((s->port + 0x100) & 0xf00, 1, 2, gus_readw, s);
-    isa_init_ioport_range(dev, (s->port + 0x100) & 0xf00, 2);
-
-    register_ioport_write (s->port + 6, 10, 1, gus_writeb, s);
-    register_ioport_write (s->port + 6, 10, 2, gus_writew, s);
-    register_ioport_read (s->port + 6, 10, 1, gus_readb, s);
-    register_ioport_read (s->port + 6, 10, 2, gus_readw, s);
-    isa_init_ioport_range(dev, s->port + 6, 10);
-
-
-    register_ioport_write (s->port + 0x100, 8, 1, gus_writeb, s);
-    register_ioport_write (s->port + 0x100, 8, 2, gus_writew, s);
-    register_ioport_read (s->port + 0x100, 8, 1, gus_readb, s);
-    register_ioport_read (s->port + 0x100, 8, 2, gus_readw, s);
-    isa_init_ioport_range(dev, s->port + 0x100, 8);
+    isa_register_old_portio_list(dev, s->port, gus_portio_list1, s, "gus");
+    isa_register_old_portio_list(dev, (s->port + 0x100) & 0xf00,
+                                 gus_portio_list2, s, "gus");
 
     DMA_register_channel (s->emu.gusdma, GUS_read_DMA, s);
     s->emu.himemaddr = s->himem;
-- 
1.7.6

[Qemu-devel] [PATCH 05/14] m48t59: Convert to isa_register_ioport.

[Richard Henderson]

The sysbus interface is as yet unconverted.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/m48t59.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/m48t59.c b/hw/m48t59.c
index 537c0f7..2520812 100644
--- a/hw/m48t59.c
+++ b/hw/m48t59.c
@@ -72,6 +72,7 @@ struct M48t59State {
 typedef struct M48t59ISAState {
     ISADevice busdev;
     M48t59State state;
+    MemoryRegion io;
 } M48t59ISAState;
 
 typedef struct M48t59SysBusState {
@@ -625,6 +626,15 @@ static void m48t59_reset_sysbus(DeviceState *d)
     m48t59_reset_common(NVRAM);
 }
 
+static const MemoryRegionPortio m48t59_portio[] = {
+    {0, 4, 1, .read = NVRAM_readb, .write = NVRAM_writeb },
+    PORTIO_END_OF_LIST(),
+};
+
+static const MemoryRegionOps m48t59_io_ops = {
+    .old_portio = m48t59_portio,
+};
+
 /* Initialisation routine */
 M48t59State *m48t59_init(qemu_irq IRQ, target_phys_addr_t mem_base,
                          uint32_t io_base, uint16_t size, int type)
@@ -668,10 +678,9 @@ M48t59State *m48t59_init_isa(uint32_t io_base, uint16_t size, int type)
     d = DO_UPCAST(M48t59ISAState, busdev, dev);
     s = &d->state;
 
+    memory_region_init_io(&d->io, &m48t59_io_ops, s, "m48t59", 4);
     if (io_base != 0) {
-        register_ioport_read(io_base, 0x04, 1, NVRAM_readb, s);
-        register_ioport_write(io_base, 0x04, 1, NVRAM_writeb, s);
-        isa_init_ioport_range(dev, io_base, 4);
+        isa_register_ioport(dev, &d->io, io_base);
     }
 
     return s;
-- 
1.7.6

[Qemu-devel] [PATCH 06/14] rtc: Convert to isa_register_ioport.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/mc146818rtc.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index feb3b25..2aaca2f 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -81,6 +81,7 @@
 
 typedef struct RTCState {
     ISADevice dev;
+    MemoryRegion io;
     uint8_t cmos_data[128];
     uint8_t cmos_index;
     struct tm current_tm;
@@ -604,6 +605,15 @@ static void rtc_reset(void *opaque)
 #endif
 }
 
+static const MemoryRegionPortio cmos_portio[] = {
+    {0, 2, 1, .read = cmos_ioport_read, .write = cmos_ioport_write },
+    PORTIO_END_OF_LIST(),
+};
+
+static const MemoryRegionOps cmos_ops = {
+    .old_portio = cmos_portio
+};
+
 static int rtc_initfn(ISADevice *dev)
 {
     RTCState *s = DO_UPCAST(RTCState, dev, dev);
@@ -632,9 +642,8 @@ static int rtc_initfn(ISADevice *dev)
         qemu_get_clock_ns(rtc_clock) + (get_ticks_per_sec() * 99) / 100;
     qemu_mod_timer(s->second_timer2, s->next_second_time);
 
-    register_ioport_write(base, 2, 1, cmos_ioport_write, s);
-    register_ioport_read(base, 2, 1, cmos_ioport_read, s);
-    isa_init_ioport_range(dev, base, 2);
+    memory_region_init_io(&s->io, &cmos_ops, s, "rtc", 2);
+    isa_register_ioport(dev, &s->io, base);
 
     qdev_set_legacy_instance_id(&dev->qdev, base, 2);
     qemu_register_reset(rtc_reset, s);
-- 
1.7.6

[Qemu-devel] [PATCH 07/14] ne2000: Convert to isa_register_ioport.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/ne2000-isa.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
index 756ed5c..11ffee7 100644
--- a/hw/ne2000-isa.c
+++ b/hw/ne2000-isa.c
@@ -68,10 +68,7 @@ static int isa_ne2000_initfn(ISADevice *dev)
     NE2000State *s = &isa->ne2000;
 
     ne2000_setup_io(s, 0x20);
-    isa_init_ioport_range(dev, isa->iobase, 16);
-    isa_init_ioport_range(dev, isa->iobase + 0x10, 2);
-    isa_init_ioport(dev, isa->iobase + 0x1f);
-    memory_region_add_subregion(get_system_io(), isa->iobase, &s->io);
+    isa_register_ioport(dev, &s->io, isa->iobase);
 
     isa_init_irq(dev, &s->irq, isa->isairq);
 
-- 
1.7.6

[Qemu-devel] [PATCH 08/14] parallel: Convert to isa_register_old_portio_list.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/parallel.c |   50 +++++++++++++++++++++++++++++++-------------------
 1 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/hw/parallel.c b/hw/parallel.c
index cc853a5..676defa 100644
--- a/hw/parallel.c
+++ b/hw/parallel.c
@@ -448,6 +448,32 @@ static void parallel_reset(void *opaque)
 
 static const int isa_parallel_io[MAX_PARALLEL_PORTS] = { 0x378, 0x278, 0x3bc };
 
+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 },
+    PORTIO_END_OF_LIST(),
+    { 0x400, 8, 1,
+      .read = parallel_ioport_ecp_read,
+      .write = parallel_ioport_ecp_write },
+    PORTIO_END_OF_LIST(),
+    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(),
+    PORTIO_END_OF_LIST(),
+};
+
 static int parallel_isa_initfn(ISADevice *dev)
 {
     static int index;
@@ -478,25 +504,11 @@ static int parallel_isa_initfn(ISADevice *dev)
         s->status = dummy;
     }
 
-    if (s->hw_driver) {
-        register_ioport_write(base, 8, 1, parallel_ioport_write_hw, s);
-        register_ioport_read(base, 8, 1, parallel_ioport_read_hw, s);
-        isa_init_ioport_range(dev, base, 8);
-
-        register_ioport_write(base+4, 1, 2, parallel_ioport_eppdata_write_hw2, s);
-        register_ioport_read(base+4, 1, 2, parallel_ioport_eppdata_read_hw2, s);
-        register_ioport_write(base+4, 1, 4, parallel_ioport_eppdata_write_hw4, s);
-        register_ioport_read(base+4, 1, 4, parallel_ioport_eppdata_read_hw4, s);
-        isa_init_ioport(dev, base+4);
-        register_ioport_write(base+0x400, 8, 1, parallel_ioport_ecp_write, s);
-        register_ioport_read(base+0x400, 8, 1, parallel_ioport_ecp_read, s);
-        isa_init_ioport_range(dev, base+0x400, 8);
-    }
-    else {
-        register_ioport_write(base, 8, 1, parallel_ioport_write_sw, s);
-        register_ioport_read(base, 8, 1, parallel_ioport_read_sw, s);
-        isa_init_ioport_range(dev, base, 8);
-    }
+    isa_register_old_portio_list(dev, base,
+                                 (s->hw_driver
+                                  ? &isa_parallel_portio_hw_list[0]
+                                  : &isa_parallel_portio_sw_list[0]),
+                                 s, "parallel");
     return 0;
 }
 
-- 
1.7.6

[Qemu-devel] [PATCH 09/14] sb16: Convert to isa_register_old_portio_list.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/sb16.c |   35 ++++++++++++++++-------------------
 1 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/hw/sb16.c b/hw/sb16.c
index a76df1b..2a960b1 100644
--- a/hw/sb16.c
+++ b/hw/sb16.c
@@ -1341,12 +1341,24 @@ static const VMStateDescription vmstate_sb16 = {
     }
 };
 
+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 },
+    PORTIO_END_OF_LIST(),
+    { 10, 1, 1, .read = dsp_read },
+    PORTIO_END_OF_LIST(),
+    { 12, 1, 1, .write = dsp_write },
+    { 12, 4, 1, .read = dsp_read },
+    PORTIO_END_OF_LIST(),
+    PORTIO_END_OF_LIST(),
+};
+
+
 static int sb16_initfn (ISADevice *dev)
 {
-    static const uint8_t dsp_write_ports[] = {0x6, 0xc};
-    static const uint8_t dsp_read_ports[] = {0x6, 0xa, 0xc, 0xd, 0xe, 0xf};
     SB16State *s;
-    int i;
 
     s = DO_UPCAST (SB16State, dev, dev);
 
@@ -1366,22 +1378,7 @@ static int sb16_initfn (ISADevice *dev)
         dolog ("warning: Could not create auxiliary timer\n");
     }
 
-    for (i = 0; i < ARRAY_SIZE (dsp_write_ports); i++) {
-        register_ioport_write (s->port + dsp_write_ports[i], 1, 1, dsp_write, s);
-        isa_init_ioport(dev, s->port + dsp_write_ports[i]);
-    }
-
-    for (i = 0; i < ARRAY_SIZE (dsp_read_ports); i++) {
-        register_ioport_read (s->port + dsp_read_ports[i], 1, 1, dsp_read, s);
-        isa_init_ioport(dev, s->port + dsp_read_ports[i]);
-    }
-
-    register_ioport_write (s->port + 0x4, 1, 1, mixer_write_indexb, s);
-    register_ioport_write (s->port + 0x4, 1, 2, mixer_write_indexw, s);
-    isa_init_ioport(dev, s->port + 0x4);
-    register_ioport_read (s->port + 0x5, 1, 1, mixer_read, s);
-    register_ioport_write (s->port + 0x5, 1, 1, mixer_write_datab, s);
-    isa_init_ioport(dev, s->port + 0x5);
+    isa_register_old_portio_list(dev, s->port, sb16_ioport_list, s, "sb16");
 
     DMA_register_channel (s->hdma, SB_read_DMA, s);
     DMA_register_channel (s->dma, SB_read_DMA, s);
-- 
1.7.6

[Qemu-devel] [PATCH 10/14] vga: Convert to isa_register_old_portio_list.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/vga-isa.c |   10 ---------
 hw/vga.c     |   61 ++++++++++++++++++++++++++++++++-------------------------
 2 files changed, 34 insertions(+), 37 deletions(-)

diff --git a/hw/vga-isa.c b/hw/vga-isa.c
index 0d19901..510ace8 100644
--- a/hw/vga-isa.c
+++ b/hw/vga-isa.c
@@ -54,16 +54,6 @@ static int vga_initfn(ISADevice *dev)
                                         isa_mem_base + 0x000a0000,
                                         vga_io_memory, 1);
     memory_region_set_coalescing(vga_io_memory);
-    isa_init_ioport(dev, 0x3c0);
-    isa_init_ioport(dev, 0x3b4);
-    isa_init_ioport(dev, 0x3ba);
-    isa_init_ioport(dev, 0x3da);
-    isa_init_ioport(dev, 0x3c0);
-#ifdef CONFIG_BOCHS_VBE
-    isa_init_ioport(dev, 0x1ce);
-    isa_init_ioport(dev, 0x1cf);
-    isa_init_ioport(dev, 0x1d0);
-#endif /* CONFIG_BOCHS_VBE */
     s->ds = graphic_console_init(s->update, s->invalidate,
                                  s->screen_dump, s->text_update, s);
 
diff --git a/hw/vga.c b/hw/vga.c
index 8acc545..227cf33 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2198,40 +2198,47 @@ void vga_common_init(VGACommonState *s, int vga_ram_size)
     vga_dirty_log_start(s);
 }
 
-/* used by both ISA and PCI */
+static const MemoryRegionPortio vga_portio_list[] = {
+    { 0x04,  2, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3b4 */
+    PORTIO_END_OF_LIST(),
+    { 0x0a,  1, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3ba */
+    PORTIO_END_OF_LIST(),
+    { 0x10, 16, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3c0 */
+    PORTIO_END_OF_LIST(),
+    { 0x14,  2, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3d4 */
+    PORTIO_END_OF_LIST(),
+    { 0x1a,  1, 1, .read = vga_ioport_read, .write = vga_ioport_write }, /* 3da */
+    PORTIO_END_OF_LIST(),
+    PORTIO_END_OF_LIST(),
+};
+
+#ifdef CONFIG_BOCHS_VBE
+static const MemoryRegionPortio vbe_portio_list[] = {
+# ifdef TARGET_I386
+    { 0, 1, 2, .read = vbe_ioport_read_index, .write = vbe_ioport_write_index },
+    { 1, 1, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
+# else
+    { 0, 2, 2, .read = vbe_ioport_read_index, .write = vbe_ioport_write_index },
+    { 2, 2, 2, .read = vbe_ioport_read_data, .write = vbe_ioport_write_data },
+# endif
+    PORTIO_END_OF_LIST(),
+    PORTIO_END_OF_LIST(),
+};
+#endif /* CONFIG_BOCHS_VBE */
+
+/* Used by both ISA and PCI */
 MemoryRegion *vga_init_io(VGACommonState *s)
 {
     MemoryRegion *vga_mem;
 
-    register_ioport_write(0x3c0, 16, 1, vga_ioport_write, s);
-
-    register_ioport_write(0x3b4, 2, 1, vga_ioport_write, s);
-    register_ioport_write(0x3d4, 2, 1, vga_ioport_write, s);
-    register_ioport_write(0x3ba, 1, 1, vga_ioport_write, s);
-    register_ioport_write(0x3da, 1, 1, vga_ioport_write, s);
-
-    register_ioport_read(0x3c0, 16, 1, vga_ioport_read, s);
-
-    register_ioport_read(0x3b4, 2, 1, vga_ioport_read, s);
-    register_ioport_read(0x3d4, 2, 1, vga_ioport_read, s);
-    register_ioport_read(0x3ba, 1, 1, vga_ioport_read, s);
-    register_ioport_read(0x3da, 1, 1, vga_ioport_read, s);
+    /* The PCI-ISA bridge should have been configured properly such that
+       this works for PCI devices as well.  This only supports one bridge,
+       but "secondary" VGA cards are generally accessed by MMIO only anyway.  */
+    isa_register_old_portio_list(NULL, 0x3b0, vga_portio_list, s, "vga");
 
 #ifdef CONFIG_BOCHS_VBE
-#if defined (TARGET_I386)
-    register_ioport_read(0x1ce, 1, 2, vbe_ioport_read_index, s);
-    register_ioport_read(0x1cf, 1, 2, vbe_ioport_read_data, s);
-
-    register_ioport_write(0x1ce, 1, 2, vbe_ioport_write_index, s);
-    register_ioport_write(0x1cf, 1, 2, vbe_ioport_write_data, s);
-#else
-    register_ioport_read(0x1ce, 1, 2, vbe_ioport_read_index, s);
-    register_ioport_read(0x1d0, 1, 2, vbe_ioport_read_data, s);
-
-    register_ioport_write(0x1ce, 1, 2, vbe_ioport_write_index, s);
-    register_ioport_write(0x1d0, 1, 2, vbe_ioport_write_data, s);
+    isa_register_old_portio_list(NULL, 0x1ce, vbe_portio_list, s, "vbe");
 #endif
-#endif /* CONFIG_BOCHS_VBE */
 
     vga_mem = qemu_malloc(sizeof(*vga_mem));
     memory_region_init_io(vga_mem, &vga_mem_ops, s,
-- 
1.7.6

[Qemu-devel] [PATCH 11/14] pc: Convert port92 to isa_register_ioport.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/pc.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/pc.c b/hw/pc.c
index d752821..cd01b7a 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -428,6 +428,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
 /* port 92 stuff: could be split off */
 typedef struct Port92State {
     ISADevice dev;
+    MemoryRegion io;
     uint8_t outport;
     qemu_irq *a20_out;
 } Port92State;
@@ -479,13 +480,22 @@ static void port92_reset(DeviceState *d)
     s->outport &= ~1;
 }
 
+static const MemoryRegionPortio port92_portio[] = {
+    {0, 1, 1, .read = port92_read, .write = port92_write },
+    PORTIO_END_OF_LIST(),
+};
+
+static const MemoryRegionOps port92_ops = {
+    .old_portio = port92_portio
+};
+
 static int port92_initfn(ISADevice *dev)
 {
     Port92State *s = DO_UPCAST(Port92State, dev, dev);
 
-    register_ioport_read(0x92, 1, 1, port92_read, s);
-    register_ioport_write(0x92, 1, 1, port92_write, s);
-    isa_init_ioport(dev, 0x92);
+    memory_region_init_io(&s->io, &port92_ops, s, "port92", 1);
+    isa_register_ioport(dev, &s->io, 0x92);
+
     s->outport = 0;
     return 0;
 }
-- 
1.7.6

[Qemu-devel] [PATCH 12/14] vmport: Convert to isa_register_ioport.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/vmport.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/hw/vmport.c b/hw/vmport.c
index c8aefaa..b5c6fa1 100644
--- a/hw/vmport.c
+++ b/hw/vmport.c
@@ -38,6 +38,7 @@
 typedef struct _VMPortState
 {
     ISADevice dev;
+    MemoryRegion io;
     IOPortReadFunc *func[VMPORT_ENTRIES];
     void *opaque[VMPORT_ENTRIES];
 } VMPortState;
@@ -120,13 +121,22 @@ void vmmouse_set_data(const uint32_t *data)
     env->regs[R_ESI] = data[4]; env->regs[R_EDI] = data[5];
 }
 
+static const MemoryRegionPortio vmport_portio[] = {
+    {0, 1, 4, .read = vmport_ioport_read, .write = vmport_ioport_write },
+    PORTIO_END_OF_LIST(),
+};
+
+static const MemoryRegionOps vmport_ops = {
+    .old_portio = vmport_portio
+};
+
 static int vmport_initfn(ISADevice *dev)
 {
     VMPortState *s = DO_UPCAST(VMPortState, dev, dev);
 
-    register_ioport_read(0x5658, 1, 4, vmport_ioport_read, s);
-    register_ioport_write(0x5658, 1, 4, vmport_ioport_write, s);
-    isa_init_ioport(dev, 0x5658);
+    memory_region_init_io(&s->io, &vmport_ops, s, "vmport", 1);
+    isa_register_ioport(dev, &s->io, 0x5658);
+
     port_state = s;
     /* Register some generic port commands */
     vmport_register(VMPORT_CMD_GETVERSION, vmport_cmd_get_version, NULL);
-- 
1.7.6

[Qemu-devel] [PATCH 13/14] ide: Convert to isa_register_old_portio_list.

[Richard Henderson]

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/ide/core.c     |   33 ++++++++++++++++++++++-----------
 hw/ide/internal.h |    3 ++-
 hw/ide/isa.c      |    4 +---
 hw/ide/piix.c     |    7 ++++---
 hw/ide/via.c      |    7 ++++---
 5 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d145b19..b19973c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -25,6 +25,7 @@
 #include <hw/hw.h>
 #include <hw/pc.h>
 #include <hw/pci.h>
+#include <hw/isa.h>
 #include "qemu-error.h"
 #include "qemu-timer.h"
 #include "sysemu.h"
@@ -1873,20 +1874,30 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
     bus->dma = &ide_dma_nop;
 }
 
-void ide_init_ioport(IDEBus *bus, int iobase, int iobase2)
+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(),
+    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(),
+    PORTIO_END_OF_LIST(),
+};
+
+void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
 {
-    register_ioport_write(iobase, 8, 1, ide_ioport_write, bus);
-    register_ioport_read(iobase, 8, 1, ide_ioport_read, bus);
+    /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
+       bridge has been setup properly to always register with ISA.  */
+    isa_register_old_portio_list(dev, iobase, ide_portio_list, bus, "ide");
+
     if (iobase2) {
-        register_ioport_read(iobase2, 1, 1, ide_status_read, bus);
-        register_ioport_write(iobase2, 1, 1, ide_cmd_write, bus);
+        isa_register_old_portio_list(dev, iobase2,
+                                     ide_portio2_list, bus, "ide");
     }
-
-    /* data ports */
-    register_ioport_write(iobase, 2, 2, ide_data_writew, bus);
-    register_ioport_read(iobase, 2, 2, ide_data_readw, bus);
-    register_ioport_write(iobase, 4, 4, ide_data_writel, bus);
-    register_ioport_read(iobase, 4, 4, ide_data_readl, bus);
 }
 
 static bool is_identify_set(void *opaque, int version_id)
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 02e805f..978d848 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -7,6 +7,7 @@
  * non-internal declarations are in hw/ide.h
  */
 #include <hw/ide.h>
+#include <hw/isa.h>
 #include "block_int.h"
 #include "iorange.h"
 #include "dma.h"
@@ -588,7 +589,7 @@ int ide_init_drive(IDEState *s, BlockDriverState *bs, IDEDriveKind kind,
 void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
                                     DriveInfo *hd1, qemu_irq irq);
-void ide_init_ioport(IDEBus *bus, int iobase, int iobase2);
+void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
 void ide_dma_cb(void *opaque, int ret);
diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index 4ac7453..f4a86f2 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -67,10 +67,8 @@ static int isa_ide_initfn(ISADevice *dev)
     ISAIDEState *s = DO_UPCAST(ISAIDEState, dev, dev);
 
     ide_bus_new(&s->bus, &s->dev.qdev, 0);
-    ide_init_ioport(&s->bus, s->iobase, s->iobase2);
+    ide_init_ioport(&s->bus, dev, s->iobase, s->iobase2);
     isa_init_irq(dev, &s->irq, s->isairq);
-    isa_init_ioport_range(dev, s->iobase, 8);
-    isa_init_ioport(dev, s->iobase2);
     ide_init2(&s->bus, s->irq);
     vmstate_register(&dev->qdev, 0, &vmstate_ide_isa, s);
     return 0;
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 8525336..4df3c0d 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -123,8 +123,7 @@ static void piix3_reset(void *opaque)
 }
 
 static void pci_piix_init_ports(PCIIDEState *d) {
-    int i;
-    struct {
+    static const struct {
         int iobase;
         int iobase2;
         int isairq;
@@ -132,10 +131,12 @@ static void pci_piix_init_ports(PCIIDEState *d) {
         {0x1f0, 0x3f6, 14},
         {0x170, 0x376, 15},
     };
+    int i;
 
     for (i = 0; i < 2; i++) {
         ide_bus_new(&d->bus[i], &d->dev.qdev, i);
-        ide_init_ioport(&d->bus[i], port_info[i].iobase, port_info[i].iobase2);
+        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
+                        port_info[i].iobase2);
         ide_init2(&d->bus[i], isa_get_irq(port_info[i].isairq));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
diff --git a/hw/ide/via.c b/hw/ide/via.c
index c0b9d43..a2fb995 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -147,8 +147,7 @@ static void via_reset(void *opaque)
 }
 
 static void vt82c686b_init_ports(PCIIDEState *d) {
-    int i;
-    struct {
+    static const struct {
         int iobase;
         int iobase2;
         int isairq;
@@ -156,10 +155,12 @@ static void vt82c686b_init_ports(PCIIDEState *d) {
         {0x1f0, 0x3f6, 14},
         {0x170, 0x376, 15},
     };
+    int i;
 
     for (i = 0; i < 2; i++) {
         ide_bus_new(&d->bus[i], &d->dev.qdev, i);
-        ide_init_ioport(&d->bus[i], port_info[i].iobase, port_info[i].iobase2);
+        ide_init_ioport(&d->bus[i], NULL, port_info[i].iobase,
+                        port_info[i].iobase2);
         ide_init2(&d->bus[i], isa_get_irq(port_info[i].isairq));
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
-- 
1.7.6

[Qemu-devel] [PATCH 14/14] isa: Remove isa_init_ioport_range and isa_init_ioport.

[Richard Henderson]

All users have been converted to either isa_register_ioport
or isa_register_old_portio_list.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 hw/isa-bus.c |   19 +++++--------------
 hw/isa.h     |    2 --
 2 files changed, 5 insertions(+), 16 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index d8e1880..f6ab1eb 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -84,24 +84,17 @@ void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq)
     dev->nirqs++;
 }
 
-void isa_init_ioport_range(ISADevice *dev, uint16_t start, uint16_t length)
+static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
 {
-    if (dev->ioport_id == 0 || start < dev->ioport_id) {
-        dev->ioport_id = start;
+    if (dev && (dev->ioport_id == 0 || ioport < dev->ioport_id)) {
+        dev->ioport_id = ioport;
     }
 }
 
-void isa_init_ioport(ISADevice *dev, uint16_t ioport)
-{
-    isa_init_ioport_range(dev, ioport, 1);
-}
-
 void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
 {
     memory_region_add_subregion(isabus->address_space_io, start, io);
-    if (dev != NULL) {
-        isa_init_ioport(dev, start);
-    }
+    isa_init_ioport(dev, start);
 }
 
 void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
@@ -114,9 +107,7 @@ void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
     /* 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.  */
-    if (dev) {
-        isa_init_ioport(dev, start);
-    }
+    isa_init_ioport(dev, start);
 
     for (; pio_start->size != 0; pio_start = pio_iter + 1) {
         unsigned int off_low = UINT_MAX, off_high = 0;
diff --git a/hw/isa.h b/hw/isa.h
index 117d8b0..49ec181 100644
--- a/hw/isa.h
+++ b/hw/isa.h
@@ -28,8 +28,6 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space_io);
 void isa_bus_irqs(qemu_irq *irqs);
 qemu_irq isa_get_irq(int isairq);
 void isa_init_irq(ISADevice *dev, qemu_irq *p, int isairq);
-void isa_init_ioport(ISADevice *dev, uint16_t ioport);
-void isa_init_ioport_range(ISADevice *dev, uint16_t start, uint16_t length);
 void isa_qdev_register(ISADeviceInfo *info);
 MemoryRegion *isa_address_space(ISADevice *dev);
 ISADevice *isa_create(const char *name);
-- 
1.7.6

Re: [Qemu-devel] [PATCH 02/14] isa: Add isa_register_old_portio_list().

[Avi Kivity]

On 08/16/2011 09:45 AM, Richard Henderson wrote:
>
> +void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
> +                                  const MemoryRegionPortio *pio_start,
> +                                  void *opaque, const char *name)

_old_ implies it's deprecated, please drop.  It's only old if it's in a 
user specified MemoryRegionOps.

> +{
> +    MemoryRegion *io_space = isabus->address_space_io;
> +    const MemoryRegionPortio *pio_iter;
> +
> +    /* 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.  */
> +    if (dev) {
> +        isa_init_ioport(dev, start);
> +    }
> +
> +    for (; pio_start->size != 0; pio_start = pio_iter + 1) {
> +        unsigned int off_low = UINT_MAX, off_high = 0;
> +        MemoryRegionOps *ops;
> +        MemoryRegion *region;
> +
> +        for (pio_iter = pio_start; pio_iter->size; ++pio_iter) {
> +            if (pio_iter->offset<  off_low) {
> +                off_low = pio_iter->offset;
> +            }
> +            if (pio_iter->offset + pio_iter->len>  off_high) {
> +                off_high = pio_iter->offset + pio_iter->len;
> +            }

This is supposed to pick up MRPs that are for the same port address?  If 
so that should be in the loop termination condition.

> +        }
> +
> +        ops = g_new(MemoryRegionOps, 1);


g_new0(), we rely on initialized memory here

> +        ops->old_portio = pio_start;
> +
> +        region = g_new(MemoryRegion, 1);

(but not here)

> +        memory_region_init_io(region, ops, opaque, name, off_high - off_low);
> +        memory_region_set_offset(region, start + off_low);

I think the memory core ignores set_offset() for portio.

> +        memory_region_add_subregion(io_space, start + off_low, region);
> +    }
> +}

> +void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start);
> +
> +/**
> + * isa_register_old_portio_list: Initialize a set of ISA io ports
> + *
> + * Several ISA devices have many dis-joint I/O ports.  Worse, these I/O
> + * ports can be interleaved with I/O ports from other devices.  This
> + * function makes it easy to create multiple MemoryRegions for a single
> + * device and use the legacy portio routines.
> + *
> + * @dev: the ISADevice against which these are registered; may be NULL.
> + * @start: the base I/O port against which the portio->offset is applied.
> + * @old_portio: A concatenation of several #MemoryRegionOps old_portio
> + *   parameters.  The entire list should be terminated by a double
> + *   PORTIO_END_OF_LIST().

double seems harsh.

> + * @opaque: passed into the old_portio callbacks.
> + * @name: passed into memory_region_init_io.
> + */
> +void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
> +                                  const MemoryRegionPortio *old_portio,
> +                                  void *opaque, const char *name);
> +
>   extern target_phys_addr_t isa_mem_base;
>
>   void isa_mmio_setup(MemoryRegion *mr, target_phys_addr_t size);


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 02/14] isa: Add isa_register_old_portio_list().

[Avi Kivity]

On 08/17/2011 06:45 AM, Avi Kivity wrote:
>
>> +{
>> +    MemoryRegion *io_space = isabus->address_space_io;
>> +    const MemoryRegionPortio *pio_iter;
>> +
>> +    /* 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.  */
>> +    if (dev) {
>> +        isa_init_ioport(dev, start);
>> +    }
>> +
>> +    for (; pio_start->size != 0; pio_start = pio_iter + 1) {
>> +        unsigned int off_low = UINT_MAX, off_high = 0;
>> +        MemoryRegionOps *ops;
>> +        MemoryRegion *region;
>> +
>> +        for (pio_iter = pio_start; pio_iter->size; ++pio_iter) {
>> +            if (pio_iter->offset<  off_low) {
>> +                off_low = pio_iter->offset;
>> +            }
>> +            if (pio_iter->offset + pio_iter->len>  off_high) {
>> +                off_high = pio_iter->offset + pio_iter->len;
>> +            }
>
> This is supposed to pick up MRPs that are for the same port address?  
> If so that should be in the loop termination condition.

Oh, after seeing a user I see how it works now.  But can't we derive 
this information instead of forcing the user to specify it?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 11/14] pc: Convert port92 to isa_register_ioport.

[Avi Kivity]

On 08/16/2011 09:45 AM, Richard Henderson wrote:
> +static const MemoryRegionPortio port92_portio[] = {
> +    {0, 1, 1, .read = port92_read, .write = port92_write },

A space character died here.

> +    PORTIO_END_OF_LIST(),
> +};
> +
> +static const MemoryRegionOps port92_ops = {
> +    .old_portio = port92_portio
> +};
> +
>

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 13/14] ide: Convert to isa_register_old_portio_list.

[Avi Kivity]

On 08/16/2011 09:45 AM, Richard Henderson wrote:
> @@ -1873,20 +1874,30 @@ void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
>       bus->dma =&ide_dma_nop;
>   }
>
> -void ide_init_ioport(IDEBus *bus, int iobase, int iobase2)
> +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(),
> +    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(),
> +    PORTIO_END_OF_LIST(),
> +};

Missing spaces.

> +
> +void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int iobase2)
>   {
> -    register_ioport_write(iobase, 8, 1, ide_ioport_write, bus);
> -    register_ioport_read(iobase, 8, 1, ide_ioport_read, bus);
> +    /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
> +       bridge has been setup properly to always register with ISA.  */
> +    isa_register_old_portio_list(dev, iobase, ide_portio_list, bus, "ide");

Which PCI-ISA bridge?  If you're behind a secondary PCI bridge, you've 
now bypassed its filtering.


> +
>       if (iobase2) {
> -        register_ioport_read(iobase2, 1, 1, ide_status_read, bus);
> -        register_ioport_write(iobase2, 1, 1, ide_cmd_write, bus);
> +        isa_register_old_portio_list(dev, iobase2,
> +                                     ide_portio2_list, bus, "ide");
>       }
> -
> -    /* data ports */
> -    register_ioport_write(iobase, 2, 2, ide_data_writew, bus);
> -    register_ioport_read(iobase, 2, 2, ide_data_readw, bus);
> -    register_ioport_write(iobase, 4, 4, ide_data_writel, bus);
> -    register_ioport_read(iobase, 4, 4, ide_data_readl, bus);
>   }
>
>

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 02/14] isa: Add isa_register_old_portio_list().

[Richard Henderson]

On 08/17/2011 06:50 AM, Avi Kivity wrote:
> Oh, after seeing a user I see how it works now. But can't we derive
> this information instead of forcing the user to specify it?

I thought about that, but then when I went to implement I found it
just as easy to have the user specify it.  With the later I get to
re-use the pieces of the read-only memory.


r~

Re: [Qemu-devel] [PATCH 02/14] isa: Add isa_register_old_portio_list().

[Avi Kivity]

On 08/17/2011 07:06 AM, Richard Henderson wrote:
> On 08/17/2011 06:50 AM, Avi Kivity wrote:
> >  Oh, after seeing a user I see how it works now. But can't we derive
> >  this information instead of forcing the user to specify it?
>
> I thought about that, but then when I went to implement I found it
> just as easy to have the user specify it.  With the later I get to
> re-use the pieces of the read-only memory.
>

Yes, question is, will the user get the rules where you need to stick an 
P_E_O_L() (between any contiguous block, yes?).

I guess we can get away with just documenting it.  The list of users is 
unlikely to grow.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 13/14] ide: Convert to isa_register_old_portio_list.

[Richard Henderson]

On 08/17/2011 07:04 AM, Avi Kivity wrote:
> Which PCI-ISA bridge?

The only implied by isa_bus_new.

>  If you're behind a secondary PCI bridge, you've now bypassed its filtering.

... Which we never will be, since the PCI-ISA bridge is not explicitly emulated,
but only implied via the way we mash things together inside QEMU.

Clearly I shouldn't have used the word, as it's confusing.


r~

Re: [Qemu-devel] [PATCH 13/14] ide: Convert to isa_register_old_portio_list.

[Avi Kivity]

On 08/17/2011 07:04 AM, Avi Kivity wrote:
>
>> +
>> +void ide_init_ioport(IDEBus *bus, ISADevice *dev, int iobase, int 
>> iobase2)
>>   {
>> -    register_ioport_write(iobase, 8, 1, ide_ioport_write, bus);
>> -    register_ioport_read(iobase, 8, 1, ide_ioport_read, bus);
>> +    /* ??? Assume only ISA and PCI configurations, and that the PCI-ISA
>> +       bridge has been setup properly to always register with ISA.  */
>> +    isa_register_old_portio_list(dev, iobase, ide_portio_list, bus, 
>> "ide");
>
> Which PCI-ISA bridge?  If you're behind a secondary PCI bridge, you've 
> now bypassed its filtering.
>

Something else - it's okay to leak all this memory for ISA, but not PCI.

We could separate this into a helper that returns an array of 
MemoryRegions.  i_r_o_p_l() could then just use the helper, while 
pci-ide would call the helper directly and then all 
pci_register_legacy_ioport() on each MemoryRegion.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 13/14] ide: Convert to isa_register_old_portio_list.

[Avi Kivity]

On 08/17/2011 07:11 AM, Richard Henderson wrote:
> On 08/17/2011 07:04 AM, Avi Kivity wrote:
> >  Which PCI-ISA bridge?
>
> The only implied by isa_bus_new.
>
> >   If you're behind a secondary PCI bridge, you've now bypassed its filtering.
>
> ... Which we never will be, since the PCI-ISA bridge is not explicitly emulated,
> but only implied via the way we mash things together inside QEMU.

Right.  The patch is broken, but no brokener than what we had before; I 
think we can/should leave any cleanups until later.


> Clearly I shouldn't have used the word, as it's confusing.
>
>

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 02/14] isa: Add isa_register_old_portio_list().

[Blue Swirl]

On Wed, Aug 17, 2011 at 1:45 PM, Avi Kivity <avi@redhat.com> wrote:
> On 08/16/2011 09:45 AM, Richard Henderson wrote:
>>
>> +void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
>> +                                  const MemoryRegionPortio *pio_start,
>> +                                  void *opaque, const char *name)
>
> _old_ implies it's deprecated, please drop.  It's only old if it's in a user
> specified MemoryRegionOps.
>
>> +{
>> +    MemoryRegion *io_space = isabus->address_space_io;
>> +    const MemoryRegionPortio *pio_iter;
>> +
>> +    /* 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.  */
>> +    if (dev) {
>> +        isa_init_ioport(dev, start);
>> +    }
>> +
>> +    for (; pio_start->size != 0; pio_start = pio_iter + 1) {
>> +        unsigned int off_low = UINT_MAX, off_high = 0;
>> +        MemoryRegionOps *ops;
>> +        MemoryRegion *region;
>> +
>> +        for (pio_iter = pio_start; pio_iter->size; ++pio_iter) {
>> +            if (pio_iter->offset<  off_low) {
>> +                off_low = pio_iter->offset;
>> +            }
>> +            if (pio_iter->offset + pio_iter->len>  off_high) {
>> +                off_high = pio_iter->offset + pio_iter->len;
>> +            }
>
> This is supposed to pick up MRPs that are for the same port address?  If so
> that should be in the loop termination condition.
>
>> +        }
>> +
>> +        ops = g_new(MemoryRegionOps, 1);
>
>
> g_new0(), we rely on initialized memory here

Please avoid g_new/g_malloc until they are taught to use qemu_malloc
or was it the other way around.

>> +        ops->old_portio = pio_start;
>> +
>> +        region = g_new(MemoryRegion, 1);
>
> (but not here)
>
>> +        memory_region_init_io(region, ops, opaque, name, off_high -
>> off_low);
>> +        memory_region_set_offset(region, start + off_low);
>
> I think the memory core ignores set_offset() for portio.
>
>> +        memory_region_add_subregion(io_space, start + off_low, region);
>> +    }
>> +}
>
>> +void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t
>> start);
>> +
>> +/**
>> + * isa_register_old_portio_list: Initialize a set of ISA io ports
>> + *
>> + * Several ISA devices have many dis-joint I/O ports.  Worse, these I/O
>> + * ports can be interleaved with I/O ports from other devices.  This
>> + * function makes it easy to create multiple MemoryRegions for a single
>> + * device and use the legacy portio routines.
>> + *
>> + * @dev: the ISADevice against which these are registered; may be NULL.
>> + * @start: the base I/O port against which the portio->offset is applied.
>> + * @old_portio: A concatenation of several #MemoryRegionOps old_portio
>> + *   parameters.  The entire list should be terminated by a double
>> + *   PORTIO_END_OF_LIST().
>
> double seems harsh.
>
>> + * @opaque: passed into the old_portio callbacks.
>> + * @name: passed into memory_region_init_io.
>> + */
>> +void isa_register_old_portio_list(ISADevice *dev, uint16_t start,
>> +                                  const MemoryRegionPortio *old_portio,
>> +                                  void *opaque, const char *name);
>> +
>>  extern target_phys_addr_t isa_mem_base;
>>
>>  void isa_mmio_setup(MemoryRegion *mr, target_phys_addr_t size);
>
>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>
>
>

Re: [Qemu-devel] [PATCH 02/14] isa: Add isa_register_old_portio_list().

[Avi Kivity]

On 08/17/2011 10:23 AM, Blue Swirl wrote:
> >
> >>  +        }
> >>  +
> >>  +        ops = g_new(MemoryRegionOps, 1);
> >
> >
> >  g_new0(), we rely on initialized memory here
>
> Please avoid g_new/g_malloc until they are taught to use qemu_malloc
> or was it the other way around.
>

Why?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 02/14] isa: Add isa_register_old_portio_list().

[Blue Swirl]

On Wed, Aug 17, 2011 at 7:07 PM, Avi Kivity <avi@redhat.com> wrote:
> On 08/17/2011 10:23 AM, Blue Swirl wrote:
>>
>> >
>> >>  +        }
>> >>  +
>> >>  +        ops = g_new(MemoryRegionOps, 1);
>> >
>> >
>> >  g_new0(), we rely on initialized memory here
>>
>> Please avoid g_new/g_malloc until they are taught to use qemu_malloc
>> or was it the other way around.
>>
>
> Why?

http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg02620.html

Re: [Qemu-devel] [PATCH 02/14] isa: Add isa_register_old_portio_list().

[Avi Kivity]

On 08/17/2011 12:32 PM, Blue Swirl wrote:
> >>  Please avoid g_new/g_malloc until they are taught to use qemu_malloc
> >>  or was it the other way around.
> >>
> >
> >  Why?
>
> http://lists.nongnu.org/archive/html/qemu-devel/2011-07/msg02620.html

I don't understand.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 04/14] gus: Convert to isa_register_old_portio_list.

[malc]

On Tue, 16 Aug 2011, Richard Henderson wrote:

> Signed-off-by: Richard Henderson <rth@twiddle.net>
> Cc: malc <av1474@comtv.ru>

This patchset breaks both gus and sb16 for me.

[..snip..]

-- 
mailto:av1474@comtv.ru