[libvirt] [PATCH 0/5] Use non-blacklisted family/model/stepping for Haswell CPU model
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=2702856bf45c82cf8e69f2064... --- 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
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
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
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=2702856bf45c82cf8e69f2064... 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
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
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); }
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
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
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=2702856bf45c82cf8e69f2064... 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
* 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=2702856bf45c82cf8e69f2064...
--- 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
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=2702856bf45c82cf8e69f2064...
--- 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
* 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=2702856bf45c82cf8e69f2064...
--- 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
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=2702856bf45c82cf8e69f2064...
--- 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
participants (4)
-
Dr. David Alan Gilbert -
Eduardo Habkost -
Laszlo Ersek -
Michael S. Tsirkin