Re: [Qemu-devel] [PATCH] migration: avoid copying ignore-shared ramblock when in incoming migration

[Peter Xu]

On Tue, Mar 19, 2019 at 11:49:22AM -0400, Catherine Ho wrote:
> Commit 18269069c310 ("migration: Introduce ignore-shared capability") 
> addes a ignore-shared capability to bypass the shared ramblock (e,g,
> membackend + numa node). It does good to live migration.
> 
> This commit expected that QEMU doesn't write to guest RAM until 
> VM starts, but it does on aarch64 qemu:
> Backtrace:
> 1  0x000055f4a296dd84 in address_space_write_rom_internal () at exec.c:3458
> 2  0x000055f4a296de3a in address_space_write_rom () at exec.c:3479
> 3  0x000055f4a2d519ff in rom_reset () at hw/core/loader.c:1101
> 4  0x000055f4a2d475ec in qemu_devices_reset () at hw/core/reset.c:69
> 5  0x000055f4a2c90a28 in qemu_system_reset () at vl.c:1675
> 6  0x000055f4a2c9851d in main () at vl.c:4552
> 
> In address_space_write_rom_internal, if the ramblock is RAM of 
> ramblock_is_ignored(), memcpy this ramblock will cause the memory-backend
> file corruption.
> But in normal case (!in_incoming_migration), this logic should be reserved.

(I am sorry if these questions are silly...)

Could I ask when a ROM will be changed during execution of the guest,
and why?

I can understand that a rom_reset() should feed the real ROM buffer
with the ROM data that provided, but I don't understand why it needs
to be changed later on and why data could corrupt (in that case it's
not only changed but shared with other processes with reasons).

> 
> Fixes: commit 18269069c310 ("migration: Introduce ignore-shared capability") 
> Signed-off-by: Catherine Ho <address@hidden>
> Suggested-by: Yury Kotov <address@hidden>
> ---
>  exec.c                    | 4 ++++
>  include/exec/cpu-common.h | 1 +
>  include/qemu/option.h     | 1 +
>  migration/ram.c           | 2 +-
>  vl.c                      | 2 ++
>  5 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index 86a38d3b3b..f08321fb7a 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -47,6 +47,7 @@
>  #include "exec/address-spaces.h"
>  #include "sysemu/xen-mapcache.h"
>  #include "trace-root.h"
> +#include "qemu/option.h"
>  
>  #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
>  #include <linux/falloc.h>
> @@ -3455,6 +3456,9 @@ static inline MemTxResult 
> address_space_write_rom_internal(AddressSpace *as,
>              l = memory_access_size(mr, l, addr1);
>          } else {
>              /* ROM/RAM case */
> +            if (in_incoming_migration && ramblock_is_ignored(mr->ram_block)) 
> {

It's hackish IMHO... and this will be true not only during the
incoming migration, but also for the rest lifecycle of the VM after
migration finished.  Is that really what you want (since after
in_incoming_migration is set, it's never cleared)?  Even if the answer
is yes, you can also consider to reuse the "incoming" variable right
above it and export that? Though maybe it needs a rename.

Thanks,

> @@ -3797,6 +3798,7 @@ int main(int argc, char **argv, char **envp)
>                      runstate_set(RUN_STATE_INMIGRATE);
>                  }
>                  incoming = optarg;
> +                in_incoming_migration = true;
>                  break;
>              case QEMU_OPTION_only_migratable:
>                  /*
> -- 
> 2.17.1
> 
> 

Regards,

-- 
Peter Xu