[Qemu-devel] [PATCH 0/7 RESEND] target-i386: Simplify kvm_cpu_fill_host() and kvm_check_features_against_host()

[Qemu-devel] [PATCH 0/7 RESEND] target-i386: Simplify kvm_cpu_fill_host() and kvm_check_features_against_host()

[Eduardo Habkost]

Resend of series submitted on 24 November 2013, that didn't get any reply. Only
change is a trivial conflict on patch 7/7.

This series simplifies kvm_cpu_fill_host() and
kvm_check_features_against_host() to simply use FeatureWord & feature_word_info
loops to fill/check feature words.

The initial motivation for this was to avoid hacks involving the "host" CPU
class on the forthcoming conversion of CPU models to be X86CPU subclasses.
Instead of requiring the kvm_arch_get_supported_cpuid() results to be stored in
the class struct for "host" (thus requiring KVM initialization hacks).

Eduardo Habkost (7):
  target-i386: kvm_cpu_fill_host(): Kill unused code
  target-i386: kvm_cpu_fill_host(): No need to check level
  target-i386: kvm_cpu_fill_host(): No need to check CPU vendor
  target-i386: kvm_cpu_fill_host(): No need to check xlevel2
  target-i386: kvm_cpu_fill_host(): Set all feature words at end of
    function
  target-i386: kvm_cpu_fill_host(): Fill feature words in a loop
  target-i386: kvm_check_features_against_host(): Kill feature word
    array

 target-i386/cpu.c | 89 +++++++++++++------------------------------------------
 1 file changed, 20 insertions(+), 69 deletions(-)

-- 
1.8.4.2

[Qemu-devel] [PATCH 1/7] target-i386: kvm_cpu_fill_host(): Kill unused code

[Eduardo Habkost]

Those host_cpuid() calls are useless. They are leftovers from when the
old code using host_cpuid() was removed.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0eea8c7..6f27273 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1178,12 +1178,10 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 
     /* Call Centaur's CPUID instruction. */
     if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
-        host_cpuid(0xC0000000, 0, &eax, &ebx, &ecx, &edx);
         eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
         if (eax >= 0xC0000001) {
             /* Support VIA max extended level */
             x86_cpu_def->xlevel2 = eax;
-            host_cpuid(0xC0000001, 0, &eax, &ebx, &ecx, &edx);
             x86_cpu_def->features[FEAT_C000_0001_EDX] =
                     kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
         }
-- 
1.8.4.2

[Qemu-devel] [PATCH 2/7] target-i386: kvm_cpu_fill_host(): No need to check level

[Eduardo Habkost]

There's no need to check level (CPUID[0].EAX) before calling
kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX), because:

 * The kernel won't return any entry for CPUID 7 if CPUID[0].EAX is < 7
   (See kvm_dev_ioctl_get_supported_cpuid() on the kernel code);
 * kvm_arch_get_supported_cpuid() will return 0 if no entry is returned
   by the kernel for the requested leaf.

This will simplify the kvm_cpu_fill_host() code a little.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 6f27273..aec0107 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1161,12 +1161,8 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     x86_cpu_def->features[FEAT_1_ECX] =
         kvm_arch_get_supported_cpuid(s, 0x1, 0, R_ECX);
 
-    if (x86_cpu_def->level >= 7) {
-        x86_cpu_def->features[FEAT_7_0_EBX] =
-                    kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
-    } else {
-        x86_cpu_def->features[FEAT_7_0_EBX] = 0;
-    }
+    x86_cpu_def->features[FEAT_7_0_EBX] =
+                kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
 
     x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
     x86_cpu_def->features[FEAT_8000_0001_EDX] =
-- 
1.8.4.2

[Qemu-devel] [PATCH 3/7] target-i386: kvm_cpu_fill_host(): No need to check CPU vendor

[Eduardo Habkost]

There's no need to check CPU vendor before calling
kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX), because:

 * The kernel won't return any entry for 0xC0000000 if host CPU vendor
   is not Centaur (See kvm_dev_ioctl_get_supported_cpuid() on the kernel
   code);
 * kvm_arch_get_supported_cpuid() will return 0 if no entry is returned
   by the kernel for the requested leaf.

This will simplify the kvm_cpu_fill_host() code a little.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index aec0107..1a86bcf 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1173,14 +1173,12 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
 
     /* Call Centaur's CPUID instruction. */
-    if (!strcmp(x86_cpu_def->vendor, CPUID_VENDOR_VIA)) {
-        eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
-        if (eax >= 0xC0000001) {
-            /* Support VIA max extended level */
-            x86_cpu_def->xlevel2 = eax;
-            x86_cpu_def->features[FEAT_C000_0001_EDX] =
-                    kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
-        }
+    eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+    if (eax >= 0xC0000001) {
+        /* Support VIA max extended level */
+        x86_cpu_def->xlevel2 = eax;
+        x86_cpu_def->features[FEAT_C000_0001_EDX] =
+                kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
     }
 
     /* Other KVM-specific feature fields: */
-- 
1.8.4.2

[Qemu-devel] [PATCH 4/7] target-i386: kvm_cpu_fill_host(): No need to check xlevel2

[Eduardo Habkost]

There's no need to check CPU xlevel2 before calling
kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX), because:

 * The kernel won't return any entry for 0xC0000000 if host CPU vendor
   is not Centaur (See kvm_dev_ioctl_get_supported_cpuid() on the kernel
   code)
 * Similarly, the kernel won't return any entry for 0xC0000001 if
   CPUID[0xC0000000].EAX is < 0xC0000001
 * kvm_arch_get_supported_cpuid() will return 0 if no entry is returned
   by the kernel for the requested leaf

For similar reasons, we can simply set x86_cpu_def->xlevel2 directly
instead of making it conditional, because it will be set to 0 CPU vendor
is not Centaur.

This will simplify the kvm_cpu_fill_host() code a little.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 1a86bcf..8bc72c2 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1173,13 +1173,11 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
 
     /* Call Centaur's CPUID instruction. */
-    eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
-    if (eax >= 0xC0000001) {
-        /* Support VIA max extended level */
-        x86_cpu_def->xlevel2 = eax;
-        x86_cpu_def->features[FEAT_C000_0001_EDX] =
-                kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
-    }
+    x86_cpu_def->xlevel2 =
+        kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+    /* Support VIA max extended level */
+    x86_cpu_def->features[FEAT_C000_0001_EDX] =
+        kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
 
     /* Other KVM-specific feature fields: */
     x86_cpu_def->features[FEAT_SVM] =
-- 
1.8.4.2

[Qemu-devel] [PATCH 5/7] target-i386: kvm_cpu_fill_host(): Set all feature words at end of function

[Eduardo Habkost]

Reorder the code so all the code that sets x86_cpu_def->features is at
the end of the function.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8bc72c2..8183be0 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1156,30 +1156,24 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
     x86_cpu_def->stepping = eax & 0x0F;
 
     x86_cpu_def->level = kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
+    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
+    x86_cpu_def->xlevel2 =
+        kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
+
+    cpu_x86_fill_model_id(x86_cpu_def->model_id);
+
     x86_cpu_def->features[FEAT_1_EDX] =
         kvm_arch_get_supported_cpuid(s, 0x1, 0, R_EDX);
     x86_cpu_def->features[FEAT_1_ECX] =
         kvm_arch_get_supported_cpuid(s, 0x1, 0, R_ECX);
-
     x86_cpu_def->features[FEAT_7_0_EBX] =
-                kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
-
-    x86_cpu_def->xlevel = kvm_arch_get_supported_cpuid(s, 0x80000000, 0, R_EAX);
+        kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
     x86_cpu_def->features[FEAT_8000_0001_EDX] =
-                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
+        kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
     x86_cpu_def->features[FEAT_8000_0001_ECX] =
-                kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
-
-    cpu_x86_fill_model_id(x86_cpu_def->model_id);
-
-    /* Call Centaur's CPUID instruction. */
-    x86_cpu_def->xlevel2 =
-        kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
-    /* Support VIA max extended level */
+        kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
     x86_cpu_def->features[FEAT_C000_0001_EDX] =
         kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
-
-    /* Other KVM-specific feature fields: */
     x86_cpu_def->features[FEAT_SVM] =
         kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
     x86_cpu_def->features[FEAT_KVM] =
-- 
1.8.4.2

[Qemu-devel] [PATCH 6/7] target-i386: kvm_cpu_fill_host(): Fill feature words in a loop

[Eduardo Habkost]

Now that the kvm_cpu_fill_host() code is simplified, we can simply set
the feature word array using a simple loop.

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

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 8183be0..e76ed1e 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1162,22 +1162,13 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
 
     cpu_x86_fill_model_id(x86_cpu_def->model_id);
 
-    x86_cpu_def->features[FEAT_1_EDX] =
-        kvm_arch_get_supported_cpuid(s, 0x1, 0, R_EDX);
-    x86_cpu_def->features[FEAT_1_ECX] =
-        kvm_arch_get_supported_cpuid(s, 0x1, 0, R_ECX);
-    x86_cpu_def->features[FEAT_7_0_EBX] =
-        kvm_arch_get_supported_cpuid(s, 0x7, 0, R_EBX);
-    x86_cpu_def->features[FEAT_8000_0001_EDX] =
-        kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_EDX);
-    x86_cpu_def->features[FEAT_8000_0001_ECX] =
-        kvm_arch_get_supported_cpuid(s, 0x80000001, 0, R_ECX);
-    x86_cpu_def->features[FEAT_C000_0001_EDX] =
-        kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
-    x86_cpu_def->features[FEAT_SVM] =
-        kvm_arch_get_supported_cpuid(s, 0x8000000A, 0, R_EDX);
-    x86_cpu_def->features[FEAT_KVM] =
-        kvm_arch_get_supported_cpuid(s, KVM_CPUID_FEATURES, 0, R_EAX);
+    FeatureWord w;
+    for (w = 0; w < FEATURE_WORDS; w++) {
+        FeatureWordInfo *wi = &feature_word_info[w];
+        x86_cpu_def->features[w] =
+            kvm_arch_get_supported_cpuid(s, wi->cpuid_eax, wi->cpuid_ecx,
+                                         wi->cpuid_reg);
+    }
 
 #endif /* CONFIG_KVM */
 }
-- 
1.8.4.2

[Qemu-devel] [PATCH 7/7 v2] target-i386: kvm_check_features_against_host(): Kill feature word array

[Eduardo Habkost]

We don't need the ft[] array on kvm_check_features_against_host()
anymore, as we can simply use the feature_word_info[] array, that has
everything we need.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Change v2:
 * Rebase and solve simple conflict with recent changes in the check/enforce code
---
 target-i386/cpu.c | 48 ++++++++++++------------------------------------
 1 file changed, 12 insertions(+), 36 deletions(-)

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index e76ed1e..583adfe 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1197,48 +1197,23 @@ static int unavailable_host_feature(FeatureWordInfo *f, uint32_t mask)
  *
  * This function may be called only if KVM is enabled.
  */
-static int kvm_check_features_against_host(X86CPU *cpu)
+static int kvm_check_features_against_host(KVMState *s, X86CPU *cpu)
 {
     CPUX86State *env = &cpu->env;
-    x86_def_t host_def;
-    uint32_t mask;
-    int rv, i;
-    struct model_features_t ft[] = {
-        {&env->features[FEAT_1_EDX],
-            &host_def.features[FEAT_1_EDX],
-            FEAT_1_EDX },
-        {&env->features[FEAT_1_ECX],
-            &host_def.features[FEAT_1_ECX],
-            FEAT_1_ECX },
-        {&env->features[FEAT_8000_0001_EDX],
-            &host_def.features[FEAT_8000_0001_EDX],
-            FEAT_8000_0001_EDX },
-        {&env->features[FEAT_8000_0001_ECX],
-            &host_def.features[FEAT_8000_0001_ECX],
-            FEAT_8000_0001_ECX },
-        {&env->features[FEAT_C000_0001_EDX],
-            &host_def.features[FEAT_C000_0001_EDX],
-            FEAT_C000_0001_EDX },
-        {&env->features[FEAT_7_0_EBX],
-            &host_def.features[FEAT_7_0_EBX],
-            FEAT_7_0_EBX },
-        {&env->features[FEAT_SVM],
-            &host_def.features[FEAT_SVM],
-            FEAT_SVM },
-        {&env->features[FEAT_KVM],
-            &host_def.features[FEAT_KVM],
-            FEAT_KVM },
-    };
+    int rv = 0;
+    FeatureWord w;
 
     assert(kvm_enabled());
 
-    kvm_cpu_fill_host(&host_def);
-    for (rv = 0, i = 0; i < ARRAY_SIZE(ft); ++i) {
-        FeatureWord w = ft[i].feat_word;
+    for (w = 0; w < FEATURE_WORDS; w++) {
         FeatureWordInfo *wi = &feature_word_info[w];
+        uint32_t guest_feat = env->features[w];
+        uint32_t host_feat = kvm_arch_get_supported_cpuid(s, wi->cpuid_eax,
+                                                             wi->cpuid_ecx,
+                                                             wi->cpuid_reg);
+        uint32_t mask;
         for (mask = 1; mask; mask <<= 1) {
-            if (*ft[i].guest_feat & mask &&
-                !(*ft[i].host_feat & mask)) {
+            if (guest_feat & mask && !(host_feat & mask)) {
                 unavailable_host_feature(wi, mask);
                 rv = 1;
             }
@@ -2556,8 +2531,9 @@ static void x86_cpu_realizefn(DeviceState *dev, Error **errp)
         env->features[FEAT_8000_0001_ECX] &= TCG_EXT3_FEATURES;
         env->features[FEAT_SVM] &= TCG_SVM_FEATURES;
     } else {
+        KVMState *s = kvm_state;
         if ((cpu->check_cpuid || cpu->enforce_cpuid)
-            && kvm_check_features_against_host(cpu) && cpu->enforce_cpuid) {
+            && kvm_check_features_against_host(s, cpu) && cpu->enforce_cpuid) {
             error_setg(&local_err,
                        "Host's CPU doesn't support requested features");
             goto out;
-- 
1.8.4.2

Re: [Qemu-devel] [PATCH 0/7 RESEND] target-i386: Simplify kvm_cpu_fill_host() and kvm_check_features_against_host()

[Eduardo Habkost]

On Mon, Jan 20, 2014 at 02:41:07PM -0200, Eduardo Habkost wrote:
> Resend of series submitted on 24 November 2013, that didn't get any reply. Only
> change is a trivial conflict on patch 7/7.

Question: which tree is the most appropriate to get this in? qom-cpu?
kvm?


> 
> This series simplifies kvm_cpu_fill_host() and
> kvm_check_features_against_host() to simply use FeatureWord & feature_word_info
> loops to fill/check feature words.
> 
> The initial motivation for this was to avoid hacks involving the "host" CPU
> class on the forthcoming conversion of CPU models to be X86CPU subclasses.
> Instead of requiring the kvm_arch_get_supported_cpuid() results to be stored in
> the class struct for "host" (thus requiring KVM initialization hacks).
> 
> Eduardo Habkost (7):
>   target-i386: kvm_cpu_fill_host(): Kill unused code
>   target-i386: kvm_cpu_fill_host(): No need to check level
>   target-i386: kvm_cpu_fill_host(): No need to check CPU vendor
>   target-i386: kvm_cpu_fill_host(): No need to check xlevel2
>   target-i386: kvm_cpu_fill_host(): Set all feature words at end of
>     function
>   target-i386: kvm_cpu_fill_host(): Fill feature words in a loop
>   target-i386: kvm_check_features_against_host(): Kill feature word
>     array
> 
>  target-i386/cpu.c | 89 +++++++++++++------------------------------------------
>  1 file changed, 20 insertions(+), 69 deletions(-)
> 
> -- 
> 1.8.4.2
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/7 RESEND] target-i386: Simplify kvm_cpu_fill_host() and kvm_check_features_against_host()

[Michael S. Tsirkin]

On Mon, Jan 20, 2014 at 04:36:56PM -0200, Eduardo Habkost wrote:
> On Mon, Jan 20, 2014 at 02:41:07PM -0200, Eduardo Habkost wrote:
> > Resend of series submitted on 24 November 2013, that didn't get any reply. Only
> > change is a trivial conflict on patch 7/7.
> 
> Question: which tree is the most appropriate to get this in? qom-cpu?
> kvm?

Either kvm or my pc tree.
Seems unrelated to qom.
Paolo - want to review and take this?

> 
> > 
> > This series simplifies kvm_cpu_fill_host() and
> > kvm_check_features_against_host() to simply use FeatureWord & feature_word_info
> > loops to fill/check feature words.
> > 
> > The initial motivation for this was to avoid hacks involving the "host" CPU
> > class on the forthcoming conversion of CPU models to be X86CPU subclasses.
> > Instead of requiring the kvm_arch_get_supported_cpuid() results to be stored in
> > the class struct for "host" (thus requiring KVM initialization hacks).
> > 
> > Eduardo Habkost (7):
> >   target-i386: kvm_cpu_fill_host(): Kill unused code
> >   target-i386: kvm_cpu_fill_host(): No need to check level
> >   target-i386: kvm_cpu_fill_host(): No need to check CPU vendor
> >   target-i386: kvm_cpu_fill_host(): No need to check xlevel2
> >   target-i386: kvm_cpu_fill_host(): Set all feature words at end of
> >     function
> >   target-i386: kvm_cpu_fill_host(): Fill feature words in a loop
> >   target-i386: kvm_check_features_against_host(): Kill feature word
> >     array
> > 
> >  target-i386/cpu.c | 89 +++++++++++++------------------------------------------
> >  1 file changed, 20 insertions(+), 69 deletions(-)
> > 
> > -- 
> > 1.8.4.2
> > 
> > 
> 
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH 0/7 RESEND] target-i386: Simplify kvm_cpu_fill_host() and kvm_check_features_against_host()

[Andreas Färber]

Am 20.01.2014 21:39, schrieb Michael S. Tsirkin:
> On Mon, Jan 20, 2014 at 04:36:56PM -0200, Eduardo Habkost wrote:
>> On Mon, Jan 20, 2014 at 02:41:07PM -0200, Eduardo Habkost wrote:
>>> Resend of series submitted on 24 November 2013, that didn't get any reply. Only
>>> change is a trivial conflict on patch 7/7.
>>
>> Question: which tree is the most appropriate to get this in? qom-cpu?
>> kvm?
> 
> Either kvm or my pc tree.
> Seems unrelated to qom.

Seems unrelated to PC. ;) I've been maintaining target-i386/cpu.c as
part of my QOM CPU tree according to MAINTAINERS.
I don't mind whether this goes through Paolo's or my tree, but for me to
take KVM related changes, I expect review from the KVM side.

> Paolo - want to review and take this?

Regards,
Andreas

>>>  target-i386/cpu.c | 89 +++++++++++++------------------------------------------
>>>  1 file changed, 20 insertions(+), 69 deletions(-)

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/7] target-i386: kvm_cpu_fill_host(): No need to check xlevel2

[Andreas Färber]

Am 20.01.2014 17:41, schrieb Eduardo Habkost:
> There's no need to check CPU xlevel2 before calling
> kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX), because:
> 
>  * The kernel won't return any entry for 0xC0000000 if host CPU vendor
>    is not Centaur (See kvm_dev_ioctl_get_supported_cpuid() on the kernel
>    code)
>  * Similarly, the kernel won't return any entry for 0xC0000001 if
>    CPUID[0xC0000000].EAX is < 0xC0000001
>  * kvm_arch_get_supported_cpuid() will return 0 if no entry is returned
>    by the kernel for the requested leaf
> 
> For similar reasons, we can simply set x86_cpu_def->xlevel2 directly
> instead of making it conditional, because it will be set to 0 CPU vendor
> is not Centaur.

Commented on the wrong version:

"... if CPU vendor is not ..." I guess. :)

> 
> This will simplify the kvm_cpu_fill_host() code a little.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target-i386/cpu.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index 1a86bcf..8bc72c2 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1173,13 +1173,11 @@ static void kvm_cpu_fill_host(x86_def_t *x86_cpu_def)
>      cpu_x86_fill_model_id(x86_cpu_def->model_id);
>  
>      /* Call Centaur's CPUID instruction. */
> -    eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> -    if (eax >= 0xC0000001) {
> -        /* Support VIA max extended level */
> -        x86_cpu_def->xlevel2 = eax;
> -        x86_cpu_def->features[FEAT_C000_0001_EDX] =
> -                kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
> -    }
> +    x86_cpu_def->xlevel2 =
> +        kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> +    /* Support VIA max extended level */
> +    x86_cpu_def->features[FEAT_C000_0001_EDX] =
> +        kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
>  
>      /* Other KVM-specific feature fields: */
>      x86_cpu_def->features[FEAT_SVM] =

Otherwise looks okay to me.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 0/7 RESEND] target-i386: Simplify kvm_cpu_fill_host() and kvm_check_features_against_host()

[Michael S. Tsirkin]

On Mon, Jan 20, 2014 at 10:07:47PM +0100, Andreas Färber wrote:
> Am 20.01.2014 21:39, schrieb Michael S. Tsirkin:
> > On Mon, Jan 20, 2014 at 04:36:56PM -0200, Eduardo Habkost wrote:
> >> On Mon, Jan 20, 2014 at 02:41:07PM -0200, Eduardo Habkost wrote:
> >>> Resend of series submitted on 24 November 2013, that didn't get any reply. Only
> >>> change is a trivial conflict on patch 7/7.
> >>
> >> Question: which tree is the most appropriate to get this in? qom-cpu?
> >> kvm?
> > 
> > Either kvm or my pc tree.
> > Seems unrelated to qom.
> 
> Seems unrelated to PC. ;) I've been maintaining target-i386/cpu.c as
> part of my QOM CPU tree according to MAINTAINERS.

Ah, right. Cool, the less work for me the better.

> I don't mind whether this goes through Paolo's or my tree, but for me to
> take KVM related changes, I expect review from the KVM side.
> 
> > Paolo - want to review and take this?
> 
> Regards,
> Andreas
> 
> >>>  target-i386/cpu.c | 89 +++++++++++++------------------------------------------
> >>>  1 file changed, 20 insertions(+), 69 deletions(-)
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 0/7 RESEND] target-i386: Simplify kvm_cpu_fill_host() and kvm_check_features_against_host()

[Paolo Bonzini]

Il 20/01/2014 21:39, Michael S. Tsirkin ha scritto:
>>> > > Resend of series submitted on 24 November 2013, that didn't get any reply. Only
>>> > > change is a trivial conflict on patch 7/7.
>> > 
>> > Question: which tree is the most appropriate to get this in? qom-cpu?
>> > kvm?
> Either kvm or my pc tree.
> Seems unrelated to qom.
> Paolo - want to review and take this?
> 

Yup, will include in the next uq/master pull request.

Paolo

Re: [Qemu-devel] [PATCH 0/7 RESEND] target-i386: Simplify kvm_cpu_fill_host() and kvm_check_features_against_host()

[Andreas Färber]

Am 21.01.2014 11:03, schrieb Paolo Bonzini:
> Il 20/01/2014 21:39, Michael S. Tsirkin ha scritto:
>>>>>> Resend of series submitted on 24 November 2013, that didn't get any reply. Only
>>>>>> change is a trivial conflict on patch 7/7.
>>>>
>>>> Question: which tree is the most appropriate to get this in? qom-cpu?
>>>> kvm?
>> Either kvm or my pc tree.
>> Seems unrelated to qom.
>> Paolo - want to review and take this?
>>
> 
> Yup, will include in the next uq/master pull request.

Thanks!

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/7] target-i386: kvm_cpu_fill_host(): No need to check xlevel2

[Paolo Bonzini]

Il 20/01/2014 17:41, Eduardo Habkost ha scritto:
> -    eax = kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> -    if (eax >= 0xC0000001) {
> -        /* Support VIA max extended level */
> -        x86_cpu_def->xlevel2 = eax;
> -        x86_cpu_def->features[FEAT_C000_0001_EDX] =
> -                kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
> -    }
> +    x86_cpu_def->xlevel2 =
> +        kvm_arch_get_supported_cpuid(s, 0xC0000000, 0, R_EAX);
> +    /* Support VIA max extended level */
> +    x86_cpu_def->features[FEAT_C000_0001_EDX] =
> +        kvm_arch_get_supported_cpuid(s, 0xC0000001, 0, R_EDX);
>  

I'm removing this comment before applying the patch (I believe the
comment was not really grammatical, and it meant something like "VIA max
extended level supported").

Paolo