[Qemu-devel] [PATCH 00/15] QEMU KVM_GET_SUPPORTED_CPUID cleanups and fixes

[Qemu-devel] [PATCH 00/15] QEMU KVM_GET_SUPPORTED_CPUID cleanups and fixes

[Eduardo Habkost]

Most of this series are just cleanups that will help when making -cpu
check/enforce work properly, with some fixes.

In addition to code movements, the main changes are:
 - x2apic won't be enabled if in-kernel irqchip is disabled
   (patch 10)
 - CPUID feature bit filtering is done much earlier, and inside target-i386/cpu.c
   (patch 13)
 - CPUID leaf 7 feature bits are now filterd based on GET_SUPPORTED_CPUID too
   (patch 15)

Eduardo Habkost (15):
  i386: kvm: kvm_arch_get_supported_cpuid: move R_EDX hack outside of
    for loop
  i386: kvm: kvm_arch_get_supported_cpuid: clean up has_kvm_features
    check
  i386: kvm: kvm_arch_get_supported_cpuid: use 'entry' variable
  i386: kvm: extract register switch to cpuid_entry_get_reg() function
  i386: kvm: extract CPUID entry lookup to cpuid_find_entry() function
  i386: kvm: extract try_get_cpuid() loop to get_supported_cpuid()
    function
  i386: kvm: kvm_arch_get_supported_cpuid: replace if+switch with
    single 'if'
  i386: kvm: set CPUID_EXT_HYPERVISOR on kvm_arch_get_supported_cpuid()
  i386: kvm: set CPUID_EXT_TSC_DEADLINE_TIMER on
    kvm_arch_get_supported_cpuid()
  i386: kvm: x2apic is not supported without in-kernel irqchip
  i386: kvm: mask cpuid_kvm_features earlier
  i386: kvm: mask cpuid_ext4_features bits earlier
  i386: kvm: filter CPUID feature words earlier, on cpu.c
  i386: kvm: reformat filter_features_for_kvm() code
  i386: kvm: filter CPUID leaf 7 based on GET_SUPPORTED_CPUID, too

 kvm.h             |   1 +
 target-i386/cpu.c |  30 +++++++++++
 target-i386/kvm.c | 153 ++++++++++++++++++++++++++++++++----------------------
 3 files changed, 122 insertions(+), 62 deletions(-)

-- 
1.7.11.4

[Qemu-devel] [PATCH 01/15] i386: kvm: kvm_arch_get_supported_cpuid: move R_EDX hack outside of for loop

[Eduardo Habkost]

The for loop will become a separate function, so clean it up so it can
become independent from the bit hacking for R_EDX.

No behavior change[1], just code movement.

[1] Well, only if the kernel returned CPUID leafs 1 or 0x80000001 as
    unsupported, but there's no kernel version that does that.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 5b18383..8b4ab34 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -155,24 +155,29 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
                 break;
             case R_EDX:
                 ret = cpuid->entries[i].edx;
-                switch (function) {
-                case 1:
-                    /* KVM before 2.6.30 misreports the following features */
-                    ret |= CPUID_MTRR | CPUID_PAT | CPUID_MCE | CPUID_MCA;
-                    break;
-                case 0x80000001:
-                    /* On Intel, kvm returns cpuid according to the Intel spec,
-                     * so add missing bits according to the AMD spec:
-                     */
-                    cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
-                    ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES;
-                    break;
-                }
                 break;
             }
         }
     }
 
+    /* Fixups for the data returned by KVM, below */
+
+    if (reg == R_EDX) {
+        switch (function) {
+        case 1:
+            /* KVM before 2.6.30 misreports the following features */
+            ret |= CPUID_MTRR | CPUID_PAT | CPUID_MCE | CPUID_MCA;
+            break;
+        case 0x80000001:
+            /* On Intel, kvm returns cpuid according to the Intel spec,
+             * so add missing bits according to the AMD spec:
+             */
+            cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
+            ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES;
+            break;
+        }
+    }
+
     g_free(cpuid);
 
     /* fallback for older kernels */
-- 
1.7.11.4

[Qemu-devel] [PATCH 02/15] i386: kvm: kvm_arch_get_supported_cpuid: clean up has_kvm_features check

[Eduardo Habkost]

Instead of a function-specific has_kvm_features variable, simply use a
"found" variable that will be checked in case we have to use the legacy
get_para_features() interface.

No behavior change, just code cleanup.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 8b4ab34..9ebde66 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -130,7 +130,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
     int i, max;
     uint32_t ret = 0;
     uint32_t cpuid_1_edx;
-    int has_kvm_features = 0;
+    bool found = false;
 
     max = 1;
     while ((cpuid = try_get_cpuid(s, max)) == NULL) {
@@ -140,9 +140,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
     for (i = 0; i < cpuid->nent; ++i) {
         if (cpuid->entries[i].function == function &&
             cpuid->entries[i].index == index) {
-            if (cpuid->entries[i].function == KVM_CPUID_FEATURES) {
-                has_kvm_features = 1;
-            }
+            found = true;
             switch (reg) {
             case R_EAX:
                 ret = cpuid->entries[i].eax;
@@ -181,7 +179,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
     g_free(cpuid);
 
     /* fallback for older kernels */
-    if (!has_kvm_features && (function == KVM_CPUID_FEATURES)) {
+    if ((function == KVM_CPUID_FEATURES) && !found) {
         ret = get_para_features(s);
     }
 
-- 
1.7.11.4

[Qemu-devel] [PATCH 03/15] i386: kvm: kvm_arch_get_supported_cpuid: use 'entry' variable

[Eduardo Habkost]

The reg switch will be moved to a separate function, so store the entry
pointer in a variable.

No behavior change, just code movement.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 9ebde66..22e8564 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -140,19 +140,20 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
     for (i = 0; i < cpuid->nent; ++i) {
         if (cpuid->entries[i].function == function &&
             cpuid->entries[i].index == index) {
+            struct kvm_cpuid_entry2 *entry = &cpuid->entries[i];
             found = true;
             switch (reg) {
             case R_EAX:
-                ret = cpuid->entries[i].eax;
+                ret = entry->eax;
                 break;
             case R_EBX:
-                ret = cpuid->entries[i].ebx;
+                ret = entry->ebx;
                 break;
             case R_ECX:
-                ret = cpuid->entries[i].ecx;
+                ret = entry->ecx;
                 break;
             case R_EDX:
-                ret = cpuid->entries[i].edx;
+                ret = entry->edx;
                 break;
             }
         }
-- 
1.7.11.4

[Qemu-devel] [PATCH 04/15] i386: kvm: extract register switch to cpuid_entry_get_reg() function

[Eduardo Habkost]

No behavior change: just code movement.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 22e8564..ae51573 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -123,6 +123,28 @@ static int get_para_features(KVMState *s)
 }
 
 
+/* Returns the value for a specific register on the cpuid entry
+ */
+static uint32_t cpuid_entry_get_reg(struct kvm_cpuid_entry2 *entry, int reg)
+{
+    uint32_t ret = 0;
+    switch (reg) {
+    case R_EAX:
+        ret = entry->eax;
+        break;
+    case R_EBX:
+        ret = entry->ebx;
+        break;
+    case R_ECX:
+        ret = entry->ecx;
+        break;
+    case R_EDX:
+        ret = entry->edx;
+        break;
+    }
+    return ret;
+}
+
 uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
                                       uint32_t index, int reg)
 {
@@ -142,20 +164,7 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
             cpuid->entries[i].index == index) {
             struct kvm_cpuid_entry2 *entry = &cpuid->entries[i];
             found = true;
-            switch (reg) {
-            case R_EAX:
-                ret = entry->eax;
-                break;
-            case R_EBX:
-                ret = entry->ebx;
-                break;
-            case R_ECX:
-                ret = entry->ecx;
-                break;
-            case R_EDX:
-                ret = entry->edx;
-                break;
-            }
+            ret = cpuid_entry_get_reg(entry, reg);
         }
     }
 
-- 
1.7.11.4

[Qemu-devel] [PATCH 05/15] i386: kvm: extract CPUID entry lookup to cpuid_find_entry() function

[Eduardo Habkost]

No behavior change, just code movement.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index ae51573..3519028 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -145,11 +145,28 @@ static uint32_t cpuid_entry_get_reg(struct kvm_cpuid_entry2 *entry, int reg)
     return ret;
 }
 
+/* Find matching entry for function/index on kvm_cpuid2 struct
+ */
+static struct kvm_cpuid_entry2 *cpuid_find_entry(struct kvm_cpuid2 *cpuid,
+                                                 uint32_t function,
+                                                 uint32_t index)
+{
+    int i;
+    for (i = 0; i < cpuid->nent; ++i) {
+        if (cpuid->entries[i].function == function &&
+            cpuid->entries[i].index == index) {
+            return &cpuid->entries[i];
+        }
+    }
+    /* not found: */
+    return NULL;
+}
+
 uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
                                       uint32_t index, int reg)
 {
     struct kvm_cpuid2 *cpuid;
-    int i, max;
+    int max;
     uint32_t ret = 0;
     uint32_t cpuid_1_edx;
     bool found = false;
@@ -159,13 +176,10 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
         max *= 2;
     }
 
-    for (i = 0; i < cpuid->nent; ++i) {
-        if (cpuid->entries[i].function == function &&
-            cpuid->entries[i].index == index) {
-            struct kvm_cpuid_entry2 *entry = &cpuid->entries[i];
-            found = true;
-            ret = cpuid_entry_get_reg(entry, reg);
-        }
+    struct kvm_cpuid_entry2 *entry = cpuid_find_entry(cpuid, function, index);
+    if (entry) {
+        found = true;
+        ret = cpuid_entry_get_reg(entry, reg);
     }
 
     /* Fixups for the data returned by KVM, below */
-- 
1.7.11.4

[Qemu-devel] [PATCH 06/15] i386: kvm: extract try_get_cpuid() loop to get_supported_cpuid() function

[Eduardo Habkost]

No behavior change, just code movement.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 3519028..7115921 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -98,6 +98,19 @@ static struct kvm_cpuid2 *try_get_cpuid(KVMState *s, int max)
     return cpuid;
 }
 
+/* Run KVM_GET_SUPPORTED_CPUID ioctl(), allocating a buffer large enough
+ * for all entries.
+ */
+static struct kvm_cpuid2 *get_supported_cpuid(KVMState *s)
+{
+    struct kvm_cpuid2 *cpuid;
+    int max = 1;
+    while ((cpuid = try_get_cpuid(s, max)) == NULL) {
+        max *= 2;
+    }
+    return cpuid;
+}
+
 struct kvm_para_features {
     int cap;
     int feature;
@@ -166,15 +179,11 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
                                       uint32_t index, int reg)
 {
     struct kvm_cpuid2 *cpuid;
-    int max;
     uint32_t ret = 0;
     uint32_t cpuid_1_edx;
     bool found = false;
 
-    max = 1;
-    while ((cpuid = try_get_cpuid(s, max)) == NULL) {
-        max *= 2;
-    }
+    cpuid = get_supported_cpuid(s);
 
     struct kvm_cpuid_entry2 *entry = cpuid_find_entry(cpuid, function, index);
     if (entry) {
-- 
1.7.11.4

[Qemu-devel] [PATCH 07/15] i386: kvm: kvm_arch_get_supported_cpuid: replace if+switch with single 'if'

[Eduardo Habkost]

Additional fixups will be added, and making them a single 'if/else if'
chain makes it clearer than two nested switch statements.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 7115921..084b40c 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -193,20 +193,15 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
 
     /* Fixups for the data returned by KVM, below */
 
-    if (reg == R_EDX) {
-        switch (function) {
-        case 1:
-            /* KVM before 2.6.30 misreports the following features */
-            ret |= CPUID_MTRR | CPUID_PAT | CPUID_MCE | CPUID_MCA;
-            break;
-        case 0x80000001:
-            /* On Intel, kvm returns cpuid according to the Intel spec,
-             * so add missing bits according to the AMD spec:
-             */
-            cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
-            ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES;
-            break;
-        }
+    if (function == 1 && reg == R_EDX) {
+        /* KVM before 2.6.30 misreports the following features */
+        ret |= CPUID_MTRR | CPUID_PAT | CPUID_MCE | CPUID_MCA;
+    } else if (function == 0x80000001 && reg == R_EDX) {
+        /* On Intel, kvm returns cpuid according to the Intel spec,
+         * so add missing bits according to the AMD spec:
+         */
+        cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
+        ret |= cpuid_1_edx & CPUID_EXT2_AMD_ALIASES;
     }
 
     g_free(cpuid);
-- 
1.7.11.4

[Qemu-devel] [PATCH 08/15] i386: kvm: set CPUID_EXT_HYPERVISOR on kvm_arch_get_supported_cpuid()

[Eduardo Habkost]

Full grep for kvm_arch_get_supported_cpuid:

   kvm.h:uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
   target-i386/cpu.c:        x86_cpu_def->cpuid_7_0_ebx_features = kvm_arch_get_supported_cpuid(kvm_state, 0x7, 0, R_EBX);
   target-i386/cpu.c:            *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX);
   target-i386/cpu.c:            *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX);
   target-i386/cpu.c:            *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX);
   target-i386/cpu.c:            *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX);
   target-i386/cpu.c:            *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX);
   target-i386/cpu.c:            *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX);
   target-i386/cpu.c:            *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX);
   target-i386/cpu.c:            *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX);
   target-i386/kvm.c:uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
   target-i386/kvm.c:        cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
   target-i386/kvm.c:    env->cpuid_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
 * target-i386/kvm.c:    env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
   target-i386/kvm.c:    env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
   target-i386/kvm.c:    env->cpuid_ext3_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
   target-i386/kvm.c:    env->cpuid_svm_features  &= kvm_arch_get_supported_cpuid(s, 0x8000000A,
   target-i386/kvm.c:        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
   target-i386/kvm.c:            kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);

Note that there is only one call for CPUID[1].ECX above (*), and it is
the one that gets hacked to include CPUID_EXT_HYPERVISOR, so we can
simply make kvm_arch_get_supported_cpuid() set it, to let the rest of
the code automatically know that the flag can be safely set by QEMU.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 084b40c..1f451e3 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -196,6 +196,11 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
     if (function == 1 && reg == R_EDX) {
         /* KVM before 2.6.30 misreports the following features */
         ret |= CPUID_MTRR | CPUID_PAT | CPUID_MCE | CPUID_MCA;
+    } else if (function == 1 && reg == R_ECX) {
+        /* We can set the hypervisor flag, even if KVM does not return it on
+         * GET_SUPPORTED_CPUID
+         */
+        ret |= CPUID_EXT_HYPERVISOR;
     } else if (function == 0x80000001 && reg == R_EDX) {
         /* On Intel, kvm returns cpuid according to the Intel spec,
          * so add missing bits according to the AMD spec:
@@ -399,10 +404,8 @@ int kvm_arch_init_vcpu(CPUX86State *env)
 
     env->cpuid_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
 
-    i = env->cpuid_ext_features & CPUID_EXT_HYPERVISOR;
     j = env->cpuid_ext_features & CPUID_EXT_TSC_DEADLINE_TIMER;
     env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
-    env->cpuid_ext_features |= i;
     if (j && kvm_irqchip_in_kernel() &&
         kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
         env->cpuid_ext_features |= CPUID_EXT_TSC_DEADLINE_TIMER;
-- 
1.7.11.4

[Qemu-devel] [PATCH 09/15] i386: kvm: set CPUID_EXT_TSC_DEADLINE_TIMER on kvm_arch_get_supported_cpuid()

[Eduardo Habkost]

This moves the CPUID_EXT_TSC_DEADLINE_TIMER CPUID flag hacking from
kvm_arch_init_vcpu() to kvm_arch_get_supported_cpuid().

Full git grep for kvm_arch_get_supported_cpuid:

   kvm.h:uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
   target-i386/cpu.c:        x86_cpu_def->cpuid_7_0_ebx_features = kvm_arch_get_supported_cpuid(kvm_state, 0x7, 0, R_EBX);
   target-i386/cpu.c:            *eax = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EAX);
   target-i386/cpu.c:            *ebx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EBX);
   target-i386/cpu.c:            *ecx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_ECX);
   target-i386/cpu.c:            *edx = kvm_arch_get_supported_cpuid(s, 0xA, count, R_EDX);
   target-i386/cpu.c:            *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX);
   target-i386/cpu.c:            *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX);
   target-i386/cpu.c:            *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX);
   target-i386/cpu.c:            *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX);
   target-i386/kvm.c:uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
   target-i386/kvm.c:        cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
   target-i386/kvm.c:    env->cpuid_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
 * target-i386/kvm.c:    env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
   target-i386/kvm.c:    env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
   target-i386/kvm.c:    env->cpuid_ext3_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
   target-i386/kvm.c:    env->cpuid_svm_features  &= kvm_arch_get_supported_cpuid(s, 0x8000000A,
   target-i386/kvm.c:        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
   target-i386/kvm.c:            kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);

Note that there is only one call for CPUID[1].ECX above (*), and it is
the one that gets hacked to include CPUID_EXT_TSC_DEADLINE_TIMER, so we
can simply make kvm_arch_get_supported_cpuid() set it, to let the rest
of the code know the flag can be safely set by QEMU.

One thing I was worrying about when doing this is that now
kvm_arch_get_supported_cpuid() depends on kvm_irqchip_in_kernel(). But
the 'kvm_kernel_irqchip' global variable is initialized during
kvm_init(), that is called very early, and kvm_init() is already a
requirement to run the GET_SUPPORTED_CPUID ioctl() (as kvm_init() is the
function that initializes the 'kvm_state' global variable).

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 1f451e3..704ad33 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -201,6 +201,14 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
          * GET_SUPPORTED_CPUID
          */
         ret |= CPUID_EXT_HYPERVISOR;
+        /* tsc-deadline flag is not returned by GET_SUPPORTED_CPUID, but it
+         * can be enabled if the kernel has KVM_CAP_TSC_DEADLINE_TIMER,
+         * and the irqchip is in the kernel.
+         */
+        if (kvm_irqchip_in_kernel() &&
+                kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
+            ret |= CPUID_EXT_TSC_DEADLINE_TIMER;
+        }
     } else if (function == 0x80000001 && reg == R_EDX) {
         /* On Intel, kvm returns cpuid according to the Intel spec,
          * so add missing bits according to the AMD spec:
@@ -404,12 +412,7 @@ int kvm_arch_init_vcpu(CPUX86State *env)
 
     env->cpuid_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
 
-    j = env->cpuid_ext_features & CPUID_EXT_TSC_DEADLINE_TIMER;
     env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
-    if (j && kvm_irqchip_in_kernel() &&
-        kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
-        env->cpuid_ext_features |= CPUID_EXT_TSC_DEADLINE_TIMER;
-    }
 
     env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
                                                              0, R_EDX);
-- 
1.7.11.4

[Qemu-devel] [PATCH 10/15] i386: kvm: x2apic is not supported without in-kernel irqchip

[Eduardo Habkost]

This is necessary so that x2apic is not improperly enabled when the
in-kernel irqchip is disabled.

This won't generate a warning with "-cpu ...,check" because the current
check/enforce code is broken (it checks the host CPU data directly,
instead of using kvm_arch_get_supported_cpuid()), but it will be
eventually fixed to properly report the missing x2apic flag.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 704ad33..590f4d5 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -209,6 +209,13 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
                 kvm_check_extension(s, KVM_CAP_TSC_DEADLINE_TIMER)) {
             ret |= CPUID_EXT_TSC_DEADLINE_TIMER;
         }
+
+        /* x2apic is reported by GET_SUPPORTED_CPUID, but it can't be enabled
+         * without the in-kernel irqchip
+         */
+        if (!kvm_irqchip_in_kernel()) {
+            ret &= ~CPUID_EXT_X2APIC;
+        }
     } else if (function == 0x80000001 && reg == R_EDX) {
         /* On Intel, kvm returns cpuid according to the Intel spec,
          * so add missing bits according to the AMD spec:
-- 
1.7.11.4

[Qemu-devel] [PATCH 11/15] i386: kvm: mask cpuid_kvm_features earlier

[Eduardo Habkost]

Instead of masking the KVM feature bits very late (while building the
KVM_SET_CPUID2 data), mask it out on env->cpuid_kvm_features, at the
same point where the other feature words are masked out.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 590f4d5..3f4bee5 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -428,6 +428,9 @@ int kvm_arch_init_vcpu(CPUX86State *env)
     env->cpuid_svm_features  &= kvm_arch_get_supported_cpuid(s, 0x8000000A,
                                                              0, R_EDX);
 
+    env->cpuid_kvm_features &=
+            kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
+
     cpuid_i = 0;
 
     /* Paravirtualization CPUIDs */
@@ -448,8 +451,7 @@ int kvm_arch_init_vcpu(CPUX86State *env)
     c = &cpuid_data.entries[cpuid_i++];
     memset(c, 0, sizeof(*c));
     c->function = KVM_CPUID_FEATURES;
-    c->eax = env->cpuid_kvm_features &
-        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
+    c->eax = env->cpuid_kvm_features;
 
     if (hyperv_enabled()) {
         memcpy(signature, "Hv#1\0\0\0\0\0\0\0\0", 12);
-- 
1.7.11.4

[Qemu-devel] [PATCH 12/15] i386: kvm: mask cpuid_ext4_features bits earlier

[Eduardo Habkost]

This way all the filtering by GET_SUPPORTED_CPUID is being done at the
same place in the code.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/kvm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 3f4bee5..335b3e7 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -431,6 +431,9 @@ int kvm_arch_init_vcpu(CPUX86State *env)
     env->cpuid_kvm_features &=
             kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
 
+    env->cpuid_ext4_features &= kvm_arch_get_supported_cpuid(s, 0xC0000001,
+                                                             0, R_EDX);
+
     cpuid_i = 0;
 
     /* Paravirtualization CPUIDs */
@@ -572,8 +575,6 @@ int kvm_arch_init_vcpu(CPUX86State *env)
 
     /* Call Centaur's CPUID instructions they are supported. */
     if (env->cpuid_xlevel2 > 0) {
-        env->cpuid_ext4_features &=
-            kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
         cpu_x86_cpuid(env, 0xC0000000, 0, &limit, &unused, &unused, &unused);
 
         for (i = 0xC0000000; i <= limit; i++) {
-- 
1.7.11.4

[Qemu-devel] [PATCH 13/15] i386: kvm: filter CPUID feature words earlier, on cpu.c

[Eduardo Habkost]

cpu.c contains the code that will check if all requested CPU features
are available, so the filtering of KVM features must be there, so we can
implement "check" and "enforce" properly.

The only point where kvm_arch_init_vcpu() is called on i386 is:

- cpu_x86_init()
  - x86_cpu_realize() (after cpu_x86_register() is called)
    - qemu_init_vcpu()
      - qemu_kvm_start_vcpu()
        - qemu_kvm_thread_fn() (on a new thread)
          - kvm_init_vcpu()
            - kvm_arch_init_vcpu()

With this patch, the filtering will be done earlier, at:
- cpu_x86_init()
  - cpu_x86_register() (before x86_cpu_realize() is called)

Also, the KVM CPUID filtering will now be done at the same place where
the TCG CPUID feature filtering is done. Later, the code can be changed
to use the same filtering code for the "check" and "enforce" modes, as
now the cpu.c code knows exactly which CPU features are going to be
exposed to the guest (and much earlier).

One thing I was worrying about when doing this is that
kvm_arch_get_supported_cpuid() depends on kvm_irqchip_in_kernel(), and
maybe the 'kvm_kernel_irqchip' global variable wasn't initialized yet at
CPU creation time. But kvm_kernel_irqchip is initialized during
kvm_init(), that is called very early (much earlier than the machine
init function), and kvm_init() is already a requirement to run the
GET_SUPPORTED_CPUID ioctl() (as kvm_init() initializes the kvm_state
global variable).

Side note: it would be nice to keep KVM-specific code inside kvm.c. The
problem is that properly implementing -cpu check/enforce code (that's
inside cpu.c) depends directly on the feature bit filtering done using
kvm_arch_get_supported_cpuid(). Currently -cpu check/enforce is broken
because it simply uses the host CPU feature bits instead of
GET_SUPPORTED_CPUID, and we need to fix that.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 kvm.h             |  1 +
 target-i386/cpu.c | 30 ++++++++++++++++++++++++++++++
 target-i386/kvm.c | 18 ------------------
 3 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/kvm.h b/kvm.h
index dea2998..150f95d 100644
--- a/kvm.h
+++ b/kvm.h
@@ -20,6 +20,7 @@
 
 #ifdef CONFIG_KVM
 #include <linux/kvm.h>
+#include <linux/kvm_para.h>
 #endif
 
 extern int kvm_allowed;
diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index bb1e44e..a5c1628 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1360,6 +1360,32 @@ CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
     return cpu_list;
 }
 
+#ifdef CONFIG_KVM
+static void filter_features_for_kvm(X86CPU *cpu)
+{
+    CPUX86State *env = &cpu->env;
+    KVMState *s = kvm_state;
+
+    env->cpuid_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
+
+    env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
+
+    env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
+                                                             0, R_EDX);
+    env->cpuid_ext3_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
+                                                             0, R_ECX);
+    env->cpuid_svm_features  &= kvm_arch_get_supported_cpuid(s, 0x8000000A,
+                                                             0, R_EDX);
+
+    env->cpuid_kvm_features &=
+            kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
+
+    env->cpuid_ext4_features &= kvm_arch_get_supported_cpuid(s, 0xC0000001,
+                                                             0, R_EDX);
+
+}
+#endif
+
 int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 {
     CPUX86State *env = &cpu->env;
@@ -1417,6 +1443,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
             );
         env->cpuid_ext3_features &= TCG_EXT3_FEATURES;
         env->cpuid_svm_features &= TCG_SVM_FEATURES;
+    } else {
+#ifdef CONFIG_KVM
+        filter_features_for_kvm(cpu);
+#endif
     }
     object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error);
     if (error_is_set(&error)) {
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 335b3e7..0257083 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -410,30 +410,12 @@ int kvm_arch_init_vcpu(CPUX86State *env)
         struct kvm_cpuid2 cpuid;
         struct kvm_cpuid_entry2 entries[100];
     } QEMU_PACKED cpuid_data;
-    KVMState *s = env->kvm_state;
     uint32_t limit, i, j, cpuid_i;
     uint32_t unused;
     struct kvm_cpuid_entry2 *c;
     uint32_t signature[3];
     int r;
 
-    env->cpuid_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
-
-    env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
-
-    env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
-                                                             0, R_EDX);
-    env->cpuid_ext3_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
-                                                             0, R_ECX);
-    env->cpuid_svm_features  &= kvm_arch_get_supported_cpuid(s, 0x8000000A,
-                                                             0, R_EDX);
-
-    env->cpuid_kvm_features &=
-            kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
-
-    env->cpuid_ext4_features &= kvm_arch_get_supported_cpuid(s, 0xC0000001,
-                                                             0, R_EDX);
-
     cpuid_i = 0;
 
     /* Paravirtualization CPUIDs */
-- 
1.7.11.4

[Qemu-devel] [PATCH 14/15] i386: kvm: reformat filter_features_for_kvm() code

[Eduardo Habkost]

Cosmetic, but it will also help to make futher patches easier to review.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a5c1628..7921b1b 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1366,22 +1366,20 @@ static void filter_features_for_kvm(X86CPU *cpu)
     CPUX86State *env = &cpu->env;
     KVMState *s = kvm_state;
 
-    env->cpuid_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
-
-    env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
-
-    env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
-                                                             0, R_EDX);
-    env->cpuid_ext3_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
-                                                             0, R_ECX);
-    env->cpuid_svm_features  &= kvm_arch_get_supported_cpuid(s, 0x8000000A,
-                                                             0, R_EDX);
-
+    env->cpuid_features &=
+        kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
+    env->cpuid_ext_features &=
+        kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
+    env->cpuid_ext2_features &=
+        kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
+    env->cpuid_ext3_features &=
+        kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
+    env->cpuid_svm_features  &=
+        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
     env->cpuid_kvm_features &=
-            kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
-
-    env->cpuid_ext4_features &= kvm_arch_get_supported_cpuid(s, 0xC0000001,
-                                                             0, R_EDX);
+        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
+    env->cpuid_ext4_features &=
+        kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
 
 }
 #endif
-- 
1.7.11.4

[Qemu-devel] [PATCH 15/15] i386: kvm: filter CPUID leaf 7 based on GET_SUPPORTED_CPUID, too

[Eduardo Habkost]

Now that CPUID leaf 7 features can be enabled/disabled on the
command-line, we need to filter them properly using GET_SUPPORTED_CPUID,
at the same place where other features are filtered out.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 7921b1b..d8cd40a 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1376,6 +1376,8 @@ static void filter_features_for_kvm(X86CPU *cpu)
         kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
     env->cpuid_svm_features  &=
         kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
+    env->cpuid_7_0_ebx_features &=
+        kvm_arch_get_supported_cpuid(s, 7, 0, R_EBX);
     env->cpuid_kvm_features &=
         kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
     env->cpuid_ext4_features &=
-- 
1.7.11.4

Re: [Qemu-devel] [PATCH 00/15] QEMU KVM_GET_SUPPORTED_CPUID cleanups and fixes

[Eduardo Habkost]

Has anybody looked at this series?

Avi, Marcelo?

On Thu, Oct 04, 2012 at 05:48:52PM -0300, Eduardo Habkost wrote:
> Most of this series are just cleanups that will help when making -cpu
> check/enforce work properly, with some fixes.
> 
> In addition to code movements, the main changes are:
>  - x2apic won't be enabled if in-kernel irqchip is disabled
>    (patch 10)
>  - CPUID feature bit filtering is done much earlier, and inside target-i386/cpu.c
>    (patch 13)
>  - CPUID leaf 7 feature bits are now filterd based on GET_SUPPORTED_CPUID too
>    (patch 15)
> 
> Eduardo Habkost (15):
>   i386: kvm: kvm_arch_get_supported_cpuid: move R_EDX hack outside of
>     for loop
>   i386: kvm: kvm_arch_get_supported_cpuid: clean up has_kvm_features
>     check
>   i386: kvm: kvm_arch_get_supported_cpuid: use 'entry' variable
>   i386: kvm: extract register switch to cpuid_entry_get_reg() function
>   i386: kvm: extract CPUID entry lookup to cpuid_find_entry() function
>   i386: kvm: extract try_get_cpuid() loop to get_supported_cpuid()
>     function
>   i386: kvm: kvm_arch_get_supported_cpuid: replace if+switch with
>     single 'if'
>   i386: kvm: set CPUID_EXT_HYPERVISOR on kvm_arch_get_supported_cpuid()
>   i386: kvm: set CPUID_EXT_TSC_DEADLINE_TIMER on
>     kvm_arch_get_supported_cpuid()
>   i386: kvm: x2apic is not supported without in-kernel irqchip
>   i386: kvm: mask cpuid_kvm_features earlier
>   i386: kvm: mask cpuid_ext4_features bits earlier
>   i386: kvm: filter CPUID feature words earlier, on cpu.c
>   i386: kvm: reformat filter_features_for_kvm() code
>   i386: kvm: filter CPUID leaf 7 based on GET_SUPPORTED_CPUID, too
> 
>  kvm.h             |   1 +
>  target-i386/cpu.c |  30 +++++++++++
>  target-i386/kvm.c | 153 ++++++++++++++++++++++++++++++++----------------------
>  3 files changed, 122 insertions(+), 62 deletions(-)
> 
> -- 
> 1.7.11.4
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 00/15] QEMU KVM_GET_SUPPORTED_CPUID cleanups and fixes

[Marcelo Tosatti]

On Thu, Oct 04, 2012 at 05:48:52PM -0300, Eduardo Habkost wrote:
> Most of this series are just cleanups that will help when making -cpu
> check/enforce work properly, with some fixes.
> 
> In addition to code movements, the main changes are:
>  - x2apic won't be enabled if in-kernel irqchip is disabled
>    (patch 10)
>  - CPUID feature bit filtering is done much earlier, and inside target-i386/cpu.c
>    (patch 13)
>  - CPUID leaf 7 feature bits are now filterd based on GET_SUPPORTED_CPUID too
>    (patch 15)
> 
> Eduardo Habkost (15):
>   i386: kvm: kvm_arch_get_supported_cpuid: move R_EDX hack outside of
>     for loop
>   i386: kvm: kvm_arch_get_supported_cpuid: clean up has_kvm_features
>     check
>   i386: kvm: kvm_arch_get_supported_cpuid: use 'entry' variable
>   i386: kvm: extract register switch to cpuid_entry_get_reg() function
>   i386: kvm: extract CPUID entry lookup to cpuid_find_entry() function
>   i386: kvm: extract try_get_cpuid() loop to get_supported_cpuid()
>     function
>   i386: kvm: kvm_arch_get_supported_cpuid: replace if+switch with
>     single 'if'
>   i386: kvm: set CPUID_EXT_HYPERVISOR on kvm_arch_get_supported_cpuid()
>   i386: kvm: set CPUID_EXT_TSC_DEADLINE_TIMER on
>     kvm_arch_get_supported_cpuid()
>   i386: kvm: x2apic is not supported without in-kernel irqchip
>   i386: kvm: mask cpuid_kvm_features earlier
>   i386: kvm: mask cpuid_ext4_features bits earlier
>   i386: kvm: filter CPUID feature words earlier, on cpu.c
>   i386: kvm: reformat filter_features_for_kvm() code
>   i386: kvm: filter CPUID leaf 7 based on GET_SUPPORTED_CPUID, too
> 
>  kvm.h             |   1 +
>  target-i386/cpu.c |  30 +++++++++++
>  target-i386/kvm.c | 153 ++++++++++++++++++++++++++++++++----------------------
>  3 files changed, 122 insertions(+), 62 deletions(-)

Applied to uq/master, thanks.