[libvirt PATCH v2 00/20] Invalidate the cpu flags cache on host cpuid mismatch
Many things can affect the availability of cpu features (e.g. software upgrades, kernel versions, kernel command line, etc.) and invalidate the cached capabilities without notice. Add CPUID information to the capabilities cache. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1953389. V1: https://listman.redhat.com/archives/libvir-list/2021-August/msg00138.html Tim Wiederhake (20): cpu_x86: Simplify x86ParseCPUID cpu_x86: Simplify x86ParseMSR cpu_x86: Add x86ParseDataItemList cpu: Change virCPUArchDataParse to take xmlNodePtr cpu: Split up virCPUDataParse cpu: Add virCPUDataIsIdentical cpu_x86: Implement virCPUDataIsIdentical for x86 cpu_arm: No need to protect macro definitions cpu_arm: Implement virCPUDataIsIdentical for arm cpu_ppc64: Implement virCPUDataIsIdentical for ppc64 cpu: Add virCPUDataNewCopy cpu_x86: Implement virCPUDataNewCopy for x86 cpu_arm: Implement virCPUDataNewCopy for arm cpu_ppc64: Implement virCPUDataNewCopy for ppc64 virhostcpu: Add virHostCPUGetCPUID cpu_x86: Add virCPUDataGetHost cpu_x86: Implement virCPUDataGetHost for x86 virQEMUCaps: Add host cpuid information virQEMUCapsCachePriv: Add host cpuid information qemu: Invalidate capabilities cache on host cpuid mismatch src/cpu/cpu.c | 97 +++++++++++- src/cpu/cpu.h | 27 +++- src/cpu/cpu_arm.c | 54 ++++++- src/cpu/cpu_ppc64.c | 44 ++++++ src/cpu/cpu_x86.c | 296 +++++++++++++++++++++++------------ src/libvirt_private.syms | 5 + src/qemu/qemu_capabilities.c | 33 +++- src/qemu/qemu_capspriv.h | 3 +- src/util/virhostcpu.c | 43 +++++ src/util/virhostcpu.h | 2 + tests/qemucapsprobe.c | 2 +- 11 files changed, 486 insertions(+), 120 deletions(-) -- 2.31.1
... by using virXMLProp*() helpers. These only require a xmlNodePtr and do not need a xmlXPathContextPtr. Reflect that in the function signature. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_x86.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 1b829e5658..b0f6fa8f34 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1021,37 +1021,28 @@ x86FeatureNames(virCPUx86Map *map, static int -x86ParseCPUID(xmlXPathContextPtr ctxt, +x86ParseCPUID(xmlNodePtr node, virCPUx86DataItem *item) { - virCPUx86CPUID *cpuid; - unsigned long eax_in, ecx_in; - unsigned long eax, ebx, ecx, edx; - int ret_eax_in, ret_ecx_in, ret_eax, ret_ebx, ret_ecx, ret_edx; - - memset(item, 0, sizeof(*item)); + virCPUx86CPUID cpuid; - eax_in = ecx_in = 0; - eax = ebx = ecx = edx = 0; - ret_eax_in = virXPathULongHex("string(@eax_in)", ctxt, &eax_in); - ret_ecx_in = virXPathULongHex("string(@ecx_in)", ctxt, &ecx_in); - ret_eax = virXPathULongHex("string(@eax)", ctxt, &eax); - ret_ebx = virXPathULongHex("string(@ebx)", ctxt, &ebx); - ret_ecx = virXPathULongHex("string(@ecx)", ctxt, &ecx); - ret_edx = virXPathULongHex("string(@edx)", ctxt, &edx); + memset(&cpuid, 0, sizeof(cpuid)); - if (ret_eax_in < 0 || ret_ecx_in == -2 || - ret_eax == -2 || ret_ebx == -2 || ret_ecx == -2 || ret_edx == -2) + if (virXMLPropUInt(node, "eax_in", 0, VIR_XML_PROP_REQUIRED, &cpuid.eax_in) < 0) + return -1; + if (virXMLPropUInt(node, "ecx_in", 0, VIR_XML_PROP_NONE, &cpuid.ecx_in) < 0) + return -1; + if (virXMLPropUInt(node, "eax", 0, VIR_XML_PROP_NONE, &cpuid.eax) < 0) + return -1; + if (virXMLPropUInt(node, "ebx", 0, VIR_XML_PROP_NONE, &cpuid.ebx) < 0) + return -1; + if (virXMLPropUInt(node, "ecx", 0, VIR_XML_PROP_NONE, &cpuid.ecx) < 0) + return -1; + if (virXMLPropUInt(node, "edx", 0, VIR_XML_PROP_NONE, &cpuid.edx) < 0) return -1; item->type = VIR_CPU_X86_DATA_CPUID; - cpuid = &item->data.cpuid; - cpuid->eax_in = eax_in; - cpuid->ecx_in = ecx_in; - cpuid->eax = eax; - cpuid->ebx = ebx; - cpuid->ecx = ecx; - cpuid->edx = edx; + item->data.cpuid = cpuid; return 0; } @@ -1122,7 +1113,7 @@ x86FeatureParse(xmlXPathContextPtr ctxt, for (i = 0; i < n; i++) { ctxt->node = nodes[i]; if (virXMLNodeNameEqual(nodes[i], "cpuid")) { - if (x86ParseCPUID(ctxt, &item) < 0) { + if (x86ParseCPUID(ctxt->node, &item) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid cpuid[%zu] in %s feature"), i, feature->name); @@ -1812,7 +1803,7 @@ virCPUx86DataParse(xmlXPathContextPtr ctxt) for (i = 0; i < n; i++) { ctxt->node = nodes[i]; if (virXMLNodeNameEqual(nodes[i], "cpuid")) { - if (x86ParseCPUID(ctxt, &item) < 0) { + if (x86ParseCPUID(ctxt->node, &item) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse cpuid[%zu]"), i); return NULL; -- 2.31.1
On 11/4/21 5:27 PM, Tim Wiederhake wrote:
... by using virXMLProp*() helpers. These only require a xmlNodePtr and do not need a xmlXPathContextPtr. Reflect that in the function signature.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_x86.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 1b829e5658..b0f6fa8f34 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1021,37 +1021,28 @@ x86FeatureNames(virCPUx86Map *map,
static int -x86ParseCPUID(xmlXPathContextPtr ctxt, +x86ParseCPUID(xmlNodePtr node, virCPUx86DataItem *item) { - virCPUx86CPUID *cpuid; - unsigned long eax_in, ecx_in; - unsigned long eax, ebx, ecx, edx; - int ret_eax_in, ret_ecx_in, ret_eax, ret_ebx, ret_ecx, ret_edx; - - memset(item, 0, sizeof(*item)); + virCPUx86CPUID cpuid;
- eax_in = ecx_in = 0; - eax = ebx = ecx = edx = 0; - ret_eax_in = virXPathULongHex("string(@eax_in)", ctxt, &eax_in); - ret_ecx_in = virXPathULongHex("string(@ecx_in)", ctxt, &ecx_in); - ret_eax = virXPathULongHex("string(@eax)", ctxt, &eax); - ret_ebx = virXPathULongHex("string(@ebx)", ctxt, &ebx); - ret_ecx = virXPathULongHex("string(@ecx)", ctxt, &ecx); - ret_edx = virXPathULongHex("string(@edx)", ctxt, &edx); + memset(&cpuid, 0, sizeof(cpuid));
Alternatively, let compiler clear the variable out in variable declaration: virCPUx86CPUID cpuid = { 0 };
- if (ret_eax_in < 0 || ret_ecx_in == -2 || - ret_eax == -2 || ret_ebx == -2 || ret_ecx == -2 || ret_edx == -2) + if (virXMLPropUInt(node, "eax_in", 0, VIR_XML_PROP_REQUIRED, &cpuid.eax_in) < 0) + return -1; + if (virXMLPropUInt(node, "ecx_in", 0, VIR_XML_PROP_NONE, &cpuid.ecx_in) < 0) + return -1; + if (virXMLPropUInt(node, "eax", 0, VIR_XML_PROP_NONE, &cpuid.eax) < 0) + return -1; + if (virXMLPropUInt(node, "ebx", 0, VIR_XML_PROP_NONE, &cpuid.ebx) < 0) + return -1; + if (virXMLPropUInt(node, "ecx", 0, VIR_XML_PROP_NONE, &cpuid.ecx) < 0) + return -1; + if (virXMLPropUInt(node, "edx", 0, VIR_XML_PROP_NONE, &cpuid.edx) < 0) return -1;
item->type = VIR_CPU_X86_DATA_CPUID; - cpuid = &item->data.cpuid; - cpuid->eax_in = eax_in; - cpuid->ecx_in = ecx_in; - cpuid->eax = eax; - cpuid->ebx = ebx; - cpuid->ecx = ecx; - cpuid->edx = edx; + item->data.cpuid = cpuid; return 0; }
@@ -1122,7 +1113,7 @@ x86FeatureParse(xmlXPathContextPtr ctxt, for (i = 0; i < n; i++) { ctxt->node = nodes[i]; if (virXMLNodeNameEqual(nodes[i], "cpuid")) { - if (x86ParseCPUID(ctxt, &item) < 0) { + if (x86ParseCPUID(ctxt->node, &item) < 0) {
I believe nodes[i] can be passed directly. Because in the next patch you rework the else branch and if that used nodes[i] too then 'ctxt->node = nodes[i]' line could be dropped.
virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid cpuid[%zu] in %s feature"), i, feature->name); @@ -1812,7 +1803,7 @@ virCPUx86DataParse(xmlXPathContextPtr ctxt) for (i = 0; i < n; i++) { ctxt->node = nodes[i]; if (virXMLNodeNameEqual(nodes[i], "cpuid")) { - if (x86ParseCPUID(ctxt, &item) < 0) { + if (x86ParseCPUID(ctxt->node, &item) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse cpuid[%zu]"), i); return NULL;
Same here. But looking more into the future, doesn't matter really, because in pach 03/20 you replace all of this with a function and use xmlNode directly. Michal
On Fri, 2021-11-05 at 16:30 +0100, Michal Prívozník wrote:
On 11/4/21 5:27 PM, Tim Wiederhake wrote:
... by using virXMLProp*() helpers. These only require a xmlNodePtr and do not need a xmlXPathContextPtr. Reflect that in the function signature.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_x86.c | 43 +++++++++++++++++-------------------------- 1 file changed, 17 insertions(+), 26 deletions(-)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 1b829e5658..b0f6fa8f34 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1021,37 +1021,28 @@ x86FeatureNames(virCPUx86Map *map, static int -x86ParseCPUID(xmlXPathContextPtr ctxt, +x86ParseCPUID(xmlNodePtr node, virCPUx86DataItem *item) { - virCPUx86CPUID *cpuid; - unsigned long eax_in, ecx_in; - unsigned long eax, ebx, ecx, edx; - int ret_eax_in, ret_ecx_in, ret_eax, ret_ebx, ret_ecx, ret_edx; - - memset(item, 0, sizeof(*item)); + virCPUx86CPUID cpuid; - eax_in = ecx_in = 0; - eax = ebx = ecx = edx = 0; - ret_eax_in = virXPathULongHex("string(@eax_in)", ctxt, &eax_in); - ret_ecx_in = virXPathULongHex("string(@ecx_in)", ctxt, &ecx_in); - ret_eax = virXPathULongHex("string(@eax)", ctxt, &eax); - ret_ebx = virXPathULongHex("string(@ebx)", ctxt, &ebx); - ret_ecx = virXPathULongHex("string(@ecx)", ctxt, &ecx); - ret_edx = virXPathULongHex("string(@edx)", ctxt, &edx); + memset(&cpuid, 0, sizeof(cpuid));
Alternatively, let compiler clear the variable out in variable declaration:
virCPUx86CPUID cpuid = { 0 };
You are right, that is cleaner. Will change before pushing. Thanks!
- if (ret_eax_in < 0 || ret_ecx_in == -2 || - ret_eax == -2 || ret_ebx == -2 || ret_ecx == -2 || ret_edx == -2) + if (virXMLPropUInt(node, "eax_in", 0, VIR_XML_PROP_REQUIRED, &cpuid.eax_in) < 0) + return -1; + if (virXMLPropUInt(node, "ecx_in", 0, VIR_XML_PROP_NONE, &cpuid.ecx_in) < 0) + return -1; + if (virXMLPropUInt(node, "eax", 0, VIR_XML_PROP_NONE, &cpuid.eax) < 0) + return -1; + if (virXMLPropUInt(node, "ebx", 0, VIR_XML_PROP_NONE, &cpuid.ebx) < 0) + return -1; + if (virXMLPropUInt(node, "ecx", 0, VIR_XML_PROP_NONE, &cpuid.ecx) < 0) + return -1; + if (virXMLPropUInt(node, "edx", 0, VIR_XML_PROP_NONE, &cpuid.edx) < 0) return -1; item->type = VIR_CPU_X86_DATA_CPUID; - cpuid = &item->data.cpuid; - cpuid->eax_in = eax_in; - cpuid->ecx_in = ecx_in; - cpuid->eax = eax; - cpuid->ebx = ebx; - cpuid->ecx = ecx; - cpuid->edx = edx; + item->data.cpuid = cpuid; return 0; } @@ -1122,7 +1113,7 @@ x86FeatureParse(xmlXPathContextPtr ctxt, for (i = 0; i < n; i++) { ctxt->node = nodes[i]; if (virXMLNodeNameEqual(nodes[i], "cpuid")) { - if (x86ParseCPUID(ctxt, &item) < 0) { + if (x86ParseCPUID(ctxt->node, &item) < 0) {
I believe nodes[i] can be passed directly. Because in the next patch you rework the else branch and if that used nodes[i] too then 'ctxt-
node = nodes[i]' line could be dropped.
virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid cpuid[%zu] in %s feature"), i, feature->name); @@ -1812,7 +1803,7 @@ virCPUx86DataParse(xmlXPathContextPtr ctxt) for (i = 0; i < n; i++) { ctxt->node = nodes[i]; if (virXMLNodeNameEqual(nodes[i], "cpuid")) { - if (x86ParseCPUID(ctxt, &item) < 0) { + if (x86ParseCPUID(ctxt->node, &item) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse cpuid[%zu]"), i); return NULL;
Same here. But looking more into the future, doesn't matter really, because in pach 03/20 you replace all of this with a function and use xmlNode directly.
Michal
... by using virXMLProp*() helpers. These only require a xmlNodePtr and do not need a xmlXPathContextPtr. Reflect that in the function signature. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_x86.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index b0f6fa8f34..006221215a 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1048,26 +1048,22 @@ x86ParseCPUID(xmlNodePtr node, static int -x86ParseMSR(xmlXPathContextPtr ctxt, +x86ParseMSR(xmlNodePtr node, virCPUx86DataItem *item) { - virCPUx86MSR *msr; - unsigned long index; - unsigned long eax; - unsigned long edx; + virCPUx86MSR msr; - memset(item, 0, sizeof(*item)); + memset(&msr, 0, sizeof(msr)); - if (virXPathULongHex("string(@index)", ctxt, &index) < 0 || - virXPathULongHex("string(@eax)", ctxt, &eax) < 0 || - virXPathULongHex("string(@edx)", ctxt, &edx) < 0) + if (virXMLPropUInt(node, "index", 0, VIR_XML_PROP_REQUIRED, &msr.index) < 0) + return -1; + if (virXMLPropUInt(node, "eax", 0, VIR_XML_PROP_REQUIRED, &msr.eax) < 0) + return -1; + if (virXMLPropUInt(node, "edx", 0, VIR_XML_PROP_REQUIRED, &msr.edx) < 0) return -1; item->type = VIR_CPU_X86_DATA_MSR; - msr = &item->data.msr; - msr->index = index; - msr->eax = eax; - msr->edx = edx; + item->data.msr = msr; return 0; } @@ -1120,7 +1116,7 @@ x86FeatureParse(xmlXPathContextPtr ctxt, return -1; } } else { - if (x86ParseMSR(ctxt, &item) < 0) { + if (x86ParseMSR(ctxt->node, &item) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid msr[%zu] in %s feature"), i, feature->name); @@ -1809,7 +1805,7 @@ virCPUx86DataParse(xmlXPathContextPtr ctxt) return NULL; } } else { - if (x86ParseMSR(ctxt, &item) < 0) { + if (x86ParseMSR(ctxt->node, &item) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse msr[%zu]"), i); return NULL; -- 2.31.1
Factor out duplicated code from x86FeatureParse and virCPUx86DataParse. This also consolidates error messages. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_x86.c | 106 ++++++++++++++++++---------------------------- 1 file changed, 41 insertions(+), 65 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 006221215a..29b4df1f79 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1068,17 +1068,51 @@ x86ParseMSR(xmlNodePtr node, } +static int +x86ParseDataItemList(virCPUx86Data *cpudata, + xmlNodePtr node) +{ + size_t i; + + if (xmlChildElementCount(node) <= 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no x86 CPU data found")); + return -1; + } + + node = xmlFirstElementChild(node); + for (i = 0; node; ++i) { + virCPUx86DataItem item; + + if (virXMLNodeNameEqual(node, "cpuid")) { + if (x86ParseCPUID(node, &item) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid cpuid[%zu]"), i); + return -1; + } + } else { + if (x86ParseMSR(node, &item) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid msr[%zu]"), i); + return -1; + } + } + + if (virCPUx86DataAddItem(cpudata, &item) < 0) + return -1; + + node = xmlNextElementSibling(node); + } + + return 0; +} + static int x86FeatureParse(xmlXPathContextPtr ctxt, const char *name, void *data) { virCPUx86Map *map = data; - g_autofree xmlNodePtr *nodes = NULL; g_autoptr(virCPUx86Feature) feature = NULL; - virCPUx86DataItem item; - size_t i; - int n; g_autofree char *str = NULL; feature = g_new0(virCPUx86Feature, 1); @@ -1095,38 +1129,8 @@ x86FeatureParse(xmlXPathContextPtr ctxt, if (STREQ_NULLABLE(str, "no")) feature->migratable = false; - n = virXPathNodeSet("./cpuid|./msr", ctxt, &nodes); - if (n < 0) - return -1; - - if (n == 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Missing cpuid or msr element in feature %s"), - feature->name); + if (x86ParseDataItemList(&feature->data, ctxt->node) < 0) return -1; - } - - for (i = 0; i < n; i++) { - ctxt->node = nodes[i]; - if (virXMLNodeNameEqual(nodes[i], "cpuid")) { - if (x86ParseCPUID(ctxt->node, &item) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid cpuid[%zu] in %s feature"), - i, feature->name); - return -1; - } - } else { - if (x86ParseMSR(ctxt->node, &item) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Invalid msr[%zu] in %s feature"), - i, feature->name); - return -1; - } - } - - if (virCPUx86DataAddItem(&feature->data, &item)) - return -1; - } if (!feature->migratable) VIR_APPEND_ELEMENT_COPY(map->migrate_blockers, map->nblockers, feature); @@ -1780,41 +1784,13 @@ virCPUx86DataFormat(const virCPUData *data) static virCPUData * virCPUx86DataParse(xmlXPathContextPtr ctxt) { - g_autofree xmlNodePtr *nodes = NULL; g_autoptr(virCPUData) cpuData = NULL; - virCPUx86DataItem item; - size_t i; - int n; - - n = virXPathNodeSet("/cpudata/cpuid|/cpudata/msr", ctxt, &nodes); - if (n <= 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("no x86 CPU data found")); - return NULL; - } if (!(cpuData = virCPUDataNew(VIR_ARCH_X86_64))) return NULL; - for (i = 0; i < n; i++) { - ctxt->node = nodes[i]; - if (virXMLNodeNameEqual(nodes[i], "cpuid")) { - if (x86ParseCPUID(ctxt->node, &item) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to parse cpuid[%zu]"), i); - return NULL; - } - } else { - if (x86ParseMSR(ctxt->node, &item) < 0) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("failed to parse msr[%zu]"), i); - return NULL; - } - } - - if (virCPUx86DataAdd(cpuData, &item) < 0) - return NULL; - } + if (x86ParseDataItemList(&cpuData->data.x86, ctxt->node) < 0) + return NULL; return g_steal_pointer(&cpuData); } -- 2.31.1
The function does not need a full xmlXPathContextPtr any longer and a later patch will require a call to this function with only a xmlNodePtr available. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu.c | 2 +- src/cpu/cpu.h | 2 +- src/cpu/cpu_x86.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index b4965f3ee0..7823c7cd3d 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -832,7 +832,7 @@ virCPUDataParse(const char *xmlStr) return NULL; } - data = driver->dataParse(ctxt); + data = driver->dataParse(ctxt->node); return data; } diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index d11d2641fc..c4897a33f5 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -98,7 +98,7 @@ typedef char * (*virCPUArchDataFormat)(const virCPUData *data); typedef virCPUData * -(*virCPUArchDataParse)(xmlXPathContextPtr ctxt); +(*virCPUArchDataParse)(xmlNodePtr node); typedef int (*virCPUArchGetModels)(char ***models); diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 29b4df1f79..a08ac225ef 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -1782,14 +1782,14 @@ virCPUx86DataFormat(const virCPUData *data) static virCPUData * -virCPUx86DataParse(xmlXPathContextPtr ctxt) +virCPUx86DataParse(xmlNodePtr node) { g_autoptr(virCPUData) cpuData = NULL; if (!(cpuData = virCPUDataNew(VIR_ARCH_X86_64))) return NULL; - if (x86ParseDataItemList(&cpuData->data.x86, ctxt->node) < 0) + if (x86ParseDataItemList(&cpuData->data.x86, node) < 0) return NULL; return g_steal_pointer(&cpuData); -- 2.31.1
This makes it possible to call virCPUDataParse with a xmlNodePtr, which will be required by a later patch. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu.c | 29 +++++++++++++++++++++-------- src/cpu/cpu.h | 2 ++ src/libvirt_private.syms | 1 + 3 files changed, 24 insertions(+), 8 deletions(-) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 7823c7cd3d..b97d06c7d8 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -803,11 +803,8 @@ virCPUDataFormat(const virCPUData *data) virCPUData * virCPUDataParse(const char *xmlStr) { - struct cpuArchDriver *driver; g_autoptr(xmlDoc) xml = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; - virCPUData *data = NULL; - g_autofree char *arch = NULL; VIR_DEBUG("xmlStr=%s", xmlStr); @@ -817,7 +814,25 @@ virCPUDataParse(const char *xmlStr) return NULL; } - if (!(arch = virXPathString("string(/cpudata/@arch)", ctxt))) { + return virCPUDataParseNode(ctxt->node); +} + + +/** + * virCPUDataParseNode: + * + * @node: XML node as produced by virCPUDataFormat + * + * Parses XML representation of virCPUData structure. + * + * Returns internal CPU data structure parsed from the XML or NULL on error. + */ +virCPUData *virCPUDataParseNode(xmlNodePtr node) +{ + g_autofree char *arch = NULL; + struct cpuArchDriver *driver; + + if (!(arch = virXMLPropString(node, "arch"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("missing CPU data architecture")); return NULL; @@ -827,13 +842,11 @@ virCPUDataParse(const char *xmlStr) return NULL; if (!driver->dataParse) { - virReportError(VIR_ERR_NO_SUPPORT, - _("cannot parse %s CPU data"), arch); + virReportError(VIR_ERR_NO_SUPPORT, _("cannot parse %s CPU data"), arch); return NULL; } - data = driver->dataParse(ctxt->node); - return data; + return driver->dataParse(node); } diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index c4897a33f5..a67af61757 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -285,3 +285,5 @@ char *virCPUDataFormat(const virCPUData *data) ATTRIBUTE_NONNULL(1); virCPUData *virCPUDataParse(const char *xmlStr) ATTRIBUTE_NONNULL(1); +virCPUData *virCPUDataParseNode(xmlNodePtr node) + ATTRIBUTE_NONNULL(1); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 55ae7d5b6f..21a723035d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1462,6 +1462,7 @@ virCPUDataFormat; virCPUDataFree; virCPUDataNew; virCPUDataParse; +virCPUDataParseNode; virCPUExpandFeatures; virCPUGetHost; virCPUGetHostIsSupported; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu.c | 28 ++++++++++++++++++++++++++++ src/cpu/cpu.h | 9 +++++++++ src/libvirt_private.syms | 1 + 3 files changed, 38 insertions(+) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index b97d06c7d8..0c1c7902f0 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -1141,6 +1141,34 @@ virCPUDataAddFeature(virCPUData *cpuData, } +/** + * virCPUDataIsIdentical: + * + * Returns VIR_CPU_COMPARE_IDENTICAL if @a and @b are identical, + * VIR_CPU_COMPARE_INCOMPATIBLE if @a and @b are not identical, or + * VIR_CPU_COMPARE_ERROR on error. + */ +virCPUCompareResult +virCPUDataIsIdentical(const virCPUData *a, + const virCPUData *b) +{ + struct cpuArchDriver *driver; + + VIR_DEBUG("a=%p, b=%p", a, b); + + if (!a || !b) + return VIR_CPU_COMPARE_ERROR; + + if (!(driver = cpuGetSubDriver(a->arch))) + return VIR_CPU_COMPARE_ERROR; + + if (!driver->dataIsIdentical) + return VIR_CPU_COMPARE_ERROR; + + return driver->dataIsIdentical(a, b); +} + + /** * virCPUArchIsSupported: * diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index a67af61757..7ecb0d6921 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -123,6 +123,10 @@ typedef int (*virCPUArchDataAddFeature)(virCPUData *cpuData, const char *name); +typedef virCPUCompareResult +(*virCPUArchDataIsIdentical)(const virCPUData *a, + const virCPUData *b); + struct cpuArchDriver { const char *name; const virArch *arch; @@ -146,6 +150,7 @@ struct cpuArchDriver { virCPUArchCopyMigratable copyMigratable; virCPUArchValidateFeatures validateFeatures; virCPUArchDataAddFeature dataAddFeature; + virCPUArchDataIsIdentical dataIsIdentical; }; @@ -275,6 +280,10 @@ int virCPUDataAddFeature(virCPUData *cpuData, const char *name); +virCPUCompareResult +virCPUDataIsIdentical(const virCPUData *a, + const virCPUData *b); + bool virCPUArchIsSupported(virArch arch); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 21a723035d..3b6645b4a8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1460,6 +1460,7 @@ virCPUDataAddFeature; virCPUDataCheckFeature; virCPUDataFormat; virCPUDataFree; +virCPUDataIsIdentical; virCPUDataNew; virCPUDataParse; virCPUDataParseNode; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_x86.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index a08ac225ef..5ce193e693 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -3332,6 +3332,74 @@ virCPUx86DataAddFeature(virCPUData *cpuData, } +static bool +virCPUx86DataItemIsIdentical(const virCPUx86DataItem *a, + const virCPUx86DataItem *b) +{ + if (a->type != b->type) + return false; + + switch (a->type) { + case VIR_CPU_X86_DATA_NONE: + break; + + case VIR_CPU_X86_DATA_CPUID: + return a->data.cpuid.eax_in == b->data.cpuid.eax_in && + a->data.cpuid.ecx_in == b->data.cpuid.ecx_in && + a->data.cpuid.eax == b->data.cpuid.eax && + a->data.cpuid.ebx == b->data.cpuid.ebx && + a->data.cpuid.ecx == b->data.cpuid.ecx && + a->data.cpuid.edx == b->data.cpuid.edx; + + case VIR_CPU_X86_DATA_MSR: + return a->data.msr.index == b->data.msr.index && + a->data.msr.eax == b->data.msr.eax && + a->data.msr.edx == b->data.msr.edx; + } + + return true; +} + +static virCPUCompareResult +virCPUx86DataIsIdentical(const virCPUData *a, + const virCPUData *b) +{ + const virCPUx86Data *adata; + const virCPUx86Data *bdata; + size_t i; + size_t j; + + if (!a || !b) + return VIR_CPU_COMPARE_ERROR; + + if (a->arch != b->arch) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + if (!((adata = &a->data.x86) && (bdata = &b->data.x86))) + return VIR_CPU_COMPARE_ERROR; + + if (adata->len != bdata->len) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + for (i = 0; i < adata->len; ++i) { + bool found = false; + + for (j = 0; j < bdata->len; ++j) { + if (!virCPUx86DataItemIsIdentical(&adata->items[i], + &bdata->items[j])) + continue; + + found = true; + break; + } + + if (!found) + return VIR_CPU_COMPARE_INCOMPATIBLE; + } + + return VIR_CPU_COMPARE_IDENTICAL; +} + static bool virCPUx86FeatureIsMSR(const char *name) { @@ -3415,4 +3483,5 @@ struct cpuArchDriver cpuDriverX86 = { .copyMigratable = virCPUx86CopyMigratable, .validateFeatures = virCPUx86ValidateFeatures, .dataAddFeature = virCPUx86DataAddFeature, + .dataIsIdentical = virCPUx86DataIsIdentical, }; -- 2.31.1
On 11/4/21 5:27 PM, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_x86.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index a08ac225ef..5ce193e693 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -3332,6 +3332,74 @@ virCPUx86DataAddFeature(virCPUData *cpuData, }
+static bool +virCPUx86DataItemIsIdentical(const virCPUx86DataItem *a, + const virCPUx86DataItem *b) +{ + if (a->type != b->type) + return false; + + switch (a->type) { + case VIR_CPU_X86_DATA_NONE: + break; + + case VIR_CPU_X86_DATA_CPUID: + return a->data.cpuid.eax_in == b->data.cpuid.eax_in && + a->data.cpuid.ecx_in == b->data.cpuid.ecx_in && + a->data.cpuid.eax == b->data.cpuid.eax && + a->data.cpuid.ebx == b->data.cpuid.ebx && + a->data.cpuid.ecx == b->data.cpuid.ecx && + a->data.cpuid.edx == b->data.cpuid.edx;
So this can be replaced with memcmp(), but the moment we will want to store a pointer in .cpuid we will have to rewrite the code to this explicit variant. So I guess keep it as is?
+ + case VIR_CPU_X86_DATA_MSR: + return a->data.msr.index == b->data.msr.index && + a->data.msr.eax == b->data.msr.eax && + a->data.msr.edx == b->data.msr.edx; + } + + return true; +} + +static virCPUCompareResult +virCPUx86DataIsIdentical(const virCPUData *a, + const virCPUData *b) +{ + const virCPUx86Data *adata; + const virCPUx86Data *bdata; + size_t i; + size_t j; + + if (!a || !b) + return VIR_CPU_COMPARE_ERROR; + + if (a->arch != b->arch) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + if (!((adata = &a->data.x86) && (bdata = &b->data.x86))) + return VIR_CPU_COMPARE_ERROR; + + if (adata->len != bdata->len) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + for (i = 0; i < adata->len; ++i) { + bool found = false; + + for (j = 0; j < bdata->len; ++j) { + if (!virCPUx86DataItemIsIdentical(&adata->items[i], + &bdata->items[j])) + continue; + + found = true; + break; + } + + if (!found) + return VIR_CPU_COMPARE_INCOMPATIBLE;
Do we need this? I mean, couldn't we replace 'found = true' with ;return VIR_CPU_COMPARE_IDENTICAL;' Or even better, drop the negation in if and return the identical value instead of continue.
+ } + + return VIR_CPU_COMPARE_IDENTICAL;
This can then be INCOMPATIBLE.
+} + static bool virCPUx86FeatureIsMSR(const char *name) { @@ -3415,4 +3483,5 @@ struct cpuArchDriver cpuDriverX86 = { .copyMigratable = virCPUx86CopyMigratable, .validateFeatures = virCPUx86ValidateFeatures, .dataAddFeature = virCPUx86DataAddFeature, + .dataIsIdentical = virCPUx86DataIsIdentical, };
Michal
On Fri, 2021-11-05 at 16:30 +0100, Michal Prívozník wrote:
On 11/4/21 5:27 PM, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_x86.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+)
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index a08ac225ef..5ce193e693 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -3332,6 +3332,74 @@ virCPUx86DataAddFeature(virCPUData *cpuData, } +static bool +virCPUx86DataItemIsIdentical(const virCPUx86DataItem *a, + const virCPUx86DataItem *b) +{ + if (a->type != b->type) + return false; + + switch (a->type) { + case VIR_CPU_X86_DATA_NONE: + break; + + case VIR_CPU_X86_DATA_CPUID: + return a->data.cpuid.eax_in == b->data.cpuid.eax_in && + a->data.cpuid.ecx_in == b->data.cpuid.ecx_in && + a->data.cpuid.eax == b->data.cpuid.eax && + a->data.cpuid.ebx == b->data.cpuid.ebx && + a->data.cpuid.ecx == b->data.cpuid.ecx && + a->data.cpuid.edx == b->data.cpuid.edx;
So this can be replaced with memcmp(), but the moment we will want to store a pointer in .cpuid we will have to rewrite the code to this explicit variant. So I guess keep it as is?
Thanks, I somehow overlooked that. Will replace this ...
+ + case VIR_CPU_X86_DATA_MSR: + return a->data.msr.index == b->data.msr.index && + a->data.msr.eax == b->data.msr.eax && + a->data.msr.edx == b->data.msr.edx; + } +
... and this with calls to `memcmp() == 0` before pushing.
+ return true; +} + +static virCPUCompareResult +virCPUx86DataIsIdentical(const virCPUData *a, + const virCPUData *b) +{ + const virCPUx86Data *adata; + const virCPUx86Data *bdata; + size_t i; + size_t j; + + if (!a || !b) + return VIR_CPU_COMPARE_ERROR; + + if (a->arch != b->arch) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + if (!((adata = &a->data.x86) && (bdata = &b->data.x86))) + return VIR_CPU_COMPARE_ERROR; + + if (adata->len != bdata->len) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + for (i = 0; i < adata->len; ++i) { + bool found = false; + + for (j = 0; j < bdata->len; ++j) { + if (!virCPUx86DataItemIsIdentical(&adata->items[i], + &bdata->items[j])) + continue; + + found = true; + break; + } + + if (!found) + return VIR_CPU_COMPARE_INCOMPATIBLE;
Do we need this? I mean, couldn't we replace 'found = true' with ;return VIR_CPU_COMPARE_IDENTICAL;' Or even better, drop the negation in if and return the identical value instead of continue.
The order of entries is not guaranteed, this is why we cannot compare the entries of "adata" and "bdata" one-by-one (O(n)), but check for every entry in "adata" whether there is an identical entry in "bdata" (O(n²)). At the "found = true;" line we only know that the i-th element in "adata" has an identical entry in "bdata"; but we still need to check elements "i+1" to "adata->len". Cheers, Tim
+ } + + return VIR_CPU_COMPARE_IDENTICAL;
This can then be INCOMPATIBLE.
+} + static bool virCPUx86FeatureIsMSR(const char *name) { @@ -3415,4 +3483,5 @@ struct cpuArchDriver cpuDriverX86 = { .copyMigratable = virCPUx86CopyMigratable, .validateFeatures = virCPUx86ValidateFeatures, .dataAddFeature = virCPUx86DataAddFeature, + .dataIsIdentical = virCPUx86DataIsIdentical, };
Michal
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_arm.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c index 09ade1d422..ac174891b7 100644 --- a/src/cpu/cpu_arm.c +++ b/src/cpu/cpu_arm.c @@ -39,12 +39,11 @@ #include "virxml.h" #define VIR_FROM_THIS VIR_FROM_CPU -#if defined(__aarch64__) + /* Shift bit mask for parsing cpu flags */ -# define BIT_SHIFTS(n) (1UL << (n)) +#define BIT_SHIFTS(n) (1UL << (n)) /* The current max number of cpu flags on ARM is 32 */ -# define MAX_CPU_FLAGS 32 -#endif +#define MAX_CPU_FLAGS 32 VIR_LOG_INIT("cpu.cpu_arm"); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_arm.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c index ac174891b7..c9114d53bf 100644 --- a/src/cpu/cpu_arm.c +++ b/src/cpu/cpu_arm.c @@ -124,6 +124,32 @@ virCPUarmDataClear(virCPUarmData *data) g_strfreev(data->features); } +static virCPUCompareResult +virCPUarmDataIsIdentical(const virCPUData *a, + const virCPUData *b) +{ + size_t i; + + if (!a || !b) + return VIR_CPU_COMPARE_ERROR; + + if (a->arch != b->arch) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + if (a->data.arm.pvr != b->data.arm.pvr) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + if (a->data.arm.vendor_id != b->data.arm.vendor_id) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + for (i = 0; i < MAX_CPU_FLAGS; ++i) { + if (STRNEQ(a->data.arm.features[i], b->data.arm.features[i])) + return VIR_CPU_COMPARE_INCOMPATIBLE; + } + + return VIR_CPU_COMPARE_IDENTICAL; +} + static void virCPUarmDataFree(virCPUData *cpuData) { @@ -674,4 +700,5 @@ struct cpuArchDriver cpuDriverArm = { .baseline = virCPUarmBaseline, .update = virCPUarmUpdate, .validateFeatures = virCPUarmValidateFeatures, + .dataIsIdentical = virCPUarmDataIsIdentical, }; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_ppc64.c | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 4909f61ff1..fcd68c8a7c 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -578,6 +578,31 @@ ppc64DriverDecode(virCPUDef *cpu, return 0; } +static virCPUCompareResult +virCPUppc64DataIsIdentical(const virCPUData *a, + const virCPUData *b) +{ + size_t i; + + if (!a || !b) + return VIR_CPU_COMPARE_ERROR; + + if (a->arch != b->arch) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + if (a->data.ppc64.len != b->data.ppc64.len) + return VIR_CPU_COMPARE_INCOMPATIBLE; + + for (i = 0; i < a->data.ppc64.len; ++i) { + if (a->data.ppc64.pvr[i].mask != b->data.ppc64.pvr[i].mask) + return VIR_CPU_COMPARE_INCOMPATIBLE; + if (a->data.ppc64.pvr[i].value != b->data.ppc64.pvr[i].value) + return VIR_CPU_COMPARE_INCOMPATIBLE; + } + + return VIR_CPU_COMPARE_IDENTICAL; +} + static void virCPUppc64DataFree(virCPUData *data) { @@ -749,4 +774,5 @@ struct cpuArchDriver cpuDriverPPC64 = { .update = virCPUppc64Update, .getModels = virCPUppc64DriverGetModels, .convertLegacy = virCPUppc64ConvertLegacy, + .dataIsIdentical = virCPUppc64DataIsIdentical, }; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu.c | 21 +++++++++++++++++++++ src/cpu/cpu.h | 7 +++++++ src/libvirt_private.syms | 1 + 3 files changed, 29 insertions(+) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 0c1c7902f0..b5669246b4 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -294,6 +294,27 @@ virCPUDataNew(virArch arch) } +/** + * virCPUDataNewCopy: + * + * Returns a copy of @data or NULL on error. + */ +virCPUData * +virCPUDataNewCopy(virCPUData *data) +{ + struct cpuArchDriver *driver; + + VIR_DEBUG("data=%p", data); + + if (!data) + return NULL; + + if ((driver = cpuGetSubDriver(data->arch)) && driver->dataCopyNew) + return driver->dataCopyNew(data); + + return NULL; +} + /** * virCPUDataFree: * diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 7ecb0d6921..d3e5ca79f2 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -62,6 +62,9 @@ typedef int virCPUData **forbidden, virCPUData **vendor); +typedef virCPUData * +(*cpuArchDataCopyNew)(virCPUData *data); + typedef void (*cpuArchDataFree) (virCPUData *data); @@ -134,6 +137,7 @@ struct cpuArchDriver { virCPUArchCompare compare; cpuArchDecode decode; cpuArchEncode encode; + cpuArchDataCopyNew dataCopyNew; cpuArchDataFree dataFree; virCPUArchGetHost getHost; virCPUArchBaseline baseline; @@ -188,6 +192,9 @@ cpuEncode (virArch arch, virCPUData * virCPUDataNew(virArch arch); +virCPUData * +virCPUDataNewCopy(virCPUData *data); + void virCPUDataFree(virCPUData *data); G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCPUData, virCPUDataFree); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b6645b4a8..665db255a6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1462,6 +1462,7 @@ virCPUDataFormat; virCPUDataFree; virCPUDataIsIdentical; virCPUDataNew; +virCPUDataNewCopy; virCPUDataParse; virCPUDataParseNode; virCPUExpandFeatures; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_x86.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 5ce193e693..0dc9a7d9fb 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -481,6 +481,23 @@ virCPUx86DataClear(virCPUx86Data *data) G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(virCPUx86Data, virCPUx86DataClear); +static virCPUData * +virCPUx86DataCopyNew(virCPUData *data) +{ + virCPUData *copy; + + if (!data) + return NULL; + + copy = virCPUDataNew(data->arch); + copy->data.x86.len = data->data.x86.len; + copy->data.x86.items = g_new0(virCPUx86DataItem, data->data.x86.len); + memcpy(copy->data.x86.items, data->data.x86.items, + data->data.x86.len * sizeof(*data->data.x86.items)); + + return copy; +} + static void virCPUx86DataFree(virCPUData *data) { @@ -3466,6 +3483,7 @@ struct cpuArchDriver cpuDriverX86 = { .compare = virCPUx86Compare, .decode = x86DecodeCPUData, .encode = x86Encode, + .dataCopyNew = virCPUx86DataCopyNew, .dataFree = virCPUx86DataFree, #if defined(__i386__) || defined(__x86_64__) .getHost = virCPUx86GetHost, -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_arm.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c index c9114d53bf..51a3c1f3ee 100644 --- a/src/cpu/cpu_arm.c +++ b/src/cpu/cpu_arm.c @@ -150,6 +150,25 @@ virCPUarmDataIsIdentical(const virCPUData *a, return VIR_CPU_COMPARE_IDENTICAL; } +static virCPUData * +virCPUarmDataCopyNew(virCPUData *data) +{ + virCPUData *copy; + size_t i; + + if (!data) + return NULL; + + copy = virCPUDataNew(data->arch); + copy->data.arm.pvr = data->data.arm.pvr; + copy->data.arm.vendor_id = data->data.arm.vendor_id; + copy->data.arm.features = g_new0(char *, MAX_CPU_FLAGS + 1); + for (i = 0; i < MAX_CPU_FLAGS; ++i) + copy->data.arm.features[i] = g_strdup(data->data.arm.features[i]); + + return copy; +} + static void virCPUarmDataFree(virCPUData *cpuData) { @@ -696,6 +715,7 @@ struct cpuArchDriver cpuDriverArm = { #endif .decode = NULL, .encode = NULL, + .dataCopyNew = virCPUarmDataCopyNew, .dataFree = virCPUarmDataFree, .baseline = virCPUarmBaseline, .update = virCPUarmUpdate, -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_ppc64.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index fcd68c8a7c..c7caaa9608 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -603,6 +603,23 @@ virCPUppc64DataIsIdentical(const virCPUData *a, return VIR_CPU_COMPARE_IDENTICAL; } +static virCPUData * +virCPUppc64DataCopyNew(virCPUData *data) +{ + virCPUData *copy; + size_t i; + + if (!data) + return NULL; + + copy = virCPUDataNew(data->arch); + copy->data.ppc64.len = data->data.ppc64.len; + for (i = 0; i < copy->data.ppc64.len; ++i) + copy->data.ppc64.pvr[i] = data->data.ppc64.pvr[i]; + + return copy; +} + static void virCPUppc64DataFree(virCPUData *data) { @@ -768,6 +785,7 @@ struct cpuArchDriver cpuDriverPPC64 = { .compare = virCPUppc64Compare, .decode = ppc64DriverDecode, .encode = NULL, + .dataCopyNew = virCPUppc64DataCopyNew, .dataFree = virCPUppc64DataFree, .getHost = virCPUppc64GetHost, .baseline = virCPUppc64Baseline, -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virhostcpu.c | 43 ++++++++++++++++++++++++++++++++++++++++ src/util/virhostcpu.h | 2 ++ 3 files changed, 46 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 665db255a6..e92670c85d 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2396,6 +2396,7 @@ virHookPresent; # util/virhostcpu.h virHostCPUGetAvailableCPUsBitmap; virHostCPUGetCount; +virHostCPUGetCPUID; virHostCPUGetHaltPollTime; virHostCPUGetInfo; virHostCPUGetKVMMaxVCPUs; diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 5dd2baf2df..3563a662d5 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -1336,6 +1336,41 @@ virHostCPUGetMSR(unsigned long index, } +struct kvm_cpuid2 * +virHostCPUGetCPUID(void) +{ + size_t i; + VIR_AUTOCLOSE fd = open(KVM_DEVICE, O_RDONLY); + + if (fd < 0) { + virReportSystemError(errno, _("Unable to open %s"), KVM_DEVICE); + return NULL; + } + + for (i = 1; i < INT32_MAX; i *= 2) { + g_autofree struct kvm_cpuid2 *kvm_cpuid = NULL; + kvm_cpuid = g_malloc0(sizeof(struct kvm_cpuid2) + + sizeof(struct kvm_cpuid_entry2) * i); + kvm_cpuid->nent = i; + + if (ioctl(fd, KVM_GET_SUPPORTED_CPUID, kvm_cpuid) == 0) { + /* filter out local apic id */ + for (i = 0; i < kvm_cpuid->nent; ++i) { + struct kvm_cpuid_entry2 *entry = &kvm_cpuid->entries[i]; + if (entry->function == 0x01 && entry->index == 0x00) + entry->ebx &= 0x00ffffff; + if (entry->function == 0x0b) + entry->edx &= 0xffffff00; + } + + return g_steal_pointer(&kvm_cpuid); + } + } + + virReportSystemError(errno, "%s", _("Cannot read host CPUID")); + return NULL; +} + /* * This function should only be called when the host CPU supports invariant TSC * (invtsc CPUID feature). @@ -1391,6 +1426,14 @@ virHostCPUGetTscInfo(void) #else +struct kvm_cpuid2 * +virHostCPUGetCPUID(void) +{ + virReportSystemError(ENOSYS, "%s", + _("Reading CPUID is not supported on this platform")); + return NULL; +} + int virHostCPUGetMSR(unsigned long index G_GNUC_UNUSED, uint64_t *msr G_GNUC_UNUSED) diff --git a/src/util/virhostcpu.h b/src/util/virhostcpu.h index d98385d53f..a96dd5afba 100644 --- a/src/util/virhostcpu.h +++ b/src/util/virhostcpu.h @@ -80,6 +80,8 @@ virHostCPUGetMicrocodeVersion(virArch hostArch) G_GNUC_NO_INLINE; int virHostCPUGetMSR(unsigned long index, uint64_t *msr); +struct kvm_cpuid2 *virHostCPUGetCPUID(void); + virHostCPUTscInfo *virHostCPUGetTscInfo(void); int virHostCPUGetSignature(char **signature); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu.c | 19 +++++++++++++++++++ src/cpu/cpu.h | 7 +++++++ src/libvirt_private.syms | 1 + 3 files changed, 27 insertions(+) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index b5669246b4..285c7eee44 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -1190,6 +1190,25 @@ virCPUDataIsIdentical(const virCPUData *a, } +/** + * virCPUDataGetHost: + * + */ +virCPUData* +virCPUDataGetHost(void) +{ + struct cpuArchDriver *driver; + + if (!(driver = cpuGetSubDriver(virArchFromHost()))) + return NULL; + + if (!driver->dataGetHost) + return NULL; + + return driver->dataGetHost(); +} + + /** * virCPUArchIsSupported: * diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index d3e5ca79f2..071b33fe76 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -130,6 +130,9 @@ typedef virCPUCompareResult (*virCPUArchDataIsIdentical)(const virCPUData *a, const virCPUData *b); +typedef virCPUData * +(*virCPUArchDataGetHost)(void); + struct cpuArchDriver { const char *name; const virArch *arch; @@ -155,6 +158,7 @@ struct cpuArchDriver { virCPUArchValidateFeatures validateFeatures; virCPUArchDataAddFeature dataAddFeature; virCPUArchDataIsIdentical dataIsIdentical; + virCPUArchDataGetHost dataGetHost; }; @@ -291,6 +295,9 @@ virCPUCompareResult virCPUDataIsIdentical(const virCPUData *a, const virCPUData *b); +virCPUData* +virCPUDataGetHost(void); + bool virCPUArchIsSupported(virArch arch); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e92670c85d..d4e2d3db74 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1460,6 +1460,7 @@ virCPUDataAddFeature; virCPUDataCheckFeature; virCPUDataFormat; virCPUDataFree; +virCPUDataGetHost; virCPUDataIsIdentical; virCPUDataNew; virCPUDataNewCopy; -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/cpu/cpu_x86.c | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 0dc9a7d9fb..a72eae07dd 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -20,6 +20,9 @@ #include <config.h> +#if WITH_LINUX_KVM_H +# include <linux/kvm.h> +#endif #include "virlog.h" #include "viralloc.h" @@ -3417,6 +3420,38 @@ virCPUx86DataIsIdentical(const virCPUData *a, return VIR_CPU_COMPARE_IDENTICAL; } +#if WITH_LINUX_KVM_H && defined(KVM_GET_MSRS) && \ + (defined(__i386__) || defined(__x86_64__)) && \ + (defined(__linux__) || defined(__FreeBSD__)) +static virCPUData * +virCPUx86DataGetHost(void) +{ + size_t i; + virCPUData *cpuid; + g_autofree struct kvm_cpuid2 *kvm_cpuid = NULL; + + if ((kvm_cpuid = virHostCPUGetCPUID()) == NULL) + return NULL; + + cpuid = virCPUDataNew(virArchFromHost()); + cpuid->data.x86.len = kvm_cpuid->nent; + cpuid->data.x86.items = g_new0(virCPUx86DataItem, kvm_cpuid->nent); + + for (i = 0; i < kvm_cpuid->nent; ++i) { + virCPUx86DataItem *item = &cpuid->data.x86.items[i]; + item->type = VIR_CPU_X86_DATA_CPUID; + item->data.cpuid.eax_in = kvm_cpuid->entries[i].function; + item->data.cpuid.ecx_in = kvm_cpuid->entries[i].index; + item->data.cpuid.eax = kvm_cpuid->entries[i].eax; + item->data.cpuid.ebx = kvm_cpuid->entries[i].ebx; + item->data.cpuid.ecx = kvm_cpuid->entries[i].ecx; + item->data.cpuid.edx = kvm_cpuid->entries[i].edx; + } + + return cpuid; +} +#endif + static bool virCPUx86FeatureIsMSR(const char *name) { @@ -3502,4 +3537,9 @@ struct cpuArchDriver cpuDriverX86 = { .validateFeatures = virCPUx86ValidateFeatures, .dataAddFeature = virCPUx86DataAddFeature, .dataIsIdentical = virCPUx86DataIsIdentical, +#if WITH_LINUX_KVM_H && defined(KVM_GET_MSRS) && \ + (defined(__i386__) || defined(__x86_64__)) && \ + (defined(__linux__) || defined(__FreeBSD__)) + .dataGetHost = virCPUx86DataGetHost, +#endif }; -- 2.31.1
Many things can affect the availability of cpu flags (e.g. software upgrades, kernel versions, kernel command line, etc.) and invalidate the cached capabilities without notice. Add CPUID information to the capabilities cache. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_capabilities.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index e4fdd9a1eb..b2d5242264 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -725,6 +725,7 @@ struct _virQEMUCaps { char *kernelVersion; virArch arch; + virCPUData *cpuData; size_t ngicCapabilities; virGICCapability *gicCapabilities; @@ -1965,6 +1966,7 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps *qemuCaps) ret->kernelVersion = g_strdup(qemuCaps->kernelVersion); ret->arch = qemuCaps->arch; + ret->cpuData = virCPUDataNewCopy(qemuCaps->cpuData); if (virQEMUCapsAccelCopy(&ret->kvm, &qemuCaps->kvm) < 0 || virQEMUCapsAccelCopy(&ret->tcg, &qemuCaps->tcg) < 0) @@ -2015,6 +2017,8 @@ void virQEMUCapsDispose(void *obj) g_free(qemuCaps->gicCapabilities); + virCPUDataFree(qemuCaps->cpuData); + virSEVCapabilitiesFree(qemuCaps->sevCapabilities); virQEMUCapsAccelClear(&qemuCaps->kvm); @@ -4260,6 +4264,12 @@ virQEMUCapsLoadCache(virArch hostArch, } VIR_FREE(str); + if (virXPathBoolean("boolean(./cpudata)", ctxt) > 0) { + qemuCaps->cpuData = virCPUDataParseNode(virXPathNode("./cpudata", ctxt)); + if (!qemuCaps->cpuData) + goto cleanup; + } + if (virQEMUCapsLoadAccel(qemuCaps, ctxt, VIR_DOMAIN_VIRT_KVM) < 0 || virQEMUCapsLoadAccel(qemuCaps, ctxt, VIR_DOMAIN_VIRT_QEMU) < 0) goto cleanup; @@ -4561,6 +4571,11 @@ virQEMUCapsFormatCache(virQEMUCaps *qemuCaps) virBufferAsprintf(&buf, "<kernelVersion>%s</kernelVersion>\n", qemuCaps->kernelVersion); + if (qemuCaps->cpuData) { + g_autofree char * cpudata = virCPUDataFormat(qemuCaps->cpuData); + virBufferAsprintf(&buf, "%s", cpudata); + } + virBufferAsprintf(&buf, "<arch>%s</arch>\n", virArchToString(qemuCaps->arch)); @@ -5418,6 +5433,7 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { qemuCaps->hostCPUSignature = g_strdup(hostCPUSignature); qemuCaps->microcodeVersion = microcodeVersion; + qemuCaps->cpuData = NULL; qemuCaps->kernelVersion = g_strdup(kernelVersion); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b2d5242264..5fa3111201 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4023,6 +4023,7 @@ struct _virQEMUCapsCachePriv { gid_t runGid; virArch hostArch; unsigned int microcodeVersion; + virCPUData *cpuData; char *kernelVersion; char *hostCPUSignature; @@ -4040,6 +4041,7 @@ virQEMUCapsCachePrivFree(void *privData) g_free(priv->libDir); g_free(priv->kernelVersion); + virCPUDataFree(priv->cpuData); g_free(priv->hostCPUSignature); g_free(priv); } @@ -5568,6 +5570,8 @@ virQEMUCapsCacheNew(const char *libDir, if (uname(&uts) == 0) priv->kernelVersion = g_strdup_printf("%s %s", uts.release, uts.version); + priv->cpuData = virCPUDataGetHost(); + cleanup: return cache; -- 2.31.1
See https://bugzilla.redhat.com/show_bug.cgi?id=1953389. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/qemu/qemu_capabilities.c | 15 ++++++++++++--- src/qemu/qemu_capspriv.h | 3 ++- tests/qemucapsprobe.c | 2 +- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 5fa3111201..67fae46a34 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4907,6 +4907,13 @@ virQEMUCapsIsValid(void *data, return false; } + if (virCPUDataIsIdentical(priv->cpuData, qemuCaps->cpuData) != + VIR_CPU_COMPARE_IDENTICAL) { + VIR_DEBUG("Outdated capabilities for '%s': host cpuid changed", + qemuCaps->binary); + return false; + } + kvmSupportsNesting = virQEMUCapsKVMSupportsNesting(); if (kvmSupportsNesting != qemuCaps->kvmSupportsNesting) { VIR_DEBUG("Outdated capabilities for '%s': kvm kernel nested " @@ -5387,7 +5394,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, gid_t runGid, const char *hostCPUSignature, unsigned int microcodeVersion, - const char *kernelVersion) + const char *kernelVersion, + virCPUData* cpuData) { g_autoptr(virQEMUCaps) qemuCaps = NULL; struct stat sb; @@ -5435,7 +5443,7 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { qemuCaps->hostCPUSignature = g_strdup(hostCPUSignature); qemuCaps->microcodeVersion = microcodeVersion; - qemuCaps->cpuData = NULL; + qemuCaps->cpuData = virCPUDataNewCopy(cpuData); qemuCaps->kernelVersion = g_strdup(kernelVersion); @@ -5460,7 +5468,8 @@ virQEMUCapsNewData(const char *binary, priv->runGid, priv->hostCPUSignature, virHostCPUGetMicrocodeVersion(priv->hostArch), - priv->kernelVersion); + priv->kernelVersion, + priv->cpuData); } diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index a54a22685e..f4f4a99d32 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -35,7 +35,8 @@ virQEMUCapsNewForBinaryInternal(virArch hostArch, gid_t runGid, const char *hostCPUSignature, unsigned int microcodeVersion, - const char *kernelVersion); + const char *kernelVersion, + virCPUData* cpuData); int virQEMUCapsLoadCache(virArch hostArch, virQEMUCaps *qemuCaps, diff --git a/tests/qemucapsprobe.c b/tests/qemucapsprobe.c index bfa8ae8db7..76c18f0dcd 100644 --- a/tests/qemucapsprobe.c +++ b/tests/qemucapsprobe.c @@ -79,7 +79,7 @@ main(int argc, char **argv) return EXIT_FAILURE; if (!(caps = virQEMUCapsNewForBinaryInternal(VIR_ARCH_NONE, argv[1], "/tmp", - -1, -1, NULL, 0, NULL))) + -1, -1, NULL, 0, NULL, NULL))) return EXIT_FAILURE; host = virArchFromHost(); -- 2.31.1
On 11/4/21 5:27 PM, Tim Wiederhake wrote:
Many things can affect the availability of cpu features (e.g. software upgrades, kernel versions, kernel command line, etc.) and invalidate the cached capabilities without notice. Add CPUID information to the capabilities cache.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1953389.
V1: https://listman.redhat.com/archives/libvir-list/2021-August/msg00138.html
Tim Wiederhake (20): cpu_x86: Simplify x86ParseCPUID cpu_x86: Simplify x86ParseMSR cpu_x86: Add x86ParseDataItemList cpu: Change virCPUArchDataParse to take xmlNodePtr cpu: Split up virCPUDataParse cpu: Add virCPUDataIsIdentical cpu_x86: Implement virCPUDataIsIdentical for x86 cpu_arm: No need to protect macro definitions cpu_arm: Implement virCPUDataIsIdentical for arm cpu_ppc64: Implement virCPUDataIsIdentical for ppc64 cpu: Add virCPUDataNewCopy cpu_x86: Implement virCPUDataNewCopy for x86 cpu_arm: Implement virCPUDataNewCopy for arm cpu_ppc64: Implement virCPUDataNewCopy for ppc64 virhostcpu: Add virHostCPUGetCPUID cpu_x86: Add virCPUDataGetHost cpu_x86: Implement virCPUDataGetHost for x86 virQEMUCaps: Add host cpuid information virQEMUCapsCachePriv: Add host cpuid information qemu: Invalidate capabilities cache on host cpuid mismatch
src/cpu/cpu.c | 97 +++++++++++- src/cpu/cpu.h | 27 +++- src/cpu/cpu_arm.c | 54 ++++++- src/cpu/cpu_ppc64.c | 44 ++++++ src/cpu/cpu_x86.c | 296 +++++++++++++++++++++++------------ src/libvirt_private.syms | 5 + src/qemu/qemu_capabilities.c | 33 +++- src/qemu/qemu_capspriv.h | 3 +- src/util/virhostcpu.c | 43 +++++ src/util/virhostcpu.h | 2 + tests/qemucapsprobe.c | 2 +- 11 files changed, 486 insertions(+), 120 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Tim Wiederhake