cmd646: amend bmdma issue fix

PATCH Applied Igor V. Kovalenko 2 participants 4 messages Apr 22, 2010 – May 1, 2010

lore

Share & tools

lore

Igor V. Kovalenko

author

2010-04-22 21:54 UTC #0 lore raw

cover cmd646: amend bmdma issue fix

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

Signature · 2 lines

Kind regards,
Igor V. Kovalenko

Igor V. Kovalenko

author

2010-04-22 21:54 UTC #1 lore raw

PATCH 1/2 cmd646: pass pci_dev as it needs it

From: Igor V. Kovalenko <[email protected]>

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 <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>

---
 hw/ide/cmd646.c   |   61 +++++++++++++++++++++++++++++++++++++++--------------
 hw/ide/internal.h |    1 -
 hw/ide/piix.c     |    1 -
 3 files changed, 45 insertions(+), 18 deletions(-)

hw/ide/cmd646.c +45 ~~-16~~

index 0875de4..c6ed0b7 100644
@@ -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);

hw/ide/internal.h +0 -1

index 027029e..2efc784 100644
@@ -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;

hw/ide/piix.c +0 -1

index 295a93d..4fa3851 100644
@@ -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);

Igor V. Kovalenko

author

2010-04-22 21:54 UTC #2 lore raw

PATCH 2/2 cmd646: fix abort due to changed opaque pointer for ioport read

In reply to Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

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 <[email protected]>

---
 hw/ide/cmd646.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

hw/ide/cmd646.c +5 -4

index c6ed0b7..cdcc9bf 100644
@@ -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);
         }

Blue Swirl

Applied

2010-05-01 21:05 UTC #3 lore raw

In reply to Igor V. Kovalenko

Thanks, applied.

Quoted 31 lines · 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