On Wed, Oct 17, 2012 at 05:03:02PM -0300, Eduardo Habkost wrote: > On Wed, Oct 17, 2012 at 09:40:59PM +0200, Michael S. Tsirkin wrote: > > 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 newer machine type. > > > > Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > > --- > > hw/pc.h | 2 ++ > > hw/pc_piix.c | 14 +++++++++++++- > > target-i386/cpu.c | 15 +++++++++++++++ > > 3 files changed, 30 insertions(+), 1 deletion(-) > > > > diff --git a/hw/pc.h b/hw/pc.h > > index 9923d96..344a545 100644 > > --- a/hw/pc.h > > +++ b/hw/pc.h > > @@ -210,4 +210,6 @@ void pc_system_firmware_init(MemoryRegion *rom_memory); > > > > int e820_add_entry(uint64_t, uint64_t, uint32_t); > > > > +void enable_kvm_pv_eoi(void); > > + > > #endif > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > > index 82364ab..9d4cf48 100644 > > --- a/hw/pc_piix.c > > +++ b/hw/pc_piix.c > > @@ -301,6 +301,18 @@ static void pc_init_pci(ram_addr_t ram_size, > > initrd_filename, cpu_model, 1, 1); > > } > > > > +static void pc_init_pci_pv_eoi(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); > > +} > > Maybe it's better to name it pc_init_pci_1_3() (or something like that)? > I plan to submit a bug fix (unrelated to PV EOI) that will require using > a separate init function for the pc-1.3 machine type too, so pv_eoi > won't be the only difference between pc-1.2 and pc-1.3. >
1.4 will need to enable it too. I'l rename pc_init_pci as pc_init_pci_1_2 instead. > > + > > static void pc_init_pci_no_kvmclock(ram_addr_t ram_size, > > const char *boot_device, > > const char *kernel_filename, > > @@ -353,7 +365,7 @@ static QEMUMachine pc_machine_v1_3 = { > > .name = "pc-1.3", > > .alias = "pc", > > .desc = "Standard PC", > > - .init = pc_init_pci, > > + .init = pc_init_pci_pv_eoi, > > .max_cpus = 255, > > .is_default = 1, > > }; > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index f3708e6..a4a8a02 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1097,6 +1097,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, > > Visitor *v, void *opaque, > > cpu->env.tsc_khz = value / 1000; > > } > > > > +static bool kvm_pv_eoi_enabled; > > Maybe we can do this as: > > static int default_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); > [...] > static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > { > [...] > plus_kvm_features = default_kvm_features; > [...] > } > [...] > void enable_kvm_pv_eoi(void) > { > default_kvm_features = (1UL << KVM_FEATURE_PV_EOI); > } How is it better? Also disabling steal time etc for 1.3? Why? > On either case, the global variable should eventually go away when we > make CPU objects support global properties. But I still support > including a solution like this one, as the CPU modelling work is taking > really long to be finished and I don't think this should hold the > inclusion of pv_eoi support. > > > > + > > static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char > > *cpu_model) > > { > > unsigned int i; > > @@ -1135,6 +1137,8 @@ static int cpu_x86_find_by_name(x86_def_t > > *x86_cpu_def, const char *cpu_model) > > (1 << KVM_FEATURE_ASYNC_PF) | > > (1 << KVM_FEATURE_STEAL_TIME) | > > (1 << KVM_FEATURE_CLOCKSOURCE_STABLE_BIT); > > + if (kvm_pv_eoi_enabled) > > + plus_kvm_features |= (0x1 << KVM_FEATURE_PV_EOI); > > #else > > plus_kvm_features = 0; > > #endif > > @@ -1953,3 +1957,14 @@ static void x86_cpu_register_types(void) > > } > > > > type_init(x86_cpu_register_types) > > + > > +/* Called from hw/pc_piix.c but there is no header > > + * both files include to put this into. > > + * Put it here to silence compiler warning. > > + */ > > Why can't pc_piix.c simply include cpu.h? > > > +void enable_kvm_pv_eoi(void); > > + > > +void enable_kvm_pv_eoi(void) > > +{ > > + kvm_pv_eoi_enabled = true; > > +} > > -- > > MST > -- > Eduardo