| Message ID | 1357336872-7200-3-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jan 04, 2013 at 08:01:03PM -0200, Eduardo Habkost wrote: > The kvm_mmu_op feature was removed from the kernel since v3.3 (released > in March 2012), it was marked for removal since January 2011 and it's > slower than shadow or hardware assisted paging (see kernel commit > fb92045843). It doesn't make sense to keep it enabled by default. > Actually it was effectively removed Oct 1 2009 by a68a6a7282373. After 3 and a half years of not having it I think we can safely drop it without trying to preserve it in older machine types. > Also, keeping it enabled by default would cause unnecessary hassle when > libvirt start using the "enforce" option. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Cc: kvm@vger.kernel.org > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Gleb Natapov <gleb@redhat.com> > Cc: Marcelo Tosatti <mtosatti@redhat.com> > Cc: libvir-list@redhat.com > Cc: Jiri Denemark <jdenemar@redhat.com> > > I was planning to reverse the logic of the compat init functions and > make pc_init_pci_1_3() enable kvm_mmu_op and then call pc_init_pci_1_4() > instead. But that would require changing pc_init_pci_no_kvmclock() and > pc_init_isa() as well. So to keep the changes simple, I am keeping the > pattern used when pc_init_pci_1_3() was introduced, making > pc_init_pci_1_4() disable kvm_mmu_op and then call pc_init_pci_1_3(). > > Changes v2: > - Coding style fix > - Removed redundant comments above machine init functions > --- > hw/pc_piix.c | 9 ++++++++- > target-i386/cpu.c | 9 +++++++++ > target-i386/cpu.h | 1 + > 3 files changed, 18 insertions(+), 1 deletion(-) > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index 99747a7..a32af6a 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c > @@ -217,6 +217,7 @@ static void pc_init1(MemoryRegion *system_memory, > } > } > > +/* machine init function for pc-0.14 - pc-1.2 */ > static void pc_init_pci(QEMUMachineInitArgs *args) > { > ram_addr_t ram_size = args->ram_size; > @@ -238,6 +239,12 @@ static void pc_init_pci_1_3(QEMUMachineInitArgs *args) > pc_init_pci(args); > } > > +static void pc_init_pci_1_4(QEMUMachineInitArgs *args) > +{ > + disable_kvm_mmu_op(); > + pc_init_pci_1_3(args); > +} > + > static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) > { > ram_addr_t ram_size = args->ram_size; > @@ -285,7 +292,7 @@ static QEMUMachine pc_machine_v1_4 = { > .name = "pc-1.4", > .alias = "pc", > .desc = "Standard PC", > - .init = pc_init_pci_1_3, > + .init = pc_init_pci_1_4, > .max_cpus = 255, > .is_default = 1, > }; > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index e6435da..c83a566 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -158,6 +158,15 @@ void enable_kvm_pv_eoi(void) > #endif > } > > +void disable_kvm_mmu_op(void) > +{ > +#ifdef CONFIG_KVM No need for ifdef here too. > + if (kvm_enabled()) { > + kvm_default_features &= ~(1UL << KVM_FEATURE_MMU_OP); clear_bit() > + } > +#endif > +} > + > void host_cpuid(uint32_t function, uint32_t count, > uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) > { > diff --git a/target-i386/cpu.h b/target-i386/cpu.h > index 1283537..27c8d0c 100644 > --- a/target-i386/cpu.h > +++ b/target-i386/cpu.h > @@ -1219,5 +1219,6 @@ void do_smm_enter(CPUX86State *env1); > void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); > > void enable_kvm_pv_eoi(void); > +void disable_kvm_mmu_op(void); > > #endif /* CPU_I386_H */ > -- > 1.7.11.7 -- Gleb.
On Sun, Jan 06, 2013 at 03:38:28PM +0200, Gleb Natapov wrote: > On Fri, Jan 04, 2013 at 08:01:03PM -0200, Eduardo Habkost wrote: > > The kvm_mmu_op feature was removed from the kernel since v3.3 (released > > in March 2012), it was marked for removal since January 2011 and it's > > slower than shadow or hardware assisted paging (see kernel commit > > fb92045843). It doesn't make sense to keep it enabled by default. > > > Actually it was effectively removed Oct 1 2009 by a68a6a7282373. After 3 > and a half years of not having it I think we can safely drop it without > trying to preserve it in older machine types. Agreed. Especially considering that the check/enforce code for KVM flags is currently broken. So probably people using pc-1.0, pc-1.1, pc-1.2 are probably _not_ getting the kvm_mmu feature exposed to the guest. > > > Also, keeping it enabled by default would cause unnecessary hassle when > > libvirt start using the "enforce" option. > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > Cc: kvm@vger.kernel.org > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Cc: Gleb Natapov <gleb@redhat.com> > > Cc: Marcelo Tosatti <mtosatti@redhat.com> > > Cc: libvir-list@redhat.com > > Cc: Jiri Denemark <jdenemar@redhat.com> > > > > I was planning to reverse the logic of the compat init functions and > > make pc_init_pci_1_3() enable kvm_mmu_op and then call pc_init_pci_1_4() > > instead. But that would require changing pc_init_pci_no_kvmclock() and > > pc_init_isa() as well. So to keep the changes simple, I am keeping the > > pattern used when pc_init_pci_1_3() was introduced, making > > pc_init_pci_1_4() disable kvm_mmu_op and then call pc_init_pci_1_3(). > > > > Changes v2: > > - Coding style fix > > - Removed redundant comments above machine init functions > > --- > > hw/pc_piix.c | 9 ++++++++- > > target-i386/cpu.c | 9 +++++++++ > > target-i386/cpu.h | 1 + > > 3 files changed, 18 insertions(+), 1 deletion(-) > > > > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > > index 99747a7..a32af6a 100644 > > --- a/hw/pc_piix.c > > +++ b/hw/pc_piix.c > > @@ -217,6 +217,7 @@ static void pc_init1(MemoryRegion *system_memory, > > } > > } > > > > +/* machine init function for pc-0.14 - pc-1.2 */ > > static void pc_init_pci(QEMUMachineInitArgs *args) > > { > > ram_addr_t ram_size = args->ram_size; > > @@ -238,6 +239,12 @@ static void pc_init_pci_1_3(QEMUMachineInitArgs *args) > > pc_init_pci(args); > > } > > > > +static void pc_init_pci_1_4(QEMUMachineInitArgs *args) > > +{ > > + disable_kvm_mmu_op(); > > + pc_init_pci_1_3(args); > > +} > > + > > static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) > > { > > ram_addr_t ram_size = args->ram_size; > > @@ -285,7 +292,7 @@ static QEMUMachine pc_machine_v1_4 = { > > .name = "pc-1.4", > > .alias = "pc", > > .desc = "Standard PC", > > - .init = pc_init_pci_1_3, > > + .init = pc_init_pci_1_4, > > .max_cpus = 255, > > .is_default = 1, > > }; > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index e6435da..c83a566 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -158,6 +158,15 @@ void enable_kvm_pv_eoi(void) > > #endif > > } > > > > +void disable_kvm_mmu_op(void) > > +{ > > +#ifdef CONFIG_KVM > No need for ifdef here too. Same case of the previous patch: KVM_FEATURE_MMU_OP is available only if CONFIG_KVM is set.
diff --git a/hw/pc_piix.c b/hw/pc_piix.c index 99747a7..a32af6a 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -217,6 +217,7 @@ static void pc_init1(MemoryRegion *system_memory, } } +/* machine init function for pc-0.14 - pc-1.2 */ static void pc_init_pci(QEMUMachineInitArgs *args) { ram_addr_t ram_size = args->ram_size; @@ -238,6 +239,12 @@ static void pc_init_pci_1_3(QEMUMachineInitArgs *args) pc_init_pci(args); } +static void pc_init_pci_1_4(QEMUMachineInitArgs *args) +{ + disable_kvm_mmu_op(); + pc_init_pci_1_3(args); +} + static void pc_init_pci_no_kvmclock(QEMUMachineInitArgs *args) { ram_addr_t ram_size = args->ram_size; @@ -285,7 +292,7 @@ static QEMUMachine pc_machine_v1_4 = { .name = "pc-1.4", .alias = "pc", .desc = "Standard PC", - .init = pc_init_pci_1_3, + .init = pc_init_pci_1_4, .max_cpus = 255, .is_default = 1, }; diff --git a/target-i386/cpu.c b/target-i386/cpu.c index e6435da..c83a566 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -158,6 +158,15 @@ void enable_kvm_pv_eoi(void) #endif } +void disable_kvm_mmu_op(void) +{ +#ifdef CONFIG_KVM + if (kvm_enabled()) { + kvm_default_features &= ~(1UL << KVM_FEATURE_MMU_OP); + } +#endif +} + void host_cpuid(uint32_t function, uint32_t count, uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx) { diff --git a/target-i386/cpu.h b/target-i386/cpu.h index 1283537..27c8d0c 100644 --- a/target-i386/cpu.h +++ b/target-i386/cpu.h @@ -1219,5 +1219,6 @@ void do_smm_enter(CPUX86State *env1); void cpu_report_tpr_access(CPUX86State *env, TPRAccess access); void enable_kvm_pv_eoi(void); +void disable_kvm_mmu_op(void); #endif /* CPU_I386_H */
The kvm_mmu_op feature was removed from the kernel since v3.3 (released in March 2012), it was marked for removal since January 2011 and it's slower than shadow or hardware assisted paging (see kernel commit fb92045843). It doesn't make sense to keep it enabled by default. Also, keeping it enabled by default would cause unnecessary hassle when libvirt start using the "enforce" option. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: kvm@vger.kernel.org Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Gleb Natapov <gleb@redhat.com> Cc: Marcelo Tosatti <mtosatti@redhat.com> Cc: libvir-list@redhat.com Cc: Jiri Denemark <jdenemar@redhat.com> I was planning to reverse the logic of the compat init functions and make pc_init_pci_1_3() enable kvm_mmu_op and then call pc_init_pci_1_4() instead. But that would require changing pc_init_pci_no_kvmclock() and pc_init_isa() as well. So to keep the changes simple, I am keeping the pattern used when pc_init_pci_1_3() was introduced, making pc_init_pci_1_4() disable kvm_mmu_op and then call pc_init_pci_1_3(). Changes v2: - Coding style fix - Removed redundant comments above machine init functions --- hw/pc_piix.c | 9 ++++++++- target-i386/cpu.c | 9 +++++++++ target-i386/cpu.h | 1 + 3 files changed, 18 insertions(+), 1 deletion(-)