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

You're right: I though [0] was more generic than [].

> 
>> +};
>>
>> +#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.

Right

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

Right again

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

ok

> 
>> +            }
>> +            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?

No, because we read a file and this file is growing while we are reading
it, so if we restart to read the file (fseek() below) because we are at
the end, we must skip again the banner.

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

I'm going to send a v2...

Thanks,
Laurent