| Message ID | 570244B3.4070105@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
* Paolo Bonzini (pbonzini@redhat.com) wrote: > > > On 01/04/2016 19:58, Dr. David Alan Gilbert wrote: > > In the continuing journeys of trying to migrate a q35 guest with ovmf, > > I've just hit this assert: > > > > qemu-system-x86_64: /root/git/qemu/block/io.c:1297: bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed. > > > > This is just ahead of rc0 - 1458317c8ada834cf39287f6d11a8cb8a37360d6 from yesterday. > > Try this... Well, migration survives; how do I test if pflash is sane after migration? Dave > Paolo > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index c475c2a..e96a7b0 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,24 @@ 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; > } -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 04/05/16 12:48, Dr. David Alan Gilbert wrote: > * Paolo Bonzini (pbonzini@redhat.com) wrote: >> >> >> On 01/04/2016 19:58, Dr. David Alan Gilbert wrote: >>> In the continuing journeys of trying to migrate a q35 guest with ovmf, >>> I've just hit this assert: >>> >>> qemu-system-x86_64: /root/git/qemu/block/io.c:1297: bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed. >>> >>> This is just ahead of rc0 - 1458317c8ada834cf39287f6d11a8cb8a37360d6 from yesterday. >> >> Try this... > > Well, migration survives; how do I test if pflash is sane after migration? You can run sha1sum before / after. The varstore is expected to change only when the UEFI variable servies are exercised. So, if you boot e.g. a Linux guest to a login prompt on the source host, checksum the varstore, then migrate the guest, then verify the checksum on the target host (or, well, shared storage, if you have set it up), it should match. Thanks Laszlo > > Dave > >> Paolo >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index c475c2a..e96a7b0 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,24 @@ 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; >> } > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
* Laszlo Ersek (lersek@redhat.com) wrote: > On 04/05/16 12:48, Dr. David Alan Gilbert wrote: > > * Paolo Bonzini (pbonzini@redhat.com) wrote: > >> > >> > >> On 01/04/2016 19:58, Dr. David Alan Gilbert wrote: > >>> In the continuing journeys of trying to migrate a q35 guest with ovmf, > >>> I've just hit this assert: > >>> > >>> qemu-system-x86_64: /root/git/qemu/block/io.c:1297: bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed. > >>> > >>> This is just ahead of rc0 - 1458317c8ada834cf39287f6d11a8cb8a37360d6 from yesterday. > >> > >> Try this... > > > > Well, migration survives; how do I test if pflash is sane after migration? > > You can run sha1sum before / after. The varstore is expected to change > only when the UEFI variable servies are exercised. So, if you boot e.g. > a Linux guest to a login prompt on the source host, checksum the > varstore, then migrate the guest, then verify the checksum on the target > host (or, well, shared storage, if you have set it up), it should match. OK, yes that works; and I also tried using efibootmgr to tweak the timeout, seeing that the sha changed and then check the sha was correct again after migration. Dave > > Thanks > Laszlo > > > > > Dave > > > >> Paolo > >> > >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > >> index c475c2a..e96a7b0 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,24 @@ 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; > >> } > > -- > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK > > > -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 04/05/16 13:25, Dr. David Alan Gilbert wrote: > * Laszlo Ersek (lersek@redhat.com) wrote: >> On 04/05/16 12:48, Dr. David Alan Gilbert wrote: >>> * Paolo Bonzini (pbonzini@redhat.com) wrote: >>>> >>>> >>>> On 01/04/2016 19:58, Dr. David Alan Gilbert wrote: >>>>> In the continuing journeys of trying to migrate a q35 guest with ovmf, >>>>> I've just hit this assert: >>>>> >>>>> qemu-system-x86_64: /root/git/qemu/block/io.c:1297: bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed. >>>>> >>>>> This is just ahead of rc0 - 1458317c8ada834cf39287f6d11a8cb8a37360d6 from yesterday. >>>> >>>> Try this... >>> >>> Well, migration survives; how do I test if pflash is sane after migration? >> >> You can run sha1sum before / after. The varstore is expected to change >> only when the UEFI variable servies are exercised. So, if you boot e.g. >> a Linux guest to a login prompt on the source host, checksum the >> varstore, then migrate the guest, then verify the checksum on the target >> host (or, well, shared storage, if you have set it up), it should match. > > OK, yes that works; and I also tried using efibootmgr to tweak the timeout, > seeing that the sha changed and then check the sha was correct again after > migration. Thank you both for fixing up the buggy-on-arrival code I had added. Laszlo
On 05/04/2016 12:48, Dr. David Alan Gilbert wrote: > * Paolo Bonzini (pbonzini@redhat.com) wrote: >> >> >> On 01/04/2016 19:58, Dr. David Alan Gilbert wrote: >>> In the continuing journeys of trying to migrate a q35 guest with ovmf, >>> I've just hit this assert: >>> >>> qemu-system-x86_64: /root/git/qemu/block/io.c:1297: bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed. >>> >>> This is just ahead of rc0 - 1458317c8ada834cf39287f6d11a8cb8a37360d6 from yesterday. >> >> Try this... > > Well, migration survives; how do I test if pflash is sane after migration? Use "-S" on the destination and check if the file has the same contents. Paolo > Dave > >> Paolo >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index c475c2a..e96a7b0 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,24 @@ 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; >> } > -- > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK >
On 4 April 2016 at 11:40, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 01/04/2016 19:58, Dr. David Alan Gilbert wrote: >> In the continuing journeys of trying to migrate a q35 guest with ovmf, >> I've just hit this assert: >> >> qemu-system-x86_64: /root/git/qemu/block/io.c:1297: bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed. >> >> This is just ahead of rc0 - 1458317c8ada834cf39287f6d11a8cb8a37360d6 from yesterday. > > Try this... Hi Paolo -- this bug is on the list of "things we should probably fix for 2.6", but your fix below doesn't have your Signed-off-by line. Were you planning to send this as a proper patch, or could you provide an s-o-b line so somebody else (David?) can take the code and clean it up into an actual patch? thanks -- PMM > > Paolo > > diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c > index c475c2a..e96a7b0 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,24 @@ 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; > }
On 14/04/2016 17:30, Peter Maydell wrote: > On 4 April 2016 at 11:40, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> >> On 01/04/2016 19:58, Dr. David Alan Gilbert wrote: >>> In the continuing journeys of trying to migrate a q35 guest with ovmf, >>> I've just hit this assert: >>> >>> qemu-system-x86_64: /root/git/qemu/block/io.c:1297: bdrv_co_do_pwritev: Assertion `!(bs->open_flags & 0x0800)' failed. >>> >>> This is just ahead of rc0 - 1458317c8ada834cf39287f6d11a8cb8a37360d6 from yesterday. >> >> Try this... > > Hi Paolo -- this bug is on the list of "things we should probably > fix for 2.6", but your fix below doesn't have your Signed-off-by > line. Were you planning to send this as a proper patch, or could > you provide an s-o-b line so somebody else (David?) can take the > code and clean it up into an actual patch? Indeed. I had sent the S-o-b to David, but that was a private email so Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> Thanks, Paolo > thanks > -- PMM > >> >> Paolo >> >> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c >> index c475c2a..e96a7b0 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,24 @@ 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; >> }
diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c index c475c2a..e96a7b0 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,24 @@ 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; }