Skip to contentSkip to conversation

qemu: enable PV EOI for qemu 1.3

PATCH Superseded Michael S. Tsirkin 3 participants 5 messages Oct 18, 2012 – Oct 22, 2012

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/1 qemu: enable PV EOI for qemu 1.3

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.

---

Changes from v2:
Address comments by Andreas:
whitespace fixes and moving function around
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(-)
hw/pc_piix.c +14 -1
index 82364ab..be69dbd 100644
@@ -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,
 };
target-i386/cpu.c +20 -13
index f3708e6..0f77449 100644
@@ -124,6 +124,25 @@ 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 enable_kvm_pv_eoi(void)
+{
+    kvm_default_features |= kvm_pv_eoi_features;
+}
+
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
 {
@@ -1107,7 +1126,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 +1146,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);
target-i386/cpu.h +2 -0
index 871c270..de33303 100644
@@ -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 */
Signature · 1 line
MST
Reviewed-by on v3 1/1
#1 lore raw

Am 18.10.2012 17:01, schrieb Michael S. Tsirkin:

Quoted 5 lines

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.

Thanks for updating,

The commit message is not so telling whether there are any downsides (security implications?) to enabling this by default if supported, so I'll leave it to Anthony to ack/apply this.

Regards,
Andreas

Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
#2 lore raw
Quoted 5 lines · Andreas Färber

Am 18.10.2012 17:01, schrieb Michael S. Tsirkin:

Quoted 4 lines

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.

Quoted 8 lines
Quoted 1 line

Thanks for updating,

The commit message is not so telling whether there are any downsides (security implications?) to enabling this by default if supported,

I don't think there could be security downsides because all this does is tell guest about the feature in a convenient way.
A well behaved guest doesn't use a feature unless it's listed but that's irrelevant for security.

Quoted 5 lines

so
I'll leave it to Anthony to ack/apply this.

Regards,
Andreas

It used to be enabled. It was turned off in ef8621b1a3b199c348606c0a11a77d8e8bf135f1 because it affected migration format and doing that just before the release seemed too risky.

Quoted 3 lines
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
#3 lore raw
Quoted 12 lines · Michael S. Tsirkin
Quoted 11 lines · Andreas Färber

Am 18.10.2012 17:01, schrieb Michael S. Tsirkin:

Quoted 5 lines

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.

Thanks for updating,

Quoted 7 lines
Quoted 2 lines

The commit message is not so telling whether there are any downsides (security implications?) to enabling this by default if supported,

I don't think there could be security downsides because all this does is tell guest about the feature in a convenient way.
A well behaved guest doesn't use a feature unless it's listed but that's irrelevant for security.

True. It could be relevant if the host kernel did check the CPUID bits before letting the guest read or write the PV EOI MSR, but that's not the case.

Quoted 16 lines
Quoted 6 lines

so
I'll leave it to Anthony to ack/apply this.

Regards,
Andreas

It used to be enabled. It was turned off in ef8621b1a3b199c348606c0a11a77d8e8bf135f1 because it affected migration format and doing that just before the release seemed too risky.

Quoted 3 lines
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Signature · 1 line
Eduardo
Reviewed-by on v3 1/1
#4 lore raw
In reply to Michael S. Tsirkin
Quoted 5 lines · 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.

Quoted 128 lines

---

Changes from v2:
Address comments by Andreas:
whitespace fixes and moving function around
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(-)
hw/pc_piix.c +14 -1
index 82364ab..be69dbd 100644
@@ -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,
 };
target-i386/cpu.c +20 -13
index f3708e6..0f77449 100644
@@ -124,6 +124,25 @@ 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 enable_kvm_pv_eoi(void)
+{
+    kvm_default_features |= kvm_pv_eoi_features;
+}
+
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx)
 {
@@ -1107,7 +1126,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 +1146,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);
target-i386/cpu.h +2 -0
index 871c270..de33303 100644
@@ -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 */
Signature · 1 line
MST
Signature · 1 line
Eduardo