On Sat, Jun 11, 2022 at 12:46:26 -0400, Cole Robinson wrote:
> Internally we already collect x86 host family + model + stepping
> numeric values. This exposed them in capabilities CPU output.
> Example:
>
> $ sudo virsh capabilities | grep -A1 -B1 signature
> <microcode version='240'/>
> <signature family='6' model='94' stepping='3'/>
> <counter name='tsc' frequency='3408010000' scaling='no'/>
>
> Users need to know these values to calculate an expected.
> SEV-ES/SEV-SNP launch measurement.
>
> Signed-off-by: Cole Robinson <crobinso@xxxxxxxxxx>
> ---
> src/conf/cpu_conf.c | 36 +++++++++++++++++++
> src/conf/cpu_conf.h | 3 ++
> src/conf/schemas/cputypes.rng | 13 +++++++
> src/cpu/cpu_x86.c | 3 ++
> .../x86_64-cpuid-A10-5800K-host.xml | 1 +
> .../x86_64-cpuid-Atom-D510-host.xml | 1 +
> .../x86_64-cpuid-Atom-N450-host.xml | 1 +
> .../x86_64-cpuid-Atom-P5362-host.xml | 1 +
> .../x86_64-cpuid-Cooperlake-host.xml | 1 +
> .../x86_64-cpuid-Core-i5-2500-host.xml | 1 +
> .../x86_64-cpuid-Core-i5-2540M-host.xml | 1 +
> .../x86_64-cpuid-Core-i5-4670T-host.xml | 1 +
> .../x86_64-cpuid-Core-i5-650-host.xml | 1 +
> .../x86_64-cpuid-Core-i5-6600-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-2600-host.xml | 1 +
> ...86_64-cpuid-Core-i7-2600-xsaveopt-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-3520M-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-3740QM-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-3770-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-4510U-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-4600U-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-5600U-arat-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-5600U-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-5600U-ibrs-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-7600U-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-7700-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-8550U-host.xml | 1 +
> .../x86_64-cpuid-Core-i7-8700-host.xml | 1 +
> .../x86_64-cpuid-Core2-E6850-host.xml | 1 +
> .../x86_64-cpuid-Core2-Q9500-host.xml | 1 +
> .../x86_64-cpuid-EPYC-7601-32-Core-host.xml | 1 +
> ...6_64-cpuid-EPYC-7601-32-Core-ibpb-host.xml | 1 +
> .../cputestdata/x86_64-cpuid-FX-8150-host.xml | 1 +
> .../x86_64-cpuid-Opteron-1352-host.xml | 1 +
> .../x86_64-cpuid-Opteron-2350-host.xml | 1 +
> .../x86_64-cpuid-Opteron-6234-host.xml | 1 +
> .../x86_64-cpuid-Opteron-6282-host.xml | 1 +
> .../x86_64-cpuid-Pentium-P6100-host.xml | 1 +
> .../x86_64-cpuid-Phenom-B95-host.xml | 1 +
> ...64-cpuid-Ryzen-7-1800X-Eight-Core-host.xml | 1 +
> .../x86_64-cpuid-Xeon-5110-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E3-1225-v5-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E3-1245-v5-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E5-2609-v3-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E5-2623-v4-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E5-2630-v3-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E5-2630-v4-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E5-2650-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E5-2650-v3-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E5-2650-v4-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E7-4820-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E7-4830-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E7-8890-v3-host.xml | 1 +
> .../x86_64-cpuid-Xeon-E7540-host.xml | 1 +
> .../x86_64-cpuid-Xeon-Gold-5115-host.xml | 1 +
> .../x86_64-cpuid-Xeon-Gold-6130-host.xml | 1 +
> .../x86_64-cpuid-Xeon-Gold-6148-host.xml | 1 +
> .../x86_64-cpuid-Xeon-Platinum-8268-host.xml | 1 +
> .../x86_64-cpuid-Xeon-Platinum-9242-host.xml | 1 +
> .../x86_64-cpuid-Xeon-W3520-host.xml | 1 +
> .../x86_64-cpuid-Xeon-X5460-host.xml | 1 +
> 61 files changed, 112 insertions(+)
>
> diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c
> index 2d447da7c3..1a09e27dca 100644
> --- a/src/conf/cpu_conf.c
> +++ b/src/conf/cpu_conf.c
> @@ -159,6 +159,9 @@ virCPUDefCopyModelFilter(virCPUDef *dst,
> dst->vendor = g_strdup(src->vendor);
> dst->vendor_id = g_strdup(src->vendor_id);
> dst->microcodeVersion = src->microcodeVersion;
> + dst->sigFamily = src->sigFamily;
> + dst->sigModel = src->sigModel;
> + dst->sigStepping = src->sigStepping;
> dst->nfeatures_max = src->nfeatures;
> dst->nfeatures = 0;
>
> @@ -210,6 +213,9 @@ virCPUDefStealModel(virCPUDef *dst,
> dst->model = g_steal_pointer(&src->model);
> dst->features = g_steal_pointer(&src->features);
> dst->microcodeVersion = src->microcodeVersion;
> + dst->sigFamily = src->sigFamily;
> + dst->sigModel = src->sigModel;
> + dst->sigStepping = src->sigStepping;
> dst->nfeatures_max = src->nfeatures_max;
> src->nfeatures_max = 0;
> dst->nfeatures = src->nfeatures;
The CPU signature is introduced only for reporting the host CPU
capability and it is not really related to the "model" part of CPU. Move
the code from the two hunks above to virCPUDefCopyWithoutModel.
> @@ -421,6 +427,7 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
> if (def->type == VIR_CPU_TYPE_HOST) {
> g_autofree char *arch = virXPathString("string(./arch[1])", ctxt);
> xmlNodePtr counter_node = NULL;
> + xmlNodePtr signature_node = NULL;
>
> if (!arch) {
> virReportError(VIR_ERR_XML_ERROR, "%s",
> @@ -441,6 +448,26 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt,
> return -1;
> }
>
> + if ((signature_node = virXPathNode("./signature[1]", ctxt))) {
> + if (virXMLPropUInt(signature_node, "family", 10,
> + VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO,
> + &def->sigFamily) < 0) {
> + return -1;
> + }
> +
> + if (virXMLPropUInt(signature_node, "model", 10,
> + VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO,
> + &def->sigModel) < 0) {
> + return -1;
> + }
> +
> + if (virXMLPropUInt(signature_node, "stepping", 10,
> + VIR_XML_PROP_REQUIRED | VIR_XML_PROP_NONZERO,
> + &def->sigStepping) < 0) {
> + return -1;
> + }
> + }
> +
Searching for 0x00 in tests/cputestdata/*.sig shows:
tests/cputestdata/x86_64-cpuid-EPYC-7502-32-Core.sig:stepping: 0 (0x00)
tests/cputestdata/x86_64-cpuid-Hygon-C86-7185-32-core.sig:model: 0 (0x00)
tests/cputestdata/x86_64-cpuid-Ice-Lake-Server.sig:stepping: 0 (0x00)
tests/cputestdata/x86_64-cpuid-Ryzen-9-3900X-12-Core.sig:stepping: 0 (0x00)
So there are CPUs with stepping or even model equal to 0. The
VIR_XML_PROP_NONZERO flag should only remain for family.
> if ((counter_node = virXPathNode("./counter[@name='tsc']", ctxt))) {
> tsc = g_new0(virHostCPUTscInfo, 1);
>
> @@ -753,6 +780,15 @@ virCPUDefFormatBuf(virBuffer *buf,
> virBufferAsprintf(buf, "<microcode version='%u'/>\n",
> def->microcodeVersion);
>
> + if (def->type == VIR_CPU_TYPE_HOST &&
> + def->sigFamily && def->sigModel && def->sigStepping) {
And you should only check family here as well.
> + virBufferAddLit(buf, "<signature");
> + virBufferAsprintf(buf, " family='%u'", def->sigFamily);
> + virBufferAsprintf(buf, " model='%u'", def->sigModel);
> + virBufferAsprintf(buf, " stepping='%u'", def->sigStepping);
> + virBufferAddLit(buf, "/>\n");
> + }
> +
> if (def->type == VIR_CPU_TYPE_HOST && def->tsc) {
> virBufferAddLit(buf, "<counter name='tsc'");
> virBufferAsprintf(buf, " frequency='%llu'", def->tsc->frequency);
> diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h
> index 2cda4ee1f4..17867d6a30 100644
> --- a/src/conf/cpu_conf.h
> +++ b/src/conf/cpu_conf.h
> @@ -134,6 +134,9 @@ struct _virCPUDef {
> unsigned int dies;
> unsigned int cores;
> unsigned int threads;
> + unsigned int sigFamily;
> + unsigned int sigModel;
> + unsigned int sigStepping;
I'd call them just family, model, and stepping. I think the semantics is
pretty obvious in the virCPUDef context.
> size_t nfeatures;
> size_t nfeatures_max;
> virCPUFeatureDef *features;
> diff --git a/src/conf/schemas/cputypes.rng b/src/conf/schemas/cputypes.rng
> index 056e66e1b4..7c84216240 100644
> --- a/src/conf/schemas/cputypes.rng
> +++ b/src/conf/schemas/cputypes.rng
> @@ -342,6 +342,19 @@
> </attribute>
> </element>
> </optional>
> + <optional>
> + <element name="signature">
> + <attribute name="family">
> + <ref name="positiveInteger"/>
> + </attribute>
> + <attribute name="model">
> + <ref name="positiveInteger"/>
> + </attribute>
> + <attribute name="stepping">
> + <ref name="positiveInteger"/>
> + </attribute>
> + </element>
> + </optional>
Only family should be positiveInteger.
> <optional>
> <element name="counter">
> <attribute name="name">
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index a4aca6055d..c4462f34a7 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -2265,6 +2265,9 @@ x86Decode(virCPUDef *cpu,
> cpuModel->nfeatures = 0;
> cpu->nfeatures_max = cpuModel->nfeatures_max;
> cpuModel->nfeatures_max = 0;
> + cpu->sigFamily = sigFamily;
> + cpu->sigModel = sigModel;
> + cpu->sigStepping = sigStepping;
>
> return 0;
> }
> diff --git a/tests/cputestdata/x86_64-cpuid-A10-5800K-host.xml b/tests/cputestdata/x86_64-cpuid-A10-5800K-host.xml
> index 2430adbfbc..220dfb6467 100644
> --- a/tests/cputestdata/x86_64-cpuid-A10-5800K-host.xml
> +++ b/tests/cputestdata/x86_64-cpuid-A10-5800K-host.xml
...
And because of the check for nonzero model and stepping, the following
files were not updated:
$ git grep -L signature tests/cputestdata/x86_64-cpuid-*-host.xml
tests/cputestdata/x86_64-cpuid-EPYC-7502-32-Core-host.xml
tests/cputestdata/x86_64-cpuid-Hygon-C86-7185-32-core-host.xml
tests/cputestdata/x86_64-cpuid-Ice-Lake-Server-host.xml
tests/cputestdata/x86_64-cpuid-Ryzen-9-3900X-12-Core-host.xml
Jirka
