On 6/21/2023 12:45 PM, Peter Xu wrote: > On Thu, Jun 15, 2023 at 01:26:40PM -0700, Steve Sistare wrote: >> Add a test case to verify that the suspended state is handled correctly in >> live migration. The test suspends the src, migrates, then wakes the dest. >> >> Add an option to suspend the src in a-b-bootblock.S, which puts the guest >> in S3 state after one round of writing to memory. The option is enabled by >> poking a 1 into the suspend_me word in the boot block prior to starting the >> src vm. Generate symbol offsets in a-b-bootblock.h so that the suspend_me >> offset is known. >> >> Signed-off-by: Steve Sistare <steven.sist...@oracle.com> > > Thanks for the test case, mostly good to me, a few trivial comments / > questions below. > >> --- >> tests/migration/i386/Makefile | 5 ++-- >> tests/migration/i386/a-b-bootblock.S | 49 >> +++++++++++++++++++++++++++++++++--- >> tests/migration/i386/a-b-bootblock.h | 22 ++++++++++------ >> tests/qtest/migration-helpers.c | 2 +- >> tests/qtest/migration-test.c | 31 +++++++++++++++++++++-- >> 5 files changed, 92 insertions(+), 17 deletions(-) >> >> diff --git a/tests/migration/i386/Makefile b/tests/migration/i386/Makefile >> index 5c03241..37a72ae 100644 >> --- a/tests/migration/i386/Makefile >> +++ b/tests/migration/i386/Makefile >> @@ -4,9 +4,10 @@ >> .PHONY: all clean >> all: a-b-bootblock.h >> >> -a-b-bootblock.h: x86.bootsect >> +a-b-bootblock.h: x86.bootsect x86.o >> echo "$$__note" > header.tmp >> xxd -i $< | sed -e 's/.*int.*//' >> header.tmp >> + nm x86.o | awk '{print "#define SYM_"$$3" 0x"$$1}' >> header.tmp >> mv header.tmp $@ >> >> x86.bootsect: x86.boot >> @@ -16,7 +17,7 @@ x86.boot: x86.o >> $(CROSS_PREFIX)objcopy -O binary $< $@ >> >> x86.o: a-b-bootblock.S >> - $(CROSS_PREFIX)gcc -m32 -march=i486 -c $< -o $@ >> + $(CROSS_PREFIX)gcc -I.. -m32 -march=i486 -c $< -o $@ >> >> clean: >> @rm -rf *.boot *.o *.bootsect >> diff --git a/tests/migration/i386/a-b-bootblock.S >> b/tests/migration/i386/a-b-bootblock.S >> index 3d464c7..63d446f 100644 >> --- a/tests/migration/i386/a-b-bootblock.S >> +++ b/tests/migration/i386/a-b-bootblock.S >> @@ -9,6 +9,21 @@ >> # >> # Author: dgilb...@redhat.com >> >> +#include "migration-test.h" >> + >> +#define ACPI_ENABLE 0xf1 >> +#define ACPI_PORT_SMI_CMD 0xb2 >> +#define ACPI_PM_BASE 0x600 >> +#define PM1A_CNT_OFFSET 4 >> + >> +#define ACPI_SCI_ENABLE 0x0001 >> +#define ACPI_SLEEP_TYPE 0x0400 >> +#define ACPI_SLEEP_ENABLE 0x2000 >> +#define SLEEP (ACPI_SCI_ENABLE + ACPI_SLEEP_TYPE + ACPI_SLEEP_ENABLE) >> + >> +#define LOW_ADDR X86_TEST_MEM_START >> +#define HIGH_ADDR X86_TEST_MEM_END >> +#define suspended HIGH_ADDR >> >> .code16 >> .org 0x7c00 >> @@ -41,12 +56,11 @@ start: # at 0x7c00 ? >> # bl keeps a counter so we limit the output speed >> mov $0, %bl >> mainloop: >> - # Start from 1MB >> - mov $(1024*1024),%eax >> + mov $LOW_ADDR,%eax >> innerloop: >> incb (%eax) >> add $4096,%eax >> - cmp $(100*1024*1024),%eax >> + cmp $HIGH_ADDR,%eax >> jl innerloop >> >> inc %bl >> @@ -57,7 +71,30 @@ innerloop: >> mov $0x3f8,%dx >> outb %al,%dx >> >> - jmp mainloop >> + # should this test suspend? >> + mov (suspend_me),%eax >> + cmp $0,%eax >> + je mainloop >> + >> + # are we waking after suspend? do not suspend again. >> + mov $suspended,%eax > > So IIUC then it'll use 4 bytes over 100MB range which means we need at > least 100MB+4bytes.. not obvious for a HIGH_ADDR definition to me.. > > Could we just define a variable inside the section like suspend_me?
No, because modifications to this memory backing the boot block are not copied to the destination. The dest reads a clean copy of the boot block from disk, as specified by the qemu command line arguments. >> + mov (%eax),%eax >> + cmp $1,%eax >> + je mainloop >> + >> + # enable acpi >> + mov $ACPI_ENABLE,%al >> + outb %al,$ACPI_PORT_SMI_CMD >> + >> + # suspend to ram >> + mov $suspended,%eax >> + movl $1,(%eax) >> + mov $SLEEP,%ax >> + mov $(ACPI_PM_BASE + PM1A_CNT_OFFSET),%dx >> + outw %ax,%dx >> + # not reached. The wakeup causes reset and restart at 0x7c00, and >> we >> + # do not save and restore registers as a real kernel would do. >> + >> >> # GDT magic from old (GPLv2) Grub startup.S >> .p2align 2 /* force 4-byte alignment */ >> @@ -83,6 +120,10 @@ gdtdesc: >> .word 0x27 /* limit */ >> .long gdt /* addr */ >> >> + /* test launcher can poke a 1 here to exercise suspend */ >> +suspend_me: >> + .int 0 >> + >> /* I'm a bootable disk */ >> .org 0x7dfe >> .byte 0x55 >> diff --git a/tests/migration/i386/a-b-bootblock.h >> b/tests/migration/i386/a-b-bootblock.h >> index b7b0fce..b39773f 100644 >> --- a/tests/migration/i386/a-b-bootblock.h >> +++ b/tests/migration/i386/a-b-bootblock.h >> @@ -4,20 +4,20 @@ >> * the header and the assembler differences in your patch submission. >> */ >> unsigned char x86_bootsect[] = { >> - 0xfa, 0x0f, 0x01, 0x16, 0x78, 0x7c, 0x66, 0xb8, 0x01, 0x00, 0x00, 0x00, >> + 0xfa, 0x0f, 0x01, 0x16, 0xa4, 0x7c, 0x66, 0xb8, 0x01, 0x00, 0x00, 0x00, >> 0x0f, 0x22, 0xc0, 0x66, 0xea, 0x20, 0x7c, 0x00, 0x00, 0x08, 0x00, 0x00, >> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe4, 0x92, 0x0c, 0x02, >> 0xe6, 0x92, 0xb8, 0x10, 0x00, 0x00, 0x00, 0x8e, 0xd8, 0x66, 0xb8, 0x41, >> 0x00, 0x66, 0xba, 0xf8, 0x03, 0xee, 0xb3, 0x00, 0xb8, 0x00, 0x00, 0x10, >> 0x00, 0xfe, 0x00, 0x05, 0x00, 0x10, 0x00, 0x00, 0x3d, 0x00, 0x00, 0x40, >> 0x06, 0x7c, 0xf2, 0xfe, 0xc3, 0x80, 0xe3, 0x3f, 0x75, 0xe6, 0x66, 0xb8, >> - 0x42, 0x00, 0x66, 0xba, 0xf8, 0x03, 0xee, 0xeb, 0xdb, 0x8d, 0x76, 0x00, >> - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0x00, 0x00, >> - 0x00, 0x9a, 0xcf, 0x00, 0xff, 0xff, 0x00, 0x00, 0x00, 0x92, 0xcf, 0x00, >> - 0x27, 0x00, 0x60, 0x7c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> + 0x42, 0x00, 0x66, 0xba, 0xf8, 0x03, 0xee, 0xa1, 0xaa, 0x7c, 0x00, 0x00, >> + 0x83, 0xf8, 0x00, 0x74, 0xd3, 0xb8, 0x00, 0x00, 0x40, 0x06, 0x8b, 0x00, >> + 0x83, 0xf8, 0x01, 0x74, 0xc7, 0xb0, 0xf1, 0xe6, 0xb2, 0xb8, 0x00, 0x00, >> + 0x40, 0x06, 0xc7, 0x00, 0x01, 0x00, 0x00, 0x00, 0x66, 0xb8, 0x01, 0x24, >> + 0x66, 0xba, 0x04, 0x06, 0x66, 0xef, 0x66, 0x90, 0x00, 0x00, 0x00, 0x00, >> + 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0x00, 0x00, 0x00, 0x9a, 0xcf, 0x00, >> + 0xff, 0xff, 0x00, 0x00, 0x00, 0x92, 0xcf, 0x00, 0x27, 0x00, 0x8c, 0x7c, >> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, >> @@ -49,3 +49,9 @@ unsigned char x86_bootsect[] = { >> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x55, 0xaa >> }; >> >> +#define SYM_gdt 0x00007c8c >> +#define SYM_gdtdesc 0x00007ca4 >> +#define SYM_innerloop 0x00007c3d >> +#define SYM_mainloop 0x00007c38 >> +#define SYM_start 0x00007c00 >> +#define SYM_suspend_me 0x00007caa >> diff --git a/tests/qtest/migration-helpers.c >> b/tests/qtest/migration-helpers.c >> index be00c52..433d678 100644 >> --- a/tests/qtest/migration-helpers.c >> +++ b/tests/qtest/migration-helpers.c >> @@ -28,7 +28,7 @@ bool migrate_watch_for_stop(QTestState *who, const char >> *name, >> { >> bool *seen = opaque; >> >> - if (g_str_equal(name, "STOP")) { >> + if (g_str_equal(name, "STOP") || g_str_equal(name, "SUSPEND")) { >> *seen = true; > > This is also a bit hachish.. "*seen" points to got_src_stop, so when > SUSPEND it'll set got_src_stop. It's not clear what stage we're in even if > that's set, irrelevant of the confusing naming after being able to SUSPEND. > > Shall we just add one got_src_suspended here and explicitly check that when > suspend=true in test? OK. I will move got_src_stop and got_src_suspend to the QTestState, and pass the QTestState as the opaque pointer. Ditto for got_dst_resume for consistency. Then we can delete a few globals as a bonus. >> return true; >> } >> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c >> index b0c355b..73b07b3 100644 >> --- a/tests/qtest/migration-test.c >> +++ b/tests/qtest/migration-test.c >> @@ -121,7 +121,7 @@ static void init_bootfile(const char *bootpath, void >> *content, size_t len) >> /* >> * Wait for some output in the serial output file, >> * we get an 'A' followed by an endless string of 'B's >> - * but on the destination we won't have the A. >> + * but on the destination we won't have the A (unless we enabled >> suspend/resume) >> */ >> static void wait_for_serial(const char *side) >> { >> @@ -507,6 +507,8 @@ typedef struct { >> bool use_dirty_ring; >> const char *opts_source; >> const char *opts_target; >> + /* suspend the src before migrating to dest. */ >> + bool suspend; >> } MigrateStart; >> >> /* >> @@ -617,6 +619,8 @@ static int test_migrate_start(QTestState **from, >> QTestState **to, >> } >> } >> >> + x86_bootsect[SYM_suspend_me - SYM_start] = args->suspend; >> + >> got_src_stop = false; >> got_dst_resume = false; >> bootpath = g_strdup_printf("%s/bootsect", tmpfs); >> @@ -1475,7 +1479,13 @@ static void test_precopy_common(MigrateCommon *args) >> */ >> wait_for_migration_complete(to); >> >> - qtest_qmp_assert_success(to, "{ 'execute' : 'cont'}"); >> + if (!args->start.suspend) { >> + qtest_qmp_assert_success(to, "{ 'execute' : 'cont'}"); >> + } > > This is live==false path, if this test needs live=true then afaict this > path won't ever trigger. I verified that live==false works, but did not add a test case for it. > Even if it triggers, "qmp_cont" skips for SUSPEND already, so I assume > we're fine. OK, I can delete the check for that reason. >> + } >> + >> + if (args->start.suspend) { >> + qtest_qmp_assert_success(to, "{'execute': 'system_wakeup'}"); > > Okay I did look up qmp_system_wakeup and I think it implicitly checks the > SUSPEND status, which is reasonable but not obvious. Not sure whether > that's intended. > > Shall we just check it explicitly with a query-status, even if so? > > If keeping relying on qmp_system_wakeup not failing, I suggest we add a > comment explaining that this explicitly checks machine state so we're sure > the SUSPEND state is migrated properly. I'll add a comment. I intended it this way, because it works, simply. >> } >> >> if (!got_dst_resume) { >> @@ -1508,6 +1518,18 @@ static void test_precopy_unix_plain(void) >> test_precopy_common(&args); >> } >> >> +static void test_precopy_unix_suspend(void) >> +{ >> + g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs); >> + MigrateCommon args = { >> + .listen_uri = uri, >> + .connect_uri = uri, >> + .live = true, > > We need explicit comment for all .live=true cases to state why. Please > refer to: > > /* > * Optional: whether the guest CPUs should be running during a precopy > * migration test. We used to always run with live but it took much > * longer so we reduced live tests to only the ones that have solid > * reason to be tested live-only. For each of the new test cases for > * precopy please provide justifications to use live explicitly (please > * refer to existing ones with live=true), or use live=off by default. > */ > bool live; > > Thanks, OK, I'll add a comment. Live runs as fast as non-live for this new test case because the source is not re-dirtying memory. - Steve > >> + .start.suspend = true, >> + }; >> + >> + test_precopy_common(&args); >> +} >> >> static void test_precopy_unix_dirty_ring(void) >> { >> @@ -2682,6 +2704,11 @@ int main(int argc, char **argv) >> >> module_call_init(MODULE_INIT_QOM); >> >> + if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) { >> + qtest_add_func("/migration/precopy/unix/suspend", >> + test_precopy_unix_suspend); >> + } >> + >> if (has_uffd) { >> qtest_add_func("/migration/postcopy/plain", test_postcopy); >> qtest_add_func("/migration/postcopy/recovery/plain", >> -- >> 1.8.3.1 >> >