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

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

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

If I understand you correctly, you want to monkey-patch
machine->boot_order from machine->init().  Issues:

* machine->init() does not have access to machine.  Fixable.

* machine is read-only, except for a few places in vl.c (one is managing
  the list of registered machines, the other monkey-patches
  machine->max_cpus to one when it's zero).  I don't want *more*
  monkey-patching, I want *less*, so we can make machines const.  In
  fact, we can make current_machine const right away, and I think we
  should (patch coming).

* If machine->init() can change the default boot order, we get a data
  dependency cycle.  Current data dependency chain:

  0. Initialize QEMUMachine *machine to the default machine.

  1. Option parsing sets machine, and collects "boot-opts" options.

  2. Evaluation of "boot-opts": find normal boot order (from
     machine->boot_order and option parameter "boot") and one-time boot
     order (if option parameter "once" is given).

     Set boot_order to the initial boot order.

     Register a reset handler to revert the boot order from one-time to
     normal, if necessary.

  3. machine->init()

     Gets passed boot_order via QEMUMachineInitArgs.  Currently doesn't
     have access to machine.

  4. Set global variable current_machine = machine.

  Cycle: step 2 uses default boot order and defines boot order, step 3
  uses boot order and defines default boot order.

  I guess we could break this cycle by some sufficiently ingenious code
  shuffling.  But I'm pretty sure that would only complicate matters.
  Right now, boot order data flows down the program text, which makes it
  easy to understand.

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

Would you accept #define DEFAULT_PC_BOOT_ORDER in pc.h?