From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58140) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VCCOx-0001oC-KU for qemu-devel@nongnu.org; Wed, 21 Aug 2013 13:41:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VCCOr-0002U7-ID for qemu-devel@nongnu.org; Wed, 21 Aug 2013 13:40:59 -0400 Date: Wed, 21 Aug 2013 20:42:35 +0300 From: "Michael S. Tsirkin" Message-ID: <20130821174235.GE12410@redhat.com> References: <1374493127-22930-1-git-send-email-armbru@redhat.com> <1374493127-22930-7-git-send-email-armbru@redhat.com> <20130821145143.GA10839@redhat.com> <87zjsbjczj.fsf@blackfin.pond.sub.org> <20130821152334.GA10984@redhat.com> <87k3jff2xi.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87k3jff2xi.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v3 6/6] hw: Clean up bogus default boot order List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: aliguori@us.ibm.com, mjt@tls.msk.ru, qemu-devel@nongnu.org, agraf@suse.de, blauwirbel@gmail.com, qemu-ppc@nongnu.org, aviksil@linux.vnet.ibm.com On Wed, Aug 21, 2013 at 06:01:45PM +0200, Markus Armbruster wrote: > "Michael S. Tsirkin" writes: > > > On Wed, Aug 21, 2013 at 05:10:56PM +0200, Markus Armbruster wrote: > >> "Michael S. Tsirkin" writes: > >> > >> > On Mon, Jul 22, 2013 at 01:38:47PM +0200, Markus Armbruster wrote: > >> >> We set default boot order "cad" in every single machine definition > >> >> except "pseries" and "moxiesim", even though very few boards actually > >> >> care for boot order, and "cad" makes sense for even fewer. > >> >> > >> >> Machines that care: > >> >> > >> >> * pc and its variants > >> >> > >> >> Accept up to three letters 'a', 'b' (undocumented alias for 'a'), > >> >> 'c', 'd' and 'n'. Reject all others (fatal with -boot). > >> >> > >> >> * nseries (n800, n810) > >> >> > >> >> Check whether order starts with 'n'. Silently ignored otherwise. > >> >> > >> >> * prep, g3beige, mac99 > >> >> > >> >> Extract the first character the machine understands (subset of > >> >> 'a'..'f'). Silently ignored otherwise. > >> >> > >> >> * spapr > >> >> > >> >> Accept an arbitrary string (vl.c restricts it to contain only > >> >> 'a'..'p', no duplicates). > >> >> > >> >> * sun4[mdc] > >> >> > >> >> Use the first character. Silently ignored otherwise. > >> >> > >> >> Strip characters these machines ignore from their default boot order. > >> >> > >> >> For all other machines, remove the unused default boot order > >> >> alltogether. > >> >> > >> >> Note that my rename of QEMUMachine member boot_order to > >> >> default_boot_order and QEMUMachineInitArgs member boot_device to > >> >> boot_order has a welcome side effect: it makes every use of boot > >> >> orders visible in this patch, for easy review. > >> >> > >> >> Signed-off-by: Markus Armbruster > >> >> --- > >> [...] > >> >> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > >> >> index 9327ac1..3700bd5 100644 > >> >> --- a/hw/i386/pc_piix.c > >> >> +++ b/hw/i386/pc_piix.c > >> >> @@ -216,7 +216,7 @@ static void pc_init1(QEMUMachineInitArgs *args, > >> >> } > >> >> } > >> >> > >> >> - pc_cmos_init(below_4g_mem_size, above_4g_mem_size, args->boot_device, > >> >> + pc_cmos_init(below_4g_mem_size, above_4g_mem_size, args->boot_order, > >> >> floppy, idebus[0], idebus[1], rtc_state); > >> >> > >> >> if (pci_enabled && usb_enabled(false)) { > >> >> @@ -324,7 +324,7 @@ static QEMUMachine pc_i440fx_machine_v1_6 = { > >> >> .hot_add_cpu = pc_hot_add_cpu, > >> >> .max_cpus = 255, > >> >> .is_default = 1, > >> >> - DEFAULT_MACHINE_OPTIONS, > >> >> + .default_boot_order = "cad", > >> >> }; > >> >> > >> >> static QEMUMachine pc_i440fx_machine_v1_5 = { > >> >> @@ -337,7 +337,7 @@ static QEMUMachine pc_i440fx_machine_v1_5 = { > >> >> PC_COMPAT_1_5, > >> >> { /* end of list */ } > >> >> }, > >> >> - DEFAULT_MACHINE_OPTIONS, > >> >> + .default_boot_order = "cad", > >> >> }; > >> >> > >> >> static QEMUMachine pc_i440fx_machine_v1_4 = { > >> > > >> > So all PC machine types share this? > >> > >> Correct, just like they share DEFAULT_MACHINE_OPTIONS before my patch. > >> Which is defined as > >> > >> #define DEFAULT_MACHINE_OPTIONS \ > >> .boot_order = "cad" > >> > >> I.e. my patch merely peels off a layer of obfuscation :) > > > > Using a macro in multiple places, instead of a hard-coded constant is > > not obfuscation. > > > >> > Can we set this in some common code, somehow? > >> > >> We don't have an inheritance notion for machine types. > >> > >> vl.c uses machine->boot_order before calling one of its methods, so > >> monkey-patching .boot_order from a method won't do. Besides, that cure > >> looks much worse than the disease to me. > >> > >> Can't think of anything else offhand. > >> > >> [...] > > > > Set this in pc_init_pci somehow? > > Too late, see "vl.c uses..." above. Ah, missed it. Can we fix that? > > Set DEFAULT_MACHINE_OPTIONS locally in this file? > > I can do #define CAD "cad" for you ;) > > Seriously, I'd be okay with ".bootorder = DEFAULT_PC_BOOT_ORDER", with > #define DEFAULT_PC_BOOT_ORDER either locally, or in > include/hw/i386/pc.h. > > Hiding the complete initialization in a macro would be bad style, in my > opinion.