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

[Qemu-devel] [PATCH v2] 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>
---
v2:
 * Rebased on kevin/block

 hw/ide/cmd646.c |   18 ++++++++++--------
 hw/ide/piix.c   |   34 ++++++++++++++++++++++++----------
 hw/ide/via.c    |   34 ++++++++++++++++++++++++----------
 3 files changed, 58 insertions(+), 28 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index e191ee6..89ba836 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -167,10 +167,6 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
 
     for(i = 0;i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
-        bmdma_init(&d->bus[i], bm);
-        bm->bus = d->bus+i;
-        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
-                                         &bm->dma);
 
         if (i == 0) {
             register_ioport_write(addr, 4, 1, bmdma_writeb_0, d);
@@ -228,6 +224,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 +250,15 @@ 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, 0);
-    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
-    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, i);
+        ide_init2(&d->bus[i], irq[i]);
+
+        bmdma_init(&d->bus[i], &d->bmdma[i]);
+        bm->bus = &d->bus[i];
+        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
+                                         &d->bmdma[i]->dma);
+    }
 
     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 a6b5d92..1cad906 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -76,10 +76,6 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
 
     for(i = 0;i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
-        bmdma_init(&d->bus[i], bm);
-        bm->bus = d->bus+i;
-        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
-                                         &bm->dma);
 
         register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);
 
@@ -112,6 +108,29 @@ 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, i);
+        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));
+
+        bmdma_init(&d->bus[i], &d->bmdma[i]);
+        d->bmdma[i].bus = &d->bus[i];
+        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
+                                         &d->bmdma[i].dma);
+    }
+}
+
 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, 0);
-    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
-    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 2603110..5b70bd2 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -78,10 +78,6 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
 
     for(i = 0;i < 2; i++) {
         BMDMAState *bm = &d->bmdma[i];
-        bmdma_init(&d->bus[i], bm);
-        bm->bus = d->bus+i;
-        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
-                                         &bm->dma);
 
         register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);
 
@@ -135,6 +131,29 @@ 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, i);
+        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));
+
+        bmdma_init(&d->bus[i], &d->bmdma[i]);
+        d->bmdma[i].bus = &d->bus[i];
+        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
+                                         &d->bmdma[i]->dma);
+    }
+}
+
 /* 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, 0);
-    ide_bus_new(&d->bus[1], &d->dev.qdev, 1);
-    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 v2] ide: Register vm change state handler once only

[Kevin Wolf]

Am 16.12.2010 16:54, 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>

Thanks, applied to the block branch.

Kevin

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

[Andreas Färber]

Am 17.12.2010 um 14:12 schrieb Kevin Wolf:

> Am 16.12.2010 16:54, 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>
>
> Thanks, applied to the block branch.

This just landed in master and breaks the build due to use of an  
undefined variable bm in hw/ide/cmd646.c...

Andreas

[Qemu-devel] [PATCH] ide: Fix build for cmd646.c

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/cmd646.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 89ba836..5d5464a 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -255,9 +255,9 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
         ide_init2(&d->bus[i], irq[i]);
 
         bmdma_init(&d->bus[i], &d->bmdma[i]);
-        bm->bus = &d->bus[i];
+        d->bmdma[i].bus = &d->bus[i];
         qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
-                                         &d->bmdma[i]->dma);
+                                         &d->bmdma[i].dma);
     }
 
     vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
-- 
1.7.2.3

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

[Kevin Wolf]

Am 17.12.2010 19:35, schrieb Andreas Färber:
> Am 17.12.2010 um 14:12 schrieb Kevin Wolf:
> 
>> Am 16.12.2010 16:54, 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>
>>
>> Thanks, applied to the block branch.
> 
> This just landed in master and breaks the build due to use of an  
> undefined variable bm in hw/ide/cmd646.c...

Can you try the patch I just sent?

Kevin

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

[Andreas Färber]

Am 17.12.2010 um 19:46 schrieb Kevin Wolf:

> Am 17.12.2010 19:35, schrieb Andreas Färber:
>> Am 17.12.2010 um 14:12 schrieb Kevin Wolf:
>>
>>> Am 16.12.2010 16:54, 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>
>>>
>>> Thanks, applied to the block branch.
>>
>> This just landed in master and breaks the build due to use of an
>> undefined variable bm in hw/ide/cmd646.c...
>
> Can you try the patch I just sent?

I just finished an identical patch. :)

Thanks,
Andreas

[Qemu-devel] Re: [PATCH] ide: Fix build for cmd646.c

[Andreas Färber]

Am 17.12.2010 um 19:44 schrieb Kevin Wolf:

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> hw/ide/cmd646.c |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index 89ba836..5d5464a 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -255,9 +255,9 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
>         ide_init2(&d->bus[i], irq[i]);
>
>         bmdma_init(&d->bus[i], &d->bmdma[i]);
> -        bm->bus = &d->bus[i];
> +        d->bmdma[i].bus = &d->bus[i];
>         qemu_add_vm_change_state_handler(d->bus[i].dma->ops- 
> >restart_cb,
> -                                         &d->bmdma[i]->dma);
> +                                         &d->bmdma[i].dma);
>     }
>
>     vmstate_register(&dev->qdev, 0, &vmstate_ide_pci, d);
> -- 
> 1.7.2.3

Acked-by: Andreas Färber <andreas.faerber@web.de>