Patchwork [13/14] ide: Convert to isa_register_old_portio_list.

login
register
mail settings
Submitter Richard Henderson
Date Aug. 16, 2011, 4:45 p.m.
Message ID <1313513145-5348-14-git-send-email-rth@twiddle.net>
Download mbox | patch
Permalink /patch/110199/
State New
Headers show

Comments

Richard Henderson - Aug. 16, 2011, 4:45 p.m.
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(-)
Avi Kivity - Aug. 17, 2011, 2:04 p.m.
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);
>   }
>
>
Richard Henderson - Aug. 17, 2011, 2:11 p.m.
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~
Avi Kivity - Aug. 17, 2011, 2:13 p.m.
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.
Avi Kivity - Aug. 17, 2011, 2:18 p.m.
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.
>
>

Patch

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