Re: [PATCH 00/11] cpu model bug fixes and definition corrections (v2)

Re: [PATCH 00/11] cpu model bug fixes and definition corrections (v2)

[Eduardo Habkost]

(CCing Marcelo, Avi, and kvm mailing list, so they can help answering
the uq/master patch flow question)

On Fri, Jun 03, 2011 at 12:51:42AM +0200, Jan Kiszka wrote:
> On 2011-06-02 21:34, Eduardo Habkost wrote:
> > Ouch, the subject prefix is completely wrong because of broken
> > git-send-email config on my side, sorry.
> > 
> > Please ignore the 'RHEL6 qemu-kvm' prefix, it is actually supposed to go
> > to the main Qemu tree.
> 
> Some of my review comments on John's original version still apply. Same
> for the advice on the patch flow (uq/master for kvm stuff).

Just to make sure I didn't miss anything:

1) uq/master flow: considering that most of the series is not
  KVM-specific but depends on patch 02/11 (Allow an optional
  qemu_early_init_vcpu()) what is the best approach? Should the whole
  series go through uq/master, or just patch 02/11? In the case of the
  latter, shall the rest of the series wait for the patch to be merged
  upstream, or should patch 02/11 go to both branches at the same time?

2) Reviewing cpu_x86_cpuid() cpuid hacking code & dropping
  -enable-nesting: should it hold the series, or may it be addressed
  after this series enter the tree?

3) Other recommendations for the qemu_early_init_vcpu() code
   (checkpatch.sh, return code evaluation, KVMState vs. VCPU): I will
   address those issues and send a new version.

Something else I may have missed?

-- 
Eduardo

Re: [PATCH 00/11] cpu model bug fixes and definition corrections (v2)

[Jan Kiszka]

On 2011-06-03 16:38, Eduardo Habkost wrote:
> (CCing Marcelo, Avi, and kvm mailing list, so they can help answering
> the uq/master patch flow question)
> 
> On Fri, Jun 03, 2011 at 12:51:42AM +0200, Jan Kiszka wrote:
>> On 2011-06-02 21:34, Eduardo Habkost wrote:
>>> Ouch, the subject prefix is completely wrong because of broken
>>> git-send-email config on my side, sorry.
>>>
>>> Please ignore the 'RHEL6 qemu-kvm' prefix, it is actually supposed to go
>>> to the main Qemu tree.
>>
>> Some of my review comments on John's original version still apply. Same
>> for the advice on the patch flow (uq/master for kvm stuff).
> 
> Just to make sure I didn't miss anything:
> 
> 1) uq/master flow: considering that most of the series is not
>   KVM-specific but depends on patch 02/11 (Allow an optional
>   qemu_early_init_vcpu()) what is the best approach? Should the whole
>   series go through uq/master, or just patch 02/11? In the case of the
>   latter, shall the rest of the series wait for the patch to be merged
>   upstream, or should patch 02/11 go to both branches at the same time?

I would suggest to break out those patches that touch KVM
infrastructure, post them for uq/master, and declare the rest of the
series to depend on them.

> 
> 2) Reviewing cpu_x86_cpuid() cpuid hacking code & dropping
>   -enable-nesting: should it hold the series, or may it be addressed
>   after this series enter the tree?

No that does not need to block the series. I would just recommend
checking if there is anything in that diff directly related. If not,
let's address it separately.

> 
> 3) Other recommendations for the qemu_early_init_vcpu() code
>    (checkpatch.sh, return code evaluation, KVMState vs. VCPU): I will
>    address those issues and send a new version.

Find some proposal for a refactored kvm_arch_get_supported_cpuid API
below.

> 
> Something else I may have missed?
> 

Nothing critical, I'm just hoping someone finds the time to fix
sysconfigs loading when starting qemu from a build directory.  :)

Thanks,
Jan

-------8<-------
From: Jan Kiszka <jan.kiszka@siemens.com>

kvm: x86: Pass KVMState to kvm_arch_get_supported_cpuid

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.

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

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 1ae2d61..fbdf612 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -106,12 +106,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);
         }
     }
@@ -121,7 +121,7 @@ static int get_para_features(CPUState *env)
 #endif
 
 
-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;
@@ -133,7 +133,7 @@ uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
 #endif
 
     max = 1;
-    while ((cpuid = try_get_cpuid(env->kvm_state, max)) == NULL) {
+    while ((cpuid = try_get_cpuid(s, max)) == NULL) {
         max *= 2;
     }
 
@@ -166,7 +166,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;
                 }
@@ -180,7 +180,7 @@ uint32_t kvm_arch_get_supported_cpuid(CPUState *env, uint32_t function,
 #ifdef CONFIG_KVM_PARA
     /* fallback for older kernels */
     if (!has_kvm_features && (function == KVM_CPUID_FEATURES)) {
-        ret = get_para_features(env);
+        ret = get_para_features(s);
     }
 #endif
 
@@ -374,6 +374,7 @@ 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;
@@ -381,20 +382,19 @@ int kvm_arch_init_vcpu(CPUState *env)
     uint32_t signature[3];
 #endif
 
-    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;
 
 #ifdef CONFIG_KVM_PARA
@@ -411,8 +411,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);
 
 #ifdef KVM_CAP_ASYNC_PF
     has_msr_async_pf_en = c->eax & (1 << KVM_FEATURE_ASYNC_PF);
@@ -485,7 +485,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++) {

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 00/11] cpu model bug fixes and definition corrections (v2)

[Markus Armbruster]

Jan Kiszka <jan.kiszka@siemens.com> writes:

[...]
>> Something else I may have missed?
>> 
>
> Nothing critical, I'm just hoping someone finds the time to fix
> sysconfigs loading when starting qemu from a build directory.  :)

There's nice to have, important, critical, and really annoying.  This
one's really annoying.