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