[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V1 3/3] tests/qtest: live migration suspended state
|
From: |
Peter Xu |
|
Subject: |
Re: [PATCH V1 3/3] tests/qtest: live migration suspended state |
|
Date: |
Wed, 21 Jun 2023 12:45:09 -0400 |
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
>
--
Peter Xu