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

[Peter Xu]

On Thu, Mar 21, 2019 at 11:31:30PM +0800, Catherine Ho wrote:
> Thanks, Peter
> See my comments below, please
> 
> On Thu, 21 Mar 2019 at 14:10, Peter Xu <address@hidden> wrote:
> >
> > On Wed, Mar 20, 2019 at 04:05:58PM +0800, chenxi He wrote:
> > > 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 see...
> >
> > I tried to dig on how x86 implemented the "-kernel" parameter and I
> > noticed that it's dumping the kernel image into fw_cfg and probably
> > that's also the reason why this should not be a problem for x86
> > because rom reset on x86 won't overwrite the image.  Meanwhile, it
> > seems totally different comparing to what has been done by ARM, which
> > should probably be using rom_add_elf_program() to load the image if my
> > reading is correct, so the kernel image is a ROM on ARM even if it can
> > be written... Is my understanding correct?
> >
> > With that, I still feel that hacking into the memory write functions
> > are tricky and I feel like it could bring hard to debug issues.  Would
> > it be possible that we identify somehow that this ROM is a fake one
> > (since it can actually be written) and we ignore the reset of it when
> > proper (e.g., the initial reset on destination)?
> I found that special rom block is "dtb"
> See
> commit 4c4bf654746eae5a042bde6c150d534d8849b762
> Author: Ard Biesheuvel <address@hidden>
> Date:   Fri Sep 12 14:06:50 2014 +0100
> 
>     hw/arm/boot: load DTB as a ROM image
> 
>     In order to make the device tree blob (DTB) available in memory not only 
> at
>     first boot, but also after system reset, use rom_blob_add_fixed() to 
> install
>     it into memory.

Ah ok.  So "-kernel" is not the problem.

> 
> So in igore-shared case, dtb is not required here ?

I don't know ARM much, so I think I'll just leave it to others (I'm
CCing Peter Maydell and Eric here since they're the ARM experts at
least I know of)...

> Maybe I could add a flag in struct Rom to set it when the rom is created by
> rom_add_blob_fixed_as()? And in rom_reset, just bypass this rom when
> in_incoming_migration && ignore_shared

Yeah it sounds better than the original proposal at least to me.  Now
rom_add_blob() has a "read_only" flag that is always equals to "true"
afaict.  Maybe you can make the dtb the first one with read_only=false
(the point where arm_load_dtb calls rom_add_blob_fixed_as but maybe
you can directly call rom_add_blob) and store it in struct Rom too.

Regards,

-- 
Peter Xu