[Qemu-devel] [PATCH] ide: Register vm change state handler once only

[Qemu-devel] [PATCH] ide: Register vm change state handler once only

[Stefan Hajnoczi]

We register the vm change state handler in a PCI BAR map() function.
This function can be called multiple times throughout the lifetime of a
PCI IDE device.  This results in duplicate vm change state handlers
being register, none of which are ever unregistered.

Instead, register the vm change state handler in the device's init
function once and for all.

piix tested, cmd646 and via not tested.

Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
 hw/ide/cmd646.c |   16 +++++++++-------
 hw/ide/piix.c   |   32 +++++++++++++++++++++++---------
 hw/ide/via.c    |   32 +++++++++++++++++++++++---------
 3 files changed, 55 insertions(+), 25 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index dfe6091..23fc6e1 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -167,9 +167,6 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
 
     for(i = 0;i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
-        d->bus[i].bmdma = bm;
-        bm->bus = d->bus+i;
-        qemu_add_vm_change_state_handler(ide_dma_restart_cb, bm);
 
         if (i == 0) {
             register_ioport_write(addr, 4, 1, bmdma_writeb_0, d);
@@ -228,6 +225,7 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
     PCIIDEState *d = DO_UPCAST(PCIIDEState, dev, dev);
     uint8_t *pci_conf = d->dev.config;
     qemu_irq *irq;
+    int i;
 
     pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_CMD);
     pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_CMD_646);
@@ -253,10 +251,14 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
     pci_conf[PCI_INTERRUPT_PIN] = 0x01; // interrupt on pin 1
 
     irq = qemu_allocate_irqs(cmd646_set_irq, d, 2);
-    ide_bus_new(&d->bus[0], &d->dev.qdev);
-    ide_bus_new(&d->bus[1], &d->dev.qdev);
-    ide_init2(&d->bus[0], irq[0]);
-    ide_init2(&d->bus[1], irq[1]);
+    for (i = 0; i < 2; i++) {
+        ide_bus_new(&d->bus[i], &d->dev.qdev);
+        ide_init2(&d->bus[i], irq[i]);
+
+        d->bus[i].bmdma = &d->bmdma[i];
+        bm->bus = &d->bus[i];
+        qemu_add_vm_change_state_handler(ide_dma_restart_cb, &d->bmdma[i]);
+    }
 
     vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
     qemu_register_reset(cmd646_reset, d);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index e02b89a..f2752e7 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -76,9 +76,6 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
 
     for(i = 0;i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
-        d->bus[i].bmdma = bm;
-        bm->bus = d->bus+i;
-        qemu_add_vm_change_state_handler(ide_dma_restart_cb, bm);
 
         register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);
 
@@ -112,6 +109,28 @@ static void piix3_reset(void *opaque)
     pci_conf[0x20] = 0x01; /* BMIBA: 20-23h */
 }
 
+static void pci_piix_init_ports(PCIIDEState *d) {
+    int i;
+    struct {
+        int iobase;
+        int iobase2;
+        int isairq;
+    } port_info[] = {
+        {0x1f0, 0x3f6, 14},
+        {0x170, 0x376, 15},
+    };
+
+    for (i = 0; i < 2; i++) {
+        ide_bus_new(&d->bus[i], &d->dev.qdev);
+        ide_init_ioport(&d->bus[i], port_info[i].iobase, port_info[i].iobase2);
+        ide_init2(&d->bus[i], isa_reserve_irq(port_info[i].isairq));
+
+        d->bus[i].bmdma = &d->bmdma[i];
+        d->bmdma[i].bus = &d->bus[i];
+        qemu_add_vm_change_state_handler(ide_dma_restart_cb, &d->bmdma[i]);
+    }
+}
+
 static int pci_piix_ide_initfn(PCIIDEState *d)
 {
     uint8_t *pci_conf = d->dev.config;
@@ -125,13 +144,8 @@ static int pci_piix_ide_initfn(PCIIDEState *d)
 
     vmstate_register(&d->dev.qdev, 0, &vmstate_ide_pci, d);
 
-    ide_bus_new(&d->bus[0], &d->dev.qdev);
-    ide_bus_new(&d->bus[1], &d->dev.qdev);
-    ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
-    ide_init_ioport(&d->bus[1], 0x170, 0x376);
+    pci_piix_init_ports(d);
 
-    ide_init2(&d->bus[0], isa_reserve_irq(14));
-    ide_init2(&d->bus[1], isa_reserve_irq(15));
     return 0;
 }
 
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 66be0c4..839c571 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -78,9 +78,6 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
 
     for(i = 0;i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
-        d->bus[i].bmdma = bm;
-        bm->bus = d->bus+i;
-        qemu_add_vm_change_state_handler(ide_dma_restart_cb, bm);
 
         register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);
 
@@ -135,6 +132,28 @@ static void via_reset(void *opaque)
     pci_set_long(pci_conf + 0xc0, 0x00020001);
 }
 
+static void vt82c686b_init_ports(PCIIDEState *d) {
+    int i;
+    struct {
+        int iobase;
+        int iobase2;
+        int isairq;
+    } port_info[] = {
+        {0x1f0, 0x3f6, 14},
+        {0x170, 0x376, 15},
+    };
+
+    for (i = 0; i < 2; i++) {
+        ide_bus_new(&d->bus[i], &d->dev.qdev);
+        ide_init2(&d->bus[i], isa_reserve_irq(port_info[i].isairq));
+        ide_init_ioport(&d->bus[i], port_info[i].iobase, port_info[i].iobase2);
+
+        d->bus[i].bmdma = &d->bmdma[i];
+        d->bmdma[i].bus = &d->bus[i];
+        qemu_add_vm_change_state_handler(ide_dma_restart_cb, &d->bmdma[i]);
+    }
+}
+
 /* via ide func */
 static int vt82c686b_ide_initfn(PCIDevice *dev)
 {
@@ -154,12 +173,7 @@ static int vt82c686b_ide_initfn(PCIDevice *dev)
 
     vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
 
-    ide_bus_new(&d->bus[0], &d->dev.qdev);
-    ide_bus_new(&d->bus[1], &d->dev.qdev);
-    ide_init2(&d->bus[0], isa_reserve_irq(14));
-    ide_init2(&d->bus[1], isa_reserve_irq(15));
-    ide_init_ioport(&d->bus[0], 0x1f0, 0x3f6);
-    ide_init_ioport(&d->bus[1], 0x170, 0x376);
+    vt82c686b_init_ports(d);
 
     return 0;
 }
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH] ide: Register vm change state handler once only

[Kevin Wolf]

Am 10.12.2010 15:47, schrieb Stefan Hajnoczi:
> We register the vm change state handler in a PCI BAR map() function.
> This function can be called multiple times throughout the lifetime of a
> PCI IDE device.  This results in duplicate vm change state handlers
> being register, none of which are ever unregistered.
> 
> Instead, register the vm change state handler in the device's init
> function once and for all.
> 
> piix tested, cmd646 and via not tested.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

This conflicts with the AHCI patches. Can you rebase on top of the block
branch?

Kevin

Re: [Qemu-devel] Re: [PATCH] ide: Register vm change state handler once only

[Stefan Hajnoczi]

On Thu, Dec 16, 2010 at 12:29 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 10.12.2010 15:47, schrieb Stefan Hajnoczi:
>> We register the vm change state handler in a PCI BAR map() function.
>> This function can be called multiple times throughout the lifetime of a
>> PCI IDE device.  This results in duplicate vm change state handlers
>> being register, none of which are ever unregistered.
>>
>> Instead, register the vm change state handler in the device's init
>> function once and for all.
>>
>> piix tested, cmd646 and via not tested.
>>
>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>
> This conflicts with the AHCI patches. Can you rebase on top of the block
> branch?

Sure.

Stefan