[Qemu-devel] [PULL 0/3] x86 bug fix for -rc1

[Qemu-devel] [PULL 0/3] x86 bug fix for -rc1

[Eduardo Habkost]

The following changes since commit 522fd24ca030c27c591dafedd65c1dfd51e40450:

  Update version for v2.10.0-rc0 release (2017-07-25 17:13:09 +0100)

are available in the git repository at:

  git://github.com/ehabkost/qemu.git tags/x86-pull-request

for you to fetch changes up to bd1820227ecc0c77cc2aeba7c7c25b2d0a72ff3c:

  target/i386: Don't use x86_cpu_load_def() on "max" CPU model (2017-07-26 14:55:12 -0300)

----------------------------------------------------------------
x86 bug fix for -rc1

Fix for a bug in "-cpu max" that breaks libvirt usage of
query-cpu-model-expansion.

----------------------------------------------------------------

Eduardo Habkost (3):
  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.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

-- 
2.9.4

[Qemu-devel] [PULL 1/3] 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>
Message-Id: <20170712162058.10538-3-ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
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 89f5fb7..156dc95 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1636,13 +1636,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] [PULL 2/3] 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>
Message-Id: <20170712162058.10538-4-ehabkost@redhat.com>
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 156dc95..8558c60 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1585,6 +1585,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] [PULL 3/3] 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.

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>
Message-Id: <20170712162058.10538-5-ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
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 8558c60..ddc45ab 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1644,15 +1644,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] [PULL 0/3] x86 bug fix for -rc1

[Peter Maydell]

On 26 July 2017 at 19:36, Eduardo Habkost <ehabkost@redhat.com> wrote:
> The following changes since commit 522fd24ca030c27c591dafedd65c1dfd51e40450:
>
>   Update version for v2.10.0-rc0 release (2017-07-25 17:13:09 +0100)
>
> are available in the git repository at:
>
>   git://github.com/ehabkost/qemu.git tags/x86-pull-request
>
> for you to fetch changes up to bd1820227ecc0c77cc2aeba7c7c25b2d0a72ff3c:
>
>   target/i386: Don't use x86_cpu_load_def() on "max" CPU model (2017-07-26 14:55:12 -0300)
>
> ----------------------------------------------------------------
> x86 bug fix for -rc1
>
> Fix for a bug in "-cpu max" that breaks libvirt usage of
> query-cpu-model-expansion.
>
> ----------------------------------------------------------------
>
> Eduardo Habkost (3):
>   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.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
>

Applied, thanks.

-- PMM