[Qemu-devel] [PATCH v4 0/3] AHCI migration

[Qemu-devel] [PATCH v4 0/3] AHCI migration

[Kevin Wolf]

Let's get Jason's patches merged while they still apply. I addressed the review
comments (mostly my own) that came up during the v3 review, otherwise this is
unchanged.

Please note that in my tests it didn't work entirely reliably and I saw guest
lockups and kernel crashes in like one of ten cases. I confirmed that the same
kind of bugs occurs with v3 of the series, so my changes are likely innocent.
Someone will have to debug this some more, but what I did took about the time
that I'm willing to spend on it right now.

Jason Baron (2):
  ahci: Remove unused AHCIDevice fields
  ahci: Add migration support

Kevin Wolf (1):
  ahci: Change data types in preparation for migration

 hw/ide/ahci.c |   97 ++++++++++++++++++++++++++++++++++++++++++++++++++------
 hw/ide/ahci.h |   20 ++++++++---
 hw/ide/ich.c  |   13 +++++--
 3 files changed, 109 insertions(+), 21 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH v4 1/3] ahci: Remove unused AHCIDevice fields

[Kevin Wolf]

From: Jason Baron <jbaron@redhat.com>

'dma_status' and 'dma_cb' are written to, but never read.
Remove these fields in preparation for AHCI migration bits.

Signed-off-by: Jason Baron <jbaron@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/ahci.c |    8 ++------
 hw/ide/ahci.h |    2 --
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 21f50ea..2d185cb 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1035,11 +1035,10 @@ out:
 static void ahci_start_dma(IDEDMA *dma, IDEState *s,
                            BlockDriverCompletionFunc *dma_cb)
 {
+#ifdef DEBUG_AHCI
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
-
+#endif
     DPRINTF(ad->port_no, "\n");
-    ad->dma_cb = dma_cb;
-    ad->dma_status |= BM_STATUS_DMAING;
     s->io_buffer_offset = 0;
     dma_cb(s, 0);
 }
@@ -1095,7 +1094,6 @@ static int ahci_dma_set_unit(IDEDMA *dma, int unit)
 static int ahci_dma_add_status(IDEDMA *dma, int status)
 {
     AHCIDevice *ad = DO_UPCAST(AHCIDevice, dma, dma);
-    ad->dma_status |= status;
     DPRINTF(ad->port_no, "set status: %x\n", status);
 
     if (status & BM_STATUS_INT) {
@@ -1114,8 +1112,6 @@ static int ahci_dma_set_inactive(IDEDMA *dma)
     /* update d2h status */
     ahci_write_fis_d2h(ad, NULL);
 
-    ad->dma_cb = NULL;
-
     if (!ad->check_bh) {
         /* maybe we still have something to process, check later */
         ad->check_bh = qemu_bh_new(ahci_check_cmd_bh, ad);
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 1200a56..735b379 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -281,11 +281,9 @@ struct AHCIDevice {
     QEMUBH *check_bh;
     uint8_t *lst;
     uint8_t *res_fis;
-    int dma_status;
     int done_atapi_packet;
     int busy_slot;
     int init_d2h_sent;
-    BlockDriverCompletionFunc *dma_cb;
     AHCICmdHdr *cur_cmd;
     NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
 };
-- 
1.7.6.5

[Qemu-devel] [PATCH v4 2/3] ahci: Change data types in preparation for migration

[Kevin Wolf]

The size of an int depends on the host, so in order to be able to
migrate these fields, make them either int32_t or bool, depending on the
use.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/ahci.c |    8 ++++----
 hw/ide/ahci.h |    8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index 2d185cb..f91cff2 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -241,7 +241,7 @@ static void  ahci_port_write(AHCIState *s, int port, int offset, uint32_t val)
             if ((pr->cmd & PORT_CMD_FIS_ON) &&
                 !s->dev[port].init_d2h_sent) {
                 ahci_init_d2h(&s->dev[port]);
-                s->dev[port].init_d2h_sent = 1;
+                s->dev[port].init_d2h_sent = true;
             }
 
             check_cmd(s, port);
@@ -494,7 +494,7 @@ static void ahci_reset_port(AHCIState *s, int port)
     pr->scr_err = 0;
     pr->scr_act = 0;
     d->busy_slot = -1;
-    d->init_d2h_sent = 0;
+    d->init_d2h_sent = false;
 
     ide_state = &s->dev[port].port.ifs[0];
     if (!ide_state->bs) {
@@ -946,7 +946,7 @@ static int handle_cmd(AHCIState *s, int port, int slot)
             ide_state->hcyl = 0xeb;
             debug_print_fis(ide_state->io_buffer, 0x10);
             ide_state->feature = IDE_FEATURE_DMA;
-            s->dev[port].done_atapi_packet = 0;
+            s->dev[port].done_atapi_packet = false;
             /* XXX send PIO setup FIS */
         }
 
@@ -991,7 +991,7 @@ static int ahci_start_transfer(IDEDMA *dma)
 
     if (is_atapi && !ad->done_atapi_packet) {
         /* already prepopulated iobuffer */
-        ad->done_atapi_packet = 1;
+        ad->done_atapi_packet = true;
         goto out;
     }
 
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 735b379..70d3b57 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -281,9 +281,9 @@ struct AHCIDevice {
     QEMUBH *check_bh;
     uint8_t *lst;
     uint8_t *res_fis;
-    int done_atapi_packet;
-    int busy_slot;
-    int init_d2h_sent;
+    bool done_atapi_packet;
+    int32_t busy_slot;
+    bool init_d2h_sent;
     AHCICmdHdr *cur_cmd;
     NCQTransferState ncq_tfs[AHCI_MAX_CMDS];
 };
@@ -295,7 +295,7 @@ typedef struct AHCIState {
     MemoryRegion idp;       /* Index-Data Pair I/O port space */
     unsigned idp_offset;    /* Offset of index in I/O port space */
     uint32_t idp_index;     /* Current IDP index */
-    int ports;
+    int32_t ports;
     qemu_irq irq;
     DMAContext *dma;
 } AHCIState;
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH v4 2/3] ahci: Change data types in preparation for migration

[Juan Quintela]

Kevin Wolf <kwolf@redhat.com> wrote:
> The size of an int depends on the host, so in order to be able to
> migrate these fields, make them either int32_t or bool, depending on the
> use.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com> 

[Qemu-devel] [PATCH v4 3/3] ahci: Add migration support

[Kevin Wolf]

From: Jason Baron <jbaron@redhat.com>

Jason tested these patches by migrating Windows 7 and Fedora 17 guests
(while under I/O) on both piix with ahci attached and on q35 (which has
a built-in AHCI controller).

Signed-off-by: Andreas Färber <afaerber@suse.de>
Signed-off-by: Jason Baron <jbaron@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
Changes from v3:
- Update the types of some fields (VMSTATE_INT32 -> VMSTATE_BOOL)
- post_load: Check that BUSY_STAT and DRQ_STAT aren't set before
  clearing busy_port
- Change vmstate_ich9_ahci.name from ahci to ich9_ahci

Changes from v2:
 -migrate all relevant ahci fields
 -flush any pending i/o in 'post_load'

Changes from v1:
 -extend Andreas Färber's patch
---
 hw/ide/ahci.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 hw/ide/ahci.h |   10 +++++++
 hw/ide/ich.c  |   13 ++++++---
 3 files changed, 99 insertions(+), 5 deletions(-)

diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index f91cff2..a645e22 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -1199,6 +1199,82 @@ void ahci_reset(AHCIState *s)
     }
 }
 
+static const VMStateDescription vmstate_ahci_device = {
+    .name = "ahci port",
+    .version_id = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_IDE_BUS(port, AHCIDevice),
+        VMSTATE_UINT32(port_state, AHCIDevice),
+        VMSTATE_UINT32(finished, AHCIDevice),
+        VMSTATE_UINT32(port_regs.lst_addr, AHCIDevice),
+        VMSTATE_UINT32(port_regs.lst_addr_hi, AHCIDevice),
+        VMSTATE_UINT32(port_regs.fis_addr, AHCIDevice),
+        VMSTATE_UINT32(port_regs.fis_addr_hi, AHCIDevice),
+        VMSTATE_UINT32(port_regs.irq_stat, AHCIDevice),
+        VMSTATE_UINT32(port_regs.irq_mask, AHCIDevice),
+        VMSTATE_UINT32(port_regs.cmd, AHCIDevice),
+        VMSTATE_UINT32(port_regs.tfdata, AHCIDevice),
+        VMSTATE_UINT32(port_regs.sig, AHCIDevice),
+        VMSTATE_UINT32(port_regs.scr_stat, AHCIDevice),
+        VMSTATE_UINT32(port_regs.scr_ctl, AHCIDevice),
+        VMSTATE_UINT32(port_regs.scr_err, AHCIDevice),
+        VMSTATE_UINT32(port_regs.scr_act, AHCIDevice),
+        VMSTATE_UINT32(port_regs.cmd_issue, AHCIDevice),
+        VMSTATE_BOOL(done_atapi_packet, AHCIDevice),
+        VMSTATE_INT32(busy_slot, AHCIDevice),
+        VMSTATE_BOOL(init_d2h_sent, AHCIDevice),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static int ahci_state_post_load(void *opaque, int version_id)
+{
+    int i;
+    struct AHCIDevice *ad;
+    AHCIState *s = opaque;
+
+    for (i = 0; i < s->ports; i++) {
+        ad = &s->dev[i];
+        AHCIPortRegs *pr = &ad->port_regs;
+
+        map_page(&ad->lst,
+                 ((uint64_t)pr->lst_addr_hi << 32) | pr->lst_addr, 1024);
+        map_page(&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 ((ad->busy_slot != -1) &&
+            !(ad->port.ifs[0].status & (BUSY_STAT|DRQ_STAT))) {
+            pr->cmd_issue &= ~(1 << ad->busy_slot);
+            ad->busy_slot = -1;
+        }
+        check_cmd(s, i);
+    }
+
+    return 0;
+}
+
+const VMStateDescription vmstate_ahci = {
+    .name = "ahci",
+    .version_id = 1,
+    .post_load = ahci_state_post_load,
+    .fields = (VMStateField []) {
+        VMSTATE_STRUCT_VARRAY_POINTER_INT32(dev, AHCIState, ports,
+                                     vmstate_ahci_device, AHCIDevice),
+        VMSTATE_UINT32(control_regs.cap, AHCIState),
+        VMSTATE_UINT32(control_regs.ghc, AHCIState),
+        VMSTATE_UINT32(control_regs.irqstatus, AHCIState),
+        VMSTATE_UINT32(control_regs.impl, AHCIState),
+        VMSTATE_UINT32(control_regs.version, AHCIState),
+        VMSTATE_UINT32(idp_index, AHCIState),
+        VMSTATE_INT32(ports, AHCIState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
 typedef struct SysbusAHCIState {
     SysBusDevice busdev;
     AHCIState ahci;
@@ -1207,7 +1283,10 @@ typedef struct SysbusAHCIState {
 
 static const VMStateDescription vmstate_sysbus_ahci = {
     .name = "sysbus-ahci",
-    .unmigratable = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_AHCI(ahci, AHCIPCIState),
+        VMSTATE_END_OF_LIST()
+    },
 };
 
 static void sysbus_ahci_reset(DeviceState *dev)
diff --git a/hw/ide/ahci.h b/hw/ide/ahci.h
index 70d3b57..85f37fe 100644
--- a/hw/ide/ahci.h
+++ b/hw/ide/ahci.h
@@ -305,6 +305,16 @@ typedef struct AHCIPCIState {
     AHCIState ahci;
 } AHCIPCIState;
 
+extern const VMStateDescription vmstate_ahci;
+
+#define VMSTATE_AHCI(_field, _state) {                               \
+    .name       = (stringify(_field)),                               \
+    .size       = sizeof(AHCIState),                                 \
+    .vmsd       = &vmstate_ahci,                                     \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = vmstate_offset_value(_state, _field, AHCIState),   \
+}
+
 typedef struct NCQFrame {
     uint8_t fis_type;
     uint8_t c;
diff --git a/hw/ide/ich.c b/hw/ide/ich.c
index 1fb803d..0897bb7 100644
--- a/hw/ide/ich.c
+++ b/hw/ide/ich.c
@@ -79,9 +79,14 @@
 #define ICH9_IDP_INDEX          0x10
 #define ICH9_IDP_INDEX_LOG2     0x04
 
-static const VMStateDescription vmstate_ahci = {
-    .name = "ahci",
-    .unmigratable = 1,
+static const VMStateDescription vmstate_ich9_ahci = {
+    .name = "ich9_ahci",
+    .version_id = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_PCI_DEVICE(card, AHCIPCIState),
+        VMSTATE_AHCI(ahci, AHCIPCIState),
+        VMSTATE_END_OF_LIST()
+    },
 };
 
 static void pci_ich9_reset(DeviceState *dev)
@@ -152,7 +157,7 @@ static void ich_ahci_class_init(ObjectClass *klass, void *data)
     k->device_id = PCI_DEVICE_ID_INTEL_82801IR;
     k->revision = 0x02;
     k->class_id = PCI_CLASS_STORAGE_SATA;
-    dc->vmsd = &vmstate_ahci;
+    dc->vmsd = &vmstate_ich9_ahci;
     dc->reset = pci_ich9_reset;
 }
 
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH v4 3/3] ahci: Add migration support

[Juan Quintela]

Kevin Wolf <kwolf@redhat.com> wrote:
> From: Jason Baron <jbaron@redhat.com>
>
> Jason tested these patches by migrating Windows 7 and Fedora 17 guests
> (while under I/O) on both piix with ahci attached and on q35 (which has
> a built-in AHCI controller).
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Signed-off-by: Jason Baron <jbaron@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

The vmstate bits are correct.  If we are still seing crashes, that is
because we haven't got all the state described.  I fully agree with
Stefan suggestion of merging the code but maintaining it no-migratable.

Later, Juan.

Re: [Qemu-devel] [PATCH v4 0/3] AHCI migration

[Stefan Hajnoczi]

On Thu, Jan 17, 2013 at 11:01:52AM +0100, Kevin Wolf wrote:
> Let's get Jason's patches merged while they still apply. I addressed the review
> comments (mostly my own) that came up during the v3 review, otherwise this is
> unchanged.
> 
> Please note that in my tests it didn't work entirely reliably and I saw guest
> lockups and kernel crashes in like one of ten cases. I confirmed that the same
> kind of bugs occurs with v3 of the series, so my changes are likely innocent.
> Someone will have to debug this some more, but what I did took about the time
> that I'm willing to spend on it right now.

It makes sense to merge these patches to avoid bitrot, but should we
keep unmigratable = 1 so that users aren't led to believe migration
works?

Developers willing to tackle the remaining problem can always comment
out the unmigratable flag during testing.  But I think users shouldn't
be exposed to something unreliable.

Stefan

Re: [Qemu-devel] [PATCH v4 0/3] AHCI migration

[Juan Quintela]

Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Thu, Jan 17, 2013 at 11:01:52AM +0100, Kevin Wolf wrote:
>> Let's get Jason's patches merged while they still apply. I addressed the review
>> comments (mostly my own) that came up during the v3 review, otherwise this is
>> unchanged.
>> 
>> Please note that in my tests it didn't work entirely reliably and I saw guest
>> lockups and kernel crashes in like one of ten cases. I confirmed that the same
>> kind of bugs occurs with v3 of the series, so my changes are likely innocent.
>> Someone will have to debug this some more, but what I did took about the time
>> that I'm willing to spend on it right now.
>
> It makes sense to merge these patches to avoid bitrot, but should we
> keep unmigratable = 1 so that users aren't led to believe migration
> works?
>
> Developers willing to tackle the remaining problem can always comment
> out the unmigratable flag during testing.  But I think users shouldn't
> be exposed to something unreliable.

Agreed with that.

Later, Juan.