| Message ID | 1686860800-34667-4-git-send-email-steven.sistare@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | fix migration of suspended runstate | expand |
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.sistare@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: dgilbert@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? > + 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? > 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. Even if it triggers, "qmp_cont" skips for SUSPEND already, so I assume we're fine. > + } > + > + 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. > } > > 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, > + .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 >
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.sistare@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: dgilbert@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 >> >
On Wed, Jun 21, 2023 at 03:39:44PM -0400, Steven Sistare wrote: > >> - 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. Oh okay, can we use HIGH_ADDR-4, then? I just still think it'll be nice if we can keep HIGH_ADDR the high bar of the whole range. Thanks,
On 6/21/2023 4:00 PM, Peter Xu wrote: > On Wed, Jun 21, 2023 at 03:39:44PM -0400, Steven Sistare wrote: >>>> - 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. > > Oh okay, can we use HIGH_ADDR-4, then? I just still think it'll be nice if > we can keep HIGH_ADDR the high bar of the whole range. Sure. I'll use LOW_ADDR + 4, and add a comment. - Steve
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: dgilbert@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 + 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; 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'}"); + } + } + + if (args->start.suspend) { + qtest_qmp_assert_success(to, "{'execute': 'system_wakeup'}"); } 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, + .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",
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.sistare@oracle.com> --- 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(-)