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

[Yury Kotov]

11.01.2019, 21:25, "Dr. David Alan Gilbert" <address@hidden>:
> * 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
>

Ok, thanks. I didn't know that. What do you think about addr checking only for
ignored RAMBlocks? So, only memory backends will be checked and their addrs must
be equal IIUC. Also, in this case there is no need to increment the version.

>>  >
>>  > 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

Regards,
Yury