[Qemu-devel] [WIP PATCH 00/24] IDE cleanups, initial work on AHCI rerror/werror=stop

[Qemu-devel] [WIP PATCH 00/24] IDE cleanups, initial work on AHCI rerror/werror=stop

[Paolo Bonzini]

Hi all,

this is some cleanup to the IDE code that I started long ago
and picked up again on my flight back from KVM Forum...

The patches clean up the callbacks to prepare for implementing request
retry for AHCI.  After these patches, PIO retry should work for AHCI and
non-NCQ retry should be very close (you need an implementation of the
new restart_dma callback).  NCQ retry and save/restore is AHCI-specific
and has to be done separately, similar to how we do it for SCSI already.

Patches 1-4 set up a new (incomplete) testcase.  Patches 5-13 are no-op
cleanups, or so I hope.  Patches 14-23 genericize retry and make it
"just work" for all adapters for PIO.  Patch 24 is another cleanup that
could probably be moved earlier in the series.

Throwing it out since I know Kevin and mst are working on AHCI, to avoid
doing duplicate work.  Patches 2 and 4 are known to be racy and
incomplete.  Extremely light testing, if something breaks tells me and
I'll look at it.

Paolo

Paolo Bonzini (24):
  blkdebug: report errors on flush too
  libqtest: return progress from qmp/qmpv
  libqtest: add QTEST_LOG for debugging qtest testcases
  ide-test: add test for werror=stop
  ide: simplify reset callbacks
  ide: simplify set_inactive callbacks
  ide: simplify async_cmd_done callbacks
  ide: simplify start_transfer callbacks
  ide: wrap start_dma callback
  ide: add trigger_irq callback
  ide: fold add_status callback into set_inactive
  ide: move BM_STATUS bits to pci.[ch]
  ide: move retry constants out of BM_STATUS_* namespace
  ide: start extracting ide_restart_dma out of bmdma_restart_dma
  ide: prepare to move restart to common code
  ide: introduce ide_register_restart_cb
  ide: do not use BMDMA in restart callback
  ide: pass IDEBus to the restart_cb
  ide: move restart callback to common code
  ide: remove restart_cb callback
  ide: replace set_unit callback with more IDEBus state
  ide: place initial state of the current request to IDEBus
  ide: migrate initial request state via IDEBus
  ide: commonize io_buffer_index initialization

 block/blkdebug.c  |  20 ++++++++
 hw/ide/ahci.c     |  45 +++-------------
 hw/ide/atapi.c    |  14 +++--
 hw/ide/cmd646.c   |   3 +-
 hw/ide/core.c     | 151 +++++++++++++++++++++++++++++++++++++++---------------
 hw/ide/internal.h |  48 ++++++++---------
 hw/ide/macio.c    |  15 ------
 hw/ide/pci.c      | 130 +++++++++++-----------------------------------
 hw/ide/pci.h      |  19 ++++---
 hw/ide/piix.c     |   3 +-
 hw/ide/via.c      |   3 +-
 tests/ide-test.c  |  74 ++++++++++++++++++++++++++
 tests/libqtest.c  |  17 ++++--
 tests/libqtest.h  |  17 +++---
 14 files changed, 307 insertions(+), 252 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 01/24] blkdebug: report errors on flush too

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/blkdebug.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 16d2b91..4626c43 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -484,6 +484,25 @@ static BlockDriverAIOCB *blkdebug_aio_writev(BlockDriverState *bs,
     return bdrv_aio_writev(bs->file, sector_num, qiov, nb_sectors, cb, opaque);
 }
 
+static BlockDriverAIOCB *blkdebug_aio_flush(BlockDriverState *bs,
+    BlockDriverCompletionFunc *cb, void *opaque)
+{
+    BDRVBlkdebugState *s = bs->opaque;
+    BlkdebugRule *rule = NULL;
+
+    QSIMPLEQ_FOREACH(rule, &s->active_rules, active_next) {
+        if (rule->options.inject.sector == -1) {
+            break;
+        }
+    }
+
+    if (rule && rule->options.inject.error) {
+        return inject_error(bs, cb, opaque, rule);
+    }
+
+    return bdrv_aio_flush(bs->file, cb, opaque);
+}
+
 
 static void blkdebug_close(BlockDriverState *bs)
 {
@@ -636,6 +655,7 @@ static BlockDriver bdrv_blkdebug = {
 
     .bdrv_aio_readv         = blkdebug_aio_readv,
     .bdrv_aio_writev        = blkdebug_aio_writev,
+    .bdrv_aio_flush         = blkdebug_aio_flush,
 
     .bdrv_debug_event           = blkdebug_debug_event,
     .bdrv_debug_breakpoint      = blkdebug_debug_breakpoint,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 01/24] blkdebug: report errors on flush too

[Kevin Wolf]

Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/blkdebug.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 02/24] libqtest: return progress from qmp/qmpv

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/libqtest.c | 10 +++++++---
 tests/libqtest.h | 17 +++++++++++------
 2 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index bb82069..5205a43 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -291,7 +291,7 @@ redo:
     return words;
 }
 
-void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
+bool qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
 {
     bool has_reply = false;
     int nesting = 0;
@@ -324,15 +324,19 @@ void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
             break;
         }
     }
+    return has_reply;
 }
 
-void qtest_qmp(QTestState *s, const char *fmt, ...)
+bool qtest_qmp(QTestState *s, const char *fmt, ...)
 {
     va_list ap;
+    bool has_reply;
 
     va_start(ap, fmt);
-    qtest_qmpv(s, fmt, ap);
+    has_reply = qtest_qmpv(s, fmt, ap);
     va_end(ap);
+
+    return has_reply;
 }
 
 const char *qtest_get_arch(void)
diff --git a/tests/libqtest.h b/tests/libqtest.h
index a6e99bd..e8a4e34 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -48,9 +48,10 @@ void qtest_quit(QTestState *s);
  * @s: #QTestState instance to operate on.
  * @fmt...: QMP message to send to qemu
  *
- * Sends a QMP message to QEMU
+ * Sends a QMP message to QEMU.  Returns true if there
+ * was a reply.
  */
-void qtest_qmp(QTestState *s, const char *fmt, ...);
+bool qtest_qmp(QTestState *s, const char *fmt, ...);
 
 /**
  * qtest_qmpv:
@@ -58,9 +59,10 @@ void qtest_qmp(QTestState *s, const char *fmt, ...);
  * @fmt: QMP message to send to QEMU
  * @ap: QMP message arguments
  *
- * Sends a QMP message to QEMU.
+ * Sends a QMP message to QEMU.  Returns true if there
+ * was a reply.
  */
-void qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
+bool qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
 
 /**
  * qtest_get_irq:
@@ -336,13 +338,16 @@ static inline void qtest_end(void)
  *
  * Sends a QMP message to QEMU
  */
-static inline void qmp(const char *fmt, ...)
+static inline bool qmp(const char *fmt, ...)
 {
     va_list ap;
+    bool has_reply;
 
     va_start(ap, fmt);
-    qtest_qmpv(global_qtest, fmt, ap);
+    has_reply = qtest_qmpv(global_qtest, fmt, ap);
     va_end(ap);
+
+    return has_reply;
 }
 
 /**
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 02/24] libqtest: return progress from qmp/qmpv

[Kevin Wolf]

Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/libqtest.c | 10 +++++++---
>  tests/libqtest.h | 17 +++++++++++------
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/libqtest.c b/tests/libqtest.c
> index bb82069..5205a43 100644
> --- a/tests/libqtest.c
> +++ b/tests/libqtest.c
> @@ -291,7 +291,7 @@ redo:
>      return words;
>  }
>  
> -void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
> +bool qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
>  {
>      bool has_reply = false;
>      int nesting = 0;
> @@ -324,15 +324,19 @@ void qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
>              break;
>          }
>      }
> +    return has_reply;
>  }
>  
> -void qtest_qmp(QTestState *s, const char *fmt, ...)
> +bool qtest_qmp(QTestState *s, const char *fmt, ...)
>  {
>      va_list ap;
> +    bool has_reply;
>  
>      va_start(ap, fmt);
> -    qtest_qmpv(s, fmt, ap);
> +    has_reply = qtest_qmpv(s, fmt, ap);
>      va_end(ap);
> +
> +    return has_reply;
>  }
>  
>  const char *qtest_get_arch(void)
> diff --git a/tests/libqtest.h b/tests/libqtest.h
> index a6e99bd..e8a4e34 100644
> --- a/tests/libqtest.h
> +++ b/tests/libqtest.h
> @@ -48,9 +48,10 @@ void qtest_quit(QTestState *s);
>   * @s: #QTestState instance to operate on.
>   * @fmt...: QMP message to send to qemu
>   *
> - * Sends a QMP message to QEMU
> + * Sends a QMP message to QEMU.  Returns true if there
> + * was a reply.
>   */
> -void qtest_qmp(QTestState *s, const char *fmt, ...);
> +bool qtest_qmp(QTestState *s, const char *fmt, ...);
>  
>  /**
>   * qtest_qmpv:
> @@ -58,9 +59,10 @@ void qtest_qmp(QTestState *s, const char *fmt, ...);
>   * @fmt: QMP message to send to QEMU
>   * @ap: QMP message arguments
>   *
> - * Sends a QMP message to QEMU.
> + * Sends a QMP message to QEMU.  Returns true if there
> + * was a reply.
>   */
> -void qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
> +bool qtest_qmpv(QTestState *s, const char *fmt, va_list ap);
>  
>  /**
>   * qtest_get_irq:
> @@ -336,13 +338,16 @@ static inline void qtest_end(void)
>   *
>   * Sends a QMP message to QEMU
>   */
> -static inline void qmp(const char *fmt, ...)
> +static inline bool qmp(const char *fmt, ...)
>  {
>      va_list ap;
> +    bool has_reply;
>  
>      va_start(ap, fmt);
> -    qtest_qmpv(global_qtest, fmt, ap);
> +    has_reply = qtest_qmpv(global_qtest, fmt, ap);
>      va_end(ap);
> +
> +    return has_reply;
>  }

Can this ever return false? If there isn't a reply, doesn't it wait
until it receives one?

Anyway, I think Stefan had some patches to actually get the reply in a
usable way. They probably conflict with this. Perhaps just applying
Stefan's patches already gives you what you need here?

Kevin

Re: [Qemu-devel] [PATCH 02/24] libqtest: return progress from qmp/qmpv

[Paolo Bonzini]

Il 30/10/2013 13:06, Kevin Wolf ha scritto:
> Anyway, I think Stefan had some patches to actually get the reply in a
> usable way. They probably conflict with this. Perhaps just applying
> Stefan's patches already gives you what you need here?

Yes, especially if they let me get both events and command returns.

Paolo

[Qemu-devel] [PATCH 03/24] libqtest: add QTEST_LOG for debugging qtest testcases

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/libqtest.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 5205a43..6e42d01 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -129,11 +129,12 @@ QTestState *qtest_init(const char *extra_args)
     if (pid == 0) {
         command = g_strdup_printf("%s "
                                   "-qtest unix:%s,nowait "
-                                  "-qtest-log /dev/null "
+                                  "-qtest-log %s "
                                   "-qmp unix:%s,nowait "
                                   "-pidfile %s "
                                   "-machine accel=qtest "
                                   "%s", qemu_binary, s->socket_path,
+                                  getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
                                   s->qmp_socket_path, pid_file,
                                   extra_args ?: "");
         execlp("/bin/sh", "sh", "-c", command, NULL);
@@ -295,6 +296,7 @@ bool qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
 {
     bool has_reply = false;
     int nesting = 0;
+    int log = getenv("QTEST_LOG") != NULL;
 
     /* Send QMP request */
     socket_sendf(s->qmp_fd, fmt, ap);
@@ -314,6 +316,9 @@ bool qtest_qmpv(QTestState *s, const char *fmt, va_list ap)
             exit(1);
         }
 
+        if (log) {
+            len = write(2, &c, 1);
+        }
         switch (c) {
         case '{':
             nesting++;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 03/24] libqtest: add QTEST_LOG for debugging qtest testcases

[Kevin Wolf]

Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  tests/libqtest.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

Nice idea, this looks useful.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[Qemu-devel] [PATCH 04/24] ide-test: add test for werror=stop

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 tests/ide-test.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 7307f1d..560dd2c 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -105,6 +105,7 @@ static QPCIBus *pcibus = NULL;
 static QGuestAllocator *guest_malloc;
 
 static char tmp_path[] = "/tmp/qtest.XXXXXX";
+static char debug_path[] = "/tmp/qtest-blkdebug.XXXXXX";
 
 static void ide_test_start(const char *cmdline_fmt, ...)
 {
@@ -465,6 +466,72 @@ static void test_flush(void)
     ide_test_quit();
 }
 
+static void prepare_blkdebug_script(const char *debug_path, const char *event)
+{
+    FILE *debug_file = fopen(debug_path, "w");
+    int ret;
+
+    fprintf(debug_file, "[inject-error]\n");
+    fprintf(debug_file, "event = \"%s\"\n", event);
+    fprintf(debug_file, "errno = \"5\"\n");
+    fprintf(debug_file, "state = \"1\"\n");
+    fprintf(debug_file, "immediately = \"off\"\n");
+    fprintf(debug_file, "once = \"on\"\n");
+
+    fprintf(debug_file, "[set-state]\n");
+    fprintf(debug_file, "event = \"%s\"\n", event);
+    fprintf(debug_file, "new_state = \"2\"\n");
+    fflush(debug_file);
+    g_assert(!ferror(debug_file));
+
+    ret = fclose(debug_file);
+    g_assert(ret == 0);
+}
+
+static void test_retry_flush(void)
+{
+    uint8_t data;
+    const char *s;
+
+    prepare_blkdebug_script(debug_path, "flush_to_disk");
+
+    ide_test_start(
+        "-vnc none "
+        "-drive file=blkdebug:%s:%s,if=ide,cache=writeback,rerror=stop,werror=stop",
+        debug_path, tmp_path);
+
+    /* FLUSH CACHE command on device 0*/
+    outb(IDE_BASE + reg_device, 0);
+    outb(IDE_BASE + reg_command, CMD_FLUSH_CACHE);
+
+    /* Check status while request is in flight*/
+    data = inb(IDE_BASE + reg_status);
+    assert_bit_set(data, BSY | DRDY);
+    assert_bit_clear(data, DF | ERR | DRQ);
+
+    sleep(1);                    /* HACK: wait for event */
+
+    /* Complete the command */
+    s = "{'execute':'cont' }";
+    while (!qmp(s)) {
+        s = "";
+        sleep(1);
+    }
+
+    /* Check registers */
+    data = inb(IDE_BASE + reg_device);
+    g_assert_cmpint(data & DEV, ==, 0);
+
+    do {
+        data = inb(IDE_BASE + reg_status);
+    } while (data & BSY);
+
+    assert_bit_set(data, DRDY);
+    assert_bit_clear(data, BSY | DF | ERR | DRQ);
+
+    ide_test_quit();
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -477,6 +544,11 @@ int main(int argc, char **argv)
         return 0;
     }
 
+    /* Create temporary blkdebug instructions */
+    fd = mkstemp(debug_path);
+    g_assert(fd >= 0);
+    close(fd);
+
     /* Create a temporary raw image */
     fd = mkstemp(tmp_path);
     g_assert(fd >= 0);
@@ -497,6 +569,8 @@ int main(int argc, char **argv)
 
     qtest_add_func("/ide/flush", test_flush);
 
+    qtest_add_func("/ide/retry/flush", test_retry_flush);
+
     ret = g_test_run();
 
     /* Cleanup */
-- 
1.8.3.1

[Qemu-devel] [PATCH 05/24] ide: simplify reset callbacks

[Paolo Bonzini]

Drop the unused return value and make the callback optional.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c     | 6 ------
 hw/ide/core.c     | 5 +++--
 hw/ide/internal.h | 3 ++-
 hw/ide/macio.c    | 1 -
 hw/ide/pci.c      | 4 +---
 5 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index a8be62c..d74b6ab 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1137,11 +1137,6 @@ static void ahci_dma_restart_cb(void *opaque, int running, RunState state)
 {
 }
 
-static int ahci_dma_reset(IDEDMA *dma)
-{
-    return 0;
-}
-
 static const IDEDMAOps ahci_dma_ops = {
     .start_dma = ahci_start_dma,
     .start_transfer = ahci_start_transfer,
@@ -1152,7 +1147,6 @@ static const IDEDMAOps ahci_dma_ops = {
     .set_inactive = ahci_dma_set_inactive,
     .async_cmd_done = ahci_async_cmd_done,
     .restart_cb = ahci_dma_restart_cb,
-    .reset = ahci_dma_reset,
 };
 
 void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 399b1ba..8437ead 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2057,7 +2057,9 @@ void ide_bus_reset(IDEBus *bus)
     }
 
     /* reset dma provider too */
-    bus->dma->ops->reset(bus->dma);
+    if (bus->dma->ops->reset) {
+        bus->dma->ops->reset(bus->dma);
+    }
 }
 
 static bool ide_cd_is_tray_open(void *opaque)
@@ -2195,7 +2197,6 @@ static const IDEDMAOps ide_dma_nop_ops = {
     .add_status     = ide_nop_int,
     .set_inactive   = ide_nop,
     .restart_cb     = ide_nop_restart,
-    .reset          = ide_nop,
 };
 
 static IDEDMA ide_dma_nop = {
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 5d1cf87..84f3a7a 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -320,6 +320,7 @@ typedef enum { IDE_HD, IDE_CD, IDE_CFATA } IDEDriveKind;
 typedef void EndTransferFunc(IDEState *);
 
 typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockDriverCompletionFunc *);
+typedef void DMAVoidFunc(IDEDMA *);
 typedef int DMAFunc(IDEDMA *);
 typedef int DMAIntFunc(IDEDMA *, int);
 typedef void DMARestartFunc(void *, int, RunState);
@@ -435,7 +436,7 @@ struct IDEDMAOps {
     DMAFunc *set_inactive;
     DMAFunc *async_cmd_done;
     DMARestartFunc *restart_cb;
-    DMAFunc *reset;
+    DMAVoidFunc *reset;
 };
 
 struct IDEDMA {
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index da94580..b3cb6ca 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -528,7 +528,6 @@ static const IDEDMAOps dbdma_ops = {
     .add_status     = ide_nop_int,
     .set_inactive   = ide_nop,
     .restart_cb     = ide_nop_restart,
-    .reset          = ide_nop,
 };
 
 static void macio_ide_realizefn(DeviceState *dev, Error **errp)
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 91151fc..a9eb5bf 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -247,7 +247,7 @@ static void bmdma_cancel(BMDMAState *bm)
     }
 }
 
-static int bmdma_reset(IDEDMA *dma)
+static void bmdma_reset(IDEDMA *dma)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
 
@@ -264,8 +264,6 @@ static int bmdma_reset(IDEDMA *dma)
     bm->cur_prd_len = 0;
     bm->sector_num = 0;
     bm->nsector = 0;
-
-    return 0;
 }
 
 static int bmdma_start_transfer(IDEDMA *dma)
-- 
1.8.3.1

[Qemu-devel] [PATCH 06/24] ide: simplify set_inactive callbacks

[Paolo Bonzini]

Drop the unused return value and make the callback optional.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c     | 8 --------
 hw/ide/core.c     | 5 +++--
 hw/ide/internal.h | 2 +-
 hw/ide/macio.c    | 1 -
 hw/ide/pci.c      | 4 +---
 5 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index d74b6ab..265e3f2 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1106,11 +1106,6 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
     return 0;
 }
 
-static int ahci_dma_set_inactive(IDEDMA *dma)
-{
-    return 0;
-}
-
 static int ahci_async_cmd_done(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
@@ -1125,8 +1120,6 @@ static int ahci_async_cmd_done(IDEDMA *dma)
         ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
         qemu_bh_schedule(ad->check_bh);
     }
-
-    return 0;
 }
 
 static void ahci_irq_set(void *opaque, int n, int level)
@@ -1144,7 +1137,6 @@ static const IDEDMAOps ahci_dma_ops = {
     .rw_buf = ahci_dma_rw_buf,
     .set_unit = ahci_dma_set_unit,
     .add_status = ahci_dma_add_status,
-    .set_inactive = ahci_dma_set_inactive,
     .async_cmd_done = ahci_async_cmd_done,
     .restart_cb = ahci_dma_restart_cb,
 };
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 8437ead..926b90a 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -578,7 +578,9 @@ static void ide_async_cmd_done(IDEState *s)
 void ide_set_inactive(IDEState *s)
 {
     s->bus->dma->aiocb = NULL;
-    s->bus->dma->ops->set_inactive(s->bus->dma);
+    if (s->bus->dma->ops->set_inactive) {
+        s->bus->dma->ops->set_inactive(s->bus->dma);
+    }
     ide_async_cmd_done(s);
 }
 
@@ -2195,7 +2197,6 @@ static const IDEDMAOps ide_dma_nop_ops = {
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
     .add_status     = ide_nop_int,
-    .set_inactive   = ide_nop,
     .restart_cb     = ide_nop_restart,
 };
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 84f3a7a..7c68f8e 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -433,7 +433,7 @@ struct IDEDMAOps {
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
     DMAIntFunc *add_status;
-    DMAFunc *set_inactive;
+    DMAVoidFunc *set_inactive;
     DMAFunc *async_cmd_done;
     DMARestartFunc *restart_cb;
     DMAVoidFunc *reset;
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index b3cb6ca..013326d 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -526,7 +526,6 @@ static const IDEDMAOps dbdma_ops = {
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
     .add_status     = ide_nop_int,
-    .set_inactive   = ide_nop,
     .restart_cb     = ide_nop_restart,
 };
 
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index a9eb5bf..b781149 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -160,15 +160,13 @@ static int bmdma_add_status(IDEDMA *dma, int status)
     return 0;
 }
 
-static int bmdma_set_inactive(IDEDMA *dma)
+static void bmdma_set_inactive(IDEDMA *dma)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
 
     bm->status &= ~BM_STATUS_DMAING;
     bm->dma_cb = NULL;
     bm->unit = -1;
-
-    return 0;
 }
 
 static void bmdma_restart_dma(BMDMAState *bm, enum ide_dma_cmd dma_cmd)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 06/24] ide: simplify set_inactive callbacks

[Kevin Wolf]

Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
> Drop the unused return value and make the callback optional.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/ahci.c     | 8 --------
>  hw/ide/core.c     | 5 +++--
>  hw/ide/internal.h | 2 +-
>  hw/ide/macio.c    | 1 -
>  hw/ide/pci.c      | 4 +---
>  5 files changed, 5 insertions(+), 15 deletions(-)

> @@ -1125,8 +1120,6 @@ static int ahci_async_cmd_done(IDEDMA *dma)
>          ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
>          qemu_bh_schedule(ad->check_bh);
>      }
> -
> -    return 0;
>  }
>  
>  static void ahci_irq_set(void *opaque, int n, int level)

This hunk should be in patch 7.

Kevin

[Qemu-devel] [PATCH 07/24] ide: simplify async_cmd_done callbacks

[Paolo Bonzini]

Drop the unused return value.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c     | 2 +-
 hw/ide/internal.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 265e3f2..b4b8386 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1106,7 +1106,7 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
     return 0;
 }
 
-static int ahci_async_cmd_done(IDEDMA *dma)
+static void ahci_async_cmd_done(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 7c68f8e..43b8f43 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -434,7 +434,7 @@ struct IDEDMAOps {
     DMAIntFunc *set_unit;
     DMAIntFunc *add_status;
     DMAVoidFunc *set_inactive;
-    DMAFunc *async_cmd_done;
+    DMAVoidFunc *async_cmd_done;
     DMARestartFunc *restart_cb;
     DMAVoidFunc *reset;
 };
-- 
1.8.3.1

[Qemu-devel] [PATCH 08/24] ide: simplify start_transfer callbacks

[Paolo Bonzini]

Drop the unused return value and make the callback optional.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c     |  4 +---
 hw/ide/core.c     | 10 +++-------
 hw/ide/internal.h |  3 +--
 hw/ide/macio.c    |  6 ------
 hw/ide/pci.c      |  6 ------
 5 files changed, 5 insertions(+), 24 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index b4b8386..23f3f22 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -981,7 +981,7 @@ out:
 }
 
 /* DMA dev <-> ram */
-static int ahci_start_transfer(IDEDMA *dma)
+static void ahci_start_transfer(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
     IDEState *s = &ad->port.ifs[0];
@@ -1031,8 +1031,6 @@ out:
         /* done with DMA */
         ahci_trigger_irq(ad->hba, ad, PORT_IRQ_STAT_DSS);
     }
-
-    return 0;
 }
 
 static void ahci_start_dma(IDEDMA *dma, IDEState *s,
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 926b90a..e64dfff 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -434,7 +434,9 @@ void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
     if (!(s->status & ERR_STAT)) {
         s->status |= DRQ_STAT;
     }
-    s->bus->dma->ops->start_transfer(s->bus->dma);
+    if (s->bus->dma->ops->start_transfer) {
+        s->bus->dma->ops->start_transfer(s->bus->dma);
+    }
 }
 
 void ide_transfer_stop(IDEState *s)
@@ -2176,11 +2178,6 @@ static void ide_nop_start(IDEDMA *dma, IDEState *s,
 {
 }
 
-static int ide_nop(IDEDMA *dma)
-{
-    return 0;
-}
-
 static int ide_nop_int(IDEDMA *dma, int x)
 {
     return 0;
@@ -2192,7 +2189,6 @@ static void ide_nop_restart(void *opaque, int x, RunState y)
 
 static const IDEDMAOps ide_dma_nop_ops = {
     .start_dma      = ide_nop_start,
-    .start_transfer = ide_nop,
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 43b8f43..92d2351 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -321,7 +321,6 @@ typedef void EndTransferFunc(IDEState *);
 
 typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockDriverCompletionFunc *);
 typedef void DMAVoidFunc(IDEDMA *);
-typedef int DMAFunc(IDEDMA *);
 typedef int DMAIntFunc(IDEDMA *, int);
 typedef void DMARestartFunc(void *, int, RunState);
 
@@ -428,7 +427,7 @@ struct IDEState {
 
 struct IDEDMAOps {
     DMAStartFunc *start_dma;
-    DMAFunc *start_transfer;
+    DMAVoidFunc *start_transfer;
     DMAIntFunc *prepare_buf;
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 013326d..56e2b0c 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -495,11 +495,6 @@ static void macio_ide_reset(DeviceState *dev)
     ide_bus_reset(&d->bus);
 }
 
-static int ide_nop(IDEDMA *dma)
-{
-    return 0;
-}
-
 static int ide_nop_int(IDEDMA *dma, int x)
 {
     return 0;
@@ -521,7 +516,6 @@ static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
 
 static const IDEDMAOps dbdma_ops = {
     .start_dma      = ide_dbdma_start,
-    .start_transfer = ide_nop,
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b781149..78c47a3 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -264,11 +264,6 @@ static void bmdma_reset(IDEDMA *dma)
     bm->nsector = 0;
 }
 
-static int bmdma_start_transfer(IDEDMA *dma)
-{
-    return 0;
-}
-
 static void bmdma_irq(void *opaque, int n, int level)
 {
     BMDMAState *bm = opaque;
@@ -504,7 +499,6 @@ void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table)
 
 static const struct IDEDMAOps bmdma_ops = {
     .start_dma = bmdma_start_dma,
-    .start_transfer = bmdma_start_transfer,
     .prepare_buf = bmdma_prepare_buf,
     .rw_buf = bmdma_rw_buf,
     .set_unit = bmdma_set_unit,
-- 
1.8.3.1

[Qemu-devel] [PATCH 09/24] ide: wrap start_dma callback

[Paolo Bonzini]

Make it optional and prepare for the next patches.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/atapi.c    |  6 ++----
 hw/ide/core.c     | 15 ++++++++-------
 hw/ide/internal.h |  1 +
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 05e60b1..a7688bf 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -255,8 +255,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size)
     if (s->atapi_dma) {
         bdrv_acct_start(s->bs, &s->acct, size, BDRV_ACCT_READ);
         s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
-        s->bus->dma->ops->start_dma(s->bus->dma, s,
-                                   ide_atapi_cmd_read_dma_cb);
+        ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
     } else {
         s->status = READY_STAT | SEEK_STAT;
         ide_atapi_cmd_reply_end(s);
@@ -375,8 +374,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->bus->dma->ops->start_dma(s->bus->dma, s,
-                               ide_atapi_cmd_read_dma_cb);
+    ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
 }
 
 static void ide_atapi_cmd_read(IDEState *s, int lba, int nb_sectors,
diff --git a/hw/ide/core.c b/hw/ide/core.c
index e64dfff..bd4a9f2 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -722,7 +722,14 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
         break;
     }
 
-    s->bus->dma->ops->start_dma(s->bus->dma, s, ide_dma_cb);
+    ide_start_dma(s, ide_dma_cb);
+}
+
+void ide_start_dma(IDEState *s, BlockDriverCompletionFunc *cb)
+{
+    if (s->bus->dma->ops->start_dma) {
+        s->bus->dma->ops->start_dma(s->bus->dma, s, cb);
+    }
 }
 
 static void ide_sector_write_timer_cb(void *opaque)
@@ -2173,11 +2180,6 @@ static void ide_init1(IDEBus *bus, int unit)
                                            ide_sector_write_timer_cb, s);
 }
 
-static void ide_nop_start(IDEDMA *dma, IDEState *s,
-                          BlockDriverCompletionFunc *cb)
-{
-}
-
 static int ide_nop_int(IDEDMA *dma, int x)
 {
     return 0;
@@ -2188,7 +2190,6 @@ static void ide_nop_restart(void *opaque, int x, RunState y)
 }
 
 static const IDEDMAOps ide_dma_nop_ops = {
-    .start_dma      = ide_nop_start,
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 92d2351..92f25ef 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -532,6 +532,7 @@ void ide_bus_reset(IDEBus *bus);
 int64_t ide_get_sector(IDEState *s);
 void ide_set_sector(IDEState *s, int64_t sector_num);
 
+void ide_start_dma(IDEState *s, BlockDriverCompletionFunc *cb);
 void ide_dma_error(IDEState *s);
 
 void ide_atapi_cmd_ok(IDEState *s);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 09/24] ide: wrap start_dma callback

[Kevin Wolf]

Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
> Make it optional and prepare for the next patches.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/atapi.c    |  6 ++----
>  hw/ide/core.c     | 15 ++++++++-------
>  hw/ide/internal.h |  1 +
>  3 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index 05e60b1..a7688bf 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -255,8 +255,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size)
>      if (s->atapi_dma) {
>          bdrv_acct_start(s->bs, &s->acct, size, BDRV_ACCT_READ);
>          s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
> -        s->bus->dma->ops->start_dma(s->bus->dma, s,
> -                                   ide_atapi_cmd_read_dma_cb);
> +        ide_start_dma(s, ide_atapi_cmd_read_dma_cb);

I was wondering whether the s->status update should be moved into
ide_start_dma(). Then I noticed that the value is different here,
because it's lacking BSY. Probably an inconsistency that wouldn't hurt
to get rid of? (The spec says that during a DMA operation BSY or DRQ or
both must be set.)

Kevin

Re: [Qemu-devel] [PATCH 09/24] ide: wrap start_dma callback

[Paolo Bonzini]

Il 30/10/2013 13:29, Kevin Wolf ha scritto:
>> >          bdrv_acct_start(s->bs, &s->acct, size, BDRV_ACCT_READ);
>> >          s->status = READY_STAT | SEEK_STAT | DRQ_STAT;
>> > -        s->bus->dma->ops->start_dma(s->bus->dma, s,
>> > -                                   ide_atapi_cmd_read_dma_cb);
>> > +        ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
> I was wondering whether the s->status update should be moved into
> ide_start_dma(). Then I noticed that the value is different here,
> because it's lacking BSY. Probably an inconsistency that wouldn't hurt
> to get rid of? (The spec says that during a DMA operation BSY or DRQ or
> both must be set.)

You are probably right.  BTW, the last patch in the series does that for
another assignment that is common to all ide_start_dma call sites.

Paolo

[Qemu-devel] [PATCH 10/24] ide: add trigger_irq callback

[Paolo Bonzini]

AHCIDevice does not have a dma_status field.  The add_status callback thus
does not make sense, start moving its functionality to new callbacks.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c     | 12 ++++++++----
 hw/ide/atapi.c    |  4 +++-
 hw/ide/internal.h |  1 +
 hw/ide/pci.c      |  7 +++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 23f3f22..7b47053 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1097,13 +1097,16 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
     DPRINTF(ad->port_no, "set status: %x\n", status);
 
-    if (status & BM_STATUS_INT) {
-        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_STAT_DSS);
-    }
-
     return 0;
 }
 
+static void ahci_dma_trigger_irq(IDEDMA *dma)
+{
+    AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
+    DPRINTF(ad->port_no, "trigger irq\n");
+    ahci_trigger_irq(ad->hba, ad, PORT_IRQ_STAT_DSS);
+}
+
 static void ahci_async_cmd_done(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
@@ -1134,6 +1137,7 @@ static const IDEDMAOps ahci_dma_ops = {
     .prepare_buf = ahci_dma_prepare_buf,
     .rw_buf = ahci_dma_rw_buf,
     .set_unit = ahci_dma_set_unit,
+    .trigger_irq = ahci_dma_trigger_irq,
     .add_status = ahci_dma_add_status,
     .async_cmd_done = ahci_async_cmd_done,
     .restart_cb = ahci_dma_restart_cb,
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index a7688bf..814cffb 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -355,7 +355,9 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
 
 eot:
     bdrv_acct_done(s->bs, &s->acct);
-    s->bus->dma->ops->add_status(s->bus->dma, BM_STATUS_INT);
+    if (s->bus->dma->ops->trigger_irq) {
+        s->bus->dma->ops->trigger_irq(s->bus->dma);
+    }
     ide_set_inactive(s);
 }
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 92f25ef..08230be 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -432,6 +432,7 @@ struct IDEDMAOps {
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
     DMAIntFunc *add_status;
+    DMAVoidFunc *trigger_irq;
     DMAVoidFunc *set_inactive;
     DMAVoidFunc *async_cmd_done;
     DMARestartFunc *restart_cb;
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 78c47a3..68d560d 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -152,6 +152,12 @@ static int bmdma_set_unit(IDEDMA *dma, int unit)
     return 0;
 }
 
+static void bmdma_trigger_irq(IDEDMA *dma)
+{
+    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
+    bm->status |= BM_STATUS_INT;
+}
+
 static int bmdma_add_status(IDEDMA *dma, int status)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
@@ -502,6 +508,7 @@ static const struct IDEDMAOps bmdma_ops = {
     .prepare_buf = bmdma_prepare_buf,
     .rw_buf = bmdma_rw_buf,
     .set_unit = bmdma_set_unit,
+    .trigger_irq = bmdma_trigger_irq,
     .add_status = bmdma_add_status,
     .set_inactive = bmdma_set_inactive,
     .restart_cb = bmdma_restart_cb,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 10/24] ide: add trigger_irq callback

[Kevin Wolf]

Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
> AHCIDevice does not have a dma_status field.  The add_status callback thus
> does not make sense, start moving its functionality to new callbacks.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This whole add_status(BM_STATUS_INT) is weird and doesn't seem to make
sense. For AHCI it triggers the actual IRQ, whereas for BMIDE it only
sets the status register bit and requires a separate call to trigger the
IRQ. This function is ide_set_irq(), which in turn ends up completely
ignored by AHCI (ahci_irq_set() is an empty function).

I think the right approach would be to let AHCI handle ide_set_irq() and
remove the add_status/trigger_irq calls completely.

>  hw/ide/ahci.c     | 12 ++++++++----
>  hw/ide/atapi.c    |  4 +++-
>  hw/ide/internal.h |  1 +
>  hw/ide/pci.c      |  7 +++++++
>  4 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 23f3f22..7b47053 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1097,13 +1097,16 @@ static int ahci_dma_add_status(IDEDMA *dma, int status)
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
>      DPRINTF(ad->port_no, "set status: %x\n", status);
>  
> -    if (status & BM_STATUS_INT) {
> -        ahci_trigger_irq(ad->hba, ad, PORT_IRQ_STAT_DSS);
> -    }
> -
>      return 0;
>  }
>  
> +static void ahci_dma_trigger_irq(IDEDMA *dma)
> +{
> +    AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> +    DPRINTF(ad->port_no, "trigger irq\n");
> +    ahci_trigger_irq(ad->hba, ad, PORT_IRQ_STAT_DSS);
> +}
> +
>  static void ahci_async_cmd_done(IDEDMA *dma)
>  {
>      AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
> @@ -1134,6 +1137,7 @@ static const IDEDMAOps ahci_dma_ops = {
>      .prepare_buf = ahci_dma_prepare_buf,
>      .rw_buf = ahci_dma_rw_buf,
>      .set_unit = ahci_dma_set_unit,
> +    .trigger_irq = ahci_dma_trigger_irq,
>      .add_status = ahci_dma_add_status,
>      .async_cmd_done = ahci_async_cmd_done,
>      .restart_cb = ahci_dma_restart_cb,
> diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
> index a7688bf..814cffb 100644
> --- a/hw/ide/atapi.c
> +++ b/hw/ide/atapi.c
> @@ -355,7 +355,9 @@ static void ide_atapi_cmd_read_dma_cb(void *opaque, int ret)
>  
>  eot:
>      bdrv_acct_done(s->bs, &s->acct);
> -    s->bus->dma->ops->add_status(s->bus->dma, BM_STATUS_INT);
> +    if (s->bus->dma->ops->trigger_irq) {
> +        s->bus->dma->ops->trigger_irq(s->bus->dma);
> +    }
>      ide_set_inactive(s);
>  }

This function looks suspicious as well, because there seems to be a path
where trigger_irq() can be called without actually triggering an IRQ.

Kevin

Re: [Qemu-devel] [PATCH 10/24] ide: add trigger_irq callback

[Paolo Bonzini]

Il 30/10/2013 13:47, Kevin Wolf ha scritto:
> This whole add_status(BM_STATUS_INT) is weird and doesn't seem to make
> sense. For AHCI it triggers the actual IRQ, whereas for BMIDE it only
> sets the status register bit and requires a separate call to trigger the
> IRQ. This function is ide_set_irq(), which in turn ends up completely
> ignored by AHCI (ahci_irq_set() is an empty function).

The problem is that the IRQ ack is done differently in AHCI (using the
write-1-clears register PORT_IRQ_STAT; PORT_IRQ_MASK can also modify the
status of the interrupt line) and PATA (reading 0x1f7).

So perhaps the legacy PIO code of hw/ide/core.c should be abstracted
out, including the new restart support.  (BTW, to test this with AHCI I
rigged up a modified version of AHCI that has the legacy ports, so that
I could run the new qtest on it).

Michael, can you look at this?

Paolo

Re: [Qemu-devel] [PATCH 10/24] ide: add trigger_irq callback

[Kevin Wolf]

Am 30.10.2013 um 14:16 hat Paolo Bonzini geschrieben:
> Il 30/10/2013 13:47, Kevin Wolf ha scritto:
> > This whole add_status(BM_STATUS_INT) is weird and doesn't seem to make
> > sense. For AHCI it triggers the actual IRQ, whereas for BMIDE it only
> > sets the status register bit and requires a separate call to trigger the
> > IRQ. This function is ide_set_irq(), which in turn ends up completely
> > ignored by AHCI (ahci_irq_set() is an empty function).
> 
> The problem is that the IRQ ack is done differently in AHCI (using the
> write-1-clears register PORT_IRQ_STAT; PORT_IRQ_MASK can also modify the
> status of the interrupt line) and PATA (reading 0x1f7).

I don't understand how this is related; I'm really talking about
internal interfaces here. ide_set_irq() and .trigger_irq() are really
two different ways for the IDE core to communicate that an IRQ should be
triggered. They can in theory be used for the same things, but are used
inconsistently.

> So perhaps the legacy PIO code of hw/ide/core.c should be abstracted
> out, including the new restart support.  (BTW, to test this with AHCI I
> rigged up a modified version of AHCI that has the legacy ports, so that
> I could run the new qtest on it).

This could be a good idea anyway.

Kevin

Re: [Qemu-devel] [PATCH 10/24] ide: add trigger_irq callback

[Paolo Bonzini]

Il 30/10/2013 14:34, Kevin Wolf ha scritto:
>> > 
>> > The problem is that the IRQ ack is done differently in AHCI (using the
>> > write-1-clears register PORT_IRQ_STAT; PORT_IRQ_MASK can also modify the
>> > status of the interrupt line) and PATA (reading 0x1f7).
> I don't understand how this is related; I'm really talking about
> internal interfaces here. ide_set_irq() and .trigger_irq() are really
> two different ways for the IDE core to communicate that an IRQ should be
> triggered. They can in theory be used for the same things, but are used
> inconsistently.

Looking at the history of BM_STATUS_INT, I found this commit of yours:

    ide/core: Remove explicit setting of BM_STATUS_INT
    
    BM_STATUS_INT is automatically set during ide_set_irq(), there's no reason to
    set it manually in addition.
    
    There is even one case where the interrupt status bit was set, but no IRQ was
    raised. This is when the PRD table was reached but there is more data to
    transfer. The correct behaviour for this case is not to set BM_STATUS_INT.
    
    Signed-off-by: Kevin Wolf <kwolf@redhat.com>

(commit 69c38b8fcec823da05f98f6ea98ec2b0013d64e2).

It seems to me that the remaining use of BM_STATUS_INT, which this
patch changes to trigger_irq, is also due to a short PRD.  So the trigger_irq
callback can go away altogether.

Paolo

[Qemu-devel] [PATCH 11/24] ide: fold add_status callback into set_inactive

[Paolo Bonzini]

It is now called only after the set_inactive callback.  Put the two together.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c     |  9 ---------
 hw/ide/atapi.c    |  2 +-
 hw/ide/core.c     | 12 ++++--------
 hw/ide/internal.h |  6 +++---
 hw/ide/macio.c    |  1 -
 hw/ide/pci.c      | 19 +++++++------------
 6 files changed, 15 insertions(+), 34 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 7b47053..1d4fb17 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1092,14 +1092,6 @@ static int ahci_dma_set_unit(IDEDMA *dma, int unit)
     return 0;
 }
 
-static int ahci_dma_add_status(IDEDMA *dma, int status)
-{
-    AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
-    DPRINTF(ad->port_no, "set status: %x\n", status);
-
-    return 0;
-}
-
 static void ahci_dma_trigger_irq(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
@@ -1138,7 +1130,6 @@ static const IDEDMAOps ahci_dma_ops = {
     .rw_buf = ahci_dma_rw_buf,
     .set_unit = ahci_dma_set_unit,
     .trigger_irq = ahci_dma_trigger_irq,
-    .add_status = ahci_dma_add_status,
     .async_cmd_done = ahci_async_cmd_done,
     .restart_cb = ahci_dma_restart_cb,
 };
diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 814cffb..6b3f6c1 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -358,7 +358,7 @@ eot:
     if (s->bus->dma->ops->trigger_irq) {
         s->bus->dma->ops->trigger_irq(s->bus->dma);
     }
-    ide_set_inactive(s);
+    ide_set_inactive(s, false);
 }
 
 /* start a CD-CDROM read command with DMA */
diff --git a/hw/ide/core.c b/hw/ide/core.c
index bd4a9f2..a62654f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -577,11 +577,11 @@ static void ide_async_cmd_done(IDEState *s)
     }
 }
 
-void ide_set_inactive(IDEState *s)
+void ide_set_inactive(IDEState *s, bool more)
 {
     s->bus->dma->aiocb = NULL;
     if (s->bus->dma->ops->set_inactive) {
-        s->bus->dma->ops->set_inactive(s->bus->dma);
+        s->bus->dma->ops->set_inactive(s->bus->dma, more);
     }
     ide_async_cmd_done(s);
 }
@@ -591,7 +591,7 @@ void ide_dma_error(IDEState *s)
     ide_transfer_stop(s);
     s->error = ABRT_ERR;
     s->status = READY_STAT | ERR_STAT;
-    ide_set_inactive(s);
+    ide_set_inactive(s, false);
     ide_set_irq(s->bus);
 }
 
@@ -696,10 +696,7 @@ eot:
     if (s->dma_cmd == IDE_DMA_READ || s->dma_cmd == IDE_DMA_WRITE) {
         bdrv_acct_done(s->bs, &s->acct);
     }
-    ide_set_inactive(s);
-    if (stay_active) {
-        s->bus->dma->ops->add_status(s->bus->dma, BM_STATUS_DMAING);
-    }
+    ide_set_inactive(s, stay_active);
 }
 
 static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
@@ -2193,7 +2190,6 @@ static const IDEDMAOps ide_dma_nop_ops = {
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
-    .add_status     = ide_nop_int,
     .restart_cb     = ide_nop_restart,
 };
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 08230be..cd5e77c 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -322,6 +322,7 @@ typedef void EndTransferFunc(IDEState *);
 typedef void DMAStartFunc(IDEDMA *, IDEState *, BlockDriverCompletionFunc *);
 typedef void DMAVoidFunc(IDEDMA *);
 typedef int DMAIntFunc(IDEDMA *, int);
+typedef void DMAStopFunc(IDEDMA *, bool);
 typedef void DMARestartFunc(void *, int, RunState);
 
 struct unreported_events {
@@ -431,9 +432,8 @@ struct IDEDMAOps {
     DMAIntFunc *prepare_buf;
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
-    DMAIntFunc *add_status;
     DMAVoidFunc *trigger_irq;
-    DMAVoidFunc *set_inactive;
+    DMAStopFunc *set_inactive;
     DMAVoidFunc *async_cmd_done;
     DMARestartFunc *restart_cb;
     DMAVoidFunc *reset;
@@ -568,7 +568,7 @@ void ide_flush_cache(IDEState *s);
 void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
                         EndTransferFunc *end_transfer_func);
 void ide_transfer_stop(IDEState *s);
-void ide_set_inactive(IDEState *s);
+void ide_set_inactive(IDEState *s, bool more);
 BlockDriverAIOCB *ide_issue_trim(BlockDriverState *bs,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque);
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 56e2b0c..3635ddf 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -519,7 +519,6 @@ static const IDEDMAOps dbdma_ops = {
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
-    .add_status     = ide_nop_int,
     .restart_cb     = ide_nop_restart,
 };
 
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 68d560d..e4fcf5a 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -158,21 +158,17 @@ static void bmdma_trigger_irq(IDEDMA *dma)
     bm->status |= BM_STATUS_INT;
 }
 
-static int bmdma_add_status(IDEDMA *dma, int status)
+static void bmdma_set_inactive(IDEDMA *dma, bool more)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
-    bm->status |= status;
 
-    return 0;
-}
-
-static void bmdma_set_inactive(IDEDMA *dma)
-{
-    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
-
-    bm->status &= ~BM_STATUS_DMAING;
     bm->dma_cb = NULL;
     bm->unit = -1;
+    if (more) {
+        bm->status |= BM_STATUS_DMAING;
+    } else {
+        bm->status &= ~BM_STATUS_DMAING;
+    }
 }
 
 static void bmdma_restart_dma(BMDMAState *bm, enum ide_dma_cmd dma_cmd)
@@ -247,7 +243,7 @@ static void bmdma_cancel(BMDMAState *bm)
 {
     if (bm->status & BM_STATUS_DMAING) {
         /* cancel DMA request */
-        bmdma_set_inactive(&bm->dma);
+        bmdma_set_inactive(&bm->dma, false);
     }
 }
 
@@ -509,7 +505,6 @@ static const struct IDEDMAOps bmdma_ops = {
     .rw_buf = bmdma_rw_buf,
     .set_unit = bmdma_set_unit,
     .trigger_irq = bmdma_trigger_irq,
-    .add_status = bmdma_add_status,
     .set_inactive = bmdma_set_inactive,
     .restart_cb = bmdma_restart_cb,
     .reset = bmdma_reset,
-- 
1.8.3.1

[Qemu-devel] [PATCH 12/24] ide: move BM_STATUS bits to pci.[ch]

[Paolo Bonzini]

They are not used by AHCI, and should not be even available there.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/internal.h | 11 -----------
 hw/ide/pci.c      |  4 ++++
 hw/ide/pci.h      |  7 +++++++
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index cd5e77c..f2d8ccc 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -485,10 +485,6 @@ struct IDEDevice {
     uint64_t wwn;
 };
 
-#define BM_STATUS_DMAING 0x01
-#define BM_STATUS_ERROR  0x02
-#define BM_STATUS_INT    0x04
-
 /* FIXME These are not status register bits */
 #define BM_STATUS_DMA_RETRY  0x08
 #define BM_STATUS_PIO_RETRY  0x10
@@ -496,13 +492,6 @@ struct IDEDevice {
 #define BM_STATUS_RETRY_FLUSH 0x40
 #define BM_STATUS_RETRY_TRIM 0x80
 
-#define BM_MIGRATION_COMPAT_STATUS_BITS \
-        (BM_STATUS_DMA_RETRY | BM_STATUS_PIO_RETRY | \
-        BM_STATUS_RETRY_READ | BM_STATUS_RETRY_FLUSH)
-
-#define BM_CMD_START     0x01
-#define BM_CMD_READ      0x08
-
 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 e4fcf5a..e61a82f 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -33,6 +33,10 @@
 
 #define BMDMA_PAGE_SIZE 4096
 
+#define BM_MIGRATION_COMPAT_STATUS_BITS \
+        (BM_STATUS_DMA_RETRY | BM_STATUS_PIO_RETRY | \
+        BM_STATUS_RETRY_READ | BM_STATUS_RETRY_FLUSH)
+
 static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
                             BlockDriverCompletionFunc *dma_cb)
 {
diff --git a/hw/ide/pci.h b/hw/ide/pci.h
index 2428275..517711f 100644
--- a/hw/ide/pci.h
+++ b/hw/ide/pci.h
@@ -3,6 +3,13 @@
 
 #include <hw/ide/internal.h>
 
+#define BM_STATUS_DMAING 0x01
+#define BM_STATUS_ERROR  0x02
+#define BM_STATUS_INT    0x04
+
+#define BM_CMD_START     0x01
+#define BM_CMD_READ      0x08
+
 typedef struct BMDMAState {
     IDEDMA dma;
     uint8_t cmd;
-- 
1.8.3.1

[Qemu-devel] [PATCH 13/24] ide: move retry constants out of BM_STATUS_* namespace

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/core.c     | 20 ++++++++++----------
 hw/ide/internal.h | 12 ++++++------
 hw/ide/pci.c      | 14 +++++++-------
 3 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index a62654f..1e3108c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -511,8 +511,8 @@ static void ide_sector_read_cb(void *opaque, int ret)
 
     bdrv_acct_done(s->bs, &s->acct);
     if (ret != 0) {
-        if (ide_handle_rw_error(s, -ret, BM_STATUS_PIO_RETRY |
-                                BM_STATUS_RETRY_READ)) {
+        if (ide_handle_rw_error(s, -ret, IDE_RETRY_PIO |
+                                IDE_RETRY_READ)) {
             return;
         }
     }
@@ -597,14 +597,14 @@ void ide_dma_error(IDEState *s)
 
 static int ide_handle_rw_error(IDEState *s, int error, int op)
 {
-    bool is_read = (op & BM_STATUS_RETRY_READ) != 0;
+    bool is_read = (op & IDE_RETRY_READ) != 0;
     BlockErrorAction action = bdrv_get_error_action(s->bs, is_read, error);
 
     if (action == BDRV_ACTION_STOP) {
         s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
         s->bus->error_status = op;
     } else if (action == BDRV_ACTION_REPORT) {
-        if (op & BM_STATUS_DMA_RETRY) {
+        if (op & IDE_RETRY_DMA) {
             dma_buf_commit(s);
             ide_dma_error(s);
         } else {
@@ -623,12 +623,12 @@ void ide_dma_cb(void *opaque, int ret)
     bool stay_active = false;
 
     if (ret < 0) {
-        int op = BM_STATUS_DMA_RETRY;
+        int op = IDE_RETRY_DMA;
 
         if (s->dma_cmd == IDE_DMA_READ)
-            op |= BM_STATUS_RETRY_READ;
+            op |= IDE_RETRY_READ;
         else if (s->dma_cmd == IDE_DMA_TRIM)
-            op |= BM_STATUS_RETRY_TRIM;
+            op |= IDE_RETRY_TRIM;
 
         if (ide_handle_rw_error(s, -ret, op)) {
             return;
@@ -746,7 +746,7 @@ static void ide_sector_write_cb(void *opaque, int ret)
     s->status &= ~BUSY_STAT;
 
     if (ret != 0) {
-        if (ide_handle_rw_error(s, -ret, BM_STATUS_PIO_RETRY)) {
+        if (ide_handle_rw_error(s, -ret, IDE_RETRY_PIO)) {
             return;
         }
     }
@@ -813,7 +813,7 @@ static void ide_flush_cb(void *opaque, int ret)
 
     if (ret < 0) {
         /* XXX: What sector number to set here? */
-        if (ide_handle_rw_error(s, -ret, BM_STATUS_RETRY_FLUSH)) {
+        if (ide_handle_rw_error(s, -ret, IDE_RETRY_FLUSH)) {
             return;
         }
     }
@@ -2356,7 +2356,7 @@ static bool ide_drive_pio_state_needed(void *opaque)
     IDEState *s = opaque;
 
     return ((s->status & DRQ_STAT) != 0)
-        || (s->bus->error_status & BM_STATUS_PIO_RETRY);
+        || (s->bus->error_status & IDE_RETRY_PIO);
 }
 
 static bool ide_tray_state_needed(void *opaque)
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index f2d8ccc..041f173 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -485,12 +485,12 @@ struct IDEDevice {
     uint64_t wwn;
 };
 
-/* FIXME These are not status register bits */
-#define BM_STATUS_DMA_RETRY  0x08
-#define BM_STATUS_PIO_RETRY  0x10
-#define BM_STATUS_RETRY_READ  0x20
-#define BM_STATUS_RETRY_FLUSH 0x40
-#define BM_STATUS_RETRY_TRIM 0x80
+/* These are used for the error_status field of IDEBus */
+#define IDE_RETRY_DMA  0x08
+#define IDE_RETRY_PIO  0x10
+#define IDE_RETRY_READ  0x20
+#define IDE_RETRY_FLUSH 0x40
+#define IDE_RETRY_TRIM 0x80
 
 static inline IDEState *idebus_active_if(IDEBus *bus)
 {
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index e61a82f..cc910de 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -34,8 +34,8 @@
 #define BMDMA_PAGE_SIZE 4096
 
 #define BM_MIGRATION_COMPAT_STATUS_BITS \
-        (BM_STATUS_DMA_RETRY | BM_STATUS_PIO_RETRY | \
-        BM_STATUS_RETRY_READ | BM_STATUS_RETRY_FLUSH)
+        (IDE_RETRY_DMA | IDE_RETRY_PIO | \
+        IDE_RETRY_READ | IDE_RETRY_FLUSH)
 
 static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
                             BlockDriverCompletionFunc *dma_cb)
@@ -204,7 +204,7 @@ static void bmdma_restart_bh(void *opaque)
         return;
     }
 
-    is_read = (bus->error_status & BM_STATUS_RETRY_READ) != 0;
+    is_read = (bus->error_status & IDE_RETRY_READ) != 0;
 
     /* The error status must be cleared before resubmitting the request: The
      * request may fail again, and this case can only be distinguished if the
@@ -212,19 +212,19 @@ static void bmdma_restart_bh(void *opaque)
     error_status = bus->error_status;
     bus->error_status = 0;
 
-    if (error_status & BM_STATUS_DMA_RETRY) {
-        if (error_status & BM_STATUS_RETRY_TRIM) {
+    if (error_status & IDE_RETRY_DMA) {
+        if (error_status & IDE_RETRY_TRIM) {
             bmdma_restart_dma(bm, IDE_DMA_TRIM);
         } else {
             bmdma_restart_dma(bm, is_read ? IDE_DMA_READ : IDE_DMA_WRITE);
         }
-    } else if (error_status & BM_STATUS_PIO_RETRY) {
+    } else if (error_status & IDE_RETRY_PIO) {
         if (is_read) {
             ide_sector_read(bmdma_active_if(bm));
         } else {
             ide_sector_write(bmdma_active_if(bm));
         }
-    } else if (error_status & BM_STATUS_RETRY_FLUSH) {
+    } else if (error_status & IDE_RETRY_FLUSH) {
         ide_flush_cache(bmdma_active_if(bm));
     }
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 13/24] ide: move retry constants out of BM_STATUS_* namespace

[Kevin Wolf]

Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/core.c     | 20 ++++++++++----------
>  hw/ide/internal.h | 12 ++++++------
>  hw/ide/pci.c      | 14 +++++++-------
>  3 files changed, 23 insertions(+), 23 deletions(-)

> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -485,12 +485,12 @@ struct IDEDevice {
>      uint64_t wwn;
>  };
>  
> -/* FIXME These are not status register bits */
> -#define BM_STATUS_DMA_RETRY  0x08
> -#define BM_STATUS_PIO_RETRY  0x10
> -#define BM_STATUS_RETRY_READ  0x20
> -#define BM_STATUS_RETRY_FLUSH 0x40
> -#define BM_STATUS_RETRY_TRIM 0x80
> +/* These are used for the error_status field of IDEBus */
> +#define IDE_RETRY_DMA  0x08
> +#define IDE_RETRY_PIO  0x10
> +#define IDE_RETRY_READ  0x20
> +#define IDE_RETRY_FLUSH 0x40
> +#define IDE_RETRY_TRIM 0x80

I wouldn't mind using the chance to align the numbers on the same
column (or at least removing one space where there are two)

Kevin

[Qemu-devel] [PATCH 14/24] ide: start extracting ide_restart_dma out of bmdma_restart_dma

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/pci.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index cc910de..c7fbe51 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -175,18 +175,24 @@ static void bmdma_set_inactive(IDEDMA *dma, bool more)
     }
 }
 
-static void bmdma_restart_dma(BMDMAState *bm, enum ide_dma_cmd dma_cmd)
+static void bmdma_restart_dma(BMDMAState *bm)
 {
     IDEState *s = bmdma_active_if(bm);
 
     ide_set_sector(s, bm->sector_num);
+    s->nsector = bm->nsector;
+    bm->cur_addr = bm->addr;
+}
+
+static void ide_restart_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
+{
+    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, s->bus->dma);
+
+    bmdma_restart_dma(bm);
     s->io_buffer_index = 0;
     s->io_buffer_size = 0;
-    s->nsector = bm->nsector;
     s->dma_cmd = dma_cmd;
-    bm->cur_addr = bm->addr;
-    bm->dma_cb = ide_dma_cb;
-    bmdma_start_dma(&bm->dma, s, bm->dma_cb);
+    bmdma_start_dma(&bm->dma, s, ide_dma_cb);
 }
 
 /* TODO This should be common IDE code */
@@ -194,6 +200,7 @@ static void bmdma_restart_bh(void *opaque)
 {
     BMDMAState *bm = opaque;
     IDEBus *bus = bm->bus;
+    IDEState *s;
     bool is_read;
     int error_status;
 
@@ -204,6 +211,7 @@ static void bmdma_restart_bh(void *opaque)
         return;
     }
 
+    s = bmdma_active_if(bm);
     is_read = (bus->error_status & IDE_RETRY_READ) != 0;
 
     /* The error status must be cleared before resubmitting the request: The
@@ -214,18 +222,18 @@ static void bmdma_restart_bh(void *opaque)
 
     if (error_status & IDE_RETRY_DMA) {
         if (error_status & IDE_RETRY_TRIM) {
-            bmdma_restart_dma(bm, IDE_DMA_TRIM);
+            ide_restart_dma(s, IDE_DMA_TRIM);
         } else {
-            bmdma_restart_dma(bm, is_read ? IDE_DMA_READ : IDE_DMA_WRITE);
+            ide_restart_dma(s, is_read ? IDE_DMA_READ : IDE_DMA_WRITE);
         }
     } else if (error_status & IDE_RETRY_PIO) {
         if (is_read) {
-            ide_sector_read(bmdma_active_if(bm));
+            ide_sector_read(s);
         } else {
-            ide_sector_write(bmdma_active_if(bm));
+            ide_sector_write(s);
         }
     } else if (error_status & IDE_RETRY_FLUSH) {
-        ide_flush_cache(bmdma_active_if(bm));
+        ide_flush_cache(s);
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 15/24] ide: prepare to move restart to common code

[Paolo Bonzini]

Using ide_start_dma instead of bmdma_start_dma introduces a new
assignment "s->bus->dma->unit = s->unit".  This introduces no
change because ide_handle_rw_error has already done the same
assignment.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/internal.h |  1 +
 hw/ide/pci.c      | 12 +++++++-----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 041f173..96969d9 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -432,6 +432,7 @@ struct IDEDMAOps {
     DMAIntFunc *prepare_buf;
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
+    DMAVoidFunc *restart_dma;
     DMAVoidFunc *trigger_irq;
     DMAStopFunc *set_inactive;
     DMAVoidFunc *async_cmd_done;
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index c7fbe51..4a1328d 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -175,8 +175,9 @@ static void bmdma_set_inactive(IDEDMA *dma, bool more)
     }
 }
 
-static void bmdma_restart_dma(BMDMAState *bm)
+static void bmdma_restart_dma(IDEDMA *dma)
 {
+    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
     IDEState *s = bmdma_active_if(bm);
 
     ide_set_sector(s, bm->sector_num);
@@ -186,13 +187,13 @@ static void bmdma_restart_dma(BMDMAState *bm)
 
 static void ide_restart_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 {
-    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, s->bus->dma);
-
-    bmdma_restart_dma(bm);
+    if (s->bus->dma->ops->restart_dma) {
+        s->bus->dma->ops->restart_dma(s->bus->dma);
+    }
     s->io_buffer_index = 0;
     s->io_buffer_size = 0;
     s->dma_cmd = dma_cmd;
-    bmdma_start_dma(&bm->dma, s, ide_dma_cb);
+    ide_start_dma(s, ide_dma_cb);
 }
 
 /* TODO This should be common IDE code */
@@ -516,6 +517,7 @@ static const struct IDEDMAOps bmdma_ops = {
     .prepare_buf = bmdma_prepare_buf,
     .rw_buf = bmdma_rw_buf,
     .set_unit = bmdma_set_unit,
+    .restart_dma = bmdma_restart_dma,
     .trigger_irq = bmdma_trigger_irq,
     .set_inactive = bmdma_set_inactive,
     .restart_cb = bmdma_restart_cb,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 15/24] ide: prepare to move restart to common code

[Paolo Bonzini]

Il 28/10/2013 17:43, Paolo Bonzini ha scritto:
> Using ide_start_dma instead of bmdma_start_dma introduces a new
> assignment "s->bus->dma->unit = s->unit".  This introduces no
> change because ide_handle_rw_error has already done the same
> assignment.

This remark is stale.

Paolo

[Qemu-devel] [PATCH 16/24] ide: introduce ide_register_restart_cb

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/cmd646.c   | 3 +--
 hw/ide/core.c     | 5 +++++
 hw/ide/internal.h | 1 +
 hw/ide/piix.c     | 3 +--
 hw/ide/via.c      | 3 +--
 5 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 0500a7a..4753543 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -294,8 +294,7 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
         d->bmdma[i].bus = &d->bus[i];
-        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
-                                         &d->bmdma[i].dma);
+        ide_register_restart_cb(&d->bus[i]);
     }
 
     vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 1e3108c..93cfd46 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2193,6 +2193,11 @@ static const IDEDMAOps ide_dma_nop_ops = {
     .restart_cb     = ide_nop_restart,
 };
 
+void ide_register_restart_cb(IDEBus *bus)
+{
+    qemu_add_vm_change_state_handler(bus->dma->ops->restart_cb, bus->dma);
+}
+
 static IDEDMA ide_dma_nop = {
     .ops = &ide_dma_nop_ops,
     .aiocb = NULL,
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 96969d9..678b33c 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -548,6 +548,7 @@ void ide_init2(IDEBus *bus, qemu_irq irq);
 void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
                                     DriveInfo *hd1, qemu_irq irq);
 void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
+void ide_register_restart_cb(IDEBus *bus);
 
 void ide_exec_cmd(IDEBus *bus, uint32_t val);
 void ide_dma_cb(void *opaque, int ret);
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index ab36749..d7f5118 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -143,8 +143,7 @@ static void pci_piix_init_ports(PCIIDEState *d) {
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
         d->bmdma[i].bus = &d->bus[i];
-        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
-                                         &d->bmdma[i].dma);
+        ide_register_restart_cb(&d->bus[i]);
     }
 }
 
diff --git a/hw/ide/via.c b/hw/ide/via.c
index 99468c7..ae52164 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -166,8 +166,7 @@ static void vt82c686b_init_ports(PCIIDEState *d) {
 
         bmdma_init(&d->bus[i], &d->bmdma[i], d);
         d->bmdma[i].bus = &d->bus[i];
-        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
-                                         &d->bmdma[i].dma);
+        ide_register_restart_cb(&d->bus[i]);
     }
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 16/24] ide: introduce ide_register_restart_cb

[Kevin Wolf]

Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/ide/cmd646.c   | 3 +--
>  hw/ide/core.c     | 5 +++++
>  hw/ide/internal.h | 1 +
>  hw/ide/piix.c     | 3 +--
>  hw/ide/via.c      | 3 +--
>  5 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
> index 0500a7a..4753543 100644
> --- a/hw/ide/cmd646.c
> +++ b/hw/ide/cmd646.c
> @@ -294,8 +294,7 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
>  
>          bmdma_init(&d->bus[i], &d->bmdma[i], d);
>          d->bmdma[i].bus = &d->bus[i];
> -        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
> -                                         &d->bmdma[i].dma);
> +        ide_register_restart_cb(&d->bus[i]);
>      }
>  
>      vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
> diff --git a/hw/ide/core.c b/hw/ide/core.c
> index 1e3108c..93cfd46 100644
> --- a/hw/ide/core.c
> +++ b/hw/ide/core.c
> @@ -2193,6 +2193,11 @@ static const IDEDMAOps ide_dma_nop_ops = {
>      .restart_cb     = ide_nop_restart,
>  };
>  
> +void ide_register_restart_cb(IDEBus *bus)
> +{
> +    qemu_add_vm_change_state_handler(bus->dma->ops->restart_cb, bus->dma);
> +}
> +
>  static IDEDMA ide_dma_nop = {
>      .ops = &ide_dma_nop_ops,
>      .aiocb = NULL,
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 96969d9..678b33c 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -548,6 +548,7 @@ void ide_init2(IDEBus *bus, qemu_irq irq);
>  void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
>                                      DriveInfo *hd1, qemu_irq irq);
>  void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
> +void ide_register_restart_cb(IDEBus *bus);
>  
>  void ide_exec_cmd(IDEBus *bus, uint32_t val);
>  void ide_dma_cb(void *opaque, int ret);
> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
> index ab36749..d7f5118 100644
> --- a/hw/ide/piix.c
> +++ b/hw/ide/piix.c
> @@ -143,8 +143,7 @@ static void pci_piix_init_ports(PCIIDEState *d) {
>  
>          bmdma_init(&d->bus[i], &d->bmdma[i], d);
>          d->bmdma[i].bus = &d->bus[i];
> -        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
> -                                         &d->bmdma[i].dma);
> +        ide_register_restart_cb(&d->bus[i]);
>      }
>  }
>  
> diff --git a/hw/ide/via.c b/hw/ide/via.c
> index 99468c7..ae52164 100644
> --- a/hw/ide/via.c
> +++ b/hw/ide/via.c
> @@ -166,8 +166,7 @@ static void vt82c686b_init_ports(PCIIDEState *d) {
>  
>          bmdma_init(&d->bus[i], &d->bmdma[i], d);
>          d->bmdma[i].bus = &d->bus[i];
> -        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
> -                                         &d->bmdma[i].dma);
> +        ide_register_restart_cb(&d->bus[i]);
>      }
>  }

Can't we instead register a callback somewhere in core.c?

Kevin

Re: [Qemu-devel] [PATCH 16/24] ide: introduce ide_register_restart_cb

[Paolo Bonzini]

Il 30/10/2013 14:13, Kevin Wolf ha scritto:
> Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/ide/cmd646.c   | 3 +--
>>  hw/ide/core.c     | 5 +++++
>>  hw/ide/internal.h | 1 +
>>  hw/ide/piix.c     | 3 +--
>>  hw/ide/via.c      | 3 +--
>>  5 files changed, 9 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
>> index 0500a7a..4753543 100644
>> --- a/hw/ide/cmd646.c
>> +++ b/hw/ide/cmd646.c
>> @@ -294,8 +294,7 @@ static int pci_cmd646_ide_initfn(PCIDevice *dev)
>>  
>>          bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>          d->bmdma[i].bus = &d->bus[i];
>> -        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
>> -                                         &d->bmdma[i].dma);
>> +        ide_register_restart_cb(&d->bus[i]);
>>      }
>>  
>>      vmstate_register(DEVICE(dev), 0, &vmstate_ide_pci, d);
>> diff --git a/hw/ide/core.c b/hw/ide/core.c
>> index 1e3108c..93cfd46 100644
>> --- a/hw/ide/core.c
>> +++ b/hw/ide/core.c
>> @@ -2193,6 +2193,11 @@ static const IDEDMAOps ide_dma_nop_ops = {
>>      .restart_cb     = ide_nop_restart,
>>  };
>>  
>> +void ide_register_restart_cb(IDEBus *bus)
>> +{
>> +    qemu_add_vm_change_state_handler(bus->dma->ops->restart_cb, bus->dma);
>> +}
>> +
>>  static IDEDMA ide_dma_nop = {
>>      .ops = &ide_dma_nop_ops,
>>      .aiocb = NULL,
>> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
>> index 96969d9..678b33c 100644
>> --- a/hw/ide/internal.h
>> +++ b/hw/ide/internal.h
>> @@ -548,6 +548,7 @@ void ide_init2(IDEBus *bus, qemu_irq irq);
>>  void ide_init2_with_non_qdev_drives(IDEBus *bus, DriveInfo *hd0,
>>                                      DriveInfo *hd1, qemu_irq irq);
>>  void ide_init_ioport(IDEBus *bus, ISADevice *isa, int iobase, int iobase2);
>> +void ide_register_restart_cb(IDEBus *bus);
>>  
>>  void ide_exec_cmd(IDEBus *bus, uint32_t val);
>>  void ide_dma_cb(void *opaque, int ret);
>> diff --git a/hw/ide/piix.c b/hw/ide/piix.c
>> index ab36749..d7f5118 100644
>> --- a/hw/ide/piix.c
>> +++ b/hw/ide/piix.c
>> @@ -143,8 +143,7 @@ static void pci_piix_init_ports(PCIIDEState *d) {
>>  
>>          bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>          d->bmdma[i].bus = &d->bus[i];
>> -        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
>> -                                         &d->bmdma[i].dma);
>> +        ide_register_restart_cb(&d->bus[i]);
>>      }
>>  }
>>  
>> diff --git a/hw/ide/via.c b/hw/ide/via.c
>> index 99468c7..ae52164 100644
>> --- a/hw/ide/via.c
>> +++ b/hw/ide/via.c
>> @@ -166,8 +166,7 @@ static void vt82c686b_init_ports(PCIIDEState *d) {
>>  
>>          bmdma_init(&d->bus[i], &d->bmdma[i], d);
>>          d->bmdma[i].bus = &d->bus[i];
>> -        qemu_add_vm_change_state_handler(d->bus[i].dma->ops->restart_cb,
>> -                                         &d->bmdma[i].dma);
>> +        ide_register_restart_cb(&d->bus[i]);
>>      }
>>  }
> 
> Can't we instead register a callback somewhere in core.c?

Yeah, once more we need to abstract the legacy I/O ports better for that.

Paolo

[Qemu-devel] [PATCH 17/24] ide: do not use BMDMA in restart callback

[Paolo Bonzini]

Whenever an error stops the VM, ide_handle_rw_error does
"s->bus->dma->unit = s->unit".  So we can just use
idebus_active_if.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/pci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4a1328d..4df50dc 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -208,17 +208,17 @@ static void bmdma_restart_bh(void *opaque)
     qemu_bh_delete(bm->bh);
     bm->bh = NULL;
 
-    if (bm->unit == (uint8_t) -1) {
+    error_status = bus->error_status;
+    if (bus->error_status == 0) {
         return;
     }
 
-    s = bmdma_active_if(bm);
+    s = idebus_active_if(bus);
     is_read = (bus->error_status & IDE_RETRY_READ) != 0;
 
     /* The error status must be cleared before resubmitting the request: The
      * request may fail again, and this case can only be distinguished if the
      * called function can set a new error status. */
-    error_status = bus->error_status;
     bus->error_status = 0;
 
     if (error_status & IDE_RETRY_DMA) {
-- 
1.8.3.1

[Qemu-devel] [PATCH 18/24] ide: pass IDEBus to the restart_cb

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/core.c | 2 +-
 hw/ide/pci.c  | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 93cfd46..5820fad 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2195,7 +2195,7 @@ static const IDEDMAOps ide_dma_nop_ops = {
 
 void ide_register_restart_cb(IDEBus *bus)
 {
-    qemu_add_vm_change_state_handler(bus->dma->ops->restart_cb, bus->dma);
+    qemu_add_vm_change_state_handler(bus->dma->ops->restart_cb, bus);
 }
 
 static IDEDMA ide_dma_nop = {
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 4df50dc..9d26a26 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -199,8 +199,8 @@ static void ide_restart_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 /* TODO This should be common IDE code */
 static void bmdma_restart_bh(void *opaque)
 {
-    BMDMAState *bm = opaque;
-    IDEBus *bus = bm->bus;
+    IDEBus *bus = opaque;
+    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, bus->dma);
     IDEState *s;
     bool is_read;
     int error_status;
@@ -240,8 +240,8 @@ static void bmdma_restart_bh(void *opaque)
 
 static void bmdma_restart_cb(void *opaque, int running, RunState state)
 {
-    IDEDMA *dma = opaque;
-    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
+    IDEBus *bus = opaque;
+    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, bus->dma);
 
     if (!running)
         return;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 18/24] ide: pass IDEBus to the restart_cb

[Paolo Bonzini]

Il 28/10/2013 17:43, Paolo Bonzini ha scritto:
> @@ -199,8 +199,8 @@ static void ide_restart_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
>  /* TODO This should be common IDE code */
>  static void bmdma_restart_bh(void *opaque)
>  {
> -    BMDMAState *bm = opaque;
> -    IDEBus *bus = bm->bus;
> +    IDEBus *bus = opaque;
> +    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, bus->dma);
>      IDEState *s;
>      bool is_read;
>      int error_status;

The corresponding change in qemu_bh_new is mistakenly in patch 19,
rather than in this one.

Paolo

[Qemu-devel] [PATCH 19/24] ide: move restart callback to common code

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/core.c     | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ide/internal.h |  2 ++
 hw/ide/pci.c      | 68 -------------------------------------------------------
 hw/ide/pci.h      |  1 -
 4 files changed, 67 insertions(+), 70 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 5820fad..256f62a 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2193,9 +2193,73 @@ static const IDEDMAOps ide_dma_nop_ops = {
     .restart_cb     = ide_nop_restart,
 };
 
+static void ide_restart_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
+{
+    s->bus->dma->ops->restart_dma(s->bus->dma);
+    s->io_buffer_index = 0;
+    s->io_buffer_size = 0;
+    s->dma_cmd = dma_cmd;
+    ide_start_dma(s, ide_dma_cb);
+}
+
+static void ide_restart_bh(void *opaque)
+{
+    IDEBus *bus = opaque;
+    IDEState *s;
+    bool is_read;
+    int error_status;
+
+    qemu_bh_delete(bus->bh);
+    bus->bh = NULL;
+
+    error_status = bus->error_status;
+    if (bus->error_status == 0) {
+        return;
+    }
+
+    s = idebus_active_if(bus);
+    is_read = (bus->error_status & IDE_RETRY_READ) != 0;
+
+    /* The error status must be cleared before resubmitting the request: The
+     * request may fail again, and this case can only be distinguished if the
+     * called function can set a new error status. */
+    bus->error_status = 0;
+
+    if (error_status & IDE_RETRY_DMA) {
+        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) {
+        if (is_read) {
+            ide_sector_read(s);
+        } else {
+            ide_sector_write(s);
+        }
+    } else if (error_status & IDE_RETRY_FLUSH) {
+        ide_flush_cache(s);
+    }
+}
+
+static void ide_restart_cb(void *opaque, int running, RunState state)
+{
+    IDEBus *bus = opaque;
+
+    if (!running)
+        return;
+
+    if (!bus->bh) {
+        bus->bh = qemu_bh_new(ide_restart_bh, bus);
+        qemu_bh_schedule(bus->bh);
+    }
+}
+
 void ide_register_restart_cb(IDEBus *bus)
 {
-    qemu_add_vm_change_state_handler(bus->dma->ops->restart_cb, bus);
+    if (bus->dma->ops->restart_dma) {
+        qemu_add_vm_change_state_handler(ide_restart_cb, bus);
+    }
 }
 
 static IDEDMA ide_dma_nop = {
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 678b33c..e24f6cb 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -452,6 +452,8 @@ struct IDEBus {
     IDEDevice *master;
     IDEDevice *slave;
     IDEState ifs[2];
+    QEMUBH *bh;
+
     int bus_id;
     int max_units;
     IDEDMA *dma;
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 9d26a26..7568f1e 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -185,73 +185,6 @@ static void bmdma_restart_dma(IDEDMA *dma)
     bm->cur_addr = bm->addr;
 }
 
-static void ide_restart_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
-{
-    if (s->bus->dma->ops->restart_dma) {
-        s->bus->dma->ops->restart_dma(s->bus->dma);
-    }
-    s->io_buffer_index = 0;
-    s->io_buffer_size = 0;
-    s->dma_cmd = dma_cmd;
-    ide_start_dma(s, ide_dma_cb);
-}
-
-/* TODO This should be common IDE code */
-static void bmdma_restart_bh(void *opaque)
-{
-    IDEBus *bus = opaque;
-    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, bus->dma);
-    IDEState *s;
-    bool is_read;
-    int error_status;
-
-    qemu_bh_delete(bm->bh);
-    bm->bh = NULL;
-
-    error_status = bus->error_status;
-    if (bus->error_status == 0) {
-        return;
-    }
-
-    s = idebus_active_if(bus);
-    is_read = (bus->error_status & IDE_RETRY_READ) != 0;
-
-    /* The error status must be cleared before resubmitting the request: The
-     * request may fail again, and this case can only be distinguished if the
-     * called function can set a new error status. */
-    bus->error_status = 0;
-
-    if (error_status & IDE_RETRY_DMA) {
-        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) {
-        if (is_read) {
-            ide_sector_read(s);
-        } else {
-            ide_sector_write(s);
-        }
-    } else if (error_status & IDE_RETRY_FLUSH) {
-        ide_flush_cache(s);
-    }
-}
-
-static void bmdma_restart_cb(void *opaque, int running, RunState state)
-{
-    IDEBus *bus = opaque;
-    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, bus->dma);
-
-    if (!running)
-        return;
-
-    if (!bm->bh) {
-        bm->bh = qemu_bh_new(bmdma_restart_bh, &bm->dma);
-        qemu_bh_schedule(bm->bh);
-    }
-}
-
 static void bmdma_cancel(BMDMAState *bm)
 {
     if (bm->status & BM_STATUS_DMAING) {
@@ -520,7 +453,6 @@ static const struct IDEDMAOps bmdma_ops = {
     .restart_dma = bmdma_restart_dma,
     .trigger_irq = bmdma_trigger_irq,
     .set_inactive = bmdma_set_inactive,
-    .restart_cb = bmdma_restart_cb,
     .reset = bmdma_reset,
 };
 
diff --git a/hw/ide/pci.h b/hw/ide/pci.h
index 517711f..78e5550 100644
--- a/hw/ide/pci.h
+++ b/hw/ide/pci.h
@@ -28,7 +28,6 @@ typedef struct BMDMAState {
     uint32_t nsector;
     MemoryRegion addr_ioport;
     MemoryRegion extra_io;
-    QEMUBH *bh;
     qemu_irq irq;
 
     /* Bit 0-2 and 7:   BM status register
-- 
1.8.3.1

[Qemu-devel] [PATCH 20/24] ide: remove restart_cb callback

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c     | 5 -----
 hw/ide/core.c     | 5 -----
 hw/ide/internal.h | 1 -
 hw/ide/macio.c    | 5 -----
 4 files changed, 16 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 1d4fb17..9cafee9 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1119,10 +1119,6 @@ static void ahci_irq_set(void *opaque, int n, int level)
 {
 }
 
-static void ahci_dma_restart_cb(void *opaque, int running, RunState state)
-{
-}
-
 static const IDEDMAOps ahci_dma_ops = {
     .start_dma = ahci_start_dma,
     .start_transfer = ahci_start_transfer,
@@ -1131,7 +1127,6 @@ static const IDEDMAOps ahci_dma_ops = {
     .set_unit = ahci_dma_set_unit,
     .trigger_irq = ahci_dma_trigger_irq,
     .async_cmd_done = ahci_async_cmd_done,
-    .restart_cb = ahci_dma_restart_cb,
 };
 
 void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 256f62a..c07c0d1 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2182,15 +2182,10 @@ static int ide_nop_int(IDEDMA *dma, int x)
     return 0;
 }
 
-static void ide_nop_restart(void *opaque, int x, RunState y)
-{
-}
-
 static const IDEDMAOps ide_dma_nop_ops = {
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
-    .restart_cb     = ide_nop_restart,
 };
 
 static void ide_restart_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index e24f6cb..1c77ad9 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -436,7 +436,6 @@ struct IDEDMAOps {
     DMAVoidFunc *trigger_irq;
     DMAStopFunc *set_inactive;
     DMAVoidFunc *async_cmd_done;
-    DMARestartFunc *restart_cb;
     DMAVoidFunc *reset;
 };
 
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index 3635ddf..d1d40d8 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -500,10 +500,6 @@ static int ide_nop_int(IDEDMA *dma, int x)
     return 0;
 }
 
-static void ide_nop_restart(void *opaque, int x, RunState y)
-{
-}
-
 static void ide_dbdma_start(IDEDMA *dma, IDEState *s,
                             BlockDriverCompletionFunc *cb)
 {
@@ -519,7 +515,6 @@ static const IDEDMAOps dbdma_ops = {
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
     .set_unit       = ide_nop_int,
-    .restart_cb     = ide_nop_restart,
 };
 
 static void macio_ide_realizefn(DeviceState *dev, Error **errp)
-- 
1.8.3.1

[Qemu-devel] [PATCH 21/24] ide: replace set_unit callback with more IDEBus state

[Paolo Bonzini]

Start moving the initial state of the current request to IDEBus, so that
AHCI can use it.  The set_unit callback is not used anymore once this is
done.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/ahci.c     |  7 -------
 hw/ide/core.c     |  6 ++++--
 hw/ide/internal.h |  2 +-
 hw/ide/macio.c    |  1 -
 hw/ide/pci.c      | 19 ++++++-------------
 hw/ide/pci.h      |  7 ++++---
 6 files changed, 15 insertions(+), 27 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 9cafee9..b7e0d18 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1086,12 +1086,6 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
     return 1;
 }
 
-static int ahci_dma_set_unit(IDEDMA *dma, int unit)
-{
-    /* only a single unit per link */
-    return 0;
-}
-
 static void ahci_dma_trigger_irq(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
@@ -1124,7 +1118,6 @@ static const IDEDMAOps ahci_dma_ops = {
     .start_transfer = ahci_start_transfer,
     .prepare_buf = ahci_dma_prepare_buf,
     .rw_buf = ahci_dma_rw_buf,
-    .set_unit = ahci_dma_set_unit,
     .trigger_irq = ahci_dma_trigger_irq,
     .async_cmd_done = ahci_async_cmd_done,
 };
diff --git a/hw/ide/core.c b/hw/ide/core.c
index c07c0d1..970f372 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -580,6 +580,7 @@ static void ide_async_cmd_done(IDEState *s)
 void ide_set_inactive(IDEState *s, bool more)
 {
     s->bus->dma->aiocb = NULL;
+    s->bus->retry_unit = -1;
     if (s->bus->dma->ops->set_inactive) {
         s->bus->dma->ops->set_inactive(s->bus->dma, more);
     }
@@ -601,7 +602,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
     BlockErrorAction action = bdrv_get_error_action(s->bs, is_read, error);
 
     if (action == BDRV_ACTION_STOP) {
-        s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
+        assert(s->bus->retry_unit == s->unit);
         s->bus->error_status = op;
     } else if (action == BDRV_ACTION_REPORT) {
         if (op & IDE_RETRY_DMA) {
@@ -724,6 +725,7 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 
 void ide_start_dma(IDEState *s, BlockDriverCompletionFunc *cb)
 {
+    s->bus->retry_unit = s->unit;
     if (s->bus->dma->ops->start_dma) {
         s->bus->dma->ops->start_dma(s->bus->dma, s, cb);
     }
@@ -2185,11 +2187,11 @@ static int ide_nop_int(IDEDMA *dma, int x)
 static const IDEDMAOps ide_dma_nop_ops = {
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
-    .set_unit       = ide_nop_int,
 };
 
 static void ide_restart_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 {
+    s->unit = s->bus->retry_unit;
     s->bus->dma->ops->restart_dma(s->bus->dma);
     s->io_buffer_index = 0;
     s->io_buffer_size = 0;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 1c77ad9..b1529be 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -431,7 +431,6 @@ struct IDEDMAOps {
     DMAVoidFunc *start_transfer;
     DMAIntFunc *prepare_buf;
     DMAIntFunc *rw_buf;
-    DMAIntFunc *set_unit;
     DMAVoidFunc *restart_dma;
     DMAVoidFunc *trigger_irq;
     DMAStopFunc *set_inactive;
@@ -461,6 +460,7 @@ struct IDEBus {
     qemu_irq irq;
 
     int error_status;
+    uint8_t retry_unit;
 };
 
 #define TYPE_IDE_DEVICE "ide-device"
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index d1d40d8..c41706c 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -514,7 +514,6 @@ static const IDEDMAOps dbdma_ops = {
     .start_dma      = ide_dbdma_start,
     .prepare_buf    = ide_nop_int,
     .rw_buf         = ide_nop_int,
-    .set_unit       = ide_nop_int,
 };
 
 static void macio_ide_realizefn(DeviceState *dev, Error **errp)
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 7568f1e..43507ec 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -42,7 +42,6 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
 
-    bm->unit = s->unit;
     bm->dma_cb = dma_cb;
     bm->cur_prd_last = 0;
     bm->cur_prd_addr = 0;
@@ -148,14 +147,6 @@ static int bmdma_rw_buf(IDEDMA *dma, int is_write)
     return 1;
 }
 
-static int bmdma_set_unit(IDEDMA *dma, int unit)
-{
-    BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
-    bm->unit = unit;
-
-    return 0;
-}
-
 static void bmdma_trigger_irq(IDEDMA *dma)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
@@ -167,7 +158,6 @@ static void bmdma_set_inactive(IDEDMA *dma, bool more)
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
 
     bm->dma_cb = NULL;
-    bm->unit = -1;
     if (more) {
         bm->status |= BM_STATUS_DMAING;
     } else {
@@ -326,6 +316,7 @@ static void ide_bmdma_pre_save(void *opaque)
     BMDMAState *bm = opaque;
     uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
 
+    bm->migration_retry_unit = bm->bus->retry_unit;
     bm->migration_compat_status =
         (bm->status & ~abused_bits) | (bm->bus->error_status & abused_bits);
 }
@@ -342,6 +333,9 @@ static int ide_bmdma_post_load(void *opaque, int version_id)
         bm->status = bm->migration_compat_status & ~abused_bits;
         bm->bus->error_status |= bm->migration_compat_status & abused_bits;
     }
+    if (bm->bus->error_status) {
+        bm->bus->retry_unit = bm->migration_retry_unit;
+    }
 
     return 0;
 }
@@ -383,7 +377,7 @@ static const VMStateDescription vmstate_bmdma = {
         VMSTATE_UINT32(addr, BMDMAState),
         VMSTATE_INT64(sector_num, BMDMAState),
         VMSTATE_UINT32(nsector, BMDMAState),
-        VMSTATE_UINT8(unit, BMDMAState),
+        VMSTATE_UINT8(migration_retry_unit, BMDMAState),
         VMSTATE_END_OF_LIST()
     },
     .subsections = (VMStateSubsection []) {
@@ -407,7 +401,7 @@ static int ide_pci_post_load(void *opaque, int version_id)
     for(i = 0; i < 2; i++) {
         /* current versions always store 0/1, but older version
            stored bigger values. We only need last bit */
-        d->bmdma[i].unit &= 1;
+        d->bmdma[i].migration_retry_unit &= 1;
         ide_bmdma_post_load(&d->bmdma[i], -1);
     }
 
@@ -449,7 +443,6 @@ static const struct IDEDMAOps bmdma_ops = {
     .start_dma = bmdma_start_dma,
     .prepare_buf = bmdma_prepare_buf,
     .rw_buf = bmdma_rw_buf,
-    .set_unit = bmdma_set_unit,
     .restart_dma = bmdma_restart_dma,
     .trigger_irq = bmdma_trigger_irq,
     .set_inactive = bmdma_set_inactive,
diff --git a/hw/ide/pci.h b/hw/ide/pci.h
index 78e5550..6f28ce4 100644
--- a/hw/ide/pci.h
+++ b/hw/ide/pci.h
@@ -22,7 +22,6 @@ typedef struct BMDMAState {
     uint32_t cur_prd_last;
     uint32_t cur_prd_addr;
     uint32_t cur_prd_len;
-    uint8_t unit;
     BlockDriverCompletionFunc *dma_cb;
     int64_t sector_num;
     uint32_t nsector;
@@ -33,6 +32,8 @@ typedef struct BMDMAState {
     /* Bit 0-2 and 7:   BM status register
      * Bit 3-6:         bus->error_status */
     uint8_t migration_compat_status;
+    uint8_t migration_retry_unit;
+
     struct PCIIDEState *pci_dev;
 } BMDMAState;
 
@@ -61,8 +62,8 @@ typedef struct PCIIDEState {
 
 static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
 {
-    assert(bmdma->unit != (uint8_t)-1);
-    return bmdma->bus->ifs + bmdma->unit;
+    assert(bmdma->bus->dma->aiocb != NULL);
+    return bmdma->bus->ifs + bmdma->bus->retry_unit;
 }
 
 
-- 
1.8.3.1

[Qemu-devel] [PATCH 22/24] ide: place initial state of the current request to IDEBus

[Paolo Bonzini]

This moves more common restarting logic to the core IDE code.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/core.c     |  6 ++++++
 hw/ide/internal.h |  2 ++
 hw/ide/pci.c      | 15 ++++++---------
 hw/ide/pci.h      |  4 ++--
 4 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 970f372..d3f2c4f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -581,6 +581,8 @@ void ide_set_inactive(IDEState *s, bool more)
 {
     s->bus->dma->aiocb = NULL;
     s->bus->retry_unit = -1;
+    s->bus->retry_sector_num = 0;
+    s->bus->retry_nsector = 0;
     if (s->bus->dma->ops->set_inactive) {
         s->bus->dma->ops->set_inactive(s->bus->dma, more);
     }
@@ -726,6 +728,8 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 void ide_start_dma(IDEState *s, BlockDriverCompletionFunc *cb)
 {
     s->bus->retry_unit = s->unit;
+    s->bus->retry_sector_num = ide_get_sector(s);
+    s->bus->retry_nsector = s->nsector;
     if (s->bus->dma->ops->start_dma) {
         s->bus->dma->ops->start_dma(s->bus->dma, s, cb);
     }
@@ -2192,6 +2196,8 @@ static const IDEDMAOps ide_dma_nop_ops = {
 static void ide_restart_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 {
     s->unit = s->bus->retry_unit;
+    ide_set_sector(s, s->bus->retry_sector_num);
+    s->nsector = s->bus->retry_nsector;
     s->bus->dma->ops->restart_dma(s->bus->dma);
     s->io_buffer_index = 0;
     s->io_buffer_size = 0;
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index b1529be..6d2781a 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -461,6 +461,8 @@ struct IDEBus {
 
     int error_status;
     uint8_t retry_unit;
+    int64_t retry_sector_num;
+    uint32_t retry_nsector;
 };
 
 #define TYPE_IDE_DEVICE "ide-device"
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 43507ec..9332844 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -46,8 +46,6 @@ static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
     bm->cur_prd_last = 0;
     bm->cur_prd_addr = 0;
     bm->cur_prd_len = 0;
-    bm->sector_num = ide_get_sector(s);
-    bm->nsector = s->nsector;
 
     if (bm->status & BM_STATUS_DMAING) {
         bm->dma_cb(bmdma_active_if(bm), 0);
@@ -168,10 +166,7 @@ static void bmdma_set_inactive(IDEDMA *dma, bool more)
 static void bmdma_restart_dma(IDEDMA *dma)
 {
     BMDMAState *bm = DO_UPCAST(BMDMAState, dma, dma);
-    IDEState *s = bmdma_active_if(bm);
 
-    ide_set_sector(s, bm->sector_num);
-    s->nsector = bm->nsector;
     bm->cur_addr = bm->addr;
 }
 
@@ -198,8 +193,6 @@ static void bmdma_reset(IDEDMA *dma)
     bm->cur_prd_last = 0;
     bm->cur_prd_addr = 0;
     bm->cur_prd_len = 0;
-    bm->sector_num = 0;
-    bm->nsector = 0;
 }
 
 static void bmdma_irq(void *opaque, int n, int level)
@@ -317,6 +310,8 @@ static void ide_bmdma_pre_save(void *opaque)
     uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
 
     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;
     bm->migration_compat_status =
         (bm->status & ~abused_bits) | (bm->bus->error_status & abused_bits);
 }
@@ -334,6 +329,8 @@ static int ide_bmdma_post_load(void *opaque, int version_id)
         bm->bus->error_status |= bm->migration_compat_status & abused_bits;
     }
     if (bm->bus->error_status) {
+        bm->bus->retry_sector_num = bm->migration_retry_sector_num;
+        bm->bus->retry_nsector = bm->migration_retry_nsector;
         bm->bus->retry_unit = bm->migration_retry_unit;
     }
 
@@ -375,8 +372,8 @@ static const VMStateDescription vmstate_bmdma = {
         VMSTATE_UINT8(cmd, BMDMAState),
         VMSTATE_UINT8(migration_compat_status, BMDMAState),
         VMSTATE_UINT32(addr, BMDMAState),
-        VMSTATE_INT64(sector_num, BMDMAState),
-        VMSTATE_UINT32(nsector, BMDMAState),
+        VMSTATE_INT64(migration_retry_sector_num, BMDMAState),
+        VMSTATE_UINT32(migration_retry_nsector, BMDMAState),
         VMSTATE_UINT8(migration_retry_unit, BMDMAState),
         VMSTATE_END_OF_LIST()
     },
diff --git a/hw/ide/pci.h b/hw/ide/pci.h
index 6f28ce4..3d19070 100644
--- a/hw/ide/pci.h
+++ b/hw/ide/pci.h
@@ -23,8 +23,6 @@ typedef struct BMDMAState {
     uint32_t cur_prd_addr;
     uint32_t cur_prd_len;
     BlockDriverCompletionFunc *dma_cb;
-    int64_t sector_num;
-    uint32_t nsector;
     MemoryRegion addr_ioport;
     MemoryRegion extra_io;
     qemu_irq irq;
@@ -33,6 +31,8 @@ typedef struct BMDMAState {
      * Bit 3-6:         bus->error_status */
     uint8_t migration_compat_status;
     uint8_t migration_retry_unit;
+    int64_t migration_retry_sector_num;
+    uint32_t migration_retry_nsector;
 
     struct PCIIDEState *pci_dev;
 } BMDMAState;
-- 
1.8.3.1

[Qemu-devel] [PATCH 23/24] ide: migrate initial request state via IDEBus

[Paolo Bonzini]

This only breaks backwards migration compatibility if the bus is in
an error state.  It is in principle possible to avoid this by making
two subsections (one for version 1, and one for version 2, but with
the same name) with different "_needed" callbacks.  The v1 callback would
return true if error_status != 0 and the bus is PATA; the v2 callback
would return true if error_status != 0 and the bus is AHCI.

Forward migration keeps working.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/core.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index d3f2c4f..01deeeb 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2544,11 +2544,14 @@ const VMStateDescription vmstate_ide_drive = {
 
 static const VMStateDescription vmstate_ide_error_status = {
     .name ="ide_bus/error",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .fields = (VMStateField []) {
         VMSTATE_INT32(error_status, IDEBus),
+        VMSTATE_INT64_V(retry_sector_num, IDEBus, 2),
+        VMSTATE_UINT32_V(retry_nsector, IDEBus, 2),
+        VMSTATE_UINT8_V(retry_unit, IDEBus, 2),
         VMSTATE_END_OF_LIST()
     }
 };
-- 
1.8.3.1

[Qemu-devel] [PATCH 24/24] ide: commonize io_buffer_index initialization

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/ide/atapi.c | 2 --
 hw/ide/core.c  | 3 +--
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index 6b3f6c1..249b8b1 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -250,7 +250,6 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size)
     s->packet_transfer_size = size;
     s->io_buffer_size = size;    /* dma: send the reply data as one chunk */
     s->elementary_transfer_size = 0;
-    s->io_buffer_index = 0;
 
     if (s->atapi_dma) {
         bdrv_acct_start(s->bs, &s->acct, size, BDRV_ACCT_READ);
@@ -368,7 +367,6 @@ static void ide_atapi_cmd_read_dma(IDEState *s, int lba, int nb_sectors,
 {
     s->lba = lba;
     s->packet_transfer_size = nb_sectors * sector_size;
-    s->io_buffer_index = 0;
     s->io_buffer_size = 0;
     s->cd_sector_size = sector_size;
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index 01deeeb..c3817bd 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -705,7 +705,6 @@ eot:
 static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 {
     s->status = READY_STAT | SEEK_STAT | DRQ_STAT | BUSY_STAT;
-    s->io_buffer_index = 0;
     s->io_buffer_size = 0;
     s->dma_cmd = dma_cmd;
 
@@ -727,6 +726,7 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 
 void ide_start_dma(IDEState *s, BlockDriverCompletionFunc *cb)
 {
+    s->io_buffer_index = 0;
     s->bus->retry_unit = s->unit;
     s->bus->retry_sector_num = ide_get_sector(s);
     s->bus->retry_nsector = s->nsector;
@@ -2199,7 +2199,6 @@ static void ide_restart_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
     ide_set_sector(s, s->bus->retry_sector_num);
     s->nsector = s->bus->retry_nsector;
     s->bus->dma->ops->restart_dma(s->bus->dma);
-    s->io_buffer_index = 0;
     s->io_buffer_size = 0;
     s->dma_cmd = dma_cmd;
     ide_start_dma(s, ide_dma_cb);
-- 
1.8.3.1

Re: [Qemu-devel] [WIP PATCH 00/24] IDE cleanups, initial work on AHCI rerror/werror=stop

[Michael S. Tsirkin]

On Mon, Oct 28, 2013 at 05:43:16PM +0100, Paolo Bonzini wrote:
> Hi all,
> 
> this is some cleanup to the IDE code that I started long ago
> and picked up again on my flight back from KVM Forum...
> 
> The patches clean up the callbacks to prepare for implementing request
> retry for AHCI.  After these patches, PIO retry should work for AHCI and
> non-NCQ retry should be very close (you need an implementation of the
> new restart_dma callback).  NCQ retry and save/restore is AHCI-specific
> and has to be done separately, similar to how we do it for SCSI already.
> 
> Patches 1-4 set up a new (incomplete) testcase.  Patches 5-13 are no-op
> cleanups, or so I hope.  Patches 14-23 genericize retry and make it
> "just work" for all adapters for PIO.  Patch 24 is another cleanup that
> could probably be moved earlier in the series.
> 
> Throwing it out since I know Kevin and mst are working on AHCI, to avoid
> doing duplicate work.  Patches 2 and 4 are known to be racy and
> incomplete.  Extremely light testing, if something breaks tells me and
> I'll look at it.
> 
> Paolo

Well ATM win7 boot is broken with non NCQ (crash on boot).
Do you patches help?

Also, without NCQ win 7 boot will sometime hang.
The following patch by Alex makes it behave better:
it still sometimes hangs for a very long time but with the patch it
will after a long time proceed. Sometimes it proceeds normally.

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index a8be62c..fbea9e8 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -961,7 +961,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
         /* We're ready to process the command in FIS byte 2. */
         ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
 
-        if (s->dev[port].port.ifs[0].status & READY_STAT) {
+        if ((s->dev[port].port.ifs[0].status & (READY_STAT|DRQ_STAT|BUSY_STAT)) ==
+            READY_STAT) {
             ahci_write_fis_d2h(&s->dev[port], cmd_fis);
         }
     }

Re: [Qemu-devel] [WIP PATCH 00/24] IDE cleanups, initial work on AHCI rerror/werror=stop

[Paolo Bonzini]

Il 28/10/2013 18:16, Michael S. Tsirkin ha scritto:
> On Mon, Oct 28, 2013 at 05:43:16PM +0100, Paolo Bonzini wrote:
>> Hi all,
>>
>> this is some cleanup to the IDE code that I started long ago
>> and picked up again on my flight back from KVM Forum...
>>
>> The patches clean up the callbacks to prepare for implementing request
>> retry for AHCI.  After these patches, PIO retry should work for AHCI and
>> non-NCQ retry should be very close (you need an implementation of the
>> new restart_dma callback).  NCQ retry and save/restore is AHCI-specific
>> and has to be done separately, similar to how we do it for SCSI already.
>>
>> Patches 1-4 set up a new (incomplete) testcase.  Patches 5-13 are no-op
>> cleanups, or so I hope.  Patches 14-23 genericize retry and make it
>> "just work" for all adapters for PIO.  Patch 24 is another cleanup that
>> could probably be moved earlier in the series.
>>
>> Throwing it out since I know Kevin and mst are working on AHCI, to avoid
>> doing duplicate work.  Patches 2 and 4 are known to be racy and
>> incomplete.  Extremely light testing, if something breaks tells me and
>> I'll look at it.
> 
> Well ATM win7 boot is broken with non NCQ (crash on boot).
> Do you patches help?

No, these are for another feature that is missing for AHCI, that is
support for stopping the VM on I/O errors.  It is
https://bugzilla.redhat.com/show_bug.cgi?id=887844

> Also, without NCQ win 7 boot will sometime hang.
> The following patch by Alex makes it behave better:
> it still sometimes hangs for a very long time but with the patch it
> will after a long time proceed. Sometimes it proceeds normally.

Yeah, I saw you and Kevin discuss it on IRC.

Paolo

> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index a8be62c..fbea9e8 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -961,7 +961,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
>          /* We're ready to process the command in FIS byte 2. */
>          ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
>  
> -        if (s->dev[port].port.ifs[0].status & READY_STAT) {
> +        if ((s->dev[port].port.ifs[0].status & (READY_STAT|DRQ_STAT|BUSY_STAT)) ==
> +            READY_STAT) {
>              ahci_write_fis_d2h(&s->dev[port], cmd_fis);
>          }
>      }
> 

Re: [Qemu-devel] [WIP PATCH 00/24] IDE cleanups, initial work on AHCI rerror/werror=stop

[Michael S. Tsirkin]

On Mon, Oct 28, 2013 at 06:26:58PM +0100, Paolo Bonzini wrote:
> Il 28/10/2013 18:16, Michael S. Tsirkin ha scritto:
> > On Mon, Oct 28, 2013 at 05:43:16PM +0100, Paolo Bonzini wrote:
> >> Hi all,
> >>
> >> this is some cleanup to the IDE code that I started long ago
> >> and picked up again on my flight back from KVM Forum...
> >>
> >> The patches clean up the callbacks to prepare for implementing request
> >> retry for AHCI.  After these patches, PIO retry should work for AHCI and
> >> non-NCQ retry should be very close (you need an implementation of the
> >> new restart_dma callback).  NCQ retry and save/restore is AHCI-specific
> >> and has to be done separately, similar to how we do it for SCSI already.
> >>
> >> Patches 1-4 set up a new (incomplete) testcase.  Patches 5-13 are no-op
> >> cleanups, or so I hope.  Patches 14-23 genericize retry and make it
> >> "just work" for all adapters for PIO.  Patch 24 is another cleanup that
> >> could probably be moved earlier in the series.
> >>
> >> Throwing it out since I know Kevin and mst are working on AHCI, to avoid
> >> doing duplicate work.  Patches 2 and 4 are known to be racy and
> >> incomplete.  Extremely light testing, if something breaks tells me and
> >> I'll look at it.
> > 
> > Well ATM win7 boot is broken with non NCQ (crash on boot).
> > Do you patches help?
> 
> No, these are for another feature that is missing for AHCI, that is
> support for stopping the VM on I/O errors.  It is
> https://bugzilla.redhat.com/show_bug.cgi?id=887844

Hmm. Actually maybe there was an io error... is there
an easy way to find out?

> > Also, without NCQ win 7 boot will sometime hang.
> > The following patch by Alex makes it behave better:
> > it still sometimes hangs for a very long time but with the patch it
> > will after a long time proceed. Sometimes it proceeds normally.
> 
> Yeah, I saw you and Kevin discuss it on IRC.
> 
> Paolo
> 
> > diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> > index a8be62c..fbea9e8 100644
> > --- a/hw/ide/ahci.c
> > +++ b/hw/ide/ahci.c
> > @@ -961,7 +961,8 @@ static int handle_cmd(AHCIState *s, int port, int slot)
> >          /* We're ready to process the command in FIS byte 2. */
> >          ide_exec_cmd(&s->dev[port].port, cmd_fis[2]);
> >  
> > -        if (s->dev[port].port.ifs[0].status & READY_STAT) {
> > +        if ((s->dev[port].port.ifs[0].status & (READY_STAT|DRQ_STAT|BUSY_STAT)) ==
> > +            READY_STAT) {
> >              ahci_write_fis_d2h(&s->dev[port], cmd_fis);
> >          }
> >      }
> > 

Re: [Qemu-devel] [WIP PATCH 00/24] IDE cleanups, initial work on AHCI rerror/werror=stop

[Paolo Bonzini]

Il 28/10/2013 19:00, Michael S. Tsirkin ha scritto:
> > No, these are for another feature that is missing for AHCI, that is
> > support for stopping the VM on I/O errors.  It is
> > https://bugzilla.redhat.com/show_bug.cgi?id=887844
> 
> Hmm. Actually maybe there was an io error... is there
> an easy way to find out?

This would be a *host* I/O error, so I doubt.

Paolo

Re: [Qemu-devel] [WIP PATCH 00/24] IDE cleanups, initial work on AHCI rerror/werror=stop

[Michael S. Tsirkin]

On Mon, Oct 28, 2013 at 07:36:32PM +0100, Paolo Bonzini wrote:
> Il 28/10/2013 19:00, Michael S. Tsirkin ha scritto:
> > > No, these are for another feature that is missing for AHCI, that is
> > > support for stopping the VM on I/O errors.  It is
> > > https://bugzilla.redhat.com/show_bug.cgi?id=887844
> > 
> > Hmm. Actually maybe there was an io error... is there
> > an easy way to find out?
> 
> This would be a *host* I/O error, so I doubt.
> 
> Paolo

Yes, I know. Something like an out of range access on a disk
can trigger it perhaps?

Re: [Qemu-devel] [WIP PATCH 00/24] IDE cleanups, initial work on AHCI rerror/werror=stop

[Paolo Bonzini]

Il 28/10/2013 19:42, Michael S. Tsirkin ha scritto:
> > > Hmm. Actually maybe there was an io error... is there
> > > an easy way to find out?
> > 
> > This would be a *host* I/O error, so I doubt.
> 
> Yes, I know. Something like an out of range access on a disk
> can trigger it perhaps?

Yes, that could happen.  The easiest way to find out is probably very
low tech: a conditional breakpoint on bdrv_aio_readv or bdrv_aio_writev.

Paolo

Re: [Qemu-devel] [WIP PATCH 00/24] IDE cleanups, initial work on AHCI rerror/werror=stop

[Kevin Wolf]

Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
> Hi all,
> 
> this is some cleanup to the IDE code that I started long ago
> and picked up again on my flight back from KVM Forum...
> 
> The patches clean up the callbacks to prepare for implementing request
> retry for AHCI.  After these patches, PIO retry should work for AHCI and
> non-NCQ retry should be very close (you need an implementation of the
> new restart_dma callback).  NCQ retry and save/restore is AHCI-specific
> and has to be done separately, similar to how we do it for SCSI already.
> 
> Patches 1-4 set up a new (incomplete) testcase.  Patches 5-13 are no-op
> cleanups, or so I hope.  Patches 14-23 genericize retry and make it
> "just work" for all adapters for PIO.  Patch 24 is another cleanup that
> could probably be moved earlier in the series.
> 
> Throwing it out since I know Kevin and mst are working on AHCI, to avoid
> doing duplicate work.  Patches 2 and 4 are known to be racy and
> incomplete.  Extremely light testing, if something breaks tells me and
> I'll look at it.

You forgot some CCs it seems. You're lucky that I'm reading qemu-devel
today, for the first time in two weeks or so. CCing myself so I can
hopefully have a look tomorrow.

Kevin

Re: [Qemu-devel] [WIP PATCH 00/24] IDE cleanups, initial work on AHCI rerror/werror=stop

[Paolo Bonzini]

Il 29/10/2013 19:30, Kevin Wolf ha scritto:
> Am 28.10.2013 um 17:43 hat Paolo Bonzini geschrieben:
>> Hi all,
>>
>> this is some cleanup to the IDE code that I started long ago
>> and picked up again on my flight back from KVM Forum...
>>
>> The patches clean up the callbacks to prepare for implementing request
>> retry for AHCI.  After these patches, PIO retry should work for AHCI and
>> non-NCQ retry should be very close (you need an implementation of the
>> new restart_dma callback).  NCQ retry and save/restore is AHCI-specific
>> and has to be done separately, similar to how we do it for SCSI already.
>>
>> Patches 1-4 set up a new (incomplete) testcase.  Patches 5-13 are no-op
>> cleanups, or so I hope.  Patches 14-23 genericize retry and make it
>> "just work" for all adapters for PIO.  Patch 24 is another cleanup that
>> could probably be moved earlier in the series.
>>
>> Throwing it out since I know Kevin and mst are working on AHCI, to avoid
>> doing duplicate work.  Patches 2 and 4 are known to be racy and
>> incomplete.  Extremely light testing, if something breaks tells me and
>> I'll look at it.
> 
> You forgot some CCs it seems. You're lucky that I'm reading qemu-devel
> today, for the first time in two weeks or so. CCing myself so I can
> hopefully have a look tomorrow.

While you finish reviewing, I pushed a version that actually boots
guests on my github repo's ide-cb branch.  I have added a patch to
enable restart for the ISA IDE controller; it cannot be tested the same
way as the PCI one (no ioapic on -M isapc), but it is less hacky than
what I had done for AHCI.

Paolo