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

[David Gibson]

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

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


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

>  }
>  
>  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);

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature