[PATCH for 7.1 v1 0/6] testing fixes and doc tweak pre-PR

[PATCH for 7.1 v1 0/6] testing fixes and doc tweak pre-PR

[Alex Bennée]

Hi,

This is a small subset of testing fixes to improve the stability of
the CI. The first ensure all jobs have a timeout (default 120s) and
then we have Thomas'  excellent work to speed up the migration test
which was the source of a number of gitlab timeouts. Finally a little
bit of extra manual text to point users in the right direction when
defining block devices.

Only one patch needs review before the PR:

 - tests/avocado: push default timeout to QemuBaseTest
   
Alex Bennée (2):
  tests/avocado: push default timeout to QemuBaseTest
  qemu-options: try and clarify preferred block semantics

Thomas Huth (4):
  tests/qtest/migration-test: Only wait for serial output where
    migration succeeds
  tests/migration/aarch64: Speed up the aarch64 migration test
  tests/migration/i386: Speed up the i386 migration test (when using
    TCG)
  tests/qtest/migration-test: Remove duplicated test_postcopy from the
    test plan

 tests/migration/aarch64/a-b-kernel.h   | 10 +++++-----
 tests/migration/i386/a-b-bootblock.h   | 12 ++++++------
 tests/qtest/migration-test.c           |  5 +++--
 qemu-options.hx                        | 13 +++++++++++++
 tests/avocado/avocado_qemu/__init__.py |  5 ++++-
 tests/migration/aarch64/a-b-kernel.S   |  3 +--
 tests/migration/i386/a-b-bootblock.S   |  1 +
 7 files changed, 33 insertions(+), 16 deletions(-)

-- 
2.30.2

[PATCH v1 1/6] tests/avocado: push default timeout to QemuBaseTest

[Alex Bennée]

All of the QEMU tests eventually end up derrived from this class. Move
the default timeout from LinuxTest to ensure we catch them all. As 15
minutes is fairly excessive we drop the default down to 2 minutes
which is a more reasonable target for tests to aim for.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Message-Id: <20220816133831.2166761-1-alex.bennee@linaro.org>

---
v2
  - lower timeout to 2 minutes/120 seconds
---
 tests/avocado/avocado_qemu/__init__.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tests/avocado/avocado_qemu/__init__.py b/tests/avocado/avocado_qemu/__init__.py
index ed4853c805..0efd2bd212 100644
--- a/tests/avocado/avocado_qemu/__init__.py
+++ b/tests/avocado/avocado_qemu/__init__.py
@@ -227,6 +227,10 @@ def exec_command_and_wait_for_pattern(test, command,
     _console_interaction(test, success_message, failure_message, command + '\r')
 
 class QemuBaseTest(avocado.Test):
+
+    # default timeout for all tests, can be overridden
+    timeout = 120
+
     def _get_unique_tag_val(self, tag_name):
         """
         Gets a tag value, if unique for a key
@@ -512,7 +516,6 @@ class LinuxTest(LinuxSSHMixIn, QemuSystemTest):
     to start with than the more vanilla `QemuSystemTest` class.
     """
 
-    timeout = 900
     distro = None
     username = 'root'
     password = 'password'
-- 
2.30.2

Re: [PATCH v1 1/6] tests/avocado: push default timeout to QemuBaseTest

[Richard Henderson]

On 8/22/22 09:56, Alex Bennée wrote:
> All of the QEMU tests eventually end up derrived from this class. Move
> the default timeout from LinuxTest to ensure we catch them all. As 15
> minutes is fairly excessive we drop the default down to 2 minutes
> which is a more reasonable target for tests to aim for.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20220816133831.2166761-1-alex.bennee@linaro.org>
> 
> ---
> v2
>    - lower timeout to 2 minutes/120 seconds

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [PATCH v1 1/6] tests/avocado: push default timeout to QemuBaseTest

[Philippe Mathieu-Daudé via]

On 22/8/22 18:56, Alex Bennée wrote:
> All of the QEMU tests eventually end up derrived from this class. Move

Typo "derived".

> the default timeout from LinuxTest to ensure we catch them all. As 15
> minutes is fairly excessive we drop the default down to 2 minutes
> which is a more reasonable target for tests to aim for.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Message-Id: <20220816133831.2166761-1-alex.bennee@linaro.org>
> 
> ---
> v2
>    - lower timeout to 2 minutes/120 seconds
> ---
>   tests/avocado/avocado_qemu/__init__.py | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v1 2/6] tests/qtest/migration-test: Only wait for serial output where migration succeeds

[Alex Bennée]

From: Thomas Huth <thuth@redhat.com>

Waiting for the serial output can take a couple of seconds - and since
we're doing a lot of migration tests, this time easily sums up to
multiple minutes. But if a test is supposed to fail, it does not make
much sense to wait for the source to be in the right state first, so
we can skip the waiting here. This way we can speed up all tests where
the migration is supposed to fail. In the gitlab-CI gprov-gcov test,
each of the migration-tests now run two minutes faster!

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
Message-Id: <20220819053802.296584-2-thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/migration-test.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 520a5f917c..7be321b62d 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1307,7 +1307,9 @@ static void test_precopy_common(MigrateCommon *args)
     }
 
     /* Wait for the first serial output from the source */
-    wait_for_serial("src_serial");
+    if (args->result == MIG_TEST_SUCCEED) {
+        wait_for_serial("src_serial");
+    }
 
     if (!args->connect_uri) {
         g_autofree char *local_connect_uri =
-- 
2.30.2

Re: [PATCH v1 2/6] tests/qtest/migration-test: Only wait for serial output where migration succeeds

[Juan Quintela]

Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Thomas Huth <thuth@redhat.com>
>
> Waiting for the serial output can take a couple of seconds - and since
> we're doing a lot of migration tests, this time easily sums up to
> multiple minutes. But if a test is supposed to fail, it does not make
> much sense to wait for the source to be in the right state first, so
> we can skip the waiting here. This way we can speed up all tests where
> the migration is supposed to fail. In the gitlab-CI gprov-gcov test,
> each of the migration-tests now run two minutes faster!
>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> Message-Id: <20220819053802.296584-2-thuth@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH v1 3/6] tests/migration/aarch64: Speed up the aarch64 migration test

[Alex Bennée]

From: Thomas Huth <thuth@redhat.com>

The migration tests spend a lot of time waiting for a sign of live
of the guest on the serial console. The aarch64 migration code only
outputs "B"s every couple of seconds (at least it takes more than 4
seconds between each characeter on my x86 laptop). There are a lot
of migration tests, and if each test that checks for a successful
migration waits for these characters before and after migration, the
wait time sums up to multiple minutes! Let's use a shorter delay to
speed things up.

While we're at it, also remove a superfluous masking with 0xff - we're
reading and storing bytes, so the upper bits of the register do not
matter anyway.

With these changes, the test runs twice as fast on my laptop, decreasing
the total run time from approx. 8 minutes to only 4 minutes!

Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20220819053802.296584-3-thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/migration/aarch64/a-b-kernel.h | 10 +++++-----
 tests/migration/aarch64/a-b-kernel.S |  3 +--
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/tests/migration/aarch64/a-b-kernel.h b/tests/migration/aarch64/a-b-kernel.h
index 0a9b01137e..34e518d061 100644
--- a/tests/migration/aarch64/a-b-kernel.h
+++ b/tests/migration/aarch64/a-b-kernel.h
@@ -10,9 +10,9 @@ unsigned char aarch64_kernel[] = {
   0x03, 0x00, 0x80, 0x52, 0xe4, 0x03, 0x00, 0xaa, 0x83, 0x00, 0x00, 0x39,
   0x84, 0x04, 0x40, 0x91, 0x9f, 0x00, 0x01, 0xeb, 0xad, 0xff, 0xff, 0x54,
   0x05, 0x00, 0x80, 0x52, 0xe4, 0x03, 0x00, 0xaa, 0x83, 0x00, 0x40, 0x39,
-  0x63, 0x04, 0x00, 0x11, 0x63, 0x1c, 0x00, 0x12, 0x83, 0x00, 0x00, 0x39,
-  0x24, 0x7e, 0x0b, 0xd5, 0x84, 0x04, 0x40, 0x91, 0x9f, 0x00, 0x01, 0xeb,
-  0x2b, 0xff, 0xff, 0x54, 0xa5, 0x04, 0x00, 0x11, 0xa5, 0x1c, 0x00, 0x12,
-  0xbf, 0x00, 0x00, 0x71, 0x81, 0xfe, 0xff, 0x54, 0x43, 0x08, 0x80, 0x52,
-  0x43, 0x00, 0x00, 0x39, 0xf1, 0xff, 0xff, 0x17
+  0x63, 0x04, 0x00, 0x11, 0x83, 0x00, 0x00, 0x39, 0x24, 0x7e, 0x0b, 0xd5,
+  0x84, 0x04, 0x40, 0x91, 0x9f, 0x00, 0x01, 0xeb, 0x4b, 0xff, 0xff, 0x54,
+  0xa5, 0x04, 0x00, 0x11, 0xa5, 0x10, 0x00, 0x12, 0xbf, 0x00, 0x00, 0x71,
+  0xa1, 0xfe, 0xff, 0x54, 0x43, 0x08, 0x80, 0x52, 0x43, 0x00, 0x00, 0x39,
+  0xf2, 0xff, 0xff, 0x17
 };
diff --git a/tests/migration/aarch64/a-b-kernel.S b/tests/migration/aarch64/a-b-kernel.S
index 0225945348..a4103ecb71 100644
--- a/tests/migration/aarch64/a-b-kernel.S
+++ b/tests/migration/aarch64/a-b-kernel.S
@@ -53,7 +53,6 @@ innerloop:
         /* increment the first byte of each page by 1 */
         ldrb    w3, [x4]
         add     w3, w3, #1
-        and     w3, w3, #0xff
         strb    w3, [x4]
 
         /* make sure QEMU user space can see consistent data as MMU is off */
@@ -64,7 +63,7 @@ innerloop:
         blt     innerloop
 
         add     w5, w5, #1
-        and     w5, w5, #0xff
+        and     w5, w5, #0x1f
         cmp     w5, #0
         bne     mainloop
 
-- 
2.30.2

[PATCH v1 4/6] tests/migration/i386: Speed up the i386 migration test (when using TCG)

[Alex Bennée]

From: Thomas Huth <thuth@redhat.com>

When KVM is not available, the i386 migration test also runs in a rather
slow fashion, since the guest code takes a couple of seconds to print
the "B"s on the serial console, and the migration test has to wait for
this each time. Let's increase the frequency here, too, so that the
delays in the migration tests get smaller.

Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20220819053802.296584-4-thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/migration/i386/a-b-bootblock.h | 12 ++++++------
 tests/migration/i386/a-b-bootblock.S |  1 +
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/tests/migration/i386/a-b-bootblock.h b/tests/migration/i386/a-b-bootblock.h
index 7d459d4fde..b7b0fce2ee 100644
--- a/tests/migration/i386/a-b-bootblock.h
+++ b/tests/migration/i386/a-b-bootblock.h
@@ -4,17 +4,17 @@
  * the header and the assembler differences in your patch submission.
  */
 unsigned char x86_bootsect[] = {
-  0xfa, 0x0f, 0x01, 0x16, 0x74, 0x7c, 0x66, 0xb8, 0x01, 0x00, 0x00, 0x00,
+  0xfa, 0x0f, 0x01, 0x16, 0x78, 0x7c, 0x66, 0xb8, 0x01, 0x00, 0x00, 0x00,
   0x0f, 0x22, 0xc0, 0x66, 0xea, 0x20, 0x7c, 0x00, 0x00, 0x08, 0x00, 0x00,
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xe4, 0x92, 0x0c, 0x02,
   0xe6, 0x92, 0xb8, 0x10, 0x00, 0x00, 0x00, 0x8e, 0xd8, 0x66, 0xb8, 0x41,
   0x00, 0x66, 0xba, 0xf8, 0x03, 0xee, 0xb3, 0x00, 0xb8, 0x00, 0x00, 0x10,
   0x00, 0xfe, 0x00, 0x05, 0x00, 0x10, 0x00, 0x00, 0x3d, 0x00, 0x00, 0x40,
-  0x06, 0x7c, 0xf2, 0xfe, 0xc3, 0x75, 0xe9, 0x66, 0xb8, 0x42, 0x00, 0x66,
-  0xba, 0xf8, 0x03, 0xee, 0xeb, 0xde, 0x66, 0x90, 0x00, 0x00, 0x00, 0x00,
-  0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0x00, 0x00, 0x00, 0x9a, 0xcf, 0x00,
-  0xff, 0xff, 0x00, 0x00, 0x00, 0x92, 0xcf, 0x00, 0x27, 0x00, 0x5c, 0x7c,
-  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+  0x06, 0x7c, 0xf2, 0xfe, 0xc3, 0x80, 0xe3, 0x3f, 0x75, 0xe6, 0x66, 0xb8,
+  0x42, 0x00, 0x66, 0xba, 0xf8, 0x03, 0xee, 0xeb, 0xdb, 0x8d, 0x76, 0x00,
+  0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0xff, 0x00, 0x00,
+  0x00, 0x9a, 0xcf, 0x00, 0xff, 0xff, 0x00, 0x00, 0x00, 0x92, 0xcf, 0x00,
+  0x27, 0x00, 0x60, 0x7c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
diff --git a/tests/migration/i386/a-b-bootblock.S b/tests/migration/i386/a-b-bootblock.S
index 3f97f28023..3d464c7568 100644
--- a/tests/migration/i386/a-b-bootblock.S
+++ b/tests/migration/i386/a-b-bootblock.S
@@ -50,6 +50,7 @@ innerloop:
         jl innerloop
 
         inc %bl
+        andb $0x3f,%bl
         jnz mainloop
 
         mov $66,%ax
-- 
2.30.2

[PATCH v1 5/6] tests/qtest/migration-test: Remove duplicated test_postcopy from the test plan

[Alex Bennée]

From: Thomas Huth <thuth@redhat.com>

test_postcopy() is currently run twice - which is just a waste of resources
and time. The commit d1a27b169b2d that introduced the duplicate talked about
renaming the "postcopy/unix" test, but apparently it forgot to remove the
old entry. Let's do that now.

Fixes: d1a27b169b ("tests: Add postcopy tls migration test")
Signed-off-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Message-Id: <20220819053802.296584-5-thuth@redhat.com>
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 tests/qtest/migration-test.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 7be321b62d..f63edd0bc8 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2461,7 +2461,6 @@ int main(int argc, char **argv)
     module_call_init(MODULE_INIT_QOM);
 
     if (has_uffd) {
-        qtest_add_func("/migration/postcopy/unix", test_postcopy);
         qtest_add_func("/migration/postcopy/plain", test_postcopy);
         qtest_add_func("/migration/postcopy/recovery/plain",
                        test_postcopy_recovery);
-- 
2.30.2

Re: [PATCH v1 5/6] tests/qtest/migration-test: Remove duplicated test_postcopy from the test plan

[Juan Quintela]

Alex Bennée <alex.bennee@linaro.org> wrote:
> From: Thomas Huth <thuth@redhat.com>
>
> test_postcopy() is currently run twice - which is just a waste of resources
> and time. The commit d1a27b169b2d that introduced the duplicate talked about
> renaming the "postcopy/unix" test, but apparently it forgot to remove the
> old entry. Let's do that now.
>
> Fixes: d1a27b169b ("tests: Add postcopy tls migration test")
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Message-Id: <20220819053802.296584-5-thuth@redhat.com>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

Reviewed-by: Juan Quintela <quintela@redhat.com>

[PATCH v1 6/6] qemu-options: try and clarify preferred block semantics

[Alex Bennée]

Try to correct any confusion about QEMU's Byzantine disk options by
laying out the preferred "modern" options as-per:

 "<danpb> (best:  -device + -blockdev,  2nd obsolete syntax: -device +
     -drive,  3rd obsolete syntax: -drive, 4th obsolete syntax: -hdNN)"

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Acked-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Cc: qemu-block@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Hanna Reitz <hreitz@redhat.com>
Cc: Thomas Huth <thuth@redhat.com>

---
v2
  - minor punctuation and flow fixes
---
 qemu-options.hx | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 3f23a42fa8..31c04f7eea 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1105,6 +1105,19 @@ DEFHEADING()
 
 DEFHEADING(Block device options:)
 
+SRST
+The QEMU block device handling options have a long history and
+have gone through several iterations as the feature set and complexity
+of the block layer have grown. Many online guides to QEMU often
+reference older and deprecated options, which can lead to confusion.
+
+The recommended modern way to describe disks is to use a combination of
+``-device`` to specify the hardware device and ``-blockdev`` to
+describe the backend. The device defines what the guest sees and the
+backend describes how QEMU handles the data.
+
+ERST
+
 DEF("fda", HAS_ARG, QEMU_OPTION_fda,
     "-fda/-fdb file  use 'file' as floppy disk 0/1 image\n", QEMU_ARCH_ALL)
 DEF("fdb", HAS_ARG, QEMU_OPTION_fdb, "", QEMU_ARCH_ALL)
-- 
2.30.2