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

[Yury Kotov]

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.

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