[Qemu-devel] [PATCH 0/4] Improvements to pxe-test

[Qemu-devel] [PATCH 0/4] Improvements to pxe-test

[David Gibson]

This series makes several cleanups and enhancements to tests/pxe-test.
In particular it improves its handling of different machine types.

David Gibson (4):
  tests/pxe-test: Remove unnecessary special case test functions
  tests/pxe-test: Use table of testcases rather than open-coding
  tests/pxe-test: Test net booting over IPv6 in some cases
  tests/pxe-test: Add some extra tests

 tests/pxe-test.c | 107 ++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 82 insertions(+), 25 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/4] tests/pxe-test: Remove unnecessary special case test functions

[David Gibson]

All of the x86 and some of the other test cases here use a common test
function, test_pxe_ipv4(), but one ppc and one s390 test use different
functions.

In the s390 case, this is completely pointless, the right parameter to
test_pxe_ipv4() will already do exactly the right thing.  For the
spapr-vlan case there's a slight difference - it will use IPv6 instead of
IPv4.

But testing just one case with IPv6 (and NOT IPv4) is rather haphazard.
Change everything to use the common test function, until we have a better
way of testing IPv6 across the board.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tests/pxe-test.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/tests/pxe-test.c b/tests/pxe-test.c
index 937f29e631..eb70aa2bc6 100644
--- a/tests/pxe-test.c
+++ b/tests/pxe-test.c
@@ -47,16 +47,6 @@ static void test_pxe_ipv4(gconstpointer data)
     g_free(dev_arg);
 }
 
-static void test_pxe_spapr_vlan(void)
-{
-    test_pxe_one("-device spapr-vlan,netdev=" NETNAME, true);
-}
-
-static void test_pxe_virtio_ccw(void)
-{
-    test_pxe_one("-device virtio-net-ccw,bootindex=1,netdev=" NETNAME, false);
-}
-
 int main(int argc, char *argv[])
 {
     int ret;
@@ -79,13 +69,14 @@ int main(int argc, char *argv[])
             qtest_add_data_func("pxe/vmxnet3", "vmxnet3", test_pxe_ipv4);
         }
     } else if (strcmp(arch, "ppc64") == 0) {
-        qtest_add_func("pxe/spapr-vlan", test_pxe_spapr_vlan);
+        qtest_add_data_func("pxe/spapr-vlan", "spapr-vlan", test_pxe_ipv4);
         if (g_test_slow()) {
             qtest_add_data_func("pxe/virtio", "virtio-net-pci", test_pxe_ipv4);
             qtest_add_data_func("pxe/e1000", "e1000", test_pxe_ipv4);
         }
     } else if (g_str_equal(arch, "s390x")) {
-        qtest_add_func("pxe/virtio-ccw", test_pxe_virtio_ccw);
+        qtest_add_data_func("pxe/virtio-ccw",
+                            "virtio-net-ccw,bootindex=1", test_pxe_ipv4);
     }
     ret = g_test_run();
     boot_sector_cleanup(disk);
-- 
2.14.3

[Qemu-devel] [PATCH 2/4] tests/pxe-test: Use table of testcases rather than open-coding

[David Gibson]

Currently pxe-tests open codes the list of tests for each architecture.
This changes it to use tables of test parameters, somewhat similar to
boot-serial-test.

This adds the machine type into the table as well, giving us the ability
to perform tests on multiple machine types for architectures where there's
more than one machine type that matters.

NOTE: This changes the names of the tests in the output, to include the
      machine type and IPv4 vs. IPv6.  I'm not sure if this has the
      potential to break existing tooling.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tests/pxe-test.c | 94 +++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 72 insertions(+), 22 deletions(-)

diff --git a/tests/pxe-test.c b/tests/pxe-test.c
index eb70aa2bc6..f9bca8976d 100644
--- a/tests/pxe-test.c
+++ b/tests/pxe-test.c
@@ -22,29 +22,86 @@
 
 static char disk[] = "tests/pxe-test-disk-XXXXXX";
 
-static void test_pxe_one(const char *params, bool ipv6)
+typedef struct testdef {
+    const char *machine;    /* Machine type */
+    const char *model;      /* NIC device model (human readable)*/
+    const char *extra;      /* Extra parameters, overriding default */
+} testdef_t;
+
+static testdef_t x86_tests[] = {
+    { "pc", "e1000" },
+    { "pc", "virtio", "-device virtio-net-pci,netdev=" NETNAME },
+    { NULL },
+};
+
+static testdef_t x86_tests_slow[] = {
+    { "pc", "ne2000", "-device ne2k_pci,netdev=" NETNAME },
+    { "pc", "eepro100", "-device i82550,netdev=" NETNAME },
+    { "pc", "rtl8139" },
+    { "pc", "vmxnet3" },
+    { NULL },
+};
+
+static testdef_t ppc64_tests[] = {
+    { "pseries", "spapr-vlan" },
+    { NULL },
+};
+
+static testdef_t ppc64_tests_slow[] = {
+    { "pseries", "virtio", "-device virtio-net-pci,netdev=" NETNAME },
+    { "pseries", "e1000" },
+    { NULL },
+};
+
+static testdef_t s390x_tests[] = {
+    { "s390-ccw-virtio", "virtio-ccw",
+      "-device virtio-net-ccw,bootindex=1,netdev=" NETNAME },
+    { NULL },
+};
+
+static void test_pxe_one(const testdef_t *test, bool ipv6)
 {
     char *args;
+    char *defextra;
+    const char *extra = test->extra;
+
+    defextra = g_strdup_printf("-device %s,netdev=" NETNAME, test->model);
+    if (!extra) {
+        extra = defextra;
+    }
 
-    args = g_strdup_printf("-machine accel=kvm:tcg -nodefaults -boot order=n "
-                           "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s,"
-                           "ipv4=%s,ipv6=%s %s", disk, ipv6 ? "off" : "on",
-                           ipv6 ? "on" : "off", params);
+    args = g_strdup_printf(
+        "-machine %s,accel=kvm:tcg -nodefaults -boot order=n "
+        "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s,ipv4=%s,ipv6=%s %s",
+        test->machine, disk, ipv6 ? "off" : "on", ipv6 ? "on" : "off", extra);
 
     qtest_start(args);
     boot_sector_test();
     qtest_quit(global_qtest);
     g_free(args);
+    g_free(defextra);
 }
 
 static void test_pxe_ipv4(gconstpointer data)
 {
-    const char *model = data;
-    char *dev_arg;
+    const testdef_t *test = data;
+
+    test_pxe_one(test, false);
+}
 
-    dev_arg = g_strdup_printf("-device %s,netdev=" NETNAME, model);
-    test_pxe_one(dev_arg, false);
-    g_free(dev_arg);
+static void test_batch(const testdef_t *tests)
+{
+    int i;
+
+    for (i = 0; tests[i].machine; i++) {
+        const testdef_t *test = &tests[i];
+        char *testname;
+
+        testname = g_strdup_printf("pxe/ipv4/%s/%s",
+                                   test->machine, test->model);
+        qtest_add_data_func(testname, test, test_pxe_ipv4);
+        g_free(testname);
+    }
 }
 
 int main(int argc, char *argv[])
@@ -59,24 +116,17 @@ int main(int argc, char *argv[])
     g_test_init(&argc, &argv, NULL);
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-        qtest_add_data_func("pxe/e1000", "e1000", test_pxe_ipv4);
-        qtest_add_data_func("pxe/virtio", "virtio-net-pci", test_pxe_ipv4);
+        test_batch(x86_tests);
         if (g_test_slow()) {
-            qtest_add_data_func("pxe/ne2000", "ne2k_pci", test_pxe_ipv4);
-            qtest_add_data_func("pxe/eepro100", "i82550", test_pxe_ipv4);
-            qtest_add_data_func("pxe/pcnet", "pcnet", test_pxe_ipv4);
-            qtest_add_data_func("pxe/rtl8139", "rtl8139", test_pxe_ipv4);
-            qtest_add_data_func("pxe/vmxnet3", "vmxnet3", test_pxe_ipv4);
+            test_batch(x86_tests_slow);
         }
     } else if (strcmp(arch, "ppc64") == 0) {
-        qtest_add_data_func("pxe/spapr-vlan", "spapr-vlan", test_pxe_ipv4);
+        test_batch(ppc64_tests);
         if (g_test_slow()) {
-            qtest_add_data_func("pxe/virtio", "virtio-net-pci", test_pxe_ipv4);
-            qtest_add_data_func("pxe/e1000", "e1000", test_pxe_ipv4);
+            test_batch(ppc64_tests_slow);
         }
     } else if (g_str_equal(arch, "s390x")) {
-        qtest_add_data_func("pxe/virtio-ccw",
-                            "virtio-net-ccw,bootindex=1", test_pxe_ipv4);
+        test_batch(s390x_tests);
     }
     ret = g_test_run();
     boot_sector_cleanup(disk);
-- 
2.14.3

[Qemu-devel] [PATCH 3/4] tests/pxe-test: Test net booting over IPv6 in some cases

[David Gibson]

This adds IPv6 net boot testing (in addition to IPv4) when in slow test
mode on ppc64 or s390.  IPv6 PXE doesn't seem to work on x86, I'm guessing
out BIOS image doesn't support it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tests/pxe-test.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/tests/pxe-test.c b/tests/pxe-test.c
index f9bca8976d..e7a0610070 100644
--- a/tests/pxe-test.c
+++ b/tests/pxe-test.c
@@ -89,7 +89,14 @@ static void test_pxe_ipv4(gconstpointer data)
     test_pxe_one(test, false);
 }
 
-static void test_batch(const testdef_t *tests)
+static void test_pxe_ipv6(gconstpointer data)
+{
+    const testdef_t *test = data;
+
+    test_pxe_one(test, true);
+}
+
+static void test_batch(const testdef_t *tests, bool ipv6)
 {
     int i;
 
@@ -101,6 +108,13 @@ static void test_batch(const testdef_t *tests)
                                    test->machine, test->model);
         qtest_add_data_func(testname, test, test_pxe_ipv4);
         g_free(testname);
+
+        if (ipv6) {
+            testname = g_strdup_printf("pxe/ipv6/%s/%s",
+                                       test->machine, test->model);
+            qtest_add_data_func(testname, test, test_pxe_ipv6);
+            g_free(testname);
+        }
     }
 }
 
@@ -116,17 +130,17 @@ int main(int argc, char *argv[])
     g_test_init(&argc, &argv, NULL);
 
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
-        test_batch(x86_tests);
+        test_batch(x86_tests, false);
         if (g_test_slow()) {
-            test_batch(x86_tests_slow);
+            test_batch(x86_tests_slow, false);
         }
     } else if (strcmp(arch, "ppc64") == 0) {
-        test_batch(ppc64_tests);
+        test_batch(ppc64_tests, g_test_slow());
         if (g_test_slow()) {
-            test_batch(ppc64_tests_slow);
+            test_batch(ppc64_tests_slow, true);
         }
     } else if (g_str_equal(arch, "s390x")) {
-        test_batch(s390x_tests);
+        test_batch(s390x_tests, g_test_slow());
     }
     ret = g_test_run();
     boot_sector_cleanup(disk);
-- 
2.14.3

[Qemu-devel] [PATCH 4/4] tests/pxe-test: Add some extra tests

[David Gibson]

Previously virtio-net was only tested for ppc64 in "slow" mode.  That
doesn't make much sense since virtio-net is used much more often in
practice than the spapr-vlan device which was tested always.  So, move
virtio-net to always be tested on ppc64.

We had no tests at all for the q35 machine, which doesn't seem wise given
it's increasing prominence.  Add a couple of tests for it, including
testing the newer e1000e adapter.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 tests/pxe-test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/pxe-test.c b/tests/pxe-test.c
index e7a0610070..16e2520940 100644
--- a/tests/pxe-test.c
+++ b/tests/pxe-test.c
@@ -31,6 +31,8 @@ typedef struct testdef {
 static testdef_t x86_tests[] = {
     { "pc", "e1000" },
     { "pc", "virtio", "-device virtio-net-pci,netdev=" NETNAME },
+    { "q35", "e1000e" },
+    { "q35", "virtio", "-device virtio-net-pci,netdev=" NETNAME },
     { NULL },
 };
 
@@ -44,11 +46,11 @@ static testdef_t x86_tests_slow[] = {
 
 static testdef_t ppc64_tests[] = {
     { "pseries", "spapr-vlan" },
+    { "pseries", "virtio", "-device virtio-net-pci,netdev=" NETNAME },
     { NULL },
 };
 
 static testdef_t ppc64_tests_slow[] = {
-    { "pseries", "virtio", "-device virtio-net-pci,netdev=" NETNAME },
     { "pseries", "e1000" },
     { NULL },
 };
-- 
2.14.3

Re: [Qemu-devel] [PATCH 4/4] tests/pxe-test: Add some extra tests

[Thomas Huth]

On 15.12.2017 11:16, David Gibson wrote:
> Previously virtio-net was only tested for ppc64 in "slow" mode.  That
> doesn't make much sense since virtio-net is used much more often in
> practice than the spapr-vlan device which was tested always.  So, move
> virtio-net to always be tested on ppc64.

Actually, I just moved virtio-net on ppc64 to the slow category a couple
of months ago:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=ab06ec43577177a442e8e5

This has been done since some people complained that SLOF is incredibly
slow in in TCG mode and thus there is the risk of hitting the timeout on
slow systems if they are overloaded.

Things will hopefully get a little bit better with the "Use
tcg_gen_lookup_and_goto_ptr" patch that you've queued in your
ppc-for-2.12 branch, but still, the test is very slow on ppc64, so I'd
rather keep it in the "slow" category. (virtio-net is already tested in
big endian mode on s390x anyway, so we at least got some endianess test
coverage for that NIC already).

> We had no tests at all for the q35 machine, which doesn't seem wise given
> it's increasing prominence.  Add a couple of tests for it, including
> testing the newer e1000e adapter.

That sounds like a good idea! I wonder whether we should also test some
"old" machine versions (in "slow" mode only, of course), like "pc-0.12"
for example, to make sure that there are no regressions with old machines?

 Thomas

Re: [Qemu-devel] [PATCH 1/4] tests/pxe-test: Remove unnecessary special case test functions

[Thomas Huth]

On 15.12.2017 11:16, David Gibson wrote:
> All of the x86 and some of the other test cases here use a common test
> function, test_pxe_ipv4(), but one ppc and one s390 test use different
> functions.
> 
> In the s390 case, this is completely pointless, the right parameter to
> test_pxe_ipv4() will already do exactly the right thing.  For the
> spapr-vlan case there's a slight difference - it will use IPv6 instead of
> IPv4.
> 
> But testing just one case with IPv6 (and NOT IPv4) is rather haphazard.
> Change everything to use the common test function, until we have a better
> way of testing IPv6 across the board.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tests/pxe-test.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] tests/pxe-test: Use table of testcases rather than open-coding

[Thomas Huth]

On 15.12.2017 11:16, David Gibson wrote:
> Currently pxe-tests open codes the list of tests for each architecture.
> This changes it to use tables of test parameters, somewhat similar to
> boot-serial-test.
> 
> This adds the machine type into the table as well, giving us the ability
> to perform tests on multiple machine types for architectures where there's
> more than one machine type that matters.
> 
> NOTE: This changes the names of the tests in the output, to include the
>       machine type and IPv4 vs. IPv6.  I'm not sure if this has the
>       potential to break existing tooling.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tests/pxe-test.c | 94 +++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 72 insertions(+), 22 deletions(-)
> 
> diff --git a/tests/pxe-test.c b/tests/pxe-test.c
> index eb70aa2bc6..f9bca8976d 100644
> --- a/tests/pxe-test.c
> +++ b/tests/pxe-test.c
> @@ -22,29 +22,86 @@
>  
>  static char disk[] = "tests/pxe-test-disk-XXXXXX";
>  
> -static void test_pxe_one(const char *params, bool ipv6)
> +typedef struct testdef {
> +    const char *machine;    /* Machine type */
> +    const char *model;      /* NIC device model (human readable)*/
> +    const char *extra;      /* Extra parameters, overriding default */
> +} testdef_t;

Not sure whether it's nicer, but you could also add a "is_slow" field to
the struct and then merge the fast and slow tables below...?

> +static testdef_t x86_tests[] = {
> +    { "pc", "e1000" },
> +    { "pc", "virtio", "-device virtio-net-pci,netdev=" NETNAME },

I think I'd rather rather use "virtio-net-pci" as model name here and
drop the "extra" parameter.

> +    { NULL },
> +};
> +
> +static testdef_t x86_tests_slow[] = {
> +    { "pc", "ne2000", "-device ne2k_pci,netdev=" NETNAME },
> +    { "pc", "eepro100", "-device i82550,netdev=" NETNAME },

dito.

> +    { "pc", "rtl8139" },
> +    { "pc", "vmxnet3" },
> +    { NULL },
> +};
> +
> +static testdef_t ppc64_tests[] = {
> +    { "pseries", "spapr-vlan" },
> +    { NULL },
> +};
> +
> +static testdef_t ppc64_tests_slow[] = {
> +    { "pseries", "virtio", "-device virtio-net-pci,netdev=" NETNAME },

dito.

> +    { "pseries", "e1000" },
> +    { NULL },
> +};
> +
> +static testdef_t s390x_tests[] = {
> +    { "s390-ccw-virtio", "virtio-ccw",
> +      "-device virtio-net-ccw,bootindex=1,netdev=" NETNAME },
> +    { NULL },
> +};
> +
> +static void test_pxe_one(const testdef_t *test, bool ipv6)
>  {
>      char *args;
> +    char *defextra;
> +    const char *extra = test->extra;
> +
> +    defextra = g_strdup_printf("-device %s,netdev=" NETNAME, test->model);
> +    if (!extra) {
> +        extra = defextra;
> +    }

I'd be nicer to do the g_strdup_printf() only if it is really necessary,
e.g.:

    const char *extra = test->extra;

    if (!extra) {
        extra = g_strdup_printf(...);
    }

> -    args = g_strdup_printf("-machine accel=kvm:tcg -nodefaults -boot order=n "
> -                           "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s,"
> -                           "ipv4=%s,ipv6=%s %s", disk, ipv6 ? "off" : "on",
> -                           ipv6 ? "on" : "off", params);
> +    args = g_strdup_printf(
> +        "-machine %s,accel=kvm:tcg -nodefaults -boot order=n "
> +        "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s,ipv4=%s,ipv6=%s %s",
> +        test->machine, disk, ipv6 ? "off" : "on", ipv6 ? "on" : "off", extra);
>  
>      qtest_start(args);
>      boot_sector_test();
>      qtest_quit(global_qtest);
>      g_free(args);
> +    g_free(defextra);

... and then:

    if (!test->extra) {
        g_free(extra);
    }

>  }
>  
>  static void test_pxe_ipv4(gconstpointer data)
>  {
> -    const char *model = data;
> -    char *dev_arg;
> +    const testdef_t *test = data;
> +
> +    test_pxe_one(test, false);
> +}
>  
> -    dev_arg = g_strdup_printf("-device %s,netdev=" NETNAME, model);
> -    test_pxe_one(dev_arg, false);
> -    g_free(dev_arg);
> +static void test_batch(const testdef_t *tests)
> +{
> +    int i;
> +
> +    for (i = 0; tests[i].machine; i++) {
> +        const testdef_t *test = &tests[i];
> +        char *testname;
> +
> +        testname = g_strdup_printf("pxe/ipv4/%s/%s",
> +                                   test->machine, test->model);
> +        qtest_add_data_func(testname, test, test_pxe_ipv4);
> +        g_free(testname);
> +    }
>  }
>  
>  int main(int argc, char *argv[])
> @@ -59,24 +116,17 @@ int main(int argc, char *argv[])
>      g_test_init(&argc, &argv, NULL);
>  
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> -        qtest_add_data_func("pxe/e1000", "e1000", test_pxe_ipv4);
> -        qtest_add_data_func("pxe/virtio", "virtio-net-pci", test_pxe_ipv4);
> +        test_batch(x86_tests);
>          if (g_test_slow()) {
> -            qtest_add_data_func("pxe/ne2000", "ne2k_pci", test_pxe_ipv4);
> -            qtest_add_data_func("pxe/eepro100", "i82550", test_pxe_ipv4);
> -            qtest_add_data_func("pxe/pcnet", "pcnet", test_pxe_ipv4);
> -            qtest_add_data_func("pxe/rtl8139", "rtl8139", test_pxe_ipv4);
> -            qtest_add_data_func("pxe/vmxnet3", "vmxnet3", test_pxe_ipv4);
> +            test_batch(x86_tests_slow);
>          }
>      } else if (strcmp(arch, "ppc64") == 0) {
> -        qtest_add_data_func("pxe/spapr-vlan", "spapr-vlan", test_pxe_ipv4);
> +        test_batch(ppc64_tests);
>          if (g_test_slow()) {
> -            qtest_add_data_func("pxe/virtio", "virtio-net-pci", test_pxe_ipv4);
> -            qtest_add_data_func("pxe/e1000", "e1000", test_pxe_ipv4);
> +            test_batch(ppc64_tests_slow);
>          }
>      } else if (g_str_equal(arch, "s390x")) {
> -        qtest_add_data_func("pxe/virtio-ccw",
> -                            "virtio-net-ccw,bootindex=1", test_pxe_ipv4);
> +        test_batch(s390x_tests);
>      }
>      ret = g_test_run();
>      boot_sector_cleanup(disk);
> 

 Thomas

Re: [Qemu-devel] [PATCH 3/4] tests/pxe-test: Test net booting over IPv6 in some cases

[Thomas Huth]

On 15.12.2017 11:16, David Gibson wrote:
> This adds IPv6 net boot testing (in addition to IPv4) when in slow test
> mode on ppc64 or s390.  IPv6 PXE doesn't seem to work on x86, I'm guessing
> out BIOS image doesn't support it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tests/pxe-test.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
[...]
>      } else if (strcmp(arch, "ppc64") == 0) {
> -        test_batch(ppc64_tests);
> +        test_batch(ppc64_tests, g_test_slow());
>          if (g_test_slow()) {
> -            test_batch(ppc64_tests_slow);
> +            test_batch(ppc64_tests_slow, true);
>          }

The NICs from ppc64_tests_slow are now never exercised with IPv4 ...
maybe it would be better to do both:

            test_batch(ppc64_tests_slow, false);
            test_batch(ppc64_tests_slow, true);

?

 Thomas

Re: [Qemu-devel] [PATCH 2/4] tests/pxe-test: Use table of testcases rather than open-coding

[David Gibson]

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

On Fri, Dec 15, 2017 at 12:08:13PM +0100, Thomas Huth wrote:
> On 15.12.2017 11:16, David Gibson wrote:
> > Currently pxe-tests open codes the list of tests for each architecture.
> > This changes it to use tables of test parameters, somewhat similar to
> > boot-serial-test.
> > 
> > This adds the machine type into the table as well, giving us the ability
> > to perform tests on multiple machine types for architectures where there's
> > more than one machine type that matters.
> > 
> > NOTE: This changes the names of the tests in the output, to include the
> >       machine type and IPv4 vs. IPv6.  I'm not sure if this has the
> >       potential to break existing tooling.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tests/pxe-test.c | 94 +++++++++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 72 insertions(+), 22 deletions(-)
> > 
> > diff --git a/tests/pxe-test.c b/tests/pxe-test.c
> > index eb70aa2bc6..f9bca8976d 100644
> > --- a/tests/pxe-test.c
> > +++ b/tests/pxe-test.c
> > @@ -22,29 +22,86 @@
> >  
> >  static char disk[] = "tests/pxe-test-disk-XXXXXX";
> >  
> > -static void test_pxe_one(const char *params, bool ipv6)
> > +typedef struct testdef {
> > +    const char *machine;    /* Machine type */
> > +    const char *model;      /* NIC device model (human readable)*/
> > +    const char *extra;      /* Extra parameters, overriding default */
> > +} testdef_t;
> 
> Not sure whether it's nicer, but you could also add a "is_slow" field to
> the struct and then merge the fast and slow tables below...?

I had a draft version like that.  I thought the separate tables was a
bit nicer than having a bunch of true/false entries in the table that
you have to look elsewhere to interpret.

> > +static testdef_t x86_tests[] = {
> > +    { "pc", "e1000" },
> > +    { "pc", "virtio", "-device virtio-net-pci,netdev=" NETNAME },
> 
> I think I'd rather rather use "virtio-net-pci" as model name here and
> drop the "extra" parameter.

I guesss.  Originally I was trying to maintain the test names, but now
that I've ended up changing them for other reasons, I guess that
doesn't matter.

> > +    { NULL },
> > +};
> > +
> > +static testdef_t x86_tests_slow[] = {
> > +    { "pc", "ne2000", "-device ne2k_pci,netdev=" NETNAME },
> > +    { "pc", "eepro100", "-device i82550,netdev=" NETNAME },
> 
> dito.
> 
> > +    { "pc", "rtl8139" },
> > +    { "pc", "vmxnet3" },
> > +    { NULL },
> > +};
> > +
> > +static testdef_t ppc64_tests[] = {
> > +    { "pseries", "spapr-vlan" },
> > +    { NULL },
> > +};
> > +
> > +static testdef_t ppc64_tests_slow[] = {
> > +    { "pseries", "virtio", "-device virtio-net-pci,netdev=" NETNAME },
> 
> dito.
> 
> > +    { "pseries", "e1000" },
> > +    { NULL },
> > +};
> > +
> > +static testdef_t s390x_tests[] = {
> > +    { "s390-ccw-virtio", "virtio-ccw",
> > +      "-device virtio-net-ccw,bootindex=1,netdev=" NETNAME },
> > +    { NULL },
> > +};
> > +
> > +static void test_pxe_one(const testdef_t *test, bool ipv6)
> >  {
> >      char *args;
> > +    char *defextra;
> > +    const char *extra = test->extra;
> > +
> > +    defextra = g_strdup_printf("-device %s,netdev=" NETNAME, test->model);
> > +    if (!extra) {
> > +        extra = defextra;
> > +    }
> 
> I'd be nicer to do the g_strdup_printf() only if it is really necessary,
> e.g.:

Well, I was trying to avoid having conditionals at the end as well as
the beginning.  But I just realised g_free(NULL) is safe, so that can
still be done.  I'll update.

>     const char *extra = test->extra;
> 
>     if (!extra) {
>         extra = g_strdup_printf(...);
>     }
> 
> > -    args = g_strdup_printf("-machine accel=kvm:tcg -nodefaults -boot order=n "
> > -                           "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s,"
> > -                           "ipv4=%s,ipv6=%s %s", disk, ipv6 ? "off" : "on",
> > -                           ipv6 ? "on" : "off", params);
> > +    args = g_strdup_printf(
> > +        "-machine %s,accel=kvm:tcg -nodefaults -boot order=n "
> > +        "-netdev user,id=" NETNAME ",tftp=./,bootfile=%s,ipv4=%s,ipv6=%s %s",
> > +        test->machine, disk, ipv6 ? "off" : "on", ipv6 ? "on" : "off", extra);
> >  
> >      qtest_start(args);
> >      boot_sector_test();
> >      qtest_quit(global_qtest);
> >      g_free(args);
> > +    g_free(defextra);
> 
> ... and then:
> 
>     if (!test->extra) {
>         g_free(extra);
>     }
> 
> >  }
> >  
> >  static void test_pxe_ipv4(gconstpointer data)
> >  {
> > -    const char *model = data;
> > -    char *dev_arg;
> > +    const testdef_t *test = data;
> > +
> > +    test_pxe_one(test, false);
> > +}
> >  
> > -    dev_arg = g_strdup_printf("-device %s,netdev=" NETNAME, model);
> > -    test_pxe_one(dev_arg, false);
> > -    g_free(dev_arg);
> > +static void test_batch(const testdef_t *tests)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; tests[i].machine; i++) {
> > +        const testdef_t *test = &tests[i];
> > +        char *testname;
> > +
> > +        testname = g_strdup_printf("pxe/ipv4/%s/%s",
> > +                                   test->machine, test->model);
> > +        qtest_add_data_func(testname, test, test_pxe_ipv4);
> > +        g_free(testname);
> > +    }
> >  }
> >  
> >  int main(int argc, char *argv[])
> > @@ -59,24 +116,17 @@ int main(int argc, char *argv[])
> >      g_test_init(&argc, &argv, NULL);
> >  
> >      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> > -        qtest_add_data_func("pxe/e1000", "e1000", test_pxe_ipv4);
> > -        qtest_add_data_func("pxe/virtio", "virtio-net-pci", test_pxe_ipv4);
> > +        test_batch(x86_tests);
> >          if (g_test_slow()) {
> > -            qtest_add_data_func("pxe/ne2000", "ne2k_pci", test_pxe_ipv4);
> > -            qtest_add_data_func("pxe/eepro100", "i82550", test_pxe_ipv4);
> > -            qtest_add_data_func("pxe/pcnet", "pcnet", test_pxe_ipv4);
> > -            qtest_add_data_func("pxe/rtl8139", "rtl8139", test_pxe_ipv4);
> > -            qtest_add_data_func("pxe/vmxnet3", "vmxnet3", test_pxe_ipv4);
> > +            test_batch(x86_tests_slow);
> >          }
> >      } else if (strcmp(arch, "ppc64") == 0) {
> > -        qtest_add_data_func("pxe/spapr-vlan", "spapr-vlan", test_pxe_ipv4);
> > +        test_batch(ppc64_tests);
> >          if (g_test_slow()) {
> > -            qtest_add_data_func("pxe/virtio", "virtio-net-pci", test_pxe_ipv4);
> > -            qtest_add_data_func("pxe/e1000", "e1000", test_pxe_ipv4);
> > +            test_batch(ppc64_tests_slow);
> >          }
> >      } else if (g_str_equal(arch, "s390x")) {
> > -        qtest_add_data_func("pxe/virtio-ccw",
> > -                            "virtio-net-ccw,bootindex=1", test_pxe_ipv4);
> > +        test_batch(s390x_tests);
> >      }
> >      ret = g_test_run();
> >      boot_sector_cleanup(disk);
> > 
> 
>  Thomas
> 

-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 3/4] tests/pxe-test: Test net booting over IPv6 in some cases

[David Gibson]

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

On Fri, Dec 15, 2017 at 12:10:56PM +0100, Thomas Huth wrote:
> On 15.12.2017 11:16, David Gibson wrote:
> > This adds IPv6 net boot testing (in addition to IPv4) when in slow test
> > mode on ppc64 or s390.  IPv6 PXE doesn't seem to work on x86, I'm guessing
> > out BIOS image doesn't support it.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tests/pxe-test.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> [...]
> >      } else if (strcmp(arch, "ppc64") == 0) {
> > -        test_batch(ppc64_tests);
> > +        test_batch(ppc64_tests, g_test_slow());
> >          if (g_test_slow()) {
> > -            test_batch(ppc64_tests_slow);
> > +            test_batch(ppc64_tests_slow, true);
> >          }
> 
> The NICs from ppc64_tests_slow are now never exercised with IPv4 ...
> maybe it would be better to do both:

Yes they are: the ipv6 parameter to test_batch() makes it do IPv6
tests in addition to IPv4, unlike test_pxe_one() which just does one
or the other.

>             test_batch(ppc64_tests_slow, false);
>             test_batch(ppc64_tests_slow, true);



-- 
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: 833 bytes --]

Re: [Qemu-devel] [PATCH 4/4] tests/pxe-test: Add some extra tests

[David Gibson]

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

On Fri, Dec 15, 2017 at 11:51:18AM +0100, Thomas Huth wrote:
> On 15.12.2017 11:16, David Gibson wrote:
> > Previously virtio-net was only tested for ppc64 in "slow" mode.  That
> > doesn't make much sense since virtio-net is used much more often in
> > practice than the spapr-vlan device which was tested always.  So, move
> > virtio-net to always be tested on ppc64.
> 
> Actually, I just moved virtio-net on ppc64 to the slow category a couple
> of months ago:
> 
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=ab06ec43577177a442e8e5
> 
> This has been done since some people complained that SLOF is incredibly
> slow in in TCG mode and thus there is the risk of hitting the timeout on
> slow systems if they are overloaded.

Hm.  If we're really concerned about the speed, I'm more inclined to
move spapr-vlan to slow than virtio-net.  virtio-net is now a much
more common case.

> Things will hopefully get a little bit better with the "Use
> tcg_gen_lookup_and_goto_ptr" patch that you've queued in your
> ppc-for-2.12 branch,

Just now merged, as it happens.

> but still, the test is very slow on ppc64, so I'd
> rather keep it in the "slow" category. (virtio-net is already tested in
> big endian mode on s390x anyway, so we at least got some endianess test
> coverage for that NIC already).

TBH, I see this as exercising the firmware netboot features more than
the device model.

> 
> > We had no tests at all for the q35 machine, which doesn't seem wise given
> > it's increasing prominence.  Add a couple of tests for it, including
> > testing the newer e1000e adapter.
> 
> That sounds like a good idea! I wonder whether we should also test some
> "old" machine versions (in "slow" mode only, of course), like "pc-0.12"
> for example, to make sure that there are no regressions with old
> machines?

Maybe.  Not really my department.
-- 
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: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH 3/4] tests/pxe-test: Test net booting over IPv6 in some cases

[Thomas Huth]

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

On 15.12.2017 13:56, David Gibson wrote:
> On Fri, Dec 15, 2017 at 12:10:56PM +0100, Thomas Huth wrote:
>> On 15.12.2017 11:16, David Gibson wrote:
>>> This adds IPv6 net boot testing (in addition to IPv4) when in slow test
>>> mode on ppc64 or s390.  IPv6 PXE doesn't seem to work on x86, I'm guessing
>>> out BIOS image doesn't support it.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>>  tests/pxe-test.c | 26 ++++++++++++++++++++------
>>>  1 file changed, 20 insertions(+), 6 deletions(-)
>> [...]
>>>      } else if (strcmp(arch, "ppc64") == 0) {
>>> -        test_batch(ppc64_tests);
>>> +        test_batch(ppc64_tests, g_test_slow());
>>>          if (g_test_slow()) {
>>> -            test_batch(ppc64_tests_slow);
>>> +            test_batch(ppc64_tests_slow, true);
>>>          }
>>
>> The NICs from ppc64_tests_slow are now never exercised with IPv4 ...
>> maybe it would be better to do both:
> 
> Yes they are: the ipv6 parameter to test_batch() makes it do IPv6
> tests in addition to IPv4, unlike test_pxe_one() which just does one
> or the other.

Ah, right, my bad, I mixed that up. And since we do not have any machine
that can only do IPv6 but not IPv4, it's fine that test_batch() does
both, so never mind, please ignore my above comment.

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]