[Qemu-devel] [PATCH 0/3] Add new CD-ROM related qtests

[Qemu-devel] [PATCH 0/3] Add new CD-ROM related qtests

[Thomas Huth]

With one of my clean-up patches (see commit 1454509726719e0933c800), I
recently accidentially broke the "-cdrom" parameter (more precisely
"-drive if=scsi") on a couple of boards, since there was no error
detected during the "make check" regression testing. This is clearly an
indication that we are lacking tests in this area.
So this small patch series now introduces some tests for CD-ROM drives:
The first two patches introduce the possibility to check that booting
from CD-ROM drives still works fine for x86 and s390x, and the third
patch adds a test that certain machines can at least still be started
with the "-cdrom" parameter (i.e. that test would have catched the
mistake that I did with my SCSI cleanup patch).

Thomas Huth (3):
  tests/boot-sector: Add magic bytes to s390x boot code header
  tests/cdboot: Test booting from CD-ROM ISO image file
  tests/cdrom-test: Test that -cdrom parameter is working

 tests/Makefile.include |  10 ++-
 tests/boot-sector.c    |   9 ++-
 tests/cdrom-test.c     | 216 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 231 insertions(+), 4 deletions(-)
 create mode 100644 tests/cdrom-test.c

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/3] tests/boot-sector: Add magic bytes to s390x boot code header

[Thomas Huth]

We're going to use the s390x boot code for testing CD-ROM booting.
But the ISO loader of the s390-ccw bios is a little bit more picky
than the network loader and expects some magic bytes in the header
of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so
we've got to add them in our boot code here, too.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/boot-sector.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/boot-sector.c b/tests/boot-sector.c
index c373f0e..04721c9 100644
--- a/tests/boot-sector.c
+++ b/tests/boot-sector.c
@@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
 };
 
 /* For s390x, use a mini "kernel" with the appropriate signature */
-static const uint8_t s390x_psw[] = {
-    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
+static const uint8_t s390x_psw_and_magic[] = {
+    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00,
+    0x02, 0x00, 0x00, 0x18, 0x60, 0x00, 0x00, 0x50,
+    0x02, 0x00, 0x00, 0x68, 0x60, 0x00, 0x00, 0x50,
+    0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40
 };
 static const uint8_t s390x_code[] = {
     0xa7, 0xf4, 0x00, 0x0a,                                /* j 0x10010 */
@@ -110,7 +113,7 @@ int boot_sector_init(char *fname)
     } else if (g_str_equal(arch, "s390x")) {
         len = 0x10000 + sizeof(s390x_code);
         boot_code = g_malloc0(len);
-        memcpy(boot_code, s390x_psw, sizeof(s390x_psw));
+        memcpy(boot_code, s390x_psw_and_magic, sizeof(s390x_psw_and_magic));
         memcpy(&boot_code[0x10000], s390x_code, sizeof(s390x_code));
     } else {
         g_assert_not_reached();
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/3] tests/cdboot: Test booting from CD-ROM ISO image file

[Thomas Huth]

We already have the code for a boot file in tests/boot-sector.c,
so if the genisoimage program is available, we can easily create
a bootable CD ISO image that we can use for testing whether our
CD-ROM emulation and the BIOS CD-ROM boot works correctly.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include |   3 +
 tests/cdrom-test.c     | 155 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 158 insertions(+)
 create mode 100644 tests/cdrom-test.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index ef9b88c..a104222 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -304,6 +304,7 @@ check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
 check-qtest-i386-y += tests/migration-test$(EXESUF)
 check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
 check-qtest-i386-y += tests/numa-test$(EXESUF)
+check-qtest-i386-y += tests/cdrom-test$(EXESUF)
 check-qtest-x86_64-y += $(check-qtest-i386-y)
 check-qtest-x86_64-y += tests/sdhci-test$(EXESUF)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
@@ -398,6 +399,7 @@ check-qtest-s390x-y += tests/virtio-balloon-test$(EXESUF)
 check-qtest-s390x-y += tests/virtio-console-test$(EXESUF)
 check-qtest-s390x-y += tests/virtio-serial-test$(EXESUF)
 check-qtest-s390x-y += tests/cpu-plug-test$(EXESUF)
+check-qtest-s390x-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-generic-y += tests/qom-test$(EXESUF)
 check-qtest-generic-y += tests/test-hmp$(EXESUF)
@@ -829,6 +831,7 @@ tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
 tests/numa-test$(EXESUF): tests/numa-test.o
 tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
 tests/sdhci-test$(EXESUF): tests/sdhci-test.o $(libqos-pc-obj-y)
+tests/cdrom-test$(EXESUF): tests/cdrom-test.o tests/boot-sector.o $(libqos-obj-y)
 
 tests/migration/stress$(EXESUF): tests/migration/stress.o
 	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
diff --git a/tests/cdrom-test.c b/tests/cdrom-test.c
new file mode 100644
index 0000000..1fc5230
--- /dev/null
+++ b/tests/cdrom-test.c
@@ -0,0 +1,155 @@
+/*
+ * Various tests for emulated CD-ROM drives.
+ *
+ * Copyright (c) 2018 Red Hat Inc.
+ *
+ * Author:
+ *    Thomas Huth <thuth@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.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "boot-sector.h"
+
+static char isoimage[] = "cdrom-boot-iso-XXXXXX";
+
+static int gen_iso(const char *fmt, ...)
+{
+    char *params, *command;
+    va_list args;
+    pid_t pid;
+    int status;
+
+    pid = fork();
+    if (pid == 0) {
+        va_start(args, fmt);
+        params = g_strdup_vprintf(fmt, args);
+        va_end(args);
+        command = g_strdup_printf("exec genisoimage %s", params);
+        g_free(params);
+        execlp("/bin/sh", "sh", "-c", command, NULL);
+        exit(1);
+    }
+    wait(&status);
+
+    return WEXITSTATUS(status);
+}
+
+static int prepare_image(const char *arch, char *isoimage)
+{
+    char srcdir[] = "cdrom-test-dir-XXXXXX";
+    char *codefile = NULL;
+    int ifh, ret = -1;
+
+    ifh = mkstemp(isoimage);
+    if (ifh < 0) {
+        perror("Error creating temporary iso image file");
+        return -1;
+    }
+    if (!mkdtemp(srcdir)) {
+        perror("Error creating temporary directory");
+        goto cleanup;
+    }
+
+    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64") ||
+        g_str_equal(arch, "s390x")) {
+        codefile = g_strdup_printf("%s/bootcode-XXXXXX", srcdir);
+        ret = boot_sector_init(codefile);
+        if (ret) {
+            goto cleanup;
+        }
+    } else {
+        g_assert_not_reached();
+    }
+
+    ret = gen_iso("-quiet -l -no-emul-boot -b %s -o %s %s",
+                  strrchr(codefile, '/') + 1, isoimage, srcdir);
+    if (ret) {
+        fprintf(stderr, "genisoimage failed: %i\n", ret);
+    }
+
+    unlink(codefile);
+
+cleanup:
+    g_free(codefile);
+    rmdir(srcdir);
+    close(ifh);
+
+    return ret;
+}
+
+static void test_cdboot(gconstpointer data)
+{
+    QTestState *qts;
+
+    qts = qtest_startf("-accel kvm:tcg -no-shutdown %s%s", (const char *)data,
+                       isoimage);
+    boot_sector_test(qts);
+    qtest_quit(qts);
+}
+
+static void add_x86_tests(void)
+{
+    qtest_add_data_func("cdboot/default", "-cdrom ", test_cdboot);
+    qtest_add_data_func("cdboot/virtio-scsi",
+                        "-device virtio-scsi -device scsi-cd,drive=cdr "
+                        "-blockdev file,node-name=cdr,filename=", test_cdboot);
+    qtest_add_data_func("cdboot/isapc", "-M isapc "
+                        "-drive if=ide,media=cdrom,file=", test_cdboot);
+    qtest_add_data_func("cdboot/am53c974",
+                        "-device am53c974 -device scsi-cd,drive=cd1 "
+                        "-drive if=none,id=cd1,format=raw,file=", test_cdboot);
+    qtest_add_data_func("cdboot/dc390",
+                        "-device dc390 -device scsi-cd,drive=cd1 "
+                        "-blockdev file,node-name=cd1,filename=", test_cdboot);
+    qtest_add_data_func("cdboot/lsi53c895a",
+                        "-device lsi53c895a -device scsi-cd,drive=cd1 "
+                        "-blockdev file,node-name=cd1,filename=", test_cdboot);
+    qtest_add_data_func("cdboot/megasas", "-M q35 "
+                        "-device megasas -device scsi-cd,drive=cd1 "
+                        "-blockdev file,node-name=cd1,filename=", test_cdboot);
+    qtest_add_data_func("cdboot/megasas-gen2", "-M q35 "
+                        "-device megasas-gen2 -device scsi-cd,drive=cd1 "
+                        "-blockdev file,node-name=cd1,filename=", test_cdboot);
+}
+
+static void add_s390x_tests(void)
+{
+    qtest_add_data_func("cdboot/default", "-cdrom ", test_cdboot);
+    qtest_add_data_func("cdboot/virtio-scsi",
+                        "-device virtio-scsi -device scsi-cd,drive=cdr "
+                        "-blockdev file,node-name=cdr,filename=", test_cdboot);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+    const char *arch = qtest_get_arch();
+
+    g_test_init(&argc, &argv, NULL);
+
+    if (gen_iso("--version > /dev/null")) {
+        /* genisoimage not available - so can't run tests */
+        return 0;
+    }
+
+    ret = prepare_image(arch, isoimage);
+    if (ret) {
+        return ret;
+    }
+
+    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64")) {
+        add_x86_tests();
+    } else if (g_str_equal(arch, "s390x")) {
+        add_s390x_tests();
+    }
+
+    ret = g_test_run();
+
+    unlink(isoimage);
+
+    return ret;
+}
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/3] tests/cdrom-test: Test that -cdrom parameter is working

[Thomas Huth]

Commit 1454509726719e0933c800 recently broke the "-cdrom" parameter
on a couple of boards without that we noticed it immediately. Thus
add a test which checks that "-cdrom" can at least be used to start
QEMU with certain machine types.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/Makefile.include |  7 +++++-
 tests/cdrom-test.c     | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/tests/Makefile.include b/tests/Makefile.include
index a104222..b744fea 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -321,8 +321,9 @@ check-qtest-microblaze-y = tests/boot-serial-test$(EXESUF)
 check-qtest-mips-y = tests/endianness-test$(EXESUF)
 
 check-qtest-mips64-y = tests/endianness-test$(EXESUF)
+check-qtest-mips64-y += tests/cdrom-test$(EXESUF)
 
-check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
+check-qtest-mips64el-y = $(check-qtest-mips64-y)
 
 check-qtest-moxie-y = tests/boot-serial-test$(EXESUF)
 
@@ -356,6 +357,7 @@ check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
 check-qtest-ppc64-y += tests/numa-test$(EXESUF)
 check-qtest-ppc64-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF)
 check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
+check-qtest-ppc64-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-sh4-y = tests/endianness-test$(EXESUF)
 
@@ -365,10 +367,12 @@ check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
 check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
 gcov-files-sparc-y = hw/timer/m48t59.c
 check-qtest-sparc-y += tests/boot-serial-test$(EXESUF)
+check-qtest-sparc-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
 check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
 check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
+check-qtest-sparc64-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 check-qtest-arm-y += tests/ds1338-test$(EXESUF)
@@ -384,6 +388,7 @@ check-qtest-arm-y += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y = tests/numa-test$(EXESUF)
 check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
 check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
+check-qtest-aarch64-y += tests/cdrom-test$(EXESUF)
 
 check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
 
diff --git a/tests/cdrom-test.c b/tests/cdrom-test.c
index 1fc5230..5e516dd 100644
--- a/tests/cdrom-test.c
+++ b/tests/cdrom-test.c
@@ -13,6 +13,7 @@
 #include "qemu/osdep.h"
 #include "libqtest.h"
 #include "boot-sector.h"
+#include "qapi/qmp/qdict.h"
 
 static char isoimage[] = "cdrom-boot-iso-XXXXXX";
 
@@ -62,7 +63,13 @@ static int prepare_image(const char *arch, char *isoimage)
             goto cleanup;
         }
     } else {
-        g_assert_not_reached();
+        /* Just create a dummy file */
+        char txt[] = "empty disc";
+        codefile = g_strdup_printf("%s/readme.txt", srcdir);
+        if (!g_file_set_contents(codefile, txt, sizeof(txt) - 1, NULL)) {
+            fprintf(stderr, "Failed to create '%s'\n", codefile);
+            goto cleanup;
+        }
     }
 
     ret = gen_iso("-quiet -l -no-emul-boot -b %s -o %s %s",
@@ -81,6 +88,32 @@ cleanup:
     return ret;
 }
 
+/**
+ * Check that at least the -cdrom parameter is basically working, i.e. we can
+ * see the filename of the ISO image in the output of "info block" afterwards
+ */
+static void test_cdrom_param(gconstpointer data)
+{
+    QTestState *qts;
+    char *resp;
+
+    qts = qtest_startf("-M %s -cdrom %s", (const char *)data, isoimage);
+    resp = qtest_hmp(qts, "info block");
+    g_assert(strstr(resp, isoimage) != 0);
+    g_free(resp);
+    qtest_quit(qts);
+}
+
+static void add_cdrom_param_tests(const char **machines)
+{
+    while (*machines) {
+        char *testname = g_strdup_printf("cdrom/param/%s", *machines);
+        qtest_add_data_func(testname, *machines, test_cdrom_param);
+        g_free(testname);
+        machines++;
+    }
+}
+
 static void test_cdboot(gconstpointer data)
 {
     QTestState *qts;
@@ -145,6 +178,34 @@ int main(int argc, char **argv)
         add_x86_tests();
     } else if (g_str_equal(arch, "s390x")) {
         add_s390x_tests();
+    } else if (g_str_equal(arch, "ppc64")) {
+        const char *ppcmachines[] = {
+            "pseries", "mac99", "g3beige", "40p", "prep", NULL
+        };
+        add_cdrom_param_tests(ppcmachines);
+    } else if (g_str_equal(arch, "sparc")) {
+        const char *sparcmachines[] = {
+            "LX", "SPARCClassic", "SPARCbook", "SS-10", "SS-20", "SS-4",
+            "SS-5", "SS-600MP", "Voyager", "leon3_generic", NULL
+        };
+        add_cdrom_param_tests(sparcmachines);
+    } else if (g_str_equal(arch, "sparc64")) {
+        const char *sparc64machines[] = {
+            "niagara", "sun4u", "sun4v", NULL
+        };
+        add_cdrom_param_tests(sparc64machines);
+    } else if (!strncmp(arch, "mips64", 6)) {
+        const char *mips64machines[] = {
+            "magnum", "malta", "mips", "mipssim", "pica61", NULL
+        };
+        add_cdrom_param_tests(mips64machines);
+    } else if (g_str_equal(arch, "aarch64")) {
+        const char *aarch64machines[] = {
+            "realview-eb", "realview-eb-mpcore", "realview-pb-a8",
+            "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",
+            "vexpress-a9", "virt", NULL
+        };
+        add_cdrom_param_tests(aarch64machines);
     }
 
     ret = g_test_run();
-- 
1.8.3.1

Re: [Qemu-devel] [qemu-s390x] [PATCH 1/3] tests/boot-sector: Add magic bytes to s390x boot code header

[Christian Borntraeger]

On 03/15/2018 08:49 AM, Thomas Huth wrote:
> We're going to use the s390x boot code for testing CD-ROM booting.
> But the ISO loader of the s390-ccw bios is a little bit more picky
> than the network loader and expects some magic bytes in the header
> of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so
> we've got to add them in our boot code here, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  tests/boot-sector.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
> index c373f0e..04721c9 100644
> --- a/tests/boot-sector.c
> +++ b/tests/boot-sector.c
> @@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
>  };
> 
>  /* For s390x, use a mini "kernel" with the appropriate signature */
> -static const uint8_t s390x_psw[] = {
> -    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
> +static const uint8_t s390x_psw_and_magic[] = {
> +    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00,
> +    0x02, 0x00, 0x00, 0x18, 0x60, 0x00, 0x00, 0x50,
> +    0x02, 0x00, 0x00, 0x68, 0x60, 0x00, 0x00, 0x50,
> +    0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40
>  };
>  static const uint8_t s390x_code[] = {
>      0xa7, 0xf4, 0x00, 0x0a,                                /* j 0x10010 */
> @@ -110,7 +113,7 @@ int boot_sector_init(char *fname)
>      } else if (g_str_equal(arch, "s390x")) {
>          len = 0x10000 + sizeof(s390x_code);
>          boot_code = g_malloc0(len);
> -        memcpy(boot_code, s390x_psw, sizeof(s390x_psw));
> +        memcpy(boot_code, s390x_psw_and_magic, sizeof(s390x_psw_and_magic));
>          memcpy(&boot_code[0x10000], s390x_code, sizeof(s390x_code));
>      } else {
>          g_assert_not_reached();
> 

Re: [Qemu-devel] [PATCH 2/3] tests/cdboot: Test booting from CD-ROM ISO image file

[Daniel P. Berrangé]

On Thu, Mar 15, 2018 at 08:49:04AM +0100, Thomas Huth wrote:
> We already have the code for a boot file in tests/boot-sector.c,
> so if the genisoimage program is available, we can easily create
> a bootable CD ISO image that we can use for testing whether our
> CD-ROM emulation and the BIOS CD-ROM boot works correctly.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include |   3 +
>  tests/cdrom-test.c     | 155 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 158 insertions(+)
>  create mode 100644 tests/cdrom-test.c
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index ef9b88c..a104222 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -304,6 +304,7 @@ check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
>  check-qtest-i386-y += tests/migration-test$(EXESUF)
>  check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
>  check-qtest-i386-y += tests/numa-test$(EXESUF)
> +check-qtest-i386-y += tests/cdrom-test$(EXESUF)
>  check-qtest-x86_64-y += $(check-qtest-i386-y)
>  check-qtest-x86_64-y += tests/sdhci-test$(EXESUF)
>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
> @@ -398,6 +399,7 @@ check-qtest-s390x-y += tests/virtio-balloon-test$(EXESUF)
>  check-qtest-s390x-y += tests/virtio-console-test$(EXESUF)
>  check-qtest-s390x-y += tests/virtio-serial-test$(EXESUF)
>  check-qtest-s390x-y += tests/cpu-plug-test$(EXESUF)
> +check-qtest-s390x-y += tests/cdrom-test$(EXESUF)
>  
>  check-qtest-generic-y += tests/qom-test$(EXESUF)
>  check-qtest-generic-y += tests/test-hmp$(EXESUF)
> @@ -829,6 +831,7 @@ tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
>  tests/numa-test$(EXESUF): tests/numa-test.o
>  tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
>  tests/sdhci-test$(EXESUF): tests/sdhci-test.o $(libqos-pc-obj-y)
> +tests/cdrom-test$(EXESUF): tests/cdrom-test.o tests/boot-sector.o $(libqos-obj-y)
>  
>  tests/migration/stress$(EXESUF): tests/migration/stress.o
>  	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
> diff --git a/tests/cdrom-test.c b/tests/cdrom-test.c
> new file mode 100644
> index 0000000..1fc5230
> --- /dev/null
> +++ b/tests/cdrom-test.c
> @@ -0,0 +1,155 @@
> +/*
> + * Various tests for emulated CD-ROM drives.
> + *
> + * Copyright (c) 2018 Red Hat Inc.
> + *
> + * Author:
> + *    Thomas Huth <thuth@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.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "boot-sector.h"
> +
> +static char isoimage[] = "cdrom-boot-iso-XXXXXX";
> +
> +static int gen_iso(const char *fmt, ...)
> +{
> +    char *params, *command;
> +    va_list args;
> +    pid_t pid;
> +    int status;
> +
> +    pid = fork();
> +    if (pid == 0) {
> +        va_start(args, fmt);
> +        params = g_strdup_vprintf(fmt, args);
> +        va_end(args);
> +        command = g_strdup_printf("exec genisoimage %s", params);
> +        g_free(params);
> +        execlp("/bin/sh", "sh", "-c", command, NULL);
> +        exit(1);
> +    }
> +    wait(&status);

IMHO this should just use g_spawn_sync(), also the use of
shell seems rather unneccessary - why not just run genisoimage
directly ?

  https://developer.gnome.org/glib/stable/glib-Spawning-Processes.html#g-spawn-sync

> +
> +    return WEXITSTATUS(status);
> +}
> +
> +static int prepare_image(const char *arch, char *isoimage)
> +{
> +    char srcdir[] = "cdrom-test-dir-XXXXXX";
> +    char *codefile = NULL;
> +    int ifh, ret = -1;
> +
> +    ifh = mkstemp(isoimage);
> +    if (ifh < 0) {
> +        perror("Error creating temporary iso image file");
> +        return -1;
> +    }
> +    if (!mkdtemp(srcdir)) {
> +        perror("Error creating temporary directory");
> +        goto cleanup;
> +    }
> +
> +    if (g_str_equal(arch, "i386") || g_str_equal(arch, "x86_64") ||
> +        g_str_equal(arch, "s390x")) {
> +        codefile = g_strdup_printf("%s/bootcode-XXXXXX", srcdir);
> +        ret = boot_sector_init(codefile);
> +        if (ret) {
> +            goto cleanup;
> +        }
> +    } else {
> +        g_assert_not_reached();
> +    }
> +
> +    ret = gen_iso("-quiet -l -no-emul-boot -b %s -o %s %s",
> +                  strrchr(codefile, '/') + 1, isoimage, srcdir);

It would be easy to just declare the args as an array here

  char *genisoargv[] = {
    "genisoimage", "-quiet", "-l", "-no-emul-boot", "-b",
    strrchr(codefile, "/") + 1, "-o", isoimage, srcdir,
    NULL,
  };

to then pass to g_spawn_sync

> +    if (ret) {
> +        fprintf(stderr, "genisoimage failed: %i\n", ret);
> +    }
> +
> +    unlink(codefile);
> +
> +cleanup:
> +    g_free(codefile);
> +    rmdir(srcdir);
> +    close(ifh);
> +
> +    return ret;
> +}

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 2/3] tests/cdboot: Test booting from CD-ROM ISO image file

[Thomas Huth]

On 15.03.2018 10:21, Daniel P. Berrangé wrote:
> On Thu, Mar 15, 2018 at 08:49:04AM +0100, Thomas Huth wrote:
>> We already have the code for a boot file in tests/boot-sector.c,
>> so if the genisoimage program is available, we can easily create
>> a bootable CD ISO image that we can use for testing whether our
>> CD-ROM emulation and the BIOS CD-ROM boot works correctly.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/Makefile.include |   3 +
>>  tests/cdrom-test.c     | 155 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 158 insertions(+)
>>  create mode 100644 tests/cdrom-test.c
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index ef9b88c..a104222 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -304,6 +304,7 @@ check-qtest-i386-$(CONFIG_POSIX) += tests/test-filter-redirector$(EXESUF)
>>  check-qtest-i386-y += tests/migration-test$(EXESUF)
>>  check-qtest-i386-y += tests/test-x86-cpuid-compat$(EXESUF)
>>  check-qtest-i386-y += tests/numa-test$(EXESUF)
>> +check-qtest-i386-y += tests/cdrom-test$(EXESUF)
>>  check-qtest-x86_64-y += $(check-qtest-i386-y)
>>  check-qtest-x86_64-y += tests/sdhci-test$(EXESUF)
>>  gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
>> @@ -398,6 +399,7 @@ check-qtest-s390x-y += tests/virtio-balloon-test$(EXESUF)
>>  check-qtest-s390x-y += tests/virtio-console-test$(EXESUF)
>>  check-qtest-s390x-y += tests/virtio-serial-test$(EXESUF)
>>  check-qtest-s390x-y += tests/cpu-plug-test$(EXESUF)
>> +check-qtest-s390x-y += tests/cdrom-test$(EXESUF)
>>  
>>  check-qtest-generic-y += tests/qom-test$(EXESUF)
>>  check-qtest-generic-y += tests/test-hmp$(EXESUF)
>> @@ -829,6 +831,7 @@ tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
>>  tests/numa-test$(EXESUF): tests/numa-test.o
>>  tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
>>  tests/sdhci-test$(EXESUF): tests/sdhci-test.o $(libqos-pc-obj-y)
>> +tests/cdrom-test$(EXESUF): tests/cdrom-test.o tests/boot-sector.o $(libqos-obj-y)
>>  
>>  tests/migration/stress$(EXESUF): tests/migration/stress.o
>>  	$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
>> diff --git a/tests/cdrom-test.c b/tests/cdrom-test.c
>> new file mode 100644
>> index 0000000..1fc5230
>> --- /dev/null
>> +++ b/tests/cdrom-test.c
>> @@ -0,0 +1,155 @@
>> +/*
>> + * Various tests for emulated CD-ROM drives.
>> + *
>> + * Copyright (c) 2018 Red Hat Inc.
>> + *
>> + * Author:
>> + *    Thomas Huth <thuth@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.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "libqtest.h"
>> +#include "boot-sector.h"
>> +
>> +static char isoimage[] = "cdrom-boot-iso-XXXXXX";
>> +
>> +static int gen_iso(const char *fmt, ...)
>> +{
>> +    char *params, *command;
>> +    va_list args;
>> +    pid_t pid;
>> +    int status;
>> +
>> +    pid = fork();
>> +    if (pid == 0) {
>> +        va_start(args, fmt);
>> +        params = g_strdup_vprintf(fmt, args);
>> +        va_end(args);
>> +        command = g_strdup_printf("exec genisoimage %s", params);
>> +        g_free(params);
>> +        execlp("/bin/sh", "sh", "-c", command, NULL);
>> +        exit(1);
>> +    }
>> +    wait(&status);
> 
> IMHO this should just use g_spawn_sync(), also the use of
> shell seems rather unneccessary - why not just run genisoimage
> directly ?

That code was simply "inspired" from the execlp() code in
qtest_init_without_qmp_handshake() ... but g_spawn_sync() sounds like a
good alternative here, so I'll try to switch to that one instead.

 Thanks,
  Thomas

Re: [Qemu-devel] [PATCH 3/3] tests/cdrom-test: Test that -cdrom parameter is working

[Philippe Mathieu-Daudé]

Hi Thomas,

On 03/15/2018 08:49 AM, Thomas Huth wrote:
> Commit 1454509726719e0933c800 recently broke the "-cdrom" parameter
> on a couple of boards without that we noticed it immediately. Thus
> add a test which checks that "-cdrom" can at least be used to start
> QEMU with certain machine types.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/Makefile.include |  7 +++++-
>  tests/cdrom-test.c     | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index a104222..b744fea 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -321,8 +321,9 @@ check-qtest-microblaze-y = tests/boot-serial-test$(EXESUF)
>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
>  
>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
> +check-qtest-mips64-y += tests/cdrom-test$(EXESUF)
>  
> -check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
> +check-qtest-mips64el-y = $(check-qtest-mips64-y)
>  
>  check-qtest-moxie-y = tests/boot-serial-test$(EXESUF)
>  
> @@ -356,6 +357,7 @@ check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
>  check-qtest-ppc64-y += tests/numa-test$(EXESUF)
>  check-qtest-ppc64-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF)
>  check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
> +check-qtest-ppc64-y += tests/cdrom-test$(EXESUF)
>  
>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>  
> @@ -365,10 +367,12 @@ check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
>  check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
>  gcov-files-sparc-y = hw/timer/m48t59.c
>  check-qtest-sparc-y += tests/boot-serial-test$(EXESUF)
> +check-qtest-sparc-y += tests/cdrom-test$(EXESUF)
>  
>  check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
>  check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>  check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
> +check-qtest-sparc64-y += tests/cdrom-test$(EXESUF)
>  
>  check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>  check-qtest-arm-y += tests/ds1338-test$(EXESUF)
> @@ -384,6 +388,7 @@ check-qtest-arm-y += tests/sdhci-test$(EXESUF)
>  check-qtest-aarch64-y = tests/numa-test$(EXESUF)
>  check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
>  check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
> +check-qtest-aarch64-y += tests/cdrom-test$(EXESUF)

Since you use g_str_equal(arch,), maybe this test can be added
regardless the arch in check-qtest-generic.

>  
>  check-qtest-microblazeel-y = $(check-qtest-microblaze-y)
>  
> diff --git a/tests/cdrom-test.c b/tests/cdrom-test.c
> index 1fc5230..5e516dd 100644
> --- a/tests/cdrom-test.c
> +++ b/tests/cdrom-test.c
> @@ -13,6 +13,7 @@
>  #include "qemu/osdep.h"
>  #include "libqtest.h"
>  #include "boot-sector.h"
> +#include "qapi/qmp/qdict.h"
>  
>  static char isoimage[] = "cdrom-boot-iso-XXXXXX";
>  
> @@ -62,7 +63,13 @@ static int prepare_image(const char *arch, char *isoimage)
>              goto cleanup;
>          }
>      } else {
> -        g_assert_not_reached();
> +        /* Just create a dummy file */
> +        char txt[] = "empty disc";
> +        codefile = g_strdup_printf("%s/readme.txt", srcdir);
> +        if (!g_file_set_contents(codefile, txt, sizeof(txt) - 1, NULL)) {
> +            fprintf(stderr, "Failed to create '%s'\n", codefile);
> +            goto cleanup;
> +        }
>      }
>  
>      ret = gen_iso("-quiet -l -no-emul-boot -b %s -o %s %s",
> @@ -81,6 +88,32 @@ cleanup:
>      return ret;
>  }
>  
> +/**
> + * Check that at least the -cdrom parameter is basically working, i.e. we can
> + * see the filename of the ISO image in the output of "info block" afterwards
> + */
> +static void test_cdrom_param(gconstpointer data)
> +{
> +    QTestState *qts;
> +    char *resp;
> +
> +    qts = qtest_startf("-M %s -cdrom %s", (const char *)data, isoimage);
> +    resp = qtest_hmp(qts, "info block");
> +    g_assert(strstr(resp, isoimage) != 0);
> +    g_free(resp);
> +    qtest_quit(qts);
> +}
> +
> +static void add_cdrom_param_tests(const char **machines)
> +{
> +    while (*machines) {
> +        char *testname = g_strdup_printf("cdrom/param/%s", *machines);
> +        qtest_add_data_func(testname, *machines, test_cdrom_param);
> +        g_free(testname);
> +        machines++;
> +    }
> +}
> +
>  static void test_cdboot(gconstpointer data)
>  {
>      QTestState *qts;
> @@ -145,6 +178,34 @@ int main(int argc, char **argv)
>          add_x86_tests();
>      } else if (g_str_equal(arch, "s390x")) {
>          add_s390x_tests();
> +    } else if (g_str_equal(arch, "ppc64")) {
> +        const char *ppcmachines[] = {
> +            "pseries", "mac99", "g3beige", "40p", "prep", NULL
> +        };
> +        add_cdrom_param_tests(ppcmachines);
> +    } else if (g_str_equal(arch, "sparc")) {
> +        const char *sparcmachines[] = {
> +            "LX", "SPARCClassic", "SPARCbook", "SS-10", "SS-20", "SS-4",
> +            "SS-5", "SS-600MP", "Voyager", "leon3_generic", NULL
> +        };
> +        add_cdrom_param_tests(sparcmachines);
> +    } else if (g_str_equal(arch, "sparc64")) {
> +        const char *sparc64machines[] = {
> +            "niagara", "sun4u", "sun4v", NULL
> +        };
> +        add_cdrom_param_tests(sparc64machines);
> +    } else if (!strncmp(arch, "mips64", 6)) {
> +        const char *mips64machines[] = {
> +            "magnum", "malta", "mips", "mipssim", "pica61", NULL
> +        };
> +        add_cdrom_param_tests(mips64machines);
> +    } else if (g_str_equal(arch, "aarch64")) {
> +        const char *aarch64machines[] = {
> +            "realview-eb", "realview-eb-mpcore", "realview-pb-a8",
> +            "realview-pbx-a9", "versatileab", "versatilepb", "vexpress-a15",
> +            "vexpress-a9", "virt", NULL
> +        };
> +        add_cdrom_param_tests(aarch64machines);
>      }
>  
>      ret = g_test_run();
> 

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

Re: [Qemu-devel] [PATCH 1/3] tests/boot-sector: Add magic bytes to s390x boot code header

[Philippe Mathieu-Daudé]

On 03/15/2018 08:49 AM, Thomas Huth wrote:
> We're going to use the s390x boot code for testing CD-ROM booting.
> But the ISO loader of the s390-ccw bios is a little bit more picky
> than the network loader and expects some magic bytes in the header
> of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so
> we've got to add them in our boot code here, too.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/boot-sector.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
> index c373f0e..04721c9 100644
> --- a/tests/boot-sector.c
> +++ b/tests/boot-sector.c
> @@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
>  };
>  
>  /* For s390x, use a mini "kernel" with the appropriate signature */
> -static const uint8_t s390x_psw[] = {
> -    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
> +static const uint8_t s390x_psw_and_magic[] = {

Can you add a comment such "see linux_s390_magic in
pc-bios/s390-ccw/bootmap.c"?

> +    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00,
> +    0x02, 0x00, 0x00, 0x18, 0x60, 0x00, 0x00, 0x50,
> +    0x02, 0x00, 0x00, 0x68, 0x60, 0x00, 0x00, 0x50,
> +    0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40
>  };
>  static const uint8_t s390x_code[] = {
>      0xa7, 0xf4, 0x00, 0x0a,                                /* j 0x10010 */
> @@ -110,7 +113,7 @@ int boot_sector_init(char *fname)
>      } else if (g_str_equal(arch, "s390x")) {
>          len = 0x10000 + sizeof(s390x_code);
>          boot_code = g_malloc0(len);
> -        memcpy(boot_code, s390x_psw, sizeof(s390x_psw));
> +        memcpy(boot_code, s390x_psw_and_magic, sizeof(s390x_psw_and_magic));
>          memcpy(&boot_code[0x10000], s390x_code, sizeof(s390x_code));
>      } else {
>          g_assert_not_reached();
> 

Re: [Qemu-devel] [PATCH 2/3] tests/cdboot: Test booting from CD-ROM ISO image file

[Eric Blake]

On 03/15/2018 05:48 AM, Thomas Huth wrote:

>>> +    pid = fork();
>>> +    if (pid == 0) {
>>> +        va_start(args, fmt);
>>> +        params = g_strdup_vprintf(fmt, args);
>>> +        va_end(args);
>>> +        command = g_strdup_printf("exec genisoimage %s", params);
>>> +        g_free(params);
>>> +        execlp("/bin/sh", "sh", "-c", command, NULL);
>>> +        exit(1);
>>> +    }
>>> +    wait(&status);
>>
>> IMHO this should just use g_spawn_sync(), also the use of
>> shell seems rather unneccessary

and potentially dangerous - if we aren't absolutely positive that we 
aren't going to improperly expand shell metacharacters embedded in params.

>> - why not just run genisoimage
>> directly ?
> 
> That code was simply "inspired" from the execlp() code in
> qtest_init_without_qmp_handshake()

Sounds like a good idea for a future cleanup patch ;)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 3/3] tests/cdrom-test: Test that -cdrom parameter is working

[Eric Blake]

On 03/15/2018 02:49 AM, Thomas Huth wrote:
> Commit 1454509726719e0933c800 recently broke the "-cdrom" parameter
> on a couple of boards without that we noticed it immediately. Thus

s/without that we noticed/without us noticing/

> add a test which checks that "-cdrom" can at least be used to start
> QEMU with certain machine types.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   tests/Makefile.include |  7 +++++-
>   tests/cdrom-test.c     | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 68 insertions(+), 2 deletions(-)
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 1/3] tests/boot-sector: Add magic bytes to s390x boot code header

[Thomas Huth]

On 15.03.2018 12:47, Philippe Mathieu-Daudé wrote:
> On 03/15/2018 08:49 AM, Thomas Huth wrote:
>> We're going to use the s390x boot code for testing CD-ROM booting.
>> But the ISO loader of the s390-ccw bios is a little bit more picky
>> than the network loader and expects some magic bytes in the header
>> of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so
>> we've got to add them in our boot code here, too.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/boot-sector.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/boot-sector.c b/tests/boot-sector.c
>> index c373f0e..04721c9 100644
>> --- a/tests/boot-sector.c
>> +++ b/tests/boot-sector.c
>> @@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
>>  };
>>  
>>  /* For s390x, use a mini "kernel" with the appropriate signature */
>> -static const uint8_t s390x_psw[] = {
>> -    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
>> +static const uint8_t s390x_psw_and_magic[] = {
> 
> Can you add a comment such "see linux_s390_magic in
> pc-bios/s390-ccw/bootmap.c"?

Sure, I'll add a comment.

 Thomas

Re: [Qemu-devel] [PATCH 3/3] tests/cdrom-test: Test that -cdrom parameter is working

[Thomas Huth]

On 15.03.2018 12:42, Philippe Mathieu-Daudé wrote:
> Hi Thomas,
> 
> On 03/15/2018 08:49 AM, Thomas Huth wrote:
>> Commit 1454509726719e0933c800 recently broke the "-cdrom" parameter
>> on a couple of boards without that we noticed it immediately. Thus
>> add a test which checks that "-cdrom" can at least be used to start
>> QEMU with certain machine types.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/Makefile.include |  7 +++++-
>>  tests/cdrom-test.c     | 63 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 68 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/Makefile.include b/tests/Makefile.include
>> index a104222..b744fea 100644
>> --- a/tests/Makefile.include
>> +++ b/tests/Makefile.include
>> @@ -321,8 +321,9 @@ check-qtest-microblaze-y = tests/boot-serial-test$(EXESUF)
>>  check-qtest-mips-y = tests/endianness-test$(EXESUF)
>>  
>>  check-qtest-mips64-y = tests/endianness-test$(EXESUF)
>> +check-qtest-mips64-y += tests/cdrom-test$(EXESUF)
>>  
>> -check-qtest-mips64el-y = tests/endianness-test$(EXESUF)
>> +check-qtest-mips64el-y = $(check-qtest-mips64-y)
>>  
>>  check-qtest-moxie-y = tests/boot-serial-test$(EXESUF)
>>  
>> @@ -356,6 +357,7 @@ check-qtest-ppc64-y += tests/display-vga-test$(EXESUF)
>>  check-qtest-ppc64-y += tests/numa-test$(EXESUF)
>>  check-qtest-ppc64-$(CONFIG_IVSHMEM) += tests/ivshmem-test$(EXESUF)
>>  check-qtest-ppc64-y += tests/cpu-plug-test$(EXESUF)
>> +check-qtest-ppc64-y += tests/cdrom-test$(EXESUF)
>>  
>>  check-qtest-sh4-y = tests/endianness-test$(EXESUF)
>>  
>> @@ -365,10 +367,12 @@ check-qtest-sparc-y = tests/prom-env-test$(EXESUF)
>>  check-qtest-sparc-y += tests/m48t59-test$(EXESUF)
>>  gcov-files-sparc-y = hw/timer/m48t59.c
>>  check-qtest-sparc-y += tests/boot-serial-test$(EXESUF)
>> +check-qtest-sparc-y += tests/cdrom-test$(EXESUF)
>>  
>>  check-qtest-sparc64-y = tests/endianness-test$(EXESUF)
>>  check-qtest-sparc64-y += tests/prom-env-test$(EXESUF)
>>  check-qtest-sparc64-y += tests/boot-serial-test$(EXESUF)
>> +check-qtest-sparc64-y += tests/cdrom-test$(EXESUF)
>>  
>>  check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>>  check-qtest-arm-y += tests/ds1338-test$(EXESUF)
>> @@ -384,6 +388,7 @@ check-qtest-arm-y += tests/sdhci-test$(EXESUF)
>>  check-qtest-aarch64-y = tests/numa-test$(EXESUF)
>>  check-qtest-aarch64-y += tests/sdhci-test$(EXESUF)
>>  check-qtest-aarch64-y += tests/boot-serial-test$(EXESUF)
>> +check-qtest-aarch64-y += tests/cdrom-test$(EXESUF)
> 
> Since you use g_str_equal(arch,), maybe this test can be added
> regardless the arch in check-qtest-generic.

Yes, sounds like a good idea. I think the test can even be run on other
architectures by using the default machine or "-M none" ... I'll give it
a try ...

>>      ret = g_test_run();
>>
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

 Thanks!
  Thomas

Re: [Qemu-devel] [PATCH 1/3] tests/boot-sector: Add magic bytes to s390x boot code header

[Michael S. Tsirkin]

On Thu, Mar 15, 2018 at 12:47:08PM +0100, Philippe Mathieu-Daudé wrote:
> On 03/15/2018 08:49 AM, Thomas Huth wrote:
> > We're going to use the s390x boot code for testing CD-ROM booting.
> > But the ISO loader of the s390-ccw bios is a little bit more picky
> > than the network loader and expects some magic bytes in the header
> > of the file (see linux_s390_magic in pc-bios/s390-ccw/bootmap.c), so
> > we've got to add them in our boot code here, too.
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >  tests/boot-sector.c | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/tests/boot-sector.c b/tests/boot-sector.c
> > index c373f0e..04721c9 100644
> > --- a/tests/boot-sector.c
> > +++ b/tests/boot-sector.c
> > @@ -68,8 +68,11 @@ static uint8_t x86_boot_sector[512] = {
> >  };
> >  
> >  /* For s390x, use a mini "kernel" with the appropriate signature */
> > -static const uint8_t s390x_psw[] = {
> > -    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00
> > +static const uint8_t s390x_psw_and_magic[] = {
> 
> Can you add a comment such "see linux_s390_magic in
> pc-bios/s390-ccw/bootmap.c"?

Or better yet, copy the code comment from there.

> > +    0x00, 0x08, 0x00, 0x00, 0x80, 0x01, 0x00, 0x00,
> > +    0x02, 0x00, 0x00, 0x18, 0x60, 0x00, 0x00, 0x50,
> > +    0x02, 0x00, 0x00, 0x68, 0x60, 0x00, 0x00, 0x50,
> > +    0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40, 0x40
> >  };
> >  static const uint8_t s390x_code[] = {
> >      0xa7, 0xf4, 0x00, 0x0a,                                /* j 0x10010 */
> > @@ -110,7 +113,7 @@ int boot_sector_init(char *fname)
> >      } else if (g_str_equal(arch, "s390x")) {
> >          len = 0x10000 + sizeof(s390x_code);
> >          boot_code = g_malloc0(len);
> > -        memcpy(boot_code, s390x_psw, sizeof(s390x_psw));
> > +        memcpy(boot_code, s390x_psw_and_magic, sizeof(s390x_psw_and_magic));
> >          memcpy(&boot_code[0x10000], s390x_code, sizeof(s390x_code));
> >      } else {
> >          g_assert_not_reached();
> >