Markus Armbruster <arm...@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 <arm...@redhat.com>
Reviewed-by: Anthony Liguori <aligu...@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
