[libvirt] [PATCH 10/10] cpu_x86: Use signature in CPU detection code

Our current detection code uses just the number of CPU features which need to be added/removed from the CPU model to fully describe the CPUID data. The smallest number wins. But this may sometimes generate wrong results as one can see from the fixed test cases. This patch modifies the algorithm to prefer the CPU model with matching signature even if this model results in a longer list of additional features. Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com&gt; --- src/cpu/cpu_map.xml | 16 +++ src/cpu/cpu_x86.c | 146 +++++++++++++++++++-- .../cputestdata/x86-cpuid-Core-i7-5600U-guest.xml | 11 +- tests/cputestdata/x86-cpuid-Core2-E6850-guest.xml | 5 +- tests/cputestdata/x86-cpuid-Core2-E6850-host.xml | 5 +- tests/cputestdata/x86-cpuid-Xeon-5110-guest.xml | 5 +- tests/cputestdata/x86-cpuid-Xeon-5110-host.xml | 5 +- tests/cputestdata/x86-host+penryn-force-result.xml | 4 +- 8 files changed, 174 insertions(+), 23 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index 0918f75..bf2dfc6 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -717,6 +717,7 @@ <!-- Intel CPU models --> <model name='Conroe'> + <signature family='6' model='15'/> <vendor name='Intel'/> <feature name='apic'/> <feature name='clflush'/> @@ -748,6 +749,7 @@ </model> <model name='Penryn'> + <signature family='6' model='23'/> <vendor name='Intel'/> <feature name='apic'/> <feature name='clflush'/> @@ -781,6 +783,7 @@ </model> <model name='Nehalem'> + <signature family='6' model='26'/> <vendor name='Intel'/> <feature name='apic'/> <feature name='clflush'/> @@ -816,6 +819,7 @@ </model> <model name='Westmere'> + <signature family='6' model='44'/> <vendor name='Intel'/> <feature name='aes'/> <feature name='apic'/> @@ -852,6 +856,7 @@ </model> <model name='SandyBridge'> + <signature family='6' model='42'/> <vendor name='Intel'/> <feature name='aes'/> <feature name='apic'/> @@ -894,6 +899,7 @@ </model> <model name='IvyBridge'> + <signature family='6' model='58'/> <vendor name='Intel'/> <feature name='aes'/> <feature name='apic'/> @@ -942,6 +948,7 @@ </model> <model name='Haswell-noTSX'> + <signature family='6' model='60'/> <vendor name='Intel'/> <feature name='aes'/> <feature name='apic'/> @@ -994,6 +1001,7 @@ </model> <model name='Haswell'> + <signature family='6' model='60'/> <vendor name='Intel'/> <feature name='aes'/> <feature name='apic'/> @@ -1048,6 +1056,7 @@ </model> <model name='Broadwell-noTSX'> + <signature family='6' model='61'/> <vendor name='Intel'/> <feature name='3dnowprefetch'/> <feature name='adx'/> @@ -1104,6 +1113,7 @@ </model> <model name='Broadwell'> + <signature family='6' model='61'/> <vendor name='Intel'/> <feature name='3dnowprefetch'/> <feature name='adx'/> @@ -1162,6 +1172,7 @@ </model> <model name='Skylake-Client'> + <signature family='6' model='94'/> <vendor name='Intel'/> <feature name='3dnowprefetch'/> <feature name='abm'/> @@ -1292,6 +1303,7 @@ </model> <model name='Opteron_G1'> + <signature family='15' model='6'/> <vendor name='AMD'/> <feature name='apic'/> <feature name='clflush'/> @@ -1321,6 +1333,7 @@ </model> <model name='Opteron_G2'> + <signature family='15' model='6'/> <vendor name='AMD'/> <feature name='apic'/> <feature name='clflush'/> @@ -1354,6 +1367,7 @@ </model> <model name='Opteron_G3'> + <signature family='15' model='6'/> <vendor name='AMD'/> <feature name='abm'/> <feature name='apic'/> @@ -1392,6 +1406,7 @@ </model> <model name='Opteron_G4'> + <signature family='21' model='1'/> <vendor name='AMD'/> <feature name='3dnowprefetch'/> <feature name='abm'/> @@ -1440,6 +1455,7 @@ </model> <model name='Opteron_G5'> + <signature family='21' model='2'/> <vendor name='AMD'/> <feature name='3dnowprefetch'/> <feature name='abm'/> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 6823a27..b1a4b93 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -135,6 +135,7 @@ typedef virCPUx86Model *virCPUx86ModelPtr; struct _virCPUx86Model { char *name; virCPUx86VendorPtr vendor; + uint32_t signature; virCPUx86Data data; }; @@ -493,6 +494,75 @@ x86DataToVendor(const virCPUx86Data *data, } +static uint32_t +x86MakeSignature(unsigned int family, + unsigned int model) +{ + uint32_t sig = 0; + + /* + * CPU signature (eax from 0x1 CPUID leaf): + * + * |31 .. 28|27 .. 20|19 .. 16|15 .. 14|13 .. 12|11 .. 8|7 .. 4|3 .. 0| + * | R | extFam | extMod | R | PType | Fam | Mod | Step | + * + * R reserved + * extFam extended family (valid only if Fam == 0xf) + * extMod extended model + * PType processor type + * Fam family + * Mod model + * Step stepping + * + * family = eax[27:20] + eax[11:8] + * model = eax[19:16] << 4 + eax[7:4] + */ + + /* extFam */ + if (family > 0xf) { + sig |= (family - 0xf) << 20; + family = 0xf; + } + + /* extMod */ + sig |= (model >> 4) << 16; + + /* Fam */ + sig |= family << 8; + + /* Mod */ + sig |= (model & 0xf) << 4; + + return sig; +} + + +/* Mask out irrelevant bits (R and Step) from processor signature. */ +#define SIGNATURE_MASK 0x0fff3ff0 + +static uint32_t +x86DataToSignature(const virCPUx86Data *data) +{ + virCPUx86CPUID leaf1 = { .eax_in = 0x1 }; + virCPUx86CPUID *cpuid; + + if (!(cpuid = x86DataCpuid(data, &leaf1))) + return 0; + + return cpuid->eax & SIGNATURE_MASK; +} + + +static int +x86DataAddSignature(virCPUx86Data *data, + uint32_t signature) +{ + virCPUx86CPUID cpuid = { .eax_in = 0x1, .eax = signature }; + + return virCPUx86DataAddCPUID(data, &cpuid); +} + + static virCPUDefPtr x86DataToCPU(const virCPUx86Data *data, virCPUx86ModelPtr model, @@ -898,6 +968,7 @@ x86ModelCopy(virCPUx86ModelPtr model) } copy->vendor = model->vendor; + copy->signature = model->signature; return copy; } @@ -1093,6 +1164,34 @@ x86ModelParse(xmlXPathContextPtr ctxt, goto error; } + if (virXPathBoolean("boolean(./signature)", ctxt)) { + unsigned int sigFamily = 0; + unsigned int sigModel = 0; + int rc; + + rc = virXPathUInt("string(./signature/@family)", ctxt, &sigFamily); + if (rc < 0) { + if (rc == -2 || (rc == 0 && !sigFamily)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid CPU family in model %s"), + model->name); + } + goto cleanup; + } + + rc = virXPathUInt("string(./signature/@model)", ctxt, &sigModel); + if (rc < 0) { + if (rc == -2 || (rc == 0 && !sigModel)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid CPU model in model %s"), + model->name); + } + goto cleanup; + } + + model->signature = x86MakeSignature(sigFamily, sigModel); + } + if (virXPathBoolean("boolean(./vendor)", ctxt)) { vendor = virXPathString("string(./vendor/@name)", ctxt); if (!vendor) { @@ -1480,6 +1579,9 @@ x86Compute(virCPUDefPtr host, &host_model->vendor->cpuid) < 0) goto error; + if (x86DataAddSignature(&guest_model->data, host_model->signature) < 0) + goto error; + if (cpu->type == VIR_CPU_TYPE_GUEST && cpu->match == VIR_CPU_MATCH_EXACT) x86DataSubtract(&guest_model->data, &diff->data); @@ -1549,16 +1651,19 @@ x86GuestData(virCPUDefPtr host, /* - * Checks whether cpuCandidate is a better fit for the CPU data than the - * currently selected one from cpuCurrent. + * Checks whether a candidate model is a better fit for the CPU data than the + * current model. * - * Returns 0 if cpuCurrent is better, - * 1 if cpuCandidate is better, - * 2 if cpuCandidate is the best one (search should stop now). + * Returns 0 if current is better, + * 1 if candidate is better, + * 2 if candidate is the best one (search should stop now). */ static int -x86DecodeUseCandidate(virCPUDefPtr cpuCurrent, +x86DecodeUseCandidate(virCPUx86ModelPtr current, + virCPUDefPtr cpuCurrent, + virCPUx86ModelPtr candidate, virCPUDefPtr cpuCandidate, + uint32_t signature, const char *preferred, bool checkPolicy) { @@ -1578,9 +1683,26 @@ x86DecodeUseCandidate(virCPUDefPtr cpuCurrent, if (!cpuCurrent) return 1; + /* Ideally we want to select a model with family/model equal to + * family/model of the real CPU. Once we found such model, we only + * consider candidates with matching family/model. + */ + if (signature && + current->signature == signature && + candidate->signature != signature) + return 0; + if (cpuCurrent->nfeatures > cpuCandidate->nfeatures) return 1; + /* Prefer a candidate with matching signature even though it would + * result in longer list of features. + */ + if (signature && + candidate->signature == signature && + current->signature != signature) + return 1; + return 0; } @@ -1597,11 +1719,12 @@ x86Decode(virCPUDefPtr cpu, virCPUx86MapPtr map; virCPUx86ModelPtr candidate; virCPUDefPtr cpuCandidate; + virCPUx86ModelPtr model = NULL; virCPUDefPtr cpuModel = NULL; virCPUx86Data copy = VIR_CPU_X86_DATA_INIT; virCPUx86Data features = VIR_CPU_X86_DATA_INIT; - const virCPUx86Data *cpuData = NULL; virCPUx86VendorPtr vendor; + uint32_t signature; ssize_t i; int rc; @@ -1612,6 +1735,7 @@ x86Decode(virCPUDefPtr cpu, return -1; vendor = x86DataToVendor(data, map); + signature = x86DataToSignature(data); /* Walk through the CPU models in reverse order to check newest * models first. @@ -1650,11 +1774,13 @@ x86Decode(virCPUDefPtr cpu, goto cleanup; cpuCandidate->type = cpu->type; - if ((rc = x86DecodeUseCandidate(cpuModel, cpuCandidate, preferred, + if ((rc = x86DecodeUseCandidate(model, cpuModel, + candidate, cpuCandidate, + signature, preferred, cpu->type == VIR_CPU_TYPE_HOST))) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; - cpuData = &candidate->data; + model = candidate; if (rc == 2) break; } else { @@ -1686,7 +1812,7 @@ x86Decode(virCPUDefPtr cpu, } if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) { - if (x86DataCopy(&copy, cpuData) < 0 || + if (x86DataCopy(&copy, &model->data) < 0 || x86DataFromCPUFeatures(&features, cpuModel, map) < 0) goto cleanup; diff --git a/tests/cputestdata/x86-cpuid-Core-i7-5600U-guest.xml b/tests/cputestdata/x86-cpuid-Core-i7-5600U-guest.xml index 78bc0a6..cd7b4bb 100644 --- a/tests/cputestdata/x86-cpuid-Core-i7-5600U-guest.xml +++ b/tests/cputestdata/x86-cpuid-Core-i7-5600U-guest.xml @@ -1,7 +1,8 @@ <cpu mode='custom' match='exact'> <arch>x86_64</arch> - <model fallback='forbid'>Skylake-Client</model> + <model fallback='forbid'>Broadwell</model> <vendor>Intel</vendor> + <feature policy='require' name='vme'/> <feature policy='require' name='ds'/> <feature policy='require' name='acpi'/> <feature policy='require' name='ss'/> @@ -18,10 +19,12 @@ <feature policy='require' name='xtpr'/> <feature policy='require' name='pdcm'/> <feature policy='require' name='osxsave'/> + <feature policy='require' name='f16c'/> + <feature policy='require' name='rdrand'/> + <feature policy='require' name='arat'/> <feature policy='require' name='tsc_adjust'/> + <feature policy='require' name='xsaveopt'/> <feature policy='require' name='pdpe1gb'/> + <feature policy='require' name='abm'/> <feature policy='require' name='invtsc'/> - <feature policy='disable' name='mpx'/> - <feature policy='disable' name='xsavec'/> - <feature policy='disable' name='xgetbv1'/> </cpu> diff --git a/tests/cputestdata/x86-cpuid-Core2-E6850-guest.xml b/tests/cputestdata/x86-cpuid-Core2-E6850-guest.xml index d9df03e..dfcbe24 100644 --- a/tests/cputestdata/x86-cpuid-Core2-E6850-guest.xml +++ b/tests/cputestdata/x86-cpuid-Core2-E6850-guest.xml @@ -1,7 +1,8 @@ <cpu mode='custom' match='exact'> <arch>x86_64</arch> - <model fallback='forbid'>core2duo</model> + <model fallback='forbid'>Conroe</model> <vendor>Intel</vendor> + <feature policy='require' name='vme'/> <feature policy='require' name='ds'/> <feature policy='require' name='acpi'/> <feature policy='require' name='ss'/> @@ -9,6 +10,7 @@ <feature policy='require' name='tm'/> <feature policy='require' name='pbe'/> <feature policy='require' name='dtes64'/> + <feature policy='require' name='monitor'/> <feature policy='require' name='ds_cpl'/> <feature policy='require' name='vmx'/> <feature policy='require' name='smx'/> @@ -17,5 +19,4 @@ <feature policy='require' name='cx16'/> <feature policy='require' name='xtpr'/> <feature policy='require' name='pdcm'/> - <feature policy='require' name='lahf_lm'/> </cpu> diff --git a/tests/cputestdata/x86-cpuid-Core2-E6850-host.xml b/tests/cputestdata/x86-cpuid-Core2-E6850-host.xml index c5ca1cb..e7ddc39 100644 --- a/tests/cputestdata/x86-cpuid-Core2-E6850-host.xml +++ b/tests/cputestdata/x86-cpuid-Core2-E6850-host.xml @@ -1,7 +1,8 @@ <cpu> <arch>x86_64</arch> - <model>core2duo</model> + <model>Conroe</model> <vendor>Intel</vendor> + <feature name='vme'/> <feature name='ds'/> <feature name='acpi'/> <feature name='ss'/> @@ -9,6 +10,7 @@ <feature name='tm'/> <feature name='pbe'/> <feature name='dtes64'/> + <feature name='monitor'/> <feature name='ds_cpl'/> <feature name='vmx'/> <feature name='smx'/> @@ -17,5 +19,4 @@ <feature name='cx16'/> <feature name='xtpr'/> <feature name='pdcm'/> - <feature name='lahf_lm'/> </cpu> diff --git a/tests/cputestdata/x86-cpuid-Xeon-5110-guest.xml b/tests/cputestdata/x86-cpuid-Xeon-5110-guest.xml index 0b9e3f6..28d112b 100644 --- a/tests/cputestdata/x86-cpuid-Xeon-5110-guest.xml +++ b/tests/cputestdata/x86-cpuid-Xeon-5110-guest.xml @@ -1,7 +1,8 @@ <cpu mode='custom' match='exact'> <arch>x86_64</arch> - <model fallback='forbid'>core2duo</model> + <model fallback='forbid'>Conroe</model> <vendor>Intel</vendor> + <feature policy='require' name='vme'/> <feature policy='require' name='ds'/> <feature policy='require' name='acpi'/> <feature policy='require' name='ss'/> @@ -9,6 +10,7 @@ <feature policy='require' name='tm'/> <feature policy='require' name='pbe'/> <feature policy='require' name='dtes64'/> + <feature policy='require' name='monitor'/> <feature policy='require' name='ds_cpl'/> <feature policy='require' name='vmx'/> <feature policy='require' name='tm2'/> @@ -16,5 +18,4 @@ <feature policy='require' name='xtpr'/> <feature policy='require' name='pdcm'/> <feature policy='require' name='dca'/> - <feature policy='require' name='lahf_lm'/> </cpu> diff --git a/tests/cputestdata/x86-cpuid-Xeon-5110-host.xml b/tests/cputestdata/x86-cpuid-Xeon-5110-host.xml index 88c61c4..ca3a84c 100644 --- a/tests/cputestdata/x86-cpuid-Xeon-5110-host.xml +++ b/tests/cputestdata/x86-cpuid-Xeon-5110-host.xml @@ -1,7 +1,8 @@ <cpu> <arch>x86_64</arch> - <model>core2duo</model> + <model>Conroe</model> <vendor>Intel</vendor> + <feature name='vme'/> <feature name='ds'/> <feature name='acpi'/> <feature name='ss'/> @@ -9,6 +10,7 @@ <feature name='tm'/> <feature name='pbe'/> <feature name='dtes64'/> + <feature name='monitor'/> <feature name='ds_cpl'/> <feature name='vmx'/> <feature name='tm2'/> @@ -16,5 +18,4 @@ <feature name='xtpr'/> <feature name='pdcm'/> <feature name='dca'/> - <feature name='lahf_lm'/> </cpu> diff --git a/tests/cputestdata/x86-host+penryn-force-result.xml b/tests/cputestdata/x86-host+penryn-force-result.xml index 000bc0d..ef0a2c0 100644 --- a/tests/cputestdata/x86-host+penryn-force-result.xml +++ b/tests/cputestdata/x86-host+penryn-force-result.xml @@ -1,4 +1,6 @@ <cpu mode='custom' match='exact'> <arch>x86_64</arch> - <model fallback='allow'>Nehalem</model> + <model fallback='allow'>Penryn</model> + <feature policy='require' name='sse4.2'/> + <feature policy='require' name='popcnt'/> </cpu> -- 2.8.4