Re: [Qemu-devel] [PATCH v4] migration: do not rom_reset() during incoming migration

[Paolo Bonzini <pbonzini@redhat.com>]

On 14/08/19 12:40, Catherine Ho wrote:
> Hi Paolo
> Ping, is any other comment I hadn't addressed?

No, I queued the patch now.

Paolo

> Cheers
> Catherine
> 
> On Thu, 6 Jun 2019 at 02:31, Dr. David Alan Gilbert <dgilbert@redhat.com
> <mailto:dgilbert@redhat.com>> wrote:
> 
>     Paolo, can you take this one please.
> 
>     * Catherine Ho (catherine.hecx@gmail.com
>     <mailto:catherine.hecx@gmail.com>) wrote:
>     > Commit 18269069c310 ("migration: Introduce ignore-shared capability")
>     > addes ignore-shared capability to bypass the shared ramblock (e,g,
>     > membackend + numa node). It does good to live migration.
>     >
>     > As told by Yury,this commit expectes 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
>     >
>     > Actually, on arm64 virt marchine, ramblock "dtb" will be filled
>     into ram
>     > druing rom_reset. In ignore-shared incoming case, this rom filling
>     > is not required since all the data has been stored in memory backend
>     > file.
>     >
>     > Further more, as suggested by Peter Xu, if we do rom_reset() now with
>     > these ROMs then the RAM data should be re-filled again too with the
>     > migration stream coming in.
>     >
>     > Fixes: commit 18269069c310 ("migration: Introduce ignore-shared
>     > capability")
>     > Suggested-by: Yury Kotov <yury-kotov@yandex-team.ru
>     <mailto:yury-kotov@yandex-team.ru>>
>     > Suggested-by: Peter Xu <peterx@redhat.com <mailto:peterx@redhat.com>>
>     > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com
>     <mailto:catherine.hecx@gmail.com>>
>     > ---
>     >  hw/core/loader.c | 9 +++++++++
>     >  1 file changed, 9 insertions(+)
>     >
>     > diff --git a/hw/core/loader.c b/hw/core/loader.c
>     > index fe5cb24122..040109464b 100644
>     > --- a/hw/core/loader.c
>     > +++ b/hw/core/loader.c
>     > @@ -1087,6 +1087,15 @@ static void rom_reset(void *unused)
>     >  {
>     >      Rom *rom;
>     > 
>     > +    /*
>     > +     * We don't need to fill in the RAM with ROM data because
>     we'll fill
>     > +     * the data in during the next incoming migration in all
>     cases.  Note
>     > +     * that some of those RAMs can actually be modified by the
>     guest on ARM
>     > +     * so this is probably the only right thing to do here.
>     > +     */
>     > +    if (runstate_check(RUN_STATE_INMIGRATE))
>     > +        return;
>     > +
>     >      QTAILQ_FOREACH(rom, &roms, next) {
>     >          if (rom->fw_file) {
>     >              continue;
>     > --
>     > 2.17.1
>     >
>     --
>     Dr. David Alan Gilbert / dgilbert@redhat.com
>     <mailto:dgilbert@redhat.com> / Manchester, UK
>