hw/core/loader.c | 15 +++++++++++++++ include/exec/cpu-common.h | 1 + migration/ram.c | 2 +- 3 files changed, 17 insertions(+), 1 deletion(-)
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.
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
during rom_reset. In ignore-shared incoming case, this rom filling
is not required since all the data has been stored in memory backend file.
Fixes: commit 18269069c310 ("migration: Introduce ignore-shared capability")
Signed-off-by: Catherine Ho <catherine.hecx@gmail.com>
Suggested-by: Yury Kotov <yury-kotov@yandex-team.ru>
---
hw/core/loader.c | 15 +++++++++++++++
include/exec/cpu-common.h | 1 +
migration/ram.c | 2 +-
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index fe5cb24122..861a03335b 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -53,6 +53,7 @@
#include "hw/nvram/fw_cfg.h"
#include "exec/memory.h"
#include "exec/address-spaces.h"
+#include "exec/cpu-common.h"
#include "hw/boards.h"
#include "qemu/cutils.h"
@@ -1086,6 +1087,9 @@ int rom_add_option(const char *file, int32_t bootindex)
static void rom_reset(void *unused)
{
Rom *rom;
+ MemoryRegion *mr;
+ hwaddr hw_addr;
+ hwaddr l;
QTAILQ_FOREACH(rom, &roms, next) {
if (rom->fw_file) {
@@ -1094,6 +1098,17 @@ static void rom_reset(void *unused)
if (rom->data == NULL) {
continue;
}
+
+ /* bypass the rom blob in ignore-shared migration case*/
+ if (runstate_check(RUN_STATE_INMIGRATE)) {
+ rcu_read_lock();
+ mr = address_space_translate(rom->as, rom->addr, &hw_addr, &l,
+ true, MEMTXATTRS_UNSPECIFIED);
+ rcu_read_unlock();
+ if (mr->ram_block != NULL && ramblock_is_ignored(mr->ram_block))
+ continue;
+ }
+
if (rom->mr) {
void *host = memory_region_get_ram_ptr(rom->mr);
memcpy(host, rom->data, rom->datasize);
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index cef8b88a2a..c80b7248a6 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -76,6 +76,7 @@ void *qemu_ram_get_host_addr(RAMBlock *rb);
ram_addr_t qemu_ram_get_offset(RAMBlock *rb);
ram_addr_t qemu_ram_get_used_length(RAMBlock *rb);
bool qemu_ram_is_shared(RAMBlock *rb);
+bool ramblock_is_ignored(RAMBlock *block);
bool qemu_ram_is_uf_zeroable(RAMBlock *rb);
void qemu_ram_set_uf_zeroable(RAMBlock *rb);
bool qemu_ram_is_migratable(RAMBlock *rb);
diff --git a/migration/ram.c b/migration/ram.c
index 35bd6213e9..d6de9d335d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -159,7 +159,7 @@ out:
return ret;
}
-static bool ramblock_is_ignored(RAMBlock *block)
+bool ramblock_is_ignored(RAMBlock *block)
{
return !qemu_ram_is_migratable(block) ||
(migrate_ignore_shared() && qemu_ram_is_shared(block));
--
2.17.1
On Tue, Apr 02, 2019 at 11:30:01AM -0400, Catherine Ho 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. > > 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 > during rom_reset. In ignore-shared incoming case, this rom filling > is not required since all the data has been stored in memory backend file. > > Fixes: commit 18269069c310 ("migration: Introduce ignore-shared capability") > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com> > Suggested-by: Yury Kotov <yury-kotov@yandex-team.ru> (note: IIUC normally you should have your signed-off to be the last line before the suggested-by :) About the patch content, I have had a question on whether we should need to check ignore-shared at all... That question lies in: https://patchwork.kernel.org/patch/10859889/#22546487 And if my understanding was correct above, IMHO the patch could be as simply be as "if (runstate_check(RUN_STATE_INMIGRATE)) return;" at [1] below. Thanks, > --- > hw/core/loader.c | 15 +++++++++++++++ > include/exec/cpu-common.h | 1 + > migration/ram.c | 2 +- > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/hw/core/loader.c b/hw/core/loader.c > index fe5cb24122..861a03335b 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -53,6 +53,7 @@ > #include "hw/nvram/fw_cfg.h" > #include "exec/memory.h" > #include "exec/address-spaces.h" > +#include "exec/cpu-common.h" > #include "hw/boards.h" > #include "qemu/cutils.h" > > @@ -1086,6 +1087,9 @@ int rom_add_option(const char *file, int32_t bootindex) > static void rom_reset(void *unused) > { > Rom *rom; > + MemoryRegion *mr; > + hwaddr hw_addr; > + hwaddr l; [1] > > QTAILQ_FOREACH(rom, &roms, next) { > if (rom->fw_file) { > @@ -1094,6 +1098,17 @@ static void rom_reset(void *unused) > if (rom->data == NULL) { > continue; > } > + > + /* bypass the rom blob in ignore-shared migration case*/ > + if (runstate_check(RUN_STATE_INMIGRATE)) { > + rcu_read_lock(); > + mr = address_space_translate(rom->as, rom->addr, &hw_addr, &l, > + true, MEMTXATTRS_UNSPECIFIED); > + rcu_read_unlock(); > + if (mr->ram_block != NULL && ramblock_is_ignored(mr->ram_block)) > + continue; > + } > + > if (rom->mr) { > void *host = memory_region_get_ram_ptr(rom->mr); > memcpy(host, rom->data, rom->datasize); > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h > index cef8b88a2a..c80b7248a6 100644 > --- a/include/exec/cpu-common.h > +++ b/include/exec/cpu-common.h > @@ -76,6 +76,7 @@ void *qemu_ram_get_host_addr(RAMBlock *rb); > ram_addr_t qemu_ram_get_offset(RAMBlock *rb); > ram_addr_t qemu_ram_get_used_length(RAMBlock *rb); > bool qemu_ram_is_shared(RAMBlock *rb); > +bool ramblock_is_ignored(RAMBlock *block); > bool qemu_ram_is_uf_zeroable(RAMBlock *rb); > void qemu_ram_set_uf_zeroable(RAMBlock *rb); > bool qemu_ram_is_migratable(RAMBlock *rb); > diff --git a/migration/ram.c b/migration/ram.c > index 35bd6213e9..d6de9d335d 100644 > --- a/migration/ram.c > +++ b/migration/ram.c > @@ -159,7 +159,7 @@ out: > return ret; > } > > -static bool ramblock_is_ignored(RAMBlock *block) > +bool ramblock_is_ignored(RAMBlock *block) > { > return !qemu_ram_is_migratable(block) || > (migrate_ignore_shared() && qemu_ram_is_shared(block)); > -- > 2.17.1 > Regards, -- Peter Xu
Hi Peter Xu On Wed, 3 Apr 2019 at 10:25, Peter Xu <peterx@redhat.com> wrote: > On Tue, Apr 02, 2019 at 11:30:01AM -0400, Catherine Ho 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. > > > > 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 > > during rom_reset. In ignore-shared incoming case, this rom filling > > is not required since all the data has been stored in memory backend > file. > > > > Fixes: commit 18269069c310 ("migration: Introduce ignore-shared > capability") > > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com> > > Suggested-by: Yury Kotov <yury-kotov@yandex-team.ru> > > (note: IIUC normally you should have your signed-off to be the last > line before the suggested-by :) > > About the patch content, I have had a question on whether we should > need to check ignore-shared at all... That question lies in: > > https://patchwork.kernel.org/patch/10859889/#22546487 > > And if my understanding was correct above, IMHO the patch could be as > simply be as "if (runstate_check(RUN_STATE_INMIGRATE)) return;" at [1] > below. > > Thanks, but I thought this method would break the x86 rom_reset logic during RUN_STATE_INMIGRATE. Please see the debugging patch and log lines below: diff --git a/hw/core/loader.c b/hw/core/loader.c index fe5cb24122..b0c871af26 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -1086,8 +1086,9 @@ int rom_add_option(const char *file, int32_t bootindex) static void rom_reset(void *unused) { Rom *rom; - QTAILQ_FOREACH(rom, &roms, next) { + if (runstate_check(RUN_STATE_INMIGRATE)) + printf("rom name=%s\n",rom->name); if (rom->fw_file) { continue; } rom name=kvmvapic.bin rom name=linuxboot_dma.bin rom name=bios-256k.bin rom name=etc/acpi/tables rom name=etc/table-loader rom name=etc/acpi/rsdp B.R. Catherine > Thanks, > > > --- > > hw/core/loader.c | 15 +++++++++++++++ > > include/exec/cpu-common.h | 1 + > > migration/ram.c | 2 +- > > 3 files changed, 17 insertions(+), 1 deletion(-) > > > > diff --git a/hw/core/loader.c b/hw/core/loader.c > > index fe5cb24122..861a03335b 100644 > > --- a/hw/core/loader.c > > +++ b/hw/core/loader.c > > @@ -53,6 +53,7 @@ > > #include "hw/nvram/fw_cfg.h" > > #include "exec/memory.h" > > #include "exec/address-spaces.h" > > +#include "exec/cpu-common.h" > > #include "hw/boards.h" > > #include "qemu/cutils.h" > > > > @@ -1086,6 +1087,9 @@ int rom_add_option(const char *file, int32_t > bootindex) > > static void rom_reset(void *unused) > > { > > Rom *rom; > > + MemoryRegion *mr; > > + hwaddr hw_addr; > > + hwaddr l; > > [1] > > > > > QTAILQ_FOREACH(rom, &roms, next) { > > if (rom->fw_file) { > > @@ -1094,6 +1098,17 @@ static void rom_reset(void *unused) > > if (rom->data == NULL) { > > continue; > > } > > + > > + /* bypass the rom blob in ignore-shared migration case*/ > > + if (runstate_check(RUN_STATE_INMIGRATE)) { > > + rcu_read_lock(); > > + mr = address_space_translate(rom->as, rom->addr, &hw_addr, > &l, > > + true, MEMTXATTRS_UNSPECIFIED); > > + rcu_read_unlock(); > > + if (mr->ram_block != NULL && > ramblock_is_ignored(mr->ram_block)) > > + continue; > > + } > > + > > if (rom->mr) { > > void *host = memory_region_get_ram_ptr(rom->mr); > > memcpy(host, rom->data, rom->datasize); > > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h > > index cef8b88a2a..c80b7248a6 100644 > > --- a/include/exec/cpu-common.h > > +++ b/include/exec/cpu-common.h > > @@ -76,6 +76,7 @@ void *qemu_ram_get_host_addr(RAMBlock *rb); > > ram_addr_t qemu_ram_get_offset(RAMBlock *rb); > > ram_addr_t qemu_ram_get_used_length(RAMBlock *rb); > > bool qemu_ram_is_shared(RAMBlock *rb); > > +bool ramblock_is_ignored(RAMBlock *block); > > bool qemu_ram_is_uf_zeroable(RAMBlock *rb); > > void qemu_ram_set_uf_zeroable(RAMBlock *rb); > > bool qemu_ram_is_migratable(RAMBlock *rb); > > diff --git a/migration/ram.c b/migration/ram.c > > index 35bd6213e9..d6de9d335d 100644 > > --- a/migration/ram.c > > +++ b/migration/ram.c > > @@ -159,7 +159,7 @@ out: > > return ret; > > } > > > > -static bool ramblock_is_ignored(RAMBlock *block) > > +bool ramblock_is_ignored(RAMBlock *block) > > { > > return !qemu_ram_is_migratable(block) || > > (migrate_ignore_shared() && qemu_ram_is_shared(block)); > > -- > > 2.17.1 > > > > Regards, > > -- > Peter Xu >
On Wed, Apr 03, 2019 at 11:21:47PM +0800, Catherine Ho wrote: > Hi Peter Xu > > On Wed, 3 Apr 2019 at 10:25, Peter Xu <peterx@redhat.com> wrote: > > > On Tue, Apr 02, 2019 at 11:30:01AM -0400, Catherine Ho 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. > > > > > > 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 > > > during rom_reset. In ignore-shared incoming case, this rom filling > > > is not required since all the data has been stored in memory backend > > file. > > > > > > Fixes: commit 18269069c310 ("migration: Introduce ignore-shared > > capability") > > > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com> > > > Suggested-by: Yury Kotov <yury-kotov@yandex-team.ru> > > > > (note: IIUC normally you should have your signed-off to be the last > > line before the suggested-by :) > > > > About the patch content, I have had a question on whether we should > > need to check ignore-shared at all... That question lies in: > > > > https://patchwork.kernel.org/patch/10859889/#22546487 > > > > And if my understanding was correct above, IMHO the patch could be as > > simply be as "if (runstate_check(RUN_STATE_INMIGRATE)) return;" at [1] > > below. > > > > > Thanks, but I thought this method would break the x86 rom_reset logic during > RUN_STATE_INMIGRATE. > Please see the debugging patch and log lines below: > diff --git a/hw/core/loader.c b/hw/core/loader.c > index fe5cb24122..b0c871af26 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -1086,8 +1086,9 @@ int rom_add_option(const char *file, int32_t > bootindex) > static void rom_reset(void *unused) > { > Rom *rom; > - > QTAILQ_FOREACH(rom, &roms, next) { > + if (runstate_check(RUN_STATE_INMIGRATE)) > + printf("rom name=%s\n",rom->name); > if (rom->fw_file) { > continue; > } > > rom name=kvmvapic.bin > rom name=linuxboot_dma.bin > rom name=bios-256k.bin > rom name=etc/acpi/tables > rom name=etc/table-loader > rom name=etc/acpi/rsdp Hi, Catherine, I only see that rom names were dumped. Could you help explain what is broken? Thanks, -- Peter Xu
Hi Peter Xu On Thu, 4 Apr 2019 at 12:25, Peter Xu <peterx@redhat.com> wrote: > On Wed, Apr 03, 2019 at 11:21:47PM +0800, Catherine Ho wrote: > > Hi Peter Xu > > > > On Wed, 3 Apr 2019 at 10:25, Peter Xu <peterx@redhat.com> wrote: > > > > > On Tue, Apr 02, 2019 at 11:30:01AM -0400, Catherine Ho 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. > > > > > > > > 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 > > > > during rom_reset. In ignore-shared incoming case, this rom filling > > > > is not required since all the data has been stored in memory backend > > > file. > > > > > > > > Fixes: commit 18269069c310 ("migration: Introduce ignore-shared > > > capability") > > > > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com> > > > > Suggested-by: Yury Kotov <yury-kotov@yandex-team.ru> > > > > > > (note: IIUC normally you should have your signed-off to be the last > > > line before the suggested-by :) > > > > > > About the patch content, I have had a question on whether we should > > > need to check ignore-shared at all... That question lies in: > > > > > > https://patchwork.kernel.org/patch/10859889/#22546487 > > > > > > And if my understanding was correct above, IMHO the patch could be as > > > simply be as "if (runstate_check(RUN_STATE_INMIGRATE)) return;" at [1] > > > below. > > > > > > > > Thanks, but I thought this method would break the x86 rom_reset logic > during > > RUN_STATE_INMIGRATE. > > Please see the debugging patch and log lines below: > > diff --git a/hw/core/loader.c b/hw/core/loader.c > > index fe5cb24122..b0c871af26 100644 > > --- a/hw/core/loader.c > > +++ b/hw/core/loader.c > > @@ -1086,8 +1086,9 @@ int rom_add_option(const char *file, int32_t > > bootindex) > > static void rom_reset(void *unused) > > { > > Rom *rom; > > - > > QTAILQ_FOREACH(rom, &roms, next) { > > + if (runstate_check(RUN_STATE_INMIGRATE)) > > + printf("rom name=%s\n",rom->name); > > if (rom->fw_file) { > > continue; > > } > > > > rom name=kvmvapic.bin > > rom name=linuxboot_dma.bin > > rom name=bios-256k.bin > > rom name=etc/acpi/tables > > rom name=etc/table-loader > > rom name=etc/acpi/rsdp > > Hi, Catherine, > > I only see that rom names were dumped. Could you help explain what is > broken? Thanks, > Thanks for the suggestion. I haven't seen any obvious errors on x86 with this change. I merely consider not to change the old code logic too much. Ok, I will change it as you suggested if no more comments. B.R. Catherine > > -- > Peter Xu >
On Thu, Apr 04, 2019 at 03:17:41PM +0800, Catherine Ho wrote: > Hi Peter Xu > > On Thu, 4 Apr 2019 at 12:25, Peter Xu <peterx@redhat.com> wrote: > > > On Wed, Apr 03, 2019 at 11:21:47PM +0800, Catherine Ho wrote: > > > Hi Peter Xu > > > > > > On Wed, 3 Apr 2019 at 10:25, Peter Xu <peterx@redhat.com> wrote: > > > > > > > On Tue, Apr 02, 2019 at 11:30:01AM -0400, Catherine Ho 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. > > > > > > > > > > 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 > > > > > during rom_reset. In ignore-shared incoming case, this rom filling > > > > > is not required since all the data has been stored in memory backend > > > > file. > > > > > > > > > > Fixes: commit 18269069c310 ("migration: Introduce ignore-shared > > > > capability") > > > > > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com> > > > > > Suggested-by: Yury Kotov <yury-kotov@yandex-team.ru> > > > > > > > > (note: IIUC normally you should have your signed-off to be the last > > > > line before the suggested-by :) > > > > > > > > About the patch content, I have had a question on whether we should > > > > need to check ignore-shared at all... That question lies in: > > > > > > > > https://patchwork.kernel.org/patch/10859889/#22546487 > > > > > > > > And if my understanding was correct above, IMHO the patch could be as > > > > simply be as "if (runstate_check(RUN_STATE_INMIGRATE)) return;" at [1] > > > > below. > > > > > > > > > > > Thanks, but I thought this method would break the x86 rom_reset logic > > during > > > RUN_STATE_INMIGRATE. > > > Please see the debugging patch and log lines below: > > > diff --git a/hw/core/loader.c b/hw/core/loader.c > > > index fe5cb24122..b0c871af26 100644 > > > --- a/hw/core/loader.c > > > +++ b/hw/core/loader.c > > > @@ -1086,8 +1086,9 @@ int rom_add_option(const char *file, int32_t > > > bootindex) > > > static void rom_reset(void *unused) > > > { > > > Rom *rom; > > > - > > > QTAILQ_FOREACH(rom, &roms, next) { > > > + if (runstate_check(RUN_STATE_INMIGRATE)) > > > + printf("rom name=%s\n",rom->name); > > > if (rom->fw_file) { > > > continue; > > > } > > > > > > rom name=kvmvapic.bin > > > rom name=linuxboot_dma.bin > > > rom name=bios-256k.bin > > > rom name=etc/acpi/tables > > > rom name=etc/table-loader > > > rom name=etc/acpi/rsdp > > > > Hi, Catherine, > > > > I only see that rom names were dumped. Could you help explain what is > > broken? Thanks, > > > Thanks for the suggestion. I haven't seen any obvious errors on x86 with > this change. > I merely consider not to change the old code logic too much. > Ok, I will change it as you suggested if no more comments. Yeah let's see whether others would have any opinion on that since so far I don't see a problem (after all those ROM data should be migrated from source later on no matter whether they were modified on source). Regards, -- Peter Xu
Hi Peter Xu On Thu, 4 Apr 2019 at 12:25, Peter Xu <peterx@redhat.com> wrote: > On Wed, Apr 03, 2019 at 11:21:47PM +0800, Catherine Ho wrote: > > Hi Peter Xu > > > > On Wed, 3 Apr 2019 at 10:25, Peter Xu <peterx@redhat.com> wrote: > > > > > On Tue, Apr 02, 2019 at 11:30:01AM -0400, Catherine Ho 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. > > > > > > > > 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 > > > > during rom_reset. In ignore-shared incoming case, this rom filling > > > > is not required since all the data has been stored in memory backend > > > file. > > > > > > > > Fixes: commit 18269069c310 ("migration: Introduce ignore-shared > > > capability") > > > > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com> > > > > Suggested-by: Yury Kotov <yury-kotov@yandex-team.ru> > > > > > > (note: IIUC normally you should have your signed-off to be the last > > > line before the suggested-by :) > > > > > > About the patch content, I have had a question on whether we should > > > need to check ignore-shared at all... That question lies in: > > > > > > https://patchwork.kernel.org/patch/10859889/#22546487 > > > > > > And if my understanding was correct above, IMHO the patch could be as > > > simply be as "if (runstate_check(RUN_STATE_INMIGRATE)) return;" at [1] > > > below. > > > > > > > > Thanks, but I thought this method would break the x86 rom_reset logic > during > > RUN_STATE_INMIGRATE. > > Please see the debugging patch and log lines below: > > diff --git a/hw/core/loader.c b/hw/core/loader.c > > index fe5cb24122..b0c871af26 100644 > > --- a/hw/core/loader.c > > +++ b/hw/core/loader.c > > @@ -1086,8 +1086,9 @@ int rom_add_option(const char *file, int32_t > > bootindex) > > static void rom_reset(void *unused) > > { > > Rom *rom; > > - > > QTAILQ_FOREACH(rom, &roms, next) { > > + if (runstate_check(RUN_STATE_INMIGRATE)) > > + printf("rom name=%s\n",rom->name); > > if (rom->fw_file) { > > continue; > > } > > > > rom name=kvmvapic.bin > > rom name=linuxboot_dma.bin > > rom name=bios-256k.bin > > rom name=etc/acpi/tables > > rom name=etc/table-loader > > rom name=etc/acpi/rsdp > > Hi, Catherine, > > I only see that rom names were dumped. Could you help explain what is > broken? Thanks, > > Sorry, I have another concern here. What if there is no memory_backend file? If there is no memory backend file (i.e. without -object memory-backend-file,id=dimm1,size=512M,mem-path=/path/memory) Should the rom blobs still be written into ram in such case? B.R. Catherine > -- > Peter Xu >
On Thu, Apr 04, 2019 at 03:33:20PM +0800, Catherine Ho wrote: > Hi Peter Xu > > On Thu, 4 Apr 2019 at 12:25, Peter Xu <peterx@redhat.com> wrote: > > > On Wed, Apr 03, 2019 at 11:21:47PM +0800, Catherine Ho wrote: > > > Hi Peter Xu > > > > > > On Wed, 3 Apr 2019 at 10:25, Peter Xu <peterx@redhat.com> wrote: > > > > > > > On Tue, Apr 02, 2019 at 11:30:01AM -0400, Catherine Ho 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. > > > > > > > > > > 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 > > > > > during rom_reset. In ignore-shared incoming case, this rom filling > > > > > is not required since all the data has been stored in memory backend > > > > file. > > > > > > > > > > Fixes: commit 18269069c310 ("migration: Introduce ignore-shared > > > > capability") > > > > > Signed-off-by: Catherine Ho <catherine.hecx@gmail.com> > > > > > Suggested-by: Yury Kotov <yury-kotov@yandex-team.ru> > > > > > > > > (note: IIUC normally you should have your signed-off to be the last > > > > line before the suggested-by :) > > > > > > > > About the patch content, I have had a question on whether we should > > > > need to check ignore-shared at all... That question lies in: > > > > > > > > https://patchwork.kernel.org/patch/10859889/#22546487 > > > > > > > > And if my understanding was correct above, IMHO the patch could be as > > > > simply be as "if (runstate_check(RUN_STATE_INMIGRATE)) return;" at [1] > > > > below. > > > > > > > > > > > Thanks, but I thought this method would break the x86 rom_reset logic > > during > > > RUN_STATE_INMIGRATE. > > > Please see the debugging patch and log lines below: > > > diff --git a/hw/core/loader.c b/hw/core/loader.c > > > index fe5cb24122..b0c871af26 100644 > > > --- a/hw/core/loader.c > > > +++ b/hw/core/loader.c > > > @@ -1086,8 +1086,9 @@ int rom_add_option(const char *file, int32_t > > > bootindex) > > > static void rom_reset(void *unused) > > > { > > > Rom *rom; > > > - > > > QTAILQ_FOREACH(rom, &roms, next) { > > > + if (runstate_check(RUN_STATE_INMIGRATE)) > > > + printf("rom name=%s\n",rom->name); > > > if (rom->fw_file) { > > > continue; > > > } > > > > > > rom name=kvmvapic.bin > > > rom name=linuxboot_dma.bin > > > rom name=bios-256k.bin > > > rom name=etc/acpi/tables > > > rom name=etc/table-loader > > > rom name=etc/acpi/rsdp > > > > Hi, Catherine, > > > > I only see that rom names were dumped. Could you help explain what is > > broken? Thanks, > > > > Sorry, I have another concern here. What if there is no memory_backend > file? > If there is no memory backend file (i.e. without -object > memory-backend-file,id=dimm1,size=512M,mem-path=/path/memory) > > Should the rom blobs still be written into ram in such case? Please see my previous reply - I think it shouldn't be needed because we should migrate very soon after this point for those RAM. In other words, IIUC even 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. Regards, -- Peter Xu
© 2016 - 2024 Red Hat, Inc.