diff mbox

[RFC] ide: Break migration by splitting error status from status register

Message ID 1306836595-8481-1-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf May 31, 2011, 10:09 a.m. UTC 
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(-)

Comments

Paolo Bonzini June 6, 2011, 12:35 p.m. UTC | #1 
On 05/31/2011 12:09 PM, Kevin Wolf wrote:
> 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?

You need to stop writing either status field to the migration stream; 
instead you recreate the "wrong" status field before saving, and set the 
"right" status fields from the saved data after loading.

On top of this, you use a subsection to save bits 3-7 of the "real" IDE 
status registers.  These had been hijacked, so there is no room for them 
in the migration stream.  Of course, the subsection is needed only if 
any of those bits is set.

Paolo
diff mbox

Patch

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));
     }
 }