Re: [Qemu-devel] [PATCH V2 1/1] tests: Add migration test for aarch64

[Wei Huang]

On 02/12/2018 11:31 AM, Andrew Jones wrote:
> On Fri, Feb 09, 2018 at 04:42:42PM -0500, Wei Huang 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 test source
>> and the build script in tests/migration. So users can change/re-compile
>> the binary as they wish.
>>
>> Signed-off-by: Wei Huang <address@hidden>
>> ---
>>  tests/Makefile.include                    |  1 +
>>  tests/migration-test.c                    | 29 ++++++++++---
>>  tests/migration/aarch64-a-b-kernel.h      | 19 +++++++++
>>  tests/migration/aarch64-a-b-kernel.s      | 67 
>> +++++++++++++++++++++++++++++++
>>  tests/migration/rebuild-aarch64-kernel.sh | 67 
>> +++++++++++++++++++++++++++++++
>>  5 files changed, 177 insertions(+), 6 deletions(-)
>>  create mode 100644 tests/migration/aarch64-a-b-kernel.h
>>  create mode 100644 tests/migration/aarch64-a-b-kernel.s
>>  create mode 100755 tests/migration/rebuild-aarch64-kernel.sh
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index f41da23..0fd18fd 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -369,6 +369,7 @@ gcov-files-arm-y += hw/timer/arm_mptimer.c
>>  check-qtest-arm-y += tests/boot-serial-test$(EXESUF)
>>  
>>  check-qtest-aarch64-y = tests/numa-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 85d4014..b16944c 100644
>> --- a/tests/migration-test.c
>> +++ b/tests/migration-test.c
>> @@ -22,8 +22,8 @@
>>  
>>  #define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
>>  
>> -const unsigned start_address = 1024 * 1024;
>> -const unsigned end_address = 100 * 1024 * 1024;
>> +unsigned start_address = 1024 * 1024;
>> +unsigned end_address = 100 * 1024 * 1024;
>>  bool got_stop;
>>  
>>  #if defined(__linux__)
>> @@ -80,12 +80,13 @@ static const char *tmpfs;
>>   * outputing a 'B' every so often if it's still running.
>>   */
>>  #include "tests/migration/x86-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);
>>  }
>>  
>> @@ -391,7 +392,7 @@ static void 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"
>> @@ -420,6 +421,22 @@ static void test_migrate_start(QTestState **from, 
>> QTestState **to,
>>                                    " -serial file:%s/dest_serial"
>>                                    " -incoming %s",
>>                                    accel, tmpfs, uri);
>> +    } else if (strcmp(arch, "aarch64") == 0) {
>> +        init_bootfile(bootpath, aarch64_kernel);
>> +        cmd_src = g_strdup_printf("-machine virt,accel=kvm:tcg -m 150M "
>> +                                  "-name vmsource,debug-threads=on -cpu 
>> host "
> 
> We can't use '-cpu host' with tcg, so the accel fallback won't work.

Will fix

> 
>> +                                  "-serial file:%s/src_serial "
>> +                                  "-kernel %s ",
>> +                                  tmpfs, bootpath);
>> +        cmd_dst = g_strdup_printf("-machine virt,accel=kvm:tcg -m 150M "
>> +                                  "-name vmdest,debug-threads=on -cpu host "
>> +                                  "-serial file:%s/dest_serial "
>> +                                  "-kernel %s "
>> +                                  "-incoming %s ",
>> +                                  tmpfs, bootpath, uri);
>> +        /* aarch64 virt machine physical mem started from 0x40000000 */
> 
> s/mem started from/memory starts at/

will fix

> 
>> +        start_address += 0x40000000;
>> +        end_address += 0x40000000;
> 
> Why is end_address == start_address?

No, these two variables have been defined above. We just need to shift
them to new locations.

> 
>>      } else {
>>          g_assert_not_reached();
>>      }
>> @@ -501,7 +518,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/aarch64-a-b-kernel.h 
>> b/tests/migration/aarch64-a-b-kernel.h
>> new file mode 100644
>> index 0000000..5bdc74b
>> --- /dev/null
>> +++ b/tests/migration/aarch64-a-b-kernel.h
>> @@ -0,0 +1,19 @@
>> +/* This file is automatically generated from
>> + * tests/migration/aarch64-a-b-kernel.s, edit that and then run
>> + * tests/migration/rebuild-aarch64-kernel.sh to update, and then
>> + * remember to send both in your patch submission.
>> + */
>> +unsigned char aarch64_kernel[] = {
>> +  0x00, 0x10, 0x38, 0xd5, 0x00, 0xf8, 0x7f, 0x92, 0x00, 0x10, 0x18, 0xd5,
>> +  0xdf, 0x3f, 0x03, 0xd5, 0x24, 0x08, 0x80, 0x52, 0x05, 0x20, 0xa1, 0xd2,
>> +  0xa4, 0x00, 0x00, 0x39, 0x06, 0x00, 0x80, 0x52, 0x03, 0xc8, 0xa8, 0xd2,
>> +  0x02, 0x02, 0xa8, 0xd2, 0x01, 0x00, 0x80, 0x52, 0x41, 0x00, 0x00, 0x39,
>> +  0x42, 0x04, 0x40, 0x91, 0x5f, 0x00, 0x03, 0xeb, 0xad, 0xff, 0xff, 0x54,
>> +  0x02, 0x02, 0xa8, 0xd2, 0x22, 0x7e, 0x0b, 0xd5, 0x41, 0x00, 0x40, 0x39,
>> +  0x21, 0x04, 0x00, 0x11, 0x21, 0x1c, 0x00, 0x12, 0x41, 0x00, 0x00, 0x39,
>> +  0x42, 0x04, 0x40, 0x91, 0x5f, 0x00, 0x03, 0xeb, 0x2b, 0xff, 0xff, 0x54,
>> +  0xc6, 0x04, 0x00, 0x11, 0xc6, 0x1c, 0x00, 0x12, 0xdf, 0x00, 0x00, 0x71,
>> +  0x81, 0xfe, 0xff, 0x54, 0x44, 0x08, 0x80, 0x52, 0x05, 0x20, 0xa1, 0xd2,
>> +  0xa4, 0x00, 0x00, 0x39, 0xf0, 0xff, 0xff, 0x97
>> +};
>> +
>> diff --git a/tests/migration/aarch64-a-b-kernel.s 
>> b/tests/migration/aarch64-a-b-kernel.s
>> new file mode 100644
>> index 0000000..bc20c81
>> --- /dev/null
>> +++ b/tests/migration/aarch64-a-b-kernel.s
>> @@ -0,0 +1,67 @@
>> +#!/bin/sh
>> +# Copyright (c) 2018 Red Hat, Inc. and/or its affiliates
>> +#
>> +# Authors:
>> +#   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.
>> +
>> +.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
> 
> The MMU is always initially off, so this isn't really necessary, but OK.
> 
>> +
>> +        /* output char 'A' to PL011 */
>> +        mov     w4, #65
>> +        mov     x5, #0x9000000
>> +        strb    w4, [x5]
>> +
>> +        /* w6 keeps a counter so we can limit the output speed */
>> +        mov     w6, #0
>> +
>> +        /* phys mem base addr = 0x40000000 */
>> +        mov     x3, #(0x40000000 + 100 *1024*1024) /* traverse 1M-100M */
>> +        mov     x2, #(0x40000000 + 1 * 1024*1024)
> 
> How about creating a tests include file that contains this memory base
> address so we don't have to scatter it around too many files? We can
> throw in other defines too.
> 
>  tests/arm-mach-virt.h:
>  #define ARM_MACH_VIRT_UART      0x09000000
>  #define ARM_MACH_VIRT_PHYS_BASE 0x40000000
>  #define ARM_MACH_VIRT_A_B_START 0x40000000
>  #define ARM_MACH_VIRT_A_B_END   (0x40000000 * 100)

Will fix with ".set"

> 
>> +
>> +        /* clean up memory first */
>> +        mov     w1, #0
>> +clean:
>> +        strb    w1, [x2]
>> +        add     x2, x2, #(4096)
>> +        cmp     x2, x3
>> +        ble     clean
> 
> Why are we only clearing the first byte of each 4K chunk? Anyway QEMU
> always gives us clean memory, so this isn't really necessary.

Unfortunately this isn't always true. I wrote a small program to check
guest VM's memory on AArch64 (every 4KB). It turned that three locations
have non-zero content:

Addr=0x40100000, Content=0x10
Addr=0x40110000, Content=0x10
Addr=0x44c01000, Content=0x6d


> 
>> +
>> +        /* main body */
>> +mainloop:
>> +        mov     x2, #(0x40000000 + 1 * 1024*1024)
> 
> nit: This reassignment of x2 would be unnecessary if you copied x2 into
>      a scratch register (x7) prior to the clean loop, and then used the
>      scratch register there. It's the same number of instructions, but
>      less hard coded constants floating around.

Will fix

> 
>> +
>> +innerloop:
>> +        /* clean cache because el2 might still cache guest data under KVM */
>> +        dc      civac, x2
> 
> Blank line here and a comment introducing the next block would be nice.
> 
>> +        ldrb    w1, [x2]
>> +        add     w1, w1, #1
>> +        and     w1, w1, #(0xff)
>> +        strb    w1, [x2]
>> +
>> +        add     x2, x2, #(4096)
>> +        cmp     x2, x3
>> +        blt     innerloop
> 
> OK, I guess we only care about the first byte of each 4K chunk.
> 
>> +
>> +        add     w6, w6, #1
>> +        and     w6, w6, #(0xff)
>> +        cmp     w6, #0
>> +        bne     mainloop
> 
> So we limit the output speed by doing the write loop 256 times? How was
> the number 256 selected? If we want to delay the outputs we can use the
> timer counter to ensure we only output every N us. I pulled together
> some code that implements a 100 us delay:
> 
>       mrs     x0, cntfrq_el0
>       mov     x1, #10000
>       udiv    x1, x0, x1
>       mrs     x0, cntvct_el0
>       add     x0, x0, x1
>  1:   isb
>       mrs     x1, cntvct_el0
>       subs    x1, x0, x1
>       b.gt    1b
> 

256 is a number I picked after trying on aarch64 server. I think I will
stick with the original design. The reason is that we don't want to rely
on hardware availability (timer) in such a short program. 256 is a good
enough choice which serves the purpose and still is very portable.

>> +
>> +        /* output char 'B' to PL011 */
>> +        mov     w4, #66
>> +        mov     x5, #0x9000000
> 
> You never overwrote x5, so you don't need to rewrite the uart addr here.
> 
>> +        strb    w4, [x5]
>> +
>> +        bl      mainloop
>> diff --git a/tests/migration/rebuild-aarch64-kernel.sh 
>> b/tests/migration/rebuild-aarch64-kernel.sh
>> new file mode 100755
>> index 0000000..0fbca99
>> --- /dev/null
>> +++ b/tests/migration/rebuild-aarch64-kernel.sh
>> @@ -0,0 +1,67 @@
>> +#!/bin/sh
>> +# Copyright (c) 2018 Red Hat, Inc. and/or its affiliates
>> +#
>> +# Authors:
>> +#   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.
>> +
>> +ASMFILE=tests/migration/aarch64-a-b-kernel.s
>> +HEADER=tests/migration/aarch64-a-b-kernel.h
>> +
>> +if [ ! -e "$ASMFILE" ]
>> +then
>> +  echo "Couldn't find $ASMFILE" >&2
>> +  exit 1
>> +fi
>> +
>> +ASM_WORK_DIR=/tmp/AARCH64BB$$
> 
> Same mktemp comment as in David's patch.

Will do

> 
>> +
>> +mkdir $ASM_WORK_DIR &&
>> +cat <<EOF > "$ASM_WORK_DIR/linker.lds" &&
>> +SECTIONS
>> +{
>> +    .text : { *(.init) *(.text) *(.text.*) }
>> +    . = ALIGN(64K);
>> +    etext = .;
>> +    .data : {
>> +        *(.data)
>> +    }
>> +    . = ALIGN(16);
>> +    .rodata : { *(.rodata) }
>> +    . = ALIGN(16);
>> +    .bss : { *(.bss) }
>> +    . = ALIGN(64K);
>> +    edata = .;
>> +    . += 64K;
>> +    . = ALIGN(64K);
>> +    /*
>> +     * stack depth is 16K for arm and PAGE_SIZE for arm64, see THREAD_SIZE
>> +     * sp must be 16 byte aligned for arm64, and 8 byte aligned for arm
>> +     * sp must always be strictly less than the true stacktop
>> +     */
>> +    stackptr = . - 16;
>> +    stacktop = .;
>> +}
>> +ENTRY(start)
>> +EOF
>> +as -march="armv8-a" -c "$ASMFILE" -o $ASM_WORK_DIR/kernel.o &&
>> +gcc -O2 -o $ASM_WORK_DIR/kernel.elf -nostdlib \
>> +    -Wl,-T,$ASM_WORK_DIR/linker.lds,--build-id=none,-Ttext=40080000 \
>> +    $ASM_WORK_DIR/kernel.o &&
> 
> You don't need the above linker script nor the 'as' line. Just do
> 
>  gcc -o $ASM_WORK_DIR/kernel.elf \
>      -nostdlib -Wl,--build-id=none,-Ttext=40080000 $ASMFILE
> 

Will do

>> +objcopy -O binary $ASM_WORK_DIR/kernel.elf $ASM_WORK_DIR/kernel.flat &&
> 
> Shouldn't gcc and objcopy have a $CROSS_COMPILER prefix variable
> prepended? Does the QEMU test build environment support cross
> compiling? Can this script get build environment variables some how?
> 

I think this is a good point. For cross compilation, I decided to change
this re-build script into a Makefile (also merged with Dave Gilbert's
version).

>> +xxd -i $ASM_WORK_DIR/kernel.flat |
>> +sed -e 's/_tmp.*_kernel_flat/aarch64_kernel/' -e 's/.*int.*//' > \
>> +  $ASM_WORK_DIR/kernel.hex &&
>> +cat - $ASM_WORK_DIR/kernel.hex <<EOF > "$HEADER"
>> +/* This file is automatically generated from
>> + * tests/migration/aarch64-a-b-kernel.s, edit that and then run
>> + * tests/migration/rebuild-aarch64-kernel.sh to update, and then
>> + * remember to send both in your patch submission.
>> + */
>> +EOF
>> +
>> +rm $ASM_WORK_DIR/kernel.hex $ASM_WORK_DIR/kernel.flat \
>> +    $ASM_WORK_DIR/kernel.elf $ASM_WORK_DIR/kernel.o $ASM_WORK_DIR/linker.lds
>> +rmdir $ASM_WORK_DIR
>> -- 
>> 1.8.3.1
>>
>>
> 
> Thanks,
> drew 
>