Re: [Qemu-devel] [PATCH] Fix pflash migration

[Kevin Wolf <kwolf@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