[Qemu-devel] [PATCH 0/3] IDE and live migration fun

[Qemu-devel] [PATCH 0/3] IDE and live migration fun

[Kevin Wolf]

Patch 1 describes the real problem that I was going to solve. While fixing
this, I stumbled across some more migration problems that are included in this
series. Some refactoring is still left to do after this, but I'd prefer to
keep this series with the hard stuff short.

This is the test case that I was using:

$ cat blkdebug.conf
[inject-error]
state = "2"
event = "read_aio"
errno = "7"
immediately = "off"
once = "on"

[set-state]
state = "1"
event = "read_aio"
new_state = "2"

[set-state]
state = "2"
event = "read_aio"
new_state = "3"

$ x86_64-softmmu/qemu-system-x86_64 -drive file=blkdebug:blkdebug.conf:disk.qcow2,rerror=stop,werror=stop -incoming tcp::1028
$ x86_64-softmmu/qemu-system-x86_64 -drive file=blkdebug:blkdebug.conf:disk.qcow2,rerror=stop,werror=stop
(qemu) migrate tcp::1028

Please note that you need to use a qcow2 image, or the blkdebug events won't be
triggered. Expected result: The very first read from the disk (BIOS reading
the MBR) fails and stops the VM. I migrate while the VM is stopped, so that the
request is queued for resubmission. After continuing, the VM should work just
as normal.

Kevin Wolf (3):
  ide: Split error status from status register
  ide: Fix ide_drive_pio_state_needed()
  ide: Add forgotten VMSTATE_END_OF_LIST in subsection

 hw/ide/core.c     |   32 +++++++++++++++++++++-
 hw/ide/internal.h |    8 ++++++
 hw/ide/pci.c      |   73 +++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/ide/pci.h      |    4 +++
 4 files changed, 108 insertions(+), 9 deletions(-)

-- 
1.7.5.2

[Qemu-devel] [PATCH 1/3] ide: Split error status from status register

[Kevin Wolf]

When adding the werror=stop mode, some flags were added to s->status
which are used to determine what kind of operation should be restarted
when the VM is continued.

Unfortunately, it turns out that s->status is in fact a device register
and as such is visible to the guest (some of the abused bits are even
writable for the guest).

For migration we keep on using the old VMState field (renamed to
migration_compat_status) if the status register doesn't use any of the
previously abused bits. If it does, we use a subsection with a clean copy of
the status register.

The error status is always sent in a subsection if there is any error. It can't
use the old field because errors happen even without PCI.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/core.c     |   28 +++++++++++++++++++-
 hw/ide/internal.h |    8 ++++++
 hw/ide/pci.c      |   73 +++++++++++++++++++++++++++++++++++++++++++++++-----
 hw/ide/pci.h      |    4 +++
 4 files changed, 105 insertions(+), 8 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 95beb17..da250ac 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -446,7 +446,7 @@ static int ide_handle_rw_error(IDEState *s, int error, int op)
     if ((error == ENOSPC && action == BLOCK_ERR_STOP_ENOSPC)
             || action == BLOCK_ERR_STOP_ANY) {
         s->bus->dma->ops->set_unit(s->bus->dma, s->unit);
-        s->bus->dma->ops->add_status(s->bus->dma, op);
+        s->bus->error_status = op;
         bdrv_mon_event(s->bs, BDRV_ACTION_STOP, is_read);
         vm_stop(VMSTOP_DISKFULL);
     } else {
@@ -1847,6 +1847,13 @@ static bool ide_atapi_gesn_needed(void *opaque)
     return s->events.new_media || s->events.eject_request;
 }
 
+static bool ide_error_needed(void *opaque)
+{
+    IDEBus *bus = opaque;
+
+    return (bus->error_status != 0);
+}
+
 /* Fields for GET_EVENT_STATUS_NOTIFICATION ATAPI command */
 const VMStateDescription vmstate_ide_atapi_gesn_state = {
     .name ="ide_drive/atapi/gesn_state",
@@ -1921,6 +1928,17 @@ const VMStateDescription vmstate_ide_drive = {
     }
 };
 
+const VMStateDescription vmstate_ide_error_status = {
+    .name ="ide_bus/error",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_INT32(error_status, IDEBus),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_ide_bus = {
     .name = "ide_bus",
     .version_id = 1,
@@ -1930,6 +1948,14 @@ const VMStateDescription vmstate_ide_bus = {
         VMSTATE_UINT8(cmd, IDEBus),
         VMSTATE_UINT8(unit, IDEBus),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_ide_error_status,
+            .needed = ide_error_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index c2b35ec..8d18cc3 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -486,6 +486,8 @@ struct IDEBus {
     uint8_t unit;
     uint8_t cmd;
     qemu_irq irq;
+
+    int error_status;
 };
 
 struct IDEDevice {
@@ -505,11 +507,17 @@ struct IDEDeviceInfo {
 #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
 #define BM_STATUS_RETRY_READ  0x20
 #define BM_STATUS_RETRY_FLUSH 0x40
 
+#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
 
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index a4726ad..7fa32bd 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -183,27 +183,33 @@ static void bmdma_restart_dma(BMDMAState *bm, int is_read)
     bmdma_start_dma(&bm->dma, s, bm->dma_cb);
 }
 
+/* TODO This should be common IDE code */
 static void bmdma_restart_bh(void *opaque)
 {
     BMDMAState *bm = opaque;
+    IDEBus *bus = bm->bus;
     int is_read;
 
     qemu_bh_delete(bm->bh);
     bm->bh = NULL;
 
-    is_read = !!(bm->status & BM_STATUS_RETRY_READ);
+    if (bm->unit == (uint8_t) -1) {
+        return;
+    }
 
-    if (bm->status & BM_STATUS_DMA_RETRY) {
-        bm->status &= ~(BM_STATUS_DMA_RETRY | BM_STATUS_RETRY_READ);
+    is_read = !!(bus->error_status & BM_STATUS_RETRY_READ);
+
+    if (bus->error_status & BM_STATUS_DMA_RETRY) {
+        bus->error_status &= ~(BM_STATUS_DMA_RETRY | BM_STATUS_RETRY_READ);
         bmdma_restart_dma(bm, is_read);
-    } else if (bm->status & BM_STATUS_PIO_RETRY) {
-        bm->status &= ~(BM_STATUS_PIO_RETRY | BM_STATUS_RETRY_READ);
+    } else if (bus->error_status & BM_STATUS_PIO_RETRY) {
+        bus->error_status &= ~(BM_STATUS_PIO_RETRY | BM_STATUS_RETRY_READ);
         if (is_read) {
             ide_sector_read(bmdma_active_if(bm));
         } else {
             ide_sector_write(bmdma_active_if(bm));
         }
-    } else if (bm->status & BM_STATUS_RETRY_FLUSH) {
+    } else if (bus->error_status & BM_STATUS_RETRY_FLUSH) {
         ide_flush_cache(bmdma_active_if(bm));
     }
 }
@@ -351,6 +357,43 @@ static bool ide_bmdma_current_needed(void *opaque)
     return (bm->cur_prd_len != 0);
 }
 
+static bool ide_bmdma_status_needed(void *opaque)
+{
+    BMDMAState *bm = opaque;
+
+    /* Older versions abused some bits in the status register for internal
+     * error state. If any of these bits are set, we must add a subsection to
+     * transfer the real status register */
+    uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
+
+    return ((bm->status & abused_bits) != 0);
+}
+
+static void ide_bmdma_pre_save(void *opaque)
+{
+    BMDMAState *bm = opaque;
+    uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
+
+    bm->migration_compat_status =
+        (bm->status & ~abused_bits) | (bm->bus->error_status & abused_bits);
+}
+
+/* This function accesses bm->bus->error_status which is loaded only after
+ * BMDMA itself. This is why the function is called from ide_pci_post_load
+ * instead of being registered with VMState where it would run too early. */
+static int ide_bmdma_post_load(void *opaque, int version_id)
+{
+    BMDMAState *bm = opaque;
+    uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
+
+    if (bm->status == 0) {
+        bm->status = bm->migration_compat_status & ~abused_bits;
+        bm->bus->error_status |= bm->migration_compat_status & abused_bits;
+    }
+
+    return 0;
+}
+
 static const VMStateDescription vmstate_bmdma_current = {
     .name = "ide bmdma_current",
     .version_id = 1,
@@ -365,15 +408,26 @@ static const VMStateDescription vmstate_bmdma_current = {
     }
 };
 
+const VMStateDescription vmstate_bmdma_status = {
+    .name ="ide bmdma/status",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_UINT8(status, BMDMAState),
+        VMSTATE_END_OF_LIST()
+    }
+};
 
 static const VMStateDescription vmstate_bmdma = {
     .name = "ide bmdma",
     .version_id = 3,
     .minimum_version_id = 0,
     .minimum_version_id_old = 0,
+    .pre_save  = ide_bmdma_pre_save,
     .fields      = (VMStateField []) {
         VMSTATE_UINT8(cmd, BMDMAState),
-        VMSTATE_UINT8(status, BMDMAState),
+        VMSTATE_UINT8(migration_compat_status, BMDMAState),
         VMSTATE_UINT32(addr, BMDMAState),
         VMSTATE_INT64(sector_num, BMDMAState),
         VMSTATE_UINT32(nsector, BMDMAState),
@@ -385,6 +439,9 @@ static const VMStateDescription vmstate_bmdma = {
             .vmsd = &vmstate_bmdma_current,
             .needed = ide_bmdma_current_needed,
         }, {
+            .vmsd = &vmstate_bmdma_status,
+            .needed = ide_bmdma_status_needed,
+        }, {
             /* empty */
         }
     }
@@ -399,7 +456,9 @@ static int ide_pci_post_load(void *opaque, int version_id)
         /* current versions always store 0/1, but older version
            stored bigger values. We only need last bit */
         d->bmdma[i].unit &= 1;
+        ide_bmdma_post_load(&d->bmdma[i], -1);
     }
+
     return 0;
 }
 
diff --git a/hw/ide/pci.h b/hw/ide/pci.h
index cd72cba..b4f3691 100644
--- a/hw/ide/pci.h
+++ b/hw/ide/pci.h
@@ -22,6 +22,10 @@ typedef struct BMDMAState {
     IORange addr_ioport;
     QEMUBH *bh;
     qemu_irq irq;
+
+    /* Bit 0-2 and 7:   BM status register
+     * Bit 3-6:         bus->error_status */
+    uint8_t migration_compat_status;
 } BMDMAState;
 
 typedef struct PCIIDEState {
-- 
1.7.5.2

[Qemu-devel] [PATCH 2/3] ide: Fix ide_drive_pio_state_needed()

[Kevin Wolf]

When a failed PIO request caused the VM to stop, we still need to transfer the
PIO state even though DRQ=0 at this point.

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

diff --git a/hw/ide/core.c b/hw/ide/core.c
index da250ac..e5def8b 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1837,7 +1837,8 @@ static bool ide_drive_pio_state_needed(void *opaque)
 {
     IDEState *s = opaque;
 
-    return (s->status & DRQ_STAT) != 0;
+    return ((s->status & DRQ_STAT) != 0)
+        || (s->bus->error_status & BM_STATUS_PIO_RETRY);
 }
 
 static bool ide_atapi_gesn_needed(void *opaque)
-- 
1.7.5.2

[Qemu-devel] [PATCH 3/3] ide: Add forgotten VMSTATE_END_OF_LIST in subsection

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/ide/core.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index e5def8b..399b74c 100644
--- a/hw/ide/core.c
+++ b/hw/ide/core.c
@@ -1864,6 +1864,7 @@ const VMStateDescription vmstate_ide_atapi_gesn_state = {
     .fields = (VMStateField []) {
         VMSTATE_BOOL(events.new_media, IDEState),
         VMSTATE_BOOL(events.eject_request, IDEState),
+        VMSTATE_END_OF_LIST()
     }
 };
 
-- 
1.7.5.2

Re: [Qemu-devel] [PATCH 1/3] ide: Split error status from status register

[Paolo Bonzini]

On 06/09/2011 03:15 PM, Kevin Wolf wrote:
> +/* This function accesses bm->bus->error_status which is loaded only after
> + * BMDMA itself. This is why the function is called from ide_pci_post_load
> + * instead of being registered with VMState where it would run too early. */
> +static int ide_bmdma_post_load(void *opaque, int version_id)
> +{
> +    BMDMAState *bm = opaque;
> +    uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
> +
> +    if (bm->status == 0) {
> +        bm->status = bm->migration_compat_status&  ~abused_bits;
> +        bm->bus->error_status |= bm->migration_compat_status&  abused_bits;
> +    }
> +
> +    return 0;
> +}
> +

Why the if?

Otherwise looks good.

Paolo

Re: [Qemu-devel] [PATCH 1/3] ide: Split error status from status register

[Kevin Wolf]

Am 09.06.2011 15:27, schrieb Paolo Bonzini:
> On 06/09/2011 03:15 PM, Kevin Wolf wrote:
>> +/* This function accesses bm->bus->error_status which is loaded only after
>> + * BMDMA itself. This is why the function is called from ide_pci_post_load
>> + * instead of being registered with VMState where it would run too early. */
>> +static int ide_bmdma_post_load(void *opaque, int version_id)
>> +{
>> +    BMDMAState *bm = opaque;
>> +    uint8_t abused_bits = BM_MIGRATION_COMPAT_STATUS_BITS;
>> +
>> +    if (bm->status == 0) {
>> +        bm->status = bm->migration_compat_status&  ~abused_bits;
>> +        bm->bus->error_status |= bm->migration_compat_status&  abused_bits;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
> 
> Why the if?

I think you're right. We could enable it unconditionally (and change the
bm->status line from = to |=), but anyway it's redundant if the
subsections are present, so it wouldn't make a difference.

Kevin

Re: [Qemu-devel] [PATCH 1/3] ide: Split error status from status register

[Paolo Bonzini]

On 06/09/2011 04:07 PM, Kevin Wolf wrote:
> I think you're right. We could enable it unconditionally (and change the
> bm->status line from = to |=), but anyway it's redundant if the
> subsections are present, so it wouldn't make a difference.

Yes, I was thinking the same.

Paolo