On Fri, Nov 27, 2015 at 11:15:10PM +0100, Thomas Huth wrote: > On 27/11/15 18:56, Eduardo Habkost wrote: > > On Fri, Nov 27, 2015 at 06:18:30PM +0100, Thomas Huth wrote: > >> On 27/11/15 10:55, Alexander Graf wrote: > >>> > >>> On 27.11.15 10:32, Thomas Huth wrote: > >>>> Add a new pseries-2.6 machine class version to make sure we can > >>>> keep the old types compatible to previous versions of QEMU in > >>>> later patches. > >>>> > >>>> Signed-off-by: Thomas Huth <th...@redhat.com> > >>>> --- > >>>> hw/ppc/spapr.c | 21 +++++++++++++++++++-- > >>>> 1 file changed, 19 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >>>> index 6bfb908..10b7c35 100644 > >>>> --- a/hw/ppc/spapr.c > >>>> +++ b/hw/ppc/spapr.c > >>>> @@ -2450,6 +2448,24 @@ static const TypeInfo spapr_machine_2_5_info = { > >>>> .class_init = spapr_machine_2_5_class_init, > >>>> }; > >>>> > >>>> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *data) > >>>> +{ > >>>> + MachineClass *mc = MACHINE_CLASS(oc); > >>>> + sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(oc); > >>>> + > >>>> + mc->name = "pseries-2.6"; > >>>> + mc->desc = "pSeries Logical Partition (PAPR compliant) v2.6"; > >>>> + mc->alias = "pseries"; > >>>> + mc->is_default = 1; > >>>> + smc->dr_lmb_enabled = true; > >>> > >>> We should probably start to follow a scheme similar to x86 where the new > >>> machine initialization calls its predecessor (2.5 in this case) to > >>> ensure we don't forget feature flags and quirks. > >>> > >>> So you can either directly call spapr_machine_2_5_class_init() from > >>> spapr_machine_2_6_class_init() or extract the quirk part > >>> (dr_lmb_enabled) into a function that gets marked as "from 2.5 on" in > >>> its function name and call it from 2_5_class_init and from a "from 2.6 > >>> on" function that gets called from 2_6_class_init. > >> > >> I like the idea of calling the functions in a chain. However, the i386 > >> people seem to do it the other way round, for example > >> pc_i440fx_2_4_machine_options() calls pc_i440fx_2_5_machine_options(). > >> That of course works, too, but it sounds a little bit cumbersome to me, > >> since when introducing a new machine class version, you do not only have > >> to introduce a function for the new class anymore, but also you have to > >> update the previous version to change the behavior that has been > >> introduced by the new function (see commit 87e896abe6d926 for example). > > > > The alias/is_default changes are only needed because we don't > > have a generic class alias system (yet), that would allow us to > > declare the "pc" alias and a default machine outside the > > machine_options() function. I agree it's cumbersome. > > > > commit 87e896abe6d926 has the extra broken_reserved_end change > > because for some reason we decided to add the broken_reserved_end > > quirk to pc-2.4 before we even introduced pc-2.5. That was an > > exception. The common case is to add the pc-2.4 quirks only after > > we added a pc-2.5 machine. > > > > The patch that adds pc-1.6, for example, looks like this: > > > > -static void pc_i440fx_2_5_machine_options(MachineClass *m) > > +static void pc_i440fx_2_6_machine_options(MachineClass *m) > > { > > pc_i440fx_machine_options(m); > > m->alias = "pc"; > > m->is_default = 1; > > } > > > > +DEFINE_I440FX_MACHINE(v2_6, "pc-i440fx-2.6", NULL, > > + pc_i440fx_2_6_machine_options); > > + > > +static void pc_i440fx_2_5_machine_options(MachineClass *m) > > +{ > > + pc_i440fx_2_6_machine_options(m); > > + m->alias = NULL; > > + m->is_default = 0; > > + SET_MACHINE_COMPAT(m, PC_COMPAT_2_5); > > +} > > > > Except for the alias/is_default stuff, it looks very simple to > > me. > > > > That said, I don't understand what you would suggest as > > alternative. Let's use pc-1.7 and pc-1.6 as examples: > > > > static void pc_compat_1_7(MachineState *machine) > > { > > pc_compat_2_0(machine); > > smbios_defaults = false; > > gigabyte_align = false; > > option_rom_has_mr = true; > > legacy_acpi_table_size = 6414; > > x86_cpu_change_kvm_default("x2apic", NULL); > > } > > > > static void pc_compat_1_6(MachineState *machine) > > { > > pc_compat_1_7(machine); > > rom_file_has_mr = false; > > has_acpi_build = false; > > } > > > > pc-1.7 and older need the smbios_defaults/gigabyte_align/ > > option_rom_has_mr/legacy_acpi_table_size/x2apic quirks. pc-2.0 > > and later don't need those quirks. How exactly would you make > > pc-1.6 and older inherit the quirks from pc-1.7, if not by > > reusing pc_compat_1_7() inside pc_compat_1_6()? > > > > (I am showing pc_compat_*() instead of *_machine_options(), > > because we're still moving compat stuff from pc_compat_*() to > > *_machine_options() functions. But the same questions apply once > > we move the compat code above to *_machine_options() functions). > > > > What's the alternative you propose? > > The quirk would have be set to false in the oldest machine instead, > something like: > > static void pc_compat_1_7(MachineState *machine) > { > pc_compat_1_6(machine); > rom_file_has_mr = true; > has_acpi_build = true; > ... > } > > static void pc_compat_1_6(MachineState *machine) > { > pc_compat_1_5(machine); > } > > ... > > static void pc_compat_0_13(MachineState *machine) > { > rom_file_has_mr = false; > has_acpi_build = false; > } > > And since "false" should also be the default for these variables, they > also could be omitted there and it would be sufficient to set > "rom_file_has_mr = true" and "has_acpi_build = true" once in the > pc_compat_1_7() function. > IMHO that should work fine, too, but maybe I just miss a point since I'm > quite new to these compatibility management stuff...
I think I see what you mean. It would work, but I see two issues: 1) The defaults in the QEMU hardware emulation code is the more recently introduced (and recommended) behavior, not the oldest legacy behavior. So the oldest machine-types would really need to set the variables to false. 2) I prefer to make the newer machine-types' code simpler and with less dependencies. The existing approch moves the complexity to the older machine-types, your suggestion moves the complexity to the newer ones. Any mistake done in the old (and probably unmaintained and unused) machine-types would affect all the newer ones. Also, this prevents us from easily deleting very old machine-types we don't want to support anymore. -- Eduardo