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

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

[Laszlo Ersek]

v3 [Markus]:
- FW_SIZE -> BLOB_SIZE
- create_firmware() -> create_blob_file()
- detach create_blob_file() comment block from firmware & temporariness
- same in filename and error messages there
- char unsigned -> uint8_t

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 | 167 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 150 insertions(+), 17 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 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 65c786c..6ac46bf 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("-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 v3 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 6ac46bf..3962bca 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("-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("-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 v3 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_blob_file() being
unused, and we'll replace it in the next patch anyway.

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

diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index 3962bca..b6e0cd3 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,68 @@ static void test_i440fx_pam(gconstpointer opaque)
     qtest_end();
 }
 
+#define BLOB_SIZE ((size_t)65536)
+
+/* Create a blob file, and return its absolute pathname as a dynamically
+ * allocated string.
+ * The file is closed before the function returns.
+ * In case of error, NULL is returned. The function prints the error message.
+ */
+static char *create_blob_file(void)
+{
+    int ret, fd;
+    char *pathname;
+    GError *error = NULL;
+
+    ret = -1;
+    fd = g_file_open_tmp("blob_XXXXXX", &pathname, &error);
+    if (fd == -1) {
+        fprintf(stderr, "unable to create blob file: %s\n", error->message);
+        g_error_free(error);
+    } else {
+        if (ftruncate(fd, BLOB_SIZE) == -1) {
+            fprintf(stderr, "ftruncate(\"%s\", %zu): %s\n", pathname,
+                    BLOB_SIZE, strerror(errno));
+        } else {
+            void *buf;
+
+            buf = mmap(NULL, BLOB_SIZE, PROT_WRITE, MAP_SHARED, fd, 0);
+            if (buf == MAP_FAILED) {
+                fprintf(stderr, "mmap(\"%s\", %zu): %s\n", pathname, BLOB_SIZE,
+                        strerror(errno));
+            } else {
+                size_t i;
+
+                for (i = 0; i < BLOB_SIZE; ++i) {
+                    ((uint8_t *)buf)[i] = i;
+                }
+                munmap(buf, BLOB_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_blob_file();
+    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 v3 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 b6e0cd3..fa3e3d6 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 BLOB_SIZE ((size_t)65536)
+#define ISA_BIOS_MAXSZ ((size_t)(128 * 1024))
 
 /* Create a blob file, and return its absolute pathname as a dynamically
  * allocated string.
@@ -326,23 +332,86 @@ static char *create_blob_file(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;
+    uint8_t *buf;
+    size_t i, isa_bios_size;
 
     fw_pathname = create_blob_file();
     g_assert(fw_pathname != NULL);
+
+    /* Better hope the user didn't put metacharacters in TMPDIR and co. */
+    cmdline = g_strdup_printf("-S %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(BLOB_SIZE);
+    memread(0x100000000ULL - BLOB_SIZE, buf, BLOB_SIZE);
+    for (i = 0; i < BLOB_SIZE; ++i) {
+        g_assert_cmphex(buf[i], ==, (uint8_t)i);
+    }
+
+    /* check in ISA space too */
+    memset(buf, 0, BLOB_SIZE);
+    isa_bios_size = ISA_BIOS_MAXSZ < BLOB_SIZE ? ISA_BIOS_MAXSZ : BLOB_SIZE;
+    memread(0x100000 - isa_bios_size, buf, isa_bios_size);
+    for (i = 0; i < isa_bios_size; ++i) {
+        g_assert_cmphex(buf[i], ==,
+                        (uint8_t)((BLOB_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 v3 0/4] i440fx-test: check firmware visibility

[Paolo Bonzini]

Il 29/11/2013 18:12, Laszlo Ersek ha scritto:
> v3 [Markus]:
> - FW_SIZE -> BLOB_SIZE
> - create_firmware() -> create_blob_file()
> - detach create_blob_file() comment block from firmware & temporariness
> - same in filename and error messages there
> - char unsigned -> uint8_t

Great idea. :)

> 
> 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 | 167 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 150 insertions(+), 17 deletions(-)
> 

Markus, can you give your Reviewed-by?

Paolo

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

[Michael S. Tsirkin]

On Fri, Nov 29, 2013 at 06:12:18PM +0100, Laszlo Ersek wrote:
> v3 [Markus]:
> - FW_SIZE -> BLOB_SIZE
> - create_firmware() -> create_blob_file()
> - detach create_blob_file() comment block from firmware & temporariness
> - same in filename and error messages there
> - char unsigned -> uint8_t
> 
> 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


Applied, thanks.

>  tests/i440fx-test.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 150 insertions(+), 17 deletions(-)
> 
> -- 
> 1.8.3.1
>