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