[Qemu-devel] [PATCH 0/4] Fix SVM and KVM features reported on "max" CPU model

[Qemu-devel] [PATCH 0/4] Fix SVM and KVM features reported on "max" CPU model

[Eduardo Habkost]

This series cleans up some of the CPU model initialization code
and fixes one of the causes of the following bug:

  https://bugzilla.redhat.com/show_bug.cgi?id=1467599
  ("Unable to start domain: the CPU is incompatible with host
  CPU: Host CPU does not provide required features: svm")

Eduardo Habkost (4):
  target/i386: Use simple static property for "model-id"
  target/i386: Use host_vendor_fms() in max_x86_cpu_initfn()
  target/i386: Define CPUID_MODEL_ID_SZ macro
  target/i386: Don't use x86_cpu_load_def() on "max" CPU model

 target/i386/cpu.h |  2 +-
 target/i386/cpu.c | 96 ++++++++++++++++++++++++-------------------------------
 2 files changed, 42 insertions(+), 56 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/4] target/i386: Use simple static property for "model-id"

[Eduardo Habkost]

Instead of storing the raw CPUID data in CPUX86State and having
to convert the data back and forth on the QOM getter/setter, use
a simple static string property, and calculate CPUID data inside
cpu_x86_cpuid().

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
 target/i386/cpu.h |  2 +-
 target/i386/cpu.c | 62 ++++++++++++++++---------------------------------------
 2 files changed, 19 insertions(+), 45 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 7a228af..f56a3ea 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1151,7 +1151,7 @@ typedef struct CPUX86State {
     FeatureWordArray features;
     /* Features that were explicitly enabled/disabled */
     FeatureWordArray user_features;
-    uint32_t cpuid_model[12];
+    char *model_id;
 
     /* MTRRs */
     uint64_t mtrr_fixed[11];
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c571772..30b704c 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1828,43 +1828,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
     }
 }
 
-static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-    CPUX86State *env = &cpu->env;
-    char *value;
-    int i;
-
-    value = g_malloc(48 + 1);
-    for (i = 0; i < 48; i++) {
-        value[i] = env->cpuid_model[i >> 2] >> (8 * (i & 3));
-    }
-    value[48] = '\0';
-    return value;
-}
-
-static void x86_cpuid_set_model_id(Object *obj, const char *model_id,
-                                   Error **errp)
-{
-    X86CPU *cpu = X86_CPU(obj);
-    CPUX86State *env = &cpu->env;
-    int c, len, i;
-
-    if (model_id == NULL) {
-        model_id = "";
-    }
-    len = strlen(model_id);
-    memset(env->cpuid_model, 0, 48);
-    for (i = 0; i < 48; i++) {
-        if (i >= len) {
-            c = '\0';
-        } else {
-            c = (uint8_t)model_id[i];
-        }
-        env->cpuid_model[i >> 2] |= c << (8 * (i & 3));
-    }
-}
-
 static void x86_cpuid_get_tsc_freq(Object *obj, Visitor *v, const char *name,
                                    void *opaque, Error **errp)
 {
@@ -2899,10 +2862,23 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
     case 0x80000002:
     case 0x80000003:
     case 0x80000004:
-        *eax = env->cpuid_model[(index - 0x80000002) * 4 + 0];
-        *ebx = env->cpuid_model[(index - 0x80000002) * 4 + 1];
-        *ecx = env->cpuid_model[(index - 0x80000002) * 4 + 2];
-        *edx = env->cpuid_model[(index - 0x80000002) * 4 + 3];
+        {
+            int i;
+            int len = strlen(env->model_id);
+            uint32_t model[4] = { 0, 0, 0, 0 };
+
+            for (i = 0; i < 16; i++) {
+                int p = (index - 0x80000002) * 16 + i;
+                if (p >= len) {
+                    continue;
+                }
+                model[i >> 2] |= ((uint8_t)env->model_id[p]) << (8 * (i & 3));
+            }
+            *eax = model[0];
+            *ebx = model[1];
+            *ecx = model[2];
+            *edx = model[3];
+        }
         break;
     case 0x80000005:
         /* cache info (L1 cache) */
@@ -3868,9 +3844,6 @@ static void x86_cpu_initfn(Object *obj)
     object_property_add_str(obj, "vendor",
                             x86_cpuid_get_vendor,
                             x86_cpuid_set_vendor, NULL);
-    object_property_add_str(obj, "model-id",
-                            x86_cpuid_get_model_id,
-                            x86_cpuid_set_model_id, NULL);
     object_property_add(obj, "tsc-frequency", "int",
                         x86_cpuid_get_tsc_freq,
                         x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
@@ -4004,6 +3977,7 @@ static Property x86_cpu_properties[] = {
     DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
     DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
     DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
+    DEFINE_PROP_STRING("model-id", X86CPU, env.model_id),
     DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, UINT32_MAX),
     DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, UINT32_MAX),
     DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, UINT32_MAX),
-- 
2.9.4

[Qemu-devel] [PATCH 2/4] target/i386: Use host_vendor_fms() in max_x86_cpu_initfn()

[Eduardo Habkost]

The existing code duplicated the logic in host_vendor_fms(), so
reuse the helper function instead.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 30b704c..a667d5d 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1588,13 +1588,8 @@ static void max_x86_cpu_initfn(Object *obj)
         X86CPUDefinition host_cpudef = { };
         uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
 
-        host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
-        x86_cpu_vendor_words2str(host_cpudef.vendor, ebx, edx, ecx);
-
-        host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
-        host_cpudef.family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
-        host_cpudef.model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
-        host_cpudef.stepping = eax & 0x0F;
+        host_vendor_fms(host_cpudef.vendor, &host_cpudef.family,
+                        &host_cpudef.model, &host_cpudef.stepping);
 
         cpu_x86_fill_model_id(host_cpudef.model_id);
 
-- 
2.9.4

[Qemu-devel] [PATCH 3/4] target/i386: Define CPUID_MODEL_ID_SZ macro

[Eduardo Habkost]

Document cpu_x86_fill_model_id() and define CPUID_MODEL_ID_SZ to
help callers use the right buffer size.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a667d5d..e2cd157 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1537,6 +1537,17 @@ static bool lmce_supported(void)
     return !!(mce_cap & MCG_LMCE_P);
 }
 
+#define CPUID_MODEL_ID_SZ 48
+
+/**
+ * cpu_x86_fill_model_id:
+ * Get CPUID model ID string from host CPU.
+ *
+ * @str should have at least CPUID_MODEL_ID_SZ bytes
+ *
+ * The function does NOT add a null terminator to the string
+ * automatically.
+ */
 static int cpu_x86_fill_model_id(char *str)
 {
     uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
-- 
2.9.4

[Qemu-devel] [PATCH 4/4] target/i386: Don't use x86_cpu_load_def() on "max" CPU model

[Eduardo Habkost]

When commit 0bacd8b3046f ('i386: Don't set CPUClass::cpu_def on
"max" model') removed the CPUClass::cpu_def field, we kept using
the x86_cpu_load_def() helper directly in max_x86_cpu_initfn(),
emulating the previous behavior when CPUClass::cpu_def was set.

However, x86_cpu_load_def() is intended to help initialization of
CPU models from the builtin_x86_defs table, and does lots of
other steps that are not necessary for "max".

One of the things x86_cpu_load_def() do is to set the properties
listed at tcg_default_props/kvm_default_props.  We must not do
that on the "max" CPU model, otherwise under KVM we will
incorrectly report all KVM features as always available, and the
"svm" feature as always unavailable.  The latter caused the bug
reported at:

  https://bugzilla.redhat.com/show_bug.cgi?id=1467599
  ("Unable to start domain: the CPU is incompatible with host CPU:
  Host CPU does not provide required features: svm")

Replace x86_cpu_load_def() with simple object_property_set*()
calls.  In addition to fixing the above bug, this makes the KVM
branch in max_x86_cpu_initfn() very similar to the existing TCG
branch.

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index e2cd157..62d8021 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1596,15 +1596,21 @@ static void max_x86_cpu_initfn(Object *obj)
     cpu->max_features = true;
 
     if (kvm_enabled()) {
-        X86CPUDefinition host_cpudef = { };
-        uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
+        char vendor[CPUID_VENDOR_SZ + 1] = { 0 };
+        char model_id[CPUID_MODEL_ID_SZ + 1] = { 0 };
+        int family, model, stepping;
 
-        host_vendor_fms(host_cpudef.vendor, &host_cpudef.family,
-                        &host_cpudef.model, &host_cpudef.stepping);
+        host_vendor_fms(vendor, &family, &model, &stepping);
 
-        cpu_x86_fill_model_id(host_cpudef.model_id);
+        cpu_x86_fill_model_id(model_id);
 
-        x86_cpu_load_def(cpu, &host_cpudef, &error_abort);
+        object_property_set_str(OBJECT(cpu), vendor, "vendor", &error_abort);
+        object_property_set_int(OBJECT(cpu), family, "family", &error_abort);
+        object_property_set_int(OBJECT(cpu), model, "model", &error_abort);
+        object_property_set_int(OBJECT(cpu), stepping, "stepping",
+                                &error_abort);
+        object_property_set_str(OBJECT(cpu), model_id, "model-id",
+                                &error_abort);
 
         env->cpuid_min_level =
             kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
-- 
2.9.4

Re: [Qemu-devel] [PATCH 1/4] target/i386: Use simple static property for "model-id"

[Igor Mammedov]

On Wed, 12 Jul 2017 13:20:55 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> Instead of storing the raw CPUID data in CPUX86State and having
> to convert the data back and forth on the QOM getter/setter, use
> a simple static string property, and calculate CPUID data inside
> cpu_x86_cpuid().
wouldn't this slow down 'cpuid' instruction execution,
especially in tcg mode?

Why do you need this patch except of nice code reduction it gives?

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target/i386/cpu.h |  2 +-
>  target/i386/cpu.c | 62 ++++++++++++++++---------------------------------------
>  2 files changed, 19 insertions(+), 45 deletions(-)
> 
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 7a228af..f56a3ea 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1151,7 +1151,7 @@ typedef struct CPUX86State {
>      FeatureWordArray features;
>      /* Features that were explicitly enabled/disabled */
>      FeatureWordArray user_features;
> -    uint32_t cpuid_model[12];
> +    char *model_id;
>  
>      /* MTRRs */
>      uint64_t mtrr_fixed[11];
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index c571772..30b704c 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1828,43 +1828,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
>      }
>  }
>  
> -static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
> -{
> -    X86CPU *cpu = X86_CPU(obj);
> -    CPUX86State *env = &cpu->env;
> -    char *value;
> -    int i;
> -
> -    value = g_malloc(48 + 1);
> -    for (i = 0; i < 48; i++) {
> -        value[i] = env->cpuid_model[i >> 2] >> (8 * (i & 3));
> -    }
> -    value[48] = '\0';
> -    return value;
> -}
> -
> -static void x86_cpuid_set_model_id(Object *obj, const char *model_id,
> -                                   Error **errp)
> -{
> -    X86CPU *cpu = X86_CPU(obj);
> -    CPUX86State *env = &cpu->env;
> -    int c, len, i;
> -
> -    if (model_id == NULL) {
> -        model_id = "";
> -    }
> -    len = strlen(model_id);
> -    memset(env->cpuid_model, 0, 48);
> -    for (i = 0; i < 48; i++) {
> -        if (i >= len) {
> -            c = '\0';
> -        } else {
> -            c = (uint8_t)model_id[i];
> -        }
> -        env->cpuid_model[i >> 2] |= c << (8 * (i & 3));
> -    }
> -}
> -
>  static void x86_cpuid_get_tsc_freq(Object *obj, Visitor *v, const char *name,
>                                     void *opaque, Error **errp)
>  {
> @@ -2899,10 +2862,23 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>      case 0x80000002:
>      case 0x80000003:
>      case 0x80000004:
> -        *eax = env->cpuid_model[(index - 0x80000002) * 4 + 0];
> -        *ebx = env->cpuid_model[(index - 0x80000002) * 4 + 1];
> -        *ecx = env->cpuid_model[(index - 0x80000002) * 4 + 2];
> -        *edx = env->cpuid_model[(index - 0x80000002) * 4 + 3];
> +        {
> +            int i;
> +            int len = strlen(env->model_id);
> +            uint32_t model[4] = { 0, 0, 0, 0 };
> +
> +            for (i = 0; i < 16; i++) {
> +                int p = (index - 0x80000002) * 16 + i;
> +                if (p >= len) {
> +                    continue;
> +                }
> +                model[i >> 2] |= ((uint8_t)env->model_id[p]) << (8 * (i & 3));
> +            }
> +            *eax = model[0];
> +            *ebx = model[1];
> +            *ecx = model[2];
> +            *edx = model[3];
> +        }
>          break;
>      case 0x80000005:
>          /* cache info (L1 cache) */
> @@ -3868,9 +3844,6 @@ static void x86_cpu_initfn(Object *obj)
>      object_property_add_str(obj, "vendor",
>                              x86_cpuid_get_vendor,
>                              x86_cpuid_set_vendor, NULL);
> -    object_property_add_str(obj, "model-id",
> -                            x86_cpuid_get_model_id,
> -                            x86_cpuid_set_model_id, NULL);
>      object_property_add(obj, "tsc-frequency", "int",
>                          x86_cpuid_get_tsc_freq,
>                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> @@ -4004,6 +3977,7 @@ static Property x86_cpu_properties[] = {
>      DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
>      DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
>      DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
> +    DEFINE_PROP_STRING("model-id", X86CPU, env.model_id),
>      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, UINT32_MAX),
>      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, UINT32_MAX),
>      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, UINT32_MAX),

Re: [Qemu-devel] [PATCH 1/4] target/i386: Use simple static property for "model-id"

[Eduardo Habkost]

On Mon, Jul 17, 2017 at 02:03:43PM +0200, Igor Mammedov wrote:
> On Wed, 12 Jul 2017 13:20:55 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > Instead of storing the raw CPUID data in CPUX86State and having
> > to convert the data back and forth on the QOM getter/setter, use
> > a simple static string property, and calculate CPUID data inside
> > cpu_x86_cpuid().
> wouldn't this slow down 'cpuid' instruction execution,
> especially in tcg mode?

It may add a few additional CPU cycles, but I really doubt we can
find a workload where CPUID speed has measurable impact.  See,
for example, how expensive the kernel KVM CPUID code
(kvm_cpuid(), kvm_find_cpuid_entry()) is.

Richard, what do you think?


> 
> Why do you need this patch except of nice code reduction it gives?

It's just for code reduction, and it is not needed for this
series, if I recall correctly.  It can be included later, if
necessary.

> 
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target/i386/cpu.h |  2 +-
> >  target/i386/cpu.c | 62 ++++++++++++++++---------------------------------------
> >  2 files changed, 19 insertions(+), 45 deletions(-)
> > 
> > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > index 7a228af..f56a3ea 100644
> > --- a/target/i386/cpu.h
> > +++ b/target/i386/cpu.h
> > @@ -1151,7 +1151,7 @@ typedef struct CPUX86State {
> >      FeatureWordArray features;
> >      /* Features that were explicitly enabled/disabled */
> >      FeatureWordArray user_features;
> > -    uint32_t cpuid_model[12];
> > +    char *model_id;
> >  
> >      /* MTRRs */
> >      uint64_t mtrr_fixed[11];
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index c571772..30b704c 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -1828,43 +1828,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
> >      }
> >  }
> >  
> > -static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
> > -{
> > -    X86CPU *cpu = X86_CPU(obj);
> > -    CPUX86State *env = &cpu->env;
> > -    char *value;
> > -    int i;
> > -
> > -    value = g_malloc(48 + 1);
> > -    for (i = 0; i < 48; i++) {
> > -        value[i] = env->cpuid_model[i >> 2] >> (8 * (i & 3));
> > -    }
> > -    value[48] = '\0';
> > -    return value;
> > -}
> > -
> > -static void x86_cpuid_set_model_id(Object *obj, const char *model_id,
> > -                                   Error **errp)
> > -{
> > -    X86CPU *cpu = X86_CPU(obj);
> > -    CPUX86State *env = &cpu->env;
> > -    int c, len, i;
> > -
> > -    if (model_id == NULL) {
> > -        model_id = "";
> > -    }
> > -    len = strlen(model_id);
> > -    memset(env->cpuid_model, 0, 48);
> > -    for (i = 0; i < 48; i++) {
> > -        if (i >= len) {
> > -            c = '\0';
> > -        } else {
> > -            c = (uint8_t)model_id[i];
> > -        }
> > -        env->cpuid_model[i >> 2] |= c << (8 * (i & 3));
> > -    }
> > -}
> > -
> >  static void x86_cpuid_get_tsc_freq(Object *obj, Visitor *v, const char *name,
> >                                     void *opaque, Error **errp)
> >  {
> > @@ -2899,10 +2862,23 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> >      case 0x80000002:
> >      case 0x80000003:
> >      case 0x80000004:
> > -        *eax = env->cpuid_model[(index - 0x80000002) * 4 + 0];
> > -        *ebx = env->cpuid_model[(index - 0x80000002) * 4 + 1];
> > -        *ecx = env->cpuid_model[(index - 0x80000002) * 4 + 2];
> > -        *edx = env->cpuid_model[(index - 0x80000002) * 4 + 3];
> > +        {
> > +            int i;
> > +            int len = strlen(env->model_id);
> > +            uint32_t model[4] = { 0, 0, 0, 0 };
> > +
> > +            for (i = 0; i < 16; i++) {
> > +                int p = (index - 0x80000002) * 16 + i;
> > +                if (p >= len) {
> > +                    continue;
> > +                }
> > +                model[i >> 2] |= ((uint8_t)env->model_id[p]) << (8 * (i & 3));
> > +            }
> > +            *eax = model[0];
> > +            *ebx = model[1];
> > +            *ecx = model[2];
> > +            *edx = model[3];
> > +        }
> >          break;
> >      case 0x80000005:
> >          /* cache info (L1 cache) */
> > @@ -3868,9 +3844,6 @@ static void x86_cpu_initfn(Object *obj)
> >      object_property_add_str(obj, "vendor",
> >                              x86_cpuid_get_vendor,
> >                              x86_cpuid_set_vendor, NULL);
> > -    object_property_add_str(obj, "model-id",
> > -                            x86_cpuid_get_model_id,
> > -                            x86_cpuid_set_model_id, NULL);
> >      object_property_add(obj, "tsc-frequency", "int",
> >                          x86_cpuid_get_tsc_freq,
> >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > @@ -4004,6 +3977,7 @@ static Property x86_cpu_properties[] = {
> >      DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
> >      DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
> >      DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
> > +    DEFINE_PROP_STRING("model-id", X86CPU, env.model_id),
> >      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, UINT32_MAX),
> >      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, UINT32_MAX),
> >      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, UINT32_MAX),
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/4] target/i386: Use simple static property for "model-id"

[Igor Mammedov]

On Mon, 17 Jul 2017 14:18:27 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jul 17, 2017 at 02:03:43PM +0200, Igor Mammedov wrote:
> > On Wed, 12 Jul 2017 13:20:55 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> >   
> > > Instead of storing the raw CPUID data in CPUX86State and having
> > > to convert the data back and forth on the QOM getter/setter, use
> > > a simple static string property, and calculate CPUID data inside
> > > cpu_x86_cpuid().  
> > wouldn't this slow down 'cpuid' instruction execution,
> > especially in tcg mode?  
> 
> It may add a few additional CPU cycles, but I really doubt we can
> find a workload where CPUID speed has measurable impact.  See,
> for example, how expensive the kernel KVM CPUID code
> (kvm_cpuid(), kvm_find_cpuid_entry()) is.
I don't expect that it would affect KVM, but for TCG any instruction
execution is 'fast' path, so I'd leave current cpu_x86_cpuid()
not to loose those few cycles, it's not worth sacrifice for the sake of cleanup.

> 
> Richard, what do you think?
> 
> 
> > 
> > Why do you need this patch except of nice code reduction it gives?  
> 
> It's just for code reduction, and it is not needed for this
> series, if I recall correctly.  It can be included later, if
> necessary.
> 
> >   
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  target/i386/cpu.h |  2 +-
> > >  target/i386/cpu.c | 62 ++++++++++++++++---------------------------------------
> > >  2 files changed, 19 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> > > index 7a228af..f56a3ea 100644
> > > --- a/target/i386/cpu.h
> > > +++ b/target/i386/cpu.h
> > > @@ -1151,7 +1151,7 @@ typedef struct CPUX86State {
> > >      FeatureWordArray features;
> > >      /* Features that were explicitly enabled/disabled */
> > >      FeatureWordArray user_features;
> > > -    uint32_t cpuid_model[12];
> > > +    char *model_id;
> > >  
> > >      /* MTRRs */
> > >      uint64_t mtrr_fixed[11];
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index c571772..30b704c 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -1828,43 +1828,6 @@ static void x86_cpuid_set_vendor(Object *obj, const char *value,
> > >      }
> > >  }
> > >  
> > > -static char *x86_cpuid_get_model_id(Object *obj, Error **errp)
> > > -{
> > > -    X86CPU *cpu = X86_CPU(obj);
> > > -    CPUX86State *env = &cpu->env;
> > > -    char *value;
> > > -    int i;
> > > -
> > > -    value = g_malloc(48 + 1);
> > > -    for (i = 0; i < 48; i++) {
> > > -        value[i] = env->cpuid_model[i >> 2] >> (8 * (i & 3));
> > > -    }
> > > -    value[48] = '\0';
> > > -    return value;
> > > -}
> > > -
> > > -static void x86_cpuid_set_model_id(Object *obj, const char *model_id,
> > > -                                   Error **errp)
> > > -{
> > > -    X86CPU *cpu = X86_CPU(obj);
> > > -    CPUX86State *env = &cpu->env;
> > > -    int c, len, i;
> > > -
> > > -    if (model_id == NULL) {
> > > -        model_id = "";
> > > -    }
> > > -    len = strlen(model_id);
> > > -    memset(env->cpuid_model, 0, 48);
> > > -    for (i = 0; i < 48; i++) {
> > > -        if (i >= len) {
> > > -            c = '\0';
> > > -        } else {
> > > -            c = (uint8_t)model_id[i];
> > > -        }
> > > -        env->cpuid_model[i >> 2] |= c << (8 * (i & 3));
> > > -    }
> > > -}
> > > -
> > >  static void x86_cpuid_get_tsc_freq(Object *obj, Visitor *v, const char *name,
> > >                                     void *opaque, Error **errp)
> > >  {
> > > @@ -2899,10 +2862,23 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
> > >      case 0x80000002:
> > >      case 0x80000003:
> > >      case 0x80000004:
> > > -        *eax = env->cpuid_model[(index - 0x80000002) * 4 + 0];
> > > -        *ebx = env->cpuid_model[(index - 0x80000002) * 4 + 1];
> > > -        *ecx = env->cpuid_model[(index - 0x80000002) * 4 + 2];
> > > -        *edx = env->cpuid_model[(index - 0x80000002) * 4 + 3];
> > > +        {
> > > +            int i;
> > > +            int len = strlen(env->model_id);
> > > +            uint32_t model[4] = { 0, 0, 0, 0 };
> > > +
> > > +            for (i = 0; i < 16; i++) {
> > > +                int p = (index - 0x80000002) * 16 + i;
> > > +                if (p >= len) {
> > > +                    continue;
> > > +                }
> > > +                model[i >> 2] |= ((uint8_t)env->model_id[p]) << (8 * (i & 3));
> > > +            }
> > > +            *eax = model[0];
> > > +            *ebx = model[1];
> > > +            *ecx = model[2];
> > > +            *edx = model[3];
> > > +        }
> > >          break;
> > >      case 0x80000005:
> > >          /* cache info (L1 cache) */
> > > @@ -3868,9 +3844,6 @@ static void x86_cpu_initfn(Object *obj)
> > >      object_property_add_str(obj, "vendor",
> > >                              x86_cpuid_get_vendor,
> > >                              x86_cpuid_set_vendor, NULL);
> > > -    object_property_add_str(obj, "model-id",
> > > -                            x86_cpuid_get_model_id,
> > > -                            x86_cpuid_set_model_id, NULL);
> > >      object_property_add(obj, "tsc-frequency", "int",
> > >                          x86_cpuid_get_tsc_freq,
> > >                          x86_cpuid_set_tsc_freq, NULL, NULL, NULL);
> > > @@ -4004,6 +3977,7 @@ static Property x86_cpu_properties[] = {
> > >      DEFINE_PROP_UINT32("phys-bits", X86CPU, phys_bits, 0),
> > >      DEFINE_PROP_BOOL("host-phys-bits", X86CPU, host_phys_bits, false),
> > >      DEFINE_PROP_BOOL("fill-mtrr-mask", X86CPU, fill_mtrr_mask, true),
> > > +    DEFINE_PROP_STRING("model-id", X86CPU, env.model_id),
> > >      DEFINE_PROP_UINT32("level", X86CPU, env.cpuid_level, UINT32_MAX),
> > >      DEFINE_PROP_UINT32("xlevel", X86CPU, env.cpuid_xlevel, UINT32_MAX),
> > >      DEFINE_PROP_UINT32("xlevel2", X86CPU, env.cpuid_xlevel2, UINT32_MAX),  
> >   
> 

Re: [Qemu-devel] [PATCH 2/4] target/i386: Use host_vendor_fms() in max_x86_cpu_initfn()

[Igor Mammedov]

On Wed, 12 Jul 2017 13:20:56 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> The existing code duplicated the logic in host_vendor_fms(), so
> reuse the helper function instead.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target/i386/cpu.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 30b704c..a667d5d 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1588,13 +1588,8 @@ static void max_x86_cpu_initfn(Object *obj)
>          X86CPUDefinition host_cpudef = { };
>          uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
>  
> -        host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
> -        x86_cpu_vendor_words2str(host_cpudef.vendor, ebx, edx, ecx);
> -
> -        host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
> -        host_cpudef.family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
> -        host_cpudef.model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
> -        host_cpudef.stepping = eax & 0x0F;
> +        host_vendor_fms(host_cpudef.vendor, &host_cpudef.family,
> +                        &host_cpudef.model, &host_cpudef.stepping);
>  
>          cpu_x86_fill_model_id(host_cpudef.model_id);
>  

Re: [Qemu-devel] [PATCH 4/4] target/i386: Don't use x86_cpu_load_def() on "max" CPU model

[Igor Mammedov]

On Wed, 12 Jul 2017 13:20:58 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> When commit 0bacd8b3046f ('i386: Don't set CPUClass::cpu_def on
> "max" model') removed the CPUClass::cpu_def field, we kept using
> the x86_cpu_load_def() helper directly in max_x86_cpu_initfn(),
> emulating the previous behavior when CPUClass::cpu_def was set.
> 
> However, x86_cpu_load_def() is intended to help initialization of
> CPU models from the builtin_x86_defs table, and does lots of
> other steps that are not necessary for "max".
> 
> One of the things x86_cpu_load_def() do is to set the properties
> listed at tcg_default_props/kvm_default_props.  We must not do
> that on the "max" CPU model, otherwise under KVM we will
> incorrectly report all KVM features as always available, and the
> "svm" feature as always unavailable.  The latter caused the bug
> reported at:
Maybe add that all available features for 'max' are set later at realize() time
to ones actually supported by host.

Also while looking at it, I've noticed that:
x86_cpu_load_def()
  ...
      if (kvm_enabled()) {                                                         
        if (!kvm_irqchip_in_kernel()) {                                          
            x86_cpu_change_kvm_default("x2apic", "off");                         
        }

and

kvm_arch_get_supported_cpuid() also having
        if (!kvm_irqchip_in_kernel()) {                                          
            ret &= ~CPUID_EXT_X2APIC;                                            
        } 

so do we really need this duplication in x86_cpu_load_def()?

> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=1467599
>   ("Unable to start domain: the CPU is incompatible with host CPU:
>   Host CPU does not provide required features: svm")
> 
> Replace x86_cpu_load_def() with simple object_property_set*()
> calls.  In addition to fixing the above bug, this makes the KVM
> branch in max_x86_cpu_initfn() very similar to the existing TCG
> branch.
> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
>  target/i386/cpu.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index e2cd157..62d8021 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1596,15 +1596,21 @@ static void max_x86_cpu_initfn(Object *obj)
>      cpu->max_features = true;
>  
>      if (kvm_enabled()) {
> -        X86CPUDefinition host_cpudef = { };
> -        uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> +        char vendor[CPUID_VENDOR_SZ + 1] = { 0 };
> +        char model_id[CPUID_MODEL_ID_SZ + 1] = { 0 };
> +        int family, model, stepping;
>  
> -        host_vendor_fms(host_cpudef.vendor, &host_cpudef.family,
> -                        &host_cpudef.model, &host_cpudef.stepping);
> +        host_vendor_fms(vendor, &family, &model, &stepping);
>  
> -        cpu_x86_fill_model_id(host_cpudef.model_id);
> +        cpu_x86_fill_model_id(model_id);
>  
> -        x86_cpu_load_def(cpu, &host_cpudef, &error_abort);
this looses 
   env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
but it looks like kvm_arch_get_supported_cpuid() will take care of it later
at realize() time.

> +        object_property_set_str(OBJECT(cpu), vendor, "vendor", &error_abort);
> +        object_property_set_int(OBJECT(cpu), family, "family", &error_abort);
> +        object_property_set_int(OBJECT(cpu), model, "model", &error_abort);
> +        object_property_set_int(OBJECT(cpu), stepping, "stepping",
> +                                &error_abort);
> +        object_property_set_str(OBJECT(cpu), model_id, "model-id",
> +                                &error_abort);
>  
>          env->cpuid_min_level =
>              kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);

Re: [Qemu-devel] [PATCH 4/4] target/i386: Don't use x86_cpu_load_def() on "max" CPU model

[Eduardo Habkost]

On Tue, Jul 18, 2017 at 03:27:26PM +0200, Igor Mammedov wrote:
> On Wed, 12 Jul 2017 13:20:58 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > When commit 0bacd8b3046f ('i386: Don't set CPUClass::cpu_def on
> > "max" model') removed the CPUClass::cpu_def field, we kept using
> > the x86_cpu_load_def() helper directly in max_x86_cpu_initfn(),
> > emulating the previous behavior when CPUClass::cpu_def was set.
> > 
> > However, x86_cpu_load_def() is intended to help initialization of
> > CPU models from the builtin_x86_defs table, and does lots of
> > other steps that are not necessary for "max".
> > 
> > One of the things x86_cpu_load_def() do is to set the properties
> > listed at tcg_default_props/kvm_default_props.  We must not do
> > that on the "max" CPU model, otherwise under KVM we will
> > incorrectly report all KVM features as always available, and the
> > "svm" feature as always unavailable.  The latter caused the bug
> > reported at:
> Maybe add that all available features for 'max' are set later at realize() time
> to ones actually supported by host.

I can add the following paragraph to the commit message.  Is it
enough to get your Reviewed-by?

    target/i386: Don't use x86_cpu_load_def() on "max" CPU model
    
    When commit 0bacd8b3046f ('i386: Don't set CPUClass::cpu_def on
    "max" model') removed the CPUClass::cpu_def field, we kept using
    the x86_cpu_load_def() helper directly in max_x86_cpu_initfn()
    because it allowed us to reuse the old max_x86_cpu_class_init()
    code.
    
    However, the only X86CPUDefinition fields we really use are
    vendor/family/model/stepping/model-id, and x86_cpu_load_def()
    tries to do other stuff that is only necessary when using named
    CPU models from builtin_x86_defs.
    
    One of the things x86_cpu_load_def() do is to set the properties
    listed at kvm_default_props.  We must not do that on "max" and
    "host" CPU models, otherwise we will incorrectly report all KVM
    features as always available, and incorrectly report the "svm"
    feature as always unavailable under KVM.  The latter caused the
    bug reported at:
    
      https://bugzilla.redhat.com/show_bug.cgi?id=1467599
      ("Unable to start domain: the CPU is incompatible with host CPU:
      Host CPU does not provide required features: svm")

    Replace x86_cpu_load_def() with simple object_property_set*()
    calls.  In addition to fixing the above bug, this makes the KVM
    code very similar to the TCG code inside max_x86_cpu_initfn().
    
+   For reference, the full list of steps performed by
+   x86_cpu_load_def() is:
+   
+   * Setting min-level and min-xlevel.  Already done by
+     max_x86_cpu_initfn().
+   * Setting family/model/stepping/model-id.  Done by the code added
+     to max_x86_cpu_initfn() in this patch.
+   * Copying def->features.  Wrong because "-cpu max" features need to
+     be calculated at realize time.  This was not a problem in the
+     current code because host_cpudef.features was all zeroes.
+   * x86_cpu_apply_props() calls.  This causes the bug above, and
+     shouldn't be done.
+   * Setting CPUID_EXT_HYPERVISOR.  Not needed because it is already
+     reported by x86_cpu_get_supported_feature_word(), and because
+     "-cpu max" features need to be calculated at realize time.
+   * Setting CPU vendor to host CPU vendor if on KVM mode.
+     Redundant, because max_x86_cpu_initfn() already sets it to the
+     host CPU vendor.
+
    Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

> 
> Also while looking at it, I've noticed that:
> x86_cpu_load_def()
>   ...
>       if (kvm_enabled()) {                                                         
>         if (!kvm_irqchip_in_kernel()) {                                          
>             x86_cpu_change_kvm_default("x2apic", "off");                         
>         }
> 
> and
> 
> kvm_arch_get_supported_cpuid() also having
>         if (!kvm_irqchip_in_kernel()) {                                          
>             ret &= ~CPUID_EXT_X2APIC;                                            
>         } 
> 
> so do we really need this duplication in x86_cpu_load_def()?

Those two pieces of code represent two different rules:

The first encodes the fact that we won't try to enable x2apic by
default if !kvm_irqchip_in_kernel().  It is required so we don't
get spurious "feature x2apic is unavailable" warnings (or fatal
errors if in enforce mode).

The second encodes the fact that we can't enable x2apic if
!kvm_irqchip_in_kernel().  It is required so we block the user
from enabling x2apic manually on the command-line.

It's true that the first rule is a direct consequence of the
second rule.  We could figure out a mechanism to make the code
for the second rule trigger the first rule automatically, but I'm
not sure it would be worth the extra complexity.  (And it's out
of the scope of this patch).

> 
> > 
> >   https://bugzilla.redhat.com/show_bug.cgi?id=1467599
> >   ("Unable to start domain: the CPU is incompatible with host CPU:
> >   Host CPU does not provide required features: svm")
> > 
> > Replace x86_cpu_load_def() with simple object_property_set*()
> > calls.  In addition to fixing the above bug, this makes the KVM
> > branch in max_x86_cpu_initfn() very similar to the existing TCG
> > branch.
> > 
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > ---
> >  target/i386/cpu.c | 18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> > 
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index e2cd157..62d8021 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -1596,15 +1596,21 @@ static void max_x86_cpu_initfn(Object *obj)
> >      cpu->max_features = true;
> >  
> >      if (kvm_enabled()) {
> > -        X86CPUDefinition host_cpudef = { };
> > -        uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> > +        char vendor[CPUID_VENDOR_SZ + 1] = { 0 };
> > +        char model_id[CPUID_MODEL_ID_SZ + 1] = { 0 };
> > +        int family, model, stepping;
> >  
> > -        host_vendor_fms(host_cpudef.vendor, &host_cpudef.family,
> > -                        &host_cpudef.model, &host_cpudef.stepping);
> > +        host_vendor_fms(vendor, &family, &model, &stepping);
> >  
> > -        cpu_x86_fill_model_id(host_cpudef.model_id);
> > +        cpu_x86_fill_model_id(model_id);
> >  
> > -        x86_cpu_load_def(cpu, &host_cpudef, &error_abort);
> this looses 
>    env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
> but it looks like kvm_arch_get_supported_cpuid() will take care of it later
> at realize() time.

Yes, kvm_arch_get_supported_cpuid() already sets
CPUID_EXT_HYPERVISOR, and on -cpu host/max, we only care about
kvm_arch_get_supported_cpuid() (for KVM) or
FeatureWord::tcg_features (for TCG).

This is similar to the x2apic case: x86_cpu_load_def() encodes
the fact that CPUID_EXT_HYPERVISOR is enabled by default (on the
predefined CPU models).  kvm_arch_get_supported_cpuid() (and
FeatureWord::tcg_features) encodes the fact that we _can_ enable
CPUID_EXT_HYPERVISOR.

> 
> > +        object_property_set_str(OBJECT(cpu), vendor, "vendor", &error_abort);
> > +        object_property_set_int(OBJECT(cpu), family, "family", &error_abort);
> > +        object_property_set_int(OBJECT(cpu), model, "model", &error_abort);
> > +        object_property_set_int(OBJECT(cpu), stepping, "stepping",
> > +                                &error_abort);
> > +        object_property_set_str(OBJECT(cpu), model_id, "model-id",
> > +                                &error_abort);
> >  
> >          env->cpuid_min_level =
> >              kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/4] target/i386: Don't use x86_cpu_load_def() on "max" CPU model

[Igor Mammedov]

On Tue, 18 Jul 2017 21:02:06 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jul 18, 2017 at 03:27:26PM +0200, Igor Mammedov wrote:
> > On Wed, 12 Jul 2017 13:20:58 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:
> > 
> > > When commit 0bacd8b3046f ('i386: Don't set CPUClass::cpu_def on
> > > "max" model') removed the CPUClass::cpu_def field, we kept using
> > > the x86_cpu_load_def() helper directly in max_x86_cpu_initfn(),
> > > emulating the previous behavior when CPUClass::cpu_def was set.
> > > 
> > > However, x86_cpu_load_def() is intended to help initialization of
> > > CPU models from the builtin_x86_defs table, and does lots of
> > > other steps that are not necessary for "max".
> > > 
> > > One of the things x86_cpu_load_def() do is to set the properties
> > > listed at tcg_default_props/kvm_default_props.  We must not do
> > > that on the "max" CPU model, otherwise under KVM we will
> > > incorrectly report all KVM features as always available, and the
> > > "svm" feature as always unavailable.  The latter caused the bug
> > > reported at:
> > Maybe add that all available features for 'max' are set later at realize() time
> > to ones actually supported by host.
> 
> I can add the following paragraph to the commit message.  Is it
> enough to get your Reviewed-by?
> 
>     target/i386: Don't use x86_cpu_load_def() on "max" CPU model
>     
>     When commit 0bacd8b3046f ('i386: Don't set CPUClass::cpu_def on
>     "max" model') removed the CPUClass::cpu_def field, we kept using
>     the x86_cpu_load_def() helper directly in max_x86_cpu_initfn()
>     because it allowed us to reuse the old max_x86_cpu_class_init()
>     code.
>     
>     However, the only X86CPUDefinition fields we really use are
>     vendor/family/model/stepping/model-id, and x86_cpu_load_def()
>     tries to do other stuff that is only necessary when using named
>     CPU models from builtin_x86_defs.
>     
>     One of the things x86_cpu_load_def() do is to set the properties
>     listed at kvm_default_props.  We must not do that on "max" and
>     "host" CPU models, otherwise we will incorrectly report all KVM
>     features as always available, and incorrectly report the "svm"
>     feature as always unavailable under KVM.  The latter caused the
>     bug reported at:
>     
>       https://bugzilla.redhat.com/show_bug.cgi?id=1467599
>       ("Unable to start domain: the CPU is incompatible with host CPU:
>       Host CPU does not provide required features: svm")
> 
>     Replace x86_cpu_load_def() with simple object_property_set*()
>     calls.  In addition to fixing the above bug, this makes the KVM
>     code very similar to the TCG code inside max_x86_cpu_initfn().
>     
> +   For reference, the full list of steps performed by
> +   x86_cpu_load_def() is:
> +   
> +   * Setting min-level and min-xlevel.  Already done by
> +     max_x86_cpu_initfn().
> +   * Setting family/model/stepping/model-id.  Done by the code added
> +     to max_x86_cpu_initfn() in this patch.
> +   * Copying def->features.  Wrong because "-cpu max" features need to
> +     be calculated at realize time.  This was not a problem in the
> +     current code because host_cpudef.features was all zeroes.
> +   * x86_cpu_apply_props() calls.  This causes the bug above, and
> +     shouldn't be done.
> +   * Setting CPUID_EXT_HYPERVISOR.  Not needed because it is already
> +     reported by x86_cpu_get_supported_feature_word(), and because
> +     "-cpu max" features need to be calculated at realize time.
> +   * Setting CPU vendor to host CPU vendor if on KVM mode.
> +     Redundant, because max_x86_cpu_initfn() already sets it to the
> +     host CPU vendor.
> +
>     Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> 
> > 
> > Also while looking at it, I've noticed that:
> > x86_cpu_load_def()
> >   ...
> >       if (kvm_enabled()) {                                                         
> >         if (!kvm_irqchip_in_kernel()) {                                          
> >             x86_cpu_change_kvm_default("x2apic", "off");                         
> >         }
> > 
> > and
> > 
> > kvm_arch_get_supported_cpuid() also having
> >         if (!kvm_irqchip_in_kernel()) {                                          
> >             ret &= ~CPUID_EXT_X2APIC;                                            
> >         } 
> > 
> > so do we really need this duplication in x86_cpu_load_def()?
> 
> Those two pieces of code represent two different rules:
> 
> The first encodes the fact that we won't try to enable x2apic by
> default if !kvm_irqchip_in_kernel().  It is required so we don't
> get spurious "feature x2apic is unavailable" warnings (or fatal
> errors if in enforce mode).
> 
> The second encodes the fact that we can't enable x2apic if
> !kvm_irqchip_in_kernel().  It is required so we block the user
> from enabling x2apic manually on the command-line.
> 
> It's true that the first rule is a direct consequence of the
> second rule.  We could figure out a mechanism to make the code
> for the second rule trigger the first rule automatically, but I'm
> not sure it would be worth the extra complexity.  (And it's out
> of the scope of this patch).
Agreed, we can figure it out later.
It will be cleanup and definitely out of scope of this patch.

> 
> > 
> > > 
> > >   https://bugzilla.redhat.com/show_bug.cgi?id=1467599
> > >   ("Unable to start domain: the CPU is incompatible with host CPU:
> > >   Host CPU does not provide required features: svm")
> > > 
> > > Replace x86_cpu_load_def() with simple object_property_set*()
> > > calls.  In addition to fixing the above bug, this makes the KVM
> > > branch in max_x86_cpu_initfn() very similar to the existing TCG
> > > branch.
> > > 
> > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> > > ---
> > >  target/i386/cpu.c | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > > index e2cd157..62d8021 100644
> > > --- a/target/i386/cpu.c
> > > +++ b/target/i386/cpu.c
> > > @@ -1596,15 +1596,21 @@ static void max_x86_cpu_initfn(Object *obj)
> > >      cpu->max_features = true;
> > >  
> > >      if (kvm_enabled()) {
> > > -        X86CPUDefinition host_cpudef = { };
> > > -        uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
> > > +        char vendor[CPUID_VENDOR_SZ + 1] = { 0 };
> > > +        char model_id[CPUID_MODEL_ID_SZ + 1] = { 0 };
> > > +        int family, model, stepping;
> > >  
> > > -        host_vendor_fms(host_cpudef.vendor, &host_cpudef.family,
> > > -                        &host_cpudef.model, &host_cpudef.stepping);
> > > +        host_vendor_fms(vendor, &family, &model, &stepping);
> > >  
> > > -        cpu_x86_fill_model_id(host_cpudef.model_id);
> > > +        cpu_x86_fill_model_id(model_id);
> > >  
> > > -        x86_cpu_load_def(cpu, &host_cpudef, &error_abort);
> > this looses 
> >    env->features[FEAT_1_ECX] |= CPUID_EXT_HYPERVISOR;
> > but it looks like kvm_arch_get_supported_cpuid() will take care of it later
> > at realize() time.
> 
> Yes, kvm_arch_get_supported_cpuid() already sets
> CPUID_EXT_HYPERVISOR, and on -cpu host/max, we only care about
> kvm_arch_get_supported_cpuid() (for KVM) or
> FeatureWord::tcg_features (for TCG).
> 
> This is similar to the x2apic case: x86_cpu_load_def() encodes
> the fact that CPUID_EXT_HYPERVISOR is enabled by default (on the
> predefined CPU models).  kvm_arch_get_supported_cpuid() (and
> FeatureWord::tcg_features) encodes the fact that we _can_ enable
> CPUID_EXT_HYPERVISOR.
> 
> > 
> > > +        object_property_set_str(OBJECT(cpu), vendor, "vendor", &error_abort);
> > > +        object_property_set_int(OBJECT(cpu), family, "family", &error_abort);
> > > +        object_property_set_int(OBJECT(cpu), model, "model", &error_abort);
> > > +        object_property_set_int(OBJECT(cpu), stepping, "stepping",
> > > +                                &error_abort);
> > > +        object_property_set_str(OBJECT(cpu), model_id, "model-id",
> > > +                                &error_abort);
> > >  
> > >          env->cpuid_min_level =
> > >              kvm_arch_get_supported_cpuid(s, 0x0, 0, R_EAX);
> > 
> 

Re: [Qemu-devel] [PATCH 1/4] target/i386: Use simple static property for "model-id"

[Paolo Bonzini]

On 18/07/2017 13:29, Igor Mammedov wrote:
>> It may add a few additional CPU cycles, but I really doubt we can
>> find a workload where CPUID speed has measurable impact.  See,
>> for example, how expensive the kernel KVM CPUID code
>> (kvm_cpuid(), kvm_find_cpuid_entry()) is.
> 
> I don't expect that it would affect KVM, but for TCG any instruction
> execution is 'fast' path, so I'd leave current cpu_x86_cpuid()
> not to loose those few cycles, it's not worth sacrifice for the sake of cleanup.

It's not like this does a QOM property lookup or anything.  I think the
patch is a good idea.

Even simpler way to write the cpuid code:

            int base = (index - 0x80000002) * 16;
            char model[16];

            if (strnlen(env->model_id, base) < base) {
                memset(model, 0, sizeof(model));
            } else {
                strncpy(model, env->model_id + base, sizeof(model));
            }
            *eax = ldl_le_p(&model[0]);
            *ebx = ldl_le_p(&model[4]);
            *ecx = ldl_le_p(&model[8]);
            *edx = ldl_le_p(&model[12]);

Paolo

Re: [Qemu-devel] [PATCH 1/4] target/i386: Use simple static property for "model-id"

[Eduardo Habkost]

On Mon, Jul 24, 2017 at 11:11:34PM +0200, Paolo Bonzini wrote:
> On 18/07/2017 13:29, Igor Mammedov wrote:
> >> It may add a few additional CPU cycles, but I really doubt we can
> >> find a workload where CPUID speed has measurable impact.  See,
> >> for example, how expensive the kernel KVM CPUID code
> >> (kvm_cpuid(), kvm_find_cpuid_entry()) is.
> > 
> > I don't expect that it would affect KVM, but for TCG any instruction
> > execution is 'fast' path, so I'd leave current cpu_x86_cpuid()
> > not to loose those few cycles, it's not worth sacrifice for the sake of cleanup.
> 
> It's not like this does a QOM property lookup or anything.  I think the
> patch is a good idea.
> 
> Even simpler way to write the cpuid code:
> 
>             int base = (index - 0x80000002) * 16;
>             char model[16];
> 
>             if (strnlen(env->model_id, base) < base) {
>                 memset(model, 0, sizeof(model));
>             } else {
>                 strncpy(model, env->model_id + base, sizeof(model));
>             }
>             *eax = ldl_le_p(&model[0]);
>             *ebx = ldl_le_p(&model[4]);
>             *ecx = ldl_le_p(&model[8]);
>             *edx = ldl_le_p(&model[12]);

Neat.  I will use it in v2.  Thanks!

-- 
Eduardo