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

[Laurent Vivier]

On 21/07/2016 15:16, Thomas Huth wrote:
> 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!

The idea comes from David Gilbert :)
Thanks David!

> 
>> 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...?

The only thing we need to have "-prom-env" is to initialize the nvram
partitions in QEMU if they don't exist.

I didn't follow this way because this work is normally to be done by the
firmware not by QEMU. If we do it in QEMU, SLOF will do not, and we
can't be sure we are doing the things well, as SLOF does.

On Mac, it is another history, originally, firmware was OpenHackware,
not OpenBIOS, and OH was only here to allow to boot the kernel nothing
more (it was really a hack: there was not forth interpreter [oh, my god!]).
So all the important stuffs were done by QEMU, like to initialize the
NVRAM, the PCI bus, ... and this allows to set values in the environment
variables.

But if you think it is viable, it should be really easy to do (like
cut'n'paste from OpenBIOS ABI)...

Thanks,
Laurent


> 
>> 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
>