From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54298) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T6TWN-0000YY-No for qemu-devel@nongnu.org; Tue, 28 Aug 2012 17:40:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T6TWM-0008MD-0V for qemu-devel@nongnu.org; Tue, 28 Aug 2012 17:40:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63874) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T6TWL-0008LK-OB for qemu-devel@nongnu.org; Tue, 28 Aug 2012 17:40:25 -0400 Date: Wed, 29 Aug 2012 00:41:38 +0300 From: "Michael S. Tsirkin" Message-ID: <20120828214138.GD5817@redhat.com> References: <246329d9a275f3735aeaeab9326b1527330fe13a.1346069810.git.mst@redhat.com> <87d32abzjl.fsf@codemonkey.ws> <20120828194038.GF6223@otherpad.lan.raisama.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120828194038.GF6223@otherpad.lan.raisama.net> 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: Eduardo Habkost Cc: gleb@redhat.com, kvm@vger.kernel.org, mtosatti@redhat.com, qemu-devel@nongnu.org, Jan Kiszka , avi@redhat.com, Anthony Liguori 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" 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 > > > --- > > > 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; > > > + > > > +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 > > > + > > > +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 > > > #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