[Qemu-devel] [PATCH 0/4] ide: Some DMA related fixes

[Qemu-devel] [PATCH 0/4] ide: Some DMA related fixes

[Kevin Wolf]

These are some fixes for the DMA code in IDE that I came up with when debugging
the crash in http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=604869

This doesn't fix the cause of the I/O errors (which is somewhere in vvfat), but
rather improves how the error situation is handled.

Kevin Wolf (4):
  ide: Factor ide_dma_set_inactive out
  ide: Set bus master inactive on error
  ide: Ignore double DMA transfer starts/stops
  ide: Reset current_addr after stopping DMA

 hw/ide/core.c |   31 +++++++++++++--------------
 hw/ide/pci.c  |   62 ++++++++++++++++++++++++++++++--------------------------
 2 files changed, 48 insertions(+), 45 deletions(-)

-- 
1.7.2.3

[Qemu-devel] [PATCH 1/4] ide: Factor ide_dma_set_inactive out

[Kevin Wolf]

Several places that stop a DMA transfer duplicate this code. Factor it out into
a common function.

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 484e0ca..7136ade 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -473,6 +473,14 @@ static void dma_buf_commit(IDEState *s, int is_write)
     qemu_sglist_destroy(&s->sg);
 }
 
+static void ide_dma_set_inactive(BMDMAState *bm)
+{
+    bm->status &= ~BM_STATUS_DMAING;
+    bm->dma_cb = NULL;
+    bm->unit = -1;
+    bm->aiocb = NULL;
+}
+
 void ide_dma_error(IDEState *s)
 {
     ide_transfer_stop(s);
@@ -587,11 +595,8 @@ static void ide_read_dma_cb(void *opaque, int ret)
         s->status = READY_STAT | SEEK_STAT;
         ide_set_irq(s->bus);
     eot:
-        bm->status &= ~BM_STATUS_DMAING;
         bm->status |= BM_STATUS_INT;
-        bm->dma_cb = NULL;
-        bm->unit = -1;
-        bm->aiocb = NULL;
+        ide_dma_set_inactive(bm);
         return;
     }
 
@@ -733,11 +738,8 @@ static void ide_write_dma_cb(void *opaque, int ret)
         s->status = READY_STAT | SEEK_STAT;
         ide_set_irq(s->bus);
     eot:
-        bm->status &= ~BM_STATUS_DMAING;
         bm->status |= BM_STATUS_INT;
-        bm->dma_cb = NULL;
-        bm->unit = -1;
-        bm->aiocb = NULL;
+        ide_dma_set_inactive(bm);
         return;
     }
 
@@ -1061,11 +1063,8 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
         s->nsector = (s->nsector & ~7) | ATAPI_INT_REASON_IO | ATAPI_INT_REASON_CD;
         ide_set_irq(s->bus);
     eot:
-        bm->status &= ~BM_STATUS_DMAING;
         bm->status |= BM_STATUS_INT;
-        bm->dma_cb = NULL;
-        bm->unit = -1;
-        bm->aiocb = NULL;
+        ide_dma_set_inactive(bm);
         return;
     }
 
@@ -2954,12 +2953,10 @@ void ide_dma_cancel(BMDMAState *bm)
             printf("aio_cancel\n");
 #endif
             bdrv_aio_cancel(bm->aiocb);
-            bm->aiocb = NULL;
         }
-        bm->status &= ~BM_STATUS_DMAING;
+
         /* cancel DMA request */
-        bm->unit = -1;
-        bm->dma_cb = NULL;
+        ide_dma_set_inactive(bm);
     }
 }
 
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 1/4] ide: Factor ide_dma_set_inactive out

[Stefan Hajnoczi]

On Fri, Nov 26, 2010 at 4:21 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Several places that stop a DMA transfer duplicate this code. Factor it out into
> a common function.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/ide/core.c |   29 +++++++++++++----------------
>  1 files changed, 13 insertions(+), 16 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 2/4] ide: Set bus master inactive on error

[Kevin Wolf]

BMIDEA in the status register must be cleared on error. This makes FreeBSD
respond (more) correctly to I/O errors.

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 7136ade..430350f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -486,6 +486,8 @@ void ide_dma_error(IDEState *s)
     ide_transfer_stop(s);
     s->error = ABRT_ERR;
     s->status = READY_STAT | ERR_STAT;
+    ide_dma_set_inactive(s->bus->bmdma);
+    s->bus->bmdma->status |= BM_STATUS_INT;
     ide_set_irq(s->bus);
 }
 
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 2/4] ide: Set bus master inactive on error

[Stefan Hajnoczi]

On Fri, Nov 26, 2010 at 4:21 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> BMIDEA in the status register must be cleared on error. This makes FreeBSD
> respond (more) correctly to I/O errors.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/ide/core.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 3/4] ide: Ignore double DMA transfer starts/stops

[Kevin Wolf]

You can only start a DMA transfer if it's not running yet, and you can only
cancel it if it's running.

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

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 3722b77..404f045 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -39,38 +39,42 @@ void bmdma_cmd_writeb(void *opaque, uint32_t addr, uint32_t val)
 #ifdef DEBUG_IDE
     printf("%s: 0x%08x\n", __func__, val);
 #endif
-    if (!(val & BM_CMD_START)) {
-        /*
-         * We can't cancel Scatter Gather DMA in the middle of the
-         * operation or a partial (not full) DMA transfer would reach
-         * the storage so we wait for completion instead (we beahve
-         * like if the DMA was completed by the time the guest trying
-         * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not
-         * set).
-         *
-         * In the future we'll be able to safely cancel the I/O if the
-         * whole DMA operation will be submitted to disk with a single
-         * aio operation with preadv/pwritev.
-         */
-        if (bm->aiocb) {
-            qemu_aio_flush();
+
+    /* Ignore writes to SSBM if it keeps the old value */
+    if ((val & BM_CMD_START) != (bm->cmd & BM_CMD_START)) {
+        if (!(val & BM_CMD_START)) {
+            /*
+             * We can't cancel Scatter Gather DMA in the middle of the
+             * operation or a partial (not full) DMA transfer would reach
+             * the storage so we wait for completion instead (we beahve
+             * like if the DMA was completed by the time the guest trying
+             * to cancel dma with bmdma_cmd_writeb with BM_CMD_START not
+             * set).
+             *
+             * In the future we'll be able to safely cancel the I/O if the
+             * whole DMA operation will be submitted to disk with a single
+             * aio operation with preadv/pwritev.
+             */
+            if (bm->aiocb) {
+                qemu_aio_flush();
 #ifdef DEBUG_IDE
-            if (bm->aiocb)
-                printf("ide_dma_cancel: aiocb still pending");
-            if (bm->status & BM_STATUS_DMAING)
-                printf("ide_dma_cancel: BM_STATUS_DMAING still pending");
+                if (bm->aiocb)
+                    printf("ide_dma_cancel: aiocb still pending");
+                if (bm->status & BM_STATUS_DMAING)
+                    printf("ide_dma_cancel: BM_STATUS_DMAING still pending");
 #endif
+            }
+        } else {
+            if (!(bm->status & BM_STATUS_DMAING)) {
+                bm->status |= BM_STATUS_DMAING;
+                /* start dma transfer if possible */
+                if (bm->dma_cb)
+                    bm->dma_cb(bm, 0);
+            }
         }
-        bm->cmd = val & 0x09;
-    } else {
-        if (!(bm->status & BM_STATUS_DMAING)) {
-            bm->status |= BM_STATUS_DMAING;
-            /* start dma transfer if possible */
-            if (bm->dma_cb)
-                bm->dma_cb(bm, 0);
-        }
-        bm->cmd = val & 0x09;
     }
+
+    bm->cmd = val & 0x09;
 }
 
 static void bmdma_addr_read(IORange *ioport, uint64_t addr,
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 3/4] ide: Ignore double DMA transfer starts/stops

[Stefan Hajnoczi]

On Fri, Nov 26, 2010 at 4:21 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> You can only start a DMA transfer if it's not running yet, and you can only
> cancel it if it's running.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/ide/pci.c |   60 ++++++++++++++++++++++++++++++---------------------------
>  1 files changed, 32 insertions(+), 28 deletions(-)

The PIIX4 spec isn't clear on the behavior but you seem to have worked
out that this makes FreeBSD happier.  Would be nice to hear from
someone who knows PIIX4 IDE well.

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

[Qemu-devel] [PATCH 4/4] ide: Reset current_addr after stopping DMA

[Kevin Wolf]

Whenever SSBM is reset in the command register all state information is lost.
Restarting DMA means that current_addr must be reset to the base address of the
PRD table. The OS is not required to change the base address register before
starting a DMA operation, it can reuse the value it wrote for an earlier
request.

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

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 404f045..ad406ee 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -65,6 +65,7 @@ void bmdma_cmd_writeb(void *opaque, uint32_t addr, uint32_t val)
 #endif
             }
         } else {
+            bm->cur_addr = bm->addr;
             if (!(bm->status & BM_STATUS_DMAING)) {
                 bm->status |= BM_STATUS_DMAING;
                 /* start dma transfer if possible */
@@ -101,7 +102,6 @@ static void bmdma_addr_write(IORange *ioport, uint64_t addr,
 #endif
     bm->addr &= ~(mask << shift);
     bm->addr |= ((data & mask) << shift) & ~3;
-    bm->cur_addr = bm->addr;
 }
 
 const IORangeOps bmdma_addr_ioport_ops = {
-- 
1.7.2.3

[Qemu-devel] Re: [PATCH 4/4] ide: Reset current_addr after stopping DMA

[Stefan Hajnoczi]

On Fri, Nov 26, 2010 at 4:21 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Whenever SSBM is reset in the command register all state information is lost.
> Restarting DMA means that current_addr must be reset to the base address of the
> PRD table. The OS is not required to change the base address register before
> starting a DMA operation, it can reuse the value it wrote for an earlier
> request.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/ide/pci.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>