Re: [Qemu-ppc] [PATCH] tests: Test IPv6 and ppc64 in the PXE tester

[Thomas Huth]

On 27.09.2016 06:17, David Gibson wrote:
> On Mon, Sep 26, 2016 at 10:17:46PM +0200, Thomas Huth wrote:
>> The firmware of the pseries machine, SLOF, is able to load files via
>> IPv6 networking, too. So to test both, network bootloading on ppc64
>> and IPv6 (via Slirp) , let's add some PXE tests for this environment,
>> too. Since we can not use the normal x86 boot sector for network boot
>> loading, we use a simple Forth script on ppc64 instead.
>>
>> Signed-off-by: Thomas Huth <address@hidden>
> 
> I certainly approve of testing IPv6 more, a couple of queries about
> the details though:
> 
>> ---
>>  tests/Makefile.include |  1 +
>>  tests/boot-sector.c    |  9 +++++++++
>>  tests/pxe-test.c       | 22 +++++++++++++++-------
>>  3 files changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index d8101b3..18bc698 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -270,6 +270,7 @@ check-qtest-ppc64-y += tests/drive_del-test$(EXESUF)
>>  check-qtest-ppc64-y += tests/postcopy-test$(EXESUF)
>>  check-qtest-ppc64-y += tests/boot-serial-test$(EXESUF)
>>  check-qtest-ppc64-y += tests/rtas-test$(EXESUF)
>> +check-qtest-ppc64-y += tests/pxe-test$(EXESUF)
>>  
>>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>>  
>> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
>> index 3ffe298..e3193c0 100644
>> --- a/tests/boot-sector.c
>> +++ b/tests/boot-sector.c
>> @@ -77,6 +77,15 @@ int boot_sector_init(const char *fname)
>>          fprintf(stderr, "Couldn't open \"%s\": %s", fname, strerror(errno));
>>          return 1;
>>      }
>> +
>> +    /* For Open Firmware based system, we can use a Forth script instead */
>> +    if (strcmp(qtest_get_arch(), "ppc64") == 0) {
> 
> As always, I'm uneasy about using arch based tests for what's really a
> machine type property.  Still, as a test case, I guess we can fix that
> when and if someone actually tries to run it for a ppc machine that's
> not spapr (or an x86 machine that's not pc, theoretically speaking).

As long as we don't have a fancy qtest_get_machine() function, I think
this is the best we can do right now. And since this code has to be
touched anyway when another machine type should be used to run the
boot_sector_init() function, I think it's OK to postpone this to this
later point in time.

>> +        memset(boot_sector, ' ', sizeof boot_sector);
>> +        sprintf((char *)boot_sector, "\\ Bootscript\n%x %x c! %x %x c!\n",
>> +                LOW(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET,
>> +                HIGH(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFSET + 
>> 1);
>> +    }
>> +
>>      fwrite(boot_sector, 1, sizeof boot_sector, f);
>>      fclose(f);
>>      return 0;
>> diff --git a/tests/pxe-test.c b/tests/pxe-test.c
>> index b2cc355..0bdb7a1 100644
>> --- a/tests/pxe-test.c
>> +++ b/tests/pxe-test.c
>> @@ -21,14 +21,14 @@
>>  
>>  static const char *disk = "tests/pxe-test-disk.raw";
>>  
>> -static void test_pxe_one(const char *params)
>> +static void test_pxe_one(const char *params, bool ipv6)
> 
> Is it wise to keep the "PXE" name.  OF style netbooting isn't really
> PXE in the sense of the Intel PXE spec, although it overlaps in the
> underlying protocols used.

Strictly speaking, you're right. But the overlap from the networking
protocol point of view is 95%, I'd guess, basically you can say that:

 PXE = TFTP + DHCP + some few DHCP extensions

... and PXE also defines a x86 API which of course does not apply for ppc.

So in my experience, most people simply talk / know about PXE, but
rather mean network booting via DHCP + TFTP. So I'm fine with keeping
the pxe wording here, but if you like, I can also add another patch to
get rid of this (but then the whole file should also be renamed, I
guess? ... is this worth the effort here?)

>>  {
>>      char *args;
>>  
>> -    args = g_strdup_printf("-machine accel=tcg "
>> -                           "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s 
>> "
>> -                           "%s ",
>> -                           disk, params);
>> +    args = g_strdup_printf("-machine accel=tcg -boot order=n "
>> +                           "-netdev user,id=" NETNAME 
>> ",tftp=./,bootfile=%s,"
>> +                           "ipv4=%s,ipv6=%s %s", disk, ipv6 ? "off" : "on",
>> +                           ipv6 ? "on" : "off", params);
>>  
>>      qtest_start(args);
>>      boot_sector_test();
>> @@ -38,12 +38,17 @@ static void test_pxe_one(const char *params)
>>  
>>  static void test_pxe_e1000(void)
>>  {
>> -    test_pxe_one("-device e1000,netdev=" NETNAME);
>> +    test_pxe_one("-device e1000,netdev=" NETNAME, false);
>>  }
>>  
>>  static void test_pxe_virtio_pci(void)
>>  {
>> -    test_pxe_one("-device virtio-net-pci,netdev=" NETNAME);
>> +    test_pxe_one("-device virtio-net-pci,netdev=" NETNAME, false);
>> +}
>> +
>> +static void test_pxe_spapr_vlan(void)
>> +{
>> +    test_pxe_one("-vga none -device spapr-vlan,netdev=" NETNAME, true);
> 
> Shouldn't we test both IPv4 *and* IPv6 for spapr - AFAICT this is only
> testing v6.

The IPv4 part of SLOF is already exercised via the test_pxe_virtio_pci
test, so I don't think we'd gain a lot more of test coverage by running
the spapr-vlan test additionally with IPv4. And since this test is
rather slow (it takes a couple of seconds), I think it's better to only
test IPv6 with spapr-vlan.

>>  }
>>  
>>  int main(int argc, char *argv[])
>> @@ -60,6 +65,9 @@ int main(int argc, char *argv[])
>>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>>          qtest_add_func("pxe/e1000", test_pxe_e1000);
>>          qtest_add_func("pxe/virtio", test_pxe_virtio_pci);
>> +    } else if (strcmp(arch, "ppc64") == 0) {
>> +        qtest_add_func("pxe/virtio", test_pxe_virtio_pci);
>> +        qtest_add_func("pxe/spapr-vlan", test_pxe_spapr_vlan);
>>      }
>>      ret = g_test_run();
>>      boot_sector_cleanup(disk);

 Thomas

signature.asc
Description: OpenPGP digital signature