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

[Markus Armbruster]

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

> On Thu, Aug 22, 2013 at 01:24:50PM +0200, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <address@hidden> writes:
>> 
>> > On Thu, Aug 22, 2013 at 10:39:34AM +0200, Markus Armbruster wrote:
>> >> "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.
>> >
>> > I agree, it's a concern.
>> >
>> >> >> > 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?
>> >
>> >
>> > Here's what bothers me.
>> > static QEMUMachine pc_i440fx_machine_v1_6 = {
>> >     .name = "pc-i440fx-1.6",
>> >     .alias = "pc", 
>> >     .desc = "Standard PC (i440FX + PIIX, 1996)", <-- mostly copied over,
>> >                            but different for 1.3 - this is likely a bug
>> >     .init = pc_init_pci_1_6,
>> >     .hot_add_cpu = pc_hot_add_cpu, <- has to be copied over at least for
>> >                                    newer PCs. Not sure about older ones -
>> >                                    could be a bug or intentional
>> >     .max_cpus = 255,               <- always 255 except isapc
>> >     DEFAULT_MACHINE_OPTIONS, <- always copied
>> > };
>> >
>> >
>> > So there's a lot of boiler plate eahc time we add
>> > a machine type.
>> >
>> > DEFAULT_MACHINE_OPTIONS kind of offered a way to address
>> > that, maybe with per-version inheritance like we do
>> > for compat properties.
>> >
>> > Now you want to remove that for style reasons, so we'll
>> > have to stay with duplicated code :(
>> 
>> DEFAULT_MACHINE_OPTIONS are copied to *every* machine.  I can't see why
>> anything that's common to every machine should be duplicated in every
>> machine type definition.
>> 
>> Turns out the stuff DEFAULT_MACHINE_OPTIONS copies shouldn't be copied:
>> it's bogus for most machines.  My patch cleans that up, no more, no
>> less.
>> 
>> There are groups of related machines, such as the PC machines, which
>> have stuff in common legitimately.  Avoiding duplicating this stuff in
>> their machine initializers may be worthwhile.  However, that's not my
>> patch's aim.  Nothing in my patch precludes future de-duplication.
>> 
>> > I'm not nacking this, but I think you'll agree it's not
>> > a clear-cut improvement
>> 
>> I agree de-duplication may be worthwhile.  I further agree my patch
>> makes the existing duplication of one initializer (default boot order) a
>> bit more visible than it was before (in addition to removing its
>> existing duplication from lots of places where it makes no sense).  It
>> doesn't affect the existing duplication of all the other initializers.
>
> Now that it's visible, maybe you can be persuaded to fix it?
> If it were not for code duplication, your patch would have
> been much smaller, right?

Not really.

The patch touches all 100 machine types.  For 65 types,
DEFAULT_MACHINE_OPTIONS is simply dropped without replacement (patch
can't get smaller than that).  The remaining 35 can be grouped as
follows:

* nseries (n800, n810)

  Two machines get .default_boot_order = ""

* prep, g3beige, mac99

  Three machines either .default_boot_order = "cd" or "cad".  Really
  depends on the machine, so no duplication here.

* spapr

  Doesn't use DEFAULT_MACHINE_OPTIONS, still touched because I rename
  .boot_order to .default_boot_order.

* sun4[mdc]

  Twelve machines get .default_boot_order = "c".  Some or all may be
  related closely enough to make this duplication, but I don't know.

* pc and its variants

  Of 19 PC types, 18 get .default_boot_order = "cad", and one (xenfv)
  gets nothing.

Total diffstat is 59 files changed, 51 insertions(+), 119 deletions(-).

If we accomplished perfect de-duplication of PC other than xenfv, we'd
save 17 insertions.  If we can do the same for sun4[mdc], we'd save
another 11.

The machinery enabling de-duplication would likely change more than 28
lines, and insert *code* rather than data.

>> >                         - if we are cleaning this up
>> > it would be better to do something that fixes the
>> > code duplication.
>> 
>> The patch is not about cleaning up code duplication in related machine
>> initializers.  It's about cleaning up bogus default boot orders.
>> 
>> I'm wary of patch series mission creep :)
>
> My point is that once we have that cleanup, it's possible
> that you will want to tweak 6/6.

I definitely would not want to tweak 6/6 for that, because it's hard
enough to review as it is, and it already got competent review.  Any
further cleanup should be done on top.

The cleanup could drop up to 30 lines of trivial code changed in this
patch.  That's not nearly enough churn to make invalidating a review
that "wasn't easy" according to Laszlo worthwhile.