Re: [Qemu-devel] [PATCH] test: port postcopy test to ppc64

[Thomas Huth]

On 21.07.2016 12:12, Laurent Vivier wrote:
> As userfaultfd syscall is available on powerpc, migration
> postcopy can be used.

Good idea to add this test for PPC, too!

> This patch adds the support needed to test this on powerpc,
> instead of using a bootsector to run code to modify memory,
> we use a FORTH script in "boot-command" property.
> 
> As spapr machine doesn't support "-prom-env" argument
> (the nvram is initialized by SLOF and not by QEMU),
> "boot-command" is provided to SLOF via a file mapped nvram
> (with "-drive file=...,if=pflash")

I wonder whether we could easily add support for the "-prom-env"
parameter for the sPAPR machine, too, since the NVRAM layout seems to be
pretty much the same as on the old CHRP Mac machines...?

> Signed-off-by: Laurent Vivier <address@hidden>
> ---
>  tests/Makefile.include |   1 +
>  tests/postcopy-test.c  | 141 
> ++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 123 insertions(+), 19 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index e7e50d6..e2d1885 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -268,6 +268,7 @@ check-qtest-sparc-y += tests/prom-env-test$(EXESUF)
>  #check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>  check-qtest-xtensaeb-y = $(check-qtest-xtensa-y)
> +check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
>  
>  check-qtest-generic-y += tests/qom-test$(EXESUF)
>  
> diff --git a/tests/postcopy-test.c b/tests/postcopy-test.c
> index 16465ab..439afd9 100644
> --- a/tests/postcopy-test.c
> +++ b/tests/postcopy-test.c
> @@ -19,6 +19,24 @@
>  #include "sysemu/char.h"
>  #include "sysemu/sysemu.h"
>  
> +/* These structures are already defined by OpenBIOS and usable with SLOF */
> +#define NVRAM_PART_SYSTEM 0x70
> +struct nvpart {
> +    uint8_t signature;
> +    uint8_t checksum;
> +    uint16_t len; /* BE, length divided by 16 */
> +    char name[12];
> +    char content[0];

IIRC zero-sized arrays are a GCC extension ... for valid C99, it might
be better to use "char content[]" instead?

> +};
>
> +#define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
> +
> +/* FW_MAX_SIZE is 4MB, but slof.bin is only 900KB,
> + * so let's modify memory between 1MB and 100MB
> + * to do like PC bootsector
> + */
> +#define FORTH_BOOTSCRIPT "boot-command=hex .\" _\" begin %x %x do i c@ 1 + i 
> c! 1000 +loop .\" B\" 0 until"

Just a matter of taste, but I somewhat dislike the idea of hiding a
string with format parameters in a macro ... I think I'd rather place
this string directly into the corresponding sprintf() statement below
instead.

>  const unsigned start_address = 1024 * 1024;
>  const unsigned end_address = 100 * 1024 * 1024;
>  bool got_stop;
> @@ -122,6 +140,52 @@ unsigned char bootsect[] = {
>    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x55, 0xaa
>  };
>  
> +static void init_bootfile_x86(const char *bootpath)
> +{
> +    FILE *bootfile = fopen(bootpath, "wb");
> +
> +    g_assert_cmpint(fwrite(bootsect, 512, 1, bootfile), ==, 1);
> +    fclose(bootfile);
> +}
> +
> +static void nvpart_checksum(struct nvpart *header)
> +{
> +    unsigned int i, sum;
> +    uint8_t *tmpptr;
> +
> +    tmpptr = (uint8_t *)header;
> +    sum = *tmpptr;
> +    for (i = 0; i < 14; i++) {
> +        sum += tmpptr[2 + i];
> +        sum = (sum + ((sum & 0xff00) >> 8)) & 0xff;
> +    }
> +    header->checksum = sum & 0xff;
> +}

Have you tried to include openbios_firmware_abi.h instead and use
OpenBIOS_finish_partition() here? That would avoid to have this code
duplicated.

> +static void init_bootfile_ppc(const char *bootpath)
> +{
> +    FILE *bootfile;
> +    char buf[MIN_NVRAM_SIZE];
> +    struct nvpart *header = (struct nvpart *)buf;
> +
> +    memset(buf, 0, MIN_NVRAM_SIZE);
> +
> +    /* Create a "common" partition in nvram to store boot-command property */
> +
> +    header->signature = NVRAM_PART_SYSTEM;
> +    memcpy(header->name, "common", 6);
> +    header->len = cpu_to_be16(MIN_NVRAM_SIZE >> 4);
> +    nvpart_checksum(header); /* can change if we change header->len */
> +
> +    sprintf(header->content, FORTH_BOOTSCRIPT, end_address, start_address);
> +
> +    /* Write partition to the NVRAM file */
> +
> +    bootfile = fopen(bootpath, "wb");
> +    g_assert_cmpint(fwrite(buf, MIN_NVRAM_SIZE, 1, bootfile), ==, 1);
> +    fclose(bootfile);
> +}
> +
>  /*
>   * Wait for some output in the serial output file,
>   * we get an 'A' followed by an endless string of 'B's
> @@ -131,10 +195,31 @@ static void wait_for_serial(const char *side)
>  {
>      char *serialpath = g_strdup_printf("%s/%s", tmpfs, side);
>      FILE *serialfile = fopen(serialpath, "r");
> +    const char *arch = qtest_get_arch();
> +    int started = (strcmp(side, "src_serial") == 0 &&
> +                   strcmp(arch, "ppc64") == 0) ? 0 : 1;
>  
>      do {
>          int readvalue = fgetc(serialfile);
>  
> +        if (!started) {
> +            /* SLOF prints its banner before starting test,
> +             * to ignore it, mark the start of the test with '_',
> +             * ignore all characters until this marker
> +             */
> +            switch (readvalue) {
> +            case '_':
> +                started = 1;
> +                break;
> +            case EOF:
> +                fseek(serialfile, 0, SEEK_SET);
> +                usleep(1000);
> +                break;
> +            default:
> +                break;

I think you could remove that default case.

> +            }
> +            continue;
> +        }
>          switch (readvalue) {
>          case 'A':
>              /* Fine */
> @@ -147,6 +232,8 @@ static void wait_for_serial(const char *side)
>              return;
>  
>          case EOF:
> +            started = (strcmp(side, "src_serial") == 0 &&
> +                       strcmp(arch, "ppc64") == 0) ? 0 : 1;

I somehow fail to see why is this needed here again? Isn't the initial
setup of "started" at the beginning of the function enough?

>              fseek(serialfile, 0, SEEK_SET);
>              usleep(1000);
>              break;
> @@ -295,32 +382,48 @@ static void test_migrate(void)
>      char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
>      QTestState *global = global_qtest, *from, *to;
>      unsigned char dest_byte_a, dest_byte_b, dest_byte_c, dest_byte_d;
> -    gchar *cmd;
> +    gchar *cmd, *cmd_src, *cmd_dst;
>      QDict *rsp;
>  
>      char *bootpath = g_strdup_printf("%s/bootsect", tmpfs);
> -    FILE *bootfile = fopen(bootpath, "wb");
> +    const char *arch = qtest_get_arch();
>  
>      got_stop = false;
> -    g_assert_cmpint(fwrite(bootsect, 512, 1, bootfile), ==, 1);
> -    fclose(bootfile);
>  
> -    cmd = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
> -                          " -name pcsource,debug-threads=on"
> -                          " -serial file:%s/src_serial"
> -                          " -drive file=%s,format=raw",
> -                          tmpfs, bootpath);
> -    from = qtest_start(cmd);
> -    g_free(cmd);
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        init_bootfile_x86(bootpath);
> +        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
> +                                  " -name pcsource,debug-threads=on"
> +                                  " -serial file:%s/src_serial"
> +                                  " -drive file=%s,format=raw",
> +                                  tmpfs, bootpath);
> +        cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
> +                                  " -name pcdest,debug-threads=on"
> +                                  " -serial file:%s/dest_serial"
> +                                  " -drive file=%s,format=raw"
> +                                  " -incoming %s",
> +                                  tmpfs, bootpath, uri);
> +    } else if (strcmp(arch, "ppc64") == 0) {
> +        init_bootfile_ppc(bootpath);
> +        cmd_src = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> +                                  " -name pcsource,debug-threads=on"
> +                                  " -serial file:%s/src_serial"
> +                                  " -drive file=%s,if=pflash,format=raw",
> +                                  tmpfs, bootpath);
> +        cmd_dst = g_strdup_printf("-machine accel=kvm:tcg -m 256M"
> +                                  " -name pcdest,debug-threads=on"
> +                                  " -serial file:%s/dest_serial"
> +                                  " -incoming %s",
> +                                  tmpfs, uri);
> +    } else {
> +        g_assert_not_reached();
> +    }
>  
> -    cmd = g_strdup_printf("-machine accel=kvm:tcg -m 150M"
> -                          " -name pcdest,debug-threads=on"
> -                          " -serial file:%s/dest_serial"
> -                          " -drive file=%s,format=raw"
> -                          " -incoming %s",
> -                          tmpfs, bootpath, uri);
> -    to = qtest_init(cmd);
> -    g_free(cmd);
> +    from = qtest_start(cmd_src);
> +    g_free(cmd_src);
> +
> +    to = qtest_init(cmd_dst);
> +    g_free(cmd_dst);
>  
>      global_qtest = from;
>      rsp = qmp("{ 'execute': 'migrate-set-capabilities',"
> 

 Thomas