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

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

[Laszlo Ersek]

v1 blurb:

> 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

The test passes with v1.7.0.

Changes in v2:
- Rebased ("-display none" is the default now).

- I looked into memory management prudence in qtest, and I can see that
  we don't care about leaking small objects at all. (See for example
  qpci_device_foreach(), which calls qpci_device_find() in a loop, and
  leaks the retval every time.)

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 v2 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 v2 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 v2 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 3962bca..5506421 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 v2 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 5506421..6de16e9 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 %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 v2 0/4] i440fx-test: check firmware visibility

[Laszlo Ersek]

On 11/28/13 19:09, Laszlo Ersek wrote:

> Changes in v2:
> - Rebased ("-display none" is the default now).
> 
> - I looked into memory management prudence in qtest, and I can see that
>   we don't care about leaking small objects at all. (See for example
>   qpci_device_foreach(), which calls qpci_device_find() in a loop, and
>   leaks the retval every time.)

Sorry, last sentence missing:
"Which is why I didn't try to plug such leaks in my series."

NB: If I don't get feedback for this, I'm dropping it.
http://thread.gmane.org/gmane.comp.emulators.qemu/240173/focus=241207

Thanks
Laszlo

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

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> 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;
>  }

There are a few more mismatched qtest_start()..qtest_quit() pairs
elsewhere.  That's an observation, not a demand :)

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

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> 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 3962bca..5506421 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)

Any particular reason for the cast?

> +
> +/* 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.
> + */

Actually, this creates a blob file.  Its temporariness and firmware-ness
are all the caller's business.  Rephrase accordingly?

Could this function be generally useful for tests?

> +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));

I wonder whether glib wants us to use g_test_message() here.

> +        } 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;

(unsigned char *), please

Why not simply unsigned char *buf?

> +                }
> +                munmap(buf, FW_SIZE);
> +                ret = 0;
> +            }
> +        }

Not sure I like this use of mmap(), but it's certainly covered by your
artistic license.

> +        close(fd);
> +        if (ret == -1) {
> +            unlink(pathname);
> +            g_free(pathname);
> +        }
> +    }
> +
> +    return ret == -1 ? NULL : pathname;
> +}

Works.  Stylistic nitpick: I'd do the error handling differently,
though.  I prefer

    if fail
        report
        bail out
    continue normally

to

    if fail
        report
    else
        continue normally

because it keeps the normal workings flowing down "straight" rather than
increasingly indented.

static char *create_firmware(void)
{
    int fd, i;
    char *pathname;
    GError *error = NULL;
    unsigned char *buf;

    fd = g_file_open_tmp("fw_blob_XXXXXX", &pathname, &error);
    g_assert_no_error(error);

    if (ftruncate(fd, FW_SIZE) < 0) {
        fprintf(stderr, "ftruncate(\"%s\", %zu): %s\n", pathname, FW_SIZE,
                strerror(errno));
	goto fail;
    }

    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));
	goto fail;
    }

    for (i = 0; i < FW_SIZE; ++i) {
        buf[i] = i;
    }
    munmap(buf, FW_SIZE);

    close(fd);
    return pathname;

err:
    close(fd);
    unlink(pathname);
    g_free(pathname);
    return NULL;
}

> +
>  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);

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

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> 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 5506421..6de16e9 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;

unsigned char *buf, please.

> +    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. */

Putting meta-characters in TMPDIR would be... adventurous.

You could add quotes.  Then it breaks only when the user puts quotes in ;-P

> +    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(FW_SIZE);
> +    memread(0x100000000ULL - FW_SIZE, buf, FW_SIZE);

Zero-fill immediately followed by read.  Suggest g_malloc().

> +    for (i = 0; i < FW_SIZE; ++i) {
> +        g_assert_cmphex(buf[i], ==, (char unsigned)i);

(unsigned char), please.

> +    }
> +
> +    /* 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);

Zero-fill immediately followed by read.  Suggest to drop memset().

> +    for (i = 0; i < isa_bios_size; ++i) {
> +        g_assert_cmphex(buf[i], ==,
> +                        (char unsigned)((FW_SIZE - isa_bios_size) + i));

Again.

> +    }
> +
> +    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;
> +}
> +

I'm not sure fixtures are justified here.  Perhaps having the test
function's test data argument point to the flag would be simpler.

> +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;

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

[Eduardo Habkost]

On Thu, Nov 28, 2013 at 07:09:13PM +0100, Laszlo Ersek wrote:
> 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>
[...]
>  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));

You can use GTest setup/teardown functions to do this automatically on
all tests, but I am not sure the code really looks better when using it.
I gave it a try and it looks like this:

diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
index 6ac46bf..c9d6f6a 100644
--- a/tests/i440fx-test.c
+++ b/tests/i440fx-test.c
@@ -25,15 +25,35 @@
 
 #define ARRAY_SIZE(array) (sizeof(array) / sizeof((array)[0]))
 
-typedef struct TestData
+typedef struct TestArgs
 {
     int num_cpus;
+} TestArgs;
+
+typedef struct TestData
+{
     QPCIBus *bus;
 } TestData;
 
-static void test_i440fx_defaults(gconstpointer opaque)
+static void test_setup(TestData *s, gconstpointer user_data)
+{
+    const TestArgs *args = user_data;
+    char *cmdline;
+
+    cmdline = g_strdup_printf("-smp %d", args->num_cpus);
+    qtest_start(cmdline);
+    g_free(cmdline);
+    s->bus = qpci_init_pc();
+}
+
+static void test_teardown(TestData *s, gconstpointer user_data)
+{
+    qtest_end();
+}
+
+static void test_i440fx_defaults(TestData *s, gconstpointer user_data)
 {
-    const TestData *s = opaque;
+    const TestArgs *args = user_data;
     QPCIDevice *dev;
     uint32_t value;
 
@@ -62,7 +82,7 @@ static void test_i440fx_defaults(gconstpointer opaque)
 
     /* 3.2.11 */
     value = qpci_config_readw(dev, 0x50); /* PMCCFG */
-    if (s->num_cpus == 1) { /* WPE */
+    if (args->num_cpus == 1) { /* WPE */
         g_assert(!(value & (1 << 15)));
     } else {
         g_assert((value & (1 << 15)));
@@ -176,9 +196,8 @@ static void write_area(uint32_t start, uint32_t end, uint8_t value)
     g_free(data);
 }
 
-static void test_i440fx_pam(gconstpointer opaque)
+static void test_i440fx_pam(TestData *s, gconstpointer user_data)
 {
-    const TestData *s = opaque;
     QPCIDevice *dev;
     int i;
     static struct {
@@ -258,26 +277,16 @@ static void test_i440fx_pam(gconstpointer opaque)
 
 int main(int argc, char **argv)
 {
-    TestData data;
-    char *cmdline;
     int ret;
+    TestArgs args = { .num_cpus = 1 };
 
     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);
+    g_test_add("/i440fx/defaults", TestData, &args, test_setup,
+               test_i440fx_defaults, test_teardown);
+    g_test_add("/i440fx/pam", TestData, &args, test_setup,
+               test_i440fx_pam, test_teardown);
 
     ret = g_test_run();
-
-    qtest_end();
-
     return ret;
 }
-- 
1.8.3.1

-- 
Eduardo

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

[Laszlo Ersek]

On 11/29/13 14:57, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:
> 
>> 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 3962bca..5506421 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)
> 
> Any particular reason for the cast?

Yes. It is a size. It is used in the controlling expression of a for()
statement, where the loop variable is a subscript. The variable is
size_t too (as it should be).

> 
>> +
>> +/* 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.
>> + */
> 
> Actually, this creates a blob file.  Its temporariness and firmware-ness
> are all the caller's business.  Rephrase accordingly?

I think that would be overkill. The function has a specific use.

> 
> Could this function be generally useful for tests?

Not sure.

> 
>> +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));
> 
> I wonder whether glib wants us to use g_test_message() here.

g_test_message()s are normally supressed, with and without gtester. With
gtester, even --verbose doesn't display them (in the default config).
"tests/i440fx-test --verbose" displays those messages.

This is an error message and I didn't want it to depend on gtester
settings or command line arguments.

> 
>> +        } 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;
> 
> (unsigned char *), please
> 
> Why not simply unsigned char *buf?

Because mmap() returns a (void*), and I need to compare that against
MAP_FAILED.

    The <sys/mman.h> header shall define the symbolic constant
    MAP_FAILED which shall have type void * and shall be used to
    indicate a failure from the mmap() function .

Your suggestion would include automatic conversion of MAP_FAILED to
(char *), which I did not want.

> 
>> +                }
>> +                munmap(buf, FW_SIZE);
>> +                ret = 0;
>> +            }
>> +        }
> 
> Not sure I like this use of mmap(), but it's certainly covered by your
> artistic license.

Well, thanks for recognizing that. Do you think you could extend your
leniency to "char unsigned" as well?

My reason for writing these types in this order (char unsigned, long
unsigned, long long unsigned) is to follow printf() format specifiers.
As in , "%lu", "%llu". (Char types are promoted so no extra width
specifiers for them in printf(), but I like to be consistent with myself.)

> 
>> +        close(fd);
>> +        if (ret == -1) {
>> +            unlink(pathname);
>> +            g_free(pathname);
>> +        }
>> +    }
>> +
>> +    return ret == -1 ? NULL : pathname;
>> +}
> 
> Works.  Stylistic nitpick: I'd do the error handling differently,
> though.  I prefer
> 
>     if fail
>         report
>         bail out
>     continue normally
> 
> to
> 
>     if fail
>         report
>     else
>         continue normally
> 
> because it keeps the normal workings flowing down "straight" rather than
> increasingly indented.

Normally I'm OK with cascading goto's and/or early returns.

I think that the way I did it here matches this situation well. After
the g_file_open_tmp() call succeeds, we must close fd in any case
(independently of whether as a whole the function succeeds or not).
Optionally, we must also unlink the file, in the same logical spot where
the close() is. (Because g_file_open() creates three resources at once,
a node in the filesystem, a file descriptor in the process, and a
dynamically allocated string.)

> 
> static char *create_firmware(void)
> {
>     int fd, i;
>     char *pathname;
>     GError *error = NULL;
>     unsigned char *buf;
> 
>     fd = g_file_open_tmp("fw_blob_XXXXXX", &pathname, &error);
>     g_assert_no_error(error);
> 
>     if (ftruncate(fd, FW_SIZE) < 0) {
>         fprintf(stderr, "ftruncate(\"%s\", %zu): %s\n", pathname, FW_SIZE,
>                 strerror(errno));
> 	goto fail;
>     }
> 
>     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));
> 	goto fail;
>     }
> 
>     for (i = 0; i < FW_SIZE; ++i) {
>         buf[i] = i;
>     }
>     munmap(buf, FW_SIZE);
> 
>     close(fd);
>     return pathname;
> 
> err:
>     close(fd);
>     unlink(pathname);
>     g_free(pathname);
>     return NULL;
> }

Your proposal duplicates the close(fd) call.

> 
>> +
>>  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);

I'm sure you're not deliberately trying to destroy my will to contribute
to upstream qemu.

Thanks
Laszlo

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

[Laszlo Ersek]

On 11/29/13 15:09, Markus Armbruster wrote:
> Laszlo Ersek <lersek@redhat.com> writes:

>> +    /* check below 4G */
>> +    buf = g_malloc0(FW_SIZE);
>> +    memread(0x100000000ULL - FW_SIZE, buf, FW_SIZE);
> 
> Zero-fill immediately followed by read.  Suggest g_malloc().

This was intentional. Please see the preexistent use of memread() in
this file, in verify_area().

> 
>> +    for (i = 0; i < FW_SIZE; ++i) {
>> +        g_assert_cmphex(buf[i], ==, (char unsigned)i);
> 
> (unsigned char), please.
> 
>> +    }
>> +
>> +    /* 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);
> 
> Zero-fill immediately followed by read.  Suggest to drop memset().

Same as above. memread() is unable to report errors. Some C library
functions also require you to set errno to zero first, then call the
function, then check errno, because some of the return values are
overlapped by success and failure returns. For memread() there's no
distinction in return value at all.

> 
>> +    for (i = 0; i < isa_bios_size; ++i) {
>> +        g_assert_cmphex(buf[i], ==,
>> +                        (char unsigned)((FW_SIZE - isa_bios_size) + i));
> 
> Again.
> 
>> +    }
>> +
>> +    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;
>> +}
>> +
> 
> I'm not sure fixtures are justified here.  Perhaps having the test
> function's test data argument point to the flag would be simpler.

I gave a lot of thought to this. Fixtures are needed. See the
explanation in patch#2.

In more detail here: the gtest framework operates in two distinct
phases. First, you set up all of your test cases in *declarative* style.
That's what all the g_test_add_*() invocations do. You need to describe
everything in advance.

Then, you call g_test_run(), and it runs your declarative script in one
atomic sequence. You cannot change stuff *between* tests. If I want to
run the same test function twice, but with different data, I have the
following choices:

- Allocate and initialize two distinct memory areas. The lifetimes of
both of these will be identical, and they will both live during the
entire test series. I configure the first test case with the address of
the first area, and the 2nd case with the address of the 2nd area.

- Keep one global (shared) area, and let the tests read and write it as
they are executed by the framework. If you reorder the tests in the
outer script, things break.

- Use fixtures. The framework allocates the area for you before each
test, calls your init function on it, runs the test, then tears down the
area. No dependency between tests. Also, if you have N cases in your
test series, you still allocate at most 1 area at any point (as opposed
to at most N, like under option #1).

> 
>> +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;

Thanks
Laszlo

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

[Laszlo Ersek]

On 11/29/13 15:53, Eduardo Habkost wrote:
> On Thu, Nov 28, 2013 at 07:09:13PM +0100, Laszlo Ersek wrote:
>> 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>
> [...]
>>  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));
> 
> You can use GTest setup/teardown functions to do this automatically on
> all tests, but I am not sure the code really looks better when using it.
> I gave it a try and it looks like this:
> 
> diff --git a/tests/i440fx-test.c b/tests/i440fx-test.c
> index 6ac46bf..c9d6f6a 100644
> --- a/tests/i440fx-test.c
> +++ b/tests/i440fx-test.c
> @@ -25,15 +25,35 @@
>  
>  #define ARRAY_SIZE(array) (sizeof(array) / sizeof((array)[0]))
>  
> -typedef struct TestData
> +typedef struct TestArgs
>  {
>      int num_cpus;
> +} TestArgs;
> +
> +typedef struct TestData
> +{
>      QPCIBus *bus;
>  } TestData;
>  
> -static void test_i440fx_defaults(gconstpointer opaque)
> +static void test_setup(TestData *s, gconstpointer user_data)
> +{
> +    const TestArgs *args = user_data;
> +    char *cmdline;
> +
> +    cmdline = g_strdup_printf("-smp %d", args->num_cpus);
> +    qtest_start(cmdline);
> +    g_free(cmdline);
> +    s->bus = qpci_init_pc();
> +}
> +
> +static void test_teardown(TestData *s, gconstpointer user_data)
> +{
> +    qtest_end();
> +}
> +
> +static void test_i440fx_defaults(TestData *s, gconstpointer user_data)
>  {
> -    const TestData *s = opaque;
> +    const TestArgs *args = user_data;
>      QPCIDevice *dev;
>      uint32_t value;
>  
> @@ -62,7 +82,7 @@ static void test_i440fx_defaults(gconstpointer opaque)
>  
>      /* 3.2.11 */
>      value = qpci_config_readw(dev, 0x50); /* PMCCFG */
> -    if (s->num_cpus == 1) { /* WPE */
> +    if (args->num_cpus == 1) { /* WPE */
>          g_assert(!(value & (1 << 15)));
>      } else {
>          g_assert((value & (1 << 15)));
> @@ -176,9 +196,8 @@ static void write_area(uint32_t start, uint32_t end, uint8_t value)
>      g_free(data);
>  }
>  
> -static void test_i440fx_pam(gconstpointer opaque)
> +static void test_i440fx_pam(TestData *s, gconstpointer user_data)
>  {
> -    const TestData *s = opaque;
>      QPCIDevice *dev;
>      int i;
>      static struct {
> @@ -258,26 +277,16 @@ static void test_i440fx_pam(gconstpointer opaque)
>  
>  int main(int argc, char **argv)
>  {
> -    TestData data;
> -    char *cmdline;
>      int ret;
> +    TestArgs args = { .num_cpus = 1 };
>  
>      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);
> +    g_test_add("/i440fx/defaults", TestData, &args, test_setup,
> +               test_i440fx_defaults, test_teardown);
> +    g_test_add("/i440fx/pam", TestData, &args, test_setup,
> +               test_i440fx_pam, test_teardown);
>  
>      ret = g_test_run();
> -
> -    qtest_end();
> -
>      return ret;
>  }
> 

I agree that this specific use of the fixtures doesn't really pay off.

Thank you
Laszlo

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

[Paolo Bonzini]

Il 29/11/2013 16:07, Laszlo Ersek ha scritto:
> I think that the way I did it here matches this situation well. After
> the g_file_open_tmp() call succeeds, we must close fd in any case
> (independently of whether as a whole the function succeeds or not).
> Optionally, we must also unlink the file, in the same logical spot where
> the close() is. (Because g_file_open() creates three resources at once,
> a node in the filesystem, a file descriptor in the process, and a
> dynamically allocated string.)

I agree that the way you wrote it makes sense if you do not use goto.

Paolo

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

[Paolo Bonzini]

Il 29/11/2013 16:30, Laszlo Ersek ha scritto:
>> > Zero-fill immediately followed by read.  Suggest to drop memset().
> Same as above. memread() is unable to report errors. Some C library
> functions also require you to set errno to zero first, then call the
> function, then check errno, because some of the return values are
> overlapped by success and failure returns. For memread() there's no
> distinction in return value at all.
> 

Errors in memread() will cause an assertion failure, but I think it's
okay to use g_malloc0.

That said, I agree that the common coding conventions (as well as the
English grammar) makes "char unsigned" look a bit weird.

Regarding usage of fixtures, I think it's within your artistic license
(quoting) and it's good to have an example to cut-and-paste from

Paolo.

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

[Andreas Färber]

Am 28.11.2013 19:18, schrieb Laszlo Ersek:
> On 11/28/13 19:09, Laszlo Ersek wrote:
> 
>> Changes in v2:
>> - Rebased ("-display none" is the default now).
>>
>> - I looked into memory management prudence in qtest, and I can see that
>>   we don't care about leaking small objects at all. (See for example
>>   qpci_device_foreach(), which calls qpci_device_find() in a loop, and
>>   leaks the retval every time.)
> 
> Sorry, last sentence missing:
> "Which is why I didn't try to plug such leaks in my series."

Laszlo,

> NB: If I don't get feedback for this, I'm dropping it.
> http://thread.gmane.org/gmane.comp.emulators.qemu/240173/focus=241207

If you're specifically waiting on my review, being the only one CC'ed,
you'll need to be more patient as other series have been waiting longer
on me.

Sorry,
Andreas

P.S. Hope you're feeling better since KVM Forum. :)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Laszlo Ersek]

On 11/29/13 18:12, Andreas Färber wrote:
> Am 28.11.2013 19:18, schrieb Laszlo Ersek:
>> On 11/28/13 19:09, Laszlo Ersek wrote:
>>
>>> Changes in v2:
>>> - Rebased ("-display none" is the default now).
>>>
>>> - I looked into memory management prudence in qtest, and I can see that
>>>   we don't care about leaking small objects at all. (See for example
>>>   qpci_device_foreach(), which calls qpci_device_find() in a loop, and
>>>   leaks the retval every time.)
>>
>> Sorry, last sentence missing:
>> "Which is why I didn't try to plug such leaks in my series."
> 
> Laszlo,
> 
>> NB: If I don't get feedback for this, I'm dropping it.
>> http://thread.gmane.org/gmane.comp.emulators.qemu/240173/focus=241207
> 
> If you're specifically waiting on my review, being the only one CC'ed,
> you'll need to be more patient as other series have been waiting longer
> on me.
> 
> Sorry,
> Andreas

I included the reference to show that I'm doing this in response to your
request. It's not something I'd have invested time in on my own behalf.
(Especially not three rounds.)

Review-wise, I'm fine with any review that gets the series merged and
the burden off my shoulder. There are patches where I feel I need advice
or sanity checking, this series is not one of them.

> P.S. Hope you're feeling better since KVM Forum. :)

My health is better, thank you very much for asking!

Cheers,
Laszlo

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

[Markus Armbruster]

Laszlo Ersek <lersek@redhat.com> writes:

> On 11/29/13 14:57, Markus Armbruster wrote:
>> Laszlo Ersek <lersek@redhat.com> writes:
>> 
>>> 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 3962bca..5506421 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)
>> 
>> Any particular reason for the cast?
>
> Yes. It is a size. It is used in the controlling expression of a for()
> statement, where the loop variable is a subscript. The variable is
> size_t too (as it should be).

I disagree, but it's your code.

>>> +
>>> +/* 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.
>>> + */
>> 
>> Actually, this creates a blob file.  Its temporariness and firmware-ness
>> are all the caller's business.  Rephrase accordingly?
>
> I think that would be overkill. The function has a specific use.
>
>> 
>> Could this function be generally useful for tests?
>
> Not sure.
>
>> 
>>> +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));
>> 
>> I wonder whether glib wants us to use g_test_message() here.
>
> g_test_message()s are normally supressed, with and without gtester. With
> gtester, even --verbose doesn't display them (in the default config).
> "tests/i440fx-test --verbose" displays those messages.
>
> This is an error message and I didn't want it to depend on gtester
> settings or command line arguments.

Makes sense.

Does glib provide means to report test errors other than g_assert() &
friends?  I'm asking because this would be the first use of stderr in
tests/*-test.c.

Losely related: should we embrace the fact that this is a test function,
whose failure is always ultimately fatal, and terminate the test right
there?

>> 
>>> +        } 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;
>> 
>> (unsigned char *), please
>> 
>> Why not simply unsigned char *buf?
>
> Because mmap() returns a (void*), and I need to compare that against
> MAP_FAILED.
>
>     The <sys/mman.h> header shall define the symbolic constant
>     MAP_FAILED which shall have type void * and shall be used to
>     indicate a failure from the mmap() function .
>
> Your suggestion would include automatic conversion of MAP_FAILED to
> (char *), which I did not want.

This isn't C++.  Generic pointers are a feature of C.  I don't think
littering the code with casts to avoid them is a good idea, because
1. casts defeat the type checker, and 2. brevity is a virtue.

>>> +                }
>>> +                munmap(buf, FW_SIZE);
>>> +                ret = 0;
>>> +            }
>>> +        }
>> 
>> Not sure I like this use of mmap(), but it's certainly covered by your
>> artistic license.
>
> Well, thanks for recognizing that. Do you think you could extend your
> leniency to "char unsigned" as well?
>
> My reason for writing these types in this order (char unsigned, long
> unsigned, long long unsigned) is to follow printf() format specifiers.
> As in , "%lu", "%llu". (Char types are promoted so no extra width
> specifiers for them in printf(), but I like to be consistent with myself.)

I prefer to be consistent with the rest of the code

    $ git-grep 'unsigned char'|wc -l
    553
    $ git-grep 'char unsigned'|wc -l
    8

even when I find it rather disagreable, like CamelCasedTypeDefs.

Besides, C uses words borrowed from English, so sticking to English
grammar (unsigned character) rather than French grammar (charactère
non-signée) makes some sense.  My brain parses the familiar "unsigned
char" more quickly than the unusual "char unsigned".

>>> +        close(fd);
>>> +        if (ret == -1) {
>>> +            unlink(pathname);
>>> +            g_free(pathname);
>>> +        }
>>> +    }
>>> +
>>> +    return ret == -1 ? NULL : pathname;
>>> +}
>> 
>> Works.  Stylistic nitpick: I'd do the error handling differently,
>> though.  I prefer
>> 
>>     if fail
>>         report
>>         bail out
>>     continue normally
>> 
>> to
>> 
>>     if fail
>>         report
>>     else
>>         continue normally
>> 
>> because it keeps the normal workings flowing down "straight" rather than
>> increasingly indented.
>
> Normally I'm OK with cascading goto's and/or early returns.
>
> I think that the way I did it here matches this situation well. After
> the g_file_open_tmp() call succeeds, we must close fd in any case
> (independently of whether as a whole the function succeeds or not).
> Optionally, we must also unlink the file, in the same logical spot where
> the close() is. (Because g_file_open() creates three resources at once,
> a node in the filesystem, a file descriptor in the process, and a
> dynamically allocated string.)
>
>> 
>> static char *create_firmware(void)
>> {
>>     int fd, i;
>>     char *pathname;
>>     GError *error = NULL;
>>     unsigned char *buf;
>> 
>>     fd = g_file_open_tmp("fw_blob_XXXXXX", &pathname, &error);
>>     g_assert_no_error(error);
>> 
>>     if (ftruncate(fd, FW_SIZE) < 0) {
>>         fprintf(stderr, "ftruncate(\"%s\", %zu): %s\n", pathname, FW_SIZE,
>>                 strerror(errno));
>> 	goto fail;
>>     }
>> 
>>     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));
>> 	goto fail;
>>     }
>> 
>>     for (i = 0; i < FW_SIZE; ++i) {
>>         buf[i] = i;
>>     }
>>     munmap(buf, FW_SIZE);
>> 
>>     close(fd);
>>     return pathname;
>> 
>> err:
>>     close(fd);
>>     unlink(pathname);
>>     g_free(pathname);
>>     return NULL;
>> }
>
> Your proposal duplicates the close(fd) call.

Yes, but there's one less variable to keep in short-term memory (ret),
and less nesting.  Overall, I find it easier to follow.  Anyway, your
choice.

>>> +
>>>  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);
>
> I'm sure you're not deliberately trying to destroy my will to contribute
> to upstream qemu.

Nope, and I'm sure you're not deliberately trying to destroy my will to
review your upstream QEMU patches.

I just re-read my review, and I still believe it was obvious enough that
most of my comments where of the type "have you thought of this, perhaps
you like it better (I do)".  I'm sorry this brushed you the wrong way.
Such comments will happen again, I'm afraid.  If I knew how to make them
more obvious without adding verbiage, I'd try.