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

[Michael S. Tsirkin]

On Wed, Aug 21, 2013 at 06:01:45PM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <address@hidden> writes:
> 
> > On Wed, Aug 21, 2013 at 05:10:56PM +0200, Markus Armbruster wrote:
> >> "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 :)
> >
> > 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.