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

[Thomas Huth <thuth@redhat.com>]

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 <thuth@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...

 Thomas