Message ID | 1474921066-22701-1-git-send-email-thuth@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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). > + 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);
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. >> + 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
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.
On 28.09.2016 03:59, David Gibson wrote: > 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: [...] >>>> 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) [...] > [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. Oh, good to know, that's the things that you miss as a non-native speaker ... so I actually really meant "a few" here (though some of the extensions are IMHO rather strange). Thomas
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) { + 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) { 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); } 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);
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> --- tests/Makefile.include | 1 + tests/boot-sector.c | 9 +++++++++ tests/pxe-test.c | 22 +++++++++++++++------- 3 files changed, 25 insertions(+), 7 deletions(-)