[Qemu-devel] [PATCH 0/5] Use non-blacklisted family/model/stepping for Haswell CPU model

[Qemu-devel] [PATCH 0/5] Use non-blacklisted family/model/stepping for Haswell CPU model

[Eduardo Habkost]

A recent glibc commit[1] added a blacklist to ensure it won't use
TSX on hosts that are known to have a broken TSX implementation.

Our existing Haswell CPU model has a blacklisted
family/model/stepping combination, so it has to be updated to
make sure guests will really use TSX. This is done by patch 5/5.

However, to do this safely we need to ensure the host CPU is not
a blacklisted one, so we won't mislead guests by exposing
known-to-be-good FMS values on a known-to-be-broken host. This is
done by patch 3/5.

[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=2702856bf45c82cf8e69f2064f5aa15c0ceb6359

---
Cc: dgilbert@redhat.com
Cc: fweimer@redhat.com
Cc: carlos@redhat.com
Cc: triegel@redhat.com
Cc: berrange@redhat.com
Cc: jdenemar@redhat.com
Cc: pbonzini@redhat.com

Eduardo Habkost (5):
  i386: Add explicit array size to x86_cpu_vendor_words2str()
  i386: host_vendor_fms() helper function
  i386/kvm: Blacklist TSX on known broken hosts
  pc: Add 2.9 machine-types
  i386: Change stepping of Haswell to non-blacklisted value

 include/hw/i386/pc.h |  6 ++++++
 target/i386/cpu.h    |  1 +
 hw/i386/pc_piix.c    | 15 ++++++++++++---
 hw/i386/pc_q35.c     | 13 +++++++++++--
 target/i386/cpu.c    | 32 ++++++++++++++++++++++----------
 target/i386/kvm.c    | 17 +++++++++++++++++
 6 files changed, 69 insertions(+), 15 deletions(-)

-- 
2.11.0.259.g40922b1

[Qemu-devel] [PATCH 1/5] i386: Add explicit array size to x86_cpu_vendor_words2str()

[Eduardo Habkost]

Add explicit array size to x86_cpu_vendor_words2str() to let the
compiler check if we are really passing an array of the right
size.

GCC still doesn't print a warning when the array is too small[1],
but clang already does.

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=50584

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

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index a149c8dc42..25b802bb06 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -169,7 +169,7 @@
 
 
 
-static void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
+static void x86_cpu_vendor_words2str(char dst[static (CPUID_VENDOR_SZ + 1)], uint32_t vendor1,
                                      uint32_t vendor2, uint32_t vendor3)
 {
     int i;
-- 
2.11.0.259.g40922b1

[Qemu-devel] [PATCH 2/5] i386: host_vendor_fms() helper function

[Eduardo Habkost]

Helper function for code that needs to check the host CPU
vendor/family/model/stepping values.

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

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index a7f2f6099d..0f4a9d412b 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1423,6 +1423,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
 void cpu_clear_apic_feature(CPUX86State *env);
 void host_cpuid(uint32_t function, uint32_t count,
                 uint32_t *eax, uint32_t *ebx, uint32_t *ecx, uint32_t *edx);
+void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping);
 
 /* helper.c */
 int x86_cpu_handle_mmu_fault(CPUState *cpu, vaddr addr,
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 25b802bb06..9dbb2d98da 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -682,6 +682,25 @@ void host_cpuid(uint32_t function, uint32_t count,
         *edx = vec[3];
 }
 
+void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping)
+{
+    uint32_t eax, ebx, ecx, edx;
+
+    host_cpuid(0x0, 0, &eax, &ebx, &ecx, &edx);
+    x86_cpu_vendor_words2str(vendor, ebx, edx, ecx);
+
+    host_cpuid(0x1, 0, &eax, &ebx, &ecx, &edx);
+    if (family) {
+        *family = ((eax >> 8) & 0x0F) + ((eax >> 20) & 0xFF);
+    }
+    if (model) {
+        *model = ((eax >> 4) & 0x0F) | ((eax & 0xF0000) >> 12);
+    }
+    if (stepping) {
+        *stepping = eax & 0x0F;
+    }
+}
+
 /* CPU class name definitions: */
 
 #define X86_CPU_TYPE_SUFFIX "-" TYPE_X86_CPU
@@ -1574,17 +1593,10 @@ static void host_x86_cpu_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
     X86CPUClass *xcc = X86_CPU_CLASS(oc);
-    uint32_t eax = 0, ebx = 0, ecx = 0, edx = 0;
 
     xcc->kvm_required = true;
 
-    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.11.0.259.g40922b1

[Qemu-devel] [PATCH 3/5] i386/kvm: Blacklist TSX on known broken hosts

[Eduardo Habkost]

Some Intel CPUs are known to have a broken TSX implementation. A
microcode update from Intel disabled TSX on those CPUs, but
GET_SUPPORTED_CPUID might be reporting it as supported if the
hosts were not updated yet.

Manually fixup the GET_SUPPORTED_CPUID data to ensure we will
never enable TSX when running on those hosts.

Reference:
* glibc commit 2702856bf45c82cf8e69f2064f5aa15c0ceb6359:
  https://sourceware.org/git/?p=glibc.git;a=commit;h=2702856bf45c82cf8e69f2064f5aa15c0ceb6359

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

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index 10a9cd8f7f..3e99512640 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -272,6 +272,19 @@ static int get_para_features(KVMState *s)
     return features;
 }
 
+static bool host_tsx_blacklisted(void)
+{
+    int family, model, stepping;\
+    char vendor[CPUID_VENDOR_SZ + 1];
+
+    host_vendor_fms(vendor, &family, &model, &stepping);
+
+    /* Check if we are running on a Haswell host known to have broken TSX */
+    return !strcmp(vendor, CPUID_VENDOR_INTEL) &&
+           (family == 6) &&
+           ((model == 63 && stepping < 4) ||
+            model == 60 || model == 69 || model == 70);
+}
 
 /* Returns the value for a specific register on the cpuid entry
  */
@@ -355,6 +368,10 @@ uint32_t kvm_arch_get_supported_cpuid(KVMState *s, uint32_t function,
         }
     } else if (function == 6 && reg == R_EAX) {
         ret |= CPUID_6_EAX_ARAT; /* safe to allow because of emulated APIC */
+    } else if (function == 7 && index == 0 && reg == R_EBX) {
+        if (host_tsx_blacklisted()) {
+            ret &= ~(CPUID_7_0_EBX_RTM | CPUID_7_0_EBX_HLE);
+        }
     } else if (function == 0x80000001 && reg == R_EDX) {
         /* On Intel, kvm returns cpuid according to the Intel spec,
          * so add missing bits according to the AMD spec:
-- 
2.11.0.259.g40922b1

[Qemu-devel] [PATCH 4/5] pc: Add 2.9 machine-types

[Eduardo Habkost]

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
Changes v1 -> v2:
* Added extra backslash to PC_COMPAT_2_8 definition
  * Suggested-by: Laszlo Ersek <lersek@redhat.com>
---
 include/hw/i386/pc.h |  2 ++
 hw/i386/pc_piix.c    | 15 ++++++++++++---
 hw/i386/pc_q35.c     | 13 +++++++++++--
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index b22e699c46..230e9e70c5 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -375,6 +375,8 @@ int e820_get_num_entries(void);
 bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
 #define PC_COMPAT_2_8 \
+    HW_COMPAT_2_8 \
+
 
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 5e1adbe53c..9f102aa388 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -437,13 +437,24 @@ static void pc_i440fx_machine_options(MachineClass *m)
     m->default_display = "std";
 }
 
-static void pc_i440fx_2_8_machine_options(MachineClass *m)
+static void pc_i440fx_2_9_machine_options(MachineClass *m)
 {
     pc_i440fx_machine_options(m);
     m->alias = "pc";
     m->is_default = 1;
 }
 
+DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
+                      pc_i440fx_2_9_machine_options);
+
+static void pc_i440fx_2_8_machine_options(MachineClass *m)
+{
+    pc_i440fx_2_9_machine_options(m);
+    m->is_default = 0;
+    m->alias = NULL;
+    SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
+}
+
 DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
                       pc_i440fx_2_8_machine_options);
 
@@ -451,8 +462,6 @@ DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
 static void pc_i440fx_2_7_machine_options(MachineClass *m)
 {
     pc_i440fx_2_8_machine_options(m);
-    m->is_default = 0;
-    m->alias = NULL;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
 }
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index d042fe0843..dd792a8547 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -301,19 +301,28 @@ static void pc_q35_machine_options(MachineClass *m)
     m->max_cpus = 288;
 }
 
-static void pc_q35_2_8_machine_options(MachineClass *m)
+static void pc_q35_2_9_machine_options(MachineClass *m)
 {
     pc_q35_machine_options(m);
     m->alias = "q35";
 }
 
+DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
+                   pc_q35_2_9_machine_options);
+
+static void pc_q35_2_8_machine_options(MachineClass *m)
+{
+    pc_q35_2_9_machine_options(m);
+    m->alias = NULL;
+    SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
+}
+
 DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL,
                    pc_q35_2_8_machine_options);
 
 static void pc_q35_2_7_machine_options(MachineClass *m)
 {
     pc_q35_2_8_machine_options(m);
-    m->alias = NULL;
     m->max_cpus = 255;
     SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
 }
-- 
2.11.0.259.g40922b1

[Qemu-devel] [PATCH 5/5] i386: Change stepping of Haswell to non-blacklisted value

[Eduardo Habkost]

glibc blacklists TSX on Haswell CPUs with model==60 and
stepping < 4. To make the Haswell CPU model more useful, make
those guests actually use TSX by changing CPU stepping to 4.

References:
* glibc commit 2702856bf45c82cf8e69f2064f5aa15c0ceb6359
  https://sourceware.org/git/?p=glibc.git;a=commit;h=2702856bf45c82cf8e69f2064f5aa15c0ceb6359

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

diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 230e9e70c5..fcd9b4f541 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -376,7 +376,11 @@ bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
 
 #define PC_COMPAT_2_8 \
     HW_COMPAT_2_8 \
-
+    {\
+        .driver   = "Haswell-" TYPE_X86_CPU,\
+        .property = "stepping",\
+        .value    = "1",\
+    },
 
 #define PC_COMPAT_2_7 \
     HW_COMPAT_2_7 \
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 9dbb2d98da..003de7e74f 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -1190,7 +1190,7 @@ static X86CPUDefinition builtin_x86_defs[] = {
         .vendor = CPUID_VENDOR_INTEL,
         .family = 6,
         .model = 60,
-        .stepping = 1,
+        .stepping = 4,
         .features[FEAT_1_EDX] =
             CPUID_VME | CPUID_SSE2 | CPUID_SSE | CPUID_FXSR | CPUID_MMX |
             CPUID_CLFLUSH | CPUID_PSE36 | CPUID_PAT | CPUID_CMOV | CPUID_MCA |
-- 
2.11.0.259.g40922b1

Re: [Qemu-devel] [PATCH 0/5] Use non-blacklisted family/model/stepping for Haswell CPU model

[no-reply]

Hi,

Your series seems to have some coding style problems. See output below for
more information:

Message-id: 20170108194041.10908-1-ehabkost@redhat.com
Subject: [Qemu-devel] [PATCH 0/5] Use non-blacklisted family/model/stepping for Haswell CPU model
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20170108194041.10908-1-ehabkost@redhat.com -> patchew/20170108194041.10908-1-ehabkost@redhat.com
Switched to a new branch 'test'
d179b85 i386: Change stepping of Haswell to non-blacklisted value
d1e2a1c pc: Add 2.9 machine-types
fd67612 i386/kvm: Blacklist TSX on known broken hosts
2639996 i386: host_vendor_fms() helper function
06c2fb0 i386: Add explicit array size to x86_cpu_vendor_words2str()

=== OUTPUT BEGIN ===
Checking PATCH 1/5: i386: Add explicit array size to x86_cpu_vendor_words2str()...
ERROR: line over 90 characters
#27: FILE: target/i386/cpu.c:172:
+static void x86_cpu_vendor_words2str(char dst[static (CPUID_VENDOR_SZ + 1)], uint32_t vendor1,

ERROR: space prohibited between function name and open parenthesis '('
#27: FILE: target/i386/cpu.c:172:
+static void x86_cpu_vendor_words2str(char dst[static (CPUID_VENDOR_SZ + 1)], uint32_t vendor1,

total: 2 errors, 0 warnings, 8 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/5: i386: host_vendor_fms() helper function...
ERROR: line over 90 characters
#20: FILE: target/i386/cpu.c:685:
+void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping)

ERROR: space prohibited between function name and open parenthesis '('
#20: FILE: target/i386/cpu.c:685:
+void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping)

ERROR: storage class should be at the beginning of the declaration
#20: FILE: target/i386/cpu.c:685:
+void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping)

ERROR: line over 90 characters
#57: FILE: target/i386/cpu.c:1599:
+    host_vendor_fms(host_cpudef.vendor, &host_cpudef.family, &host_cpudef.model, &host_cpudef.stepping);

ERROR: line over 90 characters
#69: FILE: target/i386/cpu.h:1426:
+void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping);

ERROR: space prohibited between function name and open parenthesis '('
#69: FILE: target/i386/cpu.h:1426:
+void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping);

ERROR: storage class should be at the beginning of the declaration
#69: FILE: target/i386/cpu.h:1426:
+void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping);

total: 7 errors, 0 warnings, 50 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 3/5: i386/kvm: Blacklist TSX on known broken hosts...
Checking PATCH 4/5: pc: Add 2.9 machine-types...
Checking PATCH 5/5: i386: Change stepping of Haswell to non-blacklisted value...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH 0/5] Use non-blacklisted family/model/stepping for Haswell CPU model

[Eduardo Habkost]

Oops, there are 3 actual mistakes I missed among the false
positives, below:

On Sun, Jan 08, 2017 at 11:47:21AM -0800, no-reply@patchew.org wrote:
[...]
> === OUTPUT BEGIN ===
> Checking PATCH 1/5: i386: Add explicit array size to x86_cpu_vendor_words2str()...
> ERROR: line over 90 characters
> #27: FILE: target/i386/cpu.c:172:
> +static void x86_cpu_vendor_words2str(char dst[static (CPUID_VENDOR_SZ + 1)], uint32_t vendor1,

False positive.

> 
> ERROR: space prohibited between function name and open parenthesis '('
> #27: FILE: target/i386/cpu.c:172:
> +static void x86_cpu_vendor_words2str(char dst[static (CPUID_VENDOR_SZ + 1)], uint32_t vendor1,

False positive.

> 
> total: 2 errors, 0 warnings, 8 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 2/5: i386: host_vendor_fms() helper function...
> ERROR: line over 90 characters
> #20: FILE: target/i386/cpu.c:685:
> +void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping)

Oops, will fix it.

> 
> ERROR: space prohibited between function name and open parenthesis '('
> #20: FILE: target/i386/cpu.c:685:
> +void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping)

False positive.

> 
> ERROR: storage class should be at the beginning of the declaration
> #20: FILE: target/i386/cpu.c:685:
> +void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping)
> 

False positive.

> ERROR: line over 90 characters
> #57: FILE: target/i386/cpu.c:1599:
> +    host_vendor_fms(host_cpudef.vendor, &host_cpudef.family, &host_cpudef.model, &host_cpudef.stepping);

Will fix it.

> 
> ERROR: line over 90 characters
> #69: FILE: target/i386/cpu.h:1426:
> +void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping);

Will fix it.

> 
> ERROR: space prohibited between function name and open parenthesis '('
> #69: FILE: target/i386/cpu.h:1426:
> +void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping);

False positive.

> 
> ERROR: storage class should be at the beginning of the declaration
> #69: FILE: target/i386/cpu.h:1426:
> +void host_vendor_fms(char vendor[static (CPUID_VENDOR_SZ + 1)], int *family, int *model, int *stepping);

False positive.

> 
> total: 7 errors, 0 warnings, 50 lines checked
> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> Checking PATCH 3/5: i386/kvm: Blacklist TSX on known broken hosts...
> Checking PATCH 4/5: pc: Add 2.9 machine-types...
> Checking PATCH 5/5: i386: Change stepping of Haswell to non-blacklisted value...
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/5] Use non-blacklisted family/model/stepping for Haswell CPU model

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> A recent glibc commit[1] added a blacklist to ensure it won't use
> TSX on hosts that are known to have a broken TSX implementation.
> 
> Our existing Haswell CPU model has a blacklisted
> family/model/stepping combination, so it has to be updated to
> make sure guests will really use TSX. This is done by patch 5/5.
> 
> However, to do this safely we need to ensure the host CPU is not
> a blacklisted one, so we won't mislead guests by exposing
> known-to-be-good FMS values on a known-to-be-broken host. This is
> done by patch 3/5.

I'd just like to mke sure I understand the way this will fail in a migration;
lets say we have a guest that doesn't have the new libc and hosts
with a blacklisted CPU, and -cpu Haswell.

If I understand correctly then:
  a) With 'enforce' the destination qemu will fail to start
     printing an error about the host lack of tsx feature.
  b) Without 'enforce' the destination will start but print 
     the same error as a warning, but the guest will probably
     break as soon as it tries to use a tsx feature?

Any other combination?

Dave

> [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=2702856bf45c82cf8e69f2064f5aa15c0ceb6359
> 
> ---
> Cc: dgilbert@redhat.com
> Cc: fweimer@redhat.com
> Cc: carlos@redhat.com
> Cc: triegel@redhat.com
> Cc: berrange@redhat.com
> Cc: jdenemar@redhat.com
> Cc: pbonzini@redhat.com
> 
> Eduardo Habkost (5):
>   i386: Add explicit array size to x86_cpu_vendor_words2str()
>   i386: host_vendor_fms() helper function
>   i386/kvm: Blacklist TSX on known broken hosts
>   pc: Add 2.9 machine-types
>   i386: Change stepping of Haswell to non-blacklisted value
> 
>  include/hw/i386/pc.h |  6 ++++++
>  target/i386/cpu.h    |  1 +
>  hw/i386/pc_piix.c    | 15 ++++++++++++---
>  hw/i386/pc_q35.c     | 13 +++++++++++--
>  target/i386/cpu.c    | 32 ++++++++++++++++++++++----------
>  target/i386/kvm.c    | 17 +++++++++++++++++
>  6 files changed, 69 insertions(+), 15 deletions(-)
> 
> -- 
> 2.11.0.259.g40922b1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 4/5] pc: Add 2.9 machine-types

[Laszlo Ersek]

On 01/08/17 20:40, Eduardo Habkost wrote:
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---
> Changes v1 -> v2:
> * Added extra backslash to PC_COMPAT_2_8 definition
>   * Suggested-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  include/hw/i386/pc.h |  2 ++
>  hw/i386/pc_piix.c    | 15 ++++++++++++---
>  hw/i386/pc_q35.c     | 13 +++++++++++--
>  3 files changed, 25 insertions(+), 5 deletions(-)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index b22e699c46..230e9e70c5 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -375,6 +375,8 @@ int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
>  #define PC_COMPAT_2_8 \
> +    HW_COMPAT_2_8 \
> +
>  
>  #define PC_COMPAT_2_7 \
>      HW_COMPAT_2_7 \
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 5e1adbe53c..9f102aa388 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -437,13 +437,24 @@ static void pc_i440fx_machine_options(MachineClass *m)
>      m->default_display = "std";
>  }
>  
> -static void pc_i440fx_2_8_machine_options(MachineClass *m)
> +static void pc_i440fx_2_9_machine_options(MachineClass *m)
>  {
>      pc_i440fx_machine_options(m);
>      m->alias = "pc";
>      m->is_default = 1;
>  }
>  
> +DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
> +                      pc_i440fx_2_9_machine_options);
> +
> +static void pc_i440fx_2_8_machine_options(MachineClass *m)
> +{
> +    pc_i440fx_2_9_machine_options(m);
> +    m->is_default = 0;
> +    m->alias = NULL;
> +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
> +}
> +
>  DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
>                        pc_i440fx_2_8_machine_options);
>  
> @@ -451,8 +462,6 @@ DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
>  static void pc_i440fx_2_7_machine_options(MachineClass *m)
>  {
>      pc_i440fx_2_8_machine_options(m);
> -    m->is_default = 0;
> -    m->alias = NULL;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d042fe0843..dd792a8547 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -301,19 +301,28 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->max_cpus = 288;
>  }
>  
> -static void pc_q35_2_8_machine_options(MachineClass *m)
> +static void pc_q35_2_9_machine_options(MachineClass *m)
>  {
>      pc_q35_machine_options(m);
>      m->alias = "q35";
>  }
>  
> +DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
> +                   pc_q35_2_9_machine_options);
> +
> +static void pc_q35_2_8_machine_options(MachineClass *m)
> +{
> +    pc_q35_2_9_machine_options(m);
> +    m->alias = NULL;
> +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
> +}
> +
>  DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL,
>                     pc_q35_2_8_machine_options);
>  
>  static void pc_q35_2_7_machine_options(MachineClass *m)
>  {
>      pc_q35_2_8_machine_options(m);
> -    m->alias = NULL;
>      m->max_cpus = 255;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
>  }
> 

Re: [Qemu-devel] [PATCH 4/5] pc: Add 2.9 machine-types

[Michael S. Tsirkin]

On Sun, Jan 08, 2017 at 05:40:40PM -0200, Eduardo Habkost wrote:
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>

Do I understand it correctly that you are merging this through
another tree?

In that case

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


> ---
> Changes v1 -> v2:
> * Added extra backslash to PC_COMPAT_2_8 definition
>   * Suggested-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  include/hw/i386/pc.h |  2 ++
>  hw/i386/pc_piix.c    | 15 ++++++++++++---
>  hw/i386/pc_q35.c     | 13 +++++++++++--
>  3 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index b22e699c46..230e9e70c5 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -375,6 +375,8 @@ int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
>  #define PC_COMPAT_2_8 \
> +    HW_COMPAT_2_8 \
> +
>  
>  #define PC_COMPAT_2_7 \
>      HW_COMPAT_2_7 \
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 5e1adbe53c..9f102aa388 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -437,13 +437,24 @@ static void pc_i440fx_machine_options(MachineClass *m)
>      m->default_display = "std";
>  }
>  
> -static void pc_i440fx_2_8_machine_options(MachineClass *m)
> +static void pc_i440fx_2_9_machine_options(MachineClass *m)
>  {
>      pc_i440fx_machine_options(m);
>      m->alias = "pc";
>      m->is_default = 1;
>  }
>  
> +DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
> +                      pc_i440fx_2_9_machine_options);
> +
> +static void pc_i440fx_2_8_machine_options(MachineClass *m)
> +{
> +    pc_i440fx_2_9_machine_options(m);
> +    m->is_default = 0;
> +    m->alias = NULL;
> +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
> +}
> +
>  DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
>                        pc_i440fx_2_8_machine_options);
>  
> @@ -451,8 +462,6 @@ DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
>  static void pc_i440fx_2_7_machine_options(MachineClass *m)
>  {
>      pc_i440fx_2_8_machine_options(m);
> -    m->is_default = 0;
> -    m->alias = NULL;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
>  }
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index d042fe0843..dd792a8547 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -301,19 +301,28 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->max_cpus = 288;
>  }
>  
> -static void pc_q35_2_8_machine_options(MachineClass *m)
> +static void pc_q35_2_9_machine_options(MachineClass *m)
>  {
>      pc_q35_machine_options(m);
>      m->alias = "q35";
>  }
>  
> +DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
> +                   pc_q35_2_9_machine_options);
> +
> +static void pc_q35_2_8_machine_options(MachineClass *m)
> +{
> +    pc_q35_2_9_machine_options(m);
> +    m->alias = NULL;
> +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
> +}
> +
>  DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL,
>                     pc_q35_2_8_machine_options);
>  
>  static void pc_q35_2_7_machine_options(MachineClass *m)
>  {
>      pc_q35_2_8_machine_options(m);
> -    m->alias = NULL;
>      m->max_cpus = 255;
>      SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
>  }
> -- 
> 2.11.0.259.g40922b1

Re: [Qemu-devel] [PATCH 0/5] Use non-blacklisted family/model/stepping for Haswell CPU model

[Eduardo Habkost]

On Mon, Jan 09, 2017 at 11:35:54AM +0000, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > A recent glibc commit[1] added a blacklist to ensure it won't use
> > TSX on hosts that are known to have a broken TSX implementation.
> > 
> > Our existing Haswell CPU model has a blacklisted
> > family/model/stepping combination, so it has to be updated to
> > make sure guests will really use TSX. This is done by patch 5/5.
> > 
> > However, to do this safely we need to ensure the host CPU is not
> > a blacklisted one, so we won't mislead guests by exposing
> > known-to-be-good FMS values on a known-to-be-broken host. This is
> > done by patch 3/5.
> 
> I'd just like to mke sure I understand the way this will fail in a migration;
> lets say we have a guest that doesn't have the new libc and hosts
> with a blacklisted CPU, and -cpu Haswell.
> 
> If I understand correctly then:
>   a) With 'enforce' the destination qemu will fail to start
>      printing an error about the host lack of tsx feature.

Yes.

>   b) Without 'enforce' the destination will start but print 
>      the same error as a warning, but the guest will probably
>      break as soon as it tries to use a tsx feature?

Yes. The general rule is: without "enforce", live migration can
break in unpredictable ways.

Without "enforce", QEMU will print a warning, and the VCPU will
run _without_ the TSX features on CPUID. If we're live-migrating,
it may break the guest if it tries to use a TSX feature, or break
migration if a TSX-related bit is already set on a MSR.


> 
> Any other combination?
> 
> Dave
> 
> > [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=2702856bf45c82cf8e69f2064f5aa15c0ceb6359
> > 
> > ---
> > Cc: dgilbert@redhat.com
> > Cc: fweimer@redhat.com
> > Cc: carlos@redhat.com
> > Cc: triegel@redhat.com
> > Cc: berrange@redhat.com
> > Cc: jdenemar@redhat.com
> > Cc: pbonzini@redhat.com
> > 
> > Eduardo Habkost (5):
> >   i386: Add explicit array size to x86_cpu_vendor_words2str()
> >   i386: host_vendor_fms() helper function
> >   i386/kvm: Blacklist TSX on known broken hosts
> >   pc: Add 2.9 machine-types
> >   i386: Change stepping of Haswell to non-blacklisted value
> > 
> >  include/hw/i386/pc.h |  6 ++++++
> >  target/i386/cpu.h    |  1 +
> >  hw/i386/pc_piix.c    | 15 ++++++++++++---
> >  hw/i386/pc_q35.c     | 13 +++++++++++--
> >  target/i386/cpu.c    | 32 ++++++++++++++++++++++----------
> >  target/i386/kvm.c    | 17 +++++++++++++++++
> >  6 files changed, 69 insertions(+), 15 deletions(-)
> > 
> > -- 
> > 2.11.0.259.g40922b1
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/5] pc: Add 2.9 machine-types

[Eduardo Habkost]

On Tue, Jan 10, 2017 at 06:06:33AM +0200, Michael S. Tsirkin wrote:
> On Sun, Jan 08, 2017 at 05:40:40PM -0200, Eduardo Habkost wrote:
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Igor Mammedov <imammedo@redhat.com>
> > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Do I understand it correctly that you are merging this through
> another tree?

I will apply it when including this series only if it is not
merged through another tree first. I believe multiple series
depend on adding the pc-2.9 machine-types. Feel free to apply it
to your tree if you want to.

> 
> In that case
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Thanks!

> 
> 
> > ---
> > Changes v1 -> v2:
> > * Added extra backslash to PC_COMPAT_2_8 definition
> >   * Suggested-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >  include/hw/i386/pc.h |  2 ++
> >  hw/i386/pc_piix.c    | 15 ++++++++++++---
> >  hw/i386/pc_q35.c     | 13 +++++++++++--
> >  3 files changed, 25 insertions(+), 5 deletions(-)
> > 
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index b22e699c46..230e9e70c5 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -375,6 +375,8 @@ int e820_get_num_entries(void);
> >  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
> >  
> >  #define PC_COMPAT_2_8 \
> > +    HW_COMPAT_2_8 \
> > +
> >  
> >  #define PC_COMPAT_2_7 \
> >      HW_COMPAT_2_7 \
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 5e1adbe53c..9f102aa388 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -437,13 +437,24 @@ static void pc_i440fx_machine_options(MachineClass *m)
> >      m->default_display = "std";
> >  }
> >  
> > -static void pc_i440fx_2_8_machine_options(MachineClass *m)
> > +static void pc_i440fx_2_9_machine_options(MachineClass *m)
> >  {
> >      pc_i440fx_machine_options(m);
> >      m->alias = "pc";
> >      m->is_default = 1;
> >  }
> >  
> > +DEFINE_I440FX_MACHINE(v2_9, "pc-i440fx-2.9", NULL,
> > +                      pc_i440fx_2_9_machine_options);
> > +
> > +static void pc_i440fx_2_8_machine_options(MachineClass *m)
> > +{
> > +    pc_i440fx_2_9_machine_options(m);
> > +    m->is_default = 0;
> > +    m->alias = NULL;
> > +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
> > +}
> > +
> >  DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
> >                        pc_i440fx_2_8_machine_options);
> >  
> > @@ -451,8 +462,6 @@ DEFINE_I440FX_MACHINE(v2_8, "pc-i440fx-2.8", NULL,
> >  static void pc_i440fx_2_7_machine_options(MachineClass *m)
> >  {
> >      pc_i440fx_2_8_machine_options(m);
> > -    m->is_default = 0;
> > -    m->alias = NULL;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
> >  }
> >  
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index d042fe0843..dd792a8547 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -301,19 +301,28 @@ static void pc_q35_machine_options(MachineClass *m)
> >      m->max_cpus = 288;
> >  }
> >  
> > -static void pc_q35_2_8_machine_options(MachineClass *m)
> > +static void pc_q35_2_9_machine_options(MachineClass *m)
> >  {
> >      pc_q35_machine_options(m);
> >      m->alias = "q35";
> >  }
> >  
> > +DEFINE_Q35_MACHINE(v2_9, "pc-q35-2.9", NULL,
> > +                   pc_q35_2_9_machine_options);
> > +
> > +static void pc_q35_2_8_machine_options(MachineClass *m)
> > +{
> > +    pc_q35_2_9_machine_options(m);
> > +    m->alias = NULL;
> > +    SET_MACHINE_COMPAT(m, PC_COMPAT_2_8);
> > +}
> > +
> >  DEFINE_Q35_MACHINE(v2_8, "pc-q35-2.8", NULL,
> >                     pc_q35_2_8_machine_options);
> >  
> >  static void pc_q35_2_7_machine_options(MachineClass *m)
> >  {
> >      pc_q35_2_8_machine_options(m);
> > -    m->alias = NULL;
> >      m->max_cpus = 255;
> >      SET_MACHINE_COMPAT(m, PC_COMPAT_2_7);
> >  }
> > -- 
> > 2.11.0.259.g40922b1

-- 
Eduardo

Re: [Qemu-devel] [PATCH 0/5] Use non-blacklisted family/model/stepping for Haswell CPU model

[Dr. David Alan Gilbert]

* Eduardo Habkost (ehabkost@redhat.com) wrote:
> On Mon, Jan 09, 2017 at 11:35:54AM +0000, Dr. David Alan Gilbert wrote:
> > * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > > A recent glibc commit[1] added a blacklist to ensure it won't use
> > > TSX on hosts that are known to have a broken TSX implementation.
> > > 
> > > Our existing Haswell CPU model has a blacklisted
> > > family/model/stepping combination, so it has to be updated to
> > > make sure guests will really use TSX. This is done by patch 5/5.
> > > 
> > > However, to do this safely we need to ensure the host CPU is not
> > > a blacklisted one, so we won't mislead guests by exposing
> > > known-to-be-good FMS values on a known-to-be-broken host. This is
> > > done by patch 3/5.
> > 
> > I'd just like to mke sure I understand the way this will fail in a migration;
> > lets say we have a guest that doesn't have the new libc and hosts
> > with a blacklisted CPU, and -cpu Haswell.
> > 
> > If I understand correctly then:
> >   a) With 'enforce' the destination qemu will fail to start
> >      printing an error about the host lack of tsx feature.
> 
> Yes.
> 
> >   b) Without 'enforce' the destination will start but print 
> >      the same error as a warning, but the guest will probably
> >      break as soon as it tries to use a tsx feature?
> 
> Yes. The general rule is: without "enforce", live migration can
> break in unpredictable ways.
> 
> Without "enforce", QEMU will print a warning, and the VCPU will
> run _without_ the TSX features on CPUID. If we're live-migrating,
> it may break the guest if it tries to use a TSX feature, or break
> migration if a TSX-related bit is already set on a MSR.

OK, but you've been telling people to use "enforce" long enough that
they should have listened.

Are there any other cases we have to worry about;  lets say a VM with the
new libc being migrated from an older QEMU, it suddenly changes
CPU ID to one that's supported; what happens?
I'm hoping the guest CPU ID is preserved with the TSX disabled until
a reboot?

Dave

> 
> > 
> > Any other combination?
> > 
> > Dave
> > 
> > > [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=2702856bf45c82cf8e69f2064f5aa15c0ceb6359
> > > 
> > > ---
> > > Cc: dgilbert@redhat.com
> > > Cc: fweimer@redhat.com
> > > Cc: carlos@redhat.com
> > > Cc: triegel@redhat.com
> > > Cc: berrange@redhat.com
> > > Cc: jdenemar@redhat.com
> > > Cc: pbonzini@redhat.com
> > > 
> > > Eduardo Habkost (5):
> > >   i386: Add explicit array size to x86_cpu_vendor_words2str()
> > >   i386: host_vendor_fms() helper function
> > >   i386/kvm: Blacklist TSX on known broken hosts
> > >   pc: Add 2.9 machine-types
> > >   i386: Change stepping of Haswell to non-blacklisted value
> > > 
> > >  include/hw/i386/pc.h |  6 ++++++
> > >  target/i386/cpu.h    |  1 +
> > >  hw/i386/pc_piix.c    | 15 ++++++++++++---
> > >  hw/i386/pc_q35.c     | 13 +++++++++++--
> > >  target/i386/cpu.c    | 32 ++++++++++++++++++++++----------
> > >  target/i386/kvm.c    | 17 +++++++++++++++++
> > >  6 files changed, 69 insertions(+), 15 deletions(-)
> > > 
> > > -- 
> > > 2.11.0.259.g40922b1
> > > 
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/5] Use non-blacklisted family/model/stepping for Haswell CPU model

[Eduardo Habkost]

On Thu, Jan 12, 2017 at 12:38:00PM +0000, Dr. David Alan Gilbert wrote:
> * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > On Mon, Jan 09, 2017 at 11:35:54AM +0000, Dr. David Alan Gilbert wrote:
> > > * Eduardo Habkost (ehabkost@redhat.com) wrote:
> > > > A recent glibc commit[1] added a blacklist to ensure it won't use
> > > > TSX on hosts that are known to have a broken TSX implementation.
> > > > 
> > > > Our existing Haswell CPU model has a blacklisted
> > > > family/model/stepping combination, so it has to be updated to
> > > > make sure guests will really use TSX. This is done by patch 5/5.
> > > > 
> > > > However, to do this safely we need to ensure the host CPU is not
> > > > a blacklisted one, so we won't mislead guests by exposing
> > > > known-to-be-good FMS values on a known-to-be-broken host. This is
> > > > done by patch 3/5.
> > > 
> > > I'd just like to mke sure I understand the way this will fail in a migration;
> > > lets say we have a guest that doesn't have the new libc and hosts
> > > with a blacklisted CPU, and -cpu Haswell.
> > > 
> > > If I understand correctly then:
> > >   a) With 'enforce' the destination qemu will fail to start
> > >      printing an error about the host lack of tsx feature.
> > 
> > Yes.
> > 
> > >   b) Without 'enforce' the destination will start but print 
> > >      the same error as a warning, but the guest will probably
> > >      break as soon as it tries to use a tsx feature?
> > 
> > Yes. The general rule is: without "enforce", live migration can
> > break in unpredictable ways.
> > 
> > Without "enforce", QEMU will print a warning, and the VCPU will
> > run _without_ the TSX features on CPUID. If we're live-migrating,
> > it may break the guest if it tries to use a TSX feature, or break
> > migration if a TSX-related bit is already set on a MSR.
> 
> OK, but you've been telling people to use "enforce" long enough that
> they should have listened.
> 
> Are there any other cases we have to worry about;  lets say a VM with the
> new libc being migrated from an older QEMU, it suddenly changes
> CPU ID to one that's supported; what happens?

I assume you are talking about the stepping change, when
migrating from an old QEMU to a host that is _not_ on the
blacklist. In this case, the guest won't see any changes: CPUID
family/model/stepping will be kept the same for the whole life of
the VM (even if it is shut down), thanks to the machine-type
compatibility code.

> I'm hoping the guest CPU ID is preserved with the TSX disabled until
> a reboot?

CPUID changes are effective immediately on migration. But guests
often notice the change only on the next reboot.

We could do something to make these problems less likely:
including CPUID data on the migration stream. I have considered
it on the past, but never implemented it. Maybe I should
reconsider that.

(This is another case where it would be interesting to have a
mechanism to let the destination host abort migration early: we
could make QEMU work in "enforce" mode when live-migrating, but
using the migrated CPUID data. This way CPUID changes would never
happen.)

> 
> Dave
> 
> > 
> > > 
> > > Any other combination?
> > > 
> > > Dave
> > > 
> > > > [1] https://sourceware.org/git/?p=glibc.git;a=commit;h=2702856bf45c82cf8e69f2064f5aa15c0ceb6359
> > > > 
> > > > ---
> > > > Cc: dgilbert@redhat.com
> > > > Cc: fweimer@redhat.com
> > > > Cc: carlos@redhat.com
> > > > Cc: triegel@redhat.com
> > > > Cc: berrange@redhat.com
> > > > Cc: jdenemar@redhat.com
> > > > Cc: pbonzini@redhat.com
> > > > 
> > > > Eduardo Habkost (5):
> > > >   i386: Add explicit array size to x86_cpu_vendor_words2str()
> > > >   i386: host_vendor_fms() helper function
> > > >   i386/kvm: Blacklist TSX on known broken hosts
> > > >   pc: Add 2.9 machine-types
> > > >   i386: Change stepping of Haswell to non-blacklisted value
> > > > 
> > > >  include/hw/i386/pc.h |  6 ++++++
> > > >  target/i386/cpu.h    |  1 +
> > > >  hw/i386/pc_piix.c    | 15 ++++++++++++---
> > > >  hw/i386/pc_q35.c     | 13 +++++++++++--
> > > >  target/i386/cpu.c    | 32 ++++++++++++++++++++++----------
> > > >  target/i386/kvm.c    | 17 +++++++++++++++++
> > > >  6 files changed, 69 insertions(+), 15 deletions(-)
> > > > 
> > > > -- 
> > > > 2.11.0.259.g40922b1
> > > > 
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
> > -- 
> > Eduardo
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Eduardo