[Qemu-devel] [PATCH v2 00/17] ide: rerror and werror support for IDE and AHCI

[Qemu-devel] [PATCH v2 00/17] ide: rerror and werror support for IDE and AHCI

[John Snow]

This series was written mostly by Paolo Bonzini to do two things:

1. Unify the restart callbacks for ISA, AHCI and BMDMA
2. Ensure we can restart a command after migration

Many of the early patches only make much sense considering the
end-goal of eliminating BMDMA specific restart code to be shared
with ISA and AHCI codepaths.

Migration for halted commands is fixed for ISA, PCI and AHCI.
As a consequence, operations halted via rerror=stop or werror=stop
should be able to be successfully migrated and resumed when using
ISA, PCI, or AHCI.

This series includes tests for ISA and PCI/BMDMA, but does not
yet include tests for AHCI, which require some more qtest work
to be upstreamed first. Regardless, the AHCI tests have been
written and can be observed at:
https://github.com/jnsnow/qemu/commits/ahci-devel-latest

See "ahci: add migrate dma test" and "ahci-test: add flush migrate test"
for the WIP versions of the AHCI test that I used to exercise this
patchset.

John Snow (3):
  ahci: Migrate IDEStatus
  ahci: Recompute cur_cmd on migrate post load
  qtest/ide: Test flush / retry for ISA and PCI

Paolo Bonzini (14):
  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
  ide: make more functions static
  ide: support PIO restart for the ISA controller
  ahci: add support for restarting non-queued commands

 hw/ide/ahci.c     |  37 +++++++++---------
 hw/ide/atapi.c    |   3 +-
 hw/ide/cmd646.c   |   3 +-
 hw/ide/core.c     | 109 +++++++++++++++++++++++++++++++++++++++++++++++-------
 hw/ide/internal.h |  16 +++++---
 hw/ide/isa.c      |   3 +-
 hw/ide/macio.c    |   6 ---
 hw/ide/pci.c      |  98 ++++++++----------------------------------------
 hw/ide/pci.h      |  12 +++---
 hw/ide/piix.c     |   3 +-
 hw/ide/via.c      |   3 +-
 tests/ide-test.c  |  20 +++++++---
 12 files changed, 165 insertions(+), 148 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v2 01/17] ide: start extracting ide_restart_dma out of bmdma_restart_dma

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

This patch begins refactoring the restart dma functions
out of bmdma to be shared with AHCI and other future
IDE HBA implementations.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/pci.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index bee5ad3..01f3cd2 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -184,18 +184,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->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);
+}
+
+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->dma_cmd = dma_cmd;
+    bmdma_start_dma(&bm->dma, s, ide_dma_cb);
 }
 
 /* TODO This should be common IDE code */
@@ -203,6 +209,7 @@ static void bmdma_restart_bh(void *opaque)
 {
     BMDMAState *bm = opaque;
     IDEBus *bus = bm->bus;
+    IDEState *s;
     bool is_read;
     int error_status;
 
@@ -213,6 +220,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
@@ -223,18 +231,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.9.3

[Qemu-devel] [PATCH v2 02/17] ide: prepare to move restart to common code

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

This patch adds the restart_dma callback and adjusts
the ide_restart_dma function to utilize this callback
to call the BMDMA-specific restart code instead of statically
executing BMDMA-specific code.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@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 8a3eca4..59e24e2 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -435,6 +435,7 @@ struct IDEDMAOps {
     DMAu32Func *commit_buf;
     DMAIntFunc *rw_buf;
     DMAIntFunc *set_unit;
+    DMAVoidFunc *restart_dma;
     DMAStopFunc *set_inactive;
     DMAVoidFunc *cmd_done;
     DMARestartFunc *restart_cb;
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index 01f3cd2..71c7f2e 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -184,8 +184,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);
@@ -195,13 +196,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 */
@@ -521,6 +522,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,
     .set_inactive = bmdma_set_inactive,
     .restart_cb = bmdma_restart_cb,
     .reset = bmdma_reset,
-- 
1.9.3

[Qemu-devel] [PATCH v2 03/17] ide: introduce ide_register_restart_cb

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

A helper is added that registers the IDEDMAOp .restart_cb()
via qemu_add_vm_change_state_handler instead of requiring
each HBA to register the callback themselves.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@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 c8b0322..0374653 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -368,8 +368,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 d4af5e2..179a001 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2330,6 +2330,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 59e24e2..73c5e63 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -548,6 +548,7 @@ int ide_init_drive(IDEState *s, BlockBackend *blk, IDEDriveKind kind,
                    int chs_trans);
 void ide_init2(IDEBus *bus, 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 b0172fb..247dd4f 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 4d8089d..fb3d8a0 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.9.3

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

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

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>
Signed-off-by: John Snow <jsnow@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 71c7f2e..eb480f6 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -217,17 +217,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.9.3

[Qemu-devel] [PATCH v2 05/17] ide: pass IDEBus to the restart_cb

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Pass the containing IDEBus to the restart_cb instead
of the more specific BMDMAState child.

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 179a001..a836626 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2332,7 +2332,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 eb480f6..e5caf11 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -208,8 +208,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;
@@ -249,14 +249,14 @@ 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;
 
     if (!bm->bh) {
-        bm->bh = qemu_bh_new(bmdma_restart_bh, &bm->dma);
+        bm->bh = qemu_bh_new(bmdma_restart_bh, bus);
         qemu_bh_schedule(bm->bh);
     }
 }
-- 
1.9.3

[Qemu-devel] [PATCH v2 06/17] ide: move restart callback to common code

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

With BMDMA specific excised from the restart functions,
create a HBA-agnostic restart callback to be shared
between the different HBAs.

Change the callback registered with the vmstate_change
handler to always point to ide_restart_cb instead of
relying on the IDEDMAOps.restart_cb() member.

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index a836626..15b6a3f 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2314,6 +2314,10 @@ static int ide_nop_int(IDEDMA *dma, int x)
     return 0;
 }
 
+static void ide_nop(IDEDMA *dma)
+{
+}
+
 static int32_t ide_nop_int32(IDEDMA *dma, int x)
 {
     return 0;
@@ -2325,14 +2329,79 @@ static void ide_nop_restart(void *opaque, int x, RunState y)
 
 static const IDEDMAOps ide_dma_nop_ops = {
     .prepare_buf    = ide_nop_int32,
+    .restart_dma    = ide_nop,
     .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)
+{
+    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 73c5e63..7ea64d1 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -454,6 +454,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 e5caf11..cd1bbb0 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -194,73 +194,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, bus);
-        qemu_bh_schedule(bm->bh);
-    }
-}
-
 static void bmdma_cancel(BMDMAState *bm)
 {
     if (bm->status & BM_STATUS_DMAING) {
@@ -524,7 +457,6 @@ static const struct IDEDMAOps bmdma_ops = {
     .set_unit = bmdma_set_unit,
     .restart_dma = bmdma_restart_dma,
     .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 2e9314a..ea4e051 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.9.3

[Qemu-devel] [PATCH v2 07/17] ide: remove restart_cb callback

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

With restarts now handled by ide_restart_cb and
the IDEDMAOps.restart_dma() member, remove the old
restart_cb callback.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@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 5651372..3892025 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1252,10 +1252,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,
@@ -1264,7 +1260,6 @@ static const IDEDMAOps ahci_dma_ops = {
     .rw_buf = ahci_dma_rw_buf,
     .set_unit = ahci_dma_set_unit,
     .cmd_done = ahci_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 15b6a3f..f3d98d7 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2323,16 +2323,11 @@ static int32_t ide_nop_int32(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_int32,
     .restart_dma    = ide_nop,
     .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 7ea64d1..53ce932 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -438,7 +438,6 @@ struct IDEDMAOps {
     DMAVoidFunc *restart_dma;
     DMAStopFunc *set_inactive;
     DMAVoidFunc *cmd_done;
-    DMARestartFunc *restart_cb;
     DMAVoidFunc *reset;
 };
 
diff --git a/hw/ide/macio.c b/hw/ide/macio.c
index f6074f2..e7a5c99 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -558,10 +558,6 @@ static int32_t ide_nop_int32(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,
                             BlockCompletionFunc *cb)
 {
@@ -577,7 +573,6 @@ static const IDEDMAOps dbdma_ops = {
     .prepare_buf    = ide_nop_int32,
     .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.9.3

[Qemu-devel] [PATCH v2 08/17] ide: replace set_unit callback with more IDEBus state

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

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>
Signed-off-by: John Snow <jsnow@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      |  6 ++++--
 6 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 3892025..bc6d5ce 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1226,12 +1226,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_cmd_done(IDEDMA *dma)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
@@ -1258,7 +1252,6 @@ static const IDEDMAOps ahci_dma_ops = {
     .prepare_buf = ahci_dma_prepare_buf,
     .commit_buf = ahci_commit_buf,
     .rw_buf = ahci_dma_rw_buf,
-    .set_unit = ahci_dma_set_unit,
     .cmd_done = ahci_cmd_done,
 };
 
diff --git a/hw/ide/core.c b/hw/ide/core.c
index f3d98d7..fc5d227 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -646,6 +646,7 @@ static void dma_buf_commit(IDEState *s, uint32_t tx_bytes)
 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);
     }
@@ -666,7 +667,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
     BlockErrorAction action = blk_get_error_action(s->blk, is_read, error);
 
     if (action == BLOCK_ERROR_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 == BLOCK_ERROR_ACTION_REPORT) {
         if (op & IDE_RETRY_DMA) {
@@ -799,6 +800,7 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 
 void ide_start_dma(IDEState *s, BlockCompletionFunc *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);
     }
@@ -2327,11 +2329,11 @@ static const IDEDMAOps ide_dma_nop_ops = {
     .prepare_buf    = ide_nop_int32,
     .restart_dma    = ide_nop,
     .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 53ce932..07d82c4 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -434,7 +434,6 @@ struct IDEDMAOps {
     DMAInt32Func *prepare_buf;
     DMAu32Func *commit_buf;
     DMAIntFunc *rw_buf;
-    DMAIntFunc *set_unit;
     DMAVoidFunc *restart_dma;
     DMAStopFunc *set_inactive;
     DMAVoidFunc *cmd_done;
@@ -463,6 +462,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 e7a5c99..a009674 100644
--- a/hw/ide/macio.c
+++ b/hw/ide/macio.c
@@ -572,7 +572,6 @@ static const IDEDMAOps dbdma_ops = {
     .start_dma      = ide_dbdma_start,
     .prepare_buf    = ide_nop_int32,
     .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 cd1bbb0..2b0e886 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;
@@ -163,20 +162,11 @@ 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_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 {
@@ -335,6 +325,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);
 }
@@ -351,6 +342,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;
 }
@@ -389,7 +383,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 []) {
@@ -413,7 +407,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);
     }
 
@@ -454,7 +448,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,
     .set_inactive = bmdma_set_inactive,
     .reset = bmdma_reset,
diff --git a/hw/ide/pci.h b/hw/ide/pci.h
index ea4e051..222a163 100644
--- a/hw/ide/pci.h
+++ b/hw/ide/pci.h
@@ -33,6 +33,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 +63,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->retry_unit != (uint8_t)-1);
+    return bmdma->bus->ifs + bmdma->bus->retry_unit;
 }
 
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 09/17] ide: place initial state of the current request to IDEBus

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

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

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index fc5d227..d4659dc 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -647,6 +647,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);
     }
@@ -801,6 +803,8 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 void ide_start_dma(IDEState *s, BlockCompletionFunc *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);
     }
@@ -2334,6 +2338,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 07d82c4..da2230f 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -463,6 +463,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 2b0e886..fab2abc 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);
@@ -177,10 +175,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;
 }
 
@@ -207,8 +202,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)
@@ -326,6 +319,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);
 }
@@ -343,6 +338,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;
     }
 
@@ -381,8 +378,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 222a163..0f2d4b9 100644
--- a/hw/ide/pci.h
+++ b/hw/ide/pci.h
@@ -22,10 +22,7 @@ typedef struct BMDMAState {
     uint32_t cur_prd_last;
     uint32_t cur_prd_addr;
     uint32_t cur_prd_len;
-    uint8_t unit;
     BlockCompletionFunc *dma_cb;
-    int64_t sector_num;
-    uint32_t nsector;
     MemoryRegion addr_ioport;
     MemoryRegion extra_io;
     qemu_irq irq;
@@ -34,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.9.3

[Qemu-devel] [PATCH v2 10/17] ide: migrate initial request state via IDEBus

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

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>
Signed-off-by: John Snow <jsnow@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 d4659dc..0459ea1 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -2633,10 +2633,13 @@ 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,
     .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.9.3

[Qemu-devel] [PATCH v2 11/17] ide: commonize io_buffer_index initialization

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Resetting the io_buffer_index to 0 is commonized,
with the exception of the case within ide_atapi_cmd_reply,
where we need to reset this index to 0 prior to the
ide_atapi_cmd_reply_end call.

Note that not all calls to ide_atapi_cmd_reply_end
expect the index to be 0, so setting it there is
not appropriate.

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

diff --git a/hw/ide/atapi.c b/hw/ide/atapi.c
index c63b7e5..ef620e8 100644
--- a/hw/ide/atapi.c
+++ b/hw/ide/atapi.c
@@ -252,7 +252,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) {
         block_acct_start(blk_get_stats(s->blk), &s->acct, size,
@@ -261,6 +260,7 @@ static void ide_atapi_cmd_reply(IDEState *s, int size, int max_size)
         ide_start_dma(s, ide_atapi_cmd_read_dma_cb);
     } else {
         s->status = READY_STAT | SEEK_STAT;
+        s->io_buffer_index = 0;
         ide_atapi_cmd_reply_end(s);
     }
 }
@@ -368,7 +368,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 0459ea1..b89cde0 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -780,7 +780,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;
 
@@ -802,6 +801,7 @@ static void ide_sector_start_dma(IDEState *s, enum ide_dma_cmd dma_cmd)
 
 void ide_start_dma(IDEState *s, BlockCompletionFunc *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;
@@ -2341,7 +2341,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.9.3

[Qemu-devel] [PATCH v2 12/17] ide: make more functions static

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index b89cde0..66ab93d 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -561,6 +561,8 @@ static bool ide_sect_range_ok(IDEState *s,
     return true;
 }
 
+static void ide_sector_read(IDEState *s);
+
 static void ide_sector_read_cb(void *opaque, int ret)
 {
     IDEState *s = opaque;
@@ -595,7 +597,7 @@ static void ide_sector_read_cb(void *opaque, int ret)
     s->io_buffer_offset += 512 * n;
 }
 
-void ide_sector_read(IDEState *s)
+static void ide_sector_read(IDEState *s)
 {
     int64_t sector_num;
     int n;
@@ -682,7 +684,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
     return action != BLOCK_ERROR_ACTION_IGNORE;
 }
 
-void ide_dma_cb(void *opaque, int ret)
+static void ide_dma_cb(void *opaque, int ret)
 {
     IDEState *s = opaque;
     int n;
@@ -810,6 +812,8 @@ void ide_start_dma(IDEState *s, BlockCompletionFunc *cb)
     }
 }
 
+static void ide_sector_write(IDEState *s);
+
 static void ide_sector_write_timer_cb(void *opaque)
 {
     IDEState *s = opaque;
@@ -869,7 +873,7 @@ static void ide_sector_write_cb(void *opaque, int ret)
     }
 }
 
-void ide_sector_write(IDEState *s)
+static void ide_sector_write(IDEState *s)
 {
     int64_t sector_num;
     int n;
@@ -923,7 +927,7 @@ static void ide_flush_cb(void *opaque, int ret)
     ide_set_irq(s->bus);
 }
 
-void ide_flush_cache(IDEState *s)
+static void ide_flush_cache(IDEState *s)
 {
     if (s->blk == NULL) {
         ide_flush_cb(s, 0);
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index da2230f..0beba43 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -554,10 +554,6 @@ 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);
-void ide_sector_write(IDEState *s);
-void ide_sector_read(IDEState *s);
-void ide_flush_cache(IDEState *s);
 
 void ide_transfer_start(IDEState *s, uint8_t *buf, int size,
                         EndTransferFunc *end_transfer_func);
-- 
1.9.3

[Qemu-devel] [PATCH v2 13/17] ide: support PIO restart for the ISA controller

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/isa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/ide/isa.c b/hw/ide/isa.c
index b084162..5eb35c2 100644
--- a/hw/ide/isa.c
+++ b/hw/ide/isa.c
@@ -74,7 +74,8 @@ static void isa_ide_realizefn(DeviceState *dev, Error **errp)
     isa_init_irq(isadev, &s->irq, s->isairq);
     ide_init2(&s->bus, s->irq);
     vmstate_register(dev, 0, &vmstate_ide_isa, s);
-};
+    ide_register_restart_cb(&s->bus);
+}
 
 ISADevice *isa_ide_init(ISABus *bus, int iobase, int iobase2, int isairq,
                         DriveInfo *hd0, DriveInfo *hd1)
-- 
1.9.3

[Qemu-devel] [PATCH v2 14/17] ahci: Migrate IDEStatus

[John Snow]

Amazingly, we weren't doing this before.

Make sure we migrate the IDEState structure that belongs to
the AHCIDevice.IDEBus structure during migrations.

No version numbering changes because AHCI is not officially
migratable (and we can all see with good reason why) so we
do not impact any official builds by altering the stream and
leaving it at version 1.

This fixes the rerror=stop/werror=stop test case where we wish
to migrate a halted job. Previously, the error code would not
migrate, so even if the job completed successfully, AHCI would
report an error because it would still have the placeholder
error code from initialization time.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c     | 1 +
 hw/ide/internal.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index bc6d5ce..3f4fc77 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1321,6 +1321,7 @@ static const VMStateDescription vmstate_ahci_device = {
     .version_id = 1,
     .fields = (VMStateField[]) {
         VMSTATE_IDE_BUS(port, AHCIDevice),
+        VMSTATE_IDE_DRIVE(port.ifs[0], AHCIDevice),
         VMSTATE_UINT32(port_state, AHCIDevice),
         VMSTATE_UINT32(finished, AHCIDevice),
         VMSTATE_UINT32(port_regs.lst_addr, AHCIDevice),
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index 0beba43..c278dea 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -524,6 +524,9 @@ extern const VMStateDescription vmstate_ide_drive;
 #define VMSTATE_IDE_DRIVES(_field, _state) \
     VMSTATE_STRUCT_ARRAY(_field, _state, 2, 3, vmstate_ide_drive, IDEState)
 
+#define VMSTATE_IDE_DRIVE(_field, _state) \
+    VMSTATE_STRUCT(_field, _state, 1, vmstate_ide_drive, IDEState)
+
 void ide_bus_reset(IDEBus *bus);
 int64_t ide_get_sector(IDEState *s);
 void ide_set_sector(IDEState *s, int64_t sector_num);
-- 
1.9.3

[Qemu-devel] [PATCH v2 15/17] ahci: add support for restarting non-queued commands

[John Snow]

From: Paolo Bonzini <pbonzini@redhat.com>

This is easy, since start_dma already restarts processing from the
beginning of the PRDT.

Migration is also easy to cover; the comment about busy_slot is
wrong, busy_slot will only be set if there is an error.  In this
case we have nothing to do really.  The core IDE code will restart
the operation and command list processing will proceed after the
erroring command has been completed.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 3f4fc77..c153228 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1160,6 +1160,11 @@ static void ahci_start_dma(IDEDMA *dma, IDEState *s,
     dma_cb(s, 0);
 }
 
+static void ahci_restart_dma(IDEDMA *dma)
+{
+    /* Nothing to do, ahci_start_dma already resets s->io_buffer_offset.  */
+}
+
 /**
  * Called in DMA R/W chains to read the PRDT, utilizing ahci_populate_sglist.
  * Not currently invoked by PIO R/W chains,
@@ -1248,6 +1253,7 @@ static void ahci_irq_set(void *opaque, int n, int level)
 
 static const IDEDMAOps ahci_dma_ops = {
     .start_dma = ahci_start_dma,
+    .restart_dma = ahci_restart_dma,
     .start_transfer = ahci_start_transfer,
     .prepare_buf = ahci_dma_prepare_buf,
     .commit_buf = ahci_commit_buf,
@@ -1282,6 +1288,7 @@ void ahci_init(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports)
         ad->port_no = i;
         ad->port.dma = &ad->dma;
         ad->port.dma->ops = &ahci_dma_ops;
+        ide_register_restart_cb(&ad->port);
     }
 }
 
@@ -1360,16 +1367,13 @@ static int ahci_state_post_load(void *opaque, int version_id)
         map_page(s->as, &ad->res_fis,
                  ((uint64_t)pr->fis_addr_hi << 32) | pr->fis_addr, 256);
         /*
-         * All pending i/o should be flushed out on a migrate. However,
-         * we might not have cleared the busy_slot since this is done
-         * in a bh. Also, issue i/o against any slots that are pending.
+         * If an error was there, ad->busy_slot will not be -1.  Restarting
+         * the operation will resume execution of the command list, do
+         * nothing here.
          */
-        if ((ad->busy_slot != -1) &&
-            !(ad->port.ifs[0].status & (BUSY_STAT|DRQ_STAT))) {
-            pr->cmd_issue &= ~(1 << ad->busy_slot);
-            ad->busy_slot = -1;
+        if (ad->busy_slot == -1) {
+            check_cmd(s, i);
         }
-        check_cmd(s, i);
     }
 
     return 0;
-- 
1.9.3

[Qemu-devel] [PATCH v2 16/17] ahci: Recompute cur_cmd on migrate post load

[John Snow]

When the AHCI HBA device is migrated, all of the information that
led to the request being created is stored in the AHCIDevice
structures, except for pointers into guest data where return
information needs to be stored.

The "cur_cmd" field is usually responsible for this.

To rebuild the cur_cmd pointer post-migration, we can utilize
the busy_slot index to figure out where the command header
we are still processing is.

This allows a machine in a halted state from rerror=stop or
werror=stop to be migrated and resume operations without issue.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 hw/ide/ahci.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index c153228..8078d3e 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1373,6 +1373,10 @@ static int ahci_state_post_load(void *opaque, int version_id)
          */
         if (ad->busy_slot == -1) {
             check_cmd(s, i);
+        } else {
+            /* We are in the middle of a command, and may need to access
+             * the command header in guest memory again. */
+            ad->cur_cmd = &((AHCICmdHdr *)ad->lst)[ad->busy_slot];
         }
     }
 
-- 
1.9.3

[Qemu-devel] [PATCH v2 17/17] qtest/ide: Test flush / retry for ISA and PCI

[John Snow]

This patch adds tests for werror and rerror functionality
for the PCI and ISA ide buses.

Tests for the AHCI device are to be included at a later
date after requisite patches have been merged upstream
to support needed functionality by the tests.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/ide-test.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index 29f4039..b28a302 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -118,7 +118,6 @@ static void ide_test_start(const char *cmdline_fmt, ...)
     va_end(ap);
 
     qtest_start(cmdline);
-    qtest_irq_intercept_in(global_qtest, "ioapic");
     guest_malloc = pc_alloc_init();
 
     g_free(cmdline);
@@ -388,6 +387,7 @@ static void test_bmdma_setup(void)
         "-drive file=%s,if=ide,serial=%s,cache=writeback,format=raw "
         "-global ide-hd.ver=%s",
         tmp_path, "testdisk", "version");
+    qtest_irq_intercept_in(global_qtest, "ioapic");
 }
 
 static void test_bmdma_teardown(void)
@@ -516,7 +516,7 @@ static void prepare_blkdebug_script(const char *debug_fn, const char *event)
     g_assert(ret == 0);
 }
 
-static void test_retry_flush(void)
+static void test_retry_flush(const char *machine)
 {
     uint8_t data;
     const char *s;
@@ -580,6 +580,16 @@ static void test_flush_nodev(void)
     ide_test_quit();
 }
 
+static void test_pci_retry_flush(const char *machine)
+{
+    test_retry_flush("pc");
+}
+
+static void test_isa_retry_flush(const char *machine)
+{
+    test_retry_flush("isapc");
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -617,9 +627,9 @@ int main(int argc, char **argv)
     qtest_add_func("/ide/bmdma/teardown", test_bmdma_teardown);
 
     qtest_add_func("/ide/flush", test_flush);
-    qtest_add_func("/ide/flush_nodev", test_flush_nodev);
-
-    qtest_add_func("/ide/retry/flush", test_retry_flush);
+    qtest_add_func("/ide/flush/nodev", test_flush_nodev);
+    qtest_add_func("/ide/flush/retry_pci", test_pci_retry_flush);
+    qtest_add_func("/ide/flush/retry_isa", test_isa_retry_flush);
 
     ret = g_test_run();
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v2 14/17] ahci: Migrate IDEStatus

[John Snow]

On 12/16/2014 08:36 PM, John Snow wrote:
> Amazingly, we weren't doing this before.
>
> Make sure we migrate the IDEState structure that belongs to
> the AHCIDevice.IDEBus structure during migrations.
>
> No version numbering changes because AHCI is not officially
> migratable (and we can all see with good reason why) so we
> do not impact any official builds by altering the stream and
> leaving it at version 1.
>
> This fixes the rerror=stop/werror=stop test case where we wish
> to migrate a halted job. Previously, the error code would not
> migrate, so even if the job completed successfully, AHCI would
> report an error because it would still have the placeholder
> error code from initialization time.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   hw/ide/ahci.c     | 1 +
>   hw/ide/internal.h | 3 +++
>   2 files changed, 4 insertions(+)
>
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index bc6d5ce..3f4fc77 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1321,6 +1321,7 @@ static const VMStateDescription vmstate_ahci_device = {
>       .version_id = 1,
>       .fields = (VMStateField[]) {
>           VMSTATE_IDE_BUS(port, AHCIDevice),
> +        VMSTATE_IDE_DRIVE(port.ifs[0], AHCIDevice),
>           VMSTATE_UINT32(port_state, AHCIDevice),
>           VMSTATE_UINT32(finished, AHCIDevice),
>           VMSTATE_UINT32(port_regs.lst_addr, AHCIDevice),
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 0beba43..c278dea 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -524,6 +524,9 @@ extern const VMStateDescription vmstate_ide_drive;
>   #define VMSTATE_IDE_DRIVES(_field, _state) \
>       VMSTATE_STRUCT_ARRAY(_field, _state, 2, 3, vmstate_ide_drive, IDEState)
>
> +#define VMSTATE_IDE_DRIVE(_field, _state) \
> +    VMSTATE_STRUCT(_field, _state, 1, vmstate_ide_drive, IDEState)
> +

Please check that this versioning is sane; I tested that it let me 
migrate AHCI successfully, but I don't know if this is semantically "right."

Please and thank you!
--js

>   void ide_bus_reset(IDEBus *bus);
>   int64_t ide_get_sector(IDEState *s);
>   void ide_set_sector(IDEState *s, int64_t sector_num);
>

Re: [Qemu-devel] [PATCH v2 00/17] ide: rerror and werror support for IDE and AHCI

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This series was written mostly by Paolo Bonzini to do two things:
>
> 1. Unify the restart callbacks for ISA, AHCI and BMDMA
> 2. Ensure we can restart a command after migration
>
> Many of the early patches only make much sense considering the
> end-goal of eliminating BMDMA specific restart code to be shared
> with ISA and AHCI codepaths.
>
> Migration for halted commands is fixed for ISA, PCI and AHCI.
> As a consequence, operations halted via rerror=stop or werror=stop
> should be able to be successfully migrated and resumed when using
> ISA, PCI, or AHCI.
>
> This series includes tests for ISA and PCI/BMDMA, but does not
> yet include tests for AHCI, which require some more qtest work
> to be upstreamed first. Regardless, the AHCI tests have been
> written and can be observed at:
> https://github.com/jnsnow/qemu/commits/ahci-devel-latest
>
> See "ahci: add migrate dma test" and "ahci-test: add flush migrate test"
> for the WIP versions of the AHCI test that I used to exercise this
> patchset.

I had to read this twice to even guess how this is related to the stated
subject "ide: rerror and werror support for IDE and AHCI" :)

IDE already supports rerror and werror.  I guess this series fixes
migration bugs that can bite when rerror/werror=stop.  Correct?

Re: [Qemu-devel] [PATCH v2 00/17] ide: rerror and werror support for IDE and AHCI

[Paolo Bonzini]

On 17/12/2014 09:23, Markus Armbruster wrote:
> I had to read this twice to even guess how this is related to the stated
> subject "ide: rerror and werror support for IDE and AHCI" :)
> 
> IDE already supports rerror and werror.  I guess this series fixes
> migration bugs that can bite when rerror/werror=stop.  Correct?

IDE over PCI supports rerror and werror.  IDE over ISA does not.

This series makes it possible to support it very easily, see patch 13.

Paolo

Re: [Qemu-devel] [PATCH v2 15/17] ahci: add support for restarting non-queued commands

[Paolo Bonzini]

On 17/12/2014 02:36, John Snow wrote:
> +         * If an error was there, ad->busy_slot will not be -1.  Restarting
> +         * the operation will resume execution of the command list, do
> +         * nothing here.

I wrote this comment, and I had to reread it twice to make sense of it.

Perhaps:

    * If an error was there, ad->busy_slot will not be -1.  In that
    * case, restarting the operation will resume execution of the
    * command list.  Otherwise we have to do it here.

Paolo

Re: [Qemu-devel] [PATCH v2 00/17] ide: rerror and werror support for IDE and AHCI

[John Snow]

On 12/17/2014 04:37 AM, Paolo Bonzini wrote:
>
>
> On 17/12/2014 09:23, Markus Armbruster wrote:
>> I had to read this twice to even guess how this is related to the stated
>> subject "ide: rerror and werror support for IDE and AHCI" :)
>>
>> IDE already supports rerror and werror.  I guess this series fixes
>> migration bugs that can bite when rerror/werror=stop.  Correct?
>
> IDE over PCI supports rerror and werror.  IDE over ISA does not.
>
> This series makes it possible to support it very easily, see patch 13.
>
> Paolo
>

Yes, sorry -- rerror=stop/werror=stop already worked for PCI/BMDMA, but 
this series as a whole unifies the resume pathways and irons out some 
migration issues.

Most notably, as Paolo states, this fixes ISA and my later patches fix 
AHCI. AHCI tests are still dependent upon enabling migration, so those 
aren't here yet, but I sat on this series until I convinced myself that 
AHCI was working too, hence the delay.

The practical upshot is that rerror/werror work for all the acronyms 
listed using newly unified code pathways.

--js

Re: [Qemu-devel] [PATCH v2 00/17] ide: rerror and werror support for IDE and AHCI

[John Snow]

Post-holiday bump that this is sitting out there, awaiting love.
If this gets merged, we should be able to enable Q35 migration soon, 
which would be nice.

--js

On 12/16/2014 08:35 PM, John Snow wrote:
> This series was written mostly by Paolo Bonzini to do two things:
>
> 1. Unify the restart callbacks for ISA, AHCI and BMDMA
> 2. Ensure we can restart a command after migration
>
> Many of the early patches only make much sense considering the
> end-goal of eliminating BMDMA specific restart code to be shared
> with ISA and AHCI codepaths.
>
> Migration for halted commands is fixed for ISA, PCI and AHCI.
> As a consequence, operations halted via rerror=stop or werror=stop
> should be able to be successfully migrated and resumed when using
> ISA, PCI, or AHCI.
>
> This series includes tests for ISA and PCI/BMDMA, but does not
> yet include tests for AHCI, which require some more qtest work
> to be upstreamed first. Regardless, the AHCI tests have been
> written and can be observed at:
> https://github.com/jnsnow/qemu/commits/ahci-devel-latest
>
> See "ahci: add migrate dma test" and "ahci-test: add flush migrate test"
> for the WIP versions of the AHCI test that I used to exercise this
> patchset.
>
> John Snow (3):
>    ahci: Migrate IDEStatus
>    ahci: Recompute cur_cmd on migrate post load
>    qtest/ide: Test flush / retry for ISA and PCI
>
> Paolo Bonzini (14):
>    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
>    ide: make more functions static
>    ide: support PIO restart for the ISA controller
>    ahci: add support for restarting non-queued commands
>
>   hw/ide/ahci.c     |  37 +++++++++---------
>   hw/ide/atapi.c    |   3 +-
>   hw/ide/cmd646.c   |   3 +-
>   hw/ide/core.c     | 109 +++++++++++++++++++++++++++++++++++++++++++++++-------
>   hw/ide/internal.h |  16 +++++---
>   hw/ide/isa.c      |   3 +-
>   hw/ide/macio.c    |   6 ---
>   hw/ide/pci.c      |  98 ++++++++----------------------------------------
>   hw/ide/pci.h      |  12 +++---
>   hw/ide/piix.c     |   3 +-
>   hw/ide/via.c      |   3 +-
>   tests/ide-test.c  |  20 +++++++---
>   12 files changed, 165 insertions(+), 148 deletions(-)
>

-- 
—js

Re: [Qemu-devel] [PATCH v2 14/17] ahci: Migrate IDEStatus

[Paolo Bonzini]

On 17/12/2014 02:36, John Snow wrote:
> Amazingly, we weren't doing this before.
> 
> Make sure we migrate the IDEState structure that belongs to
> the AHCIDevice.IDEBus structure during migrations.
> 
> No version numbering changes because AHCI is not officially
> migratable (and we can all see with good reason why) so we
> do not impact any official builds by altering the stream and
> leaving it at version 1.
> 
> This fixes the rerror=stop/werror=stop test case where we wish
> to migrate a halted job. Previously, the error code would not
> migrate, so even if the job completed successfully, AHCI would
> report an error because it would still have the placeholder
> error code from initialization time.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

> ---
>  hw/ide/ahci.c     | 1 +
>  hw/ide/internal.h | 3 +++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index bc6d5ce..3f4fc77 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1321,6 +1321,7 @@ static const VMStateDescription vmstate_ahci_device = {
>      .version_id = 1,
>      .fields = (VMStateField[]) {
>          VMSTATE_IDE_BUS(port, AHCIDevice),
> +        VMSTATE_IDE_DRIVE(port.ifs[0], AHCIDevice),
>          VMSTATE_UINT32(port_state, AHCIDevice),
>          VMSTATE_UINT32(finished, AHCIDevice),
>          VMSTATE_UINT32(port_regs.lst_addr, AHCIDevice),
> diff --git a/hw/ide/internal.h b/hw/ide/internal.h
> index 0beba43..c278dea 100644
> --- a/hw/ide/internal.h
> +++ b/hw/ide/internal.h
> @@ -524,6 +524,9 @@ extern const VMStateDescription vmstate_ide_drive;
>  #define VMSTATE_IDE_DRIVES(_field, _state) \
>      VMSTATE_STRUCT_ARRAY(_field, _state, 2, 3, vmstate_ide_drive, IDEState)
>  
> +#define VMSTATE_IDE_DRIVE(_field, _state) \
> +    VMSTATE_STRUCT(_field, _state, 1, vmstate_ide_drive, IDEState)
> +
>  void ide_bus_reset(IDEBus *bus);
>  int64_t ide_get_sector(IDEState *s);
>  void ide_set_sector(IDEState *s, int64_t sector_num);
> 

Re: [Qemu-devel] [PATCH v2 16/17] ahci: Recompute cur_cmd on migrate post load

[Paolo Bonzini]

On 17/12/2014 02:36, John Snow wrote:
> When the AHCI HBA device is migrated, all of the information that
> led to the request being created is stored in the AHCIDevice
> structures, except for pointers into guest data where return
> information needs to be stored.
> 
> The "cur_cmd" field is usually responsible for this.
> 
> To rebuild the cur_cmd pointer post-migration, we can utilize
> the busy_slot index to figure out where the command header
> we are still processing is.
> 
> This allows a machine in a halted state from rerror=stop or
> werror=stop to be migrated and resume operations without issue.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>


> ---
>  hw/ide/ahci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index c153228..8078d3e 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1373,6 +1373,10 @@ static int ahci_state_post_load(void *opaque, int version_id)
>           */
>          if (ad->busy_slot == -1) {
>              check_cmd(s, i);
> +        } else {
> +            /* We are in the middle of a command, and may need to access
> +             * the command header in guest memory again. */
> +            ad->cur_cmd = &((AHCICmdHdr *)ad->lst)[ad->busy_slot];
>          }
>      }
>  
> 

Re: [Qemu-devel] [PATCH v2 17/17] qtest/ide: Test flush / retry for ISA and PCI

[Paolo Bonzini]

On 17/12/2014 02:36, John Snow wrote:
> This patch adds tests for werror and rerror functionality
> for the PCI and ISA ide buses.
> 
> Tests for the AHCI device are to be included at a later
> date after requisite patches have been merged upstream
> to support needed functionality by the tests.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  tests/ide-test.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/ide-test.c b/tests/ide-test.c
> index 29f4039..b28a302 100644
> --- a/tests/ide-test.c
> +++ b/tests/ide-test.c
> @@ -118,7 +118,6 @@ static void ide_test_start(const char *cmdline_fmt, ...)
>      va_end(ap);
>  
>      qtest_start(cmdline);
> -    qtest_irq_intercept_in(global_qtest, "ioapic");
>      guest_malloc = pc_alloc_init();
>  
>      g_free(cmdline);
> @@ -388,6 +387,7 @@ static void test_bmdma_setup(void)
>          "-drive file=%s,if=ide,serial=%s,cache=writeback,format=raw "
>          "-global ide-hd.ver=%s",
>          tmp_path, "testdisk", "version");
> +    qtest_irq_intercept_in(global_qtest, "ioapic");
>  }
>  
>  static void test_bmdma_teardown(void)
> @@ -516,7 +516,7 @@ static void prepare_blkdebug_script(const char *debug_fn, const char *event)
>      g_assert(ret == 0);
>  }
>  
> -static void test_retry_flush(void)
> +static void test_retry_flush(const char *machine)
>  {
>      uint8_t data;
>      const char *s;
> @@ -580,6 +580,16 @@ static void test_flush_nodev(void)
>      ide_test_quit();
>  }
>  
> +static void test_pci_retry_flush(const char *machine)
> +{
> +    test_retry_flush("pc");
> +}
> +
> +static void test_isa_retry_flush(const char *machine)
> +{
> +    test_retry_flush("isapc");
> +}
> +
>  int main(int argc, char **argv)
>  {
>      const char *arch = qtest_get_arch();
> @@ -617,9 +627,9 @@ int main(int argc, char **argv)
>      qtest_add_func("/ide/bmdma/teardown", test_bmdma_teardown);
>  
>      qtest_add_func("/ide/flush", test_flush);
> -    qtest_add_func("/ide/flush_nodev", test_flush_nodev);
> -
> -    qtest_add_func("/ide/retry/flush", test_retry_flush);
> +    qtest_add_func("/ide/flush/nodev", test_flush_nodev);
> +    qtest_add_func("/ide/flush/retry_pci", test_pci_retry_flush);
> +    qtest_add_func("/ide/flush/retry_isa", test_isa_retry_flush);
>  
>      ret = g_test_run();
>  
> 

Quite different from my original version, so:

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v2 00/17] ide: rerror and werror support for IDE and AHCI

[Paolo Bonzini]

On 30/01/2015 01:44, John Snow wrote:
> Post-holiday bump that this is sitting out there, awaiting love.
> If this gets merged, we should be able to enable Q35 migration soon,
> which would be nice.

Not sure how valuable my opinion is as the author of around 85% of the
series...  So I reviewed the rest.

Paolo

Re: [Qemu-devel] [PATCH v2 00/17] ide: rerror and werror support for IDE and AHCI

[John Snow]

On 01/30/2015 04:38 AM, Paolo Bonzini wrote:
>
>
> On 30/01/2015 01:44, John Snow wrote:
>> Post-holiday bump that this is sitting out there, awaiting love.
>> If this gets merged, we should be able to enable Q35 migration soon,
>> which would be nice.
>
> Not sure how valuable my opinion is as the author of around 85% of the
> series...  So I reviewed the rest.
>
> Paolo
>

Understood, thank you!

Re: [Qemu-devel] [PATCH v2 16/17] ahci: Recompute cur_cmd on migrate post load

[Stefan Hajnoczi]

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

On Tue, Dec 16, 2014 at 08:36:06PM -0500, John Snow wrote:
> When the AHCI HBA device is migrated, all of the information that
> led to the request being created is stored in the AHCIDevice
> structures, except for pointers into guest data where return
> information needs to be stored.
> 
> The "cur_cmd" field is usually responsible for this.
> 
> To rebuild the cur_cmd pointer post-migration, we can utilize
> the busy_slot index to figure out where the command header
> we are still processing is.
> 
> This allows a machine in a halted state from rerror=stop or
> werror=stop to be migrated and resume operations without issue.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  hw/ide/ahci.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index c153228..8078d3e 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1373,6 +1373,10 @@ static int ahci_state_post_load(void *opaque, int version_id)
>           */
>          if (ad->busy_slot == -1) {
>              check_cmd(s, i);
> +        } else {
> +            /* We are in the middle of a command, and may need to access
> +             * the command header in guest memory again. */
> +            ad->cur_cmd = &((AHCICmdHdr *)ad->lst)[ad->busy_slot];

Where do we check that ad->busy_slot is within ad->lst[] bounds?

If a malicious source sends a bogus value, this patch will lead to
out-of-bounds accesses.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 00/17] ide: rerror and werror support for IDE and AHCI

[Stefan Hajnoczi]

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

On Tue, Dec 16, 2014 at 08:35:50PM -0500, John Snow wrote:
> This series was written mostly by Paolo Bonzini to do two things:
> 
> 1. Unify the restart callbacks for ISA, AHCI and BMDMA
> 2. Ensure we can restart a command after migration
> 
> Many of the early patches only make much sense considering the
> end-goal of eliminating BMDMA specific restart code to be shared
> with ISA and AHCI codepaths.
> 
> Migration for halted commands is fixed for ISA, PCI and AHCI.
> As a consequence, operations halted via rerror=stop or werror=stop
> should be able to be successfully migrated and resumed when using
> ISA, PCI, or AHCI.
> 
> This series includes tests for ISA and PCI/BMDMA, but does not
> yet include tests for AHCI, which require some more qtest work
> to be upstreamed first. Regardless, the AHCI tests have been
> written and can be observed at:
> https://github.com/jnsnow/qemu/commits/ahci-devel-latest
> 
> See "ahci: add migrate dma test" and "ahci-test: add flush migrate test"
> for the WIP versions of the AHCI test that I used to exercise this
> patchset.
> 
> John Snow (3):
>   ahci: Migrate IDEStatus
>   ahci: Recompute cur_cmd on migrate post load
>   qtest/ide: Test flush / retry for ISA and PCI
> 
> Paolo Bonzini (14):
>   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
>   ide: make more functions static
>   ide: support PIO restart for the ISA controller
>   ahci: add support for restarting non-queued commands
> 
>  hw/ide/ahci.c     |  37 +++++++++---------
>  hw/ide/atapi.c    |   3 +-
>  hw/ide/cmd646.c   |   3 +-
>  hw/ide/core.c     | 109 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  hw/ide/internal.h |  16 +++++---
>  hw/ide/isa.c      |   3 +-
>  hw/ide/macio.c    |   6 ---
>  hw/ide/pci.c      |  98 ++++++++----------------------------------------
>  hw/ide/pci.h      |  12 +++---
>  hw/ide/piix.c     |   3 +-
>  hw/ide/via.c      |   3 +-
>  tests/ide-test.c  |  20 +++++++---
>  12 files changed, 165 insertions(+), 148 deletions(-)

Looks good but please double-check input validation for migrated fields.
I pointed out a patch that seems to lack input validation.

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v2 16/17] ahci: Recompute cur_cmd on migrate post load

[John Snow]

On 02/10/2015 04:56 AM, Stefan Hajnoczi wrote:
> On Tue, Dec 16, 2014 at 08:36:06PM -0500, John Snow wrote:
>> When the AHCI HBA device is migrated, all of the information that
>> led to the request being created is stored in the AHCIDevice
>> structures, except for pointers into guest data where return
>> information needs to be stored.
>>
>> The "cur_cmd" field is usually responsible for this.
>>
>> To rebuild the cur_cmd pointer post-migration, we can utilize
>> the busy_slot index to figure out where the command header
>> we are still processing is.
>>
>> This allows a machine in a halted state from rerror=stop or
>> werror=stop to be migrated and resume operations without issue.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   hw/ide/ahci.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
>> index c153228..8078d3e 100644
>> --- a/hw/ide/ahci.c
>> +++ b/hw/ide/ahci.c
>> @@ -1373,6 +1373,10 @@ static int ahci_state_post_load(void *opaque, int version_id)
>>            */
>>           if (ad->busy_slot == -1) {
>>               check_cmd(s, i);
>> +        } else {
>> +            /* We are in the middle of a command, and may need to access
>> +             * the command header in guest memory again. */
>> +            ad->cur_cmd = &((AHCICmdHdr *)ad->lst)[ad->busy_slot];
>
> Where do we check that ad->busy_slot is within ad->lst[] bounds?
>
> If a malicious source sends a bogus value, this patch will lead to
> out-of-bounds accesses.
>
> Stefan
>

Good point. I'll recheck the series with this in mind.