[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V1 3/3] tests/qtest: live migration suspended state
|
From: |
Steven Sistare |
|
Subject: |
Re: [PATCH V1 3/3] tests/qtest: live migration suspended state |
|
Date: |
Wed, 21 Jun 2023 15:39:44 -0400 |
|
User-agent: |
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 |
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
>>
>