Re: [PATCH V1 3/3] tests/qtest: live migration suspended state

[Peter Xu <peterx@redhat.com>]

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