From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38634) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bp4fP-0005Eo-3T for qemu-devel@nongnu.org; Tue, 27 Sep 2016 22:32:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bp4fM-00013D-G6 for qemu-devel@nongnu.org; Tue, 27 Sep 2016 22:32:13 -0400 Date: Wed, 28 Sep 2016 11:59:00 +1000 From: David Gibson Message-ID: <20160928015900.GB18880@umbus> References: <1474921066-22701-1-git-send-email-thuth@redhat.com> <20160927041707.GG30322@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mxv5cy4qt+RJ9ypb" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH] tests: Test IPv6 and ppc64 in the PXE tester List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: qemu-devel@nongnu.org, Jason Wang , Samuel Thibault , Jan Kiszka , Michael S Tsirkin , Victor Kaplansky , Alexander Graf , qemu-ppc@nongnu.org --mxv5cy4qt+RJ9ypb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > >=20 > > I certainly approve of testing IPv6 more, a couple of queries about > > the details though: > >=20 > >> --- > >> 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 +=3D tests/drive_del-test$(EXE= SUF) > >> check-qtest-ppc64-y +=3D tests/postcopy-test$(EXESUF) > >> check-qtest-ppc64-y +=3D tests/boot-serial-test$(EXESUF) > >> check-qtest-ppc64-y +=3D tests/rtas-test$(EXESUF) > >> +check-qtest-ppc64-y +=3D tests/pxe-test$(EXESUF) > >> =20 > >> check-qtest-sh4-y =3D tests/endianness-test$(EXESUF) > >> =20 > >> 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(e= rrno)); > >> return 1; > >> } > >> + > >> + /* For Open Firmware based system, we can use a Forth script inst= ead */ > >> + if (strcmp(qtest_get_arch(), "ppc64") =3D=3D 0) { > >=20 > > 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). >=20 > 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_OFFSE= T, > >> + HIGH(SIGNATURE), BOOT_SECTOR_ADDRESS + SIGNATURE_OFFS= ET + 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 @@ > >> =20 > >> static const char *disk =3D "tests/pxe-test-disk.raw"; > >> =20 > >> -static void test_pxe_one(const char *params) > >> +static void test_pxe_one(const char *params, bool ipv6) > >=20 > > 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. >=20 > 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: >=20 > PXE =3D 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. >=20 > 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. >=20 > >> { > >> char *args; > >> =20 > >> - args =3D g_strdup_printf("-machine accel=3Dtcg " > >> - "-netdev user,id=3D" NETNAME ",tftp=3D./,b= ootfile=3D%s " > >> - "%s ", > >> - disk, params); > >> + args =3D g_strdup_printf("-machine accel=3Dtcg -boot order=3Dn " > >> + "-netdev user,id=3D" NETNAME ",tftp=3D./,b= ootfile=3D%s," > >> + "ipv4=3D%s,ipv6=3D%s %s", disk, ipv6 ? "of= f" : "on", > >> + ipv6 ? "on" : "off", params); > >> =20 > >> qtest_start(args); > >> boot_sector_test(); > >> @@ -38,12 +38,17 @@ static void test_pxe_one(const char *params) > >> =20 > >> static void test_pxe_e1000(void) > >> { > >> - test_pxe_one("-device e1000,netdev=3D" NETNAME); > >> + test_pxe_one("-device e1000,netdev=3D" NETNAME, false); > >> } > >> =20 > >> static void test_pxe_virtio_pci(void) > >> { > >> - test_pxe_one("-device virtio-net-pci,netdev=3D" NETNAME); > >> + test_pxe_one("-device virtio-net-pci,netdev=3D" NETNAME, false); > >> +} > >> + > >> +static void test_pxe_spapr_vlan(void) > >> +{ > >> + test_pxe_one("-vga none -device spapr-vlan,netdev=3D" NETNAME, tr= ue); > >=20 > > Shouldn't we test both IPv4 *and* IPv6 for spapr - AFAICT this is only > > testing v6. >=20 > 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. >=20 > >> } > >> =20 > >> int main(int argc, char *argv[]) > >> @@ -60,6 +65,9 @@ int main(int argc, char *argv[]) > >> if (strcmp(arch, "i386") =3D=3D 0 || strcmp(arch, "x86_64") =3D= =3D 0) { > >> qtest_add_func("pxe/e1000", test_pxe_e1000); > >> qtest_add_func("pxe/virtio", test_pxe_virtio_pci); > >> + } else if (strcmp(arch, "ppc64") =3D=3D 0) { > >> + qtest_add_func("pxe/virtio", test_pxe_virtio_pci); > >> + qtest_add_func("pxe/spapr-vlan", test_pxe_spapr_vlan); > >> } > >> ret =3D g_test_run(); > >> boot_sector_cleanup(disk); >=20 > Thomas >=20 >=20 [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. --=20 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 --mxv5cy4qt+RJ9ypb Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJX6yPhAAoJEGw4ysog2bOScRoQAOHxkuhZZrStik6r6E5WFEt9 C3/Lnpp40f6HPQ2qdLm1JubqWOjeW1W75K138lv8LpHjm/SaVxxLoR7tOz7cuxaa zRTucpMn57xIYueBSkdCoC0gEB55KoxQV9ufJUNImuLLZxs7sz+TQhw868Y1tQbU HQ0EkOn3hj4PheqozOcjooIIhQxGa1G7n3N89ququ1WFU71iT3Hq6IO5dnZisKc5 v8gEdnUL8/UTUTqp6t6QXg+wFpKlXuZtCEKw9ydtUsCc8ML4gWlOvm/ebsbACYeF TFj4FO7d2VgPLFalggQrDoC3AErMO8JwP7+22IgxuNva6T9U9/kwxiTrdNCp+qTk iXoUCiwt150UNNg/2fdLkVOwGsXzV1hb8QnPSFUy7vWyaGlwBNAKe8/2pWL3DTR3 +xCm+qPgZTY4fAQ3/x6QY2QKUoyuQtJ7H5acTF9V4Z15kKRM1x9xMoUeUjaOHyAf v4UEM1WbvIRQ+JY/3HVrl1+6pZYZUvtU1JZUAXS0ZFIv5eUkzvp7nv21YkA9dwwa bFbGxqz2o8KIMVJCZ6CyZA/u8w7v+3IFolh9uVSjMxyQKcYRyZa0XIV1TdeNkNwy xaBSGkp9zxIR0+R8DuHaMgZHhqHKgVR0yWRAAMrWE1vBBVwyYjjSmHQ3KXsQWKG6 d2cGemjwPsbRx4G46I7/ =mOtc -----END PGP SIGNATURE----- --mxv5cy4qt+RJ9ypb--