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.
>> + 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
signature.asc
Description: OpenPGP digital signature
