[libvirt] [PATCH 11/26] cpu_x86: Allow multiple signatures for a CPU model

[Jiri Denemark]

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 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
...
> >@@ -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