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

[Qemu-devel] [PATCHv2 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.

Changes since v1:
  * Simplified handling of device model options, at the expense of
    further changes to the test names
  * Assorted minor changes based on review
  
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 | 100 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 75 insertions(+), 25 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCHv2 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>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---
 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] [PATCHv2 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 | 87 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 65 insertions(+), 22 deletions(-)

diff --git a/tests/pxe-test.c b/tests/pxe-test.c
index eb70aa2bc6..8c310a8129 100644
--- a/tests/pxe-test.c
+++ b/tests/pxe-test.c
@@ -22,14 +22,52 @@
 
 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 */
+    const char *devopts;    /* Device options */
+} testdef_t;
+
+static testdef_t x86_tests[] = {
+    { "pc", "e1000" },
+    { "pc", "virtio-net-pci" },
+    { NULL },
+};
+
+static testdef_t x86_tests_slow[] = {
+    { "pc", "ne2k_pci", },
+    { "pc", "i82550", },
+    { "pc", "rtl8139" },
+    { "pc", "vmxnet3" },
+    { NULL },
+};
+
+static testdef_t ppc64_tests[] = {
+    { "pseries", "spapr-vlan" },
+    { NULL },
+};
+
+static testdef_t ppc64_tests_slow[] = {
+    { "pseries", "virtio-net-pci", },
+    { "pseries", "e1000" },
+    { NULL },
+};
+
+static testdef_t s390x_tests[] = {
+    { "s390-ccw-virtio", "virtio-net-ccw", ",bootindex=1" },
+    { NULL },
+};
+
+static void test_pxe_one(const testdef_t *test, bool ipv6)
 {
     char *args;
 
-    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 "
+        "-device %s%s,netdev=" NETNAME,
+        test->machine, disk, ipv6 ? "off" : "on", ipv6 ? "on" : "off",
+        test->model, test->devopts ? test->devopts : "");
 
     qtest_start(args);
     boot_sector_test();
@@ -39,12 +77,24 @@ static void test_pxe_one(const char *params, bool ipv6)
 
 static void test_pxe_ipv4(gconstpointer data)
 {
-    const char *model = data;
-    char *dev_arg;
+    const testdef_t *test = data;
 
-    dev_arg = g_strdup_printf("-device %s,netdev=" NETNAME, model);
-    test_pxe_one(dev_arg, false);
-    g_free(dev_arg);
+    test_pxe_one(test, false);
+}
+
+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 +109,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

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

[Thomas Huth]

On 18.12.2017 11:04, 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 | 87 ++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 65 insertions(+), 22 deletions(-)
> 
> diff --git a/tests/pxe-test.c b/tests/pxe-test.c
> index eb70aa2bc6..8c310a8129 100644
> --- a/tests/pxe-test.c
> +++ b/tests/pxe-test.c
> @@ -22,14 +22,52 @@
>  
>  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 */
> +    const char *devopts;    /* Device options */
> +} testdef_t;
[...]
> +static testdef_t s390x_tests[] = {
> +    { "s390-ccw-virtio", "virtio-net-ccw", ",bootindex=1" },
> +    { NULL },
> +};
> +
> +static void test_pxe_one(const testdef_t *test, bool ipv6)
>  {
>      char *args;
>  
> -    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 "
> +        "-device %s%s,netdev=" NETNAME,
> +        test->machine, disk, ipv6 ? "off" : "on", ipv6 ? "on" : "off",
> +        test->model, test->devopts ? test->devopts : "");

Looking at this again, I think we could also simply always add the
"bootindex=1" to the args string - this should not hurt, rather might
help to boot faster, and then you could get rid of the"devopts" field in
the testdef_t struct.

Anyway, with or without that modification:

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

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

[David Gibson]

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

On Mon, Dec 18, 2017 at 11:34:07AM +0100, Thomas Huth wrote:
> On 18.12.2017 11:04, 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 | 87 ++++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 65 insertions(+), 22 deletions(-)
> > 
> > diff --git a/tests/pxe-test.c b/tests/pxe-test.c
> > index eb70aa2bc6..8c310a8129 100644
> > --- a/tests/pxe-test.c
> > +++ b/tests/pxe-test.c
> > @@ -22,14 +22,52 @@
> >  
> >  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 */
> > +    const char *devopts;    /* Device options */
> > +} testdef_t;
> [...]
> > +static testdef_t s390x_tests[] = {
> > +    { "s390-ccw-virtio", "virtio-net-ccw", ",bootindex=1" },
> > +    { NULL },
> > +};
> > +
> > +static void test_pxe_one(const testdef_t *test, bool ipv6)
> >  {
> >      char *args;
> >  
> > -    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 "
> > +        "-device %s%s,netdev=" NETNAME,
> > +        test->machine, disk, ipv6 ? "off" : "on", ipv6 ? "on" : "off",
> > +        test->model, test->devopts ? test->devopts : "");
> 
> Looking at this again, I think we could also simply always add the
> "bootindex=1" to the args string - this should not hurt, rather might
> help to boot faster, and then you could get rid of the"devopts" field in
> the testdef_t struct.

Good idea, I've made that change.

> Anyway, with or without that modification:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

-- 
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 --]

[Qemu-devel] [PATCHv2 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
our 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 8c310a8129..18e02c246c 100644
--- a/tests/pxe-test.c
+++ b/tests/pxe-test.c
@@ -82,7 +82,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;
 
@@ -94,6 +101,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);
+        }
     }
 }
 
@@ -109,17 +123,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

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

[Thomas Huth]

On 18.12.2017 11:04, 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
> our 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(-)

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

[Qemu-devel] [PATCHv2 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 its 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 18e02c246c..009a82bb97 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-net-pci" },
+    { "q35", "e1000e" },
+    { "q35", "virtio-net-pci", },
     { NULL },
 };
 
@@ -44,11 +46,11 @@ static testdef_t x86_tests_slow[] = {
 
 static testdef_t ppc64_tests[] = {
     { "pseries", "spapr-vlan" },
+    { "pseries", "virtio-net-pci", },
     { NULL },
 };
 
 static testdef_t ppc64_tests_slow[] = {
-    { "pseries", "virtio-net-pci", },
     { "pseries", "e1000" },
     { NULL },
 };
-- 
2.14.3

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

[Thomas Huth]

On 18.12.2017 11:04, 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.

Assuming that the "Use tcg_gen_lookup_and_goto_ptr" patch and Paolo's
"optimize cmp translation" will give us enough speed-up in TCG mode:

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