From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49980) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a2RIa-0005Vr-F2 for qemu-devel@nongnu.org; Fri, 27 Nov 2015 17:15:25 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a2RIV-0005v4-EI for qemu-devel@nongnu.org; Fri, 27 Nov 2015 17:15:24 -0500 References: <1448616740-27332-1-git-send-email-thuth@redhat.com> <1448616740-27332-2-git-send-email-thuth@redhat.com> <5658287A.1040209@suse.de> <56589066.1010108@redhat.com> <20151127175642.GA23717@thinpad.lan.raisama.net> From: Thomas Huth Message-ID: <5658D5EE.3020002@redhat.com> Date: Fri, 27 Nov 2015 23:15:10 +0100 MIME-Version: 1.0 In-Reply-To: <20151127175642.GA23717@thinpad.lan.raisama.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/2] hw/ppc/spapr: Create pseries-2.6 machine List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Nikunj A Dadhania , "Michael S. Tsirkin" , Alexey Kardashevskiy , qemu-devel@nongnu.org, Alexander Graf , qemu-ppc@nongnu.org, Paolo Bonzini , Richard Henderson , David Gibson 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 >>>> --- >>>> 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 = =3D { >>>> .class_init =3D spapr_machine_2_5_class_init, >>>> }; >>>> =20 >>>> +static void spapr_machine_2_6_class_init(ObjectClass *oc, void *dat= a) >>>> +{ >>>> + MachineClass *mc =3D MACHINE_CLASS(oc); >>>> + sPAPRMachineClass *smc =3D SPAPR_MACHINE_CLASS(oc); >>>> + >>>> + mc->name =3D "pseries-2.6"; >>>> + mc->desc =3D "pSeries Logical Partition (PAPR compliant) v2.6"; >>>> + mc->alias =3D "pseries"; >>>> + mc->is_default =3D 1; >>>> + smc->dr_lmb_enabled =3D 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 ha= ve >> to introduce a function for the new class anymore, but also you have t= o >> update the previous version to change the behavior that has been >> introduced by the new function (see commit 87e896abe6d926 for example)= . >=20 > 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. >=20 > 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. >=20 > The patch that adds pc-1.6, for example, looks like this: >=20 > -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 =3D "pc"; > m->is_default =3D 1; > } > =20 > +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 =3D NULL; > + m->is_default =3D 0; > + SET_MACHINE_COMPAT(m, PC_COMPAT_2_5); > +} >=20 > Except for the alias/is_default stuff, it looks very simple to > me. >=20 > That said, I don't understand what you would suggest as > alternative. Let's use pc-1.7 and pc-1.6 as examples: >=20 > static void pc_compat_1_7(MachineState *machine) > { > pc_compat_2_0(machine); > smbios_defaults =3D false; > gigabyte_align =3D false; > option_rom_has_mr =3D true; > legacy_acpi_table_size =3D 6414; > x86_cpu_change_kvm_default("x2apic", NULL); > } >=20 > static void pc_compat_1_6(MachineState *machine) > { > pc_compat_1_7(machine); > rom_file_has_mr =3D false; > has_acpi_build =3D false; > } >=20 > 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()? >=20 > (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). >=20 > 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 =3D true; has_acpi_build =3D 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 =3D false; has_acpi_build =3D 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 =3D true" and "has_acpi_build =3D 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