* [Qemu-devel] [PATCH] Fix pflash migration
@ 2016-04-15 11:41 Dr. David Alan Gilbert (git)
2016-04-15 12:19 ` Laszlo Ersek
0 siblings, 1 reply; 3+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2016-04-15 11:41 UTC (permalink / raw)
To: qemu-devel; +Cc: pbonzini, peter.maydell, lersek, kwolf, jsnow
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;
}
--
2.5.5
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH] Fix pflash migration
2016-04-15 11:41 [Qemu-devel] [PATCH] Fix pflash migration Dr. David Alan Gilbert (git)
@ 2016-04-15 12:19 ` Laszlo Ersek
2016-04-15 15:28 ` Kevin Wolf
0 siblings, 1 reply; 3+ messages in thread
From: Laszlo Ersek @ 2016-04-15 12:19 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git), qemu-devel
Cc: pbonzini, peter.maydell, kwolf, jsnow
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>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [Qemu-devel] [PATCH] Fix pflash migration
2016-04-15 12:19 ` Laszlo Ersek
@ 2016-04-15 15:28 ` Kevin Wolf
0 siblings, 0 replies; 3+ messages in thread
From: Kevin Wolf @ 2016-04-15 15:28 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Dr. David Alan Gilbert (git), qemu-devel, pbonzini, peter.maydell,
jsnow
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-04-15 15:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-15 11:41 [Qemu-devel] [PATCH] Fix pflash migration Dr. David Alan Gilbert (git)
2016-04-15 12:19 ` Laszlo Ersek
2016-04-15 15:28 ` Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).