[Qemu-devel] [PATCH 0/2] Disable kvm_mmu_op by default on pc-1.4

[Qemu-devel] [PATCH 0/2] Disable kvm_mmu_op by default on pc-1.4

[Eduardo Habkost]

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.


Eduardo Habkost (2):
  target-i386: don't set any KVM flag by default if KVM is disabled
  target-i386: disable kvm_mmu_op by default on pc-1.4

 hw/pc_piix.c      | 11 ++++++++++-
 target-i386/cpu.c | 15 ++++++++++++---
 target-i386/cpu.h |  1 +
 3 files changed, 23 insertions(+), 4 deletions(-)

-- 
1.7.11.7

[Qemu-devel] [PATCH 1/2] target-i386: Don't set any KVM flag by default if KVM is disabled

[Eduardo Habkost]

This is a cleanup that tries to solve two small issues:

 - We don't need a separate kvm_pv_eoi_features variable just to keep a
   constant calculated at compile-time, and this style would require
   adding a separate variable (that's declared twice because of the
   CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
   by machine-type compat code.
 - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
   even when KVM is disabled at runtime. This small incosistency in
   the cpuid_kvm_features field isn't a problem today because
   cpuid_kvm_features is ignored by the TCG code, but it may cause
   unexpected problems later when refactoring the CPUID handling code.

This patch eliminates the kvm_pv_eoi_features variable and simply uses
CONFIG_KVM and kvm_enabled() inside the enable_kvm_pv_eoi() compat
function, so it enables kvm_pv_eoi only if KVM is enabled. I believe
this makes the behavior of enable_kvm_pv_eoi() clearer and easier to
understand.

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>
---
 target-i386/cpu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 82685dc..808001a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -145,15 +145,16 @@ static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
         (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 enable_kvm_pv_eoi(void)
 {
-    kvm_default_features |= kvm_pv_eoi_features;
+#ifdef CONFIG_KVM
+    if (kvm_enabled())
+        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
+#endif
 }
 
 void host_cpuid(uint32_t function, uint32_t count,
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 1/2] target-i386: Don't set any KVM flag by default if KVM is disabled

[Blue Swirl]

On Fri, Jan 4, 2013 at 2:52 PM, Eduardo Habkost <ehabkost@redhat.com> wrote:
> This is a cleanup that tries to solve two small issues:
>
>  - We don't need a separate kvm_pv_eoi_features variable just to keep a
>    constant calculated at compile-time, and this style would require
>    adding a separate variable (that's declared twice because of the
>    CONFIG_KVM ifdef) for each feature that's going to be enabled/disable
>    by machine-type compat code.
>  - The pc-1.3 code is setting the kvm_pv_eoi flag on cpuid_kvm_features
>    even when KVM is disabled at runtime. This small incosistency in
>    the cpuid_kvm_features field isn't a problem today because
>    cpuid_kvm_features is ignored by the TCG code, but it may cause
>    unexpected problems later when refactoring the CPUID handling code.
>
> This patch eliminates the kvm_pv_eoi_features variable and simply uses
> CONFIG_KVM and kvm_enabled() inside the enable_kvm_pv_eoi() compat
> function, so it enables kvm_pv_eoi only if KVM is enabled. I believe
> this makes the behavior of enable_kvm_pv_eoi() clearer and easier to
> understand.
>
> 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>
> ---
>  target-i386/cpu.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 82685dc..808001a 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -145,15 +145,16 @@ static uint32_t kvm_default_features = (1 << KVM_FEATURE_CLOCKSOURCE) |
>          (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 enable_kvm_pv_eoi(void)
>  {
> -    kvm_default_features |= kvm_pv_eoi_features;
> +#ifdef CONFIG_KVM
> +    if (kvm_enabled())

Missing braces, please read CODING_STYLE and use checkpatch.pl to find
problems in patches.

> +        kvm_default_features |= (1UL << KVM_FEATURE_PV_EOI);
> +#endif
>  }
>
>  void host_cpuid(uint32_t function, uint32_t count,
> --
> 1.7.11.7
>
>

Re: [Qemu-devel] [PATCH 1/2] target-i386: Don't set any KVM flag by default if KVM is disabled

[Eduardo Habkost]

On Fri, Jan 04, 2013 at 08:47:07PM +0000, Blue Swirl wrote:
> >  {
> > -    kvm_default_features |= kvm_pv_eoi_features;
> > +#ifdef CONFIG_KVM
> > +    if (kvm_enabled())
> 
> Missing braces, please read CODING_STYLE and use checkpatch.pl to find
> problems in patches.

Sorry (again). I will soon send a new version fixing the coding style
problems.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/2] target-i386: Don't set any KVM flag by default if KVM is disabled

[Eduardo Habkost]

On Fri, Jan 04, 2013 at 07:02:17PM -0200, Eduardo Habkost wrote:
> On Fri, Jan 04, 2013 at 08:47:07PM +0000, Blue Swirl wrote:
> > >  {
> > > -    kvm_default_features |= kvm_pv_eoi_features;
> > > +#ifdef CONFIG_KVM
> > > +    if (kvm_enabled())
> > 
> > Missing braces, please read CODING_STYLE and use checkpatch.pl to find
> > problems in patches.
> 
> Sorry (again). I will soon send a new version fixing the coding style
> problems.

checkpatch.pl is behaving weirdly, here:

$ git show 193e2f6 | grep 'if (kvm'
+    if (kvm_enabled())
$ git show 193e2f6 | ./scripts/checkpatch.pl -
total: 0 errors, 0 warnings, 19 lines checked

Your patch has no obvious style problems and is ready for submission.
$ 

-- 
Eduardo

[Qemu-devel] [PATCH 2/2] target-i386: Disable kvm_mmu_op by default on pc-1.4

[Eduardo Habkost]

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(). 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().

---
 hw/pc_piix.c      | 11 ++++++++++-
 target-i386/cpu.c |  8 ++++++++
 target-i386/cpu.h |  1 +
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 99747a7..a6bf645 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;
@@ -232,12 +233,20 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
              initrd_filename, cpu_model, 1, 1);
 }
 
+/* machine init function for pc-1.3 */
 static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
 {
     enable_kvm_pv_eoi();
     pc_init_pci(args);
 }
 
+/* machine init function for pc-1.4 */
+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 +294,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 808001a..ec877c7 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -157,6 +157,14 @@ 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 */
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH 2/2] target-i386: Disable kvm_mmu_op by default on pc-1.4

[Blue Swirl]

On Fri, Jan 4, 2013 at 2:52 PM, Eduardo Habkost <ehabkost@redhat.com> 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.
>
> 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(). 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().
>
> ---
>  hw/pc_piix.c      | 11 ++++++++++-
>  target-i386/cpu.c |  8 ++++++++
>  target-i386/cpu.h |  1 +
>  3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 99747a7..a6bf645 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;
> @@ -232,12 +233,20 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
>               initrd_filename, cpu_model, 1, 1);
>  }
>
> +/* machine init function for pc-1.3 */

The comment does give much information compared to the function name.

>  static void pc_init_pci_1_3(QEMUMachineInitArgs *args)
>  {
>      enable_kvm_pv_eoi();
>      pc_init_pci(args);
>  }
>
> +/* machine init function for pc-1.4 */

Ditto.

> +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 +294,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 808001a..ec877c7 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -157,6 +157,14 @@ void enable_kvm_pv_eoi(void)
>  #endif
>  }
>
> +void disable_kvm_mmu_op(void)
> +{
> +#ifdef CONFIG_KVM
> +    if (kvm_enabled())

Braces.

> +        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 */
> --
> 1.7.11.7
>
>

Re: [Qemu-devel] [PATCH 2/2] target-i386: Disable kvm_mmu_op by default on pc-1.4

[Eduardo Habkost]

On Fri, Jan 04, 2013 at 08:48:42PM +0000, Blue Swirl wrote:
[...]
> > +/* 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;
> > @@ -232,12 +233,20 @@ static void pc_init_pci(QEMUMachineInitArgs *args)
> >               initrd_filename, cpu_model, 1, 1);
> >  }
> >
> > +/* machine init function for pc-1.3 */
> 
> The comment does give much information compared to the function name.

You can't be sure from the function name if it's just for
1.3 or for 1.3 and higher/lower. e.g. before this patch, the
pc_init_pci_1_3() function was used by both pc-1.3 and pc-1.4.

But I will change this, anyway: I will document only the non-trivial
cases (that don't apply to only one machine-type version), it should be
better than redundant comments.

[...]
> > +void disable_kvm_mmu_op(void)
> > +{
> > +#ifdef CONFIG_KVM
> > +    if (kvm_enabled())
> 
> Braces.

Will be fixed in the next version.

-- 
Eduardo