From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eduardo Habkost Subject: Re: [Qemu-devel] [PATCH 3/4] pc: Add 2.9 machine-types Date: Wed, 4 Jan 2017 15:46:25 -0200 Message-ID: <20170104174625.GL3315@thinpad.lan.raisama.net> References: <1482866480-26208-1-git-send-email-ehabkost@redhat.com> <1482866480-26208-4-git-send-email-ehabkost@redhat.com> <20170104142007.GJ3315@thinpad.lan.raisama.net> <4c5191bf-aac6-a702-126e-a27f9cb2d941@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: qemu-devel@nongnu.org, Haozhong Zhang , kvm@vger.kernel.org, "Michael S. Tsirkin" , Marcelo Tosatti , Igor Mammedov , Paolo Bonzini To: Laszlo Ersek Return-path: Received: from mx1.redhat.com ([209.132.183.28]:46918 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S968710AbdADRq1 (ORCPT ); Wed, 4 Jan 2017 12:46:27 -0500 Content-Disposition: inline In-Reply-To: <4c5191bf-aac6-a702-126e-a27f9cb2d941@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Jan 04, 2017 at 05:40:08PM +0100, Laszlo Ersek wrote: > On 01/04/17 15:20, Eduardo Habkost wrote: > > On Wed, Jan 04, 2017 at 03:01:07PM +0100, Laszlo Ersek wrote: > >> On 12/27/16 20:21, Eduardo Habkost wrote: > >>> Cc: "Michael S. Tsirkin" > >>> Cc: Laszlo Ersek > >>> Cc: Igor Mammedov > >>> Signed-off-by: Eduardo Habkost > >>> --- > >>> include/hw/i386/pc.h | 1 + > >>> hw/i386/pc_piix.c | 15 ++++++++++++--- > >>> hw/i386/pc_q35.c | 13 +++++++++++-- > >>> 3 files changed, 24 insertions(+), 5 deletions(-) > >>> > >>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h > >>> index b22e699..ceeacca 100644 > >>> --- a/include/hw/i386/pc.h > >>> +++ b/include/hw/i386/pc.h > >>> @@ -375,6 +375,7 @@ int e820_get_num_entries(void); > >>> bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *); > >>> > >>> #define PC_COMPAT_2_8 \ > >>> + HW_COMPAT_2_8 > >>> > >>> #define PC_COMPAT_2_7 \ > >>> HW_COMPAT_2_7 \ > >> > >> Here I would recommend two things: > >> - introduce an empty PC_COMPAT_2_9 macro > >> - in the PC_COMPAT_2_8 macro being modified, add a trailing backslash > >> after HW_COMPAT_2_8 > >> > >> Both of these aim at keeping up the current pattern; namely, for the > >> most recent machine type, a PC_COMPAT macro should exist (even if empty > >> / unused), > > > > Why? An empty and unused macro would only confuse people reading > > the code. The empty PC_COMPAT_2_8 macro was added to the 2.8 tree > > by mistake in commit 14c985cffa6c. > > * First, the mistake was not in commit 14c985cffa6cb -- I see that "git > blame" fingers that commit, but if you actually show the commit, it > added the macro with a non-empty replacement text: > > +#define PC_COMPAT_2_8 \ > + {\ > + .driver = TYPE_X86_CPU,\ > + .property = "l3-cache",\ > + .value = "off",\ > + }, > + > + The mistake on 14c985cffa6c was adding a macro that was not used for anything. > > Instead, the mistake was (apparently) in commit 152fcbecad37, where > PC_COMPAT_2_8 was "emptied", but preserved. Yes, I agree this was (also) a mistake. > > * Second, you do realize periodic contributors / reviewers like me > cannot be expected to do a good job writing / reviewing patches if > existing practice in the tree cannot be trusted as example. I agree completely. :) > > If a standalone (empty) PC_COMPAT_2_9 macro is wrong / confusing (which > I agree it could be), then the empty PC_COMPAT_2_8 macro was wrong / > confusing just the same, and the reviewers of 152fcbecad37 should have > arguably caught it at that time. Yes, I agree. I just thought you had a reason to believe the existing empty macro was useful for something. I did ask Igor to remove the PC_COMPAT_2_8 macro[1]. Michael and Igor insisted that this should be done by a separate patch. That additional patch was never written, though, and that's also my fault (I just forgot about it after sending my R-b). [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg398426.html > > Anyway, my R-b stands, with whichever updates you prefer from my > suggestions. Thanks! > > Thanks > Laszlo > > > > >> plus each PC_COMPAT macro should be extensible by plain > >> appending (hence the final trailing backslash). > > > > Good idea. > > > >> > >> Functionally the above hunk is good of course, so I'll leave it to you > >> whether you actually want to address my comments. > >> > >> The rest looks fine; I should be able to rebase my broadcast SMI (v5) > >> stuff to this. > >> > >> Changed or unchanged: > >> > >> Reviewed-by: Laszlo Ersek > > > > Thanks! > > > >> > >> Thanks > >> Laszlo > >> > >>> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > >>> index 5e1adbe..9f102aa 100644 > >>> --- a/hw/i386/pc_piix.c > >>> +++ b/hw/i386/pc_piix.c > >>> @@ -437,13 +437,24 @@ static void pc_i440fx_machine_options(MachineClass *m) > >>> m->default_display = "std"; > >>> } > >>> > >>> -static void pc_i440fx_2_8_machine_options(MachineClass *m) > >>> +static void pc_i440fx_2_9_machine_options(MachineClass *m) > >>> { > >>> pc_i440fx_machine_options(m); > >>> m->alias = "pc"; > >>> m->is_default = 1; > >>> } > >>> > >>> +DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL, > >>> + pc_i440fx_2_9_machine_options); > >>> + > >>> +static void pc_i440fx_2_8_machine_options(MachineClass *m) > >>> +{ > >>> + pc_i440fx_2_9_machine_options(m); > >>> + m->is_default = 0; > >>> + m->alias = NULL; > >>> + SET_MACHINE_COMPAT(m, PC_COMPAT_2_8); > >>> +} > >>> + > >>> DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL, > >>> pc_i440fx_2_8_machine_options); > >>> > >>> @@ -451,8 +462,6 @@ DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL, > >>> static void pc_i440fx_2_7_machine_options(MachineClass *m) > >>> { > >>> pc_i440fx_2_8_machine_options(m); > >>> - m->is_default = 0; > >>> - m->alias = NULL; > >>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_7); > >>> } > >>> > >>> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c > >>> index d042fe0..dd792a8 100644 > >>> --- a/hw/i386/pc_q35.c > >>> +++ b/hw/i386/pc_q35.c > >>> @@ -301,19 +301,28 @@ static void pc_q35_machine_options(MachineClass *m) > >>> m->max_cpus = 288; > >>> } > >>> > >>> -static void pc_q35_2_8_machine_options(MachineClass *m) > >>> +static void pc_q35_2_9_machine_options(MachineClass *m) > >>> { > >>> pc_q35_machine_options(m); > >>> m->alias = "q35"; > >>> } > >>> > >>> +DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL, > >>> + pc_q35_2_9_machine_options); > >>> + > >>> +static void pc_q35_2_8_machine_options(MachineClass *m) > >>> +{ > >>> + pc_q35_2_9_machine_options(m); > >>> + m->alias = NULL; > >>> + SET_MACHINE_COMPAT(m, PC_COMPAT_2_8); > >>> +} > >>> + > >>> DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL, > >>> pc_q35_2_8_machine_options); > >>> > >>> static void pc_q35_2_7_machine_options(MachineClass *m) > >>> { > >>> pc_q35_2_8_machine_options(m); > >>> - m->alias = NULL; > >>> m->max_cpus = 255; > >>> SET_MACHINE_COMPAT(m, PC_COMPAT_2_7); > >>> } > >>> > >> > > > -- Eduardo