[libvirt] [PATCH 11/26] cpu_x86: Allow multiple signatures for a CPU model
Ján Tomko
jtomko at redhat.com
Thu Feb 28 15:27:34 UTC 2019
On Wed, Feb 27, 2019 at 02:29:01PM +0100, Jiri Denemark wrote:
>CPU signatures in the cpu_map serve as a hint for CPUID to CPU model
>matching algorithm. If the CPU signatures matches any CPU model in the
>cpu_map, this model will be the preferred one.
>
>This works out well and solved several mismatches, but in real world
>CPUs which should match a single CPU model may be produced with several
>different signatures. For example, low voltage Broadwell CPUs for
>laptops and Broadwell CPUs for servers differ in CPU model numbers while
>we should detect them all as Broadwell CPU model.
>
>This patch adds support for storing several signatures for a single CPU
>model to make this hint useful for more CPUs. Later commits will provide
>additional signatures for existing CPU models, which will correct some
>results in our CPU test suite.
>
>Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
>---
> src/cpu/cpu_x86.c | 104 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 86 insertions(+), 18 deletions(-)
>
>diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
>index c2f22f399f..89303400af 100644
>--- a/src/cpu/cpu_x86.c
>+++ b/src/cpu/cpu_x86.c
>@@ -974,11 +975,29 @@ x86ModelFree(virCPUx86ModelPtr model)
> return;
>
> VIR_FREE(model->name);
>+ VIR_FREE(model->signatures);
> virCPUx86DataClear(&model->data);
> VIR_FREE(model);
> }
>
>
>+static int
>+x86ModelCopySignatures(virCPUx86ModelPtr dst,
>+ virCPUx86ModelPtr src)
>+{
This would have been easier to read split into several patches,
e.g. adding this helper that will just:
return dst->signature == src->signature;
Although there are not that many users of this function anyway.
>+ size_t i;
>+
>+ if (VIR_ALLOC_N(dst->signatures, src->nsignatures) < 0)
>+ return -1;
>+
>+ dst->nsignatures = src->nsignatures;
>+ for (i = 0; i < src->nsignatures; i++)
>+ dst->signatures[i] = src->signatures[i];
>+
>+ return 0;
>+}
>+
>+
> static virCPUx86ModelPtr
> x86ModelCopy(virCPUx86ModelPtr model)
> {
>@@ -986,13 +1005,13 @@ x86ModelCopy(virCPUx86ModelPtr model)
>
> if (VIR_ALLOC(copy) < 0 ||
> VIR_STRDUP(copy->name, model->name) < 0 ||
>+ x86ModelCopySignatures(copy, model) < 0 ||
> x86DataCopy(©->data, &model->data) < 0) {
> x86ModelFree(copy);
> return NULL;
> }
>
> copy->vendor = model->vendor;
>- copy->signature = model->signature;
>
> return copy;
> }
>@@ -1178,8 +1197,8 @@ x86ModelParseAncestor(virCPUx86ModelPtr model,
> }
>
> model->vendor = ancestor->vendor;
>- model->signature = ancestor->signature;
>- if (x86DataCopy(&model->data, &ancestor->data) < 0)
>+ if (x86ModelCopySignatures(model, ancestor) < 0 ||
>+ x86DataCopy(&model->data, &ancestor->data) < 0)
> return -1;
>
> return 0;
>@@ -1187,16 +1206,32 @@ x86ModelParseAncestor(virCPUx86ModelPtr model,
>
>
> static int
>-x86ModelParseSignature(virCPUx86ModelPtr model,
>- xmlXPathContextPtr ctxt)
>+x86ModelParseSignatures(virCPUx86ModelPtr model,
>+ xmlXPathContextPtr ctxt)
> {
>- int rc;
This variable was added to this place just a few commits above.
>+ VIR_AUTOFREE(xmlNodePtr *) nodes = NULL;
>+ xmlNodePtr root = ctxt->node;
>+ size_t i;
>+ int n;
>
>- if (virXPathBoolean("boolean(./signature)", ctxt)) {
>+ if ((n = virXPathNodeSet("./signature", ctxt, &nodes)) <= 0)
>+ return n;
>+
>+ /* Remove inherited signatures. */
>+ VIR_FREE(model->signatures);
>+
>+ model->nsignatures = n;
>+ if (VIR_ALLOC_N(model->signatures, n) < 0)
>+ return -1;
>+
>+ for (i = 0; i < n; i++) {
> unsigned int sigFamily = 0;
> unsigned int sigModel = 0;
>+ int rc;
>
>- rc = virXPathUInt("string(./signature/@family)", ctxt, &sigFamily);
>+ ctxt->node = nodes[i];
>+
>+ rc = virXPathUInt("string(@family)", ctxt, &sigFamily);
The ctxt->node and XPath changes could have been separate.
> if (rc < 0 || sigFamily == 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Invalid CPU signature family in model %s"),
>@@ -1204,7 +1239,7 @@ x86ModelParseSignature(virCPUx86ModelPtr model,
> return -1;
> }
>
>- rc = virXPathUInt("string(./signature/@model)", ctxt, &sigModel);
>+ rc = virXPathUInt("string(@model)", ctxt, &sigModel);
> if (rc < 0 || sigModel == 0) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Invalid CPU signature model in model %s"),
>@@ -1212,9 +1247,10 @@ x86ModelParseSignature(virCPUx86ModelPtr model,
> return -1;
> }
>
>- model->signature = x86MakeSignature(sigFamily, sigModel, 0);
>+ model->signatures[i] = x86MakeSignature(sigFamily, sigModel, 0);
> }
>
>+ ctxt->node = root;
> return 0;
> }
>
>@@ -1311,7 +1347,7 @@ x86ModelParse(xmlXPathContextPtr ctxt,
> if (x86ModelParseAncestor(model, ctxt, map) < 0)
> goto cleanup;
>
>- if (x86ModelParseSignature(model, ctxt) < 0)
>+ if (x86ModelParseSignatures(model, ctxt) < 0)
> goto cleanup;
>
> if (x86ModelParseVendor(model, ctxt, map) < 0)
>@@ -1615,7 +1651,8 @@ x86Compute(virCPUDefPtr host,
> &host_model->vendor->cpuid) < 0)
> goto error;
>
>- if (x86DataAddSignature(&guest_model->data, host_model->signature) < 0)
>+ if (host_model->signatures &&
>+ x86DataAddSignature(&guest_model->data, *host_model->signatures) < 0)
> goto error;
>
> if (cpu->type == VIR_CPU_TYPE_GUEST
>@@ -1721,6 +1758,21 @@ virCPUx86Compare(virCPUDefPtr host,
> }
>
>
>+static bool
>+x86ModelHasSignature(virCPUx86ModelPtr model,
>+ uint32_t signature)
>+{
>+ size_t i;
>+
>+ for (i = 0; i < model->nsignatures; i++) {
>+ if (model->signatures[i] == signature)
>+ return true;
>+ }
>+
>+ return false;
>+}
>+
>+
> /*
> * Checks whether a candidate model is a better fit for the CPU data than the
> * current model.
>@@ -1762,8 +1814,8 @@ x86DecodeUseCandidate(virCPUx86ModelPtr current,
> * consider candidates with matching family/model.
> */
> if (signature &&
>- current->signature == signature &&
>- candidate->signature != signature) {
>+ x86ModelHasSignature(current, signature) &&
>+ !x86ModelHasSignature(candidate, signature)) {
> VIR_DEBUG("%s differs in signature from matching %s",
> cpuCandidate->model, cpuCurrent->model);
> return 0;
>@@ -1779,8 +1831,8 @@ x86DecodeUseCandidate(virCPUx86ModelPtr current,
> * result in longer list of features.
> */
> if (signature &&
>- candidate->signature == signature &&
>- current->signature != signature) {
>+ x86ModelHasSignature(candidate, signature) &&
>+ !x86ModelHasSignature(current, signature)) {
> VIR_DEBUG("%s provides matching signature", cpuCandidate->model);
> return 1;
> }
>@@ -1844,6 +1896,8 @@ x86Decode(virCPUDefPtr cpu,
> virCPUx86Data features = VIR_CPU_X86_DATA_INIT;
> virCPUx86VendorPtr vendor;
> virDomainCapsCPUModelPtr hvModel = NULL;
>+ virBuffer sigBuf = VIR_BUFFER_INITIALIZER;
>+ char *sigs = NULL;
> uint32_t signature;
> ssize_t i;
> int rc;
>@@ -1936,6 +1990,18 @@ x86Decode(virCPUDefPtr cpu,
> if (vendor && VIR_STRDUP(cpu->vendor, vendor->name) < 0)
> goto cleanup;
>
>+ for (i = 0; i < model->nsignatures; i++)
>+ virBufferAsprintf(&sigBuf, "%06x,", model->signatures[i]);
>+
>+ virBufferTrim(&sigBuf, ",", -1);
>+ if (virBufferCheckError(&sigBuf) < 0)
>+ goto cleanup;
>+
>+ sigs = virBufferContentAndReset(&sigBuf);
>+
Formatting the list of signatures is a) unrelated to decoding the CPU
b) only used for debug output
Can you split it out in a separate function to address a)?
I don't think we have a way to avoid b).
Alternatively, you can format just the model name.
>+ VIR_DEBUG("Using CPU model %s (signatures %s) for CPU with signature %06lx",
>+ model->name, sigs, (long)signature);
>+
> VIR_STEAL_PTR(cpu->model, cpuModel->model);
> VIR_STEAL_PTR(cpu->features, cpuModel->features);
> cpu->nfeatures = cpuModel->nfeatures;
Reviewed-by: Ján Tomko <jtomko at redhat.com>
Jano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190228/95fb237e/attachment-0001.sig>
More information about the libvir-list
mailing list