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

[Blue Swirl]

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?

>
> But OK, will rename _kvm_pv_eoi_disabled.
> _ + lower case is guaranteed OK.

No, just use kvm_pv_eoi_disabled, the underscore is useless.

>
>
>> > +
>> > +void disable_kvm_pv_eoi(void)
>> > +{
>> > +       __kvm_pv_eoi_disabled = true;
>> > +}
>> > +
>> > +bool kvm_pv_eoi_disabled(void)
>> > +{
>> > +       return __kvm_pv_eoi_disabled;
>> > +}
>> > diff --git a/hw/cpu_flags.h b/hw/cpu_flags.h
>> > new file mode 100644
>> > index 0000000..05777b6
>> > --- /dev/null
>> > +++ b/hw/cpu_flags.h
>> > @@ -0,0 +1,9 @@
>> > +#ifndef HW_CPU_FLAGS_H
>> > +#define HW_CPU_FLAGS_H
>> > +
>> > +#include <stdbool.h>
>> > +
>> > +void disable_kvm_pv_eoi(void);
>> > +bool kvm_pv_eoi_disabled(void);
>> > +
>> > +#endif
>> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> > index 008d42f..bdbceda 100644
>> > --- a/hw/pc_piix.c
>> > +++ b/hw/pc_piix.c
>> > @@ -46,6 +46,7 @@
>> >  #ifdef CONFIG_XEN
>> >  #  include <xen/hvm/hvm_info_table.h>
>> >  #endif
>> > +#include "cpu_flags.h"
>> >
>> >  #define MAX_IDE_BUS 2
>> >
>> > @@ -371,6 +372,7 @@ static QEMUMachine pc_machine_v1_2 = {
>> >
>> >  static void pc_machine_v1_1_compat(void)
>> >  {
>> > +    disable_kvm_pv_eoi();
>> >  }
>> >
>> >  static void pc_init_pci_v1_1(ram_addr_t ram_size,
>> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
>> > index 120a2e3..0d02fd1 100644
>> > --- a/target-i386/cpu.c
>> > +++ b/target-i386/cpu.c
>> > @@ -23,6 +23,7 @@
>> >
>> >  #include "cpu.h"
>> >  #include "kvm.h"
>> > +#include "asm/kvm_para.h"
>> >
>> >  #include "qemu-option.h"
>> >  #include "qemu-config.h"
>> > @@ -33,6 +34,7 @@
>> >  #include "hyperv.h"
>> >
>> >  #include "hw/hw.h"
>> > +#include "hw/cpu_flags.h"
>> >
>> >  /* feature flags taken from "Intel Processor Identification and the CPUID
>> >   * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
>> > @@ -889,6 +891,12 @@ static int cpu_x86_find_by_name(x86_def_t 
>> > *x86_cpu_def, const char *cpu_model)
>> >
>> >      plus_kvm_features = ~0; /* not supported bits will be filtered out 
>> > later */
>> >
>> > +    /* Disable PV EOI for old machine types.
>> > +     * Feature flags can still override. */
>> > +    if (kvm_pv_eoi_disabled()) {
>> > +        plus_kvm_features &= ~(0x1 << KVM_FEATURE_PV_EOI);
>> > +    }
>> > +
>> >      add_flagname_to_bitmaps("hypervisor", &plus_features,
>> >          &plus_ext_features, &plus_ext2_features, &plus_ext3_features,
>> >          &plus_kvm_features, &plus_svm_features);
>> > --
>> > MST
>> >
>> >