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

>                         - 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 :)