Re: [Qemu-devel] [PATCH v3 6/6] hw: Clean up bogus default boot order

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> 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 <address@hidden>
>> ---
[...]
>> 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 :)

> 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.

[...]