Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Create pseries-2.6 machine

[Eduardo Habkost]

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 <address@hidden>
> >> ---
> >>  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