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?
>
> Thus here's a question for the x86 people: Was this order done on
> purpose and if so, why? Or has it just grown historically that way and
> would it maybe make sense to change the order to the IMHO more intuitive
> way, so that the newer machine setup function calls the older one
> instead of the other way round?
Why would we ever want a new machine-type call the older one? Why
would a newer machine-type inherit the quirks from the older
machine-types? The newer machine-types need less quirks than the
older ones, not more quirks.
>
> Next question is of course: What do we do in sPAPR land? Go the x86 way
> or do it the "big endian" way ;-) and do it the other way round?
If you decide to do it in a different way, I am curious to see
how you plan to make it work.
--
Eduardo
