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

[Anthony Liguori]

Markus Armbruster <address@hidden> 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 <address@hidden>

Reviewed-by: Anthony Liguori <address@hidden>

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