[Qemu-devel] [RFC][PATCH] ide: Break migration by splitting 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).

Splitting the internal status and the status register into two different
variables is easy enough, but this will break migration: We must have a
way to detect what s->status really means. Is it only the status register
(as used by new versions) or do we have to extract internal error status
flags?

Here we seem to be lacking some kind of optional subsection that would
be simply ignored by older versions, but can contain information for new
versions. Is there any precedence on how to solve this?
---
 hw/ide/core.c     |   22 +++++++++++++++++++++-
 hw/ide/internal.h |    5 +++++
 hw/ide/pci.c      |   19 +++++++++++++------
 3 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/hw/ide/core.c b/hw/ide/core.c
index 95beb17..dc9b94b 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->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)
+{
+    IDEState *s = opaque;
+
+    return (s->error_status != 0);
+}
+
 /* Fields for GET_EVENT_STATUS_NOTIFICATION ATAPI command */
 const VMStateDescription vmstate_ide_atapi_gesn_state = {
     .name ="ide_drive/atapi/gesn_state",
@@ -1879,6 +1886,16 @@ const VMStateDescription vmstate_ide_drive_pio_state = {
     }
 };
 
+const VMStateDescription vmstate_ide_error_status = {
+    .name ="ide_drive/error",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields = (VMStateField []) {
+        VMSTATE_INT32(error_status, IDEState),
+    }
+};
+
 const VMStateDescription vmstate_ide_drive = {
     .name = "ide_drive",
     .version_id = 3,
@@ -1916,6 +1933,9 @@ const VMStateDescription vmstate_ide_drive = {
             .vmsd = &vmstate_ide_atapi_gesn_state,
             .needed = ide_atapi_gesn_needed,
         }, {
+            .vmsd = &vmstate_ide_error_status,
+            .needed = ide_error_needed,
+        }, {
             /* empty */
         }
     }
diff --git a/hw/ide/internal.h b/hw/ide/internal.h
index c2b35ec..9ba4b34 100644
--- a/hw/ide/internal.h
+++ b/hw/ide/internal.h
@@ -440,6 +440,9 @@ struct IDEState {
     uint8_t end_transfer_fn_idx;
     QEMUTimer *sector_write_timer; /* only used for win2k install hack */
     uint32_t irq_count; /* counts IRQs when using win2k install hack */
+
+    int error_status;
+
     /* CF-ATA extended error */
     uint8_t ext_error;
     /* CF-ATA metadata storage */
@@ -505,6 +508,8 @@ 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
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index a4726ad..531ad97 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -183,27 +183,34 @@ 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;
+    IDEState *s;
     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;
+    }
+
+    s = bmdma_active_if(bm);
+    is_read = !!(s->error_status & BM_STATUS_RETRY_READ);
 
-    if (bm->status & BM_STATUS_DMA_RETRY) {
-        bm->status &= ~(BM_STATUS_DMA_RETRY | BM_STATUS_RETRY_READ);
+    if (s->error_status & BM_STATUS_DMA_RETRY) {
+        s->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 (s->error_status & BM_STATUS_PIO_RETRY) {
+        s->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 (s->error_status & BM_STATUS_RETRY_FLUSH) {
         ide_flush_cache(bmdma_active_if(bm));
     }
 }
-- 
1.7.5.2