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

[Michael S. Tsirkin]

On Mon, Aug 26, 2013 at 09:12:41AM +0200, Markus Armbruster wrote:
> "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.

I'm fine with cleanup being on top if this helps.