Re: [Qemu-devel] [PATCH 1/4] migration: add RAMBlock's offset validation

[Dr. David Alan Gilbert]

* Igor Mammedov (address@hidden) wrote:
> On Thu, 10 Jan 2019 20:14:19 +0000
> "Dr. David Alan Gilbert" <address@hidden> wrote:
> 
> > * Yury Kotov (address@hidden) wrote:
> > > RAM migration has a RAMBlock validation stage (flag 
> > > RAM_SAVE_FLAG_MEM_SIZE).
> > > In this stage QEMU checks further information about RAMBlock:
> > > 1. Presence (by idstr),
> > > 2. Length (trying to resize, when differs),
> > > 3. Optional page size.
> > > 
> > > This patch adds a check for RAMBlock's offset. Currently we check it 
> > > during
> > > RAM pages loading - every RAM page has an offset in its header. But there 
> > > is a
> > > case when we don't send RAM pages (see below).
> > > 
> > > The following commits introduce a capability (ignore-external) to skip 
> > > some
> > > RAM blocks from migration. In such case the migration stream contains only
> > > meta information about RAM blocks to validate them. So, the only way to 
> > > check
> > > block's offset is to send it explicitly.
> > > 
> > > Signed-off-by: Yury Kotov <address@hidden>  
> > 
> > But why check that offsets match?  THey aren't supposed to!
> > Offset's are entirely private to each qemu and they're allowed to be
> > different; the only requirement is that the length and name of each
> > RAMBlock matches, then all the operations we do over the migration
> > stream are relative to the start of the block.
> > 
> > One example where they are validly different is where you hotplug some
> > RAM, so for example:
> > 
> > 
> >   source qemu
> >       -M 4G
> >       hotplug PCI card
> >       hotplug 2G
> > 
> >   destination qemu
> >       -M 4G
> >       PCI card declared on the command line
> >       extra 2G declared on the command line
> > 
> > The offsets are different but we can migrate that case fine.
> PCI mappings are updated after migration is done, to make DST match SRC
> (get_pci_config_device) that mapping is irrelevant here, but I was under
> impression that offset in terms of MemoryRegion (GPA) was passed in
> migration stream, even if we do not update memory mappings for RAM
> (mapping depends on hotplug/CLI order or explicit addr on CLI for memory
> devices).

The GPA must match, but the offset within ram_addr_t space is abstract
and isn't visible to the guest.

Dave

> > 
> > Dave
> > 
> > > ---
> > >  migration/ram.c | 15 +++++++++++++--
> > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 7e7deec4d8..39629254e1 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -3171,6 +3171,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
> > >          if (migrate_postcopy_ram() && block->page_size != 
> > > qemu_host_page_size) {
> > >              qemu_put_be64(f, block->page_size);
> > >          }
> > > +        qemu_put_be64(f, block->offset);
> > >      }
> > >  
> > >      rcu_read_unlock();
> > > @@ -4031,7 +4032,7 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> > > version_id)
> > >  
> > >      seq_iter++;
> > >  
> > > -    if (version_id != 4) {
> > > +    if (version_id < 4) {
> > >          ret = -EINVAL;
> > >      }
> > >  
> > > @@ -4132,6 +4133,16 @@ static int ram_load(QEMUFile *f, void *opaque, int 
> > > version_id)
> > >                              ret = -EINVAL;
> > >                          }
> > >                      }
> > > +                    if (version_id >= 5) {
> > > +                        ram_addr_t offset;
> > > +                        offset = qemu_get_be64(f);
> > > +                        if (block->offset != offset) {
> > > +                            error_report("Mismatched RAM block offset %s 
> > > "
> > > +                                         "%" PRId64 "!= %" PRId64,
> > > +                                         id, offset, 
> > > (uint64_t)block->offset);
> > > +                            ret = -EINVAL;
> > > +                        }
> > > +                    }
> > >                      ram_control_load_hook(f, RAM_CONTROL_BLOCK_REG,
> > >                                            block->idstr);
> > >                  } else {
> > > @@ -4363,5 +4374,5 @@ static SaveVMHandlers savevm_ram_handlers = {
> > >  void ram_mig_init(void)
> > >  {
> > >      qemu_mutex_init(&XBZRLE.lock);
> > > -    register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, 
> > > &ram_state);
> > > +    register_savevm_live(NULL, "ram", 0, 5, &savevm_ram_handlers, 
> > > &ram_state);
> > >  }
> > > -- 
> > > 2.20.1
> > >   
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK