[Qemu-devel] [PATCH 0/4] i440fx-test: check firmware visibility

[Qemu-devel] [PATCH 0/4] i440fx-test: check firmware visibility

[Laszlo Ersek]

Marcel's commit

  commit a53ae8e934cd54686875b5bcfc2f434244ee55d6
  Author: Marcel Apfelbaum <marcel.a@redhat.com>
  Date:   Mon Sep 16 11:21:16 2013 +0300

      hw/pci: partially handle pci master abort

has exposed a conflict (an unintended, unordered overlap) between the
memory ranges "pci-hole" and "system.flash". When the boot firmware is
passed with -pflash, the "pci-hole" region hides it, and the guest
cannot execute the firmware.

The test cases added by this series should help avoid regressions in
this area.

On top of v1.7.0-rc0, the "/i440fx/firmware/bios" test passes even
without fixing the memory region conflict (consistently with the fact
that we never noticed the conflict in practice due to using -bios
exclusively).

The "/i440fx/firmware/pflash" test catches the problem, unless one of
the discussed fixes are applied (ie. shrinking "pci-hole", or explicitly
ordering it under "system.flash"):

  $ tests/i440fx-test --verbose
  [...]
  GTest: run: /i440fx/firmware/bios
  (MSG: qemu cmdline: -S -display none -bios /tmp/fw_blob_MA3Y5W)
  GTest: result: OK
  GTest: run: /i440fx/firmware/pflash
  (MSG: qemu cmdline: -S -display none -pflash /tmp/fw_blob_ELLU5W)
  **
  ERROR:tests/i440fx-test.c:368:test_i440fx_firmware: assertion failed
  (buf[i] == (char unsigned)i): (0x000000ff == 0x00000000)
  Aborted

Laszlo Ersek (4):
  i440fx-test: qtest_start() should be paired with qtest_end()
  i440fx-test: give each GTest case its own qtest
  i440fx-test: generate temporary firmware blob
  i440fx-test: verify firmware under 4G and 1M, both -bios and -pflash

 tests/i440fx-test.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 152 insertions(+), 17 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/4] i440fx-test: qtest_start() should be paired with qtest_end()

[Laszlo Ersek]

Similarly to commit 1d9358e6
("libqtest: New qtest_end() to go with qtest_start()").

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 tests/i440fx-test.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index 08ce820..422f7be 100644
--- a/tests/i440fx-test.c
+++ b/tests/i440fx-test.c
@@ -2,9 +2,11 @@
  * qtest I440FX test case
  *
  * Copyright IBM, Corp. 2012-2013
+ * Copyright Red Hat, Inc. 2013
  *
  * Authors:
  *  Anthony Liguori   <aliguori@us.ibm.com>
+ *  Laszlo Ersek      <lersek@redhat.com>
  *
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
@@ -256,7 +258,6 @@ static void test_i440fx_pam(gconstpointer opaque)
 
 int main(int argc, char **argv)
 {
-    QTestState *s;
     TestData data;
     char *cmdline;
     int ret;
@@ -266,20 +267,17 @@ int main(int argc, char **argv)
     data.num_cpus = 1;
 
     cmdline = g_strdup_printf("-display none -smp %d", data.num_cpus);
-    s = qtest_start(cmdline);
+    qtest_start(cmdline);
     g_free(cmdline);
 
     data.bus = qpci_init_pc();
 
     g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults);
     g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam);
-    
 
     ret = g_test_run();
 
-    if (s) {
-        qtest_quit(s);
-    }
+    qtest_end();
 
     return ret;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/4] i440fx-test: give each GTest case its own qtest

[Laszlo Ersek]

The current two GTest cases, /i440fx/defaults and /i440fx/pam can share a
qemu process, but the next two cases will need dedicated instances. It is
messy (and order-dependent) to dynamically configure GTest cases one by
one to start, stop, or keep the current qtest (*); let's just have each
GTest work with its own qtest. The performance difference should be
negligible.

(*) As g_test_run() can be invoked at most once per process startup, and
it runs GTest cases in sequence, we'd need clumsy data structures to
control each GTest case to start/stop/keep the qemu instance. Or, we'd
have to code the same information into the test methods themselves, which
would make them even more order-dependent.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 tests/i440fx-test.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index 422f7be..dc81fb2 100644
--- a/tests/i440fx-test.c
+++ b/tests/i440fx-test.c
@@ -28,16 +28,27 @@
 typedef struct TestData
 {
     int num_cpus;
-    QPCIBus *bus;
 } TestData;
 
+static QPCIBus *test_start_get_bus(const TestData *s)
+{
+    char *cmdline;
+
+    cmdline = g_strdup_printf("-display none -smp %d", s->num_cpus);
+    qtest_start(cmdline);
+    g_free(cmdline);
+    return qpci_init_pc();
+}
+
 static void test_i440fx_defaults(gconstpointer opaque)
 {
     const TestData *s = opaque;
+    QPCIBus *bus;
     QPCIDevice *dev;
     uint32_t value;
 
-    dev = qpci_device_find(s->bus, QPCI_DEVFN(0, 0));
+    bus = test_start_get_bus(s);
+    dev = qpci_device_find(bus, QPCI_DEVFN(0, 0));
     g_assert(dev != NULL);
 
     /* 3.2.2 */
@@ -121,6 +132,8 @@ static void test_i440fx_defaults(gconstpointer opaque)
     g_assert_cmpint(qpci_config_readb(dev, 0x91), ==, 0x00); /* ERRSTS */
     /* 3.2.26 */
     g_assert_cmpint(qpci_config_readb(dev, 0x93), ==, 0x00); /* TRC */
+
+    qtest_end();
 }
 
 #define PAM_RE 1
@@ -179,6 +192,7 @@ static void write_area(uint32_t start, uint32_t end, uint8_t value)
 static void test_i440fx_pam(gconstpointer opaque)
 {
     const TestData *s = opaque;
+    QPCIBus *bus;
     QPCIDevice *dev;
     int i;
     static struct {
@@ -201,7 +215,8 @@ static void test_i440fx_pam(gconstpointer opaque)
         { 0xEC000, 0xEFFFF }, /* BIOS Extension */
     };
 
-    dev = qpci_device_find(s->bus, QPCI_DEVFN(0, 0));
+    bus = test_start_get_bus(s);
+    dev = qpci_device_find(bus, QPCI_DEVFN(0, 0));
     g_assert(dev != NULL);
 
     for (i = 0; i < ARRAY_SIZE(pam_area); i++) {
@@ -254,30 +269,21 @@ static void test_i440fx_pam(gconstpointer opaque)
         /* Verify the area is not our new mask */
         g_assert(!verify_area(pam_area[i].start, pam_area[i].end, 0x82));
     }
+    qtest_end();
 }
 
 int main(int argc, char **argv)
 {
     TestData data;
-    char *cmdline;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
     data.num_cpus = 1;
 
-    cmdline = g_strdup_printf("-display none -smp %d", data.num_cpus);
-    qtest_start(cmdline);
-    g_free(cmdline);
-
-    data.bus = qpci_init_pc();
-
     g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults);
     g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam);
 
     ret = g_test_run();
-
-    qtest_end();
-
     return ret;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/4] i440fx-test: generate temporary firmware blob

[Laszlo Ersek]

The blob is 64K in size and contains 0x00..0xFF repeatedly.

The client code added to main() wouldn't make much sense in the long term.
It helps with debugging and it silences gcc about create_firmware() being
unused, and we'll replace it in the next patch anyway.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 tests/i440fx-test.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index dc81fb2..1fe98de 100644
--- a/tests/i440fx-test.c
+++ b/tests/i440fx-test.c
@@ -20,6 +20,11 @@
 
 #include <glib.h>
 #include <string.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <errno.h>
+#include <sys/mman.h>
+#include <stdlib.h>
 
 #define BROKEN 1
 
@@ -272,13 +277,70 @@ static void test_i440fx_pam(gconstpointer opaque)
     qtest_end();
 }
 
+#define FW_SIZE ((size_t)65536)
+
+/* Create a temporary firmware blob, and return its absolute pathname as a
+ * dynamically allocated string.
+ * The file is closed before the function returns; it should be unlinked after
+ * use.
+ * In case of error, NULL is returned. The function prints the error message.
+ */
+static char *create_firmware(void)
+{
+    int ret, fd;
+    char *pathname;
+    GError *error = NULL;
+
+    ret = -1;
+    fd = g_file_open_tmp("fw_blob_XXXXXX", &pathname, &error);
+    if (fd == -1) {
+        fprintf(stderr, "unable to create temporary firmware blob: %s\n",
+                error->message);
+        g_error_free(error);
+    } else {
+        if (ftruncate(fd, FW_SIZE) == -1) {
+            fprintf(stderr, "ftruncate(\"%s\", %zu): %s\n", pathname, FW_SIZE,
+                    strerror(errno));
+        } else {
+            void *buf;
+
+            buf = mmap(NULL, FW_SIZE, PROT_WRITE, MAP_SHARED, fd, 0);
+            if (buf == MAP_FAILED) {
+                fprintf(stderr, "mmap(\"%s\", %zu): %s\n", pathname, FW_SIZE,
+                        strerror(errno));
+            } else {
+                size_t i;
+
+                for (i = 0; i < FW_SIZE; ++i) {
+                    ((char unsigned *)buf)[i] = i;
+                }
+                munmap(buf, FW_SIZE);
+                ret = 0;
+            }
+        }
+        close(fd);
+        if (ret == -1) {
+            unlink(pathname);
+            g_free(pathname);
+        }
+    }
+
+    return ret == -1 ? NULL : pathname;
+}
+
 int main(int argc, char **argv)
 {
+    char *fw_pathname;
     TestData data;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
+    fw_pathname = create_firmware();
+    g_assert(fw_pathname != NULL);
+    unlink(fw_pathname);
+    g_free(fw_pathname);
+
     data.num_cpus = 1;
 
     g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults);
-- 
1.8.3.1

[Qemu-devel] [PATCH 4/4] i440fx-test: verify firmware under 4G and 1M, both -bios and -pflash

[Laszlo Ersek]

Check whether the firmware is not hidden by other memory regions.

Qemu is started in paused mode: it shouldn't try to interpret generated
garbage.

Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 tests/i440fx-test.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 6 deletions(-)

diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index 1fe98de..43adcb8 100644
--- a/tests/i440fx-test.c
+++ b/tests/i440fx-test.c
@@ -35,6 +35,11 @@ typedef struct TestData
     int num_cpus;
 } TestData;
 
+typedef struct FirmwareTestFixture {
+    /* decides whether we're testing -bios or -pflash */
+    bool is_bios;
+} FirmwareTestFixture;
+
 static QPCIBus *test_start_get_bus(const TestData *s)
 {
     char *cmdline;
@@ -278,6 +283,7 @@ static void test_i440fx_pam(gconstpointer opaque)
 }
 
 #define FW_SIZE ((size_t)65536)
+#define ISA_BIOS_MAXSZ ((size_t)(128 * 1024))
 
 /* Create a temporary firmware blob, and return its absolute pathname as a
  * dynamically allocated string.
@@ -328,23 +334,86 @@ static char *create_firmware(void)
     return ret == -1 ? NULL : pathname;
 }
 
-int main(int argc, char **argv)
+static void test_i440fx_firmware(FirmwareTestFixture *fixture,
+                                 gconstpointer user_data)
 {
-    char *fw_pathname;
-    TestData data;
-    int ret;
-
-    g_test_init(&argc, &argv, NULL);
+    char *fw_pathname, *cmdline;
+    char unsigned *buf;
+    size_t i, isa_bios_size;
 
     fw_pathname = create_firmware();
     g_assert(fw_pathname != NULL);
+
+    /* Better hope the user didn't put metacharacters in TMPDIR and co. */
+    cmdline = g_strdup_printf("-S -display none %s %s",
+                              fixture->is_bios ? "-bios" : "-pflash",
+                              fw_pathname);
+    g_test_message("qemu cmdline: %s", cmdline);
+    qtest_start(cmdline);
+    g_free(cmdline);
+
+    /* Qemu has loaded the firmware (because qtest_start() only returns after
+     * the QMP handshake completes). We must unlink the firmware blob right
+     * here, because any assertion firing below would leak it in the
+     * filesystem. This is also the reason why we recreate the blob every time
+     * this function is invoked.
+     */
     unlink(fw_pathname);
     g_free(fw_pathname);
 
+    /* check below 4G */
+    buf = g_malloc0(FW_SIZE);
+    memread(0x100000000ULL - FW_SIZE, buf, FW_SIZE);
+    for (i = 0; i < FW_SIZE; ++i) {
+        g_assert_cmphex(buf[i], ==, (char unsigned)i);
+    }
+
+    /* check in ISA space too */
+    memset(buf, 0, FW_SIZE);
+    isa_bios_size = ISA_BIOS_MAXSZ < FW_SIZE ? ISA_BIOS_MAXSZ : FW_SIZE;
+    memread(0x100000 - isa_bios_size, buf, isa_bios_size);
+    for (i = 0; i < isa_bios_size; ++i) {
+        g_assert_cmphex(buf[i], ==,
+                        (char unsigned)((FW_SIZE - isa_bios_size) + i));
+    }
+
+    g_free(buf);
+    qtest_end();
+}
+
+static void add_firmware_test(const char *testpath,
+                              void (*setup_fixture)(FirmwareTestFixture *f,
+                                                    gconstpointer test_data))
+{
+    g_test_add(testpath, FirmwareTestFixture, NULL, setup_fixture,
+               test_i440fx_firmware, NULL);
+}
+
+static void request_bios(FirmwareTestFixture *fixture,
+                         gconstpointer user_data)
+{
+    fixture->is_bios = true;
+}
+
+static void request_pflash(FirmwareTestFixture *fixture,
+                           gconstpointer user_data)
+{
+    fixture->is_bios = false;
+}
+
+int main(int argc, char **argv)
+{
+    TestData data;
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
     data.num_cpus = 1;
 
     g_test_add_data_func("/i440fx/defaults", &data, test_i440fx_defaults);
     g_test_add_data_func("/i440fx/pam", &data, test_i440fx_pam);
+    add_firmware_test("/i440fx/firmware/bios", request_bios);
+    add_firmware_test("/i440fx/firmware/pflash", request_pflash);
 
     ret = g_test_run();
     return ret;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/4] i440fx-test: check firmware visibility

[Laszlo Ersek]

On 11/09/13 01:04, Laszlo Ersek wrote:

> Laszlo Ersek (4):
>   i440fx-test: qtest_start() should be paired with qtest_end()
>   i440fx-test: give each GTest case its own qtest
>   i440fx-test: generate temporary firmware blob
>   i440fx-test: verify firmware under 4G and 1M, both -bios and -pflash
> 
>  tests/i440fx-test.c | 169 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 152 insertions(+), 17 deletions(-)

Self-NAK

I'll have to send a new version, for at least two reasons:

- The original code (before patch #1) doesn't bother to free "data.bus"
(the retval of qpci_init_pc()). Therefore I assumed this function only
grabbed a (non-counted) reference. This is not the case: the original
code leaks it (although it exits soon after), and my patch #2 doubles
the leak.

- There's something fishy with g_assert() firing in the new test case. I
end up with a hung qemu process, reparented to init:

  x86_64-softmmu/qemu-system-x86_64 \
    -qtest unix:/tmp/qtest-26926.sock,nowait -qtest-log /dev/null \
    -qmp unix:/tmp/qtest-26926.qmp,nowait \
    -pidfile /tmp/qtest-26926.pid -machine accel=qtest \
    -S -display none -pflash /tmp/fw_blob_553Y5W

I have no idea if this has to do with my unorthodox use of "-S" in
qtest, or if it's a general shortcoming of qtest (ie. aborting before
qtest_end()).

Anyway the series should be reviewable as-is, so I'll wait a bit for
comments.

Thanks!
Laszlo