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

[Qemu-devel] [PATCH 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.

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 loose 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    | 33 +++++++++++++++++++--------------
 hw/ide/core.c     | 32 ++++++++++++++++++--------------
 hw/ide/internal.h | 25 +++++++++++++++++++++++++
 hw/ide/pci.c      |  3 +++
 4 files changed, 65 insertions(+), 28 deletions(-)

[Qemu-devel] [PATCH 1/3] ide: don't loose 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 calling the DMA callback with an
artificial error code in pre_save if the callback is already set but
DMAING is clear. The callback then sets bus->error_status to indicate
the need for restart; however since DMAING is clear the state upon
restore will be exactly "ready forDMA" as before the save.

This patch only fixes the IDE DMA case; the ATAPI DMA is left with a stub
for now since its restart hadn't been implemented yet. It will be
addressed in the followup patch.

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    | 9 ++++++++-
 hw/ide/core.c     | 9 ++++++++-
 hw/ide/internal.h | 7 +++++++
 hw/ide/pci.c      | 3 +++
 4 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 1fe58ab..268220d 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -381,7 +381,14 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
     IDEState *s = opaque;
     int data_offset, n;
 
-    if (ret < 0) {
+    if (ret < 0) { /* XXX: handle -ESTATESAVE for migration */
+        if (ret == -ESTATESAVE) {
+            /*
+             * This case is not really an error
+             * but a request to save the state.
+             */
+            return;
+        }
         ide_atapi_io_error(s, ret);
         goto eot;
     }
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 241e840..872e11f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -810,7 +810,14 @@ static void ide_dma_cb(void *opaque, int ret)
         else if (s->dma_cmd == IDE_DMA_TRIM)
             op |= IDE_RETRY_TRIM;
 
-        if (ide_handle_rw_error(s, -ret, op)) {
+        if (ret == -ESTATESAVE) {
+            /*
+             * This case is not really an error
+             * but a request to save the state.
+             */
+            s->bus->error_status = op;
+            return;
+        } else if (ide_handle_rw_error(s, -ret, op)) {
             return;
         }
     }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 86bde26..dcd8627 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -513,6 +513,13 @@ struct IDEDevice {
 #define IDE_RETRY_TRIM 0x80
 #define IDE_RETRY_HBA  0x100
 
+/*
+ * a code to trigger entering error path and save/restore the "ready to DMA"
+ * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not supposed to
+ * come from the block layer
+ */
+#define ESTATESAVE EINPROGRESS
+
 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..e1f89fa 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -308,6 +308,9 @@ 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->dma_cb(bmdma_active_if(bm), -ESTATESAVE);
+    }
     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 268220d..8816b38 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -495,14 +495,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.

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     | 23 ++++++++++-------------
 hw/ide/internal.h | 18 ++++++++++++++++++
 3 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 8816b38..0d51b44 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -375,22 +375,25 @@ 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) { /* XXX: handle -ESTATESAVE for migration */
+    if (ret < 0) {
         if (ret == -ESTATESAVE) {
             /*
              * This case is not really an error
              * but a request to save the state.
              */
+            s->bus->error_status = IDE_RETRY_ATAPI;
             return;
+        } else if (ide_handle_rw_error(s, -ret, IDE_RETRY_ATAPI)) {
+            if (s->bus->error_status) {
+                return;
+            }
+            goto eot;
         }
-        ide_atapi_io_error(s, ret);
-        goto eot;
     }
 
     if (s->io_buffer_size > 0) {
@@ -488,10 +491,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 872e11f..6d7533f 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);
         }
@@ -2533,13 +2534,13 @@ static void ide_restart_bh(void *opaque)
         }
     }
 
-    if (error_status & IDE_RETRY_DMA) {
+    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 {
@@ -2547,15 +2548,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 dcd8627..2e845d0 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -508,11 +508,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))
+
 /*
  * a code to trigger entering error path and save/restore the "ready to DMA"
  * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not supposed to
@@ -604,4 +620,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 3/3] ide: really restart pending and in-flight atapi dma

[John Snow]

On 03/23/2016 06:26 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.
> 
> 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     | 23 ++++++++++-------------
>  hw/ide/internal.h | 18 ++++++++++++++++++
>  3 files changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 8816b38..0d51b44 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -375,22 +375,25 @@ 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) { /* XXX: handle -ESTATESAVE for migration */
> +    if (ret < 0) {
>          if (ret == -ESTATESAVE) {
>              /*
>               * This case is not really an error
>               * but a request to save the state.
>               */
> +            s->bus->error_status = IDE_RETRY_ATAPI;
>              return;
> +        } else if (ide_handle_rw_error(s, -ret, IDE_RETRY_ATAPI)) {
> +            if (s->bus->error_status) {
> +                return;
> +            }
> +            goto eot;
>          }
> -        ide_atapi_io_error(s, ret);
> -        goto eot;
>      }
>  
>      if (s->io_buffer_size > 0) {
> @@ -488,10 +491,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 872e11f..6d7533f 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);
>          }
> @@ -2533,13 +2534,13 @@ static void ide_restart_bh(void *opaque)
>          }
>      }
>  
> -    if (error_status & IDE_RETRY_DMA) {
> +    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 {
> @@ -2547,15 +2548,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 dcd8627..2e845d0 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -508,11 +508,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))
> +
>  /*
>   * a code to trigger entering error path and save/restore the "ready to DMA"
>   * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not supposed to
> @@ -604,4 +620,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, this causes "make check" to fail on the AHCI test.

Try this:

export QTEST_QEMU_BINARY ./x86_64-softmmu/qemu-system-x86_64
make tests/ahci-test
./tests/ahci-test -p /x86_64/ahci/io/ncq/retry

The QEMU instance aborts in hw/ide/core.c line 2555,
function 'ide_restart_bh'

    } else {
        abort(); <-- here.
    }

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

[Paolo Bonzini]

On 23/03/2016 11:26, Denis V. Lunev wrote:
> 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 calling the DMA callback with an
> artificial error code in pre_save if the callback is already set but
> DMAING is clear. The callback then sets bus->error_status to indicate
> the need for restart; however since DMAING is clear the state upon
> restore will be exactly "ready forDMA" as before the save.

Could you just use the dma_cmd field to build the error_status?

For extra points, make ide_handle_rw_error convert IDE_DMA_* to
IDE_RETRY_* so that the callers only need to pass in IDE_RETRY_DMA (like
they only need to pass in IDE_RETRY_PIO).

Paolo

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

[Pavel Butsykin]

On 23.03.2016 22:36, John Snow wrote:
>
>
> On 03/23/2016 06:26 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.
>>
>> 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     | 23 ++++++++++-------------
>>   hw/ide/internal.h | 18 ++++++++++++++++++
>>   3 files changed, 35 insertions(+), 21 deletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 8816b38..0d51b44 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -375,22 +375,25 @@ 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) { /* XXX: handle -ESTATESAVE for migration */
>> +    if (ret < 0) {
>>           if (ret == -ESTATESAVE) {
>>               /*
>>                * This case is not really an error
>>                * but a request to save the state.
>>                */
>> +            s->bus->error_status = IDE_RETRY_ATAPI;
>>               return;
>> +        } else if (ide_handle_rw_error(s, -ret, IDE_RETRY_ATAPI)) {
>> +            if (s->bus->error_status) {
>> +                return;
>> +            }
>> +            goto eot;
>>           }
>> -        ide_atapi_io_error(s, ret);
>> -        goto eot;
>>       }
>>
>>       if (s->io_buffer_size > 0) {
>> @@ -488,10 +491,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 872e11f..6d7533f 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);
>>           }
>> @@ -2533,13 +2534,13 @@ static void ide_restart_bh(void *opaque)
>>           }
>>       }
>>
>> -    if (error_status & IDE_RETRY_DMA) {
>> +    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 {
>> @@ -2547,15 +2548,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 dcd8627..2e845d0 100644
>> --- a/hw/ide/internal.h
>> +++ b/hw/ide/internal.h
>> @@ -508,11 +508,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))
>> +
>>   /*
>>    * a code to trigger entering error path and save/restore the "ready to DMA"
>>    * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not supposed to
>> @@ -604,4 +620,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, this causes "make check" to fail on the AHCI test.
>
> Try this:
>
> export QTEST_QEMU_BINARY ./x86_64-softmmu/qemu-system-x86_64
> make tests/ahci-test
> ./tests/ahci-test -p /x86_64/ahci/io/ncq/retry
>
> The QEMU instance aborts in hw/ide/core.c line 2555,
> function 'ide_restart_bh'
>
>      } else {
>          abort(); <-- here.
>      }
>
>
Thank you, it seems that I have missed the case for the IDE_RETRY_HBA.

     /* The HBA has generically asked to be kicked on retry */
     if (error_status & IDE_RETRY_HBA) {
         if (s->bus->dma->ops->restart) {
             s->bus->dma->ops->restart(s->bus->dma);
         }
     }
This case uses only to the AHCI. Why are we after the processing of
the IDE_RETRY_HBA case still need to handle other IDE_RETRY_* cases?
It seems that here we just need to get out from the ide_restart_bh function.

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

[Pavel Butsykin]

On 23.03.2016 23:34, Paolo Bonzini wrote:
>
>
> On 23/03/2016 11:26, Denis V. Lunev wrote:
>> 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 calling the DMA callback with an
>> artificial error code in pre_save if the callback is already set but
>> DMAING is clear. The callback then sets bus->error_status to indicate
>> the need for restart; however since DMAING is clear the state upon
>> restore will be exactly "ready forDMA" as before the save.
>
> Could you just use the dma_cmd field to build the error_status?
>
> For extra points, make ide_handle_rw_error convert IDE_DMA_* to
> IDE_RETRY_* so that the callers only need to pass in IDE_RETRY_DMA (like
> they only need to pass in IDE_RETRY_PIO).
>
> Paolo
>
You mean to do something like that:

ide_handle_rw_error(s, -ret, s->dma_cmd | IDE_RETRY_DMA)

?

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

[Paolo Bonzini]

On 24/03/2016 10:24, Pavel Butsykin wrote:
>> For extra points, make ide_handle_rw_error convert IDE_DMA_* to
>> IDE_RETRY_* so that the callers only need to pass in IDE_RETRY_DMA
>>
> You mean to do something like that:
> 
> ide_handle_rw_error(s, -ret, s->dma_cmd | IDE_RETRY_DMA)

Just ide_handle_rw_error(s, -ret, IDE_RETRY_DMA)

and in ide_handle_rw_error

    if (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;
    }

But I'm not sure anymore this is a good idea.

However, if it works, using dma_cmd would be better than using the dma_cb.

Paolo

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

[Pavel Butsykin]

On 24.03.2016 14:23, Paolo Bonzini wrote:
>
>
> On 24/03/2016 10:24, Pavel Butsykin wrote:
>>> For extra points, make ide_handle_rw_error convert IDE_DMA_* to
>>> IDE_RETRY_* so that the callers only need to pass in IDE_RETRY_DMA
>>>
>> You mean to do something like that:
>>
>> ide_handle_rw_error(s, -ret, s->dma_cmd | IDE_RETRY_DMA)
>
> Just ide_handle_rw_error(s, -ret, IDE_RETRY_DMA)
>
> and in ide_handle_rw_error
>
>      if (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;
>      }
>
> But I'm not sure anymore this is a good idea.
>
Look at the ide_sector_read_cb function. The ->dma_cmd field is used 
only for the DMA, we can't use the ide_cmd_is_read macro for PIO or
ATAPI. In fact, the ide_handle_rw_error looks like a generic function
and I do not see any good reason to move the handling of the dma_cmd
field(especially in the context of bug fixes).

> However, if it works, using dma_cmd would be better than using the dma_cb.
>
We can't use the dma_cmd field to build the ATAPI ->error_status. Your
proposal is quite logical, but please look at the 3rd patch, there to
build ->error_status we can use the same dma_cb, it is very convenient.

> Paolo
>

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

[Paolo Bonzini]

On 24/03/2016 14:38, Pavel Butsykin wrote:
> Look at the ide_sector_read_cb function. The ->dma_cmd field is used
> only for the DMA, we can't use the ide_cmd_is_read macro for PIO or
> ATAPI. In fact, the ide_handle_rw_error looks like a generic function
> and I do not see any good reason to move the handling of the dma_cmd
> field(especially in the context of bug fixes).

I agree now.

>> However, if it works, using dma_cmd would be better than using the
>> dma_cb.
>>
> We can't use the dma_cmd field to build the ATAPI ->error_status. Your
> proposal is quite logical, but please look at the 3rd patch, there to
> build ->error_status we can use the same dma_cb, it is very convenient.

Couldn't you just add a new dma_cmd value IDE_DMA_ATAPI, and set it
before every call to ide_start_dma(s, ide_atapi_cmd_read_dma_cb)?

Thanks,

Paolo

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 346 bytes --]

On 03/24/2016 03:24 AM, Pavel Butsykin wrote:
> On 23.03.2016 23:34, Paolo Bonzini wrote:

s/loose/lose/ in the subject line (lose rhymes with "use" and deals with
loss; loose rhymes with "goose" and is the opposite of tighten)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

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

[Pavel Butsykin]

On 24.03.2016 17:07, Paolo Bonzini wrote:
>
>
> On 24/03/2016 14:38, Pavel Butsykin wrote:
>> Look at the ide_sector_read_cb function. The ->dma_cmd field is used
>> only for the DMA, we can't use the ide_cmd_is_read macro for PIO or
>> ATAPI. In fact, the ide_handle_rw_error looks like a generic function
>> and I do not see any good reason to move the handling of the dma_cmd
>> field(especially in the context of bug fixes).
>
> I agree now.
>
>>> However, if it works, using dma_cmd would be better than using the
>>> dma_cb.
>>>
>> We can't use the dma_cmd field to build the ATAPI ->error_status. Your
>> proposal is quite logical, but please look at the 3rd patch, there to
>> build ->error_status we can use the same dma_cb, it is very convenient.
>
> Couldn't you just add a new dma_cmd value IDE_DMA_ATAPI, and set it
> before every call to ide_start_dma(s, ide_atapi_cmd_read_dma_cb)?
>
You want something like this:
dma_cb()
{
...
     if (ret < 0) {
         if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s))) {
             return;
         }
     }
...
}

static void ide_bmdma_pre_save(void *opaque)
{
...
     if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
         bm->bus->error_status = ide_dma_cmd_to_retry(bmdma_active_if(bm));
     }
...
}

??

> Thanks,
>
> Paolo
>

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

[Paolo Bonzini]

On 24/03/2016 16:11, Pavel Butsykin wrote:
>>
> You want something like this:
> dma_cb()
> {
> ...
>     if (ret < 0) {
>         if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s))) {
>             return;
>         }
>     }
> ...
> }
> 
> static void ide_bmdma_pre_save(void *opaque)
> {
> ...
>     if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
>         bm->bus->error_status = ide_dma_cmd_to_retry(bmdma_active_if(bm));
>     }
> ...
> }

Yes, that would do.

Paolo

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

[Pavel Butsykin]

On 24.03.2016 18:12, Paolo Bonzini wrote:
>
>
> On 24/03/2016 16:11, Pavel Butsykin wrote:
>>>
>> You want something like this:
>> dma_cb()
>> {
>> ...
>>      if (ret < 0) {
>>          if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s))) {
>>              return;
>>          }
>>      }
>> ...
>> }
>>
>> static void ide_bmdma_pre_save(void *opaque)
>> {
>> ...
>>      if (!(bm->status & BM_STATUS_DMAING) && bm->dma_cb) {
>>          bm->bus->error_status = ide_dma_cmd_to_retry(bmdma_active_if(bm));
>>      }
>> ...
>> }
>
> Yes, that would do.
>
Well, I can do it, looks quite ok.

> Paolo
>

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

[Pavel Butsykin]

On 24.03.2016 17:47, Eric Blake wrote:
> On 03/24/2016 03:24 AM, Pavel Butsykin wrote:
>> On 23.03.2016 23:34, Paolo Bonzini wrote:
>
> s/loose/lose/ in the subject line (lose rhymes with "use" and deals with
> loss; loose rhymes with "goose" and is the opposite of tighten)
>
>
thanks)

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

[John Snow]

On 03/23/2016 06:26 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 calling the DMA callback with an
> artificial error code in pre_save if the callback is already set but
> DMAING is clear. The callback then sets bus->error_status to indicate
> the need for restart; however since DMAING is clear the state upon
> restore will be exactly "ready forDMA" as before the save.
> 
> This patch only fixes the IDE DMA case; the ATAPI DMA is left with a stub
> for now since its restart hadn't been implemented yet. It will be
> addressed in the followup patch.
> 
> 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    | 9 ++++++++-
>  hw/ide/core.c     | 9 ++++++++-
>  hw/ide/internal.h | 7 +++++++
>  hw/ide/pci.c      | 3 +++
>  4 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 1fe58ab..268220d 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -381,7 +381,14 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
>      IDEState *s = opaque;
>      int data_offset, n;
>  
> -    if (ret < 0) {
> +    if (ret < 0) { /* XXX: handle -ESTATESAVE for migration */
> +        if (ret == -ESTATESAVE) {
> +            /*
> +             * This case is not really an error
> +             * but a request to save the state.
> +             */
> +            return;
> +        }
>          ide_atapi_io_error(s, ret);
>          goto eot;
>      }
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 241e840..872e11f 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -810,7 +810,14 @@ static void ide_dma_cb(void *opaque, int ret)
>          else if (s->dma_cmd == IDE_DMA_TRIM)
>              op |= IDE_RETRY_TRIM;
>  
> -        if (ide_handle_rw_error(s, -ret, op)) {
> +        if (ret == -ESTATESAVE) {
> +            /*
> +             * This case is not really an error
> +             * but a request to save the state.
> +             */
> +            s->bus->error_status = op;
> +            return;
> +        } else if (ide_handle_rw_error(s, -ret, op)) {
>              return;
>          }
>      }
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 86bde26..dcd8627 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -513,6 +513,13 @@ struct IDEDevice {
>  #define IDE_RETRY_TRIM 0x80
>  #define IDE_RETRY_HBA  0x100
>  
> +/*
> + * a code to trigger entering error path and save/restore the "ready to DMA"
> + * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not supposed to
> + * come from the block layer
> + */
> +#define ESTATESAVE EINPROGRESS
> +
>  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..e1f89fa 100644
> --- a/hw/ide/pci.c
> +++ b/hw/ide/pci.c
> @@ -308,6 +308,9 @@ 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->dma_cb(bmdma_active_if(bm), -ESTATESAVE);
> +    }
>      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;
> 

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

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

[Denis V. Lunev]

On 03/30/2016 08:35 PM, John Snow wrote:
>
> On 03/23/2016 06:26 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 calling the DMA callback with an
>> artificial error code in pre_save if the callback is already set but
>> DMAING is clear. The callback then sets bus->error_status to indicate
>> the need for restart; however since DMAING is clear the state upon
>> restore will be exactly "ready forDMA" as before the save.
>>
>> This patch only fixes the IDE DMA case; the ATAPI DMA is left with a stub
>> for now since its restart hadn't been implemented yet. It will be
>> addressed in the followup patch.
>>
>> 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    | 9 ++++++++-
>>   hw/ide/core.c     | 9 ++++++++-
>>   hw/ide/internal.h | 7 +++++++
>>   hw/ide/pci.c      | 3 +++
>>   4 files changed, 26 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>> index 1fe58ab..268220d 100644
>> --- a/hw/ide/atapi.c
>> +++ b/hw/ide/atapi.c
>> @@ -381,7 +381,14 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
>>       IDEState *s = opaque;
>>       int data_offset, n;
>>   
>> -    if (ret < 0) {
>> +    if (ret < 0) { /* XXX: handle -ESTATESAVE for migration */
>> +        if (ret == -ESTATESAVE) {
>> +            /*
>> +             * This case is not really an error
>> +             * but a request to save the state.
>> +             */
>> +            return;
>> +        }
>>           ide_atapi_io_error(s, ret);
>>           goto eot;
>>       }
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 241e840..872e11f 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -810,7 +810,14 @@ static void ide_dma_cb(void *opaque, int ret)
>>           else if (s->dma_cmd == IDE_DMA_TRIM)
>>               op |= IDE_RETRY_TRIM;
>>   
>> -        if (ide_handle_rw_error(s, -ret, op)) {
>> +        if (ret == -ESTATESAVE) {
>> +            /*
>> +             * This case is not really an error
>> +             * but a request to save the state.
>> +             */
>> +            s->bus->error_status = op;
>> +            return;
>> +        } else if (ide_handle_rw_error(s, -ret, op)) {
>>               return;
>>           }
>>       }
>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>> index 86bde26..dcd8627 100644
>> --- a/hw/ide/internal.h
>> +++ b/hw/ide/internal.h
>> @@ -513,6 +513,13 @@ struct IDEDevice {
>>   #define IDE_RETRY_TRIM 0x80
>>   #define IDE_RETRY_HBA  0x100
>>   
>> +/*
>> + * a code to trigger entering error path and save/restore the "ready to DMA"
>> + * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not supposed to
>> + * come from the block layer
>> + */
>> +#define ESTATESAVE EINPROGRESS
>> +
>>   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..e1f89fa 100644
>> --- a/hw/ide/pci.c
>> +++ b/hw/ide/pci.c
>> @@ -308,6 +308,9 @@ 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->dma_cb(bmdma_active_if(bm), -ESTATESAVE);
>> +    }
>>       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;
>>
> Reviewed-by: John Snow <jsnow@redhat.com>
there is never patch sent [PATCH 1/3] ide: don't lose pending dma state 
28.03
as v2.

Den

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

[John Snow]

On 03/30/2016 01:38 PM, Denis V. Lunev wrote:
> On 03/30/2016 08:35 PM, John Snow wrote:
>>
>> On 03/23/2016 06:26 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 calling the DMA callback with an
>>> artificial error code in pre_save if the callback is already set but
>>> DMAING is clear. The callback then sets bus->error_status to indicate
>>> the need for restart; however since DMAING is clear the state upon
>>> restore will be exactly "ready forDMA" as before the save.
>>>
>>> This patch only fixes the IDE DMA case; the ATAPI DMA is left with a
>>> stub
>>> for now since its restart hadn't been implemented yet. It will be
>>> addressed in the followup patch.
>>>
>>> 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    | 9 ++++++++-
>>>   hw/ide/core.c     | 9 ++++++++-
>>>   hw/ide/internal.h | 7 +++++++
>>>   hw/ide/pci.c      | 3 +++
>>>   4 files changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
>>> index 1fe58ab..268220d 100644
>>> --- a/hw/ide/atapi.c
>>> +++ b/hw/ide/atapi.c
>>> @@ -381,7 +381,14 @@ static void ide_atapi_cmd_read_dma_cb(void
>>> *opaque, int ret)
>>>       IDEState *s = opaque;
>>>       int data_offset, n;
>>>   -    if (ret < 0) {
>>> +    if (ret < 0) { /* XXX: handle -ESTATESAVE for migration */
>>> +        if (ret == -ESTATESAVE) {
>>> +            /*
>>> +             * This case is not really an error
>>> +             * but a request to save the state.
>>> +             */
>>> +            return;
>>> +        }
>>>           ide_atapi_io_error(s, ret);
>>>           goto eot;
>>>       }
>>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>>> index 241e840..872e11f 100644
>>> --- a/hw/ide/core.c
>>> +++ b/hw/ide/core.c
>>> @@ -810,7 +810,14 @@ static void ide_dma_cb(void *opaque, int ret)
>>>           else if (s->dma_cmd == IDE_DMA_TRIM)
>>>               op |= IDE_RETRY_TRIM;
>>>   -        if (ide_handle_rw_error(s, -ret, op)) {
>>> +        if (ret == -ESTATESAVE) {
>>> +            /*
>>> +             * This case is not really an error
>>> +             * but a request to save the state.
>>> +             */
>>> +            s->bus->error_status = op;
>>> +            return;
>>> +        } else if (ide_handle_rw_error(s, -ret, op)) {
>>>               return;
>>>           }
>>>       }
>>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>>> index 86bde26..dcd8627 100644
>>> --- a/hw/ide/internal.h
>>> +++ b/hw/ide/internal.h
>>> @@ -513,6 +513,13 @@ struct IDEDevice {
>>>   #define IDE_RETRY_TRIM 0x80
>>>   #define IDE_RETRY_HBA  0x100
>>>   +/*
>>> + * a code to trigger entering error path and save/restore the "ready
>>> to DMA"
>>> + * state just like DMA-ing state. (Ab)use EINPROGRESS as it's not
>>> supposed to
>>> + * come from the block layer
>>> + */
>>> +#define ESTATESAVE EINPROGRESS
>>> +
>>>   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..e1f89fa 100644
>>> --- a/hw/ide/pci.c
>>> +++ b/hw/ide/pci.c
>>> @@ -308,6 +308,9 @@ 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->dma_cb(bmdma_active_if(bm), -ESTATESAVE);
>>> +    }
>>>       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;
>>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
> there is never patch sent [PATCH 1/3] ide: don't lose pending dma state
> 28.03
> as v2.
> 
> Den

My mistake. I tested the new one but replied to the wrong mail.

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

[John Snow]

On 04/12/2016 08:17 AM, Pavel Butsykin wrote:
> On 12.04.2016 01:18, Eric Blake wrote:
>> On 04/06/2016 12:40 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>>

and these seem prone to false positives; where it might be better to do:

>>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>>> ---
>>
>> I'll leave the technical feasibility of this to others, but have some
>> coding style comments:
>>
>>
>>> @@ -783,8 +782,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);
>>
>> I'd probably have split this into two patches; one adding the accessor
>> macros for existing access, and the other adding the new bit pattern
>> (mixing a conversion along with new code is a bit trickier to review in
>> one patch).
>>
>>
>>> +++ 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
>>
>> Please keep the trailing comma, so that the next addition to this enum
>> won't have to adjust an existing line.
>>
> ok.
> 
>>>   };
>>>
>>>   #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
>>
>> Seems rather sparse on the comments about which bit patterns are valid
>> together.  If IDE_RETRY_READ is always used with at least one other bit,
>> it might make more sense to have an IDE_RETRY_MASK that selects the set
>> of bits being multiplexed, and/or macros that define the bits used in
>> combination.  Something along the lines of:
>>
>> #define IDE_RETRY_MASK        0x38
>> #define IDE_RETRY_READ_DMA    0x28
>> #define IDE_RETRY_READ_PIO    0x30
>> #define IDE_RETRY_ATAPI       0x20
>>
>>>
>>> +#define IS_IDE_RETRY_DMA(_status) \
>>> +    ((_status) & IDE_RETRY_DMA)
>>> +
>>> +#define IS_IDE_RETRY_PIO(_status) \
>>> +    ((_status) & IDE_RETRY_PIO)
>>
>> and these seem prone to false positives; where it might be better to do:
>>
>> #define IS_IDE_RETRY_DMA(_status) \
>>      (((_status) & IDE_RETRY_MASK) == IDE_RETRY_READ_DMA)
>>
> This is not entirely true, because IDE_RETRY_DMA can be used for READ or
> WRITE operation.
> 
>>> +
>>> +/*
>>> + * 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))
>>> +
>>
>> And this evaluates _status more than once, compared to:
>>
>> #define IS_IDE_RETRY_ATAPI(_status) \
>>      (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI)
>>
>>
> Yes, it looks much nicer. I can make the change as a follow-up patch.
> 

I can squash the patch in staging.

Thanks,
--js

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

[Pavel Butsykin]

On 12.04.2016 01:18, Eric Blake wrote:
> On 04/06/2016 12:40 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>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> ---
>
> I'll leave the technical feasibility of this to others, but have some
> coding style comments:
>
>
>> @@ -783,8 +782,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);
>
> I'd probably have split this into two patches; one adding the accessor
> macros for existing access, and the other adding the new bit pattern
> (mixing a conversion along with new code is a bit trickier to review in
> one patch).
>
>
>> +++ 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
>
> Please keep the trailing comma, so that the next addition to this enum
> won't have to adjust an existing line.
>
ok.

>>   };
>>
>>   #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
>
> Seems rather sparse on the comments about which bit patterns are valid
> together.  If IDE_RETRY_READ is always used with at least one other bit,
> it might make more sense to have an IDE_RETRY_MASK that selects the set
> of bits being multiplexed, and/or macros that define the bits used in
> combination.  Something along the lines of:
>
> #define IDE_RETRY_MASK        0x38
> #define IDE_RETRY_READ_DMA    0x28
> #define IDE_RETRY_READ_PIO    0x30
> #define IDE_RETRY_ATAPI       0x20
>
>>
>> +#define IS_IDE_RETRY_DMA(_status) \
>> +    ((_status) & IDE_RETRY_DMA)
>> +
>> +#define IS_IDE_RETRY_PIO(_status) \
>> +    ((_status) & IDE_RETRY_PIO)
>
> and these seem prone to false positives; where it might be better to do:
>
> #define IS_IDE_RETRY_DMA(_status) \
>      (((_status) & IDE_RETRY_MASK) == IDE_RETRY_READ_DMA)
>
This is not entirely true, because IDE_RETRY_DMA can be used for READ or
WRITE operation.

>> +
>> +/*
>> + * 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))
>> +
>
> And this evaluates _status more than once, compared to:
>
> #define IS_IDE_RETRY_ATAPI(_status) \
>      (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI)
>
>
Yes, it looks much nicer. I can make the change as a follow-up patch.

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3826 bytes --]

On 04/06/2016 12:40 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>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---

I'll leave the technical feasibility of this to others, but have some
coding style comments:


> @@ -783,8 +782,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);

I'd probably have split this into two patches; one adding the accessor
macros for existing access, and the other adding the new bit pattern
(mixing a conversion along with new code is a bit trickier to review in
one patch).


> +++ 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

Please keep the trailing comma, so that the next addition to this enum
won't have to adjust an existing line.

>  };
>  
>  #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

Seems rather sparse on the comments about which bit patterns are valid
together.  If IDE_RETRY_READ is always used with at least one other bit,
it might make more sense to have an IDE_RETRY_MASK that selects the set
of bits being multiplexed, and/or macros that define the bits used in
combination.  Something along the lines of:

#define IDE_RETRY_MASK        0x38
#define IDE_RETRY_READ_DMA    0x28
#define IDE_RETRY_READ_PIO    0x30
#define IDE_RETRY_ATAPI       0x20

>  
> +#define IS_IDE_RETRY_DMA(_status) \
> +    ((_status) & IDE_RETRY_DMA)
> +
> +#define IS_IDE_RETRY_PIO(_status) \
> +    ((_status) & IDE_RETRY_PIO)

and these seem prone to false positives; where it might be better to do:

#define IS_IDE_RETRY_DMA(_status) \
    (((_status) & IDE_RETRY_MASK) == IDE_RETRY_READ_DMA)

> +
> +/*
> + * 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))
> +

And this evaluates _status more than once, compared to:

#define IS_IDE_RETRY_ATAPI(_status) \
    (((_status) & IDE_RETRY_MASK) == IDE_RETRY_ATAPI)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[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>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 hw/ide/atapi.c    | 13 ++++++-------
 hw/ide/core.c     | 30 +++++++++++++++---------------
 hw/ide/internal.h | 21 +++++++++++++++++++++
 hw/ide/macio.c    |  2 ++
 4 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index acc52cd..2bb606c 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -375,15 +375,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) {
@@ -481,10 +484,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 58d0687..41e6a2d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -57,7 +57,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)
@@ -773,7 +772,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);
@@ -783,8 +782,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);
         }
@@ -872,6 +873,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;
 
@@ -1634,6 +1637,9 @@ static bool cmd_packet(IDEState *s, uint8_t cmd)
 
     s->status = READY_STAT | SEEK_STAT;
     s->atapi_dma = s->feature & 1;
+    if (s->atapi_dma) {
+        s->dma_cmd = IDE_DMA_ATAPI;
+    }
     s->nsector = 1;
     ide_transfer_start(s, s->io_buffer, ATAPI_PACKET_SIZE,
                        ide_atapi_cmd);
@@ -2518,15 +2524,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 {
@@ -2534,15 +2538,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 */
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 1725e5b..76256eb 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -346,6 +346,8 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     case IDE_DMA_TRIM:
         pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
         break;
+    default:
+        abort();
     }
 
     return;
-- 
2.1.4

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

[John Snow]

On 04/04/2016 12:54 PM, Pavel Butsykin wrote:
> On 04.04.2016 19:24, John Snow wrote:
>>
>>
>> On 04/04/2016 06:32 AM, Pavel Butsykin wrote:
>>> On 02.04.2016 01:34, John Snow wrote:
>>>>
>>>>
>>>> On 04/01/2016 10:32 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>
>>>>> ---
>>>>>    hw/ide/atapi.c    | 15 ++++++++-------
>>>>>    hw/ide/core.c     | 27 ++++++++++++---------------
>>>>>    hw/ide/internal.h | 21 +++++++++++++++++++++
>>>>>    hw/ide/macio.c    |  2 ++
>>>>>    4 files changed, 43 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;
>>>>
>>>> You could even set this as far back as cmd_packet in core.c if you
>>>> wanted, I think.
>>>>
>>>>>            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;
>>>>> +        }
>>>>
>>>> There's probably no actually decent way to handle this with our current
>>>> design, but part of the issue here is that the ATAPI module here is not
>>>> supposed to be an "IDE" device as such, so making calls to commands
>>>> that
>>>> manipulate IDE registers is a little fuzzy.
>>>>
>>>> In this case, we've basically hardcoded callbacks to ide_atapi_io_error
>>>> inside of the core function so it all comes out the same, but the
>>>> design
>>>> is still sort of fuzzy.
>>>>
>>>> I'll grant you that this ENTIRE design of the ATAPI module is crap to
>>>> start with, though, so... it's fine for now.
>>>>
>>>> :(
>>>>
>>>>>        }
>>>>>
>>>>>        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;
>>>>
>>>> Yeah, if we just filter this earlier, we won't need to set it in
>>>> multiple places I think.
>>>>
>>>>>        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 */
>>>>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>>>>> index 1725e5b..76256eb 100644
>>>>> --- a/hw/ide/macio.c
>>>>> +++ b/hw/ide/macio.c
>>>>> @@ -346,6 +346,8 @@ static void pmac_ide_transfer_cb(void *opaque,
>>>>> int ret)
>>>>>        case IDE_DMA_TRIM:
>>>>>            pmac_dma_trim(s->blk, offset, io->len,
>>>>> pmac_ide_transfer_cb, io);
>>>>>            break;
>>>>> +    default:
>>>>> +        abort();
>>>>>        }
>>>>>
>>>>>        return;
>>>>>
>>>>
>>>> Good, I think this is workable. Thank you for fixing this. I'll try to
>>>> squeeze it in for rc1 so we can get some testing done on it.
>>>
>>> Thank you for review. What it means for these patches? Do you accept
>>> patches or want something else?
>>
>> Can you comment on the different places where you're setting s->dma_cmd?
>> If it can moved forward into e.g. cmd_packet() I think that's better
>> than trying to catch it inside of the different execution paths.
>>
>> You can set it in advance and then rely on the error functions to clear
>> it out if we don't get as far as actually DMAing any information.
>>
>> I'd like to do that if possible, unless you know of some technical
>> reason why we can't.
>>
> This can be done, because the main condition is that there is the
> presence of the ->atapi_dma flag.
> 

Send that as a V4 and I'll stage it, thanks!

--js

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

[Pavel Butsykin]

On 04.04.2016 19:24, John Snow wrote:
>
>
> On 04/04/2016 06:32 AM, Pavel Butsykin wrote:
>> On 02.04.2016 01:34, John Snow wrote:
>>>
>>>
>>> On 04/01/2016 10:32 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>
>>>> ---
>>>>    hw/ide/atapi.c    | 15 ++++++++-------
>>>>    hw/ide/core.c     | 27 ++++++++++++---------------
>>>>    hw/ide/internal.h | 21 +++++++++++++++++++++
>>>>    hw/ide/macio.c    |  2 ++
>>>>    4 files changed, 43 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;
>>>
>>> You could even set this as far back as cmd_packet in core.c if you
>>> wanted, I think.
>>>
>>>>            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;
>>>> +        }
>>>
>>> There's probably no actually decent way to handle this with our current
>>> design, but part of the issue here is that the ATAPI module here is not
>>> supposed to be an "IDE" device as such, so making calls to commands that
>>> manipulate IDE registers is a little fuzzy.
>>>
>>> In this case, we've basically hardcoded callbacks to ide_atapi_io_error
>>> inside of the core function so it all comes out the same, but the design
>>> is still sort of fuzzy.
>>>
>>> I'll grant you that this ENTIRE design of the ATAPI module is crap to
>>> start with, though, so... it's fine for now.
>>>
>>> :(
>>>
>>>>        }
>>>>
>>>>        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;
>>>
>>> Yeah, if we just filter this earlier, we won't need to set it in
>>> multiple places I think.
>>>
>>>>        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 */
>>>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>>>> index 1725e5b..76256eb 100644
>>>> --- a/hw/ide/macio.c
>>>> +++ b/hw/ide/macio.c
>>>> @@ -346,6 +346,8 @@ static void pmac_ide_transfer_cb(void *opaque,
>>>> int ret)
>>>>        case IDE_DMA_TRIM:
>>>>            pmac_dma_trim(s->blk, offset, io->len,
>>>> pmac_ide_transfer_cb, io);
>>>>            break;
>>>> +    default:
>>>> +        abort();
>>>>        }
>>>>
>>>>        return;
>>>>
>>>
>>> Good, I think this is workable. Thank you for fixing this. I'll try to
>>> squeeze it in for rc1 so we can get some testing done on it.
>>
>> Thank you for review. What it means for these patches? Do you accept
>> patches or want something else?
>
> Can you comment on the different places where you're setting s->dma_cmd?
> If it can moved forward into e.g. cmd_packet() I think that's better
> than trying to catch it inside of the different execution paths.
>
> You can set it in advance and then rely on the error functions to clear
> it out if we don't get as far as actually DMAing any information.
>
> I'd like to do that if possible, unless you know of some technical
> reason why we can't.
>
This can be done, because the main condition is that there is the
presence of the ->atapi_dma flag.

> --js
>

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

[John Snow]

On 04/04/2016 06:32 AM, Pavel Butsykin wrote:
> On 02.04.2016 01:34, John Snow wrote:
>>
>>
>> On 04/01/2016 10:32 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>
>>> ---
>>>   hw/ide/atapi.c    | 15 ++++++++-------
>>>   hw/ide/core.c     | 27 ++++++++++++---------------
>>>   hw/ide/internal.h | 21 +++++++++++++++++++++
>>>   hw/ide/macio.c    |  2 ++
>>>   4 files changed, 43 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;
>>
>> You could even set this as far back as cmd_packet in core.c if you
>> wanted, I think.
>>
>>>           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;
>>> +        }
>>
>> There's probably no actually decent way to handle this with our current
>> design, but part of the issue here is that the ATAPI module here is not
>> supposed to be an "IDE" device as such, so making calls to commands that
>> manipulate IDE registers is a little fuzzy.
>>
>> In this case, we've basically hardcoded callbacks to ide_atapi_io_error
>> inside of the core function so it all comes out the same, but the design
>> is still sort of fuzzy.
>>
>> I'll grant you that this ENTIRE design of the ATAPI module is crap to
>> start with, though, so... it's fine for now.
>>
>> :(
>>
>>>       }
>>>
>>>       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;
>>
>> Yeah, if we just filter this earlier, we won't need to set it in
>> multiple places I think.
>>
>>>       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 */
>>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>>> index 1725e5b..76256eb 100644
>>> --- a/hw/ide/macio.c
>>> +++ b/hw/ide/macio.c
>>> @@ -346,6 +346,8 @@ static void pmac_ide_transfer_cb(void *opaque,
>>> int ret)
>>>       case IDE_DMA_TRIM:
>>>           pmac_dma_trim(s->blk, offset, io->len,
>>> pmac_ide_transfer_cb, io);
>>>           break;
>>> +    default:
>>> +        abort();
>>>       }
>>>
>>>       return;
>>>
>>
>> Good, I think this is workable. Thank you for fixing this. I'll try to
>> squeeze it in for rc1 so we can get some testing done on it.
> 
> Thank you for review. What it means for these patches? Do you accept
> patches or want something else?

Can you comment on the different places where you're setting s->dma_cmd?
If it can moved forward into e.g. cmd_packet() I think that's better
than trying to catch it inside of the different execution paths.

You can set it in advance and then rely on the error functions to clear
it out if we don't get as far as actually DMAing any information.

I'd like to do that if possible, unless you know of some technical
reason why we can't.

--js

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

[Pavel Butsykin]

On 02.04.2016 01:34, John Snow wrote:
>
>
> On 04/01/2016 10:32 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>
>> ---
>>   hw/ide/atapi.c    | 15 ++++++++-------
>>   hw/ide/core.c     | 27 ++++++++++++---------------
>>   hw/ide/internal.h | 21 +++++++++++++++++++++
>>   hw/ide/macio.c    |  2 ++
>>   4 files changed, 43 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;
>
> You could even set this as far back as cmd_packet in core.c if you
> wanted, I think.
>
>>           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;
>> +        }
>
> There's probably no actually decent way to handle this with our current
> design, but part of the issue here is that the ATAPI module here is not
> supposed to be an "IDE" device as such, so making calls to commands that
> manipulate IDE registers is a little fuzzy.
>
> In this case, we've basically hardcoded callbacks to ide_atapi_io_error
> inside of the core function so it all comes out the same, but the design
> is still sort of fuzzy.
>
> I'll grant you that this ENTIRE design of the ATAPI module is crap to
> start with, though, so... it's fine for now.
>
> :(
>
>>       }
>>
>>       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;
>
> Yeah, if we just filter this earlier, we won't need to set it in
> multiple places I think.
>
>>       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 */
>> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
>> index 1725e5b..76256eb 100644
>> --- a/hw/ide/macio.c
>> +++ b/hw/ide/macio.c
>> @@ -346,6 +346,8 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>>       case IDE_DMA_TRIM:
>>           pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
>>           break;
>> +    default:
>> +        abort();
>>       }
>>
>>       return;
>>
>
> Good, I think this is workable. Thank you for fixing this. I'll try to
> squeeze it in for rc1 so we can get some testing done on it.

Thank you for review. What it means for these patches? Do you accept
patches or want something else?
>
> --js
>

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

[John Snow]

On 04/01/2016 10:32 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>
> ---
>  hw/ide/atapi.c    | 15 ++++++++-------
>  hw/ide/core.c     | 27 ++++++++++++---------------
>  hw/ide/internal.h | 21 +++++++++++++++++++++
>  hw/ide/macio.c    |  2 ++
>  4 files changed, 43 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;

You could even set this as far back as cmd_packet in core.c if you
wanted, I think.

>          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;
> +        }

There's probably no actually decent way to handle this with our current
design, but part of the issue here is that the ATAPI module here is not
supposed to be an "IDE" device as such, so making calls to commands that
manipulate IDE registers is a little fuzzy.

In this case, we've basically hardcoded callbacks to ide_atapi_io_error
inside of the core function so it all comes out the same, but the design
is still sort of fuzzy.

I'll grant you that this ENTIRE design of the ATAPI module is crap to
start with, though, so... it's fine for now.

:(

>      }
>  
>      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;

Yeah, if we just filter this earlier, we won't need to set it in
multiple places I think.

>      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 */
> diff --git a/hw/ide/macio.c b/hw/ide/macio.c
> index 1725e5b..76256eb 100644
> --- a/hw/ide/macio.c
> +++ b/hw/ide/macio.c
> @@ -346,6 +346,8 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
>      case IDE_DMA_TRIM:
>          pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
>          break;
> +    default:
> +        abort();
>      }
>  
>      return;
> 

Good, I think this is workable. Thank you for fixing this. I'll try to
squeeze it in for rc1 so we can get some testing done on it.

--js

[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>
---
 hw/ide/atapi.c    | 15 ++++++++-------
 hw/ide/core.c     | 27 ++++++++++++---------------
 hw/ide/internal.h | 21 +++++++++++++++++++++
 hw/ide/macio.c    |  2 ++
 4 files changed, 43 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 */
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 1725e5b..76256eb 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -346,6 +346,8 @@ static void pmac_ide_transfer_cb(void *opaque, int ret)
     case IDE_DMA_TRIM:
         pmac_dma_trim(s->blk, offset, io->len, pmac_ide_transfer_cb, io);
         break;
+    default:
+        abort();
     }
 
     return;
-- 
2.1.4

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