From patchwork Wed Feb 20 16:55:35 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Markus Armbruster X-Patchwork-Id: 222134 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 94FE82C007B for ; Thu, 21 Feb 2013 05:31:43 +1100 (EST) Received: from localhost ([::1]:49308 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U8CyD-0004fp-JU for incoming@patchwork.ozlabs.org; Wed, 20 Feb 2013 11:56:37 -0500 Received: from eggs.gnu.org ([208.118.235.92]:53076) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U8CxU-0002qq-5T for qemu-devel@nongnu.org; Wed, 20 Feb 2013 11:55:57 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1U8CxK-0005CK-VJ for qemu-devel@nongnu.org; Wed, 20 Feb 2013 11:55:52 -0500 Received: from oxygen.pond.sub.org ([2a01:4f8:121:10e4::3]:45968) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1U8CxK-0005AD-Cp for qemu-devel@nongnu.org; Wed, 20 Feb 2013 11:55:42 -0500 Received: from blackfin.pond.sub.org (p5B32BA07.dip.t-dialin.net [91.50.186.7]) by oxygen.pond.sub.org (Postfix) with ESMTPA id F0B759FE6C; Wed, 20 Feb 2013 17:55:38 +0100 (CET) Received: by blackfin.pond.sub.org (Postfix, from userid 1000) id D0A7F200B1; Wed, 20 Feb 2013 17:55:37 +0100 (CET) From: Markus Armbruster To: qemu-devel@nongnu.org Date: Wed, 20 Feb 2013 17:55:35 +0100 Message-Id: <1361379337-15044-4-git-send-email-armbru@redhat.com> X-Mailer: git-send-email 1.7.11.7 In-Reply-To: <1361379337-15044-1-git-send-email-armbru@redhat.com> References: <1361379337-15044-1-git-send-email-armbru@redhat.com> X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a01:4f8:121:10e4::3 Cc: jan.kiszka@siemens.com, aliguori@us.ibm.com, alex.williamson@redhat.com, aviksil@linux.vnet.ibm.com Subject: [Qemu-devel] [PATCH 3/5] vl: Fix -boot order and once regressions, and related bugs X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org 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 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 has was broken some time before commit 37905d6a, and fixed only for a single occurence of "once" there. * 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 --- vl.c | 59 ++++++++++++++++++++++++++++++----------------------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/vl.c b/vl.c index 4331aef..33e4116 100644 --- a/vl.c +++ b/vl.c @@ -2802,7 +2802,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; @@ -3088,31 +3088,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: @@ -4009,6 +3987,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 = ""; } @@ -4169,9 +4172,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,