| Message ID | 20121017221548.GA14428@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 18.10.2012 00:15, schrieb Michael S. Tsirkin: > Enable KVM PV EOI by default. You can still disable it with > -kvm_pv_eoi cpu flag. To avoid breaking cross-version migration, > enable only for qemu 1.3 (or in the future, newer) machine type. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Changes from v1: > Address comments by Eduardo: > use include instead of duplicate definition > reduce ifdef spagetti in code using features mask > rename init from _pv_eoi to _1_3 to enable adding > more stuff in this version > > hw/pc_piix.c | 15 ++++++++++++++- > target-i386/cpu.c | 33 ++++++++++++++++++++------------- > target-i386/cpu.h | 2 ++ > 3 files changed, 36 insertions(+), 14 deletions(-) > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index 82364ab..607de77 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c > @@ -43,6 +43,7 @@ > #include "xen.h" > #include "memory.h" > #include "exec-memory.h" > +#include "cpu.h" > #ifdef CONFIG_XEN > # include <xen/hvm/hvm_info_table.h> > #endif > @@ -301,6 +302,18 @@ static void pc_init_pci(ram_addr_t ram_size, > initrd_filename, cpu_model, 1, 1); > } > > +static void pc_init_pci_1_3(ram_addr_t ram_size, > + const char *boot_device, > + const char *kernel_filename, > + const char *kernel_cmdline, > + const char *initrd_filename, > + const char *cpu_model) Indentation was not adapted to name change? > +{ > + enable_kvm_pv_eoi(); > + pc_init_pci(ram_size, boot_device, kernel_filename, > + kernel_cmdline, initrd_filename, cpu_model); There's two tabs here. Thought you wanted to s/pc_init_pci/pc_init_pci_1_2/g and name this pc_init_pci()? Are you expecting there to be a pc_init_pci_1_4()? > +} > + > static void pc_init_pci_no_kvmclock(ram_addr_t ram_size, > const char *boot_device, > const char *kernel_filename, > @@ -353,7 +366,7 @@ static QEMUMachine pc_machine_v1_3 = { > .name = "pc-1.3", > .alias = "pc", > .desc = "Standard PC", > - .init = pc_init_pci, > + .init = pc_init_pci_1_3, > .max_cpus = 255, > .is_default = 1, > }; > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index f3708e6..5f316b3 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -124,6 +124,20 @@ typedef struct model_features_t { > int check_cpuid = 0; > int enforce_cpuid = 0; > > +#if defined(CONFIG_KVM) > +static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | > + (1 << KVM_FEATURE_NOP_IO_DELAY) | > + (1 << KVM_FEATURE_MMU_OP) | > + (1 << KVM_FEATURE_CLOCKSOURCE2) | > + (1 << KVM_FEATURE_ASYNC_PF) | > + (1 << KVM_FEATURE_STEAL_TIME) | > + (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); > +static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI); > +#else > +static uint32_t kvm_default_features = 0; > +static const uint32_t kvm_pv_eoi_features = 0; > +#endif > + > void host_cpuid(uint32_t function, uint32_t count, > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > { > @@ -1107,7 +1121,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > /* Features to be added*/ > uint32_t plus_features = 0, plus_ext_features = 0; > uint32_t plus_ext2_features = 0, plus_ext3_features = 0; > - uint32_t plus_kvm_features = 0, plus_svm_features = 0; > + uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0; > uint32_t plus_7_0_ebx_features = 0; > /* Features to be removed */ > uint32_t minus_features = 0, minus_ext_features = 0; > @@ -1127,18 +1141,6 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > memcpy(x86_cpu_def, def, sizeof(*def)); > } > > -#if defined(CONFIG_KVM) > - plus_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) | > - (1 << KVM_FEATURE_NOP_IO_DELAY) | > - (1 << KVM_FEATURE_MMU_OP) | > - (1 << KVM_FEATURE_CLOCKSOURCE2) | > - (1 << KVM_FEATURE_ASYNC_PF) | > - (1 << KVM_FEATURE_STEAL_TIME) | > - (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); > -#else > - plus_kvm_features = 0; > -#endif > - > add_flagname_to_bitmaps("hypervisor", &plus_features, > &plus_ext_features, &plus_ext2_features, &plus_ext3_features, > &plus_kvm_features, &plus_svm_features, &plus_7_0_ebx_features); > @@ -1953,3 +1955,8 @@ static void x86_cpu_register_types(void) > } > > type_init(x86_cpu_register_types) > + > +void enable_kvm_pv_eoi(void) > +{ > + kvm_default_features |= kvm_pv_eoi_features; > +} Indentation is off (7 spaces). I'd prefer to place this function below the variable definitions, so that type_init() remains at the bottom of the file. Otherwise I'm happy with the modified cpu.h-based interaction. So far there's no conflicts here, so it could go through either PC maintainer, kvm/uq or qom-cpu. Andreas > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 871c270..de33303 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -1188,4 +1188,6 @@ void do_smm_enter(CPUX86State *env1); > > void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); > > +void enable_kvm_pv_eoi(void); > + > #endif /* CPU_I386_H */
On Thu, Oct 18, 2012 at 04:13:38PM +0200, Andreas Färber wrote: > Am 18.10.2012 00:15, schrieb Michael S. Tsirkin: > > Enable KVM PV EOI by default. You can still disable it with > > -kvm_pv_eoi cpu flag. To avoid breaking cross-version migration, > > enable only for qemu 1.3 (or in the future, newer) machine type. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Changes from v1: > > Address comments by Eduardo: > > use include instead of duplicate definition > > reduce ifdef spagetti in code using features mask > > rename init from _pv_eoi to _1_3 to enable adding > > more stuff in this version > > > > hw/pc_piix.c | 15 ++++++++++++++- > > target-i386/cpu.c | 33 ++++++++++++++++++++------------- > > target-i386/cpu.h | 2 ++ > > 3 files changed, 36 insertions(+), 14 deletions(-) > > > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > > index 82364ab..607de77 100644 > > --- a/hw/pc_piix.c > > +++ b/hw/pc_piix.c > > @@ -43,6 +43,7 @@ > > #include "xen.h" > > #include "memory.h" > > #include "exec-memory.h" > > +#include "cpu.h" > > #ifdef CONFIG_XEN > > # include <xen/hvm/hvm_info_table.h> > > #endif > > @@ -301,6 +302,18 @@ static void pc_init_pci(ram_addr_t ram_size, > > initrd_filename, cpu_model, 1, 1); > > } > > > > +static void pc_init_pci_1_3(ram_addr_t ram_size, > > + const char *boot_device, > > + const char *kernel_filename, > > + const char *kernel_cmdline, > > + const char *initrd_filename, > > + const char *cpu_model) > > Indentation was not adapted to name change? > > > +{ > > + enable_kvm_pv_eoi(); > > + pc_init_pci(ram_size, boot_device, kernel_filename, > > + kernel_cmdline, initrd_filename, cpu_model); > > There's two tabs here. > > Thought you wanted to s/pc_init_pci/pc_init_pci_1_2/g and name this > pc_init_pci()? I decided it's not worth it - better keep the patch small. > Are you expecting there to be a pc_init_pci_1_4()? I don't know. Maybe we will be able to stick to .. _1_3 for the next 10 revisions. > > +} > > + > > static void pc_init_pci_no_kvmclock(ram_addr_t ram_size, > > const char *boot_device, > > const char *kernel_filename, > > @@ -353,7 +366,7 @@ static QEMUMachine pc_machine_v1_3 = { > > .name = "pc-1.3", > > .alias = "pc", > > .desc = "Standard PC", > > - .init = pc_init_pci, > > + .init = pc_init_pci_1_3, > > .max_cpus = 255, > > .is_default = 1, > > }; > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index f3708e6..5f316b3 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -124,6 +124,20 @@ typedef struct model_features_t { > > int check_cpuid = 0; > > int enforce_cpuid = 0; > > > > +#if defined(CONFIG_KVM) > > +static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | > > + (1 << KVM_FEATURE_NOP_IO_DELAY) | > > + (1 << KVM_FEATURE_MMU_OP) | > > + (1 << KVM_FEATURE_CLOCKSOURCE2) | > > + (1 << KVM_FEATURE_ASYNC_PF) | > > + (1 << KVM_FEATURE_STEAL_TIME) | > > + (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); > > +static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI); > > +#else > > +static uint32_t kvm_default_features = 0; > > +static const uint32_t kvm_pv_eoi_features = 0; > > +#endif > > + > > void host_cpuid(uint32_t function, uint32_t count, > > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > > { > > @@ -1107,7 +1121,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > > /* Features to be added*/ > > uint32_t plus_features = 0, plus_ext_features = 0; > > uint32_t plus_ext2_features = 0, plus_ext3_features = 0; > > - uint32_t plus_kvm_features = 0, plus_svm_features = 0; > > + uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0; > > uint32_t plus_7_0_ebx_features = 0; > > /* Features to be removed */ > > uint32_t minus_features = 0, minus_ext_features = 0; > > @@ -1127,18 +1141,6 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > > memcpy(x86_cpu_def, def, sizeof(*def)); > > } > > > > -#if defined(CONFIG_KVM) > > - plus_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) | > > - (1 << KVM_FEATURE_NOP_IO_DELAY) | > > - (1 << KVM_FEATURE_MMU_OP) | > > - (1 << KVM_FEATURE_CLOCKSOURCE2) | > > - (1 << KVM_FEATURE_ASYNC_PF) | > > - (1 << KVM_FEATURE_STEAL_TIME) | > > - (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); > > -#else > > - plus_kvm_features = 0; > > -#endif > > - > > add_flagname_to_bitmaps("hypervisor", &plus_features, > > &plus_ext_features, &plus_ext2_features, &plus_ext3_features, > > &plus_kvm_features, &plus_svm_features, &plus_7_0_ebx_features); > > @@ -1953,3 +1955,8 @@ static void x86_cpu_register_types(void) > > } > > > > type_init(x86_cpu_register_types) > > + > > +void enable_kvm_pv_eoi(void) > > +{ > > + kvm_default_features |= kvm_pv_eoi_features; > > +} > > Indentation is off (7 spaces). > > I'd prefer to place this function below the variable definitions, so > that type_init() remains at the bottom of the file. > > Otherwise I'm happy with the modified cpu.h-based interaction. > So far there's no conflicts here, so it could go through either PC > maintainer, kvm/uq or qom-cpu. > > Andreas > > > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > > index 871c270..de33303 100644 > > --- a/target-i386/cpu.h > > +++ b/target-i386/cpu.h > > @@ -1188,4 +1188,6 @@ void do_smm_enter(CPUX86State *env1); > > > > void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); > > > > +void enable_kvm_pv_eoi(void); > > + > > #endif /* CPU_I386_H */ I'll fix the whitespace and repost. Thanks for the review. > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
diff --git a/hw/pc_piix.c b/hw/pc_piix.c index 82364ab..607de77 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -43,6 +43,7 @@ #include "xen.h" #include "memory.h" #include "exec-memory.h" +#include "cpu.h" #ifdef CONFIG_XEN # include <xen/hvm/hvm_info_table.h> #endif @@ -301,6 +302,18 @@ static void pc_init_pci(ram_addr_t ram_size, initrd_filename, cpu_model, 1, 1); } +static void pc_init_pci_1_3(ram_addr_t ram_size, + const char *boot_device, + const char *kernel_filename, + const char *kernel_cmdline, + const char *initrd_filename, + const char *cpu_model) +{ + enable_kvm_pv_eoi(); + pc_init_pci(ram_size, boot_device, kernel_filename, + kernel_cmdline, initrd_filename, cpu_model); +} + static void pc_init_pci_no_kvmclock(ram_addr_t ram_size, const char *boot_device, const char *kernel_filename, @@ -353,7 +366,7 @@ static QEMUMachine pc_machine_v1_3 = { .name = "pc-1.3", .alias = "pc", .desc = "Standard PC", - .init = pc_init_pci, + .init = pc_init_pci_1_3, .max_cpus = 255, .is_default = 1, }; diff --git a/target-i386/cpu.c b/target-i386/cpu.c index f3708e6..5f316b3 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -124,6 +124,20 @@ typedef struct model_features_t { int check_cpuid = 0; int enforce_cpuid = 0; +#if defined(CONFIG_KVM) +static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) | + (1 << KVM_FEATURE_NOP_IO_DELAY) | + (1 << KVM_FEATURE_MMU_OP) | + (1 << KVM_FEATURE_CLOCKSOURCE2) | + (1 << KVM_FEATURE_ASYNC_PF) | + (1 << KVM_FEATURE_STEAL_TIME) | + (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); +static const uint32_t kvm_pv_eoi_features = (0x1 << KVM_FEATURE_PV_EOI); +#else +static uint32_t kvm_default_features = 0; +static const uint32_t kvm_pv_eoi_features = 0; +#endif + void host_cpuid(uint32_t function, uint32_t count, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) { @@ -1107,7 +1121,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) /* Features to be added*/ uint32_t plus_features = 0, plus_ext_features = 0; uint32_t plus_ext2_features = 0, plus_ext3_features = 0; - uint32_t plus_kvm_features = 0, plus_svm_features = 0; + uint32_t plus_kvm_features = kvm_default_features, plus_svm_features = 0; uint32_t plus_7_0_ebx_features = 0; /* Features to be removed */ uint32_t minus_features = 0, minus_ext_features = 0; @@ -1127,18 +1141,6 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) memcpy(x86_cpu_def, def, sizeof(*def)); } -#if defined(CONFIG_KVM) - plus_kvm_features = (1 << KVM_FEATURE_CLOCKSOURCE) | - (1 << KVM_FEATURE_NOP_IO_DELAY) | - (1 << KVM_FEATURE_MMU_OP) | - (1 << KVM_FEATURE_CLOCKSOURCE2) | - (1 << KVM_FEATURE_ASYNC_PF) | - (1 << KVM_FEATURE_STEAL_TIME) | - (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); -#else - plus_kvm_features = 0; -#endif - add_flagname_to_bitmaps("hypervisor", &plus_features, &plus_ext_features, &plus_ext2_features, &plus_ext3_features, &plus_kvm_features, &plus_svm_features, &plus_7_0_ebx_features); @@ -1953,3 +1955,8 @@ static void x86_cpu_register_types(void) } type_init(x86_cpu_register_types) + +void enable_kvm_pv_eoi(void) +{ + kvm_default_features |= kvm_pv_eoi_features; +} diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 871c270..de33303 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -1188,4 +1188,6 @@ void do_smm_enter(CPUX86State *env1); void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); +void enable_kvm_pv_eoi(void); + #endif /* CPU_I386_H */
Enable KVM PV EOI by default. You can still disable it with -kvm_pv_eoi cpu flag. To avoid breaking cross-version migration, enable only for qemu 1.3 (or in the future, newer) machine type. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Changes from v1: Address comments by Eduardo: use include instead of duplicate definition reduce ifdef spagetti in code using features mask rename init from _pv_eoi to _1_3 to enable adding more stuff in this version hw/pc_piix.c | 15 ++++++++++++++- target-i386/cpu.c | 33 ++++++++++++++++++++------------- target-i386/cpu.h | 2 ++ 3 files changed, 36 insertions(+), 14 deletions(-)