Patchwork floppy: save and restore DIR register

login
register
mail settings
Submitter Jason Wang
Date March 25, 2011, 6:27 a.m.
Message ID <20110325062741.23726.36621.stgit@dhcp-91-7.nay.redhat.com.englab.nay.redhat.com>
Download mbox | patch
Permalink /patch/88331/
State New
Headers show

Comments

Jason Wang - March 25, 2011, 6:27 a.m.
We need to keep DIR register unchanged across migration, but currently it
depends on the media_changed flags from block layer and we do not save/restore
it which could let the guest driver think the floppy have changed after
migration. To fix this, a new filed media_changed in FDrive strcutre was
introduced in order to save and restore the it from block layer through
pre_save/post_load callbacks.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/fdc.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)
Juan Quintela - March 25, 2011, 9:25 a.m.
Jason Wang <jasowang@redhat.com> wrote:
> We need to keep DIR register unchanged across migration, but currently it
> depends on the media_changed flags from block layer and we do not save/restore
> it which could let the guest driver think the floppy have changed after
> migration. To fix this, a new filed media_changed in FDrive strcutre was
> introduced in order to save and restore the it from block layer through
> pre_save/post_load callbacks.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/fdc.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/hw/fdc.c b/hw/fdc.c
> index 9fdbc75..8257d51 100644
> --- a/hw/fdc.c
> +++ b/hw/fdc.c
> @@ -36,6 +36,7 @@
>  #include "qdev-addr.h"
>  #include "blockdev.h"
>  #include "sysemu.h"
> +#include "block_int.h"
>  
>  /********************************************************/
>  /* debug Floppy devices */
> @@ -82,6 +83,7 @@ typedef struct FDrive {
>      uint8_t max_track;        /* Nb of tracks           */
>      uint16_t bps;             /* Bytes per sector       */
>      uint8_t ro;               /* Is read-only           */
> +    uint8_t media_changed;    /* Is media changed       */
>  } FDrive;
>  
>  static void fd_init(FDrive *drv)
> @@ -533,6 +535,42 @@ static CPUWriteMemoryFunc * const fdctrl_mem_write_strict[3] = {
>      NULL,
>  };
>  
> +static void fdrive_media_changed_pre_save(void *opaque)
> +{
> +    FDrive *drive = opaque;
> +
> +    drive->media_changed = drive->bs->media_changed;
> +}
> +
> +static int fdrive_media_changed_post_load(void *opaque, int version_id)
> +{
> +    FDrive *drive = opaque;
> +
> +    drive->bs->media_changed = drive->media_changed;

shouldn't this be something like:

if (!drive->bs)
   return 1;

drive->bs->media_changed = drive->media_changed.

???

Otherwise, we can can have troubles if floppy is not opened in the other side.


Rest of code looks nice, thanks.

Later, Juan.
Paolo Bonzini - March 25, 2011, 11:56 a.m.
On 03/25/2011 07:27 AM, Jason Wang wrote:
> We need to keep DIR register unchanged across migration, but currently it
> depends on the media_changed flags from block layer and we do not save/restore
> it which could let the guest driver think the floppy have changed after
> migration. To fix this, a new filed media_changed in FDrive strcutre was
> introduced in order to save and restore the it from block layer through
> pre_save/post_load callbacks.

I guess you can avoid saving if the media changed flag is zero, too 
(which should be the common case after the guest has booted, right?).

Paolo
Jason Wang - March 28, 2011, 2:40 a.m.
Paolo Bonzini writes:
 > On 03/25/2011 07:27 AM, Jason Wang wrote:
 > > We need to keep DIR register unchanged across migration, but currently it
 > > depends on the media_changed flags from block layer and we do not save/restore
 > > it which could let the guest driver think the floppy have changed after
 > > migration. To fix this, a new filed media_changed in FDrive strcutre was
 > > introduced in order to save and restore the it from block layer through
 > > pre_save/post_load callbacks.
 > 
 > I guess you can avoid saving if the media changed flag is zero, too 
 > (which should be the common case after the guest has booted, right?).
 > 
 > Paolo
 > 

Yes, zero is the common case, but the bdrv_open() called by listening qemu in
dest mode would always set the media_changed to one, so we must save and restore
it during migration.
Paolo Bonzini - March 28, 2011, 7:49 a.m.
On 03/28/2011 04:40 AM, Jason Wang wrote:
> > On 03/25/2011 07:27 AM, Jason Wang wrote:
> > >  We need to keep DIR register unchanged across migration, but currently it
> > >  depends on the media_changed flags from block layer and we do not save/restore
> > >  it which could let the guest driver think the floppy have changed after
> > >  migration. To fix this, a new filed media_changed in FDrive strcutre was
> > >  introduced in order to save and restore the it from block layer through
> > >  pre_save/post_load callbacks.
> >
> >  I guess you can avoid saving if the media changed flag is zero, too
> >  (which should be the common case after the guest has booted, right?).
> >
> >  Paolo
>
> Yes, zero is the common case, but the bdrv_open() called by listening qemu in
> dest mode would always set the media_changed to one, so we must save and restore
> it during migration.

If zero is the common case, you can do something like this to avoid 
saving the subsection in the common case and allow seamless migration 
from 0.15 to 0.14:

static void fdrive_pre_save(void *opaque)
{
     FDrive *drive = opaque;

     if (drive->bs != NULL) {
         drive->media_changed = drive->bs->media_changed;
     } else {
         drive->media_changed = 2;
     }
}

static void fdrive_pre_load(void *opaque)
{
     FDrive *drive = opaque;

     /* Set the default value in case there is no subsection.  */
     if (drive->bs != NULL) {
         drive->media_changed = 0;
     } else {
         drive->media_changed = 2;
     }
}

static void fdrive_post_load(void *opaque)
{
     /* Fail if the source machine had a disk and we don't.  */
     if (drive->bs == NULL && drive->media_changed != 2) {
         return 1;
     }

     /* Reset the field if both us and the source machine have a disk. */
     if (drive->bs != NULL && drive->media_changed != 2) {
         /* bdrv_open() called in dest node may set the
            media_changed flag when trying to open floppy image.
            Copy it here, so that it is reset even if the subsection
            was not saved.  */
         drive->bs->media_changed = drive->media_changed;
     }
     return 0;
}

static bool fdrive_media_changed_needed(void *opaque)
{
     FDrive *drive = opaque;

     return (drive->bs != NULL && drive->media_changed != 0);
}

Note that there is a small change here: if the media changed flag is 1 
and you migrate 0.14 to 0.15, the destination machine will see the flag 
reset to 0 (while 0.14 to 0.14 will see the flag set to 1 just because 
that's how bdrv_open leaves it).

To fix this, you can define a boolean qdev property 
default_migration_media_changed and use it instead of the two hard-coded 
"0" values.  Then, register the default value of the property in 
hw/pc_piix.c to be 0 in pc-0.15, and 1 in pc-0.14 and older.

Paolo

Patch

diff --git a/hw/fdc.c b/hw/fdc.c
index 9fdbc75..8257d51 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -36,6 +36,7 @@ 
 #include "qdev-addr.h"
 #include "blockdev.h"
 #include "sysemu.h"
+#include "block_int.h"
 
 /********************************************************/
 /* debug Floppy devices */
@@ -82,6 +83,7 @@  typedef struct FDrive {
     uint8_t max_track;        /* Nb of tracks           */
     uint16_t bps;             /* Bytes per sector       */
     uint8_t ro;               /* Is read-only           */
+    uint8_t media_changed;    /* Is media changed       */
 } FDrive;
 
 static void fd_init(FDrive *drv)
@@ -533,6 +535,42 @@  static CPUWriteMemoryFunc * const fdctrl_mem_write_strict[3] = {
     NULL,
 };
 
+static void fdrive_media_changed_pre_save(void *opaque)
+{
+    FDrive *drive = opaque;
+
+    drive->media_changed = drive->bs->media_changed;
+}
+
+static int fdrive_media_changed_post_load(void *opaque, int version_id)
+{
+    FDrive *drive = opaque;
+
+    drive->bs->media_changed = drive->media_changed;
+
+    return 0;
+}
+
+static bool fdrive_has_driver(void *opaque)
+{
+    FDrive *drive = opaque;
+
+    return drive->bs != NULL;
+}
+
+static const VMStateDescription vmstate_fdrive_media_changed = {
+    .name = "fdrive/media_changed",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .pre_save = fdrive_media_changed_pre_save,
+    .post_load = fdrive_media_changed_post_load,
+    .fields      = (VMStateField [] ) {
+        VMSTATE_UINT8(media_changed, FDrive),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_fdrive = {
     .name = "fdrive",
     .version_id = 1,
@@ -543,6 +581,14 @@  static const VMStateDescription vmstate_fdrive = {
         VMSTATE_UINT8(track, FDrive),
         VMSTATE_UINT8(sect, FDrive),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &vmstate_fdrive_media_changed,
+            .needed = &fdrive_has_driver,
+        } , {
+            /* empty */
+        }
     }
 };