qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V9 4/4] tests: Add migration test for aarch64


From: Dr. David Alan Gilbert
Subject: Re: [Qemu-devel] [PATCH V9 4/4] tests: Add migration test for aarch64
Date: Fri, 28 Sep 2018 15:04:31 +0100
User-agent: Mutt/1.10.1 (2018-07-13)

* Wei Huang (address@hidden) wrote:
> 
> 
> On 09/26/2018 11:31 AM, Dr. David Alan Gilbert wrote:
> > * Wei Huang (address@hidden) wrote:
> >> This patch adds migration test support for aarch64. The test code, which
> >> implements the same functionality as x86, is booted as a kernel in qemu.
> >> Here are the design choices we make for aarch64:
> >>
> >>  * We choose this -kernel approach because aarch64 QEMU doesn't provide a
> >>    built-in fw like x86 does. So instead of relying on a boot loader, we
> >>    use -kernel approach for aarch64.
> >>  * The serial output is sent to PL011 directly.
> >>  * The physical memory base for mach-virt machine is 0x40000000. We change
> >>    the start_address and end_address for aarch64.
> >>
> >> In addition to providing the binary, this patch also includes the source
> >> code and the build script in tests/migration/aarch64. So users can change
> >> the source and/or re-compile the binary as they wish.
> >>
> >> Reviewed-by: Juan Quintela <address@hidden>
> >> Signed-off-by: Wei Huang <address@hidden>
> > 
> > I've had to bounce this patch (kept the other 3) because the test
> > is actually failing on aarch64 kvm.
> > It's about 1 in 5 for me on one of the larger (40ish core) boxes which
> > is otherwise idle.
> > 
> > /aarch64/migration/precopy/unix: Memory content inconsistency at 43cf1000 
> > first_byte = 5 last_byte = 4 current = 5 hit_edge = 1
> > Memory content inconsistency at 43cf2000 first_byte = 5 last_byte = 4 
> > current = 5 hit_edge = 1
> > Memory content inconsistency at 43cf3000 first_byte = 5 last_byte = 4 
> > current = 5 hit_edge = 1
> > Memory content inconsistency at 43cf4000 first_byte = 5 last_byte = 4 
> > current = 5 hit_edge = 1
> > Memory content inconsistency at 43cf5000 first_byte = 5 last_byte = 4 
> > current = 5 hit_edge = 1
> > Memory content inconsistency at 43cf6000 first_byte = 5 last_byte = 4 
> > current = 5 hit_edge = 1
> > Memory content inconsistency at 43cf7000 first_byte = 5 last_byte = 4 
> > current = 5 hit_edge = 1
> 
> Thanks, Dave. The root cause is the wrong place of cache invalidation.
> Here is the fix which has been tested on Amberwing. Could you please
> verify it from your side? After that, I can either send in a new patch
> or you can help me revise the existing one.

Thanks, I'll grab a box and try it.
If it works I'd prefer if you can resubmit the patch with the fix in.

However, this fix looks suspicious to me.  We check the guest ram
contents after we stop the guest;  if the guests RAM is inconsistent as
seen by QEMU without the 'dc' then how will this work with normal
applications?

Shouldn't this be something qemu or kvm is doing when it stops the
guest?

Dave

> Thanks,
> -Wei
> 
> 
> diff --git a/tests/migration/aarch64/a-b-kernel.S
> b/tests/migration/aarch64/a-b-
> index 507af30..c8d2720 100644
> --- a/tests/migration/aarch64/a-b-kernel.S
> +++ b/tests/migration/aarch64/a-b-kernel.S
> @@ -50,15 +50,15 @@ mainloop:
>          mov     x4, x0
> 
>  innerloop:
> -        /* clean cache because el2 might still cache guest data under
> KVM */
> -        dc      civac, x4
> -
>          /* increment the first byte of each page by 1 */
>          ldrb    w3, [x4]
>          add     w3, w3, #1
>          and     w3, w3, #0xff
>          strb    w3, [x4]
> 
> +        /* clean cache because el2 might still cache guest data under
> KVM */
> +        dc      civac, x4
> +
>          add     x4, x4, #TEST_MEM_PAGE_SIZE
>          cmp     x4, x1
>          blt     innerloop
> 
> 
> > 
> > Dave
> > 
> >> ---
> >>  tests/Makefile.include               |  1 +
> >>  tests/migration-test.c               | 27 +++++++++++--
> >>  tests/migration/Makefile             |  2 +-
> >>  tests/migration/aarch64/Makefile     | 20 ++++++++++
> >>  tests/migration/aarch64/a-b-kernel.S | 75 
> >> ++++++++++++++++++++++++++++++++++++
> >>  tests/migration/aarch64/a-b-kernel.h | 19 +++++++++
> >>  tests/migration/migration-test.h     |  9 +++++
> >>  7 files changed, 148 insertions(+), 5 deletions(-)
> >>  create mode 100644 tests/migration/aarch64/Makefile
> >>  create mode 100644 tests/migration/aarch64/a-b-kernel.S
> >>  create mode 100644 tests/migration/aarch64/a-b-kernel.h
> >>
> >> diff --git a/tests/Makefile.include b/tests/Makefile.include
> >> index 87c81d1..fab8fb9 100644
> >> --- a/tests/Makefile.include
> >> +++ b/tests/Makefile.include
> >> @@ -390,6 +390,7 @@ check-qtest-arm-y += tests/hexloader-test$(EXESUF)
> >>  check-qtest-aarch64-y = tests/numa-test$(EXESUF)
> >>  check-qtest-aarch64-$(CONFIG_SDHCI) += tests/sdhci-test$(EXESUF)
> >>  check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
> >> +check-qtest-aarch64-y += tests/migration-test$(EXESUF)
> >>  
> >>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
> >>  
> >> diff --git a/tests/migration-test.c b/tests/migration-test.c
> >> index 17c6896..ecfae0b 100644
> >> --- a/tests/migration-test.c
> >> +++ b/tests/migration-test.c
> >> @@ -86,12 +86,13 @@ static const char *tmpfs;
> >>   * repeatedly. It outputs a 'B' at a fixed rate while it's still running.
> >>   */
> >>  #include "tests/migration/i386/a-b-bootblock.h"
> >> +#include "tests/migration/aarch64/a-b-kernel.h"
> >>  
> >> -static void init_bootfile_x86(const char *bootpath)
> >> +static void init_bootfile(const char *bootpath, void *content)
> >>  {
> >>      FILE *bootfile = fopen(bootpath, "wb");
> >>  
> >> -    g_assert_cmpint(fwrite(x86_bootsect, 512, 1, bootfile), ==, 1);
> >> +    g_assert_cmpint(fwrite(content, 512, 1, bootfile), ==, 1);
> >>      fclose(bootfile);
> >>  }
> >>  
> >> @@ -428,7 +429,7 @@ static int test_migrate_start(QTestState **from, 
> >> QTestState **to,
> >>      got_stop = false;
> >>  
> >>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> >> -        init_bootfile_x86(bootpath);
> >> +        init_bootfile(bootpath, x86_bootsect);
> >>          cmd_src = g_strdup_printf("-machine accel=%s -m 150M"
> >>                                    " -name source,debug-threads=on"
> >>                                    " -serial file:%s/src_serial"
> >> @@ -459,6 +460,24 @@ static int test_migrate_start(QTestState **from, 
> >> QTestState **to,
> >>  
> >>          start_address = PPC_TEST_MEM_START;
> >>          end_address = PPC_TEST_MEM_END;
> >> +    } else if (strcmp(arch, "aarch64") == 0) {
> >> +        init_bootfile(bootpath, aarch64_kernel);
> >> +        cmd_src = g_strdup_printf("-machine virt,accel=%s,gic-version=max 
> >> "
> >> +                                  "-name vmsource,debug-threads=on -cpu 
> >> max "
> >> +                                  "-m 150M -serial file:%s/src_serial "
> >> +                                  "-kernel %s ",
> >> +                                  accel, tmpfs, bootpath);
> >> +        cmd_dst = g_strdup_printf("-machine virt,accel=%s,gic-version=max 
> >> "
> >> +                                  "-name vmdest,debug-threads=on -cpu max 
> >> "
> >> +                                  "-m 150M -serial file:%s/dest_serial "
> >> +                                  "-kernel %s "
> >> +                                  "-incoming %s ",
> >> +                                  accel, tmpfs, bootpath, uri);
> >> +
> >> +        start_address = ARM_TEST_MEM_START;
> >> +        end_address = ARM_TEST_MEM_END;
> >> +
> >> +        g_assert(sizeof(aarch64_kernel) <= ARM_TEST_MAX_KERNEL_SIZE);
> >>      } else {
> >>          g_assert_not_reached();
> >>      }
> >> @@ -545,7 +564,7 @@ static void test_deprecated(void)
> >>  {
> >>      QTestState *from;
> >>  
> >> -    from = qtest_start("");
> >> +    from = qtest_start("-machine none");
> >>  
> >>      deprecated_set_downtime(from, 0.12345);
> >>      deprecated_set_speed(from, 12345);
> >> diff --git a/tests/migration/Makefile b/tests/migration/Makefile
> >> index a9ed875..13af934 100644
> >> --- a/tests/migration/Makefile
> >> +++ b/tests/migration/Makefile
> >> @@ -5,7 +5,7 @@
> >>  # See the COPYING file in the top-level directory.
> >>  #
> >>  
> >> -TARGET_LIST = i386
> >> +TARGET_LIST = i386 aarch64
> >>  
> >>  SRC_PATH = ../..
> >>  
> >> diff --git a/tests/migration/aarch64/Makefile 
> >> b/tests/migration/aarch64/Makefile
> >> new file mode 100644
> >> index 0000000..d440fa8
> >> --- /dev/null
> >> +++ b/tests/migration/aarch64/Makefile
> >> @@ -0,0 +1,20 @@
> >> +# To specify cross compiler prefix, use CROSS_PREFIX=
> >> +#   $ make CROSS_PREFIX=aarch64-linux-gnu-
> >> +
> >> +.PHONY: all clean
> >> +all: a-b-kernel.h
> >> +
> >> +a-b-kernel.h: aarch64.kernel
> >> +  echo "$$__note" > header.tmp
> >> +  xxd -i $< | sed -e 's/.*int.*//' >> header.tmp
> >> +  mv header.tmp $@
> >> +
> >> +aarch64.kernel: aarch64.elf
> >> +  $(CROSS_PREFIX)objcopy -O binary $< $@
> >> +
> >> +aarch64.elf: a-b-kernel.S
> >> +  $(CROSS_PREFIX)gcc -o $@ -nostdlib -Wl,--build-id=none $<
> >> +
> >> +clean:
> >> +  @rm -rf *.kernel *.elf
> >> +
> >> diff --git a/tests/migration/aarch64/a-b-kernel.S 
> >> b/tests/migration/aarch64/a-b-kernel.S
> >> new file mode 100644
> >> index 0000000..507af30
> >> --- /dev/null
> >> +++ b/tests/migration/aarch64/a-b-kernel.S
> >> @@ -0,0 +1,75 @@
> >> +#
> >> +# Copyright (c) 2018 Red Hat, Inc. and/or its affiliates
> >> +#
> >> +# Author:
> >> +#   Wei Huang <address@hidden>
> >> +#
> >> +# This work is licensed under the terms of the GNU GPL, version 2 or 
> >> later.
> >> +# See the COPYING file in the top-level directory.
> >> +#
> >> +# Note: Please make sure the compiler compiles the assembly code below 
> >> with
> >> +# pc-relative address. Also the branch instructions should use relative
> >> +# addresses only.
> >> +
> >> +#include "../migration-test.h"
> >> +
> >> +.section .text
> >> +
> >> +        .globl  _start
> >> +
> >> +_start:
> >> +        /* disable MMU to use phys mem address */
> >> +        mrs     x0, sctlr_el1
> >> +        bic     x0, x0, #(1<<0)
> >> +        msr     sctlr_el1, x0
> >> +        isb
> >> +
> >> +        /* traverse test memory region */
> >> +        mov     x0, #ARM_TEST_MEM_START
> >> +        mov     x1, #ARM_TEST_MEM_END
> >> +
> >> +        /* output char 'A' to PL011 */
> >> +        mov     w3, 'A'
> >> +        mov     x2, #ARM_MACH_VIRT_UART
> >> +        strb    w3, [x2]
> >> +
> >> +        /* clean up memory */
> >> +        mov     w3, #0
> >> +        mov     x4, x0
> >> +clean:
> >> +        strb    w3, [x4]
> >> +        add     x4, x4, #TEST_MEM_PAGE_SIZE
> >> +        cmp     x4, x1
> >> +        ble     clean
> >> +
> >> +        /* w5 keeps a counter so we can limit the output speed */
> >> +        mov     w5, #0
> >> +
> >> +        /* main body */
> >> +mainloop:
> >> +        mov     x4, x0
> >> +
> >> +innerloop:
> >> +        /* clean cache because el2 might still cache guest data under KVM 
> >> */
> >> +        dc      civac, x4
> >> +
> >> +        /* increment the first byte of each page by 1 */
> >> +        ldrb    w3, [x4]
> >> +        add     w3, w3, #1
> >> +        and     w3, w3, #0xff
> >> +        strb    w3, [x4]
> >> +
> >> +        add     x4, x4, #TEST_MEM_PAGE_SIZE
> >> +        cmp     x4, x1
> >> +        blt     innerloop
> >> +
> >> +        add     w5, w5, #1
> >> +        and     w5, w5, #0xff
> >> +        cmp     w5, #0
> >> +        bne     mainloop
> >> +
> >> +        /* output char 'B' to PL011 */
> >> +        mov     w3, 'B'
> >> +        strb    w3, [x2]
> >> +
> >> +        b       mainloop
> >> diff --git a/tests/migration/aarch64/a-b-kernel.h 
> >> b/tests/migration/aarch64/a-b-kernel.h
> >> new file mode 100644
> >> index 0000000..521125e
> >> --- /dev/null
> >> +++ b/tests/migration/aarch64/a-b-kernel.h
> >> @@ -0,0 +1,19 @@
> >> +/* This file is automatically generated from the assembly file in
> >> + * tests/migration/aarch64. Edit that file and then run "make all"
> >> + * inside tests/migration to update, and then remember to send both
> >> + * the header and the assembler differences in your patch submission.
> >> + */
> >> +unsigned char aarch64_kernel[] = {
> >> +  0x00, 0x10, 0x38, 0xd5, 0x00, 0xf8, 0x7f, 0x92, 0x00, 0x10, 0x18, 0xd5,
> >> +  0xdf, 0x3f, 0x03, 0xd5, 0x00, 0x02, 0xa8, 0xd2, 0x01, 0xc8, 0xa8, 0xd2,
> >> +  0x23, 0x08, 0x80, 0x52, 0x02, 0x20, 0xa1, 0xd2, 0x43, 0x00, 0x00, 0x39,
> >> +  0x03, 0x00, 0x80, 0x52, 0xe4, 0x03, 0x00, 0xaa, 0x83, 0x00, 0x00, 0x39,
> >> +  0x84, 0x04, 0x40, 0x91, 0x9f, 0x00, 0x01, 0xeb, 0xad, 0xff, 0xff, 0x54,
> >> +  0x05, 0x00, 0x80, 0x52, 0xe4, 0x03, 0x00, 0xaa, 0x24, 0x7e, 0x0b, 0xd5,
> >> +  0x83, 0x00, 0x40, 0x39, 0x63, 0x04, 0x00, 0x11, 0x63, 0x1c, 0x00, 0x12,
> >> +  0x83, 0x00, 0x00, 0x39, 0x84, 0x04, 0x40, 0x91, 0x9f, 0x00, 0x01, 0xeb,
> >> +  0x2b, 0xff, 0xff, 0x54, 0xa5, 0x04, 0x00, 0x11, 0xa5, 0x1c, 0x00, 0x12,
> >> +  0xbf, 0x00, 0x00, 0x71, 0x81, 0xfe, 0xff, 0x54, 0x43, 0x08, 0x80, 0x52,
> >> +  0x43, 0x00, 0x00, 0x39, 0xf1, 0xff, 0xff, 0x17
> >> +};
> >> +
> >> diff --git a/tests/migration/migration-test.h 
> >> b/tests/migration/migration-test.h
> >> index c4c0c52..6939a13 100644
> >> --- a/tests/migration/migration-test.h
> >> +++ b/tests/migration/migration-test.h
> >> @@ -18,4 +18,13 @@
> >>  #define PPC_TEST_MEM_START (1 * 1024 * 1024)
> >>  #define PPC_TEST_MEM_END   (100 * 1024 * 1024)
> >>  
> >> +/* ARM */
> >> +#define ARM_TEST_MEM_START (0x40000000 + 1 * 1024 * 1024)
> >> +#define ARM_TEST_MEM_END   (0x40000000 + 100 * 1024 * 1024)
> >> +#define ARM_MACH_VIRT_UART 0x09000000
> >> +/* AArch64 kernel load address is 0x40080000, and the test memory starts 
> >> at
> >> + * 0x40100000. So the maximum allowable kernel size is 512KB.
> >> + */
> >> +#define ARM_TEST_MAX_KERNEL_SIZE (512 * 1024)
> >> +
> >>  #endif /* _TEST_MIGRATION_H_ */
> >> -- 
> >> 1.8.3.1
> >>
> >>
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> > 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK



reply via email to

[Prev in Thread] Current Thread [Next in Thread]