From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33996) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VBQQD-00012M-GA for qemu-devel@nongnu.org; Mon, 19 Aug 2013 10:27:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VBQQ7-0003xF-FX for qemu-devel@nongnu.org; Mon, 19 Aug 2013 10:27:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45904) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VBQQ6-0003x3-VY for qemu-devel@nongnu.org; Mon, 19 Aug 2013 10:26:59 -0400 Date: Mon, 19 Aug 2013 17:28:45 +0300 From: "Michael S. Tsirkin" Message-ID: <20130819142845.GA5762@redhat.com> References: <20130818135002.GA5999@redhat.com> <20130819120428.GL2631@otherpad.lan.raisama.net> <20130819130210.GA810@redhat.com> <20130819141923.GH14443@otherpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20130819141923.GH14443@otherpad.lan.raisama.net> Subject: Re: [Qemu-devel] [PATCH] pc: cleanup 1.4 compat support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eduardo Habkost Cc: Paolo Bonzini , Anthony Liguori , Gerd Hoffmann , qemu-devel@nongnu.org, Andreas =?iso-8859-1?Q?F=E4rber?= On Mon, Aug 19, 2013 at 11:19:23AM -0300, Eduardo Habkost wrote: > On Mon, Aug 19, 2013 at 04:02:10PM +0300, Michael S. Tsirkin wrote: > > On Mon, Aug 19, 2013 at 09:04:28AM -0300, Eduardo Habkost wrote: > > > On Sun, Aug 18, 2013 at 04:50:02PM +0300, Michael S. Tsirkin wrote: > > > > Make 1.4 compat code call the 1.6 one, reducing > > > > code duplication. Add comment explaining why we can't > > > > make 1.4 call 1.5 as usual. > > > > > > > > Signed-off-by: Michael S. Tsirkin > > > > --- > > > > hw/i386/pc_piix.c | 4 ++-- > > > > hw/i386/pc_q35.c | 4 ++-- > > > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c > > > > index 15c932e..86cfadc 100644 > > > > --- a/hw/i386/pc_piix.c > > > > +++ b/hw/i386/pc_piix.c > > > > @@ -272,8 +272,8 @@ static void pc_init_pci_1_4(QEMUMachineInitArgs *args) > > > > { > > > > x86_cpu_compat_set_features("n270", FEAT_1_ECX, 0, CPUID_EXT_MOVBE); > > > > x86_cpu_compat_set_features("Westmere", FEAT_1_ECX, 0, CPUID_EXT_PCLMULQDQ); > > > > - has_pci_info = false; > > > > - pc_init_pci(args); > > > > + /* 1.5 was special - it enabled pvpanic in builtin machine */ > > > > + pc_init_pci_1_6(args); > > > > } > > > > > > > > > > I find this confusing (and probably it will be even more confusing when > > > people read this code a few years from now). I would prefer the > > > following pattern: > > > > > > static void pc_compat_1_5() > > > { > > > /* do 1.5 stuff (possibly undoing 1.6 stuff) */ > > > has_pvpanic = true; > > > } > > > > > > static void pc_init_pci_1_5() > > > { > > > pc_compat_1_5(); > > > pc_init_pci(); > > > } > > > > > > static void pc_compat_1_4() > > > { > > > pc_compat_1_5(); > > > /* do 1.4 stuff (possibly undoing 1.5 stuff) */ > > > has_pvpanic = false; > > > } > > > > > > static void pc_init_pci_1_4() > > > { > > > pc_compat_1_4(); > > > pc_init_pci(); > > > } > > > > > > static void pc_compat_1_3() > > > { > > > pc_compat_1_3(); > > > /* do 1.3 stuff (possibly undoing 1.4 stuff) */ > > > } > > > > > > static void pc_init_pci_1_3() > > > { > > > pc_compat_1_3(); > > > pc_init_pci(); > > > } > > > > > > Otherwise the same problem may happen again in the future. > > > > Sure, why not. Want to send a patch like this? > > Prefer a patch on top of this one, or instead of it? > > If you are OK with it, I will send a patch replacing this one. Sure, go ahead. > -- > Eduardo