Re: [PATCH qom-cpu 02/11] target-i386: Disable kvm_mmu_op by default on pc-1.4

[Gleb Natapov <gleb@redhat.com>]

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.