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

[Dr. David Alan Gilbert]

* Yury Kotov (address@hidden) wrote:
> 10.01.2019, 23:14, "Dr. David Alan Gilbert" <address@hidden>:
> > * 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.
> >
> 
> Yes, you are right. It seems that instead I should check block->mr->addr.

I don't think that's guaranteed to be the same either;  for example
a video buffer or ROM on a  PCI card gets mapped into different parts of
the guests physical address space depending on writes into the PCI
bars.  Some RAMBlocks dont even have a mapping associated with them.

If you do want to add something here, please don't increment the version
- unless we're desperate I want to keep the version number the same so
that backwards migration works.

Dave

> >
> > 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.
> >
> > 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
> 
> Regards,
> Yury
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK