From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55282) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T6OaR-0004If-PQ for qemu-devel@nongnu.org; Tue, 28 Aug 2012 12:24:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T6OaQ-0005Y4-3v for qemu-devel@nongnu.org; Tue, 28 Aug 2012 12:24:19 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42320) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T6OaP-0005Xy-S5 for qemu-devel@nongnu.org; Tue, 28 Aug 2012 12:24:18 -0400 Date: Tue, 28 Aug 2012 18:46:26 +0300 From: "Michael S. Tsirkin" Message-ID: <20120828154626.GD2903@redhat.com> References: <246329d9a275f3735aeaeab9326b1527330fe13a.1346069810.git.mst@redhat.com> <20120827190636.GC13049@redhat.com> <20120827192452.GE13049@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: gleb@redhat.com, kvm@vger.kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, Jan Kiszka , avi@redhat.com, Anthony Liguori On Mon, Aug 27, 2012 at 07:40:56PM +0000, Blue Swirl wrote: > On Mon, Aug 27, 2012 at 7:24 PM, Michael S. Tsirkin wrote: > > On Mon, Aug 27, 2012 at 07:12:27PM +0000, Blue Swirl wrote: > >> On Mon, Aug 27, 2012 at 7:06 PM, Michael S. Tsirkin wrote: > >> > On Mon, Aug 27, 2012 at 06:58:29PM +0000, Blue Swirl wrote: > >> >> On Mon, Aug 27, 2012 at 12:20 PM, Michael S. Tsirkin wrote: > >> >> > In preparation for adding PV EOI support, disable PV EOI by default for > >> >> > 1.1 and older machine types, to avoid CPUID changing during migration. > >> >> > > >> >> > PV EOI can still be enabled/disabled by specifying it explicitly. > >> >> > Enable for 1.1 > >> >> > -M pc-1.1 -cpu kvm64,+kvm_pv_eoi > >> >> > Disable for 1.2 > >> >> > -M pc-1.2 -cpu kvm64,-kvm_pv_eoi > >> >> > > >> >> > Signed-off-by: Michael S. Tsirkin > >> >> > --- > >> >> > hw/Makefile.objs | 2 +- > >> >> > hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ > >> >> > hw/cpu_flags.h | 9 +++++++++ > >> >> > hw/pc_piix.c | 2 ++ > >> >> > target-i386/cpu.c | 8 ++++++++ > >> >> > 5 files changed, 52 insertions(+), 1 deletion(-) > >> >> > create mode 100644 hw/cpu_flags.c > >> >> > create mode 100644 hw/cpu_flags.h > >> >> > > >> >> > diff --git a/hw/Makefile.objs b/hw/Makefile.objs > >> >> > index 850b87b..3f2532a 100644 > >> >> > --- a/hw/Makefile.objs > >> >> > +++ b/hw/Makefile.objs > >> >> > @@ -1,5 +1,5 @@ > >> >> > hw-obj-y = usb/ ide/ > >> >> > -hw-obj-y += loader.o > >> >> > +hw-obj-y += loader.o cpu_flags.o > >> >> > hw-obj-$(CONFIG_VIRTIO) += virtio-console.o > >> >> > hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o > >> >> > hw-obj-y += fw_cfg.o > >> >> > diff --git a/hw/cpu_flags.c b/hw/cpu_flags.c > >> >> > new file mode 100644 > >> >> > index 0000000..2422d20 > >> >> > --- /dev/null > >> >> > +++ b/hw/cpu_flags.c > >> >> > @@ -0,0 +1,32 @@ > >> >> > +/* > >> >> > + * CPU compatibility flags. > >> >> > + * > >> >> > + * Copyright (c) 2012 Red Hat Inc. > >> >> > + * Author: Michael S. Tsirkin. > >> >> > + * > >> >> > + * This program is free software; you can redistribute it and/or modify > >> >> > + * it under the terms of the GNU General Public License as published by > >> >> > + * the Free Software Foundation; either version 2 of the License, or > >> >> > + * (at your option) any later version. > >> >> > + * > >> >> > + * This program is distributed in the hope that it will be useful, > >> >> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > >> >> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > >> >> > + * GNU General Public License for more details. > >> >> > + * > >> >> > + * You should have received a copy of the GNU General Public License along > >> >> > + * with this program; if not, see . > >> >> > + */ > >> >> > +#include "hw/cpu_flags.h" > >> >> > + > >> >> > +static bool __kvm_pv_eoi_disabled; > >> >> > >> >> Don't use identifiers with leading underscores. > >> > > >> > C99 spec says " > >> > Any other predefined macro names > >> > shall begin with a leading underscore followed by an uppercase letter or > >> > a second underscore. > >> > " > >> > > >> > what are chances of compiler predefining macro __kvm_pv_eoi_disabled? > >> > >> Why do you even consider that since it's trivially easy to use > >> something else? If a standard (and HACKING in our case) specifies > >> something, why do you want to fight it? > > > > I missed this in HACKING, you are right: > > > > 2.4. Reserved namespaces in C and POSIX > > Underscore capital, double underscore, and underscore 't' suffixes > > should be avoided. > > > > so _kvm_pv_eoi_disabled is ok __kvm_pv_eoi_disabled is not. > > Will fix. > > No leading underscores. They are not used in QEMU. They are *widely* used in QEMU to mark internal stuff. E.g. parameters in many macros. In reality __ is also widely used. I'm still mulling removing 2.4 from HACKING - it appears too draconian, the chances of a conflict with preprocessor are remote and if it triggers, it's trivial to catch. We also have lots of existing code violating this rule. And the rule about _t suffix is just silly. > > > >> > > >> > But OK, will rename _kvm_pv_eoi_disabled. > >> > _ + lower case is guaranteed OK. > >> > >> No, just use kvm_pv_eoi_disabled, the underscore is useless. > > > > It isn't useless, this avoids conflict with function name. > > _ says it's an internal variable used to implement kvm_pv_eoi_disabled > > in a very clear way. > > Sure, but there are infinite number of ways of making the identifiers > unique. Using leading underscores is a way to ever conflict with > compiler, linker, libc, POSIX etc. I think you are mistaken. Anything to support this claim? > Don't do it. > Where's your imagination, can't you invent any other prefix or suffix? I like _, I think it's a standard C way to mark internal stuff and there is no need to invent anything. > > > > -- > > MST