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

[Eduardo Habkost]

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