"Michael S. Tsirkin" <m...@redhat.com> writes:
> On Wed, Aug 21, 2013 at 05:10:56PM +0200, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <m...@redhat.com> 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 <arm...@redhat.com>
>> >> ---
>> [...]
>> >> 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.
> 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.
