| Message ID | 1460720490-9545-1-git-send-email-dgilbert@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/15/16 13:41, Dr. David Alan Gilbert (git) wrote: > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > Pflash migration (e.g. q35 + EFI variable storage) fails > with the assert: > > bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed. > > This avoids the problem by delaying the pflash update until after > the device loads complete. > > Tested by: > Migrating Q35/EFI vm. > Changing efi variable content (with efiboot in the guest) > md5sum'ing the variable file before migration and after. > > This is a fix that Paolo posted in the message > 570244B3.4070105@redhat.com > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > --- > hw/block/pflash_cfi01.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index c475c2a..106a775 100644 > --- a/hw/block/pflash_cfi01.c > +++ b/hw/block/pflash_cfi01.c > @@ -46,6 +46,7 @@ > #include "exec/address-spaces.h" > #include "qemu/host-utils.h" > #include "hw/sysbus.h" > +#include "sysemu/sysemu.h" > > #define PFLASH_BUG(fmt, ...) \ > do { \ > @@ -97,6 +98,7 @@ struct pflash_t { > MemoryRegion mem; > char *name; > void *storage; > + VMChangeStateEntry *vmstate; > }; > > static int pflash_post_load(void *opaque, int version_id); > @@ -944,13 +946,25 @@ MemoryRegion *pflash_cfi01_get_memory(pflash_t *fl) > return &fl->mem; > } > > +static void postload_update_cb(void *opaque, int running, RunState state) > +{ > + pflash_t *pfl = opaque; > + > + /* This is called after bdrv_invalidate_cache_all. */ > + qemu_del_vm_change_state_handler(pfl->vmstate); > + pfl->vmstate = NULL; > + > + DPRINTF("%s: updating bdrv for %s\n", __func__, pfl->name); > + pflash_update(pfl, 0, pfl->sector_len * pfl->nb_blocs); > +} > + > static int pflash_post_load(void *opaque, int version_id) > { > pflash_t *pfl = opaque; > > if (!pfl->ro) { > - DPRINTF("%s: updating bdrv for %s\n", __func__, pfl->name); > - pflash_update(pfl, 0, pfl->sector_len * pfl->nb_blocs); > + pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb, > + pfl); > } > return 0; > } > I can't offer an in-depth review (I don't know migration internals / vmstate changes well enough, sorry), but I agree with the fix (and am thankful for it), and both the patch and the testing look good to me. Perhaps some assert()s could be added to postload_update_cb(), about "running" and "state", but I'm not certain. Acked-by: Laszlo Ersek <lersek@redhat.com>
Am 15.04.2016 um 14:19 hat Laszlo Ersek geschrieben: > On 04/15/16 13:41, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com> > > > > Pflash migration (e.g. q35 + EFI variable storage) fails > > with the assert: > > > > bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed. > > > > This avoids the problem by delaying the pflash update until after > > the device loads complete. > > > > Tested by: > > Migrating Q35/EFI vm. > > Changing efi variable content (with efiboot in the guest) > > md5sum'ing the variable file before migration and after. > > > > This is a fix that Paolo posted in the message > > 570244B3.4070105@redhat.com > > > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > hw/block/pflash_cfi01.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > > index c475c2a..106a775 100644 > > --- a/hw/block/pflash_cfi01.c > > +++ b/hw/block/pflash_cfi01.c > > @@ -46,6 +46,7 @@ > > #include "exec/address-spaces.h" > > #include "qemu/host-utils.h" > > #include "hw/sysbus.h" > > +#include "sysemu/sysemu.h" > > > > #define PFLASH_BUG(fmt, ...) \ > > do { \ > > @@ -97,6 +98,7 @@ struct pflash_t { > > MemoryRegion mem; > > char *name; > > void *storage; > > + VMChangeStateEntry *vmstate; > > }; > > > > static int pflash_post_load(void *opaque, int version_id); > > @@ -944,13 +946,25 @@ MemoryRegion *pflash_cfi01_get_memory(pflash_t *fl) > > return &fl->mem; > > } > > > > +static void postload_update_cb(void *opaque, int running, RunState state) > > +{ > > + pflash_t *pfl = opaque; > > + > > + /* This is called after bdrv_invalidate_cache_all. */ > > + qemu_del_vm_change_state_handler(pfl->vmstate); > > + pfl->vmstate = NULL; > > + > > + DPRINTF("%s: updating bdrv for %s\n", __func__, pfl->name); > > + pflash_update(pfl, 0, pfl->sector_len * pfl->nb_blocs); > > +} > > + > > static int pflash_post_load(void *opaque, int version_id) > > { > > pflash_t *pfl = opaque; > > > > if (!pfl->ro) { > > - DPRINTF("%s: updating bdrv for %s\n", __func__, pfl->name); > > - pflash_update(pfl, 0, pfl->sector_len * pfl->nb_blocs); > > + pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb, > > + pfl); > > } > > return 0; > > } > > > > I can't offer an in-depth review (I don't know migration internals / > vmstate changes well enough, sorry), but I agree with the fix (and am > thankful for it), and both the patch and the testing look good to me. > > Perhaps some assert()s could be added to postload_update_cb(), about > "running" and "state", but I'm not certain. > > Acked-by: Laszlo Ersek <lersek@redhat.com> Thanks, applied to the block branch. Kevin
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index c475c2a..106a775 100644 --- a/hw/block/pflash_cfi01.c +++ b/hw/block/pflash_cfi01.c @@ -46,6 +46,7 @@ #include "exec/address-spaces.h" #include "qemu/host-utils.h" #include "hw/sysbus.h" +#include "sysemu/sysemu.h" #define PFLASH_BUG(fmt, ...) \ do { \ @@ -97,6 +98,7 @@ struct pflash_t { MemoryRegion mem; char *name; void *storage; + VMChangeStateEntry *vmstate; }; static int pflash_post_load(void *opaque, int version_id); @@ -944,13 +946,25 @@ MemoryRegion *pflash_cfi01_get_memory(pflash_t *fl) return &fl->mem; } +static void postload_update_cb(void *opaque, int running, RunState state) +{ + pflash_t *pfl = opaque; + + /* This is called after bdrv_invalidate_cache_all. */ + qemu_del_vm_change_state_handler(pfl->vmstate); + pfl->vmstate = NULL; + + DPRINTF("%s: updating bdrv for %s\n", __func__, pfl->name); + pflash_update(pfl, 0, pfl->sector_len * pfl->nb_blocs); +} + static int pflash_post_load(void *opaque, int version_id) { pflash_t *pfl = opaque; if (!pfl->ro) { - DPRINTF("%s: updating bdrv for %s\n", __func__, pfl->name); - pflash_update(pfl, 0, pfl->sector_len * pfl->nb_blocs); + pfl->vmstate = qemu_add_vm_change_state_handler(postload_update_cb, + pfl); } return 0; }