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

[Thomas Huth <thuth@redhat.com>]

On 28/11/15 16:09, Eduardo Habkost wrote:
> 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.
[...]
>>>>> 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.

Ok, thanks a lot for the explanation, that makes sense! So I think we'll
do it the same way in the pseries machine, too ... and apparently David
has even already posted a patch to do so :-)

 Thomas