[Qemu-devel] [PATCH 0/2] cmd646: amend bmdma issue fix

[Qemu-devel] [PATCH 0/2] cmd646: amend bmdma issue fix

[Igor V. Kovalenko]

Hi!

The following series are not picked from mailing list, so this is a resend.

Short history is that original fix for cmd646 bmdma issue was done by
reverting pci_dev removal from driver. That was a quick workaround
and later Juan was kind enough to improve driver code.

Patches on the list:
 - http://lists.gnu.org/archive/html/qemu-devel/2009-12/msg01421.html
 - http://lists.gnu.org/archive/html/qemu-devel/2009-12/msg01439.html

---

Igor V. Kovalenko (2):
      cmd646: pass pci_dev as it needs it
      cmd646: fix abort due to changed opaque pointer for ioport read


 hw/ide/cmd646.c   |   66 +++++++++++++++++++++++++++++++++++++++--------------
 hw/ide/internal.h |    1 -
 hw/ide/piix.c     |    1 -
 3 files changed, 48 insertions(+), 20 deletions(-)

-- 
Kind regards,
Igor V. Kovalenko

[Qemu-devel] [PATCH 1/2] cmd646: pass pci_dev as it needs it

[Igor V. Kovalenko]

From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

Instead of doing tricks to get the pci_dev, just pass it in the 1st
place.  Patch is a bit longer that reverting the pci_dev field, but it
states more clearly (IMHO) what we are doing.

It also fixes the bm test, now that you told me that ->unit is not
always valid.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 hw/ide/cmd646.c   |   61 +++++++++++++++++++++++++++++++++++++++--------------
 hw/ide/internal.h |    1 -
 hw/ide/piix.c     |    1 -
 3 files changed, 45 insertions(+), 18 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 0875de4..c6ed0b7 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -68,15 +68,9 @@ static void ide_map(PCIDevice *pci_dev, int region_num,
     }
 }
 
-static PCIIDEState *pci_from_bm(BMDMAState *bm)
+static uint32_t bmdma_readb_common(PCIIDEState *pci_dev, BMDMAState *bm,
+                                   uint32_t addr)
 {
-    return bm->pci_dev;
-}
-
-static uint32_t bmdma_readb(void *opaque, uint32_t addr)
-{
-    BMDMAState *bm = opaque;
-    PCIIDEState *pci_dev = pci_from_bm(bm);
     uint32_t val;
 
     switch(addr & 3) {
@@ -90,7 +84,7 @@ static uint32_t bmdma_readb(void *opaque, uint32_t addr)
         val = bm->status;
         break;
     case 3:
-        if (bm->unit == 0) {
+        if (bm == &pci_dev->bmdma[0]) {
             val = pci_dev->dev.config[UDIDETCR0];
         } else {
             val = pci_dev->dev.config[UDIDETCR1];
@@ -106,10 +100,25 @@ static uint32_t bmdma_readb(void *opaque, uint32_t addr)
     return val;
 }
 
-static void bmdma_writeb(void *opaque, uint32_t addr, uint32_t val)
+static uint32_t bmdma_readb_0(void *opaque, uint32_t addr)
+{
+    PCIIDEState *pci_dev = opaque;
+    BMDMAState *bm = &pci_dev->bmdma[0];
+
+    return bmdma_readb_common(pci_dev, bm, addr);
+}
+
+static uint32_t bmdma_readb_1(void *opaque, uint32_t addr)
+{
+    PCIIDEState *pci_dev = opaque;
+    BMDMAState *bm = &pci_dev->bmdma[1];
+
+    return bmdma_readb_common(pci_dev, bm, addr);
+}
+
+static void bmdma_writeb_common(PCIIDEState *pci_dev, BMDMAState *bm,
+                                uint32_t addr, uint32_t val)
 {
-    BMDMAState *bm = opaque;
-    PCIIDEState *pci_dev = pci_from_bm(bm);
 #ifdef DEBUG_IDE
     printf("bmdma: writeb 0x%02x : 0x%02x\n", addr, val);
 #endif
@@ -123,7 +132,7 @@ static void bmdma_writeb(void *opaque, uint32_t addr, uint32_t val)
         bm->status = (val & 0x60) | (bm->status & 1) | (bm->status & ~val & 0x06);
         break;
     case 3:
-        if (bm->unit == 0)
+        if (bm == &pci_dev->bmdma[0])
             pci_dev->dev.config[UDIDETCR0] = val;
         else
             pci_dev->dev.config[UDIDETCR1] = val;
@@ -131,6 +140,22 @@ static void bmdma_writeb(void *opaque, uint32_t addr, uint32_t val)
     }
 }
 
+static void bmdma_writeb_0(void *opaque, uint32_t addr, uint32_t val)
+{
+    PCIIDEState *pci_dev = opaque;
+    BMDMAState *bm = &pci_dev->bmdma[0];
+
+    bmdma_writeb_common(pci_dev, bm, addr, val);
+}
+
+static void bmdma_writeb_1(void *opaque, uint32_t addr, uint32_t val)
+{
+    PCIIDEState *pci_dev = opaque;
+    BMDMAState *bm = &pci_dev->bmdma[1];
+
+    bmdma_writeb_common(pci_dev, bm, addr, val);
+}
+
 static void bmdma_map(PCIDevice *pci_dev, int region_num,
                     pcibus_t addr, pcibus_t size, int type)
 {
@@ -141,13 +166,17 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
         BMDMAState *bm = &d->bmdma[i];
         d->bus[i].bmdma = bm;
         bm->bus = d->bus+i;
-        bm->pci_dev = d;
         qemu_add_vm_change_state_handler(ide_dma_restart_cb, bm);
 
         register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);
 
-        register_ioport_write(addr + 1, 3, 1, bmdma_writeb, bm);
-        register_ioport_read(addr, 4, 1, bmdma_readb, bm);
+        if (i == 0) {
+            register_ioport_write(addr + 1, 3, 1, bmdma_writeb_0, d);
+            register_ioport_read(addr, 4, 1, bmdma_readb_0, d);
+        } else {
+            register_ioport_write(addr + 1, 3, 1, bmdma_writeb_1, d);
+            register_ioport_read(addr, 4, 1, bmdma_readb_1, d);
+        }
 
         register_ioport_write(addr + 4, 4, 1, bmdma_addr_writeb, bm);
         register_ioport_read(addr + 4, 4, 1, bmdma_addr_readb, bm);
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 027029e..2efc784 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -483,7 +483,6 @@ struct BMDMAState {
     uint8_t status;
     uint32_t addr;
 
-    struct PCIIDEState *pci_dev;
     IDEBus *bus;
     /* current transfer state */
     uint32_t cur_addr;
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index 295a93d..4fa3851 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -78,7 +78,6 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
         BMDMAState *bm = &d->bmdma[i];
         d->bus[i].bmdma = bm;
         bm->bus = d->bus+i;
-        bm->pci_dev = d;
         qemu_add_vm_change_state_handler(ide_dma_restart_cb, bm);
 
         register_ioport_write(addr, 1, 1, bmdma_cmd_writeb, bm);

[Qemu-devel] [PATCH 2/2] cmd646: fix abort due to changed opaque pointer for ioport read

[Igor V. Kovalenko]

From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

We cannot install different opaque pointer for read and write
of the same i/o address.

- handle zero address in bmdma_writeb_common and install
  the same opaque pointer for both read and write access.

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 hw/ide/cmd646.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index c6ed0b7..cdcc9bf 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -123,6 +123,9 @@ static void bmdma_writeb_common(PCIIDEState *pci_dev, BMDMAState *bm,
     printf("bmdma: writeb 0x%02x : 0x%02x\n", addr, val);
 #endif
     switch(addr & 3) {
+    case 0:
+        bmdma_cmd_writeb(bm, addr, val);
+        break;
     case 1:
         pci_dev->dev.config[MRDMODE] =
             (pci_dev->dev.config[MRDMODE] & ~0x30) | (val & 0x30);
@@ -168,13 +171,11 @@ static void bmdma_map(PCIDevice *pci_dev, int region_num,
         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);
-
         if (i == 0) {
-            register_ioport_write(addr + 1, 3, 1, bmdma_writeb_0, d);
+            register_ioport_write(addr, 4, 1, bmdma_writeb_0, d);
             register_ioport_read(addr, 4, 1, bmdma_readb_0, d);
         } else {
-            register_ioport_write(addr + 1, 3, 1, bmdma_writeb_1, d);
+            register_ioport_write(addr, 4, 1, bmdma_writeb_1, d);
             register_ioport_read(addr, 4, 1, bmdma_readb_1, d);
         }
 

Re: [Qemu-devel] [PATCH 0/2] cmd646: amend bmdma issue fix

[Blue Swirl]

Thanks, applied.

On 4/23/10, Igor V. Kovalenko <igor.v.kovalenko@gmail.com> wrote:
> Hi!
>
>  The following series are not picked from mailing list, so this is a resend.
>
>  Short history is that original fix for cmd646 bmdma issue was done by
>  reverting pci_dev removal from driver. That was a quick workaround
>  and later Juan was kind enough to improve driver code.
>
>  Patches on the list:
>   - http://lists.gnu.org/archive/html/qemu-devel/2009-12/msg01421.html
>   - http://lists.gnu.org/archive/html/qemu-devel/2009-12/msg01439.html
>
>  ---
>
>  Igor V. Kovalenko (2):
>       cmd646: pass pci_dev as it needs it
>       cmd646: fix abort due to changed opaque pointer for ioport read
>
>
>   hw/ide/cmd646.c   |   66 +++++++++++++++++++++++++++++++++++++++--------------
>   hw/ide/internal.h |    1 -
>   hw/ide/piix.c     |    1 -
>   3 files changed, 48 insertions(+), 20 deletions(-)
>
>  --
>  Kind regards,
>
> Igor V. Kovalenko
>
>
>