Re: [libvirt] [PATCH 11/26] cpu_x86: Allow multiple signatures for a CPU model
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(a)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