From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35903) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3Mjj-0002gN-OZ for qemu-devel@nongnu.org; Mon, 30 Nov 2015 06:35:17 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3Mjf-0005OA-K4 for qemu-devel@nongnu.org; Mon, 30 Nov 2015 06:35:15 -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> <5658D5EE.3020002@redhat.com> <20151128150910.GB23717@thinpad.lan.raisama.net> From: Thomas Huth Message-ID: <565C346A.1010304@redhat.com> Date: Mon, 30 Nov 2015 12:35:06 +0100 MIME-Version: 1.0 In-Reply-To: <20151128150910.GB23717@thinpad.lan.raisama.net> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 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