On Thu, Aug 22, 2013 at 10:39:34AM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <m...@redhat.com> writes:
>
> > On Wed, Aug 21, 2013 at 06:01:45PM +0200, Markus Armbruster wrote:
> >> "Michael S. Tsirkin" <m...@redhat.com> writes:
> >>
> >> > On Wed, Aug 21, 2013 at 05:10:56PM +0200, Markus Armbruster wrote:
> >> >> "Michael S. Tsirkin" <m...@redhat.com> 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 <arm...@redhat.com>
> >> >> >> ---
> >> >> [...]
> >> >> >> 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 :(
I'm not nacking this, but I think you'll agree it's not
a clear-cut improvement - if we are cleaning this up
it would be better to do something that fixes the
code duplication.
--
MST
