| Message ID | 246329d9a275f3735aeaeab9326b1527330fe13a.1346069810.git.mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Aug 27, 2012 at 12:20 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> > --- > 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. > + > +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 > >
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 <mst@redhat.com> 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 <mst@redhat.com> > > --- > > 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? But OK, will rename _kvm_pv_eoi_disabled. _ + lower case is guaranteed OK. > > + > > +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 > > > >
On Mon, Aug 27, 2012 at 7:06 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> >> > --- >> > 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 >> > >> >
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 <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> > >> > --- > >> > 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. > > > > 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.
On Mon, Aug 27, 2012 at 7:24 PM, Michael S. Tsirkin <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> >> >> > --- >> >> > 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. > >> > >> > 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. Don't do it. Where's your imagination, can't you invent any other prefix or suffix? > > -- > MST
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 <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> > >> >> > --- > >> >> > 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
On Tue, 28 Aug 2012, Michael S. Tsirkin wrote: > 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 <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> > > >> >> > --- > > >> >> > hw/Makefile.objs | 2 +- > > >> >> > hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ > > >> >> > hw/cpu_flags.h | 9 +++++++++ > > >> >> > hw/pc_piix.c | 2 ++ [..snip..] > > > > 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. > ISO/IEC 9899:TC3 7.1.3#1 - All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use. IOW no __ or _[A-Z] at all. - All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces. IOW _ as the name of an argument to a macro is (probably) okay. > 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. http://pubs.opengroup.org/onlinepubs/7908799/xns/namespace.html [..snip..]
"Michael S. Tsirkin" <mst@redhat.com> writes: > 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 <mst@redhat.com> > --- > 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; > + > +void disable_kvm_pv_eoi(void) > +{ > + __kvm_pv_eoi_disabled = true; > +} > + > +bool kvm_pv_eoi_disabled(void) > +{ > + return __kvm_pv_eoi_disabled; > +} Would this make more sense as a CPU flag or at least the KVM PV LAPIC? We really ought to embed the LAPIC in the CPU objects. Then it would all make a bit more sense conceptionally. But I definitely thing a CPU feature flag makes the most sense here. Then we can handle it via globals once we make CPU's qdev devices. Regards, Anthony Liguori > 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
On Tue, Aug 28, 2012 at 02:13:18PM -0500, Anthony Liguori wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > 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 <mst@redhat.com> > > --- > > 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; > > + > > +void disable_kvm_pv_eoi(void) > > +{ > > + __kvm_pv_eoi_disabled = true; > > +} > > + > > +bool kvm_pv_eoi_disabled(void) > > +{ > > + return __kvm_pv_eoi_disabled; > > +} > > Would this make more sense as a CPU flag or at least the KVM PV LAPIC? It is a CPU flag, already. The problem is that QEMU defaults to enabling blindly everything reported by the host kernel, potentially breaking migration (and this still has to be fixed). > > We really ought to embed the LAPIC in the CPU objects. Then it would > all make a bit more sense conceptionally. But I definitely thing a CPU > feature flag makes the most sense here. > > Then we can handle it via globals once we make CPU's qdev devices. Yes, this is a perfect candidate to use globals/qdev for machine-type compatibility. > > Regards, > > Anthony Liguori > > > 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 >
On Tue, Aug 28, 2012 at 02:13:18PM -0500, Anthony Liguori wrote: > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > 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 <mst@redhat.com> > > --- > > 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; > > + > > +void disable_kvm_pv_eoi(void) > > +{ > > + __kvm_pv_eoi_disabled = true; > > +} > > + > > +bool kvm_pv_eoi_disabled(void) > > +{ > > + return __kvm_pv_eoi_disabled; > > +} > > Would this make more sense as a CPU flag or at least the KVM PV LAPIC? > > We really ought to embed the LAPIC in the CPU objects. Then it would > all make a bit more sense conceptionally. But I definitely thing a CPU > feature flag makes the most sense here. > > Then we can handle it via globals once we make CPU's qdev devices. > > Regards, > > Anthony Liguori Sorry I do not understand what you suggest. This is a trivial bugfix patch it can be written in a million ways, I am confused by all the improvement suggestions. Could you please simply send me a patch? I can even test it. All I care about is that we get the functionality above: -M pc-1.1 --- disabled -M pc-1.2 --- enabled -M pc-1.1 -cpu kvm64,+kvm_pv_eoi -- enabled -M pc-1.2 -cpu kvm64,-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
On Tue, Aug 28, 2012 at 04:40:38PM -0300, Eduardo Habkost wrote: > On Tue, Aug 28, 2012 at 02:13:18PM -0500, Anthony Liguori wrote: > > "Michael S. Tsirkin" <mst@redhat.com> writes: > > > > > 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 <mst@redhat.com> > > > --- > > > 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; > > > + > > > +void disable_kvm_pv_eoi(void) > > > +{ > > > + __kvm_pv_eoi_disabled = true; > > > +} > > > + > > > +bool kvm_pv_eoi_disabled(void) > > > +{ > > > + return __kvm_pv_eoi_disabled; > > > +} > > > > Would this make more sense as a CPU flag or at least the KVM PV LAPIC? > > It is a CPU flag, already. The problem is that QEMU defaults to enabling > blindly everything reported by the host kernel, potentially breaking > migration (and this still has to be fixed). > > > > We really ought to embed the LAPIC in the CPU objects. Then it would > > all make a bit more sense conceptionally. But I definitely thing a CPU > > feature flag makes the most sense here. > > > > Then we can handle it via globals once we make CPU's qdev devices. > > Yes, this is a perfect candidate to use globals/qdev for machine-type > compatibility. OK so ... ACK? > > > > Regards, > > > > Anthony Liguori > > > > > 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 > > > > -- > Eduardo
On Tue, Aug 28, 2012 at 09:02:05PM +0400, malc wrote: > On Tue, 28 Aug 2012, Michael S. Tsirkin wrote: > > > 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 <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> 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 <mst@redhat.com> > > > >> >> > --- > > > >> >> > hw/Makefile.objs | 2 +- > > > >> >> > hw/cpu_flags.c | 32 ++++++++++++++++++++++++++++++++ > > > >> >> > hw/cpu_flags.h | 9 +++++++++ > > > >> >> > hw/pc_piix.c | 2 ++ > > [..snip..] > > > > > > > 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. > > > > ISO/IEC 9899:TC3 7.1.3#1 > > - All identifiers that begin with an underscore and either an > uppercase letter or another underscore are always reserved for any use. > > IOW no __ or _[A-Z] at all. > > - All identifiers that begin with an underscore are always reserved > for use as identifiers with file scope in both the ordinary and tag > name spaces. > > IOW _ as the name of an argument to a macro is (probably) okay. > > > 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. > > http://pubs.opengroup.org/onlinepubs/7908799/xns/namespace.html > > [..snip..] It's still silly :) > -- > mailto:av1474@comtv.ru
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; + +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);
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 <mst@redhat.com> --- 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