[libvirt] [PATCH 0/6] Fix guest CPU updates for inactive domains
This series originally started as a cleanup work, but it turned out two real bugs got fixed with this cleanup :-) Since commit v2.2.0-199-g7ce711a30e libvirt stores an updated guest CPU in domain's live definition and there's no need to update it every time we want to format the definition. Thus libvirt should never internally request a guest CPU update when formatting domain XML. But if it is asked by a user to do so, it should use the same host CPU model which is used when starting a domain to provide meaningful results. https://bugzilla.redhat.com/show_bug.cgi?id=1481309 https://bugzilla.redhat.com/show_bug.cgi?id=1485022 Jiri Denemark (6): qemuxml2xmltest: Add tests for Power CPUs cpu_conf: Drop updateCPU from virCPUDefFormat cpu_conf: Simplify formatting of guest CPU attributes conf: Drop unused VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU qemu: Use correct host model for updating guest cpu qemu: Don't update CPU when formatting live def src/bhyve/bhyve_driver.c | 2 +- src/conf/capabilities.c | 2 +- src/conf/cpu_conf.c | 38 +++++++++------------ src/conf/cpu_conf.h | 9 ++--- src/conf/domain_capabilities.c | 2 +- src/conf/domain_conf.c | 6 +--- src/conf/domain_conf.h | 1 - src/conf/snapshot_conf.c | 3 +- src/libxl/libxl_driver.c | 2 +- src/qemu/qemu_domain.c | 17 +++++++--- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_driver.c | 9 ++++- src/qemu/qemu_migration_cookie.c | 2 +- src/test/test_driver.c | 2 +- src/vz/vz_driver.c | 2 +- tests/cputest.c | 15 ++++----- .../ppc64-host+guest-compat-incompatible.xml | 2 +- .../ppc64-host+guest-compat-invalid.xml | 2 +- .../cputestdata/ppc64-host+guest-compat-valid.xml | 2 +- .../qemuxml2xmlout-pseries-cpu-compat-power9.xml | 38 +++++++++++++++++++++ .../qemuxml2xmlout-pseries-cpu-compat.xml | 38 +++++++++++++++++++++ .../qemuxml2xmlout-pseries-cpu-exact.xml | 39 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 4 +++ 23 files changed, 178 insertions(+), 62 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat-power9.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-exact.xml -- 2.14.1
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- .../qemuxml2xmlout-pseries-cpu-compat-power9.xml | 38 +++++++++++++++++++++ .../qemuxml2xmlout-pseries-cpu-compat.xml | 38 +++++++++++++++++++++ .../qemuxml2xmlout-pseries-cpu-exact.xml | 39 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 4 +++ 4 files changed, 119 insertions(+) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat-power9.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-exact.xml diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat-power9.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat-power9.xml new file mode 100644 index 0000000000..f020056219 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat-power9.xml @@ -0,0 +1,38 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='host-model' check='partial'> + <model fallback='allow'>power9</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio' reg='0x30000000'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio' reg='0x30000000'/> + </console> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat.xml new file mode 100644 index 0000000000..3cbce9fe6a --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat.xml @@ -0,0 +1,38 @@ +<domain type='kvm'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='host-model' check='partial'> + <model fallback='allow'>power7</model> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio' reg='0x30000000'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio' reg='0x30000000'/> + </console> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-exact.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-exact.xml new file mode 100644 index 0000000000..d69b387686 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-exact.xml @@ -0,0 +1,39 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid> + <memory unit='KiB'>524288</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='ppc64' machine='pseries'>hvm</type> + <boot dev='hd'/> + </os> + <cpu mode='custom' match='exact' check='none'> + <model fallback='allow'>POWER7</model> + <vendor>IBM</vendor> + </cpu> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-ppc64</emulator> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/> + </controller> + <controller type='pci' index='0' model='pci-root'> + <model name='spapr-pci-host-bridge'/> + <target index='0'/> + </controller> + <serial type='pty'> + <target port='0'/> + <address type='spapr-vio' reg='0x30000000'/> + </serial> + <console type='pty'> + <target type='serial' port='0'/> + <address type='spapr-vio' reg='0x30000000'/> + </console> + <memballoon model='none'/> + <panic model='pseries'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index efac9e8286..d15ea93b19 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -1216,6 +1216,10 @@ mymain(void) DO_TEST("smartcard-passthrough-spicevmc", NONE); DO_TEST("smartcard-controller", NONE); + DO_TEST("pseries-cpu-compat-power9", NONE); + DO_TEST("pseries-cpu-compat", NONE); + DO_TEST("pseries-cpu-exact", NONE); + if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL) virFileDeleteTree(fakerootdir); -- 2.14.1
In the past we updated host-model CPUs with host CPU data by adding a model and features, but keeping the host-model mode. And since the CPU model is not normally formatted for host-model CPU defs, we had to pass the updateCPU flag to the formatting code to be able to properly output updated host-model CPUs. Libvirt doesn't do this anymore, host-model CPUs are turned into custom mode CPUs once updated with host CPU data and thus there's no reason for keeping the hacks inside CPU XML formatters. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/bhyve/bhyve_driver.c | 2 +- src/conf/capabilities.c | 2 +- src/conf/cpu_conf.c | 20 +++++++------------- src/conf/cpu_conf.h | 9 +++------ src/conf/domain_capabilities.c | 2 +- src/conf/domain_conf.c | 3 +-- src/libxl/libxl_driver.c | 2 +- src/qemu/qemu_domain.c | 4 ++-- src/qemu/qemu_driver.c | 2 +- src/qemu/qemu_migration_cookie.c | 2 +- src/test/test_driver.c | 2 +- src/vz/vz_driver.c | 2 +- tests/cputest.c | 15 +++++++-------- .../ppc64-host+guest-compat-incompatible.xml | 2 +- .../cputestdata/ppc64-host+guest-compat-invalid.xml | 2 +- tests/cputestdata/ppc64-host+guest-compat-valid.xml | 2 +- 16 files changed, 31 insertions(+), 42 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index e8241f39ff..c096b5562f 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -1441,7 +1441,7 @@ bhyveConnectBaselineCPU(virConnectPtr conn, virCPUExpandFeatures(cpus[0]->arch, cpu) < 0) goto cleanup; - cpustr = virCPUDefFormat(cpu, NULL, false); + cpustr = virCPUDefFormat(cpu, NULL); cleanup: virCPUDefListFree(cpus); diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index ba554535ae..9920a675ac 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -984,7 +984,7 @@ virCapabilitiesFormatXML(virCapsPtr caps) virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</features>\n"); } - virCPUDefFormatBuf(&buf, caps->host.cpu, false); + virCPUDefFormatBuf(&buf, caps->host.cpu); for (i = 0; i < caps->host.nPagesSize; i++) { virBufferAsprintf(&buf, "<pages unit='KiB' size='%u'/>\n", diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 0bd56c9d28..6058d26fa5 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -574,12 +574,11 @@ virCPUDefParseXML(xmlXPathContextPtr ctxt, char * virCPUDefFormat(virCPUDefPtr def, - virDomainNumaPtr numa, - bool updateCPU) + virDomainNumaPtr numa) { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (virCPUDefFormatBufFull(&buf, def, numa, updateCPU) < 0) + if (virCPUDefFormatBufFull(&buf, def, numa) < 0) goto cleanup; if (virBufferCheckError(&buf) < 0) @@ -596,8 +595,7 @@ virCPUDefFormat(virCPUDefPtr def, int virCPUDefFormatBufFull(virBufferPtr buf, virCPUDefPtr def, - virDomainNumaPtr numa, - bool updateCPU) + virDomainNumaPtr numa) { int ret = -1; virBuffer attributeBuf = VIR_BUFFER_INITIALIZER; @@ -619,9 +617,7 @@ virCPUDefFormatBufFull(virBufferPtr buf, virBufferAsprintf(&attributeBuf, " mode='%s'", tmp); } - if (def->model && - (def->mode == VIR_CPU_MODE_CUSTOM || - updateCPU)) { + if (def->model && def->mode == VIR_CPU_MODE_CUSTOM) { if (!(tmp = virCPUMatchTypeToString(def->match))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected CPU match policy %d"), @@ -642,7 +638,7 @@ virCPUDefFormatBufFull(virBufferPtr buf, if (def->type == VIR_CPU_TYPE_HOST && def->arch) virBufferAsprintf(&childrenBuf, "<arch>%s</arch>\n", virArchToString(def->arch)); - if (virCPUDefFormatBuf(&childrenBuf, def, updateCPU) < 0) + if (virCPUDefFormatBuf(&childrenBuf, def) < 0) goto cleanup; if (virDomainNumaDefCPUFormat(&childrenBuf, numa) < 0) @@ -677,8 +673,7 @@ virCPUDefFormatBufFull(virBufferPtr buf, int virCPUDefFormatBuf(virBufferPtr buf, - virCPUDefPtr def, - bool updateCPU) + virCPUDefPtr def) { size_t i; bool formatModel; @@ -688,8 +683,7 @@ virCPUDefFormatBuf(virBufferPtr buf, return 0; formatModel = (def->mode == VIR_CPU_MODE_CUSTOM || - def->mode == VIR_CPU_MODE_HOST_MODEL || - updateCPU); + def->mode == VIR_CPU_MODE_HOST_MODEL); formatFallback = (def->type == VIR_CPU_TYPE_GUEST && (def->mode == VIR_CPU_MODE_HOST_MODEL || (def->mode == VIR_CPU_MODE_CUSTOM && def->model))); diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index d3e2c84102..b1a512b19a 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -195,18 +195,15 @@ virCPUDefIsEqual(virCPUDefPtr src, char * virCPUDefFormat(virCPUDefPtr def, - virDomainNumaPtr numa, - bool updateCPU); + virDomainNumaPtr numa); int virCPUDefFormatBuf(virBufferPtr buf, - virCPUDefPtr def, - bool updateCPU); + virCPUDefPtr def); int virCPUDefFormatBufFull(virBufferPtr buf, virCPUDefPtr def, - virDomainNumaPtr numa, - bool updateCPU); + virDomainNumaPtr numa); int virCPUDefAddFeature(virCPUDefPtr cpu, diff --git a/src/conf/domain_capabilities.c b/src/conf/domain_capabilities.c index 35f8128e70..f62038b96c 100644 --- a/src/conf/domain_capabilities.c +++ b/src/conf/domain_capabilities.c @@ -413,7 +413,7 @@ virDomainCapsCPUFormat(virBufferPtr buf, virBufferAddLit(buf, "supported='yes'>\n"); virBufferAdjustIndent(buf, 2); - virCPUDefFormatBuf(buf, cpu->hostModel, false); + virCPUDefFormatBuf(buf, cpu->hostModel); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</mode>\n"); diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cc5e79b70b..35adce95b5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -25720,8 +25720,7 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, "</features>\n"); } - if (virCPUDefFormatBufFull(buf, def->cpu, def->numa, - !!(flags & VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU)) < 0) + if (virCPUDefFormatBufFull(buf, def->cpu, def->numa) < 0) goto error; virBufferAsprintf(buf, "<clock offset='%s'", diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 4861e5db21..bf3625e34a 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -6471,7 +6471,7 @@ libxlConnectBaselineCPU(virConnectPtr conn, virCPUExpandFeatures(cpus[0]->arch, cpu) < 0) goto cleanup; - cpustr = virCPUDefFormat(cpu, NULL, false); + cpustr = virCPUDefFormat(cpu, NULL); cleanup: virCPUDefListFree(cpus); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 50b536eec7..aba4111ba2 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1945,7 +1945,7 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virBufferEscapeString(buf, "<channelTargetDir path='%s'/>\n", priv->channelTargetDir); - virCPUDefFormatBufFull(buf, priv->origCPU, NULL, false); + virCPUDefFormatBufFull(buf, priv->origCPU, NULL); if (priv->chardevStdioLogd) virBufferAddLit(buf, "<chardevStdioLogd/>\n"); @@ -9803,7 +9803,7 @@ qemuDomainSaveCookieFormat(virBufferPtr buf, qemuDomainSaveCookiePtr cookie = (qemuDomainSaveCookiePtr) obj; if (cookie->cpu && - virCPUDefFormatBufFull(buf, cookie->cpu, NULL, false) < 0) + virCPUDefFormatBufFull(buf, cookie->cpu, NULL) < 0) return -1; return 0; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e1a0dd553e..77308d547e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13019,7 +13019,7 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, virCPUExpandFeatures(cpus[0]->arch, cpu) < 0) goto cleanup; - cpustr = virCPUDefFormat(cpu, NULL, false); + cpustr = virCPUDefFormat(cpu, NULL); cleanup: virCPUDefListFree(cpus); diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index 4914c77ef0..eef40a6cd0 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -771,7 +771,7 @@ qemuMigrationCookieXMLFormat(virQEMUDriverPtr driver, qemuMigrationCookieStatisticsXMLFormat(buf, mig->jobInfo); if (mig->flags & QEMU_MIGRATION_COOKIE_CPU && mig->cpu) - virCPUDefFormatBufFull(buf, mig->cpu, NULL, false); + virCPUDefFormatBufFull(buf, mig->cpu, NULL); virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</qemu-migration>\n"); diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6e8a4b5782..599c5ed545 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -1551,7 +1551,7 @@ testConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, virCPUExpandFeatures(cpus[0]->arch, cpu) < 0) goto cleanup; - cpustr = virCPUDefFormat(cpu, NULL, false); + cpustr = virCPUDefFormat(cpu, NULL); cleanup: virCPUDefListFree(cpus); diff --git a/src/vz/vz_driver.c b/src/vz/vz_driver.c index daeed5f114..9ebb51d60a 100644 --- a/src/vz/vz_driver.c +++ b/src/vz/vz_driver.c @@ -964,7 +964,7 @@ vzConnectBaselineCPU(virConnectPtr conn, virCPUExpandFeatures(cpus[0]->arch, cpu) < 0) goto cleanup; - cpustr = virCPUDefFormat(cpu, NULL, false); + cpustr = virCPUDefFormat(cpu, NULL); cleanup: virCPUDefListFree(cpus); diff --git a/tests/cputest.c b/tests/cputest.c index d325b5315c..913ca77231 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -150,8 +150,7 @@ cpuTestLoadMultiXML(virArch arch, static int cpuTestCompareXML(virArch arch, virCPUDef *cpu, - const char *name, - bool updateCPU) + const char *name) { char *xml = NULL; char *actual = NULL; @@ -161,7 +160,7 @@ cpuTestCompareXML(virArch arch, abs_srcdir, virArchToString(arch), name) < 0) goto cleanup; - if (!(actual = virCPUDefFormat(cpu, NULL, updateCPU))) + if (!(actual = virCPUDefFormat(cpu, NULL))) goto cleanup; if (virTestCompareToFile(actual, xml) < 0) @@ -281,7 +280,7 @@ cpuTestGuestCPU(const void *arg) } result = virBufferContentAndReset(&buf); - if (cpuTestCompareXML(data->arch, cpu, result, false) < 0) + if (cpuTestCompareXML(data->arch, cpu, result) < 0) goto cleanup; ret = 0; @@ -355,7 +354,7 @@ cpuTestBaseline(const void *arg) if (virAsprintf(&result, "%s-%s", data->name, suffix) < 0) goto cleanup; - if (cpuTestCompareXML(data->arch, baseline, result, false) < 0) + if (cpuTestCompareXML(data->arch, baseline, result) < 0) goto cleanup; for (i = 0; i < ncpus; i++) { @@ -409,7 +408,7 @@ cpuTestUpdate(const void *arg) if (virAsprintf(&result, "%s+%s", data->host, data->name) < 0) goto cleanup; - ret = cpuTestCompareXML(data->arch, cpu, result, true); + ret = cpuTestCompareXML(data->arch, cpu, result); cleanup: virCPUDefFree(host); @@ -501,7 +500,7 @@ cpuTestCPUID(bool guest, const void *arg) guest ? "guest" : "host") < 0) goto cleanup; - ret = cpuTestCompareXML(data->arch, cpu, result, false); + ret = cpuTestCompareXML(data->arch, cpu, result); cleanup: VIR_FREE(hostFile); @@ -716,7 +715,7 @@ cpuTestJSONCPUID(const void *arg) if (virQEMUCapsInitCPUModel(qemuCaps, VIR_DOMAIN_VIRT_KVM, cpu, false) != 0) goto cleanup; - ret = cpuTestCompareXML(data->arch, cpu, result, false); + ret = cpuTestCompareXML(data->arch, cpu, result); cleanup: qemuMonitorCPUModelInfoFree(model); diff --git a/tests/cputestdata/ppc64-host+guest-compat-incompatible.xml b/tests/cputestdata/ppc64-host+guest-compat-incompatible.xml index 1fab751ada..0595ba0efd 100644 --- a/tests/cputestdata/ppc64-host+guest-compat-incompatible.xml +++ b/tests/cputestdata/ppc64-host+guest-compat-incompatible.xml @@ -1,3 +1,3 @@ -<cpu mode='host-model' match='exact'> +<cpu mode='host-model'> <model fallback='allow'>power8</model> </cpu> diff --git a/tests/cputestdata/ppc64-host+guest-compat-invalid.xml b/tests/cputestdata/ppc64-host+guest-compat-invalid.xml index bc0d829673..edf874f4f4 100644 --- a/tests/cputestdata/ppc64-host+guest-compat-invalid.xml +++ b/tests/cputestdata/ppc64-host+guest-compat-invalid.xml @@ -1,3 +1,3 @@ -<cpu mode='host-model' match='exact'> +<cpu mode='host-model'> <model fallback='allow'>power7+</model> </cpu> diff --git a/tests/cputestdata/ppc64-host+guest-compat-valid.xml b/tests/cputestdata/ppc64-host+guest-compat-valid.xml index da9cc91885..f32cd93093 100644 --- a/tests/cputestdata/ppc64-host+guest-compat-valid.xml +++ b/tests/cputestdata/ppc64-host+guest-compat-valid.xml @@ -1,3 +1,3 @@ -<cpu mode='host-model' match='exact'> +<cpu mode='host-model'> <model fallback='allow'>power6</model> </cpu> -- 2.14.1
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/cpu_conf.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 6058d26fa5..02506c020b 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -604,20 +604,20 @@ virCPUDefFormatBufFull(virBufferPtr buf, if (!def) return 0; - /* Format attributes */ - if (def->type == VIR_CPU_TYPE_GUEST) { + /* Format attributes for guest CPUs unless they only specify + * topology or cache. */ + if (def->type == VIR_CPU_TYPE_GUEST && + (def->mode != VIR_CPU_MODE_CUSTOM || def->model)) { const char *tmp; - if (def->mode != VIR_CPU_MODE_CUSTOM || def->model) { - if (!(tmp = virCPUModeTypeToString(def->mode))) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Unexpected CPU mode %d"), def->mode); - goto cleanup; - } - virBufferAsprintf(&attributeBuf, " mode='%s'", tmp); + if (!(tmp = virCPUModeTypeToString(def->mode))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Unexpected CPU mode %d"), def->mode); + goto cleanup; } + virBufferAsprintf(&attributeBuf, " mode='%s'", tmp); - if (def->model && def->mode == VIR_CPU_MODE_CUSTOM) { + if (def->mode == VIR_CPU_MODE_CUSTOM) { if (!(tmp = virCPUMatchTypeToString(def->match))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unexpected CPU match policy %d"), -- 2.14.1
The only real usage of this flag was removed by "cpu_conf: Drop updateCPU from virCPUDefFormat". Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 3 --- src/conf/domain_conf.h | 1 - src/conf/snapshot_conf.c | 3 +-- 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35adce95b5..984db98107 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -87,7 +87,6 @@ struct _virDomainXMLOption { #define VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS \ (VIR_DOMAIN_DEF_FORMAT_SECURE | \ VIR_DOMAIN_DEF_FORMAT_INACTIVE | \ - VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU | \ VIR_DOMAIN_DEF_FORMAT_MIGRATABLE) VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, @@ -25974,8 +25973,6 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) formatFlags |= VIR_DOMAIN_DEF_FORMAT_SECURE; if (flags & VIR_DOMAIN_XML_INACTIVE) formatFlags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; - if (flags & VIR_DOMAIN_XML_UPDATE_CPU) - formatFlags |= VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU; if (flags & VIR_DOMAIN_XML_MIGRATABLE) formatFlags |= VIR_DOMAIN_DEF_FORMAT_MIGRATABLE; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bb3b6f0c3c..4603e4f97e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2857,7 +2857,6 @@ typedef enum { typedef enum { VIR_DOMAIN_DEF_FORMAT_SECURE = 1 << 0, VIR_DOMAIN_DEF_FORMAT_INACTIVE = 1 << 1, - VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU = 1 << 2, VIR_DOMAIN_DEF_FORMAT_MIGRATABLE = 1 << 3, /* format internal domain status information */ VIR_DOMAIN_DEF_FORMAT_STATUS = 1 << 4, diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 5a07d3734a..07706e0b26 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -703,8 +703,7 @@ virDomainSnapshotDefFormat(const char *domain_uuid, virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i; - virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE | - VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU, NULL); + virCheckFlags(VIR_DOMAIN_DEF_FORMAT_SECURE, NULL); flags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; -- 2.14.1
On 09/18/2017 03:35 PM, Jiri Denemark wrote:
The only real usage of this flag was removed by "cpu_conf: Drop updateCPU from virCPUDefFormat".
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/conf/domain_conf.c | 3 --- src/conf/domain_conf.h | 1 - src/conf/snapshot_conf.c | 3 +-- 3 files changed, 1 insertion(+), 6 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 35adce95b5..984db98107 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -87,7 +87,6 @@ struct _virDomainXMLOption { #define VIR_DOMAIN_DEF_FORMAT_COMMON_FLAGS \ (VIR_DOMAIN_DEF_FORMAT_SECURE | \ VIR_DOMAIN_DEF_FORMAT_INACTIVE | \ - VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU | \ VIR_DOMAIN_DEF_FORMAT_MIGRATABLE)
VIR_ENUM_IMPL(virDomainTaint, VIR_DOMAIN_TAINT_LAST, @@ -25974,8 +25973,6 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags) formatFlags |= VIR_DOMAIN_DEF_FORMAT_SECURE; if (flags & VIR_DOMAIN_XML_INACTIVE) formatFlags |= VIR_DOMAIN_DEF_FORMAT_INACTIVE; - if (flags & VIR_DOMAIN_XML_UPDATE_CPU) - formatFlags |= VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU; if (flags & VIR_DOMAIN_XML_MIGRATABLE) formatFlags |= VIR_DOMAIN_DEF_FORMAT_MIGRATABLE;
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bb3b6f0c3c..4603e4f97e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2857,7 +2857,6 @@ typedef enum { typedef enum { VIR_DOMAIN_DEF_FORMAT_SECURE = 1 << 0, VIR_DOMAIN_DEF_FORMAT_INACTIVE = 1 << 1, - VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU = 1 << 2, VIR_DOMAIN_DEF_FORMAT_MIGRATABLE = 1 << 3, /* format internal domain status information */ VIR_DOMAIN_DEF_FORMAT_STATUS = 1 << 4,
Should we update these so that the flags are consecutive? VIR_DOMAIN_DEF_FORMAT_MIGRATABLE can be 1 << 2, VIR_DOMAIN_DEF_FORMAT_STATUS can be 1 << 3, and so on. These are just internal flags so there would be no ABI brekage if we do that. Michal
On Tue, Sep 19, 2017 at 09:36:18 +0200, Michal Privoznik wrote:
On 09/18/2017 03:35 PM, Jiri Denemark wrote:
The only real usage of this flag was removed by "cpu_conf: Drop updateCPU from virCPUDefFormat".
...
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bb3b6f0c3c..4603e4f97e 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2857,7 +2857,6 @@ typedef enum { typedef enum { VIR_DOMAIN_DEF_FORMAT_SECURE = 1 << 0, VIR_DOMAIN_DEF_FORMAT_INACTIVE = 1 << 1, - VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU = 1 << 2, VIR_DOMAIN_DEF_FORMAT_MIGRATABLE = 1 << 3, /* format internal domain status information */ VIR_DOMAIN_DEF_FORMAT_STATUS = 1 << 4,
Should we update these so that the flags are consecutive? VIR_DOMAIN_DEF_FORMAT_MIGRATABLE can be 1 << 2, VIR_DOMAIN_DEF_FORMAT_STATUS can be 1 << 3, and so on. These are just internal flags so there would be no ABI brekage if we do that.
It's a bit easier to review the change without touch the rest of the enum values, but I guess it could confuse anyone looking at the enum later. I updated all values before pushing this series. Thanks. Jirka
when a user requested a domain xml description with vir_domain_xml_update_cpu flag, libvirt would use the host cpu definition from host capabilities rather than the one which will actually be used once the domain is started. https://bugzilla.redhat.com/show_bug.cgi?id=1481309 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_domain.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index aba4111ba2..74284f40c1 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4553,6 +4553,7 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, int ret = -1; virDomainDefPtr copy = NULL; virCapsPtr caps = NULL; + virQEMUCapsPtr qemuCaps = NULL; if (!(caps = virQEMUDriverGetCapabilities(driver, false))) goto cleanup; @@ -4571,7 +4572,14 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, def->cpu && (def->cpu->mode != VIR_CPU_MODE_CUSTOM || def->cpu->model)) { - if (virCPUUpdate(def->os.arch, def->cpu, caps->host.cpu) < 0) + if (!(qemuCaps = virQEMUCapsCacheLookupCopy(driver->qemuCapsCache, + def->emulator, + def->os.machine))) + goto cleanup; + + if (virCPUUpdate(def->os.arch, def->cpu, + virQEMUCapsGetHostModel(qemuCaps, def->virtType, + VIR_QEMU_CAPS_HOST_CPU_MIGRATABLE)) < 0) goto cleanup; } @@ -4689,6 +4697,7 @@ qemuDomainDefFormatBufInternal(virQEMUDriverPtr driver, cleanup: virDomainDefFree(copy); virObjectUnref(caps); + virObjectUnref(qemuCaps); return ret; } -- 2.14.1
Since commit v2.2.0-199-g7ce711a30e libvirt stores an updated guest CPU in domain's live definition and there's no need to update it every time we want to format the definition. The commit itself tried to address this in qemuDomainFormatXML, but forgot to fix qemuDomainDefFormatLive. Not to mention that masking a previously set flag is only acceptable if the flag was set by a public API user. Internally, libvirt should have never set the flag in the first place. https://bugzilla.redhat.com/show_bug.cgi?id=1485022 Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_domain.c | 2 -- src/qemu/qemu_domain.h | 3 +-- src/qemu/qemu_driver.c | 7 +++++++ 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 74284f40c1..5ef98911dc 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -4757,8 +4757,6 @@ char *qemuDomainFormatXML(virQEMUDriverPtr driver, } else { def = vm->def; origCPU = priv->origCPU; - if (virDomainObjIsActive(vm)) - flags &= ~VIR_DOMAIN_XML_UPDATE_CPU; } return qemuDomainDefFormatXMLInternal(driver, def, origCPU, flags); diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b291dc3082..09201b1a40 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -40,8 +40,7 @@ # include "logging/log_manager.h" # define QEMU_DOMAIN_FORMAT_LIVE_FLAGS \ - (VIR_DOMAIN_XML_SECURE | \ - VIR_DOMAIN_XML_UPDATE_CPU) + (VIR_DOMAIN_XML_SECURE) # if ULONG_MAX == 4294967295 /* QEMU has a 64-bit limit, but we are limited by our historical choice of diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 77308d547e..a29bbea55d 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6997,6 +6997,13 @@ static char if ((flags & VIR_DOMAIN_XML_MIGRATABLE)) flags |= QEMU_DOMAIN_FORMAT_LIVE_FLAGS; + /* The CPU is already updated in the domain's live definition, we need to + * ignore the VIR_DOMAIN_XML_UPDATE_CPU flag. + */ + if (virDomainObjIsActive(vm) && + !(flags & VIR_DOMAIN_DEF_FORMAT_INACTIVE)) + flags &= ~VIR_DOMAIN_XML_UPDATE_CPU; + ret = qemuDomainFormatXML(driver, vm, flags); cleanup: -- 2.14.1
On 09/18/2017 03:35 PM, Jiri Denemark wrote:
This series originally started as a cleanup work, but it turned out two real bugs got fixed with this cleanup :-)
Since commit v2.2.0-199-g7ce711a30e libvirt stores an updated guest CPU in domain's live definition and there's no need to update it every time we want to format the definition. Thus libvirt should never internally request a guest CPU update when formatting domain XML. But if it is asked by a user to do so, it should use the same host CPU model which is used when starting a domain to provide meaningful results.
https://bugzilla.redhat.com/show_bug.cgi?id=1481309 https://bugzilla.redhat.com/show_bug.cgi?id=1485022
Jiri Denemark (6): qemuxml2xmltest: Add tests for Power CPUs cpu_conf: Drop updateCPU from virCPUDefFormat cpu_conf: Simplify formatting of guest CPU attributes conf: Drop unused VIR_DOMAIN_DEF_FORMAT_UPDATE_CPU qemu: Use correct host model for updating guest cpu qemu: Don't update CPU when formatting live def
src/bhyve/bhyve_driver.c | 2 +- src/conf/capabilities.c | 2 +- src/conf/cpu_conf.c | 38 +++++++++------------ src/conf/cpu_conf.h | 9 ++--- src/conf/domain_capabilities.c | 2 +- src/conf/domain_conf.c | 6 +--- src/conf/domain_conf.h | 1 - src/conf/snapshot_conf.c | 3 +- src/libxl/libxl_driver.c | 2 +- src/qemu/qemu_domain.c | 17 +++++++--- src/qemu/qemu_domain.h | 3 +- src/qemu/qemu_driver.c | 9 ++++- src/qemu/qemu_migration_cookie.c | 2 +- src/test/test_driver.c | 2 +- src/vz/vz_driver.c | 2 +- tests/cputest.c | 15 ++++----- .../ppc64-host+guest-compat-incompatible.xml | 2 +- .../ppc64-host+guest-compat-invalid.xml | 2 +- .../cputestdata/ppc64-host+guest-compat-valid.xml | 2 +- .../qemuxml2xmlout-pseries-cpu-compat-power9.xml | 38 +++++++++++++++++++++ .../qemuxml2xmlout-pseries-cpu-compat.xml | 38 +++++++++++++++++++++ .../qemuxml2xmlout-pseries-cpu-exact.xml | 39 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 4 +++ 23 files changed, 178 insertions(+), 62 deletions(-) create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat-power9.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-compat.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-pseries-cpu-exact.xml
ACK series. Michal
participants (2)
-
Jiri Denemark -
Michal Privoznik