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 <th...@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
signature.asc
Description: PGP signature
