[Qemu-devel] [PATCH v3 00/16] -boot and -no-fd-bootchk fixes

[Qemu-devel] [PATCH v3 00/16] -boot and -no-fd-bootchk fixes

[Markus Armbruster]

This has been rotting in my tree since February.  Sorry about that.

I'm afraid -boot regressed in 1.4, specifically commit e4ada29e.  This
series fixes it, along with related bugs, and tops off with tests.

PATCH 01-03 fix the regression, PATCH 04 cleans up afterwards.  I'm
refraining from nominating them for stable, because we regressed quite
some time ago, and the fix isn't exactly minimal.

PATCH 05 makes -no-fd-bootchk behave more sanely, and PATCH 06 fixes
up docs.  The case for stable is even weaker here: the old behavior
hasn't changed in quite a few releases, and nobody complained.

PATCH 07 tweaks qtest to make testing -boot once possible.  The
remaining patches add tests.

v3:
* Rebased, with only trivial conflicts
* PATCH 08 cosmetic improvements
* More test cases: new PATCH 09-16
v2:
* New PATCH 7 to make testing -boot once possible
* Old PATCH 5 reworked and extended became PATCH 8
* Writing more tests uncovered -no-fd-bootchk weirdness, cleaned up in
  new PATCH 5+6

Andreas Färber (1):
  boot-order-test: Add tests for PowerMacs

Markus Armbruster (15):
  vl: Clean up parsing of -boot option argument
  qemu-option: check_params() is now unused, drop it
  vl: Fix -boot order and once regressions, and related bugs
  vl: Rename *boot_devices to *boot_order, for consistency
  pc: Make -no-fd-bootchk stick across boot order changes
  doc: Drop ref to Bochs from -no-fd-bootchk documentation
  qtest: Don't reset on qtest chardev connect
  boot-order-test: New; covering just PC for now
  boot-order-test: Cover -boot once in ppc tests
  boot-order-test: Better separate target-specific and generic parts
  boot-order-test: Code motion for better readability
  boot-order-test: Add tests for PowerPC PREP
  boot-order-test: Add tests for Sun4m
  boot-order-test: Support fw_cfg in I/O space
  boot-order-test: Add tests for Sun4u

 hw/i386/pc.c            |   7 +-
 include/hw/hw.h         |   4 +-
 include/qemu/option.h   |   2 -
 qemu-options.hx         |   3 +-
 qtest.c                 |   7 +-
 tests/Makefile          |   4 +
 tests/boot-order-test.c | 234 ++++++++++++++++++++++++++++++++++++++++++++++++
 util/qemu-option.c      |  30 -------
 vl.c                    | 121 +++++++++----------------
 9 files changed, 291 insertions(+), 121 deletions(-)
 create mode 100644 tests/boot-order-test.c

-- 
1.7.11.7

[Qemu-devel] [PATCH v3 01/16] vl: Clean up parsing of -boot option argument

[Markus Armbruster]

Commit 3d3b8303 threw in some QemuOpts parsing without replacing the
existing ad hoc parser, resulting in a confusing mess.  Clean it up.

Two user-visible changes:

1. Invalid options are reported more nicely.  Before:

        qemu: unknown boot parameter 'x' in 'x=y'

   After:

        qemu-system-x86_64: -boot x=y: Invalid parameter 'x'

2. If -boot is given multiple times, options accumulate, just like for
   -machine.  Before, only options order, once and menu accumulated.
   For the other ones, all but the first -boot in non-legacy syntax
   got simply ignored.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c | 84 ++++++++++++++++++--------------------------------------------------
 1 file changed, 22 insertions(+), 62 deletions(-)

diff --git a/vl.c b/vl.c
index 180fdde..a0ac6e9 100644
--- a/vl.c
+++ b/vl.c
@@ -436,9 +436,10 @@ static QemuOptsList qemu_machine_opts = {
 
 static QemuOptsList qemu_boot_opts = {
     .name = "boot-opts",
+    .implied_opt_name = "order",
+    .merge_lists = true,
     .head = QTAILQ_HEAD_INITIALIZER(qemu_boot_opts.head),
     .desc = {
-        /* the three names below are not used now */
         {
             .name = "order",
             .type = QEMU_OPT_STRING,
@@ -447,8 +448,7 @@ static QemuOptsList qemu_boot_opts = {
             .type = QEMU_OPT_STRING,
         }, {
             .name = "menu",
-            .type = QEMU_OPT_STRING,
-        /* following are really used */
+            .type = QEMU_OPT_BOOL,
         }, {
             .name = "splash",
             .type = QEMU_OPT_STRING,
@@ -1159,7 +1159,7 @@ int qemu_boot_set(const char *boot_devices)
     return boot_set_handler(boot_set_opaque, boot_devices);
 }
 
-static void validate_bootdevices(char *devices)
+static void validate_bootdevices(const char *devices)
 {
     /* We just do some generic consistency checks */
     const char *p;
@@ -3134,70 +3134,30 @@ int main(int argc, char **argv, char **envp)
                 break;
             case QEMU_OPTION_boot:
                 {
-                    static const char * const params[] = {
-                        "order", "once", "menu",
-                        "splash", "splash-time",
-                        "reboot-timeout", "strict", NULL
-                    };
-                    char buf[sizeof(boot_devices)];
                     char *standard_boot_devices;
-                    int legacy = 0;
-
-                    if (!strchr(optarg, '=')) {
-                        legacy = 1;
-                        pstrcpy(buf, sizeof(buf), optarg);
-                    } else if (check_params(buf, sizeof(buf), params, optarg) < 0) {
-                        fprintf(stderr,
-                                "qemu: unknown boot parameter '%s' in '%s'\n",
-                                buf, optarg);
+                    const char *order, *once;
+
+                    opts = qemu_opts_parse(qemu_find_opts("boot-opts"),
+                                           optarg, 1);
+                    if (!opts) {
                         exit(1);
                     }
 
-                    if (legacy ||
-                        get_param_value(buf, sizeof(buf), "order", optarg)) {
-                        validate_bootdevices(buf);
-                        pstrcpy(boot_devices, sizeof(boot_devices), buf);
+                    order = qemu_opt_get(opts, "order");
+                    if (order) {
+                        validate_bootdevices(order);
+                        pstrcpy(boot_devices, sizeof(boot_devices), order);
                     }
-                    if (!legacy) {
-                        if (get_param_value(buf, sizeof(buf),
-                                            "once", optarg)) {
-                            validate_bootdevices(buf);
-                            standard_boot_devices = g_strdup(boot_devices);
-                            pstrcpy(boot_devices, sizeof(boot_devices), buf);
-                            qemu_register_reset(restore_boot_devices,
-                                                standard_boot_devices);
-                        }
-                        if (get_param_value(buf, sizeof(buf),
-                                            "menu", optarg)) {
-                            if (!strcmp(buf, "on")) {
-                                boot_menu = 1;
-                            } else if (!strcmp(buf, "off")) {
-                                boot_menu = 0;
-                            } else {
-                                fprintf(stderr,
-                                        "qemu: invalid option value '%s'\n",
-                                        buf);
-                                exit(1);
-                            }
-                        }
-                        if (get_param_value(buf, sizeof(buf),
-                                            "strict", optarg)) {
-                            if (!strcmp(buf, "on")) {
-                                boot_strict = true;
-                            } else if (!strcmp(buf, "off")) {
-                                boot_strict = false;
-                            } else {
-                                fprintf(stderr,
-                                        "qemu: invalid option value '%s'\n",
-                                        buf);
-                                exit(1);
-                            }
-                        }
-                        if (!qemu_opts_parse(qemu_find_opts("boot-opts"),
-                                             optarg, 0)) {
-                            exit(1);
-                        }
+
+                    once = qemu_opt_get(opts, "once");
+                    if (once) {
+                        validate_bootdevices(once);
+                        standard_boot_devices = g_strdup(boot_devices);
+                        pstrcpy(boot_devices, sizeof(boot_devices), once);
+                        qemu_register_reset(restore_boot_devices,
+                                            standard_boot_devices);
                     }
+                    boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
                 }
                 break;
             case QEMU_OPTION_fda:
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 02/16] qemu-option: check_params() is now unused, drop it

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/option.h |  2 --
 util/qemu-option.c    | 30 ------------------------------
 2 files changed, 32 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index bdb6d21..a83c700 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -55,8 +55,6 @@ int get_next_param_value(char *buf, int buf_size,
                          const char *tag, const char **pstr);
 int get_param_value(char *buf, int buf_size,
                     const char *tag, const char *str);
-int check_params(char *buf, int buf_size,
-                 const char * const *params, const char *str);
 
 
 /*
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 8b74bf1..412c425 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -123,36 +123,6 @@ int get_param_value(char *buf, int buf_size,
     return get_next_param_value(buf, buf_size, tag, &str);
 }
 
-int check_params(char *buf, int buf_size,
-                 const char * const *params, const char *str)
-{
-    const char *p;
-    int i;
-
-    p = str;
-    while (*p != '\0') {
-        p = get_opt_name(buf, buf_size, p, '=');
-        if (*p != '=') {
-            return -1;
-        }
-        p++;
-        for (i = 0; params[i] != NULL; i++) {
-            if (!strcmp(params[i], buf)) {
-                break;
-            }
-        }
-        if (params[i] == NULL) {
-            return -1;
-        }
-        p = get_opt_value(NULL, 0, p);
-        if (*p != ',') {
-            break;
-        }
-        p++;
-    }
-    return 0;
-}
-
 /*
  * Searches an option list for an option with the given name
  */
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 03/16] vl: Fix -boot order and once regressions, and related bugs

[Markus Armbruster]

Option "once" sets up a different boot order just for the initial
boot.  Boot order reverts back to normal on reset.  Option "order"
changes the normal boot order.

The reversal is implemented by reset handler restore_boot_devices(),
which takes the boot order to revert to as argument.
restore_boot_devices() does nothing on its first call, because that
must be the initial machine reset.  On its second call, it changes the
boot order back, and unregisters itself.

Because we register the handler right when -boot gets parsed, we can
revert to an incorrect normal boot order, and multiple -boot can
interact in funny ways.

Here's how things work without -boot once or order:

* boot_devices is "".

* main() passes machine->boot_order to to machine->init(), because
  boot_devices is "".  machine->init() configures firmware
  accordingly.  For PC machines, machine->boot_order is "cad", and
  pc_cmos_init() writes it to RTC CMOS, where SeaBIOS picks it up.

Now consider -boot order=:

* boot_devices is "".

* -boot order= sets boot_devices to "" (no change).

* main() passes machine->boot_order to to machine->init(), because
  boot_devices is "", as above.

  Bug: -boot order= has no effect.  Broken in commit e4ada29e.

Next, consider -boot once=a:

* boot_devices is "".

* -boot once=a registers restore_boot_devices() with argument "", and
  sets boot_devices to "a".

* main() passes boot_devices "a" to machine->init(), which configures
  firmware accordingly.  For PC machines, pc_cmos_init() writes the
  boot order to RTC CMOS.

* main() calls qemu_system_reset().  This runs reset handlers.

  - restore_boot_devices() gets called with argument "".  Does
    nothing, because it's the first call.

* Machine boots, boot order is "a".

* Machine resets (e.g. monitor command).  Reset handlers run.

  - restore_boot_devices() gets called with argument "".  Calls
    qemu_boot_set("") to reconfigure firmware.  For PC machines,
    pc_boot_set() writes it into RTC CMOS.  Reset handler
    unregistered.

    Bug: boot order reverts to "" instead of machine->boot_order.  The
    actual boot order depends on how firmware interprets "".  Broken
    in commit e4ada29e.

Next, consider -boot once=a -boot order=c:

* boot_devices is "".

* -boot once=a registers restore_boot_devices() with argument "", and
  sets boot_devices to "a".

* -boot order=c sets boot_devices to "c".

* main() passes boot_devices "c" to machine->init(), which configures
  firmware accordingly.  For PC machines, pc_cmos_init() writes the
  boot order to RTC CMOS.

* main() calls qemu_system_reset().  This runs reset handlers.

  - restore_boot_devices() gets called with argument "".  Does
    nothing, because it's the first call.

* Machine boots, boot order is "c".

  Bug: it should be "a".  I figure this has always been broken.

* Machine resets (e.g. monitor command).  Reset handlers run.

  - restore_boot_devices() gets called with argument "".  Calls
    qemu_boot_set("") to reconfigure firmware.  For PC machines,
    pc_boot_set() writes it into RTC CMOS.  Reset handler
    unregistered.

    Bug: boot order reverts to "" instead of "c".  I figure this has
    always been broken, just differently broken before commit
    e4ada29e.

Next, consider -boot once=a -boot once=b -boot once=c:

* boot_devices is "".

* -boot once=a registers restore_boot_devices() with argument "", and
  sets boot_devices to "a".

* -boot once=b registers restore_boot_devices() with argument "a", and
  sets boot_devices to "b".

* -boot once=c registers restore_boot_devices() with argument "b", and
  sets boot_devices to "c".

* main() passes boot_devices "c" to machine->init(), which configures
  firmware accordingly.  For PC machines, pc_cmos_init() writes the
  boot order to RTC CMOS.

* main() calls qemu_system_reset().  This runs reset handlers.

  - restore_boot_devices() gets called with argument "".  Does
    nothing, because it's the first call.

  - restore_boot_devices() gets called with argument "a".  Calls
    qemu_boot_set("a") to reconfigure firmware.  For PC machines,
    pc_boot_set() writes it into RTC CMOS.  Reset handler
    unregistered.

  - restore_boot_devices() gets called with argument "b".  Calls
    qemu_boot_set("b") to reconfigure firmware.  For PC machines,
    pc_boot_set() writes it into RTC CMOS.  Reset handler
    unregistered.

* Machine boots, boot order is "b".

  Bug: should really be "c", because that came last, and for all other
  -boot options, the last one wins.  I figure this was broken some
  time before commit 37905d6a, and fixed there only for a single
  occurence of "once".

* Machine resets (e.g. monitor command).  Reset handlers run.

  - restore_boot_devices() gets called with argument "".  Calls
    qemu_boot_set("") to reconfigure firmware.  For PC machines,
    pc_boot_set() writes it into RTC CMOS.  Reset handler
    unregistered.

    Same bug as above: boot order reverts to "" instead of
    machine->boot_order.

Fix by acting upon -boot options order, once and menu only after
option parsing is complete, and the machine is known.  This is how the
other -boot options work already.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c | 59 ++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/vl.c b/vl.c
index a0ac6e9..f51d8e8 100644
--- a/vl.c
+++ b/vl.c
@@ -2843,7 +2843,7 @@ int main(int argc, char **argv, char **envp)
     const char *icount_option = NULL;
     const char *initrd_filename;
     const char *kernel_filename, *kernel_cmdline;
-    char boot_devices[33] = "";
+    const char *boot_order = NULL;
     DisplayState *ds;
     int cyls, heads, secs, translation;
     QemuOpts *hda_opts = NULL, *opts, *machine_opts;
@@ -3133,31 +3133,9 @@ int main(int argc, char **argv, char **envp)
                 drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
                 break;
             case QEMU_OPTION_boot:
-                {
-                    char *standard_boot_devices;
-                    const char *order, *once;
-
-                    opts = qemu_opts_parse(qemu_find_opts("boot-opts"),
-                                           optarg, 1);
-                    if (!opts) {
-                        exit(1);
-                    }
-
-                    order = qemu_opt_get(opts, "order");
-                    if (order) {
-                        validate_bootdevices(order);
-                        pstrcpy(boot_devices, sizeof(boot_devices), order);
-                    }
-
-                    once = qemu_opt_get(opts, "once");
-                    if (once) {
-                        validate_bootdevices(once);
-                        standard_boot_devices = g_strdup(boot_devices);
-                        pstrcpy(boot_devices, sizeof(boot_devices), once);
-                        qemu_register_reset(restore_boot_devices,
-                                            standard_boot_devices);
-                    }
-                    boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
+                opts = qemu_opts_parse(qemu_find_opts("boot-opts"), optarg, 1);
+                if (!opts) {
+                    exit(1);
                 }
                 break;
             case QEMU_OPTION_fda:
@@ -4093,6 +4071,31 @@ int main(int argc, char **argv, char **envp)
         kernel_filename = initrd_filename = kernel_cmdline = NULL;
     }
 
+    if (!boot_order) {
+        boot_order = machine->boot_order;
+    }
+    opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
+    if (opts) {
+        char *normal_boot_order;
+        const char *order, *once;
+
+        order = qemu_opt_get(opts, "order");
+        if (order) {
+            validate_bootdevices(order);
+            boot_order = order;
+        }
+
+        once = qemu_opt_get(opts, "once");
+        if (once) {
+            validate_bootdevices(once);
+            normal_boot_order = g_strdup(boot_order);
+            boot_order = once;
+            qemu_register_reset(restore_boot_devices, normal_boot_order);
+        }
+
+        boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
+    }
+
     if (!kernel_cmdline) {
         kernel_cmdline = "";
     }
@@ -4257,9 +4260,7 @@ int main(int argc, char **argv, char **envp)
     qdev_machine_init();
 
     QEMUMachineInitArgs args = { .ram_size = ram_size,
-                                 .boot_device = (boot_devices[0] == '\0') ?
-                                                machine->boot_order :
-                                                boot_devices,
+                                 .boot_device = boot_order,
                                  .kernel_filename = kernel_filename,
                                  .kernel_cmdline = kernel_cmdline,
                                  .initrd_filename = initrd_filename,
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 04/16] vl: Rename *boot_devices to *boot_order, for consistency

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/hw/hw.h |  4 ++--
 vl.c            | 16 ++++++++--------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/hw/hw.h b/include/hw/hw.h
index 1fb9afa..cc9f847 100644
--- a/include/hw/hw.h
+++ b/include/hw/hw.h
@@ -44,9 +44,9 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
 
 /* handler to set the boot_device order for a specific type of QEMUMachine */
 /* return 0 if success */
-typedef int QEMUBootSetHandler(void *opaque, const char *boot_devices);
+typedef int QEMUBootSetHandler(void *opaque, const char *boot_order);
 void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
-int qemu_boot_set(const char *boot_devices);
+int qemu_boot_set(const char *boot_order);
 
 #ifdef NEED_CPU_H
 #if TARGET_LONG_BITS == 64
diff --git a/vl.c b/vl.c
index f51d8e8..17daedd 100644
--- a/vl.c
+++ b/vl.c
@@ -1151,12 +1151,12 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
     boot_set_opaque = opaque;
 }
 
-int qemu_boot_set(const char *boot_devices)
+int qemu_boot_set(const char *boot_order)
 {
     if (!boot_set_handler) {
         return -EINVAL;
     }
-    return boot_set_handler(boot_set_opaque, boot_devices);
+    return boot_set_handler(boot_set_opaque, boot_order);
 }
 
 static void validate_bootdevices(const char *devices)
@@ -1187,9 +1187,9 @@ static void validate_bootdevices(const char *devices)
     }
 }
 
-static void restore_boot_devices(void *opaque)
+static void restore_boot_order(void *opaque)
 {
-    char *standard_boot_devices = opaque;
+    char *normal_boot_order = opaque;
     static int first = 1;
 
     /* Restore boot order and remove ourselves after the first boot */
@@ -1198,10 +1198,10 @@ static void restore_boot_devices(void *opaque)
         return;
     }
 
-    qemu_boot_set(standard_boot_devices);
+    qemu_boot_set(normal_boot_order);
 
-    qemu_unregister_reset(restore_boot_devices, standard_boot_devices);
-    g_free(standard_boot_devices);
+    qemu_unregister_reset(restore_boot_order, normal_boot_order);
+    g_free(normal_boot_order);
 }
 
 void add_boot_device_path(int32_t bootindex, DeviceState *dev,
@@ -4090,7 +4090,7 @@ int main(int argc, char **argv, char **envp)
             validate_bootdevices(once);
             normal_boot_order = g_strdup(boot_order);
             boot_order = once;
-            qemu_register_reset(restore_boot_devices, normal_boot_order);
+            qemu_register_reset(restore_boot_order, normal_boot_order);
         }
 
         boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 05/16] pc: Make -no-fd-bootchk stick across boot order changes

[Markus Armbruster]

Option -no-fd-bootchk asks the BIOS to attempt booting from a floppy
even when the boot sector signature isn't there, by setting a bit in
RTC CMOS.  It was added back in 2006 (commit 52ca8d6a).

Two years later, commit 0ecdffbb added monitor command boot_set.
Implemented by new function pc_boot_set().  It unconditionally clears
the floppy signature bit in CMOS.

Commit e0f084bf added -boot option once to automatically change the
boot order on first reset.  Reuses pc_boot_set(), thus also clears the
floppy signature bit.  Commit d9346e81 took care to preserve this
behavior.

Thus, -no-fd-bootchk applies to any number of boots.  Except it
applies just to the first boot with -boot once, and never after
boot_set.  Weird.  Make it stick instead: set the bit according to
-no-fd-bootchk in pc_boot_set().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/pc.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4844a6b..7e524fc 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -266,7 +266,7 @@ static int boot_device2nibble(char boot_device)
     return 0;
 }
 
-static int set_boot_dev(ISADevice *s, const char *boot_device, int fd_bootchk)
+static int set_boot_dev(ISADevice *s, const char *boot_device)
 {
 #define PC_MAX_BOOT_DEVICES 3
     int nbds, bds[3] = { 0, };
@@ -292,7 +292,7 @@ static int set_boot_dev(ISADevice *s, const char *boot_device, int fd_bootchk)
 
 static int pc_boot_set(void *opaque, const char *boot_device)
 {
-    return set_boot_dev(opaque, boot_device, 0);
+    return set_boot_dev(opaque, boot_device);
 }
 
 typedef struct pc_cmos_init_late_arg {
@@ -407,8 +407,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
     cpu_hotplug_cb.cpu_added_notifier.notify = rtc_notify_cpu_added;
     qemu_register_cpu_added_notifier(&cpu_hotplug_cb.cpu_added_notifier);
 
-    /* set boot devices, and disable floppy signature check if requested */
-    if (set_boot_dev(s, boot_device, fd_bootchk)) {
+    if (set_boot_dev(s, boot_device)) {
         exit(1);
     }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 06/16] doc: Drop ref to Bochs from -no-fd-bootchk documentation

[Markus Armbruster]

Manual page and qemu-doc on talk about "Bochs BIOS".  We use SeaBIOS,
and it implements the feature.  Replace by just "BIOS", and drop the
TODO line wondering about the Bochs reference.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-options.hx | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index bf94862..8355f9b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1268,9 +1268,8 @@ DEF("no-fd-bootchk", 0, QEMU_OPTION_no_fd_bootchk,
 STEXI
 @item -no-fd-bootchk
 @findex -no-fd-bootchk
-Disable boot signature checking for floppy disks in Bochs BIOS. It may
+Disable boot signature checking for floppy disks in BIOS. May
 be needed to boot from old floppy disks.
-TODO: check reference to Bochs BIOS.
 ETEXI
 
 DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 07/16] qtest: Don't reset on qtest chardev connect

[Markus Armbruster]

libqtest's qtest_init() connecting to the qtest socket triggers reset.
This was coded in the hope we could use the same QEMU process for
multiple tests that way.  Never used.  Injects an extra reset even
when it's not used, and that can mess up tests such as the one of
-boot once I'm about to add.  Drop it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qtest.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/qtest.c b/qtest.c
index 07a9612..74f1842 100644
--- a/qtest.c
+++ b/qtest.c
@@ -472,7 +472,12 @@ static void qtest_event(void *opaque, int event)
 
     switch (event) {
     case CHR_EVENT_OPENED:
-        qemu_system_reset(false);
+        /*
+         * We used to call qemu_system_reset() here, hoping we could
+         * use the same process for multiple tests that way.  Never
+         * used.  Injects an extra reset even when it's not used, and
+         * that can mess up tests, e.g. -boot once.
+         */
         for (i = 0; i < ARRAY_SIZE(irq_levels); i++) {
             irq_levels[i] = 0;
         }
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 08/16] boot-order-test: New; covering just PC for now

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile          |  2 ++
 tests/boot-order-test.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 tests/boot-order-test.c

diff --git a/tests/Makefile b/tests/Makefile
index c107489..394e029 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -54,6 +54,7 @@ gcov-files-i386-y = hw/fdc.c
 check-qtest-i386-y += tests/ide-test$(EXESUF)
 check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
 gcov-files-i386-y += hw/hd-geometry.c
+check-qtest-i386-y += tests/boot-order-test$(EXESUF)
 check-qtest-i386-y += tests/rtc-test$(EXESUF)
 check-qtest-i386-y += tests/i440fx-test$(EXESUF)
 check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
@@ -130,6 +131,7 @@ tests/m48t59-test$(EXESUF): tests/m48t59-test.o
 tests/fdc-test$(EXESUF): tests/fdc-test.o
 tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y)
 tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
+tests/boot-order-test$(EXESUF): tests/boot-order-test.o
 tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
 tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
 tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
new file mode 100644
index 0000000..2215710
--- /dev/null
+++ b/tests/boot-order-test.c
@@ -0,0 +1,68 @@
+/*
+ * Boot order test cases.
+ *
+ * Copyright (c) 2013 Red Hat Inc.
+ *
+ * Authors:
+ *  Markus Armbruster <armbru@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 <glib.h>
+#include "libqtest.h"
+
+static void test_pc_cmos_byte(int reg, int expected)
+{
+    int actual;
+
+    outb(0x70, reg);
+    actual = inb(0x71);
+    g_assert_cmphex(actual, ==, expected);
+}
+
+static void test_pc_cmos(uint8_t boot1, uint8_t boot2)
+{
+    test_pc_cmos_byte(0x38, boot1);
+    test_pc_cmos_byte(0x3d, boot2);
+}
+
+static void test_pc_with_args(const char *test_args,
+                              uint8_t boot1, uint8_t boot2,
+                              uint8_t reboot1, uint8_t reboot2)
+{
+    char *args = g_strdup_printf("-nodefaults -display none %s", test_args);
+
+    qtest_start(args);
+    test_pc_cmos(boot1, boot2);
+    qmp("{ 'execute': 'system_reset' }");
+    test_pc_cmos(reboot1, reboot2);
+    qtest_quit(global_qtest);
+    g_free(args);
+}
+
+static void test_pc_boot_order(void)
+{
+    test_pc_with_args("", 0x30, 0x12, 0x30, 0x12);
+    test_pc_with_args("-no-fd-bootchk", 0x31, 0x12, 0x31, 0x12);
+    test_pc_with_args("-boot c", 0, 0x02, 0, 0x02);
+    test_pc_with_args("-boot nda", 0x10, 0x34, 0x10, 0x34);
+    test_pc_with_args("-boot order=", 0, 0, 0, 0);
+    test_pc_with_args("-boot order= -boot order=c", 0, 0x02, 0, 0x02);
+    test_pc_with_args("-boot once=a", 0, 0x01, 0x30, 0x12);
+    test_pc_with_args("-boot once=a -no-fd-bootchk", 0x01, 0x01, 0x31, 0x12);
+    test_pc_with_args("-boot once=a,order=c", 0, 0x01, 0, 0x02);
+    test_pc_with_args("-boot once=d -boot order=nda", 0, 0x03, 0x10, 0x34);
+    test_pc_with_args("-boot once=a -boot once=b -boot once=c",
+                      0, 0x02, 0x30, 0x12);
+}
+
+int main(int argc, char *argv[])
+{
+    g_test_init(&argc, &argv, NULL);
+
+    qtest_add_func("boot-order/pc", test_pc_boot_order);
+
+    return g_test_run();
+}
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 09/16] boot-order-test: Add tests for PowerMacs

[Markus Armbruster]

From: Andreas Färber <afaerber@suse.de>

They set the boot device via fw_cfg, which is then translated to a boot
path of "hd" or "cd" in OpenBIOS.

Signed-off-by: Andreas Färber <afaerber@suse.de>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/Makefile          |  2 ++
 tests/boot-order-test.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/tests/Makefile b/tests/Makefile
index 394e029..9653fce 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -67,6 +67,8 @@ gcov-files-sparc-y += hw/m48t59.c
 gcov-files-sparc64-y += hw/m48t59.c
 check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 gcov-files-arm-y += hw/tmp105.c
+check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
+check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
 
diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 2215710..c1cc2a6 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -10,8 +10,10 @@
  * See the COPYING file in the top-level directory.
  */
 
+#include <string.h>
 #include <glib.h>
 #include "libqtest.h"
+#include "qemu/bswap.h"
 
 static void test_pc_cmos_byte(int reg, int expected)
 {
@@ -58,11 +60,73 @@ static void test_pc_boot_order(void)
                       0, 0x02, 0x30, 0x12);
 }
 
+#define G3BEIGE_CFG_ADDR 0xf0000510
+#define MAC99_CFG_ADDR   0xf0000510
+
+#define NO_QEMU_PROTOS
+#include "hw/nvram/fw_cfg.h"
+#undef NO_QEMU_PROTOS
+
+static void powermac_fw_cfg_read(bool newworld, uint16_t cmd,
+                                 uint8_t *buf, unsigned int len)
+{
+    unsigned int i;
+
+    writew(newworld ? MAC99_CFG_ADDR : G3BEIGE_CFG_ADDR, cmd);
+    for (i = 0; i < len; i++) {
+        buf[i] = readb((newworld ? MAC99_CFG_ADDR : G3BEIGE_CFG_ADDR) + 2);
+    }
+}
+
+static uint16_t powermac_fw_cfg_read16(bool newworld, uint16_t cmd)
+{
+    uint16_t value;
+
+    powermac_fw_cfg_read(newworld, cmd, (uint8_t *)&value, sizeof(value));
+    return le16_to_cpu(value);
+}
+
+static void test_powermac_with_args(bool newworld, const char *extra_args,
+                                    uint16_t expected_boot,
+                                    uint16_t expected_reboot)
+{
+    char *args = g_strdup_printf("-nodefaults -display none -machine %s %s",
+                                 newworld ? "mac99" : "g3beige", extra_args);
+    uint16_t actual;
+    qtest_start(args);
+    actual = powermac_fw_cfg_read16(newworld, FW_CFG_BOOT_DEVICE);
+    g_assert_cmphex(actual, ==, expected_boot);
+    qmp("{ 'execute': 'system_reset' }");
+    actual = powermac_fw_cfg_read16(newworld, FW_CFG_BOOT_DEVICE);
+    g_assert_cmphex(actual, ==, expected_reboot);
+    qtest_quit(global_qtest);
+    g_free(args);
+}
+
+static void test_powermac_boot_order(void)
+{
+    int i;
+
+    for (i = 0; i < 2; i++) {
+        bool newworld = (i == 1);
+
+        test_powermac_with_args(newworld, "", 'c', 'c');
+        test_powermac_with_args(newworld, "-boot c", 'c', 'c');
+        test_powermac_with_args(newworld, "-boot d", 'd', 'd');
+    }
+}
+
 int main(int argc, char *argv[])
 {
+    const char *arch = qtest_get_arch();
+
     g_test_init(&argc, &argv, NULL);
 
-    qtest_add_func("boot-order/pc", test_pc_boot_order);
+    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
+        qtest_add_func("boot-order/pc", test_pc_boot_order);
+    } else if (strcmp(arch, "ppc") == 0 || strcmp(arch, "ppc64") == 0) {
+        qtest_add_func("boot-order/powermac", test_powermac_boot_order);
+    }
 
     return g_test_run();
 }
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 10/16] boot-order-test: Cover -boot once in ppc tests

[Markus Armbruster]

Cc: Andreas Färber <afaerber@suse.de>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/boot-order-test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index c1cc2a6..f6dece6 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -113,6 +113,7 @@ static void test_powermac_boot_order(void)
         test_powermac_with_args(newworld, "", 'c', 'c');
         test_powermac_with_args(newworld, "-boot c", 'c', 'c');
         test_powermac_with_args(newworld, "-boot d", 'd', 'd');
+        test_powermac_with_args(newworld, "-boot once=d,order=c", 'd', 'c');
     }
 }
 
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 11/16] boot-order-test: Better separate target-specific and generic parts

[Markus Armbruster]

The initial version did just PC.  I didn't bother to separate out
generic parts, because I don't like to abstract from a single case.

Now we have two cases, PC and PowerMac, and I'm about to add two more.
Time to do it right.

To ease review, this commit changes the code without in-place, and
only the next commit reorders it for better readability.

Cc: Andreas Färber <afaerber@suse.de>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/boot-order-test.c | 160 ++++++++++++++++++++++++++++++------------------
 1 file changed, 99 insertions(+), 61 deletions(-)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index f6dece6..23edacf 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -15,106 +15,141 @@
 #include "libqtest.h"
 #include "qemu/bswap.h"
 
-static void test_pc_cmos_byte(int reg, int expected)
+static uint8_t read_mc146818(uint16_t port, uint8_t reg)
 {
-    int actual;
-
-    outb(0x70, reg);
-    actual = inb(0x71);
-    g_assert_cmphex(actual, ==, expected);
+    outb(port, reg);
+    return inb(port + 1);
 }
 
-static void test_pc_cmos(uint8_t boot1, uint8_t boot2)
+static uint64_t read_boot_order_pc(void)
 {
-    test_pc_cmos_byte(0x38, boot1);
-    test_pc_cmos_byte(0x3d, boot2);
+    uint8_t b1 = read_mc146818(0x70, 0x38);
+    uint8_t b2 = read_mc146818(0x70, 0x3d);
+
+    return b1 | (b2 << 8);
 }
 
-static void test_pc_with_args(const char *test_args,
-                              uint8_t boot1, uint8_t boot2,
-                              uint8_t reboot1, uint8_t reboot2)
+static void test_a_boot_order(const char *machine,
+                              const char *test_args,
+                              uint64_t (*read_boot_order)(void),
+                              uint64_t expected_boot,
+                              uint64_t expected_reboot)
 {
-    char *args = g_strdup_printf("-nodefaults -display none %s", test_args);
+    char *args;
+    uint64_t actual;
 
+    args = g_strdup_printf("-nodefaults -display none%s%s %s",
+                           machine ? " -M " : "",
+                           machine ?: "",
+                           test_args);
     qtest_start(args);
-    test_pc_cmos(boot1, boot2);
+    actual = read_boot_order();
+    g_assert_cmphex(actual, ==, expected_boot);
     qmp("{ 'execute': 'system_reset' }");
-    test_pc_cmos(reboot1, reboot2);
+    actual = read_boot_order();
+    g_assert_cmphex(actual, ==, expected_reboot);
     qtest_quit(global_qtest);
     g_free(args);
 }
 
+typedef struct {
+    const char *args;
+    uint64_t expected_boot;
+    uint64_t expected_reboot;
+} boot_order_test;
+
+static void test_boot_orders(const char *machine,
+                             uint64_t (*read_boot_order)(void),
+                             const boot_order_test *tests)
+{
+    int i;
+
+    for (i = 0; tests[i].args; i++) {
+        test_a_boot_order(machine, tests[i].args,
+                          read_boot_order,
+                          tests[i].expected_boot,
+                          tests[i].expected_reboot);
+    }
+}
+
+static const boot_order_test test_cases_pc[] = {
+    { "",
+      0x1230, 0x1230 },
+    { "-no-fd-bootchk",
+      0x1231, 0x1231 },
+    { "-boot c",
+      0x0200, 0x0200 },
+    { "-boot nda",
+      0x3410, 0x3410 },
+    { "-boot order=",
+      0, 0 },
+    { "-boot order= -boot order=c",
+      0x0200, 0x0200 },
+    { "-boot once=a",
+      0x0100, 0x1230 },
+    { "-boot once=a -no-fd-bootchk",
+      0x0101, 0x1231 },
+    { "-boot once=a,order=c",
+      0x0100, 0x0200 },
+    { "-boot once=d -boot order=nda",
+      0x0300, 0x3410 },
+    { "-boot once=a -boot once=b -boot once=c",
+      0x0200, 0x1230 },
+    {}
+};
+
 static void test_pc_boot_order(void)
 {
-    test_pc_with_args("", 0x30, 0x12, 0x30, 0x12);
-    test_pc_with_args("-no-fd-bootchk", 0x31, 0x12, 0x31, 0x12);
-    test_pc_with_args("-boot c", 0, 0x02, 0, 0x02);
-    test_pc_with_args("-boot nda", 0x10, 0x34, 0x10, 0x34);
-    test_pc_with_args("-boot order=", 0, 0, 0, 0);
-    test_pc_with_args("-boot order= -boot order=c", 0, 0x02, 0, 0x02);
-    test_pc_with_args("-boot once=a", 0, 0x01, 0x30, 0x12);
-    test_pc_with_args("-boot once=a -no-fd-bootchk", 0x01, 0x01, 0x31, 0x12);
-    test_pc_with_args("-boot once=a,order=c", 0, 0x01, 0, 0x02);
-    test_pc_with_args("-boot once=d -boot order=nda", 0, 0x03, 0x10, 0x34);
-    test_pc_with_args("-boot once=a -boot once=b -boot once=c",
-                      0, 0x02, 0x30, 0x12);
+    test_boot_orders(NULL, read_boot_order_pc, test_cases_pc);
 }
 
-#define G3BEIGE_CFG_ADDR 0xf0000510
-#define MAC99_CFG_ADDR   0xf0000510
+#define PMAC_CFG_ADDR 0xf0000510
 
 #define NO_QEMU_PROTOS
 #include "hw/nvram/fw_cfg.h"
 #undef NO_QEMU_PROTOS
 
-static void powermac_fw_cfg_read(bool newworld, uint16_t cmd,
-                                 uint8_t *buf, unsigned int len)
+static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
+                        void *buf, size_t len)
 {
-    unsigned int i;
+    uint8_t *p = buf;
+    size_t i;
 
-    writew(newworld ? MAC99_CFG_ADDR : G3BEIGE_CFG_ADDR, cmd);
+    writew(cfg_addr, cmd);
     for (i = 0; i < len; i++) {
-        buf[i] = readb((newworld ? MAC99_CFG_ADDR : G3BEIGE_CFG_ADDR) + 2);
+        p[i] = readb(cfg_addr + 2);
     }
 }
 
-static uint16_t powermac_fw_cfg_read16(bool newworld, uint16_t cmd)
+static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
 {
     uint16_t value;
 
-    powermac_fw_cfg_read(newworld, cmd, (uint8_t *)&value, sizeof(value));
+    read_fw_cfg(cfg_addr, cmd, &value, sizeof(value));
     return le16_to_cpu(value);
 }
 
-static void test_powermac_with_args(bool newworld, const char *extra_args,
-                                    uint16_t expected_boot,
-                                    uint16_t expected_reboot)
+static uint64_t read_boot_order_pmac(void)
 {
-    char *args = g_strdup_printf("-nodefaults -display none -machine %s %s",
-                                 newworld ? "mac99" : "g3beige", extra_args);
-    uint16_t actual;
-    qtest_start(args);
-    actual = powermac_fw_cfg_read16(newworld, FW_CFG_BOOT_DEVICE);
-    g_assert_cmphex(actual, ==, expected_boot);
-    qmp("{ 'execute': 'system_reset' }");
-    actual = powermac_fw_cfg_read16(newworld, FW_CFG_BOOT_DEVICE);
-    g_assert_cmphex(actual, ==, expected_reboot);
-    qtest_quit(global_qtest);
-    g_free(args);
+    return read_fw_cfg_i16(PMAC_CFG_ADDR, FW_CFG_BOOT_DEVICE);
 }
 
-static void test_powermac_boot_order(void)
-{
-    int i;
+static const boot_order_test test_cases_fw_cfg[] = {
+    { "", 'c', 'c' },
+    { "-boot c", 'c', 'c' },
+    { "-boot d", 'd', 'd' },
+    { "-boot once=d,order=c", 'd', 'c' },
+    {}
+};
 
-    for (i = 0; i < 2; i++) {
-        bool newworld = (i == 1);
+static void test_pmac_oldworld_boot_order(void)
+{
+    test_boot_orders("g3beige", read_boot_order_pmac, test_cases_fw_cfg);
+}
 
-        test_powermac_with_args(newworld, "", 'c', 'c');
-        test_powermac_with_args(newworld, "-boot c", 'c', 'c');
-        test_powermac_with_args(newworld, "-boot d", 'd', 'd');
-        test_powermac_with_args(newworld, "-boot once=d,order=c", 'd', 'c');
-    }
+static void test_pmac_newworld_boot_order(void)
+{
+    test_boot_orders("mac99", read_boot_order_pmac, test_cases_fw_cfg);
 }
 
 int main(int argc, char *argv[])
@@ -126,7 +161,10 @@ int main(int argc, char *argv[])
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         qtest_add_func("boot-order/pc", test_pc_boot_order);
     } else if (strcmp(arch, "ppc") == 0 || strcmp(arch, "ppc64") == 0) {
-        qtest_add_func("boot-order/powermac", test_powermac_boot_order);
+        qtest_add_func("boot-order/pmac_oldworld",
+                       test_pmac_oldworld_boot_order);
+        qtest_add_func("boot-order/pmac_newworld",
+                       test_pmac_newworld_boot_order);
     }
 
     return g_test_run();
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 12/16] boot-order-test: Code motion for better readability

[Markus Armbruster]

Cc: Andreas Färber <afaerber@suse.de>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/boot-order-test.c | 48 ++++++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 23edacf..003140f 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -15,19 +15,15 @@
 #include "libqtest.h"
 #include "qemu/bswap.h"
 
-static uint8_t read_mc146818(uint16_t port, uint8_t reg)
-{
-    outb(port, reg);
-    return inb(port + 1);
-}
-
-static uint64_t read_boot_order_pc(void)
-{
-    uint8_t b1 = read_mc146818(0x70, 0x38);
-    uint8_t b2 = read_mc146818(0x70, 0x3d);
+#define NO_QEMU_PROTOS
+#include "hw/nvram/fw_cfg.h"
+#undef NO_QEMU_PROTOS
 
-    return b1 | (b2 << 8);
-}
+typedef struct {
+    const char *args;
+    uint64_t expected_boot;
+    uint64_t expected_reboot;
+} boot_order_test;
 
 static void test_a_boot_order(const char *machine,
                               const char *test_args,
@@ -52,12 +48,6 @@ static void test_a_boot_order(const char *machine,
     g_free(args);
 }
 
-typedef struct {
-    const char *args;
-    uint64_t expected_boot;
-    uint64_t expected_reboot;
-} boot_order_test;
-
 static void test_boot_orders(const char *machine,
                              uint64_t (*read_boot_order)(void),
                              const boot_order_test *tests)
@@ -72,6 +62,20 @@ static void test_boot_orders(const char *machine,
     }
 }
 
+static uint8_t read_mc146818(uint16_t port, uint8_t reg)
+{
+    outb(port, reg);
+    return inb(port + 1);
+}
+
+static uint64_t read_boot_order_pc(void)
+{
+    uint8_t b1 = read_mc146818(0x70, 0x38);
+    uint8_t b2 = read_mc146818(0x70, 0x3d);
+
+    return b1 | (b2 << 8);
+}
+
 static const boot_order_test test_cases_pc[] = {
     { "",
       0x1230, 0x1230 },
@@ -103,12 +107,6 @@ static void test_pc_boot_order(void)
     test_boot_orders(NULL, read_boot_order_pc, test_cases_pc);
 }
 
-#define PMAC_CFG_ADDR 0xf0000510
-
-#define NO_QEMU_PROTOS
-#include "hw/nvram/fw_cfg.h"
-#undef NO_QEMU_PROTOS
-
 static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
                         void *buf, size_t len)
 {
@@ -129,6 +127,8 @@ static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
     return le16_to_cpu(value);
 }
 
+#define PMAC_CFG_ADDR 0xf0000510
+
 static uint64_t read_boot_order_pmac(void)
 {
     return read_fw_cfg_i16(PMAC_CFG_ADDR, FW_CFG_BOOT_DEVICE);
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 13/16] boot-order-test: Add tests for PowerPC PREP

[Markus Armbruster]

Cc: Andreas Färber <afaerber@suse.de>
Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc@nongnu.org
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/boot-order-test.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 003140f..0060905 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -107,6 +107,32 @@ static void test_pc_boot_order(void)
     test_boot_orders(NULL, read_boot_order_pc, test_cases_pc);
 }
 
+static uint8_t read_m48t59(uint64_t addr, uint16_t reg)
+{
+    writeb(addr, reg & 0xff);
+    writeb(addr + 1, reg >> 8);
+    return readb(addr + 3);
+}
+
+#define PREP_ISA_IO_BASE 0x80000000
+
+static uint64_t read_boot_order_prep(void)
+{
+    return read_m48t59(PREP_ISA_IO_BASE + 0x74, 0x34);
+}
+
+static const boot_order_test test_cases_prep[] = {
+    { "", 'c', 'c' },
+    { "-boot c", 'c', 'c' },
+    { "-boot d", 'd', 'd' },
+    {}
+};
+
+static void test_prep_boot_order(void)
+{
+    test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
+}
+
 static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
                         void *buf, size_t len)
 {
@@ -161,6 +187,7 @@ int main(int argc, char *argv[])
     if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
         qtest_add_func("boot-order/pc", test_pc_boot_order);
     } else if (strcmp(arch, "ppc") == 0 || strcmp(arch, "ppc64") == 0) {
+        qtest_add_func("boot-order/prep", test_prep_boot_order);
         qtest_add_func("boot-order/pmac_oldworld",
                        test_pmac_oldworld_boot_order);
         qtest_add_func("boot-order/pmac_newworld",
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 14/16] boot-order-test: Add tests for Sun4m

[Markus Armbruster]

Cc: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/boot-order-test.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 0060905..7b1edc1 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -178,6 +178,18 @@ static void test_pmac_newworld_boot_order(void)
     test_boot_orders("mac99", read_boot_order_pmac, test_cases_fw_cfg);
 }
 
+#define SUN4M_CFG_ADDR 0xd00000510ULL
+
+static uint64_t read_boot_order_sun4m(void)
+{
+    return read_fw_cfg_i16(SUN4M_CFG_ADDR, FW_CFG_BOOT_DEVICE);
+}
+
+static void test_sun4m_boot_order(void)
+{
+    test_boot_orders("SS-5", read_boot_order_sun4m, test_cases_fw_cfg);
+}
+
 int main(int argc, char *argv[])
 {
     const char *arch = qtest_get_arch();
@@ -192,6 +204,8 @@ int main(int argc, char *argv[])
                        test_pmac_oldworld_boot_order);
         qtest_add_func("boot-order/pmac_newworld",
                        test_pmac_newworld_boot_order);
+    } else if (strcmp(arch, "sparc") == 0) {
+        qtest_add_func("boot-order/sun4m", test_sun4m_boot_order);
     }
 
     return g_test_run();
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 15/16] boot-order-test: Support fw_cfg in I/O space

[Markus Armbruster]

Next commit needs it.

Cc: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/boot-order-test.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index 7b1edc1..d1d99f8 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -133,23 +133,31 @@ static void test_prep_boot_order(void)
     test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
 }
 
-static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
+static void read_fw_cfg(uint64_t cfg_addr, bool addr_is_io, uint16_t cmd,
                         void *buf, size_t len)
 {
     uint8_t *p = buf;
     size_t i;
 
-    writew(cfg_addr, cmd);
-    for (i = 0; i < len; i++) {
-        p[i] = readb(cfg_addr + 2);
+    if (addr_is_io) {
+        outw(cfg_addr, cmd);
+        for (i = 0; i < len; i++) {
+            p[i] = inb(cfg_addr + 1);
+        }
+    } else {
+        writew(cfg_addr, cmd);
+        for (i = 0; i < len; i++) {
+            p[i] = readb(cfg_addr + 2);
+        }
     }
 }
 
-static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
+static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, bool addr_is_io,
+                                    uint16_t cmd)
 {
     uint16_t value;
 
-    read_fw_cfg(cfg_addr, cmd, &value, sizeof(value));
+    read_fw_cfg(cfg_addr, addr_is_io, cmd, &value, sizeof(value));
     return le16_to_cpu(value);
 }
 
@@ -157,7 +165,7 @@ static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
 
 static uint64_t read_boot_order_pmac(void)
 {
-    return read_fw_cfg_i16(PMAC_CFG_ADDR, FW_CFG_BOOT_DEVICE);
+    return read_fw_cfg_i16(PMAC_CFG_ADDR, false, FW_CFG_BOOT_DEVICE);
 }
 
 static const boot_order_test test_cases_fw_cfg[] = {
@@ -182,7 +190,7 @@ static void test_pmac_newworld_boot_order(void)
 
 static uint64_t read_boot_order_sun4m(void)
 {
-    return read_fw_cfg_i16(SUN4M_CFG_ADDR, FW_CFG_BOOT_DEVICE);
+    return read_fw_cfg_i16(SUN4M_CFG_ADDR, false, FW_CFG_BOOT_DEVICE);
 }
 
 static void test_sun4m_boot_order(void)
-- 
1.7.11.7

[Qemu-devel] [PATCH v3 16/16] boot-order-test: Add tests for Sun4u

[Markus Armbruster]

Cc: Blue Swirl <blauwirbel@gmail.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/boot-order-test.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
index d1d99f8..37c7227 100644
--- a/tests/boot-order-test.c
+++ b/tests/boot-order-test.c
@@ -198,6 +198,18 @@ static void test_sun4m_boot_order(void)
     test_boot_orders("SS-5", read_boot_order_sun4m, test_cases_fw_cfg);
 }
 
+#define SUN4U_CFG_IOPORT 0x510
+
+static uint64_t read_boot_order_sun4u(void)
+{
+    return read_fw_cfg_i16(SUN4U_CFG_IOPORT, true, FW_CFG_BOOT_DEVICE);
+}
+
+static void test_sun4u_boot_order(void)
+{
+    test_boot_orders("sun4u", read_boot_order_sun4u, test_cases_fw_cfg);
+}
+
 int main(int argc, char *argv[])
 {
     const char *arch = qtest_get_arch();
@@ -214,6 +226,8 @@ int main(int argc, char *argv[])
                        test_pmac_newworld_boot_order);
     } else if (strcmp(arch, "sparc") == 0) {
         qtest_add_func("boot-order/sun4m", test_sun4m_boot_order);
+    } else if (strcmp(arch, "sparc64") == 0) {
+        qtest_add_func("boot-order/sun4u", test_sun4u_boot_order);
     }
 
     return g_test_run();
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH v3 01/16] vl: Clean up parsing of -boot option argument

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Commit 3d3b8303 threw in some QemuOpts parsing without replacing the
> existing ad hoc parser, resulting in a confusing mess.  Clean it up.
>
> Two user-visible changes:
>
> 1. Invalid options are reported more nicely.  Before:
>
>         qemu: unknown boot parameter 'x' in 'x=y'
>
>    After:
>
>         qemu-system-x86_64: -boot x=y: Invalid parameter 'x'
>
> 2. If -boot is given multiple times, options accumulate, just like for
>    -machine.  Before, only options order, once and menu accumulated.
>    For the other ones, all but the first -boot in non-legacy syntax
>    got simply ignored.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  vl.c | 84 ++++++++++++++++++--------------------------------------------------
>  1 file changed, 22 insertions(+), 62 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 180fdde..a0ac6e9 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -436,9 +436,10 @@ static QemuOptsList qemu_machine_opts = {
>  
>  static QemuOptsList qemu_boot_opts = {
>      .name = "boot-opts",
> +    .implied_opt_name = "order",
> +    .merge_lists = true,
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_boot_opts.head),
>      .desc = {
> -        /* the three names below are not used now */
>          {
>              .name = "order",
>              .type = QEMU_OPT_STRING,
> @@ -447,8 +448,7 @@ static QemuOptsList qemu_boot_opts = {
>              .type = QEMU_OPT_STRING,
>          }, {
>              .name = "menu",
> -            .type = QEMU_OPT_STRING,
> -        /* following are really used */
> +            .type = QEMU_OPT_BOOL,
>          }, {
>              .name = "splash",
>              .type = QEMU_OPT_STRING,
> @@ -1159,7 +1159,7 @@ int qemu_boot_set(const char *boot_devices)
>      return boot_set_handler(boot_set_opaque, boot_devices);
>  }
>  
> -static void validate_bootdevices(char *devices)
> +static void validate_bootdevices(const char *devices)
>  {
>      /* We just do some generic consistency checks */
>      const char *p;
> @@ -3134,70 +3134,30 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_boot:
>                  {
> -                    static const char * const params[] = {
> -                        "order", "once", "menu",
> -                        "splash", "splash-time",
> -                        "reboot-timeout", "strict", NULL
> -                    };
> -                    char buf[sizeof(boot_devices)];
>                      char *standard_boot_devices;
> -                    int legacy = 0;
> -
> -                    if (!strchr(optarg, '=')) {
> -                        legacy = 1;
> -                        pstrcpy(buf, sizeof(buf), optarg);
> -                    } else if (check_params(buf, sizeof(buf), params, optarg) < 0) {
> -                        fprintf(stderr,
> -                                "qemu: unknown boot parameter '%s' in '%s'\n",
> -                                buf, optarg);
> +                    const char *order, *once;
> +
> +                    opts = qemu_opts_parse(qemu_find_opts("boot-opts"),
> +                                           optarg, 1);
> +                    if (!opts) {
>                          exit(1);
>                      }
>  
> -                    if (legacy ||
> -                        get_param_value(buf, sizeof(buf), "order", optarg)) {
> -                        validate_bootdevices(buf);
> -                        pstrcpy(boot_devices, sizeof(boot_devices), buf);
> +                    order = qemu_opt_get(opts, "order");
> +                    if (order) {
> +                        validate_bootdevices(order);
> +                        pstrcpy(boot_devices, sizeof(boot_devices), order);
>                      }
> -                    if (!legacy) {
> -                        if (get_param_value(buf, sizeof(buf),
> -                                            "once", optarg)) {
> -                            validate_bootdevices(buf);
> -                            standard_boot_devices = g_strdup(boot_devices);
> -                            pstrcpy(boot_devices, sizeof(boot_devices), buf);
> -                            qemu_register_reset(restore_boot_devices,
> -                                                standard_boot_devices);
> -                        }
> -                        if (get_param_value(buf, sizeof(buf),
> -                                            "menu", optarg)) {
> -                            if (!strcmp(buf, "on")) {
> -                                boot_menu = 1;
> -                            } else if (!strcmp(buf, "off")) {
> -                                boot_menu = 0;
> -                            } else {
> -                                fprintf(stderr,
> -                                        "qemu: invalid option value '%s'\n",
> -                                        buf);
> -                                exit(1);
> -                            }
> -                        }
> -                        if (get_param_value(buf, sizeof(buf),
> -                                            "strict", optarg)) {
> -                            if (!strcmp(buf, "on")) {
> -                                boot_strict = true;
> -                            } else if (!strcmp(buf, "off")) {
> -                                boot_strict = false;
> -                            } else {
> -                                fprintf(stderr,
> -                                        "qemu: invalid option value '%s'\n",
> -                                        buf);
> -                                exit(1);
> -                            }
> -                        }
> -                        if (!qemu_opts_parse(qemu_find_opts("boot-opts"),
> -                                             optarg, 0)) {
> -                            exit(1);
> -                        }
> +
> +                    once = qemu_opt_get(opts, "once");
> +                    if (once) {
> +                        validate_bootdevices(once);
> +                        standard_boot_devices = g_strdup(boot_devices);
> +                        pstrcpy(boot_devices, sizeof(boot_devices), once);
> +                        qemu_register_reset(restore_boot_devices,
> +                                            standard_boot_devices);
>                      }
> +                    boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
>                  }
>                  break;
>              case QEMU_OPTION_fda:
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 02/16] qemu-option: check_params() is now unused, drop it

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  include/qemu/option.h |  2 --
>  util/qemu-option.c    | 30 ------------------------------
>  2 files changed, 32 deletions(-)
>
> diff --git a/include/qemu/option.h b/include/qemu/option.h
> index bdb6d21..a83c700 100644
> --- a/include/qemu/option.h
> +++ b/include/qemu/option.h
> @@ -55,8 +55,6 @@ int get_next_param_value(char *buf, int buf_size,
>                           const char *tag, const char **pstr);
>  int get_param_value(char *buf, int buf_size,
>                      const char *tag, const char *str);
> -int check_params(char *buf, int buf_size,
> -                 const char * const *params, const char *str);
>  
>  
>  /*
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 8b74bf1..412c425 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -123,36 +123,6 @@ int get_param_value(char *buf, int buf_size,
>      return get_next_param_value(buf, buf_size, tag, &str);
>  }
>  
> -int check_params(char *buf, int buf_size,
> -                 const char * const *params, const char *str)
> -{
> -    const char *p;
> -    int i;
> -
> -    p = str;
> -    while (*p != '\0') {
> -        p = get_opt_name(buf, buf_size, p, '=');
> -        if (*p != '=') {
> -            return -1;
> -        }
> -        p++;
> -        for (i = 0; params[i] != NULL; i++) {
> -            if (!strcmp(params[i], buf)) {
> -                break;
> -            }
> -        }
> -        if (params[i] == NULL) {
> -            return -1;
> -        }
> -        p = get_opt_value(NULL, 0, p);
> -        if (*p != ',') {
> -            break;
> -        }
> -        p++;
> -    }
> -    return 0;
> -}
> -
>  /*
>   * Searches an option list for an option with the given name
>   */
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 03/16] vl: Fix -boot order and once regressions, and related bugs

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Option "once" sets up a different boot order just for the initial
> boot.  Boot order reverts back to normal on reset.  Option "order"
> changes the normal boot order.
>
> The reversal is implemented by reset handler restore_boot_devices(),
> which takes the boot order to revert to as argument.
> restore_boot_devices() does nothing on its first call, because that
> must be the initial machine reset.  On its second call, it changes the
> boot order back, and unregisters itself.
>
> Because we register the handler right when -boot gets parsed, we can
> revert to an incorrect normal boot order, and multiple -boot can
> interact in funny ways.
>
> Here's how things work without -boot once or order:
>
> * boot_devices is "".
>
> * main() passes machine->boot_order to to machine->init(), because
>   boot_devices is "".  machine->init() configures firmware
>   accordingly.  For PC machines, machine->boot_order is "cad", and
>   pc_cmos_init() writes it to RTC CMOS, where SeaBIOS picks it up.
>
> Now consider -boot order=:
>
> * boot_devices is "".
>
> * -boot order= sets boot_devices to "" (no change).
>
> * main() passes machine->boot_order to to machine->init(), because
>   boot_devices is "", as above.
>
>   Bug: -boot order= has no effect.  Broken in commit e4ada29e.
>
> Next, consider -boot once=a:
>
> * boot_devices is "".
>
> * -boot once=a registers restore_boot_devices() with argument "", and
>   sets boot_devices to "a".
>
> * main() passes boot_devices "a" to machine->init(), which configures
>   firmware accordingly.  For PC machines, pc_cmos_init() writes the
>   boot order to RTC CMOS.
>
> * main() calls qemu_system_reset().  This runs reset handlers.
>
>   - restore_boot_devices() gets called with argument "".  Does
>     nothing, because it's the first call.
>
> * Machine boots, boot order is "a".
>
> * Machine resets (e.g. monitor command).  Reset handlers run.
>
>   - restore_boot_devices() gets called with argument "".  Calls
>     qemu_boot_set("") to reconfigure firmware.  For PC machines,
>     pc_boot_set() writes it into RTC CMOS.  Reset handler
>     unregistered.
>
>     Bug: boot order reverts to "" instead of machine->boot_order.  The
>     actual boot order depends on how firmware interprets "".  Broken
>     in commit e4ada29e.
>
> Next, consider -boot once=a -boot order=c:
>
> * boot_devices is "".
>
> * -boot once=a registers restore_boot_devices() with argument "", and
>   sets boot_devices to "a".
>
> * -boot order=c sets boot_devices to "c".
>
> * main() passes boot_devices "c" to machine->init(), which configures
>   firmware accordingly.  For PC machines, pc_cmos_init() writes the
>   boot order to RTC CMOS.
>
> * main() calls qemu_system_reset().  This runs reset handlers.
>
>   - restore_boot_devices() gets called with argument "".  Does
>     nothing, because it's the first call.
>
> * Machine boots, boot order is "c".
>
>   Bug: it should be "a".  I figure this has always been broken.
>
> * Machine resets (e.g. monitor command).  Reset handlers run.
>
>   - restore_boot_devices() gets called with argument "".  Calls
>     qemu_boot_set("") to reconfigure firmware.  For PC machines,
>     pc_boot_set() writes it into RTC CMOS.  Reset handler
>     unregistered.
>
>     Bug: boot order reverts to "" instead of "c".  I figure this has
>     always been broken, just differently broken before commit
>     e4ada29e.
>
> Next, consider -boot once=a -boot once=b -boot once=c:
>
> * boot_devices is "".
>
> * -boot once=a registers restore_boot_devices() with argument "", and
>   sets boot_devices to "a".
>
> * -boot once=b registers restore_boot_devices() with argument "a", and
>   sets boot_devices to "b".
>
> * -boot once=c registers restore_boot_devices() with argument "b", and
>   sets boot_devices to "c".
>
> * main() passes boot_devices "c" to machine->init(), which configures
>   firmware accordingly.  For PC machines, pc_cmos_init() writes the
>   boot order to RTC CMOS.
>
> * main() calls qemu_system_reset().  This runs reset handlers.
>
>   - restore_boot_devices() gets called with argument "".  Does
>     nothing, because it's the first call.
>
>   - restore_boot_devices() gets called with argument "a".  Calls
>     qemu_boot_set("a") to reconfigure firmware.  For PC machines,
>     pc_boot_set() writes it into RTC CMOS.  Reset handler
>     unregistered.
>
>   - restore_boot_devices() gets called with argument "b".  Calls
>     qemu_boot_set("b") to reconfigure firmware.  For PC machines,
>     pc_boot_set() writes it into RTC CMOS.  Reset handler
>     unregistered.
>
> * Machine boots, boot order is "b".
>
>   Bug: should really be "c", because that came last, and for all other
>   -boot options, the last one wins.  I figure this was broken some
>   time before commit 37905d6a, and fixed there only for a single
>   occurence of "once".
>
> * Machine resets (e.g. monitor command).  Reset handlers run.
>
>   - restore_boot_devices() gets called with argument "".  Calls
>     qemu_boot_set("") to reconfigure firmware.  For PC machines,
>     pc_boot_set() writes it into RTC CMOS.  Reset handler
>     unregistered.
>
>     Same bug as above: boot order reverts to "" instead of
>     machine->boot_order.
>
> Fix by acting upon -boot options order, once and menu only after
> option parsing is complete, and the machine is known.  This is how the
> other -boot options work already.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  vl.c | 59 ++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index a0ac6e9..f51d8e8 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2843,7 +2843,7 @@ int main(int argc, char **argv, char **envp)
>      const char *icount_option = NULL;
>      const char *initrd_filename;
>      const char *kernel_filename, *kernel_cmdline;
> -    char boot_devices[33] = "";
> +    const char *boot_order = NULL;
>      DisplayState *ds;
>      int cyls, heads, secs, translation;
>      QemuOpts *hda_opts = NULL, *opts, *machine_opts;
> @@ -3133,31 +3133,9 @@ int main(int argc, char **argv, char **envp)
>                  drive_add(IF_DEFAULT, 2, optarg, CDROM_OPTS);
>                  break;
>              case QEMU_OPTION_boot:
> -                {
> -                    char *standard_boot_devices;
> -                    const char *order, *once;
> -
> -                    opts = qemu_opts_parse(qemu_find_opts("boot-opts"),
> -                                           optarg, 1);
> -                    if (!opts) {
> -                        exit(1);
> -                    }
> -
> -                    order = qemu_opt_get(opts, "order");
> -                    if (order) {
> -                        validate_bootdevices(order);
> -                        pstrcpy(boot_devices, sizeof(boot_devices), order);
> -                    }
> -
> -                    once = qemu_opt_get(opts, "once");
> -                    if (once) {
> -                        validate_bootdevices(once);
> -                        standard_boot_devices = g_strdup(boot_devices);
> -                        pstrcpy(boot_devices, sizeof(boot_devices), once);
> -                        qemu_register_reset(restore_boot_devices,
> -                                            standard_boot_devices);
> -                    }
> -                    boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
> +                opts = qemu_opts_parse(qemu_find_opts("boot-opts"), optarg, 1);
> +                if (!opts) {
> +                    exit(1);
>                  }
>                  break;
>              case QEMU_OPTION_fda:
> @@ -4093,6 +4071,31 @@ int main(int argc, char **argv, char **envp)
>          kernel_filename = initrd_filename = kernel_cmdline = NULL;
>      }
>  
> +    if (!boot_order) {
> +        boot_order = machine->boot_order;
> +    }
> +    opts = qemu_opts_find(qemu_find_opts("boot-opts"), NULL);
> +    if (opts) {
> +        char *normal_boot_order;
> +        const char *order, *once;
> +
> +        order = qemu_opt_get(opts, "order");
> +        if (order) {
> +            validate_bootdevices(order);
> +            boot_order = order;
> +        }
> +
> +        once = qemu_opt_get(opts, "once");
> +        if (once) {
> +            validate_bootdevices(once);
> +            normal_boot_order = g_strdup(boot_order);
> +            boot_order = once;
> +            qemu_register_reset(restore_boot_devices, normal_boot_order);
> +        }
> +
> +        boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
> +    }
> +
>      if (!kernel_cmdline) {
>          kernel_cmdline = "";
>      }
> @@ -4257,9 +4260,7 @@ int main(int argc, char **argv, char **envp)
>      qdev_machine_init();
>  
>      QEMUMachineInitArgs args = { .ram_size = ram_size,
> -                                 .boot_device = (boot_devices[0] == '\0') ?
> -                                                machine->boot_order :
> -                                                boot_devices,
> +                                 .boot_device = boot_order,
>                                   .kernel_filename = kernel_filename,
>                                   .kernel_cmdline = kernel_cmdline,
>                                   .initrd_filename = initrd_filename,
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 04/16] vl: Rename *boot_devices to *boot_order, for consistency

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  include/hw/hw.h |  4 ++--
>  vl.c            | 16 ++++++++--------
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/include/hw/hw.h b/include/hw/hw.h
> index 1fb9afa..cc9f847 100644
> --- a/include/hw/hw.h
> +++ b/include/hw/hw.h
> @@ -44,9 +44,9 @@ void qemu_unregister_reset(QEMUResetHandler *func, void *opaque);
>  
>  /* handler to set the boot_device order for a specific type of QEMUMachine */
>  /* return 0 if success */
> -typedef int QEMUBootSetHandler(void *opaque, const char *boot_devices);
> +typedef int QEMUBootSetHandler(void *opaque, const char *boot_order);
>  void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque);
> -int qemu_boot_set(const char *boot_devices);
> +int qemu_boot_set(const char *boot_order);
>  
>  #ifdef NEED_CPU_H
>  #if TARGET_LONG_BITS == 64
> diff --git a/vl.c b/vl.c
> index f51d8e8..17daedd 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1151,12 +1151,12 @@ void qemu_register_boot_set(QEMUBootSetHandler *func, void *opaque)
>      boot_set_opaque = opaque;
>  }
>  
> -int qemu_boot_set(const char *boot_devices)
> +int qemu_boot_set(const char *boot_order)
>  {
>      if (!boot_set_handler) {
>          return -EINVAL;
>      }
> -    return boot_set_handler(boot_set_opaque, boot_devices);
> +    return boot_set_handler(boot_set_opaque, boot_order);
>  }
>  
>  static void validate_bootdevices(const char *devices)
> @@ -1187,9 +1187,9 @@ static void validate_bootdevices(const char *devices)
>      }
>  }
>  
> -static void restore_boot_devices(void *opaque)
> +static void restore_boot_order(void *opaque)
>  {
> -    char *standard_boot_devices = opaque;
> +    char *normal_boot_order = opaque;
>      static int first = 1;
>  
>      /* Restore boot order and remove ourselves after the first boot */
> @@ -1198,10 +1198,10 @@ static void restore_boot_devices(void *opaque)
>          return;
>      }
>  
> -    qemu_boot_set(standard_boot_devices);
> +    qemu_boot_set(normal_boot_order);
>  
> -    qemu_unregister_reset(restore_boot_devices, standard_boot_devices);
> -    g_free(standard_boot_devices);
> +    qemu_unregister_reset(restore_boot_order, normal_boot_order);
> +    g_free(normal_boot_order);
>  }
>  
>  void add_boot_device_path(int32_t bootindex, DeviceState *dev,
> @@ -4090,7 +4090,7 @@ int main(int argc, char **argv, char **envp)
>              validate_bootdevices(once);
>              normal_boot_order = g_strdup(boot_order);
>              boot_order = once;
> -            qemu_register_reset(restore_boot_devices, normal_boot_order);
> +            qemu_register_reset(restore_boot_order, normal_boot_order);
>          }
>  
>          boot_menu = qemu_opt_get_bool(opts, "menu", boot_menu);
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 05/16] pc: Make -no-fd-bootchk stick across boot order changes

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Option -no-fd-bootchk asks the BIOS to attempt booting from a floppy
> even when the boot sector signature isn't there, by setting a bit in
> RTC CMOS.  It was added back in 2006 (commit 52ca8d6a).
>
> Two years later, commit 0ecdffbb added monitor command boot_set.
> Implemented by new function pc_boot_set().  It unconditionally clears
> the floppy signature bit in CMOS.
>
> Commit e0f084bf added -boot option once to automatically change the
> boot order on first reset.  Reuses pc_boot_set(), thus also clears the
> floppy signature bit.  Commit d9346e81 took care to preserve this
> behavior.

Quite a history there :-)

Does anyone still use no-fd-bootchk?  Do you know what the original
use-case was?

> Thus, -no-fd-bootchk applies to any number of boots.  Except it
> applies just to the first boot with -boot once, and never after
> boot_set.  Weird.  Make it stick instead: set the bit according to
> -no-fd-bootchk in pc_boot_set().
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/i386/pc.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 4844a6b..7e524fc 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -266,7 +266,7 @@ static int boot_device2nibble(char boot_device)
>      return 0;
>  }
>  
> -static int set_boot_dev(ISADevice *s, const char *boot_device, int fd_bootchk)
> +static int set_boot_dev(ISADevice *s, const char *boot_device)
>  {
>  #define PC_MAX_BOOT_DEVICES 3
>      int nbds, bds[3] = { 0, };
> @@ -292,7 +292,7 @@ static int set_boot_dev(ISADevice *s, const char *boot_device, int fd_bootchk)
>  
>  static int pc_boot_set(void *opaque, const char *boot_device)
>  {
> -    return set_boot_dev(opaque, boot_device, 0);
> +    return set_boot_dev(opaque, boot_device);
>  }
>  
>  typedef struct pc_cmos_init_late_arg {
> @@ -407,8 +407,7 @@ void pc_cmos_init(ram_addr_t ram_size, ram_addr_t above_4g_mem_size,
>      cpu_hotplug_cb.cpu_added_notifier.notify = rtc_notify_cpu_added;
>      qemu_register_cpu_added_notifier(&cpu_hotplug_cb.cpu_added_notifier);
>  
> -    /* set boot devices, and disable floppy signature check if requested */
> -    if (set_boot_dev(s, boot_device, fd_bootchk)) {
> +    if (set_boot_dev(s, boot_device)) {
>          exit(1);
>      }
>  
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 06/16] doc: Drop ref to Bochs from -no-fd-bootchk documentation

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Manual page and qemu-doc on talk about "Bochs BIOS".  We use SeaBIOS,
> and it implements the feature.  Replace by just "BIOS", and drop the
> TODO line wondering about the Bochs reference.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  qemu-options.hx | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index bf94862..8355f9b 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1268,9 +1268,8 @@ DEF("no-fd-bootchk", 0, QEMU_OPTION_no_fd_bootchk,
>  STEXI
>  @item -no-fd-bootchk
>  @findex -no-fd-bootchk
> -Disable boot signature checking for floppy disks in Bochs BIOS. It may
> +Disable boot signature checking for floppy disks in BIOS. May
>  be needed to boot from old floppy disks.
> -TODO: check reference to Bochs BIOS.
>  ETEXI
>  
>  DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 07/16] qtest: Don't reset on qtest chardev connect

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> libqtest's qtest_init() connecting to the qtest socket triggers reset.
> This was coded in the hope we could use the same QEMU process for
> multiple tests that way.  Never used.  Injects an extra reset even
> when it's not used, and that can mess up tests such as the one of
> -boot once I'm about to add.  Drop it.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

We could always add a reset qtest command.  Probably makes more sense really.

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  qtest.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/qtest.c b/qtest.c
> index 07a9612..74f1842 100644
> --- a/qtest.c
> +++ b/qtest.c
> @@ -472,7 +472,12 @@ static void qtest_event(void *opaque, int event)
>  
>      switch (event) {
>      case CHR_EVENT_OPENED:
> -        qemu_system_reset(false);
> +        /*
> +         * We used to call qemu_system_reset() here, hoping we could
> +         * use the same process for multiple tests that way.  Never
> +         * used.  Injects an extra reset even when it's not used, and
> +         * that can mess up tests, e.g. -boot once.
> +         */
>          for (i = 0; i < ARRAY_SIZE(irq_levels); i++) {
>              irq_levels[i] = 0;
>          }
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 08/16] boot-order-test: New; covering just PC for now

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/Makefile          |  2 ++
>  tests/boot-order-test.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 70 insertions(+)
>  create mode 100644 tests/boot-order-test.c
>
> diff --git a/tests/Makefile b/tests/Makefile
> index c107489..394e029 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -54,6 +54,7 @@ gcov-files-i386-y = hw/fdc.c
>  check-qtest-i386-y += tests/ide-test$(EXESUF)
>  check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
>  gcov-files-i386-y += hw/hd-geometry.c
> +check-qtest-i386-y += tests/boot-order-test$(EXESUF)
>  check-qtest-i386-y += tests/rtc-test$(EXESUF)
>  check-qtest-i386-y += tests/i440fx-test$(EXESUF)
>  check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
> @@ -130,6 +131,7 @@ tests/m48t59-test$(EXESUF): tests/m48t59-test.o
>  tests/fdc-test$(EXESUF): tests/fdc-test.o
>  tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y)
>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
> +tests/boot-order-test$(EXESUF): tests/boot-order-test.o
>  tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
>  tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
>  tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
> new file mode 100644
> index 0000000..2215710
> --- /dev/null
> +++ b/tests/boot-order-test.c
> @@ -0,0 +1,68 @@
> +/*
> + * Boot order test cases.
> + *
> + * Copyright (c) 2013 Red Hat Inc.
> + *
> + * Authors:
> + *  Markus Armbruster <armbru@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 <glib.h>
> +#include "libqtest.h"
> +
> +static void test_pc_cmos_byte(int reg, int expected)
> +{
> +    int actual;
> +
> +    outb(0x70, reg);
> +    actual = inb(0x71);
> +    g_assert_cmphex(actual, ==, expected);
> +}
> +
> +static void test_pc_cmos(uint8_t boot1, uint8_t boot2)
> +{
> +    test_pc_cmos_byte(0x38, boot1);
> +    test_pc_cmos_byte(0x3d, boot2);
> +}
> +
> +static void test_pc_with_args(const char *test_args,
> +                              uint8_t boot1, uint8_t boot2,
> +                              uint8_t reboot1, uint8_t reboot2)
> +{
> +    char *args = g_strdup_printf("-nodefaults -display none %s", test_args);
> +
> +    qtest_start(args);
> +    test_pc_cmos(boot1, boot2);
> +    qmp("{ 'execute': 'system_reset' }");

I think this races.  I'd suggest doing a tight loop of this test and
running it a few thousand times to see if you can catch it.

qmp_system_reset() calls qemu_system_reset_requested() which stops all
CPUs but let's control fall back to the main loop which actually does
the device reset.

I think there's a tiny window where this command could return while the
reset routines have not been actually called yet.

Technically speaking, I think it's necessary to wait for a reset event
to know that the device model has been reset.

Regards,

Anthony Liguori

> +    test_pc_cmos(reboot1, reboot2);
> +    qtest_quit(global_qtest);
> +    g_free(args);
> +}
> +
> +static void test_pc_boot_order(void)
> +{
> +    test_pc_with_args("", 0x30, 0x12, 0x30, 0x12);
> +    test_pc_with_args("-no-fd-bootchk", 0x31, 0x12, 0x31, 0x12);
> +    test_pc_with_args("-boot c", 0, 0x02, 0, 0x02);
> +    test_pc_with_args("-boot nda", 0x10, 0x34, 0x10, 0x34);
> +    test_pc_with_args("-boot order=", 0, 0, 0, 0);
> +    test_pc_with_args("-boot order= -boot order=c", 0, 0x02, 0, 0x02);
> +    test_pc_with_args("-boot once=a", 0, 0x01, 0x30, 0x12);
> +    test_pc_with_args("-boot once=a -no-fd-bootchk", 0x01, 0x01, 0x31, 0x12);
> +    test_pc_with_args("-boot once=a,order=c", 0, 0x01, 0, 0x02);
> +    test_pc_with_args("-boot once=d -boot order=nda", 0, 0x03, 0x10, 0x34);
> +    test_pc_with_args("-boot once=a -boot once=b -boot once=c",
> +                      0, 0x02, 0x30, 0x12);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    qtest_add_func("boot-order/pc", test_pc_boot_order);
> +
> +    return g_test_run();
> +}
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 09/16] boot-order-test: Add tests for PowerMacs

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> From: Andreas Färber <afaerber@suse.de>
>
> They set the boot device via fw_cfg, which is then translated to a boot
> path of "hd" or "cd" in OpenBIOS.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/Makefile          |  2 ++
>  tests/boot-order-test.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 67 insertions(+), 1 deletion(-)
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 394e029..9653fce 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -67,6 +67,8 @@ gcov-files-sparc-y += hw/m48t59.c
>  gcov-files-sparc64-y += hw/m48t59.c
>  check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>  gcov-files-arm-y += hw/tmp105.c
> +check-qtest-ppc-y += tests/boot-order-test$(EXESUF)
> +check-qtest-ppc64-y += tests/boot-order-test$(EXESUF)
>  
>  GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
>  
> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
> index 2215710..c1cc2a6 100644
> --- a/tests/boot-order-test.c
> +++ b/tests/boot-order-test.c
> @@ -10,8 +10,10 @@
>   * See the COPYING file in the top-level directory.
>   */
>  
> +#include <string.h>
>  #include <glib.h>
>  #include "libqtest.h"
> +#include "qemu/bswap.h"
>  
>  static void test_pc_cmos_byte(int reg, int expected)
>  {
> @@ -58,11 +60,73 @@ static void test_pc_boot_order(void)
>                        0, 0x02, 0x30, 0x12);
>  }
>  
> +#define G3BEIGE_CFG_ADDR 0xf0000510
> +#define MAC99_CFG_ADDR   0xf0000510
> +
> +#define NO_QEMU_PROTOS
> +#include "hw/nvram/fw_cfg.h"
> +#undef NO_QEMU_PROTOS
> +
> +static void powermac_fw_cfg_read(bool newworld, uint16_t cmd,
> +                                 uint8_t *buf, unsigned int len)
> +{
> +    unsigned int i;
> +
> +    writew(newworld ? MAC99_CFG_ADDR : G3BEIGE_CFG_ADDR, cmd);
> +    for (i = 0; i < len; i++) {
> +        buf[i] = readb((newworld ? MAC99_CFG_ADDR : G3BEIGE_CFG_ADDR) + 2);
> +    }
> +}
> +
> +static uint16_t powermac_fw_cfg_read16(bool newworld, uint16_t cmd)
> +{
> +    uint16_t value;
> +
> +    powermac_fw_cfg_read(newworld, cmd, (uint8_t *)&value, sizeof(value));
> +    return le16_to_cpu(value);
> +}
> +
> +static void test_powermac_with_args(bool newworld, const char *extra_args,
> +                                    uint16_t expected_boot,
> +                                    uint16_t expected_reboot)
> +{
> +    char *args = g_strdup_printf("-nodefaults -display none -machine %s %s",
> +                                 newworld ? "mac99" : "g3beige", extra_args);
> +    uint16_t actual;
> +    qtest_start(args);
> +    actual = powermac_fw_cfg_read16(newworld, FW_CFG_BOOT_DEVICE);
> +    g_assert_cmphex(actual, ==, expected_boot);
> +    qmp("{ 'execute': 'system_reset' }");

Same concern here but otherwise looks good.

Regards,

Anthony Liguori

> +    actual = powermac_fw_cfg_read16(newworld, FW_CFG_BOOT_DEVICE);
> +    g_assert_cmphex(actual, ==, expected_reboot);
> +    qtest_quit(global_qtest);
> +    g_free(args);
> +}
> +
> +static void test_powermac_boot_order(void)
> +{
> +    int i;
> +
> +    for (i = 0; i < 2; i++) {
> +        bool newworld = (i == 1);
> +
> +        test_powermac_with_args(newworld, "", 'c', 'c');
> +        test_powermac_with_args(newworld, "-boot c", 'c', 'c');
> +        test_powermac_with_args(newworld, "-boot d", 'd', 'd');
> +    }
> +}
> +
>  int main(int argc, char *argv[])
>  {
> +    const char *arch = qtest_get_arch();
> +
>      g_test_init(&argc, &argv, NULL);
>  
> -    qtest_add_func("boot-order/pc", test_pc_boot_order);
> +    if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> +        qtest_add_func("boot-order/pc", test_pc_boot_order);
> +    } else if (strcmp(arch, "ppc") == 0 || strcmp(arch, "ppc64") == 0) {
> +        qtest_add_func("boot-order/powermac", test_powermac_boot_order);
> +    }
>  
>      return g_test_run();
>  }
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 10/16] boot-order-test: Cover -boot once in ppc tests

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Cc: Andreas Färber <afaerber@suse.de>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  tests/boot-order-test.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
> index c1cc2a6..f6dece6 100644
> --- a/tests/boot-order-test.c
> +++ b/tests/boot-order-test.c
> @@ -113,6 +113,7 @@ static void test_powermac_boot_order(void)
>          test_powermac_with_args(newworld, "", 'c', 'c');
>          test_powermac_with_args(newworld, "-boot c", 'c', 'c');
>          test_powermac_with_args(newworld, "-boot d", 'd', 'd');
> +        test_powermac_with_args(newworld, "-boot once=d,order=c", 'd', 'c');
>      }
>  }
>  
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 11/16] boot-order-test: Better separate target-specific and generic parts

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> The initial version did just PC.  I didn't bother to separate out
> generic parts, because I don't like to abstract from a single case.
>
> Now we have two cases, PC and PowerMac, and I'm about to add two more.
> Time to do it right.
>
> To ease review, this commit changes the code without in-place, and
> only the next commit reorders it for better readability.
>
> Cc: Andreas Färber <afaerber@suse.de>
> Cc: Alexander Graf <agraf@suse.de>
> Cc: qemu-ppc@nongnu.org
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/boot-order-test.c | 160 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 99 insertions(+), 61 deletions(-)
>
> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
> index f6dece6..23edacf 100644
> --- a/tests/boot-order-test.c
> +++ b/tests/boot-order-test.c
> @@ -15,106 +15,141 @@
>  #include "libqtest.h"
>  #include "qemu/bswap.h"
>  
> -static void test_pc_cmos_byte(int reg, int expected)
> +static uint8_t read_mc146818(uint16_t port, uint8_t reg)
>  {
> -    int actual;
> -
> -    outb(0x70, reg);
> -    actual = inb(0x71);
> -    g_assert_cmphex(actual, ==, expected);
> +    outb(port, reg);
> +    return inb(port + 1);
>  }

It's worth looking at the rtc-test case and pulling out
cmos_read/write() into libqos I suspect.

Regards,

Anthony Liguori

>  
> -static void test_pc_cmos(uint8_t boot1, uint8_t boot2)
> +static uint64_t read_boot_order_pc(void)
>  {
> -    test_pc_cmos_byte(0x38, boot1);
> -    test_pc_cmos_byte(0x3d, boot2);
> +    uint8_t b1 = read_mc146818(0x70, 0x38);
> +    uint8_t b2 = read_mc146818(0x70, 0x3d);
> +
> +    return b1 | (b2 << 8);
>  }
>  
> -static void test_pc_with_args(const char *test_args,
> -                              uint8_t boot1, uint8_t boot2,
> -                              uint8_t reboot1, uint8_t reboot2)
> +static void test_a_boot_order(const char *machine,
> +                              const char *test_args,
> +                              uint64_t (*read_boot_order)(void),
> +                              uint64_t expected_boot,
> +                              uint64_t expected_reboot)
>  {
> -    char *args = g_strdup_printf("-nodefaults -display none %s", test_args);
> +    char *args;
> +    uint64_t actual;
>  
> +    args = g_strdup_printf("-nodefaults -display none%s%s %s",
> +                           machine ? " -M " : "",
> +                           machine ?: "",
> +                           test_args);
>      qtest_start(args);
> -    test_pc_cmos(boot1, boot2);
> +    actual = read_boot_order();
> +    g_assert_cmphex(actual, ==, expected_boot);
>      qmp("{ 'execute': 'system_reset' }");
> -    test_pc_cmos(reboot1, reboot2);
> +    actual = read_boot_order();
> +    g_assert_cmphex(actual, ==, expected_reboot);
>      qtest_quit(global_qtest);
>      g_free(args);
>  }
>  
> +typedef struct {
> +    const char *args;
> +    uint64_t expected_boot;
> +    uint64_t expected_reboot;
> +} boot_order_test;
> +
> +static void test_boot_orders(const char *machine,
> +                             uint64_t (*read_boot_order)(void),
> +                             const boot_order_test *tests)
> +{
> +    int i;
> +
> +    for (i = 0; tests[i].args; i++) {
> +        test_a_boot_order(machine, tests[i].args,
> +                          read_boot_order,
> +                          tests[i].expected_boot,
> +                          tests[i].expected_reboot);
> +    }
> +}
> +
> +static const boot_order_test test_cases_pc[] = {
> +    { "",
> +      0x1230, 0x1230 },
> +    { "-no-fd-bootchk",
> +      0x1231, 0x1231 },
> +    { "-boot c",
> +      0x0200, 0x0200 },
> +    { "-boot nda",
> +      0x3410, 0x3410 },
> +    { "-boot order=",
> +      0, 0 },
> +    { "-boot order= -boot order=c",
> +      0x0200, 0x0200 },
> +    { "-boot once=a",
> +      0x0100, 0x1230 },
> +    { "-boot once=a -no-fd-bootchk",
> +      0x0101, 0x1231 },
> +    { "-boot once=a,order=c",
> +      0x0100, 0x0200 },
> +    { "-boot once=d -boot order=nda",
> +      0x0300, 0x3410 },
> +    { "-boot once=a -boot once=b -boot once=c",
> +      0x0200, 0x1230 },
> +    {}
> +};
> +
>  static void test_pc_boot_order(void)
>  {
> -    test_pc_with_args("", 0x30, 0x12, 0x30, 0x12);
> -    test_pc_with_args("-no-fd-bootchk", 0x31, 0x12, 0x31, 0x12);
> -    test_pc_with_args("-boot c", 0, 0x02, 0, 0x02);
> -    test_pc_with_args("-boot nda", 0x10, 0x34, 0x10, 0x34);
> -    test_pc_with_args("-boot order=", 0, 0, 0, 0);
> -    test_pc_with_args("-boot order= -boot order=c", 0, 0x02, 0, 0x02);
> -    test_pc_with_args("-boot once=a", 0, 0x01, 0x30, 0x12);
> -    test_pc_with_args("-boot once=a -no-fd-bootchk", 0x01, 0x01, 0x31, 0x12);
> -    test_pc_with_args("-boot once=a,order=c", 0, 0x01, 0, 0x02);
> -    test_pc_with_args("-boot once=d -boot order=nda", 0, 0x03, 0x10, 0x34);
> -    test_pc_with_args("-boot once=a -boot once=b -boot once=c",
> -                      0, 0x02, 0x30, 0x12);
> +    test_boot_orders(NULL, read_boot_order_pc, test_cases_pc);
>  }
>  
> -#define G3BEIGE_CFG_ADDR 0xf0000510
> -#define MAC99_CFG_ADDR   0xf0000510
> +#define PMAC_CFG_ADDR 0xf0000510
>  
>  #define NO_QEMU_PROTOS
>  #include "hw/nvram/fw_cfg.h"
>  #undef NO_QEMU_PROTOS
>  
> -static void powermac_fw_cfg_read(bool newworld, uint16_t cmd,
> -                                 uint8_t *buf, unsigned int len)
> +static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
> +                        void *buf, size_t len)
>  {
> -    unsigned int i;
> +    uint8_t *p = buf;
> +    size_t i;
>  
> -    writew(newworld ? MAC99_CFG_ADDR : G3BEIGE_CFG_ADDR, cmd);
> +    writew(cfg_addr, cmd);
>      for (i = 0; i < len; i++) {
> -        buf[i] = readb((newworld ? MAC99_CFG_ADDR : G3BEIGE_CFG_ADDR) + 2);
> +        p[i] = readb(cfg_addr + 2);
>      }
>  }
>  
> -static uint16_t powermac_fw_cfg_read16(bool newworld, uint16_t cmd)
> +static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
>  {
>      uint16_t value;
>  
> -    powermac_fw_cfg_read(newworld, cmd, (uint8_t *)&value, sizeof(value));
> +    read_fw_cfg(cfg_addr, cmd, &value, sizeof(value));
>      return le16_to_cpu(value);
>  }
>  
> -static void test_powermac_with_args(bool newworld, const char *extra_args,
> -                                    uint16_t expected_boot,
> -                                    uint16_t expected_reboot)
> +static uint64_t read_boot_order_pmac(void)
>  {
> -    char *args = g_strdup_printf("-nodefaults -display none -machine %s %s",
> -                                 newworld ? "mac99" : "g3beige", extra_args);
> -    uint16_t actual;
> -    qtest_start(args);
> -    actual = powermac_fw_cfg_read16(newworld, FW_CFG_BOOT_DEVICE);
> -    g_assert_cmphex(actual, ==, expected_boot);
> -    qmp("{ 'execute': 'system_reset' }");
> -    actual = powermac_fw_cfg_read16(newworld, FW_CFG_BOOT_DEVICE);
> -    g_assert_cmphex(actual, ==, expected_reboot);
> -    qtest_quit(global_qtest);
> -    g_free(args);
> +    return read_fw_cfg_i16(PMAC_CFG_ADDR, FW_CFG_BOOT_DEVICE);
>  }
>  
> -static void test_powermac_boot_order(void)
> -{
> -    int i;
> +static const boot_order_test test_cases_fw_cfg[] = {
> +    { "", 'c', 'c' },
> +    { "-boot c", 'c', 'c' },
> +    { "-boot d", 'd', 'd' },
> +    { "-boot once=d,order=c", 'd', 'c' },
> +    {}
> +};
>  
> -    for (i = 0; i < 2; i++) {
> -        bool newworld = (i == 1);
> +static void test_pmac_oldworld_boot_order(void)
> +{
> +    test_boot_orders("g3beige", read_boot_order_pmac, test_cases_fw_cfg);
> +}
>  
> -        test_powermac_with_args(newworld, "", 'c', 'c');
> -        test_powermac_with_args(newworld, "-boot c", 'c', 'c');
> -        test_powermac_with_args(newworld, "-boot d", 'd', 'd');
> -        test_powermac_with_args(newworld, "-boot once=d,order=c", 'd', 'c');
> -    }
> +static void test_pmac_newworld_boot_order(void)
> +{
> +    test_boot_orders("mac99", read_boot_order_pmac, test_cases_fw_cfg);
>  }
>  
>  int main(int argc, char *argv[])
> @@ -126,7 +161,10 @@ int main(int argc, char *argv[])
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
>          qtest_add_func("boot-order/pc", test_pc_boot_order);
>      } else if (strcmp(arch, "ppc") == 0 || strcmp(arch, "ppc64") == 0) {
> -        qtest_add_func("boot-order/powermac", test_powermac_boot_order);
> +        qtest_add_func("boot-order/pmac_oldworld",
> +                       test_pmac_oldworld_boot_order);
> +        qtest_add_func("boot-order/pmac_newworld",
> +                       test_pmac_newworld_boot_order);
>      }
>  
>      return g_test_run();
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 15/16] boot-order-test: Support fw_cfg in I/O space

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Next commit needs it.
>
> Cc: Blue Swirl <blauwirbel@gmail.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  tests/boot-order-test.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
> index 7b1edc1..d1d99f8 100644
> --- a/tests/boot-order-test.c
> +++ b/tests/boot-order-test.c
> @@ -133,23 +133,31 @@ static void test_prep_boot_order(void)
>      test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
>  }
>  
> -static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
> +static void read_fw_cfg(uint64_t cfg_addr, bool addr_is_io, uint16_t cmd,
>                          void *buf, size_t len)

I missed it earlier, but you can use libqos/fw_cfg.h for this.

Regards,

Anthony Liguori

>  {
>      uint8_t *p = buf;
>      size_t i;
>  
> -    writew(cfg_addr, cmd);
> -    for (i = 0; i < len; i++) {
> -        p[i] = readb(cfg_addr + 2);
> +    if (addr_is_io) {
> +        outw(cfg_addr, cmd);
> +        for (i = 0; i < len; i++) {
> +            p[i] = inb(cfg_addr + 1);
> +        }
> +    } else {
> +        writew(cfg_addr, cmd);
> +        for (i = 0; i < len; i++) {
> +            p[i] = readb(cfg_addr + 2);
> +        }
>      }
>  }
>  
> -static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
> +static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, bool addr_is_io,
> +                                    uint16_t cmd)
>  {
>      uint16_t value;
>  
> -    read_fw_cfg(cfg_addr, cmd, &value, sizeof(value));
> +    read_fw_cfg(cfg_addr, addr_is_io, cmd, &value, sizeof(value));
>      return le16_to_cpu(value);
>  }
>  
> @@ -157,7 +165,7 @@ static uint16_t read_fw_cfg_i16(uint64_t cfg_addr, uint16_t cmd)
>  
>  static uint64_t read_boot_order_pmac(void)
>  {
> -    return read_fw_cfg_i16(PMAC_CFG_ADDR, FW_CFG_BOOT_DEVICE);
> +    return read_fw_cfg_i16(PMAC_CFG_ADDR, false, FW_CFG_BOOT_DEVICE);
>  }
>  
>  static const boot_order_test test_cases_fw_cfg[] = {
> @@ -182,7 +190,7 @@ static void test_pmac_newworld_boot_order(void)
>  
>  static uint64_t read_boot_order_sun4m(void)
>  {
> -    return read_fw_cfg_i16(SUN4M_CFG_ADDR, FW_CFG_BOOT_DEVICE);
> +    return read_fw_cfg_i16(SUN4M_CFG_ADDR, false, FW_CFG_BOOT_DEVICE);
>  }
>  
>  static void test_sun4m_boot_order(void)
> -- 
> 1.7.11.7

Re: [Qemu-devel] [PATCH v3 15/16] boot-order-test: Support fw_cfg in I/O space

[Andreas Färber]

Am 14.06.2013 15:53, schrieb Anthony Liguori:
> Markus Armbruster <armbru@redhat.com> writes:
> 
>> Next commit needs it.
>>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/boot-order-test.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>> index 7b1edc1..d1d99f8 100644
>> --- a/tests/boot-order-test.c
>> +++ b/tests/boot-order-test.c
>> @@ -133,23 +133,31 @@ static void test_prep_boot_order(void)
>>      test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
>>  }
>>  
>> -static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
>> +static void read_fw_cfg(uint64_t cfg_addr, bool addr_is_io, uint16_t cmd,
>>                          void *buf, size_t len)
> 
> I missed it earlier, but you can use libqos/fw_cfg.h for this.

Agree, but that didn't exist at the time. :-)

Regards,
Andreas

-- 
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 v3 05/16] pc: Make -no-fd-bootchk stick across boot order changes

[Markus Armbruster]

Anthony Liguori <aliguori@us.ibm.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Option -no-fd-bootchk asks the BIOS to attempt booting from a floppy
>> even when the boot sector signature isn't there, by setting a bit in
>> RTC CMOS.  It was added back in 2006 (commit 52ca8d6a).
>>
>> Two years later, commit 0ecdffbb added monitor command boot_set.
>> Implemented by new function pc_boot_set().  It unconditionally clears
>> the floppy signature bit in CMOS.
>>
>> Commit e0f084bf added -boot option once to automatically change the
>> boot order on first reset.  Reuses pc_boot_set(), thus also clears the
>> floppy signature bit.  Commit d9346e81 took care to preserve this
>> behavior.
>
> Quite a history there :-)
>
> Does anyone still use no-fd-bootchk?

No idea.

>                                       Do you know what the original
> use-case was?

Its commit message is of no help.  Best we got is the option
documentation:

    Disable boot signature checking for floppy disks in Bochs BIOS.  It
    may be needed to boot from old floppy disks.

As far as I can tell, SeaBIOS implements this, too.

>> Thus, -no-fd-bootchk applies to any number of boots.  Except it
>> applies just to the first boot with -boot once, and never after
>> boot_set.  Weird.  Make it stick instead: set the bit according to
>> -no-fd-bootchk in pc_boot_set().
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Thanks!

Re: [Qemu-devel] [PATCH v3 07/16] qtest: Don't reset on qtest chardev connect

[Markus Armbruster]

Anthony Liguori <aliguori@us.ibm.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> libqtest's qtest_init() connecting to the qtest socket triggers reset.
>> This was coded in the hope we could use the same QEMU process for
>> multiple tests that way.  Never used.  Injects an extra reset even
>> when it's not used, and that can mess up tests such as the one of
>> -boot once I'm about to add.  Drop it.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> We could always add a reset qtest command.  Probably makes more sense really.

Agree.  But since I don't need it at this time...

> Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Thanks!

Re: [Qemu-devel] [PATCH v3 08/16] boot-order-test: New; covering just PC for now

[Markus Armbruster]

Anthony Liguori <aliguori@us.ibm.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/Makefile          |  2 ++
>>  tests/boot-order-test.c | 68
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 70 insertions(+)
>>  create mode 100644 tests/boot-order-test.c
>>
>> diff --git a/tests/Makefile b/tests/Makefile
>> index c107489..394e029 100644
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -54,6 +54,7 @@ gcov-files-i386-y = hw/fdc.c
>>  check-qtest-i386-y += tests/ide-test$(EXESUF)
>>  check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
>>  gcov-files-i386-y += hw/hd-geometry.c
>> +check-qtest-i386-y += tests/boot-order-test$(EXESUF)
>>  check-qtest-i386-y += tests/rtc-test$(EXESUF)
>>  check-qtest-i386-y += tests/i440fx-test$(EXESUF)
>>  check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
>> @@ -130,6 +131,7 @@ tests/m48t59-test$(EXESUF): tests/m48t59-test.o
>>  tests/fdc-test$(EXESUF): tests/fdc-test.o
>>  tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y)
>>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
>> +tests/boot-order-test$(EXESUF): tests/boot-order-test.o
>>  tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
>>  tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
>>  tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>> new file mode 100644
>> index 0000000..2215710
>> --- /dev/null
>> +++ b/tests/boot-order-test.c
>> @@ -0,0 +1,68 @@
>> +/*
>> + * Boot order test cases.
>> + *
>> + * Copyright (c) 2013 Red Hat Inc.
>> + *
>> + * Authors:
>> + *  Markus Armbruster <armbru@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 <glib.h>
>> +#include "libqtest.h"
>> +
>> +static void test_pc_cmos_byte(int reg, int expected)
>> +{
>> +    int actual;
>> +
>> +    outb(0x70, reg);
>> +    actual = inb(0x71);
>> +    g_assert_cmphex(actual, ==, expected);
>> +}
>> +
>> +static void test_pc_cmos(uint8_t boot1, uint8_t boot2)
>> +{
>> +    test_pc_cmos_byte(0x38, boot1);
>> +    test_pc_cmos_byte(0x3d, boot2);
>> +}
>> +
>> +static void test_pc_with_args(const char *test_args,
>> +                              uint8_t boot1, uint8_t boot2,
>> +                              uint8_t reboot1, uint8_t reboot2)
>> +{
>> +    char *args = g_strdup_printf("-nodefaults -display none %s", test_args);
>> +
>> +    qtest_start(args);
>> +    test_pc_cmos(boot1, boot2);
>> +    qmp("{ 'execute': 'system_reset' }");
        test_pc_cmos(reboot1, reboot2);
>
> I think this races.  I'd suggest doing a tight loop of this test and
> running it a few thousand times to see if you can catch it.
>
> qmp_system_reset() calls qemu_system_reset_requested() which stops all
> CPUs but let's control fall back to the main loop which actually does
> the device reset.
>
> I think there's a tiny window where this command could return while the
> reset routines have not been actually called yet.
>
> Technically speaking, I think it's necessary to wait for a reset event
> to know that the device model has been reset.

Hmm.

First attempt to "win" this race: tight loop around test_a_boot_order(),
i.e. the complete test.  Failed because libqtest leaks two file
descriptors and some memory per iteration.  With that fixed (patch
coming), I still couldn't make the test fail in >75,000 runs on two
otherwise pretty much unloaded cores.

Second attempt: tight loop around just

    qmp("{ 'execute': 'system_reset' }");
    actual = read_boot_order();
    g_assert_cmphex(actual, ==, expected_reboot);

Still no luck with x86, but "success" with ppc.

Waiting for the event RESET is safe.  But doing that right involves
quite some infrastructure work.  All we have now is qtest_qmpv(), which
sends the command, then reads QMP output character by character until it
got a complete object.  Normally, that's the QMP command response.  But
it could be an event.  Racy all by itself, even without my "help" :)

Oh, and it doesn't know about strings, it just counts curlies.  If the
output has unmatched curlies in strings...  I wonder how this code ever
made it past review ;-P

The proper solution is real QMP support in libqtest.  Unfortunately,
that's not something I can do right now.

fdc-test.c uses qmp("") to ignore an expected event.  If I put a similar
hack into boot-order-test.c, ppc survives >500,000 iterations.  Good
enough to get this test in?

[...]

Re: [Qemu-devel] [PATCH v3 08/16] boot-order-test: New; covering just PC for now

[Andreas Färber]

Am 18.06.2013 15:33, schrieb Markus Armbruster:
> Anthony Liguori <aliguori@us.ibm.com> writes:
> 
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  tests/Makefile          |  2 ++
>>>  tests/boot-order-test.c | 68
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 70 insertions(+)
>>>  create mode 100644 tests/boot-order-test.c
>>>
>>> diff --git a/tests/Makefile b/tests/Makefile
>>> index c107489..394e029 100644
>>> --- a/tests/Makefile
>>> +++ b/tests/Makefile
>>> @@ -54,6 +54,7 @@ gcov-files-i386-y = hw/fdc.c
>>>  check-qtest-i386-y += tests/ide-test$(EXESUF)
>>>  check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
>>>  gcov-files-i386-y += hw/hd-geometry.c
>>> +check-qtest-i386-y += tests/boot-order-test$(EXESUF)
>>>  check-qtest-i386-y += tests/rtc-test$(EXESUF)
>>>  check-qtest-i386-y += tests/i440fx-test$(EXESUF)
>>>  check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
>>> @@ -130,6 +131,7 @@ tests/m48t59-test$(EXESUF): tests/m48t59-test.o
>>>  tests/fdc-test$(EXESUF): tests/fdc-test.o
>>>  tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y)
>>>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
>>> +tests/boot-order-test$(EXESUF): tests/boot-order-test.o
>>>  tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
>>>  tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
>>>  tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
>>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>>> new file mode 100644
>>> index 0000000..2215710
>>> --- /dev/null
>>> +++ b/tests/boot-order-test.c
>>> @@ -0,0 +1,68 @@
>>> +/*
>>> + * Boot order test cases.
>>> + *
>>> + * Copyright (c) 2013 Red Hat Inc.
>>> + *
>>> + * Authors:
>>> + *  Markus Armbruster <armbru@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 <glib.h>
>>> +#include "libqtest.h"
>>> +
>>> +static void test_pc_cmos_byte(int reg, int expected)
>>> +{
>>> +    int actual;
>>> +
>>> +    outb(0x70, reg);
>>> +    actual = inb(0x71);
>>> +    g_assert_cmphex(actual, ==, expected);
>>> +}
>>> +
>>> +static void test_pc_cmos(uint8_t boot1, uint8_t boot2)
>>> +{
>>> +    test_pc_cmos_byte(0x38, boot1);
>>> +    test_pc_cmos_byte(0x3d, boot2);
>>> +}
>>> +
>>> +static void test_pc_with_args(const char *test_args,
>>> +                              uint8_t boot1, uint8_t boot2,
>>> +                              uint8_t reboot1, uint8_t reboot2)
>>> +{
>>> +    char *args = g_strdup_printf("-nodefaults -display none %s", test_args);
>>> +
>>> +    qtest_start(args);
>>> +    test_pc_cmos(boot1, boot2);
>>> +    qmp("{ 'execute': 'system_reset' }");
>         test_pc_cmos(reboot1, reboot2);
>>
>> I think this races.  I'd suggest doing a tight loop of this test and
>> running it a few thousand times to see if you can catch it.
>>
>> qmp_system_reset() calls qemu_system_reset_requested() which stops all
>> CPUs but let's control fall back to the main loop which actually does
>> the device reset.
>>
>> I think there's a tiny window where this command could return while the
>> reset routines have not been actually called yet.
>>
>> Technically speaking, I think it's necessary to wait for a reset event
>> to know that the device model has been reset.
> 
> Hmm.
> 
> First attempt to "win" this race: tight loop around test_a_boot_order(),
> i.e. the complete test.  Failed because libqtest leaks two file
> descriptors and some memory per iteration.  With that fixed (patch
> coming), I still couldn't make the test fail in >75,000 runs on two
> otherwise pretty much unloaded cores.
> 
> Second attempt: tight loop around just
> 
>     qmp("{ 'execute': 'system_reset' }");
>     actual = read_boot_order();
>     g_assert_cmphex(actual, ==, expected_reboot);
> 
> Still no luck with x86, but "success" with ppc.
> 
> Waiting for the event RESET is safe.  But doing that right involves
> quite some infrastructure work.  All we have now is qtest_qmpv(), which
> sends the command, then reads QMP output character by character until it
> got a complete object.  Normally, that's the QMP command response.  But
> it could be an event.  Racy all by itself, even without my "help" :)
> 
> Oh, and it doesn't know about strings, it just counts curlies.  If the
> output has unmatched curlies in strings...  I wonder how this code ever
> made it past review ;-P
> 
> The proper solution is real QMP support in libqtest.  Unfortunately,
> that's not something I can do right now.

Didn't Michael or Jason have patches improving the qtest QMP support for
migration some months ago? At least they allowed to access the QMP
response in parsed form IIRC.

Andreas

> 
> fdc-test.c uses qmp("") to ignore an expected event.  If I put a similar
> hack into boot-order-test.c, ppc survives >500,000 iterations.  Good
> enough to get this test in?
> 
> [...]
> 


-- 
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 v3 08/16] boot-order-test: New; covering just PC for now

[Anthony Liguori]

Markus Armbruster <armbru@redhat.com> writes:

> Anthony Liguori <aliguori@us.ibm.com> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  tests/Makefile          |  2 ++
>>>  tests/boot-order-test.c | 68
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 70 insertions(+)
>>>  create mode 100644 tests/boot-order-test.c
>>>
>>> diff --git a/tests/Makefile b/tests/Makefile
>>> index c107489..394e029 100644
>>> --- a/tests/Makefile
>>> +++ b/tests/Makefile
>>> @@ -54,6 +54,7 @@ gcov-files-i386-y = hw/fdc.c
>>>  check-qtest-i386-y += tests/ide-test$(EXESUF)
>>>  check-qtest-i386-y += tests/hd-geo-test$(EXESUF)
>>>  gcov-files-i386-y += hw/hd-geometry.c
>>> +check-qtest-i386-y += tests/boot-order-test$(EXESUF)
>>>  check-qtest-i386-y += tests/rtc-test$(EXESUF)
>>>  check-qtest-i386-y += tests/i440fx-test$(EXESUF)
>>>  check-qtest-i386-y += tests/fw_cfg-test$(EXESUF)
>>> @@ -130,6 +131,7 @@ tests/m48t59-test$(EXESUF): tests/m48t59-test.o
>>>  tests/fdc-test$(EXESUF): tests/fdc-test.o
>>>  tests/ide-test$(EXESUF): tests/ide-test.o $(libqos-pc-obj-y)
>>>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o
>>> +tests/boot-order-test$(EXESUF): tests/boot-order-test.o
>>>  tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(libqos-omap-obj-y)
>>>  tests/i440fx-test$(EXESUF): tests/i440fx-test.o $(libqos-pc-obj-y)
>>>  tests/fw_cfg-test$(EXESUF): tests/fw_cfg-test.o $(libqos-pc-obj-y)
>>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>>> new file mode 100644
>>> index 0000000..2215710
>>> --- /dev/null
>>> +++ b/tests/boot-order-test.c
>>> @@ -0,0 +1,68 @@
>>> +/*
>>> + * Boot order test cases.
>>> + *
>>> + * Copyright (c) 2013 Red Hat Inc.
>>> + *
>>> + * Authors:
>>> + *  Markus Armbruster <armbru@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 <glib.h>
>>> +#include "libqtest.h"
>>> +
>>> +static void test_pc_cmos_byte(int reg, int expected)
>>> +{
>>> +    int actual;
>>> +
>>> +    outb(0x70, reg);
>>> +    actual = inb(0x71);
>>> +    g_assert_cmphex(actual, ==, expected);
>>> +}
>>> +
>>> +static void test_pc_cmos(uint8_t boot1, uint8_t boot2)
>>> +{
>>> +    test_pc_cmos_byte(0x38, boot1);
>>> +    test_pc_cmos_byte(0x3d, boot2);
>>> +}
>>> +
>>> +static void test_pc_with_args(const char *test_args,
>>> +                              uint8_t boot1, uint8_t boot2,
>>> +                              uint8_t reboot1, uint8_t reboot2)
>>> +{
>>> +    char *args = g_strdup_printf("-nodefaults -display none %s", test_args);
>>> +
>>> +    qtest_start(args);
>>> +    test_pc_cmos(boot1, boot2);
>>> +    qmp("{ 'execute': 'system_reset' }");
>         test_pc_cmos(reboot1, reboot2);
>>
>> I think this races.  I'd suggest doing a tight loop of this test and
>> running it a few thousand times to see if you can catch it.
>>
>> qmp_system_reset() calls qemu_system_reset_requested() which stops all
>> CPUs but let's control fall back to the main loop which actually does
>> the device reset.
>>
>> I think there's a tiny window where this command could return while the
>> reset routines have not been actually called yet.
>>
>> Technically speaking, I think it's necessary to wait for a reset event
>> to know that the device model has been reset.
>
> Hmm.
>
> First attempt to "win" this race: tight loop around test_a_boot_order(),
> i.e. the complete test.  Failed because libqtest leaks two file
> descriptors and some memory per iteration.  With that fixed (patch
> coming), I still couldn't make the test fail in >75,000 runs on two
> otherwise pretty much unloaded cores.
>
> Second attempt: tight loop around just
>
>     qmp("{ 'execute': 'system_reset' }");
>     actual = read_boot_order();
>     g_assert_cmphex(actual, ==, expected_reboot);
>
> Still no luck with x86, but "success" with ppc.
>
> Waiting for the event RESET is safe.  But doing that right involves
> quite some infrastructure work.  All we have now is qtest_qmpv(), which
> sends the command, then reads QMP output character by character until it
> got a complete object.  Normally, that's the QMP command response.  But
> it could be an event.  Racy all by itself, even without my "help" :)
>
> Oh, and it doesn't know about strings, it just counts curlies.  If the
> output has unmatched curlies in strings...  I wonder how this code ever
> made it past review ;-P
>
> The proper solution is real QMP support in libqtest.  Unfortunately,
> that's not something I can do right now.
>
> fdc-test.c uses qmp("") to ignore an expected event.  If I put a similar
> hack into boot-order-test.c, ppc survives >500,000 iterations.  Good
> enough to get this test in?

With a very big comment stating what's happening.

Regards,

Anthony Liguori

>
> [...]

Re: [Qemu-devel] [PATCH v3 15/16] boot-order-test: Support fw_cfg in I/O space

[Markus Armbruster]

Anthony Liguori <aliguori@us.ibm.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Next commit needs it.
>>
>> Cc: Blue Swirl <blauwirbel@gmail.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  tests/boot-order-test.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>> index 7b1edc1..d1d99f8 100644
>> --- a/tests/boot-order-test.c
>> +++ b/tests/boot-order-test.c
>> @@ -133,23 +133,31 @@ static void test_prep_boot_order(void)
>>      test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
>>  }
>>  
>> -static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
>> +static void read_fw_cfg(uint64_t cfg_addr, bool addr_is_io, uint16_t cmd,
>>                          void *buf, size_t len)
>
> I missed it earlier, but you can use libqos/fw_cfg.h for this.

Missed on rebase, thanks for pointing it out.

Two options:

(1) You commit this minor code duplication now, and I promise to clean
    it up in a follow-up series.

(2) You tell me to rework this series so it uses libqos/fw_cfg.h from
    the start.

I'd prefer (1).

Re: [Qemu-devel] [PATCH v3 15/16] boot-order-test: Support fw_cfg in I/O space

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Anthony Liguori <aliguori@us.ibm.com> writes:
>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>>> Next commit needs it.
>>>
>>> Cc: Blue Swirl <blauwirbel@gmail.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  tests/boot-order-test.c | 24 ++++++++++++++++--------
>>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/tests/boot-order-test.c b/tests/boot-order-test.c
>>> index 7b1edc1..d1d99f8 100644
>>> --- a/tests/boot-order-test.c
>>> +++ b/tests/boot-order-test.c
>>> @@ -133,23 +133,31 @@ static void test_prep_boot_order(void)
>>>      test_boot_orders("prep", read_boot_order_prep, test_cases_prep);
>>>  }
>>>  
>>> -static void read_fw_cfg(uint64_t cfg_addr, uint16_t cmd,
>>> +static void read_fw_cfg(uint64_t cfg_addr, bool addr_is_io, uint16_t cmd,
>>>                          void *buf, size_t len)
>>
>> I missed it earlier, but you can use libqos/fw_cfg.h for this.
>
> Missed on rebase, thanks for pointing it out.
>
> Two options:
>
> (1) You commit this minor code duplication now, and I promise to clean
>     it up in a follow-up series.
>
> (2) You tell me to rework this series so it uses libqos/fw_cfg.h from
>     the start.
>
> I'd prefer (1).

(3) Commit PATCH 01-06/16 now (which passed your review), bounce the
rest back to me.

Still prefer (1), though :)

Re: [Qemu-devel] [PATCH v3 00/16] -boot and -no-fd-bootchk fixes

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH v3 05/16] pc: Make -no-fd-bootchk stick across boot order changes

[Kevin O'Connor]

On Tue, Jun 18, 2013 at 01:39:25PM +0200, Markus Armbruster wrote:
> Anthony Liguori <aliguori@us.ibm.com> writes:
> > Markus Armbruster <armbru@redhat.com> writes:
> >
> >> Option -no-fd-bootchk asks the BIOS to attempt booting from a floppy
> >> even when the boot sector signature isn't there, by setting a bit in
> >> RTC CMOS.  It was added back in 2006 (commit 52ca8d6a).
> >>
> >> Two years later, commit 0ecdffbb added monitor command boot_set.
> >> Implemented by new function pc_boot_set().  It unconditionally clears
> >> the floppy signature bit in CMOS.
> >>
> >> Commit e0f084bf added -boot option once to automatically change the
> >> boot order on first reset.  Reuses pc_boot_set(), thus also clears the
> >> floppy signature bit.  Commit d9346e81 took care to preserve this
> >> behavior.
> >
> > Quite a history there :-)
> >
> > Does anyone still use no-fd-bootchk?
> 
> No idea.

I've used it to test really old floppies.

> > use-case was?
> 
> Its commit message is of no help.  Best we got is the option
> documentation:
> 
>     Disable boot signature checking for floppy disks in Bochs BIOS.  It
>     may be needed to boot from old floppy disks.
> 
> As far as I can tell, SeaBIOS implements this, too.

The SeaBIOS and Bochs code check that there is a FAT partition
signature (0xaa55) on the first sector of the floppy drive.  This way,
if the disk doesn't look like it is valid, we wont try to execute the
first sector if it is just random junk.  However, really old floppies
didn't put this signature in the first sector and were still bootable.
So, no-fd-bootchk could be used to disable this check and still boot
really old floppies.

-Kevin