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@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
@@ -147,7 +147,8 @@ typedef virCPUx86Model *virCPUx86ModelPtr;
struct _virCPUx86Model {
char *name;
virCPUx86VendorPtr vendor;
- uint32_t signature;
+ size_t nsignatures;
+ uint32_t *signatures;
virCPUx86Data data;
};
@@ -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)
+{
+ 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;
+ 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);
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);
+
+ 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;
@@ -1950,6 +2016,7 @@ x86Decode(virCPUDefPtr cpu,
virCPUx86DataClear(&data);
virCPUx86DataClear(©);
virCPUx86DataClear(&features);
+ VIR_FREE(sigs);
return ret;
}
@@ -2848,7 +2915,8 @@ virCPUx86Translate(virCPUDefPtr cpu,
virCPUx86DataAddCPUIDInt(&model->data, &model->vendor->cpuid) < 0)
goto cleanup;
- if (x86DataAddSignature(&model->data, model->signature) < 0)
+ if (model->signatures &&
+ x86DataAddSignature(&model->data, model->signatures[0]) < 0)
goto cleanup;
if (!(translated = virCPUDefCopyWithoutModel(cpu)))
--
2.21.0
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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@redhat.com>
Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 28, 2019 at 16:27:34 +0100, Ján Tomko wrote:
> 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@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
...
> >@@ -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.
Yeah, I moved it inside the for loop in this patch as it made more sense
to me. I can keep it here if you wish.
>
> >+ 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.
I don't think so, the change is part of the switch from one <signature>
element to several <signature> elements.
>
> > if (rc < 0 || sigFamily == 0) {
> > virReportError(VIR_ERR_INTERNAL_ERROR,
> > _("Invalid CPU signature family in model %s"),
...
> >@@ -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)?
OK, I was too lazy when I was writing this :-)
> I don't think we have a way to avoid b).
> Alternatively, you can format just the model name.
That was in the first (private) version of this patch, but I realized
it's useful to know whether the CPU model was selected because it
matched an existing signature or whether it is just a guess based on the
number of additional features.
Jirka
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2026 Red Hat, Inc.