[Qemu-devel] [PATCH v2 for 2.6 0/3] ide: fix loss of the dma/atapi state during migration

[Qemu-devel] [PATCH v2 for 2.6 0/3] ide: fix loss of the dma/atapi state during migration

[Denis V. Lunev]

This patch set fixes bugs in the IDE DMA and the IDE ATAPI on operations to
save/restore the state.

>From the user point of view this results in IDE timeouts in the guest
when the user reads from the DVD like the following:

[424332.169229] ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen
[424332.170423] sr 0:0:0:0: [sr0] CDB:
[424332.171234] Read(10): 28 00 00 00 02 e4 00 00 01 00
[424332.172418] ata1.00: cmd a0/01:00:00:00:08/00:00:00:00:00/a0 tag 0 dma 2048 in
         res 40/00:02:00:0c:00/00:00:00:00:00/a0 Emask 0x4 (timeout)
[424332.174877] ata1.00: status: { DRDY }
[424337.212099] ata1: link is slow to respond, please be patient (ready=0)
[424342.220084] ata1: device not ready (errno=-16), forcing hardreset
[424342.222700] ata1: soft resetting link
[424342.381059] ata1.00: configured for MWDMA2
[424342.383693] ata1: EH complete

Another similar nasty effects are possible.

Changes from v1:
- added converter of IDE_DMA_* to IDE_RETRY_* (1)
- fixed handling of the IDE_RETRY_HBA at the ide_restart_bh function (3)

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: John Snow <jsnow@redhat.com>

Pavel Butsykin (3):
  ide: don't lose pending dma state
  ide: restart atapi dma by re-evaluating command packet
  ide: really restart pending and in-flight atapi dma

 hw/ide/atapi.c    | 28 ++++++++++++++--------------
 hw/ide/core.c     | 36 +++++++++++++-----------------------
 hw/ide/internal.h | 36 ++++++++++++++++++++++++++++++++++++
 hw/ide/pci.c      |  4 ++++
 4 files changed, 67 insertions(+), 37 deletions(-)

-- 
2.1.4

[Qemu-devel] [PATCH 1/3] ide: don't lose pending dma state

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

If the migration occurs after the IDE DMA has been set up but before it
has been initiated, the state gets lost upon save/restore. Specifically,
->dma_cb callback gets cleared, so, when the guest eventually starts bus
mastering, the DMA never completes, causing the guest to time out the
operation.

OTOH all the infrastructure is already in place to restart the DMA if
the migration happens while the DMA is in progress.

So reuse that infrastructure, by setting bus->error_status based on
->dma_cmd in pre_save if ->dma_cb callback is already set but DMAING is
clear. This will indicate the need for restart and make sure ->dma_cb is
restored in ide_restart_bh(); however since DMAING is clear the state
upon restore will be exactly "ready for DMA" as before the save.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c     |  9 +--------
 hw/ide/internal.h | 15 +++++++++++++++
 hw/ide/pci.c      |  4 ++++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 241e840..8f86036 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -803,14 +803,7 @@ static void ide_dma_cb(void *opaque, int ret)
         return;
     }
     if (ret < 0) {
-        int op = IDE_RETRY_DMA;
-
-        if (s->dma_cmd == IDE_DMA_READ)
-            op |= IDE_RETRY_READ;
-        else if (s->dma_cmd == IDE_DMA_TRIM)
-            op |= IDE_RETRY_TRIM;
-
-        if (ide_handle_rw_error(s, -ret, op)) {
+        if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
             return;
         }
     }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 86bde26..68c7d0d 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -513,6 +513,21 @@ struct IDEDevice {
 #define IDE_RETRY_TRIM 0x80
 #define IDE_RETRY_HBA  0x100
 
+static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
+{
+    switch (dma_cmd) {
+    case IDE_DMA_READ:
+        return IDE_RETRY_DMA | IDE_RETRY_READ;
+    case IDE_DMA_WRITE:
+        return IDE_RETRY_DMA;
+    case IDE_DMA_TRIM:
+        return IDE_RETRY_DMA | IDE_RETRY_TRIM;
+    default:
+        break;
+    }
+    return 0;
+}
+
 static inline IDEState *idebus_active_if(IDEBus *bus)
 {
     return bus->ifs + bus->unit;
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 92ffee7..8d56a00 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -308,6 +308,10 @@ static void ide_bmdma_pre_save(void *opaque)
     BMDMAState *bm = opaque;
     uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
 
+    if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
+        bm->bus->error_status =
+            ide_dma_cmd_to_retry(bmdma_active_if(bm)->dma_cmd);
+    }
     bm->migration_retry_unit = bm->bus->retry_unit;
     bm->migration_retry_sector_num = bm->bus->retry_sector_num;
     bm->migration_retry_nsector = bm->bus->retry_nsector;
-- 
2.1.4

[Qemu-devel] [PATCH 2/3] ide: restart atapi dma by re-evaluating command packet

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

ide_atapi_dma_restart() used to just complete the DMA with an error,
under the assumption that there isn't enough information to restart it.

However, as the contents of the ->io_buffer is preserved, it looks safe to
just re-evaluate it and dispatch the ATAPI command again.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 1fe58ab..acc52cd 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -488,14 +488,13 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors,
 void ide_atapi_dma_restart(IDEState *s)
 {
     /*
-     * I'm not sure we have enough stored to restart the command
-     * safely, so give the guest an error it should recover from.
-     * I'm assuming most guests will try to recover from something
-     * listed as a medium error on a CD; it seems to work on Linux.
-     * This would be more of a problem if we did any other type of
-     * DMA operation.
+     * At this point we can just re-evaluate the packet command and start over.
+     * The presence of ->dma_cb callback in the pre_save ensures that the packet
+     * command has been completely sent and we can safely restart command.
      */
-    ide_atapi_cmd_error(s, MEDIUM_ERROR, ASC_NO_SEEK_COMPLETE);
+    s->unit = s->bus->retry_unit;
+    s->bus->dma->ops->restart_dma(s->bus->dma);
+    ide_atapi_cmd(s);
 }
 
 static inline uint8_t ide_atapi_set_profile(uint8_t *buf, uint8_t *index,
-- 
2.1.4

[Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

Restart of ATAPI DMA used to be unreachable, because the request to do
so wasn't indicated in bus->error_status due to the lack of spare bits, and
ide_restart_bh() would return early doing nothing.

This patch makes use of the observation that not all bit combinations were
possible in ->error_status. In particular, IDE_RETRY_READ only made sense
together with IDE_RETRY_DMA or IDE_RETRY_PIO. This allows to re-use
IDE_RETRY_READ alone as an indicator of ATAPI DMA restart request.

To makes things more uniform, ATAPI DMA gets its own value for ->dma_cmd.
As a means against confusion, macros are added to test the state of
->error_status.

The patch fixes the restart of both in-flight and pending ATAPI DMA,
following the scheme similar to that of IDE DMA.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: John Snow <jsnow@redhat.com>
---
 hw/ide/atapi.c    | 15 ++++++++-------
 hw/ide/core.c     | 27 ++++++++++++---------------
 hw/ide/internal.h | 21 +++++++++++++++++++++
 3 files changed, 41 insertions(+), 22 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index acc52cd..fb9ae43 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -342,6 +342,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size)
         block_acct_start(blk_get_stats(s->blk), &s->acct, size,
                          BLOCK_ACCT_READ);
         s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
+        s->dma_cmd = IDE_DMA_ATAPI;
         ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
     } else {
         s->status = READY_STAT | SEEK_STAT;
@@ -375,15 +376,18 @@ static void ide_atapi_cmd_check_status(IDEState *s)
 }
 /* ATAPI DMA support */
 
-/* XXX: handle read errors */
 static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
 {
     IDEState *s = opaque;
     int data_offset, n;
 
     if (ret < 0) {
-        ide_atapi_io_error(s, ret);
-        goto eot;
+        if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
+            if (s->bus->error_status) {
+                return;
+            }
+            goto eot;
+        }
     }
 
     if (s->io_buffer_size > 0) {
@@ -464,6 +468,7 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors,
 
     /* XXX: check if BUSY_STAT should be set */
     s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
+    s->dma_cmd = IDE_DMA_ATAPI;
     ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
 }
 
@@ -481,10 +486,6 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors,
     }
 }
 
-
-/* Called by *_restart_bh when the transfer function points
- * to ide_atapi_cmd
- */
 void ide_atapi_dma_restart(IDEState *s)
 {
     /*
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 8f86036..0425d86 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -56,7 +56,6 @@ static const int smart_attributes[][12] = {
     { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32},
 };
 
-static int ide_handle_rw_error(IDEState *s, int error, int op);
 static void ide_dummy_transfer_stop(IDEState *s);
 
 static void padstr(char *str, const char *src, int len)
@@ -772,7 +771,7 @@ void ide_dma_error(IDEState *s)
     ide_set_irq(s->bus);
 }
 
-static int ide_handle_rw_error(IDEState *s, int error, int op)
+int ide_handle_rw_error(IDEState *s, int error, int op)
 {
     bool is_read = (op & IDE_RETRY_READ) != 0;
     BlockErrorAction action = blk_get_error_action(s->blk, is_read, error);
@@ -782,8 +781,10 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
         s->bus->error_status = op;
     } else if (action == BLOCK_ERROR_ACTION_REPORT) {
         block_acct_failed(blk_get_stats(s->blk), &s->acct);
-        if (op & IDE_RETRY_DMA) {
+        if (IS_IDE_RETRY_DMA(op)) {
             ide_dma_error(s);
+        } else if (IS_IDE_RETRY_ATAPI(op)) {
+            ide_atapi_io_error(s, -error);
         } else {
             ide_rw_error(s);
         }
@@ -871,6 +872,8 @@ static void ide_dma_cb(void *opaque, int ret)
                                         ide_issue_trim, ide_dma_cb, s,
                                         DMA_DIRECTION_TO_DEVICE);
         break;
+    default:
+        abort();
     }
     return;
 
@@ -2517,15 +2520,13 @@ static void ide_restart_bh(void *opaque)
         if (s->bus->dma->ops->restart) {
             s->bus->dma->ops->restart(s->bus->dma);
         }
-    }
-
-    if (error_status & IDE_RETRY_DMA) {
+    } else if (IS_IDE_RETRY_DMA(error_status)) {
         if (error_status & IDE_RETRY_TRIM) {
             ide_restart_dma(s, IDE_DMA_TRIM);
         } else {
             ide_restart_dma(s, is_read ? IDE_DMA_READ : IDE_DMA_WRITE);
         }
-    } else if (error_status & IDE_RETRY_PIO) {
+    } else if (IS_IDE_RETRY_PIO(error_status)) {
         if (is_read) {
             ide_sector_read(s);
         } else {
@@ -2533,15 +2534,11 @@ static void ide_restart_bh(void *opaque)
         }
     } else if (error_status & IDE_RETRY_FLUSH) {
         ide_flush_cache(s);
+    } else if (IS_IDE_RETRY_ATAPI(error_status)) {
+        assert(s->end_transfer_func == ide_atapi_cmd);
+        ide_atapi_dma_restart(s);
     } else {
-        /*
-         * We've not got any bits to tell us about ATAPI - but
-         * we do have the end_transfer_func that tells us what
-         * we're trying to do.
-         */
-        if (s->end_transfer_func == ide_atapi_cmd) {
-            ide_atapi_dma_restart(s);
-        }
+        abort();
     }
 }
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 68c7d0d..eb006c2 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -338,6 +338,7 @@ enum ide_dma_cmd {
     IDE_DMA_READ,
     IDE_DMA_WRITE,
     IDE_DMA_TRIM,
+    IDE_DMA_ATAPI
 };
 
 #define ide_cmd_is_read(s) \
@@ -508,11 +509,27 @@ struct IDEDevice {
 /* These are used for the error_status field of IDEBus */
 #define IDE_RETRY_DMA  0x08
 #define IDE_RETRY_PIO  0x10
+#define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */
 #define IDE_RETRY_READ  0x20
 #define IDE_RETRY_FLUSH 0x40
 #define IDE_RETRY_TRIM 0x80
 #define IDE_RETRY_HBA  0x100
 
+#define IS_IDE_RETRY_DMA(_status) \
+    ((_status) & IDE_RETRY_DMA)
+
+#define IS_IDE_RETRY_PIO(_status) \
+    ((_status) & IDE_RETRY_PIO)
+
+/*
+ * The method of the IDE_RETRY_ATAPI determination is to use a previously
+ * impossible bit combination as a new status value.
+ */
+#define IS_IDE_RETRY_ATAPI(_status)   \
+    (((_status) & IDE_RETRY_ATAPI) && \
+     !IS_IDE_RETRY_DMA(_status) &&    \
+     !IS_IDE_RETRY_PIO(_status))
+
 static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
 {
     switch (dma_cmd) {
@@ -522,6 +539,8 @@ static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
         return IDE_RETRY_DMA;
     case IDE_DMA_TRIM:
         return IDE_RETRY_DMA | IDE_RETRY_TRIM;
+    case IDE_DMA_ATAPI:
+        return IDE_RETRY_ATAPI;
     default:
         break;
     }
@@ -612,4 +631,6 @@ void ide_bus_new(IDEBus *idebus, size_t idebus_size, DeviceState *dev,
                  int bus_id, int max_units);
 IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
 
+int ide_handle_rw_error(IDEState *s, int error, int op);
+
 #endif /* HW_IDE_INTERNAL_H */
-- 
2.1.4

Re: [Qemu-devel] [PATCH 1/3] ide: don't lose pending dma state

[John Snow]

On 03/28/2016 07:48 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> If the migration occurs after the IDE DMA has been set up but before it
> has been initiated, the state gets lost upon save/restore. Specifically,
> ->dma_cb callback gets cleared, so, when the guest eventually starts bus
> mastering, the DMA never completes, causing the guest to time out the
> operation.
> 
> OTOH all the infrastructure is already in place to restart the DMA if
> the migration happens while the DMA is in progress.
> 
> So reuse that infrastructure, by setting bus->error_status based on
> ->dma_cmd in pre_save if ->dma_cb callback is already set but DMAING is
> clear. This will indicate the need for restart and make sure ->dma_cb is
> restored in ide_restart_bh(); however since DMAING is clear the state
> upon restore will be exactly "ready for DMA" as before the save.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/core.c     |  9 +--------
>  hw/ide/internal.h | 15 +++++++++++++++
>  hw/ide/pci.c      |  4 ++++
>  3 files changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 241e840..8f86036 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -803,14 +803,7 @@ static void ide_dma_cb(void *opaque, int ret)
>          return;
>      }
>      if (ret < 0) {
> -        int op = IDE_RETRY_DMA;
> -
> -        if (s->dma_cmd == IDE_DMA_READ)
> -            op |= IDE_RETRY_READ;
> -        else if (s->dma_cmd == IDE_DMA_TRIM)
> -            op |= IDE_RETRY_TRIM;
> -
> -        if (ide_handle_rw_error(s, -ret, op)) {
> +        if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
>              return;
>          }
>      }
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 86bde26..68c7d0d 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -513,6 +513,21 @@ struct IDEDevice {
>  #define IDE_RETRY_TRIM 0x80
>  #define IDE_RETRY_HBA  0x100
>  
> +static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
> +{
> +    switch (dma_cmd) {
> +    case IDE_DMA_READ:
> +        return IDE_RETRY_DMA | IDE_RETRY_READ;
> +    case IDE_DMA_WRITE:
> +        return IDE_RETRY_DMA;
> +    case IDE_DMA_TRIM:
> +        return IDE_RETRY_DMA | IDE_RETRY_TRIM;
> +    default:
> +        break;
> +    }
> +    return 0;
> +}
> +
>  static inline IDEState *idebus_active_if(IDEBus *bus)
>  {
>      return bus->ifs + bus->unit;
> diff --git a/hw/ide/pci.c b/hw/ide/pci.c
> index 92ffee7..8d56a00 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -308,6 +308,10 @@ static void ide_bmdma_pre_save(void *opaque)
>      BMDMAState *bm = opaque;
>      uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
>  
> +    if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
> +        bm->bus->error_status =
> +            ide_dma_cmd_to_retry(bmdma_active_if(bm)->dma_cmd);
> +    }
>      bm->migration_retry_unit = bm->bus->retry_unit;
>      bm->migration_retry_sector_num = bm->bus->retry_sector_num;
>      bm->migration_retry_nsector = bm->bus->retry_nsector;
> 

_this_ is the one I meant to R-B, thanks Denis. (Testing the others now,
but having some laptop issues. Please stand by.)

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH 3/3] ide: really restart pending and in-flight atapi dma

[John Snow]

On 03/28/2016 07:48 AM, Denis V. Lunev wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> Restart of ATAPI DMA used to be unreachable, because the request to do
> so wasn't indicated in bus->error_status due to the lack of spare bits, and
> ide_restart_bh() would return early doing nothing.
> 
> This patch makes use of the observation that not all bit combinations were
> possible in ->error_status. In particular, IDE_RETRY_READ only made sense
> together with IDE_RETRY_DMA or IDE_RETRY_PIO. This allows to re-use
> IDE_RETRY_READ alone as an indicator of ATAPI DMA restart request.
> 
> To makes things more uniform, ATAPI DMA gets its own value for ->dma_cmd.
> As a means against confusion, macros are added to test the state of
> ->error_status.
> 
> The patch fixes the restart of both in-flight and pending ATAPI DMA,
> following the scheme similar to that of IDE DMA.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/atapi.c    | 15 ++++++++-------
>  hw/ide/core.c     | 27 ++++++++++++---------------
>  hw/ide/internal.h | 21 +++++++++++++++++++++
>  3 files changed, 41 insertions(+), 22 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index acc52cd..fb9ae43 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -342,6 +342,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size)
>          block_acct_start(blk_get_stats(s->blk), &s->acct, size,
>                           BLOCK_ACCT_READ);
>          s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
> +        s->dma_cmd = IDE_DMA_ATAPI;
>          ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
>      } else {
>          s->status = READY_STAT | SEEK_STAT;
> @@ -375,15 +376,18 @@ static void ide_atapi_cmd_check_status(IDEState *s)
>  }
>  /* ATAPI DMA support */
>  
> -/* XXX: handle read errors */
>  static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
>  {
>      IDEState *s = opaque;
>      int data_offset, n;
>  
>      if (ret < 0) {
> -        ide_atapi_io_error(s, ret);
> -        goto eot;
> +        if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
> +            if (s->bus->error_status) {
> +                return;
> +            }
> +            goto eot;
> +        }
>      }
>  
>      if (s->io_buffer_size > 0) {
> @@ -464,6 +468,7 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors,
>  
>      /* XXX: check if BUSY_STAT should be set */
>      s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
> +    s->dma_cmd = IDE_DMA_ATAPI;
>      ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
>  }
>  
> @@ -481,10 +486,6 @@ static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors,
>      }
>  }
>  
> -
> -/* Called by *_restart_bh when the transfer function points
> - * to ide_atapi_cmd
> - */
>  void ide_atapi_dma_restart(IDEState *s)
>  {
>      /*
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 8f86036..0425d86 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -56,7 +56,6 @@ static const int smart_attributes[][12] = {
>      { 190,  0x03, 0x00, 0x45, 0x45, 0x1f, 0x00, 0x1f, 0x1f, 0x00, 0x00, 0x32},
>  };
>  
> -static int ide_handle_rw_error(IDEState *s, int error, int op);
>  static void ide_dummy_transfer_stop(IDEState *s);
>  
>  static void padstr(char *str, const char *src, int len)
> @@ -772,7 +771,7 @@ void ide_dma_error(IDEState *s)
>      ide_set_irq(s->bus);
>  }
>  
> -static int ide_handle_rw_error(IDEState *s, int error, int op)
> +int ide_handle_rw_error(IDEState *s, int error, int op)
>  {
>      bool is_read = (op & IDE_RETRY_READ) != 0;
>      BlockErrorAction action = blk_get_error_action(s->blk, is_read, error);
> @@ -782,8 +781,10 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
>          s->bus->error_status = op;
>      } else if (action == BLOCK_ERROR_ACTION_REPORT) {
>          block_acct_failed(blk_get_stats(s->blk), &s->acct);
> -        if (op & IDE_RETRY_DMA) {
> +        if (IS_IDE_RETRY_DMA(op)) {
>              ide_dma_error(s);
> +        } else if (IS_IDE_RETRY_ATAPI(op)) {
> +            ide_atapi_io_error(s, -error);
>          } else {
>              ide_rw_error(s);
>          }
> @@ -871,6 +872,8 @@ static void ide_dma_cb(void *opaque, int ret)
>                                          ide_issue_trim, ide_dma_cb, s,
>                                          DMA_DIRECTION_TO_DEVICE);
>          break;
> +    default:
> +        abort();
>      }
>      return;
>  
> @@ -2517,15 +2520,13 @@ static void ide_restart_bh(void *opaque)
>          if (s->bus->dma->ops->restart) {
>              s->bus->dma->ops->restart(s->bus->dma);
>          }
> -    }
> -
> -    if (error_status & IDE_RETRY_DMA) {
> +    } else if (IS_IDE_RETRY_DMA(error_status)) {
>          if (error_status & IDE_RETRY_TRIM) {
>              ide_restart_dma(s, IDE_DMA_TRIM);
>          } else {
>              ide_restart_dma(s, is_read ? IDE_DMA_READ : IDE_DMA_WRITE);
>          }
> -    } else if (error_status & IDE_RETRY_PIO) {
> +    } else if (IS_IDE_RETRY_PIO(error_status)) {
>          if (is_read) {
>              ide_sector_read(s);
>          } else {
> @@ -2533,15 +2534,11 @@ static void ide_restart_bh(void *opaque)
>          }
>      } else if (error_status & IDE_RETRY_FLUSH) {
>          ide_flush_cache(s);
> +    } else if (IS_IDE_RETRY_ATAPI(error_status)) {
> +        assert(s->end_transfer_func == ide_atapi_cmd);
> +        ide_atapi_dma_restart(s);
>      } else {
> -        /*
> -         * We've not got any bits to tell us about ATAPI - but
> -         * we do have the end_transfer_func that tells us what
> -         * we're trying to do.
> -         */
> -        if (s->end_transfer_func == ide_atapi_cmd) {
> -            ide_atapi_dma_restart(s);
> -        }
> +        abort();
>      }
>  }
>  
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 68c7d0d..eb006c2 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -338,6 +338,7 @@ enum ide_dma_cmd {
>      IDE_DMA_READ,
>      IDE_DMA_WRITE,
>      IDE_DMA_TRIM,
> +    IDE_DMA_ATAPI
>  };
>  
>  #define ide_cmd_is_read(s) \
> @@ -508,11 +509,27 @@ struct IDEDevice {
>  /* These are used for the error_status field of IDEBus */
>  #define IDE_RETRY_DMA  0x08
>  #define IDE_RETRY_PIO  0x10
> +#define IDE_RETRY_ATAPI 0x20 /* reused IDE_RETRY_READ bit */
>  #define IDE_RETRY_READ  0x20
>  #define IDE_RETRY_FLUSH 0x40
>  #define IDE_RETRY_TRIM 0x80
>  #define IDE_RETRY_HBA  0x100
>  
> +#define IS_IDE_RETRY_DMA(_status) \
> +    ((_status) & IDE_RETRY_DMA)
> +
> +#define IS_IDE_RETRY_PIO(_status) \
> +    ((_status) & IDE_RETRY_PIO)
> +
> +/*
> + * The method of the IDE_RETRY_ATAPI determination is to use a previously
> + * impossible bit combination as a new status value.
> + */
> +#define IS_IDE_RETRY_ATAPI(_status)   \
> +    (((_status) & IDE_RETRY_ATAPI) && \
> +     !IS_IDE_RETRY_DMA(_status) &&    \
> +     !IS_IDE_RETRY_PIO(_status))
> +
>  static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
>  {
>      switch (dma_cmd) {
> @@ -522,6 +539,8 @@ static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
>          return IDE_RETRY_DMA;
>      case IDE_DMA_TRIM:
>          return IDE_RETRY_DMA | IDE_RETRY_TRIM;
> +    case IDE_DMA_ATAPI:
> +        return IDE_RETRY_ATAPI;
>      default:
>          break;
>      }
> @@ -612,4 +631,6 @@ void ide_bus_new(IDEBus *idebus, size_t idebus_size, DeviceState *dev,
>                   int bus_id, int max_units);
>  IDEDevice *ide_create_drive(IDEBus *bus, int unit, DriveInfo *drive);
>  
> +int ide_handle_rw_error(IDEState *s, int error, int op);
> +
>  #endif /* HW_IDE_INTERNAL_H */
> 


Sorry, I'm afraid this doesn't compile:

/home/bos/jhuston/src/qemu/hw/ide/macio.c: In function
‘pmac_ide_transfer_cb’:
/home/bos/jhuston/src/qemu/hw/ide/macio.c:339:5: error: enumeration
value ‘IDE_DMA_ATAPI’ not handled in switch [-Werror=switch]
switch (s->dma_cmd) {
^
cc1: all warnings being treated as errors
/home/bos/jhuston/src/qemu/rules.mak:57: recipe for target
'hw/ide/macio.o' failed
make: *** [hw/ide/macio.o] Error 1
make: *** Waiting for unfinished jobs....

[Qemu-devel] [PATCH 1/3] ide: don't lose pending dma state

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

If the migration occurs after the IDE DMA has been set up but before it
has been initiated, the state gets lost upon save/restore. Specifically,
->dma_cb callback gets cleared, so, when the guest eventually starts bus
mastering, the DMA never completes, causing the guest to time out the
operation.

OTOH all the infrastructure is already in place to restart the DMA if
the migration happens while the DMA is in progress.

So reuse that infrastructure, by setting bus->error_status based on ->dma_cmd
in pre_save if ->dma_cb callback is already set but DMAING is clear. This will
indicate the need for restart and make sure ->dma_cb is restored in
ide_restart_bh(); howeover since DMAING is clear the state upon restore will be
exactly "ready for DMA" as before the save.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c     |  9 +--------
 hw/ide/internal.h | 15 +++++++++++++++
 hw/ide/pci.c      |  4 ++++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 241e840..8f86036 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -803,14 +803,7 @@ static void ide_dma_cb(void *opaque, int ret)
         return;
     }
     if (ret < 0) {
-        int op = IDE_RETRY_DMA;
-
-        if (s->dma_cmd == IDE_DMA_READ)
-            op |= IDE_RETRY_READ;
-        else if (s->dma_cmd == IDE_DMA_TRIM)
-            op |= IDE_RETRY_TRIM;
-
-        if (ide_handle_rw_error(s, -ret, op)) {
+        if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
             return;
         }
     }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 86bde26..68c7d0d 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -513,6 +513,21 @@ struct IDEDevice {
 #define IDE_RETRY_TRIM 0x80
 #define IDE_RETRY_HBA  0x100
 
+static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
+{
+    switch (dma_cmd) {
+    case IDE_DMA_READ:
+        return IDE_RETRY_DMA | IDE_RETRY_READ;
+    case IDE_DMA_WRITE:
+        return IDE_RETRY_DMA;
+    case IDE_DMA_TRIM:
+        return IDE_RETRY_DMA | IDE_RETRY_TRIM;
+    default:
+        break;
+    }
+    return 0;
+}
+
 static inline IDEState *idebus_active_if(IDEBus *bus)
 {
     return bus->ifs + bus->unit;
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 92ffee7..8d56a00 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -308,6 +308,10 @@ static void ide_bmdma_pre_save(void *opaque)
     BMDMAState *bm = opaque;
     uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
 
+    if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
+        bm->bus->error_status =
+            ide_dma_cmd_to_retry(bmdma_active_if(bm)->dma_cmd);
+    }
     bm->migration_retry_unit = bm->bus->retry_unit;
     bm->migration_retry_sector_num = bm->bus->retry_sector_num;
     bm->migration_retry_nsector = bm->bus->retry_nsector;
-- 
2.1.4

[Qemu-devel] [PATCH 1/3] ide: don't lose pending dma state

[Denis V. Lunev]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

If the migration occurs after the IDE DMA has been set up but before it
has been initiated, the state gets lost upon save/restore. Specifically,
->dma_cb callback gets cleared, so, when the guest eventually starts bus
mastering, the DMA never completes, causing the guest to time out the
operation.

OTOH all the infrastructure is already in place to restart the DMA if
the migration happens while the DMA is in progress.

So reuse that infrastructure, by setting bus->error_status based on
->dma_cmd in pre_save if ->dma_cb callback is already set but DMAING is
clear. This will indicate the need for restart and make sure ->dma_cb
is restored in ide_restart_bh(); howeover since DMAING is clear the state
upon restore will be exactly "ready for DMA" as before the save.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 hw/ide/core.c     |  9 +--------
 hw/ide/internal.h | 15 +++++++++++++++
 hw/ide/pci.c      |  4 ++++
 3 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 90524d5..58d0687 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -804,14 +804,7 @@ static void ide_dma_cb(void *opaque, int ret)
         return;
     }
     if (ret < 0) {
-        int op = IDE_RETRY_DMA;
-
-        if (s->dma_cmd == IDE_DMA_READ)
-            op |= IDE_RETRY_READ;
-        else if (s->dma_cmd == IDE_DMA_TRIM)
-            op |= IDE_RETRY_TRIM;
-
-        if (ide_handle_rw_error(s, -ret, op)) {
+        if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) {
             return;
         }
     }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 86bde26..68c7d0d 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -513,6 +513,21 @@ struct IDEDevice {
 #define IDE_RETRY_TRIM 0x80
 #define IDE_RETRY_HBA  0x100
 
+static inline uint8_t ide_dma_cmd_to_retry(uint8_t dma_cmd)
+{
+    switch (dma_cmd) {
+    case IDE_DMA_READ:
+        return IDE_RETRY_DMA | IDE_RETRY_READ;
+    case IDE_DMA_WRITE:
+        return IDE_RETRY_DMA;
+    case IDE_DMA_TRIM:
+        return IDE_RETRY_DMA | IDE_RETRY_TRIM;
+    default:
+        break;
+    }
+    return 0;
+}
+
 static inline IDEState *idebus_active_if(IDEBus *bus)
 {
     return bus->ifs + bus->unit;
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 92ffee7..8d56a00 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -308,6 +308,10 @@ static void ide_bmdma_pre_save(void *opaque)
     BMDMAState *bm = opaque;
     uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
 
+    if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
+        bm->bus->error_status =
+            ide_dma_cmd_to_retry(bmdma_active_if(bm)->dma_cmd);
+    }
     bm->migration_retry_unit = bm->bus->retry_unit;
     bm->migration_retry_sector_num = bm->bus->retry_sector_num;
     bm->migration_retry_nsector = bm->bus->retry_nsector;
-- 
2.1.4