Re: [Qemu-devel] [PATCHv2 3/4] cpuid: disable pv eoi for 1.1 and older compat types

[Michael S. Tsirkin]

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 <address@hidden> 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 <address@hidden> 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 <address@hidden> 
> >> >> 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 <address@hidden>
> >> >> > ---
> >> >> >  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 <http://www.gnu.org/licenses/>.
> >> >> > + */
> >> >> > +#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