| Message ID | 1301638940-22372-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/01/11 08:22, Jason Wang wrote: > We need to keep DIR register unchanged across migration, but currently it > depends on the media_changed flags from block layer. Since we do not > save/restore it and the bdrv_open() called in dest node may set the > media_changed flag when trying to open floppy image, guest driver may 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> Looked through this, and it looks perfectly reasonable to me. Reviewed-by: Jes Sorensen <Jes.Sorensen@redhat.com>
On 04/01/2011 08:22 AM, Jason Wang wrote: > + > + if (drive->bs == NULL) { > + return 1; Is it okay to return 1 here? Have you tested the case when both the source and the target drives have no floppy? (The "media_changed == 2" in my sample code was basically a way to save the "media present" state of the drive on the source). > + } else { > + drive->bs->media_changed = drive->media_changed; > + return 0; > + } The code is also missing the case of a pc-0.14 machine. The problem here is that the pc-0.15 machine hasn't been created yet, you get the honor. :( Paolo
Paolo Bonzini writes: > On 04/01/2011 08:22 AM, Jason Wang wrote: > > + > > + if (drive->bs == NULL) { > > + return 1; > > Is it okay to return 1 here? Have you tested the case when both the > source and the target drives have no floppy? > Thanks for the reminding, and it could be fixed by put all pre/post callbacks into the subsections. > (The "media_changed == 2" in my sample code was basically a way to save > the "media present" state of the drive on the source). > Right, but it would make subsection saving be the common case (consider most of the vm may just have one floppy but we have two drives). A better solution maybe: 1 Set default_migration_media_changed be 0 for 0.15 and 1 for elder 2 Unconditiaonlly send subsection when it was 0, and do not send subsection when it was 1 3 Set media_changed to default_migration_media_changed in pre_load() 4 Let all pre/post to be in subsection After those, we can make sure the migration between 0.15 could get correct media_changed, also make sure the seamless migration between 0.15 and older machine types. Any suggestions? > > + } else { > > + drive->bs->media_changed = drive->media_changed; > > + return 0; > > + } > > The code is also missing the case of a pc-0.14 machine. The problem > here is that the pc-0.15 machine hasn't been created yet, you get the > honor. :( > Would create such one :) > Paolo
On 04/01/2011 12:03 PM, Jason Wang wrote: > Paolo Bonzini writes: > > On 04/01/2011 08:22 AM, Jason Wang wrote: > > > + > > > + if (drive->bs == NULL) { > > > + return 1; > > > > Is it okay to return 1 here? Have you tested the case when both the > > source and the target drives have no floppy? > > > > Thanks for the reminding, and it could be fixed by put all pre/post callbacks > into the subsections. Not sure that works, post callbacks are not called for subsections that are not needed. > > (The "media_changed == 2" in my sample code was basically a way to save > > the "media present" state of the drive on the source). > > Right, but it would make subsection saving be the common case (consider most of > the vm may just have one floppy but we have two drives). Hmm, right, there's 2 bits involved---media present and media changed. > 1 Set default_migration_media_changed be 0 for 0.15 and 1 for elder > 2 Unconditionally send subsection when it was 0, and do not send subsection when > it was 1 Which means, never send it for pc-0.14 and always for pc-0.15. At this point I'm starting to think that this new-to-old migration business is a lost cause, and it's easier to bump the version number and just default the field to 1 if the version is old. :( The problem here is that you're solving a different problem than what subsections were supposed to help with. Subsections help with "the value that old versions used is usually but not always correct". Here you have "the value that old versions used is usually *in*correct, but nobody noticed so far". Michael/Amit, as you were the ones proposing stronger new-to-old support, can you read the thread and see if you have any ideas? Paolo
Jason Wang <jasowang@redhat.com> wrote: > Paolo Bonzini writes: > > On 04/01/2011 08:22 AM, Jason Wang wrote: > > > + > > > + if (drive->bs == NULL) { > > > + return 1; > > > > Is it okay to return 1 here? Have you tested the case when both the > > source and the target drives have no floppy? > > > > Thanks for the reminding, and it could be fixed by put all pre/post callbacks > into the subsections. > > > (The "media_changed == 2" in my sample code was basically a way to save > > the "media present" state of the drive on the source). > > > > Right, but it would make subsection saving be the common case (consider most of > the vm may just have one floppy but we have two drives). A better solution maybe: > > 1 Set default_migration_media_changed be 0 for 0.15 and 1 for elder > 2 Unconditiaonlly send subsection when it was 0, and do not send subsection when > it was 1 If you do this, you are just removing the capability of migrating to older versions. If you need to send a subsection unconditionlly, then just upgrade the version of the device, it is going to give the same result. Later, Juan.
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. Since we do not > save/restore it and the bdrv_open() called in dest node may set the > media_changed flag when trying to open floppy image, guest driver may 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> > --- > > Changed from V2: > > According to Paolo's suggestions, a default_migration_media_changed property was > added to avoid saving subsections as much as possible. Its was set media_changed > in pre_load callback and then we can avoid the saving when it was equal to the > media_changed when migrating the FDrive. Behaviors of elder machine types are > also kept through compat_props. > > Changed from V1: > > Check the drive->bs during post_load. > > hw/fdc.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > hw/pc_piix.c | 48 +++++++++++++++++++++++++++++++++ > 2 files changed, 128 insertions(+), 3 deletions(-) > > +static int fdrive_pre_load(void *opaque) > +static void fdrive_pre_save(void *opaque) > +static int fdrive_post_load(void *opaque, int version_id) This three functions can be moved to vmstate_fdrive_media_changed, once there, it simplifies the "check of drive->bs != NULL. I really think that we can remove all the default_migration_media_changed code. I think that: - if drive is present: we sent media_changed. - if drive is not present, we don't sent it. And that is it, no problem with older versions at all. If drive is there, we sent the current media_changed value. It is just easier to see what is the default "media_changed" value. And we only sent it if it is different. If we are migrating to 0.14, and media_changed "changed", migration just fails, as expected. Trying to make device working at this level is not going to work. We need to fix at the device init devel, and that infrastructure is not there yet. Later, Juan.
diff --git a/hw/fdc.c b/hw/fdc.c index 9fdbc75..1f3bbd2 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 */ @@ -67,6 +68,8 @@ typedef enum FDiskFlags { FDISK_DBL_SIDES = 0x01, } FDiskFlags; +typedef struct FDCtrl FDCtrl; + typedef struct FDrive { BlockDriverState *bs; /* Drive status */ @@ -82,6 +85,8 @@ 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 */ + FDCtrl *fdctrl; } FDrive; static void fd_init(FDrive *drv) @@ -201,8 +206,6 @@ static void fd_revalidate(FDrive *drv) /********************************************************/ /* Intel 82078 floppy disk controller emulation */ -typedef struct FDCtrl FDCtrl; - static void fdctrl_reset(FDCtrl *fdctrl, int do_irq); static void fdctrl_reset_fifo(FDCtrl *fdctrl); static int fdctrl_transfer_handler (void *opaque, int nchan, @@ -414,6 +417,7 @@ struct FDCtrl { uint8_t num_floppies; FDrive drives[MAX_FD]; int reset_sensei; + uint8_t default_migration_media_changed; }; typedef struct FDCtrlSysBus { @@ -533,16 +537,82 @@ static CPUWriteMemoryFunc * const fdctrl_mem_write_strict[3] = { NULL, }; +static int fdrive_pre_load(void *opaque) +{ + FDrive *drive = opaque; + + if (drive->bs != NULL) { + drive->media_changed = drive->fdctrl->default_migration_media_changed; + } + + return 0; +} + +static void fdrive_pre_save(void *opaque) +{ + FDrive *drive = opaque; + + if (drive->bs != NULL) { + drive->media_changed = drive->bs->media_changed; + } +} + +static int fdrive_post_load(void *opaque, int version_id) +{ + FDrive *drive = opaque; + + if (drive->bs == NULL) { + return 1; + } else { + drive->bs->media_changed = drive->media_changed; + return 0; + } +} + +static bool fdrive_media_changed_needed(void *opaque) +{ + FDrive *drive = opaque; + + /* Only save media_changed when it was not equal to default to avoid saving + * subsections as much as possible. + */ + return (drive->bs != NULL && + drive->bs->media_changed != + drive->fdctrl->default_migration_media_changed); +} + +static const VMStateDescription vmstate_fdrive_media_changed = { + .name = "fdrive/media_changed", + .version_id = 1, + .minimum_version_id = 1, + .minimum_version_id_old = 1, + .fields = (VMStateField[]) { + VMSTATE_UINT8(media_changed, FDrive), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_fdrive = { .name = "fdrive", .version_id = 1, .minimum_version_id = 1, .minimum_version_id_old = 1, - .fields = (VMStateField []) { + .pre_load = fdrive_pre_load, + .pre_save = fdrive_pre_save, + .post_load = fdrive_post_load, + .fields = (VMStateField[]) { VMSTATE_UINT8(head, FDrive), VMSTATE_UINT8(track, FDrive), VMSTATE_UINT8(sect, FDrive), VMSTATE_END_OF_LIST() + }, + .subsections = (VMStateSubsection[]) { + { + .vmsd = &vmstate_fdrive_media_changed, + .needed = &fdrive_media_changed_needed, + } , { + /* empty */ + } } }; @@ -1766,6 +1836,7 @@ static int fdctrl_connect_drives(FDCtrl *fdctrl) fd_revalidate(drive); if (drive->bs) { bdrv_set_removable(drive->bs, 1); + drive->fdctrl = fdctrl; } } return 0; @@ -1934,6 +2005,8 @@ static ISADeviceInfo isa_fdc_info = { DEFINE_PROP_DRIVE("driveB", FDCtrlISABus, state.drives[1].bs), DEFINE_PROP_INT32("bootindexA", FDCtrlISABus, bootindexA, -1), DEFINE_PROP_INT32("bootindexB", FDCtrlISABus, bootindexB, -1), + DEFINE_PROP_UINT8("default_migration_media_changed", FDCtrlISABus, + state.default_migration_media_changed, 0), DEFINE_PROP_END_OF_LIST(), }, }; @@ -1957,6 +2030,8 @@ static SysBusDeviceInfo sysbus_fdc_info = { .qdev.props = (Property[]) { DEFINE_PROP_DRIVE("driveA", FDCtrlSysBus, state.drives[0].bs), DEFINE_PROP_DRIVE("driveB", FDCtrlSysBus, state.drives[1].bs), + DEFINE_PROP_UINT8("default_migration_media_changed", FDCtrlSysBus, + state.default_migration_media_changed, 0), DEFINE_PROP_END_OF_LIST(), }, }; @@ -1969,6 +2044,8 @@ static SysBusDeviceInfo sun4m_fdc_info = { .qdev.reset = fdctrl_external_reset_sysbus, .qdev.props = (Property[]) { DEFINE_PROP_DRIVE("drive", FDCtrlSysBus, state.drives[0].bs), + DEFINE_PROP_UINT8("default_migration_media_changed", FDCtrlSysBus, + state.default_migration_media_changed, 0), DEFINE_PROP_END_OF_LIST(), }, }; diff --git a/hw/pc_piix.c b/hw/pc_piix.c index b3ede89..b80551a 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -258,6 +258,18 @@ static QEMUMachine pc_machine_v0_13 = { .driver = "PCI", .property = "command_serr_enable", .value = "off", + }, { + .driver = "isa-fdc", + .property = "default_migration_media_changed", + .value = stringify(1), + }, { + .driver = "sysbus-fdc", + .property = "default_migration_media_changed", + .value = stringify(1), + }, { + .driver = "SUNW,fdtwo", + .property = "default_migration_media_changed", + .value = stringify(1), }, { /* end of list */ } }, @@ -289,6 +301,18 @@ static QEMUMachine pc_machine_v0_12 = { .driver = "PCI", .property = "command_serr_enable", .value = "off", + }, { + .driver = "isa-fdc", + .property = "default_migration_media_changed", + .value = stringify(1), + }, { + .driver = "sysbus-fdc", + .property = "default_migration_media_changed", + .value = stringify(1), + }, { + .driver = "SUNW,fdtwo", + .property = "default_migration_media_changed", + .value = stringify(1), }, { /* end of list */ } } @@ -328,6 +352,18 @@ static QEMUMachine pc_machine_v0_11 = { .driver = "PCI", .property = "command_serr_enable", .value = "off", + }, { + .driver = "isa-fdc", + .property = "default_migration_media_changed", + .value = stringify(1), + }, { + .driver = "sysbus-fdc", + .property = "default_migration_media_changed", + .value = stringify(1), + }, { + .driver = "SUNW,fdtwo", + .property = "default_migration_media_changed", + .value = stringify(1), }, { /* end of list */ } } @@ -379,6 +415,18 @@ static QEMUMachine pc_machine_v0_10 = { .driver = "PCI", .property = "command_serr_enable", .value = "off", + }, { + .driver = "isa-fdc", + .property = "default_migration_media_changed", + .value = stringify(1), + }, { + .driver = "sysbus-fdc", + .property = "default_migration_media_changed", + .value = stringify(1), + }, { + .driver = "SUNW,fdtwo", + .property = "default_migration_media_changed", + .value = stringify(1), }, { /* end of list */ } },
We need to keep DIR register unchanged across migration, but currently it depends on the media_changed flags from block layer. Since we do not save/restore it and the bdrv_open() called in dest node may set the media_changed flag when trying to open floppy image, guest driver may 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> --- Changed from V2: According to Paolo's suggestions, a default_migration_media_changed property was added to avoid saving subsections as much as possible. Its was set media_changed in pre_load callback and then we can avoid the saving when it was equal to the media_changed when migrating the FDrive. Behaviors of elder machine types are also kept through compat_props. Changed from V1: Check the drive->bs during post_load. hw/fdc.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-- hw/pc_piix.c | 48 +++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 3 deletions(-)