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

[chenxi He]

On Wed, 20 Mar 2019 at 13:07, Peter Xu <address@hidden> wrote:
>
> 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?
Sure :),  as told by Peter
"as part of reset, we write the contents of ROM blobs, ELF files loaded through
-kernel, etc, to the guest memory."

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

I guess (sorry about that), rom data should be restore on reset. I watched about
several mega bytes passed to memcpy in my debuggin.
But if the ram is memory-backend-file and shared, all the neccessry data had
been reserved and unchanged in memory-backend file.

Best regard,
Catherine

>
> >
> > 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, I am considerring clear it after
process_incoming_migration_co->qemu_loadvm_state

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