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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7115 bytes --]

On Tue, Sep 27, 2016 at 09:17:19AM +0200, Thomas Huth wrote:
> 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 <thuth@redhat.com>
> > 
> > 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.

I concur.

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

(aside on subtle English usage at [0] if you're interested)

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

Hm.. you convinced me.  Let's just leave the name as is.

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

Fair enough.  Ok, I'll go back and apply this patch as is.

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

[0] A native speaker would probably say "a few" DHCP extensions here.
"some few", oddly enough, reads as very slight sarcasm implying that
there are actually quite a lot of extensions, or at least more than
you'd expect.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]