diff mbox

[11/12] kvm: x86: Pass KVMState to kvm_arch_get_supported_cpuid

Message ID 3c5277ff7e8ba99b17755e52e2d855a01dcefb87.1307542247.git.jan.kiszka@siemens.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jan Kiszka June 8, 2011, 2:11 p.m. UTC 
kvm_arch_get_supported_cpuid checks for global cpuid restrictions, it
does not require any CPUState reference. Changing its interface allows
to call it before any VCPU is initialized.

CC: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 kvm.h               |    2 +-
 target-i386/cpuid.c |   20 ++++++++++++--------
 target-i386/kvm.c   |   30 +++++++++++++++---------------
 3 files changed, 28 insertions(+), 24 deletions(-)

Comments

Eduardo Habkost June 9, 2011, 5:33 p.m. UTC | #1 
On Wed, Jun 08, 2011 at 04:11:05PM +0200, Jan Kiszka wrote:
> kvm_arch_get_supported_cpuid checks for global cpuid restrictions, it
> does not require any CPUState reference. Changing its interface allows
> to call it before any VCPU is initialized.

I'm wondering if it wouldn't be simpler to keep the existing interface
but just initialize CPUState->kvm_state earlier (today it is initialized
only on kvm_init_vcpu(), although the kvm_state global is initialized
much earlier).

Even with this new KVMState-based interface, code that needs to use
these functions before kvm_init_vcpu() (e.g.
check_features_against_host()) will have to use the 'kvm_state' global
(and I would like to avoid that).
Jan Kiszka June 9, 2011, 5:41 p.m. UTC | #2 
On 2011-06-09 19:33, Eduardo Habkost wrote:
> On Wed, Jun 08, 2011 at 04:11:05PM +0200, Jan Kiszka wrote:
>> kvm_arch_get_supported_cpuid checks for global cpuid restrictions, it
>> does not require any CPUState reference. Changing its interface allows
>> to call it before any VCPU is initialized.
> 
> I'm wondering if it wouldn't be simpler to keep the existing interface
> but just initialize CPUState->kvm_state earlier (today it is initialized
> only on kvm_init_vcpu(), although the kvm_state global is initialized
> much earlier).

If there was more need for a slit up, I would agree. But I do not see it.

> 
> Even with this new KVMState-based interface, code that needs to use
> these functions before kvm_init_vcpu() (e.g.
> check_features_against_host()) will have to use the 'kvm_state' global
> (and I would like to avoid that).

That is a different topic that needs to be resolved for other use cases
as well (e.g. for kvm devices). In many cases you should be able to find
a way to pass kvm_state from functions that already holds a reference.
Is that not true for your use case?

Jan
Eduardo Habkost June 9, 2011, 6:08 p.m. UTC | #3 
On Thu, Jun 09, 2011 at 07:41:03PM +0200, Jan Kiszka wrote:
<snip>
> > I'm wondering if it wouldn't be simpler to keep the existing interface
> > but just initialize CPUState->kvm_state earlier (today it is initialized
> > only on kvm_init_vcpu(), although the kvm_state global is initialized
> > much earlier).
> 
> If there was more need for a slit up, I would agree. But I do not see it.

I guess it won't hurt to make the functions require only what they
really need.

> > 
> > Even with this new KVMState-based interface, code that needs to use
> > these functions before kvm_init_vcpu() (e.g.
> > check_features_against_host()) will have to use the 'kvm_state' global
> > (and I would like to avoid that).
> 
> That is a different topic that needs to be resolved for other use cases
> as well (e.g. for kvm devices). In many cases you should be able to find
> a way to pass kvm_state from functions that already holds a reference.
> Is that not true for your use case?

My use case is cpu_x86_register()/check_features_against_host(), that
hold only a CPUState reference, and I wouldn't like to add a new
reference to the kvm_state global variable there.
Jan Kiszka June 9, 2011, 7:53 p.m. UTC | #4 
On 2011-06-09 20:08, Eduardo Habkost wrote:
> On Thu, Jun 09, 2011 at 07:41:03PM +0200, Jan Kiszka wrote:
> <snip>
>>> I'm wondering if it wouldn't be simpler to keep the existing interface
>>> but just initialize CPUState->kvm_state earlier (today it is initialized
>>> only on kvm_init_vcpu(), although the kvm_state global is initialized
>>> much earlier).
>>
>> If there was more need for a slit up, I would agree. But I do not see it.
> 
> I guess it won't hurt to make the functions require only what they
> really need.

Yes, so this patch has an independent value IMO.

> 
>>>
>>> Even with this new KVMState-based interface, code that needs to use
>>> these functions before kvm_init_vcpu() (e.g.
>>> check_features_against_host()) will have to use the 'kvm_state' global
>>> (and I would like to avoid that).
>>
>> That is a different topic that needs to be resolved for other use cases
>> as well (e.g. for kvm devices). In many cases you should be able to find
>> a way to pass kvm_state from functions that already holds a reference.
>> Is that not true for your use case?
> 
> My use case is cpu_x86_register()/check_features_against_host(), that
> hold only a CPUState reference, and I wouldn't like to add a new
> reference to the kvm_state global variable there.

For me, this is like requiring kvm_state for kvm_check_extension, and I
wouldn't hesitate using the global state until we solved that in a
better way. But if you feel like it's not appropriate here, then let's
go for early/late vcpu init.

Jan
diff mbox

Patch

diff --git a/kvm.h b/kvm.h
index d565dba..243b063 100644
--- a/kvm.h
+++ b/kvm.h
@@ -157,7 +157,7 @@  bool kvm_arch_stop_on_emulation_error(CPUState *env);
 
 int kvm_check_extension(KVMState *s, unsigned int extension);
 
-uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
+uint32_t kvm_arch_get_supported_cpuid(KVMState *env, uint32_t function,
                                       uint32_t index, int reg);
 void kvm_cpu_synchronize_state(CPUState *env);
 void kvm_cpu_synchronize_post_reset(CPUState *env);
diff --git a/target-i386/cpuid.c b/target-i386/cpuid.c
index 79e7580..e1ae3af 100644
--- a/target-i386/cpuid.c
+++ b/target-i386/cpuid.c
@@ -1144,10 +1144,12 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         break;
     case 7:
         if (kvm_enabled()) {
-            *eax = kvm_arch_get_supported_cpuid(env, 0x7, count, R_EAX);
-            *ebx = kvm_arch_get_supported_cpuid(env, 0x7, count, R_EBX);
-            *ecx = kvm_arch_get_supported_cpuid(env, 0x7, count, R_ECX);
-            *edx = kvm_arch_get_supported_cpuid(env, 0x7, count, R_EDX);
+            KVMState *s = env->kvm_state;
+
+            *eax = kvm_arch_get_supported_cpuid(s, 0x7, count, R_EAX);
+            *ebx = kvm_arch_get_supported_cpuid(s, 0x7, count, R_EBX);
+            *ecx = kvm_arch_get_supported_cpuid(s, 0x7, count, R_ECX);
+            *edx = kvm_arch_get_supported_cpuid(s, 0x7, count, R_EDX);
         } else {
             *eax = 0;
             *ebx = 0;
@@ -1179,10 +1181,12 @@  void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
             break;
         }
         if (kvm_enabled()) {
-            *eax = kvm_arch_get_supported_cpuid(env, 0xd, count, R_EAX);
-            *ebx = kvm_arch_get_supported_cpuid(env, 0xd, count, R_EBX);
-            *ecx = kvm_arch_get_supported_cpuid(env, 0xd, count, R_ECX);
-            *edx = kvm_arch_get_supported_cpuid(env, 0xd, count, R_EDX);
+            KVMState *s = env->kvm_state;
+
+            *eax = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EAX);
+            *ebx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EBX);
+            *ecx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_ECX);
+            *edx = kvm_arch_get_supported_cpuid(s, 0xd, count, R_EDX);
         } else {
             *eax = 0;
             *ebx = 0;
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 1c2d32c..5ebb054 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -98,12 +98,12 @@  struct kvm_para_features {
     { -1, -1 }
 };
 
-static int get_para_features(CPUState *env)
+static int get_para_features(KVMState *s)
 {
     int i, features = 0;
 
     for (i = 0; i < ARRAY_SIZE(para_features) - 1; i++) {
-        if (kvm_check_extension(env->kvm_state, para_features[i].cap)) {
+        if (kvm_check_extension(s, para_features[i].cap)) {
             features |= (1 << para_features[i].feature);
         }
     }
@@ -112,7 +112,7 @@  static int get_para_features(CPUState *env)
 }
 
 
-uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
+uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
                                       uint32_t index, int reg)
 {
     struct kvm_cpuid2 *cpuid;
@@ -122,7 +122,7 @@  uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
     int has_kvm_features = 0;
 
     max = 1;
-    while ((cpuid = try_get_cpuid(env->kvm_state, max)) == NULL) {
+    while ((cpuid = try_get_cpuid(s, max)) == NULL) {
         max *= 2;
     }
 
@@ -153,7 +153,7 @@  uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
                     /* 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(env, 1, 0, R_EDX);
+                    cpuid_1_edx = kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
                     ret |= cpuid_1_edx & 0x183f7ff;
                     break;
                 }
@@ -166,7 +166,7 @@  uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
 
     /* fallback for older kernels */
     if (!has_kvm_features && (function == KVM_CPUID_FEATURES)) {
-        ret = get_para_features(env);
+        ret = get_para_features(s);
     }
 
     return ret;
@@ -349,25 +349,25 @@  int kvm_arch_init_vcpu(CPUState *env)
         struct kvm_cpuid2 cpuid;
         struct kvm_cpuid_entry2 entries[100];
     } __attribute__((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];
 
-    env->cpuid_features &= kvm_arch_get_supported_cpuid(env, 1, 0, R_EDX);
+    env->cpuid_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_EDX);
 
     i = env->cpuid_ext_features & CPUID_EXT_HYPERVISOR;
-    env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(env, 1, 0, R_ECX);
+    env->cpuid_ext_features &= kvm_arch_get_supported_cpuid(s, 1, 0, R_ECX);
     env->cpuid_ext_features |= i;
 
-    env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(env, 0x80000001,
+    env->cpuid_ext2_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
                                                              0, R_EDX);
-    env->cpuid_ext3_features &= kvm_arch_get_supported_cpuid(env, 0x80000001,
+    env->cpuid_ext3_features &= kvm_arch_get_supported_cpuid(s, 0x80000001,
                                                              0, R_ECX);
-    env->cpuid_svm_features  &= kvm_arch_get_supported_cpuid(env, 0x8000000A,
+    env->cpuid_svm_features  &= kvm_arch_get_supported_cpuid(s, 0x8000000A,
                                                              0, R_EDX);
 
-
     cpuid_i = 0;
 
     /* Paravirtualization CPUIDs */
@@ -383,8 +383,8 @@  int kvm_arch_init_vcpu(CPUState *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(env,
-                                                KVM_CPUID_FEATURES, 0, R_EAX);
+    c->eax = env->cpuid_kvm_features &
+        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
 
     has_msr_async_pf_en = c->eax & (1 << KVM_FEATURE_ASYNC_PF);
 
@@ -453,7 +453,7 @@  int kvm_arch_init_vcpu(CPUState *env)
     /* Call Centaur's CPUID instructions they are supported. */
     if (env->cpuid_xlevel2 > 0) {
         env->cpuid_ext4_features &=
-            kvm_arch_get_supported_cpuid(env, 0xC0000001, 0, R_EDX);
+            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++) {