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@xxxxxxxxxx>
> >---
> > 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
