[libvirt] [PATCH 0/7] qemu: Use migratable host CPU model from QEMU
Current libvirt makes a migratable host CPU model by removing features marked in cpu_map.xml with migratable='no'. This is not ideal because cpu_map.xml is a static database while feature migrtability may differ depending on a hypervisor or its version. Thus we should preferably use the data we got from QEMU. Jiri Denemark (7): cpu: Introduce virCPUCopyMigratable qemu: Move common code in virQEMUCapsInitCPUModel one layer up qemu: Add migratable parameter to virQEMUCapsInitCPUModel qemu: Move qemuCaps->{kvm,tcg}CPUModel into a struct qemu: Store migratable host CPU model in qemuCaps qemu: Pass migratable host model to virCPUUpdate cpu: Drop feature filtering from virCPUUpdate src/cpu/cpu.c | 29 +++++++ src/cpu/cpu.h | 8 ++ src/cpu/cpu_x86.c | 34 ++++++-- src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 200 ++++++++++++++++++++++++++++++------------- src/qemu/qemu_capabilities.h | 3 +- src/qemu/qemu_capspriv.h | 3 +- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_process.c | 4 +- tests/cputest.c | 9 +- 10 files changed, 221 insertions(+), 72 deletions(-) -- 2.12.0
This new internal API makes a copy of virCPUDef while removing all features which would block migration. It uses cpu_map.xml as a database of such features, which should only be used as a fallback when we cannot get the data from a hypervisor. The main goal of this API is to decouple this filtering from virCPUUpdate so that the hypervisor driver can filter the features according to the hypervisor. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu.c | 29 +++++++++++++++++++++++++++++ src/cpu/cpu.h | 8 ++++++++ src/cpu/cpu_x86.c | 25 +++++++++++++++++++++++++ src/libvirt_private.syms | 1 + 4 files changed, 63 insertions(+) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 93647a2ed..1cc68f142 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -1130,3 +1130,32 @@ virCPUExpandFeatures(virArch arch, VIR_DEBUG("nfeatures=%zu", cpu->nfeatures); return 0; } + + +/** + * virCPUCopyMigratable: + * + * @arch: CPU architecture + * @cpu: CPU definition to be copied + * + * Makes a copy of @cpu with all features which would block migration removed. + * + * Returns the copy of the CPU or NULL on error. + */ +virCPUDefPtr +virCPUCopyMigratable(virArch arch, + virCPUDefPtr cpu) +{ + struct cpuArchDriver *driver; + + VIR_DEBUG("arch=%s, cpu=%p, model=%s", + virArchToString(arch), cpu, NULLSTR(cpu->model)); + + if (!(driver = cpuGetSubDriver(arch))) + return NULL; + + if (driver->copyMigratable) + return driver->copyMigratable(cpu); + else + return virCPUDefCopy(cpu); +} diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index 8c238ad55..352445c40 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -118,6 +118,9 @@ typedef int typedef int (*virCPUArchExpandFeatures)(virCPUDefPtr cpu); +typedef virCPUDefPtr +(*virCPUArchCopyMigratable)(virCPUDefPtr cpu); + struct cpuArchDriver { const char *name; const virArch *arch; @@ -138,6 +141,7 @@ struct cpuArchDriver { virCPUArchTranslate translate; virCPUArchConvertLegacy convertLegacy; virCPUArchExpandFeatures expandFeatures; + virCPUArchCopyMigratable copyMigratable; }; @@ -254,6 +258,10 @@ int virCPUExpandFeatures(virArch arch, virCPUDefPtr cpu); +virCPUDefPtr +virCPUCopyMigratable(virArch arch, + virCPUDefPtr cpu); + /* virCPUDataFormat and virCPUDataParse are implemented for unit tests only and * have no real-life usage */ diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index 48648a7f4..a771b251e 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2903,6 +2903,30 @@ virCPUx86ExpandFeatures(virCPUDefPtr cpu) } +static virCPUDefPtr +virCPUx86CopyMigratable(virCPUDefPtr cpu) +{ + virCPUDefPtr copy; + virCPUx86MapPtr map; + + if (!(map = virCPUx86GetMap())) + return NULL; + + if (!(copy = virCPUDefCopyWithoutModel(cpu))) + return NULL; + + if (virCPUDefCopyModelFilter(copy, cpu, false, + x86FeatureIsMigratable, map) < 0) + goto error; + + return copy; + + error: + virCPUDefFree(copy); + return NULL; +} + + int virCPUx86DataAddCPUID(virCPUDataPtr cpuData, const virCPUx86CPUID *cpuid) @@ -2978,4 +3002,5 @@ struct cpuArchDriver cpuDriverX86 = { .getModels = virCPUx86GetModels, .translate = virCPUx86Translate, .expandFeatures = virCPUx86ExpandFeatures, + .copyMigratable = virCPUx86CopyMigratable, }; diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index b551cb86a..dc6db3b28 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1016,6 +1016,7 @@ virCPUCheckFeature; virCPUCompare; virCPUCompareXML; virCPUConvertLegacy; +virCPUCopyMigratable; virCPUDataCheckFeature; virCPUDataFormat; virCPUDataFree; -- 2.12.0
On 03/31/2017 01:53 PM, Jiri Denemark wrote:
This new internal API makes a copy of virCPUDef while removing all features which would block migration. It uses cpu_map.xml as a database of such features, which should only be used as a fallback when we cannot get the data from a hypervisor. The main goal of this API is to decouple this filtering from virCPUUpdate so that the hypervisor driver can filter the features according to the hypervisor.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu.c | 29 +++++++++++++++++++++++++++++ src/cpu/cpu.h | 8 ++++++++ src/cpu/cpu_x86.c | 25 +++++++++++++++++++++++++ src/libvirt_private.syms | 1 + 4 files changed, 63 insertions(+)
diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 93647a2ed..1cc68f142 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -1130,3 +1130,32 @@ virCPUExpandFeatures(virArch arch, VIR_DEBUG("nfeatures=%zu", cpu->nfeatures); return 0; } + + +/** + * virCPUCopyMigratable: + * + * @arch: CPU architecture + * @cpu: CPU definition to be copied + * + * Makes a copy of @cpu with all features which would block migration removed. + * + * Returns the copy of the CPU or NULL on error. + */ +virCPUDefPtr +virCPUCopyMigratable(virArch arch, + virCPUDefPtr cpu) +{ + struct cpuArchDriver *driver; + + VIR_DEBUG("arch=%s, cpu=%p, model=%s", + virArchToString(arch), cpu, NULLSTR(cpu->model)); + + if (!(driver = cpuGetSubDriver(arch))) + return NULL; + + if (driver->copyMigratable) + return driver->copyMigratable(cpu); + else + return virCPUDefCopy(cpu);
Not quite sure I read the function comments similarly with respect to what the code is doing (and peeking ahead to patch 5 where this is first called). Essentially this API either makes a 'migratable' version of a virCPUDef or it just copies the (eventually called) "full" version of a virCPUDef for architectures that don't support or need to change the "full" virCPUDef to disable migratability features... (If I've read the tea leaves right of course ;-)) ACK with that adjustment, John
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b1245ad5d..1a15750a3 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3111,17 +3111,11 @@ virQEMUCapsCPUFilterFeatures(const char *name, */ static int virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps, - virDomainVirtType type, + qemuMonitorCPUModelInfoPtr modelInfo, virCPUDefPtr cpu) { - qemuMonitorCPUModelInfoPtr modelInfo; size_t i; - if (type == VIR_DOMAIN_VIRT_KVM) - modelInfo = qemuCaps->kvmCPUModelInfo; - else - modelInfo = qemuCaps->tcgCPUModelInfo; - if (!modelInfo) { virReportError(VIR_ERR_INTERNAL_ERROR, _("missing host CPU model info from QEMU capabilities " @@ -3163,9 +3157,9 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps, static int virQEMUCapsInitCPUModelX86(virQEMUCapsPtr qemuCaps, virDomainVirtType type, + qemuMonitorCPUModelInfoPtr model, virCPUDefPtr cpu) { - qemuMonitorCPUModelInfoPtr model; virCPUDataPtr data = NULL; unsigned long long sigFamily = 0; unsigned long long sigModel = 0; @@ -3174,11 +3168,6 @@ virQEMUCapsInitCPUModelX86(virQEMUCapsPtr qemuCaps, int ret = -1; size_t i; - if (type == VIR_DOMAIN_VIRT_KVM) - model = qemuCaps->kvmCPUModelInfo; - else - model = qemuCaps->tcgCPUModelInfo; - if (!model) return 1; @@ -3239,12 +3228,18 @@ virQEMUCapsInitCPUModel(virQEMUCapsPtr qemuCaps, virDomainVirtType type, virCPUDefPtr cpu) { + qemuMonitorCPUModelInfoPtr model; int ret = 1; + if (type == VIR_DOMAIN_VIRT_KVM) + model = qemuCaps->kvmCPUModelInfo; + else + model = qemuCaps->tcgCPUModelInfo; + if (ARCH_IS_S390(qemuCaps->arch)) - ret = virQEMUCapsInitCPUModelS390(qemuCaps, type, cpu); + ret = virQEMUCapsInitCPUModelS390(qemuCaps, model, cpu); else if (ARCH_IS_X86(qemuCaps->arch)) - ret = virQEMUCapsInitCPUModelX86(qemuCaps, type, cpu); + ret = virQEMUCapsInitCPUModelX86(qemuCaps, type, model, cpu); if (ret == 0) cpu->fallback = VIR_CPU_FALLBACK_FORBID; -- 2.12.0
On 03/31/2017 01:53 PM, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-)
ACK, John
The caller can ask for a migratable CPU model by passing true for the new parameter. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 36 +++++++++++++++++++++++++----------- src/qemu/qemu_capspriv.h | 3 ++- tests/cputest.c | 2 +- 3 files changed, 28 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 1a15750a3..b8e4e47b6 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3112,7 +3112,8 @@ virQEMUCapsCPUFilterFeatures(const char *name, static int virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps, qemuMonitorCPUModelInfoPtr modelInfo, - virCPUDefPtr cpu) + virCPUDefPtr cpu, + bool migratable) { size_t i; @@ -3140,8 +3141,12 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps, if (VIR_STRDUP(feature->name, prop->name) < 0) return -1; - feature->policy = prop->value.boolean ? VIR_CPU_FEATURE_REQUIRE - : VIR_CPU_FEATURE_DISABLE; + + if (!prop->value.boolean || + (migratable && prop->migratable == VIR_TRISTATE_BOOL_NO)) + feature->policy = VIR_CPU_FEATURE_DISABLE; + else + feature->policy = VIR_CPU_FEATURE_REQUIRE; cpu->nfeatures++; } @@ -3158,7 +3163,8 @@ static int virQEMUCapsInitCPUModelX86(virQEMUCapsPtr qemuCaps, virDomainVirtType type, qemuMonitorCPUModelInfoPtr model, - virCPUDefPtr cpu) + virCPUDefPtr cpu, + bool migratable) { virCPUDataPtr data = NULL; unsigned long long sigFamily = 0; @@ -3179,9 +3185,13 @@ virQEMUCapsInitCPUModelX86(virQEMUCapsPtr qemuCaps, switch (prop->type) { case QEMU_MONITOR_CPU_PROPERTY_BOOLEAN: - if (prop->value.boolean && - virCPUx86DataAddFeature(data, prop->name) < 0) + if (!prop->value.boolean || + (migratable && prop->migratable == VIR_TRISTATE_BOOL_NO)) + continue; + + if (virCPUx86DataAddFeature(data, prop->name) < 0) goto cleanup; + break; case QEMU_MONITOR_CPU_PROPERTY_STRING: @@ -3220,13 +3230,14 @@ virQEMUCapsInitCPUModelX86(virQEMUCapsPtr qemuCaps, /** * Returns 0 when host CPU model provided by QEMU was filled in qemuCaps, - * 1 when the caller should fall back to using virCapsPtr->host.cpu, + * 1 when the caller should fall back to other methods * -1 on error. */ int virQEMUCapsInitCPUModel(virQEMUCapsPtr qemuCaps, virDomainVirtType type, - virCPUDefPtr cpu) + virCPUDefPtr cpu, + bool migratable) { qemuMonitorCPUModelInfoPtr model; int ret = 1; @@ -3236,10 +3247,13 @@ virQEMUCapsInitCPUModel(virQEMUCapsPtr qemuCaps, else model = qemuCaps->tcgCPUModelInfo; + if (migratable && model && !model->migratability) + return 1; + if (ARCH_IS_S390(qemuCaps->arch)) - ret = virQEMUCapsInitCPUModelS390(qemuCaps, model, cpu); + ret = virQEMUCapsInitCPUModelS390(qemuCaps, model, cpu, migratable); else if (ARCH_IS_X86(qemuCaps->arch)) - ret = virQEMUCapsInitCPUModelX86(qemuCaps, type, model, cpu); + ret = virQEMUCapsInitCPUModelX86(qemuCaps, type, model, cpu, migratable); if (ret == 0) cpu->fallback = VIR_CPU_FALLBACK_FORBID; @@ -3268,7 +3282,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, cpu->match = VIR_CPU_MATCH_EXACT; cpu->fallback = VIR_CPU_FALLBACK_ALLOW; - if ((rc = virQEMUCapsInitCPUModel(qemuCaps, type, cpu)) < 0) { + if ((rc = virQEMUCapsInitCPUModel(qemuCaps, type, cpu, false)) < 0) { goto error; } else if (rc == 1) { VIR_DEBUG("No host CPU model info from QEMU; probing host CPU directly"); diff --git a/src/qemu/qemu_capspriv.h b/src/qemu/qemu_capspriv.h index 61ccd4517..1baaaf334 100644 --- a/src/qemu/qemu_capspriv.h +++ b/src/qemu/qemu_capspriv.h @@ -81,7 +81,8 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, int virQEMUCapsInitCPUModel(virQEMUCapsPtr qemuCaps, virDomainVirtType type, - virCPUDefPtr cpu); + virCPUDefPtr cpu, + bool migratable); void virQEMUCapsSetCPUModelInfo(virQEMUCapsPtr qemuCaps, diff --git a/tests/cputest.c b/tests/cputest.c index 3d3e43f16..8c07cf4f6 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -709,7 +709,7 @@ cpuTestJSONCPUID(const void *arg) cpu->match = VIR_CPU_MATCH_EXACT; cpu->fallback = VIR_CPU_FALLBACK_FORBID; - if (virQEMUCapsInitCPUModel(qemuCaps, VIR_DOMAIN_VIRT_KVM, cpu) != 0) + if (virQEMUCapsInitCPUModel(qemuCaps, VIR_DOMAIN_VIRT_KVM, cpu, false) != 0) goto cleanup; ret = cpuTestCompareXML(data->arch, cpu, result, false); -- 2.12.0
On 03/31/2017 01:54 PM, Jiri Denemark wrote:
The caller can ask for a migratable CPU model by passing true for the new parameter.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 36 +++++++++++++++++++++++++----------- src/qemu/qemu_capspriv.h | 3 ++- tests/cputest.c | 2 +- 3 files changed, 28 insertions(+), 13 deletions(-)
ACK, John
We will need to store two more host CPU models and nested structs look better than separate items with long complicated names. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 65 ++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b8e4e47b6..c75aa570c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -414,8 +414,10 @@ struct _virQEMUCaps { * re-computed from the other fields or external data sources every * time we probe QEMU or load the results from the cache. */ - virCPUDefPtr kvmCPUModel; - virCPUDefPtr tcgCPUModel; + struct { + virCPUDefPtr kvm; + virCPUDefPtr tcg; + } hostCPU; }; struct virQEMUCapsSearchData { @@ -2082,6 +2084,31 @@ virQEMUCapsNew(void) } +static int +virQEMUCapsCopyHostCPUData(virQEMUCapsPtr dst, + virQEMUCapsPtr src) +{ + + if (src->kvmCPUModelInfo && + !(dst->kvmCPUModelInfo = qemuMonitorCPUModelInfoCopy(src->kvmCPUModelInfo))) + return -1; + + if (src->tcgCPUModelInfo && + !(dst->tcgCPUModelInfo = qemuMonitorCPUModelInfoCopy(src->tcgCPUModelInfo))) + return -1; + + if (src->hostCPU.kvm && + !(dst->hostCPU.kvm = virCPUDefCopy(src->hostCPU.kvm))) + return -1; + + if (src->hostCPU.tcg && + !(dst->hostCPU.tcg = virCPUDefCopy(src->hostCPU.tcg))) + return -1; + + return 0; +} + + virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) { virQEMUCapsPtr ret = virQEMUCapsNew(); @@ -2119,20 +2146,7 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) goto error; } - if (qemuCaps->kvmCPUModel && - !(ret->kvmCPUModel = virCPUDefCopy(qemuCaps->kvmCPUModel))) - goto error; - - if (qemuCaps->tcgCPUModel && - !(ret->tcgCPUModel = virCPUDefCopy(qemuCaps->tcgCPUModel))) - goto error; - - if (qemuCaps->kvmCPUModelInfo && - !(ret->kvmCPUModelInfo = qemuMonitorCPUModelInfoCopy(qemuCaps->kvmCPUModelInfo))) - goto error; - - if (qemuCaps->tcgCPUModelInfo && - !(ret->tcgCPUModelInfo = qemuMonitorCPUModelInfoCopy(qemuCaps->tcgCPUModelInfo))) + if (virQEMUCapsCopyHostCPUData(ret, qemuCaps) < 0) goto error; if (VIR_ALLOC_N(ret->machineTypes, qemuCaps->nmachineTypes) < 0) @@ -2183,8 +2197,8 @@ void virQEMUCapsDispose(void *obj) qemuMonitorCPUModelInfoFree(qemuCaps->kvmCPUModelInfo); qemuMonitorCPUModelInfoFree(qemuCaps->tcgCPUModelInfo); - virCPUDefFree(qemuCaps->kvmCPUModel); - virCPUDefFree(qemuCaps->tcgCPUModel); + virCPUDefFree(qemuCaps->hostCPU.kvm); + virCPUDefFree(qemuCaps->hostCPU.tcg); } void @@ -2413,9 +2427,9 @@ virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps, virDomainVirtType type) { if (type == VIR_DOMAIN_VIRT_KVM) - return qemuCaps->kvmCPUModel; + return qemuCaps->hostCPU.kvm; else - return qemuCaps->tcgCPUModel; + return qemuCaps->hostCPU.tcg; } @@ -3296,9 +3310,9 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, } if (type == VIR_DOMAIN_VIRT_KVM) - qemuCaps->kvmCPUModel = cpu; + qemuCaps->hostCPU.kvm = cpu; else - qemuCaps->tcgCPUModel = cpu; + qemuCaps->hostCPU.tcg = cpu; cleanup: virCPUDefFree(hostCPU); @@ -4053,10 +4067,9 @@ virQEMUCapsReset(virQEMUCapsPtr qemuCaps) qemuCaps->kvmCPUModelInfo = NULL; qemuCaps->tcgCPUModelInfo = NULL; - virCPUDefFree(qemuCaps->kvmCPUModel); - virCPUDefFree(qemuCaps->tcgCPUModel); - qemuCaps->kvmCPUModel = NULL; - qemuCaps->tcgCPUModel = NULL; + virCPUDefFree(qemuCaps->hostCPU.kvm); + virCPUDefFree(qemuCaps->hostCPU.tcg); + memset(&qemuCaps->hostCPU, 0, sizeof(qemuCaps->hostCPU)); } -- 2.12.0
On 03/31/2017 01:54 PM, Jiri Denemark wrote:
We will need to store two more host CPU models and nested structs look better than separate items with long complicated names.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 65 ++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 26 deletions(-)
There's two patches in here... One to have a separate virQEMUCapsCopyHostCPUData and the other to create the 'hostCPU'
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b8e4e47b6..c75aa570c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -414,8 +414,10 @@ struct _virQEMUCaps { * re-computed from the other fields or external data sources every * time we probe QEMU or load the results from the cache. */ - virCPUDefPtr kvmCPUModel; - virCPUDefPtr tcgCPUModel; + struct { + virCPUDefPtr kvm; + virCPUDefPtr tcg; + } hostCPU; };
Considering what you do in the next patch... Further pushing down kvm & tcg into another struct, why not introduce virQEMUCapsCPUModel now and make it a bit "flatter": struct virQEMUCapsCPUModel capsCPUModel; where virQEMUCapsCPUModel is: virCPUDefPtr kvmFull; virCPUDefPtr tcgFull; and eventually adds virCPUDefPtr kvmMigrate; virCPUDefPtr tcgMigrate; You could also eventually have use the Ptr and pass by & rather than seeing things like "hostCpu.kvm.full", the helper routines would take a virQEMUCapsCPUModelPtr and be able to use "capsCPUModel->kvmFull", "capsCPUModel->tcgFull", etc. I only say this because when I see "a.b.c" in code, I have two thoughts - one is this a union and is there anyway to "hide" the 'path.to.the.data.we.want'...
struct virQEMUCapsSearchData { @@ -2082,6 +2084,31 @@ virQEMUCapsNew(void) }
+static int +virQEMUCapsCopyHostCPUData(virQEMUCapsPtr dst, + virQEMUCapsPtr src) +{ + + if (src->kvmCPUModelInfo && + !(dst->kvmCPUModelInfo = qemuMonitorCPUModelInfoCopy(src->kvmCPUModelInfo))) + return -1; + + if (src->tcgCPUModelInfo && + !(dst->tcgCPUModelInfo = qemuMonitorCPUModelInfoCopy(src->tcgCPUModelInfo))) + return -1; + + if (src->hostCPU.kvm && + !(dst->hostCPU.kvm = virCPUDefCopy(src->hostCPU.kvm))) + return -1; + + if (src->hostCPU.tcg && + !(dst->hostCPU.tcg = virCPUDefCopy(src->hostCPU.tcg))) + return -1; + + return 0; +} + + virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) { virQEMUCapsPtr ret = virQEMUCapsNew(); @@ -2119,20 +2146,7 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) goto error; }
- if (qemuCaps->kvmCPUModel && - !(ret->kvmCPUModel = virCPUDefCopy(qemuCaps->kvmCPUModel))) - goto error; - - if (qemuCaps->tcgCPUModel && - !(ret->tcgCPUModel = virCPUDefCopy(qemuCaps->tcgCPUModel))) - goto error; - - if (qemuCaps->kvmCPUModelInfo && - !(ret->kvmCPUModelInfo = qemuMonitorCPUModelInfoCopy(qemuCaps->kvmCPUModelInfo))) - goto error; - - if (qemuCaps->tcgCPUModelInfo && - !(ret->tcgCPUModelInfo = qemuMonitorCPUModelInfoCopy(qemuCaps->tcgCPUModelInfo))) + if (virQEMUCapsCopyHostCPUData(ret, qemuCaps) < 0) goto error;
The above is "somewhat" it's own hunk with some code motion and some change to use the hostCpu.{kvm|tcg}
if (VIR_ALLOC_N(ret->machineTypes, qemuCaps->nmachineTypes) < 0) @@ -2183,8 +2197,8 @@ void virQEMUCapsDispose(void *obj)
qemuMonitorCPUModelInfoFree(qemuCaps->kvmCPUModelInfo); qemuMonitorCPUModelInfoFree(qemuCaps->tcgCPUModelInfo); - virCPUDefFree(qemuCaps->kvmCPUModel); - virCPUDefFree(qemuCaps->tcgCPUModel); + virCPUDefFree(qemuCaps->hostCPU.kvm); + virCPUDefFree(qemuCaps->hostCPU.tcg);
Hmmm. perhaps a 3rd possible patch... Considering patch 5 and a single capsCPUModel, this could become a helper that knows how to virCPUDefFree each of the structure elements.
}
void @@ -2413,9 +2427,9 @@ virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps, virDomainVirtType type) { if (type == VIR_DOMAIN_VIRT_KVM) - return qemuCaps->kvmCPUModel; + return qemuCaps->hostCPU.kvm; else - return qemuCaps->tcgCPUModel; + return qemuCaps->hostCPU.tcg; }
@@ -3296,9 +3310,9 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, }
if (type == VIR_DOMAIN_VIRT_KVM) - qemuCaps->kvmCPUModel = cpu; + qemuCaps->hostCPU.kvm = cpu; else - qemuCaps->tcgCPUModel = cpu; + qemuCaps->hostCPU.tcg = cpu;
Seems to me this hunk "could" be it's own helper too - something like virQEMUCapsSetHostModel... especially once migrate CPU is added...
cleanup: virCPUDefFree(hostCPU); @@ -4053,10 +4067,9 @@ virQEMUCapsReset(virQEMUCapsPtr qemuCaps) qemuCaps->kvmCPUModelInfo = NULL; qemuCaps->tcgCPUModelInfo = NULL;
- virCPUDefFree(qemuCaps->kvmCPUModel); - virCPUDefFree(qemuCaps->tcgCPUModel); - qemuCaps->kvmCPUModel = NULL; - qemuCaps->tcgCPUModel = NULL; + virCPUDefFree(qemuCaps->hostCPU.kvm); + virCPUDefFree(qemuCaps->hostCPU.tcg);
Similar usage of single helper to free everything. John
+ memset(&qemuCaps->hostCPU, 0, sizeof(qemuCaps->hostCPU)); }
On Tue, Apr 04, 2017 at 12:34:49 -0400, John Ferlan wrote:
On 03/31/2017 01:54 PM, Jiri Denemark wrote:
We will need to store two more host CPU models and nested structs look better than separate items with long complicated names.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 65 ++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 26 deletions(-)
There's two patches in here... One to have a separate virQEMUCapsCopyHostCPUData and the other to create the 'hostCPU'
Oops, I accidentally pushed this series when I wanted to push the "qemu: Properly reset all migration capabilities" one :-( Are you OK with me sending the additional changes as follow-up patches rather than reverting the patches and pushing the updated ones? Jirka
On Fri, Apr 07, 2017 at 01:07:52PM +0200, Jiri Denemark wrote:
On Tue, Apr 04, 2017 at 12:34:49 -0400, John Ferlan wrote:
On 03/31/2017 01:54 PM, Jiri Denemark wrote:
We will need to store two more host CPU models and nested structs look better than separate items with long complicated names.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 65 ++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 26 deletions(-)
There's two patches in here... One to have a separate virQEMUCapsCopyHostCPUData and the other to create the 'hostCPU'
Oops, I accidentally pushed this series when I wanted to push the "qemu: Properly reset all migration capabilities" one :-( Are you OK with me sending the additional changes as follow-up patches rather than reverting the patches and pushing the updated ones?
Reverting the patches and doing them right would IMO look better, especially if we ever need to backport them somewhere. Jan
On Fri, Apr 07, 2017 at 13:18:53 +0200, Ján Tomko wrote:
On Fri, Apr 07, 2017 at 01:07:52PM +0200, Jiri Denemark wrote:
On Tue, Apr 04, 2017 at 12:34:49 -0400, John Ferlan wrote:
On 03/31/2017 01:54 PM, Jiri Denemark wrote:
We will need to store two more host CPU models and nested structs look better than separate items with long complicated names.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 65 ++++++++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 26 deletions(-)
There's two patches in here... One to have a separate virQEMUCapsCopyHostCPUData and the other to create the 'hostCPU'
Oops, I accidentally pushed this series when I wanted to push the "qemu: Properly reset all migration capabilities" one :-( Are you OK with me sending the additional changes as follow-up patches rather than reverting the patches and pushing the updated ones?
Reverting the patches and doing them right would IMO look better, especially if we ever need to backport them somewhere.
OK, I guess it's probably also going to be easier to make the changes and review the resulting patches. I've sent the reverts... Jirka
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 91 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 71 insertions(+), 20 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c75aa570c..b426a5abc 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -373,6 +373,14 @@ struct virQEMUCapsMachineType { unsigned int maxCpus; bool hotplugCpus; }; + +typedef struct _virQEMUCapsCPUModel virQEMUCapsCPUModel; +typedef virQEMUCapsCPUModel *virQEMUCapsCPUModelPtr; +struct _virQEMUCapsCPUModel { + virCPUDefPtr full; + virCPUDefPtr migratable; +}; + /* * Update the XML parser/formatter when adding more * information to this struct so that it gets cached @@ -415,8 +423,8 @@ struct _virQEMUCaps { * time we probe QEMU or load the results from the cache. */ struct { - virCPUDefPtr kvm; - virCPUDefPtr tcg; + virQEMUCapsCPUModel kvm; + virQEMUCapsCPUModel tcg; } hostCPU; }; @@ -2097,12 +2105,20 @@ virQEMUCapsCopyHostCPUData(virQEMUCapsPtr dst, !(dst->tcgCPUModelInfo = qemuMonitorCPUModelInfoCopy(src->tcgCPUModelInfo))) return -1; - if (src->hostCPU.kvm && - !(dst->hostCPU.kvm = virCPUDefCopy(src->hostCPU.kvm))) + if (src->hostCPU.kvm.full && + !(dst->hostCPU.kvm.full = virCPUDefCopy(src->hostCPU.kvm.full))) return -1; - if (src->hostCPU.tcg && - !(dst->hostCPU.tcg = virCPUDefCopy(src->hostCPU.tcg))) + if (src->hostCPU.kvm.migratable && + !(dst->hostCPU.kvm.migratable = virCPUDefCopy(src->hostCPU.kvm.migratable))) + return -1; + + if (src->hostCPU.tcg.full && + !(dst->hostCPU.tcg.full = virCPUDefCopy(src->hostCPU.tcg.full))) + return -1; + + if (src->hostCPU.tcg.migratable && + !(dst->hostCPU.tcg.migratable = virCPUDefCopy(src->hostCPU.tcg.migratable))) return -1; return 0; @@ -2197,8 +2213,10 @@ void virQEMUCapsDispose(void *obj) qemuMonitorCPUModelInfoFree(qemuCaps->kvmCPUModelInfo); qemuMonitorCPUModelInfoFree(qemuCaps->tcgCPUModelInfo); - virCPUDefFree(qemuCaps->hostCPU.kvm); - virCPUDefFree(qemuCaps->hostCPU.tcg); + virCPUDefFree(qemuCaps->hostCPU.kvm.full); + virCPUDefFree(qemuCaps->hostCPU.kvm.migratable); + virCPUDefFree(qemuCaps->hostCPU.tcg.full); + virCPUDefFree(qemuCaps->hostCPU.tcg.migratable); } void @@ -2427,9 +2445,9 @@ virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps, virDomainVirtType type) { if (type == VIR_DOMAIN_VIRT_KVM) - return qemuCaps->hostCPU.kvm; + return qemuCaps->hostCPU.kvm.full; else - return qemuCaps->hostCPU.tcg; + return qemuCaps->hostCPU.tcg.full; } @@ -3276,26 +3294,40 @@ virQEMUCapsInitCPUModel(virQEMUCapsPtr qemuCaps, } +static virCPUDefPtr +virQEMUCapsNewHostCPUModel(void) +{ + virCPUDefPtr cpu; + + if (VIR_ALLOC(cpu) < 0) + return NULL; + + cpu->type = VIR_CPU_TYPE_GUEST; + cpu->mode = VIR_CPU_MODE_CUSTOM; + cpu->match = VIR_CPU_MATCH_EXACT; + cpu->fallback = VIR_CPU_FALLBACK_ALLOW; + + return cpu; +} + + void virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, virCapsPtr caps, virDomainVirtType type) { virCPUDefPtr cpu = NULL; + virCPUDefPtr migCPU = NULL; virCPUDefPtr hostCPU = NULL; + virQEMUCapsCPUModelPtr model; int rc; if (!caps || !virQEMUCapsGuestIsNative(caps->host.arch, qemuCaps->arch)) return; - if (VIR_ALLOC(cpu) < 0) + if (!(cpu = virQEMUCapsNewHostCPUModel())) goto error; - cpu->type = VIR_CPU_TYPE_GUEST; - cpu->mode = VIR_CPU_MODE_CUSTOM; - cpu->match = VIR_CPU_MATCH_EXACT; - cpu->fallback = VIR_CPU_FALLBACK_ALLOW; - if ((rc = virQEMUCapsInitCPUModel(qemuCaps, type, cpu, false)) < 0) { goto error; } else if (rc == 1) { @@ -3309,10 +3341,26 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, goto error; } + if (!(migCPU = virQEMUCapsNewHostCPUModel())) + goto error; + + if ((rc = virQEMUCapsInitCPUModel(qemuCaps, type, migCPU, true)) < 0) { + goto error; + } else if (rc == 1) { + VIR_DEBUG("CPU migratibility not provided by QEMU"); + + virCPUDefFree(migCPU); + if (!(migCPU = virCPUCopyMigratable(qemuCaps->arch, cpu))) + goto error; + } + if (type == VIR_DOMAIN_VIRT_KVM) - qemuCaps->hostCPU.kvm = cpu; + model = &qemuCaps->hostCPU.kvm; else - qemuCaps->hostCPU.tcg = cpu; + model = &qemuCaps->hostCPU.tcg; + + model->full = cpu; + model->migratable = migCPU; cleanup: virCPUDefFree(hostCPU); @@ -3320,6 +3368,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, error: virCPUDefFree(cpu); + virCPUDefFree(migCPU); virResetLastError(); goto cleanup; } @@ -4067,8 +4116,10 @@ virQEMUCapsReset(virQEMUCapsPtr qemuCaps) qemuCaps->kvmCPUModelInfo = NULL; qemuCaps->tcgCPUModelInfo = NULL; - virCPUDefFree(qemuCaps->hostCPU.kvm); - virCPUDefFree(qemuCaps->hostCPU.tcg); + virCPUDefFree(qemuCaps->hostCPU.kvm.full); + virCPUDefFree(qemuCaps->hostCPU.kvm.migratable); + virCPUDefFree(qemuCaps->hostCPU.tcg.full); + virCPUDefFree(qemuCaps->hostCPU.tcg.migratable); memset(&qemuCaps->hostCPU, 0, sizeof(qemuCaps->hostCPU)); } -- 2.12.0
On 03/31/2017 01:54 PM, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 91 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 71 insertions(+), 20 deletions(-)
There's three patches in one here... Adding virQEMUCapsCPUModel, adding virQEMUCapsNewHostCPUModel, and creating the migratable CPUs defs and code. Once I got to this patch, I went back to patch 4 with my thoughts around nested structs and a flatter capsCPUModel instead...
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c75aa570c..b426a5abc 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -373,6 +373,14 @@ struct virQEMUCapsMachineType { unsigned int maxCpus; bool hotplugCpus; }; + +typedef struct _virQEMUCapsCPUModel virQEMUCapsCPUModel; +typedef virQEMUCapsCPUModel *virQEMUCapsCPUModelPtr; +struct _virQEMUCapsCPUModel { + virCPUDefPtr full; + virCPUDefPtr migratable; +}; + /* * Update the XML parser/formatter when adding more * information to this struct so that it gets cached @@ -415,8 +423,8 @@ struct _virQEMUCaps { * time we probe QEMU or load the results from the cache. */ struct { - virCPUDefPtr kvm; - virCPUDefPtr tcg; + virQEMUCapsCPUModel kvm; + virQEMUCapsCPUModel tcg; } hostCPU; };
@@ -2097,12 +2105,20 @@ virQEMUCapsCopyHostCPUData(virQEMUCapsPtr dst, !(dst->tcgCPUModelInfo = qemuMonitorCPUModelInfoCopy(src->tcgCPUModelInfo))) return -1;
- if (src->hostCPU.kvm && - !(dst->hostCPU.kvm = virCPUDefCopy(src->hostCPU.kvm))) + if (src->hostCPU.kvm.full && + !(dst->hostCPU.kvm.full = virCPUDefCopy(src->hostCPU.kvm.full))) return -1;
- if (src->hostCPU.tcg && - !(dst->hostCPU.tcg = virCPUDefCopy(src->hostCPU.tcg))) + if (src->hostCPU.kvm.migratable && + !(dst->hostCPU.kvm.migratable = virCPUDefCopy(src->hostCPU.kvm.migratable))) + return -1; + + if (src->hostCPU.tcg.full && + !(dst->hostCPU.tcg.full = virCPUDefCopy(src->hostCPU.tcg.full))) + return -1; + + if (src->hostCPU.tcg.migratable && + !(dst->hostCPU.tcg.migratable = virCPUDefCopy(src->hostCPU.tcg.migratable))) return -1;
Lots of copy'n'paste, but if you went with a flatter capsCPUModel, then there'd be less churn in name changes and only new settings for kvmMigrate and tcgMigrate.
return 0; @@ -2197,8 +2213,10 @@ void virQEMUCapsDispose(void *obj)
qemuMonitorCPUModelInfoFree(qemuCaps->kvmCPUModelInfo); qemuMonitorCPUModelInfoFree(qemuCaps->tcgCPUModelInfo); - virCPUDefFree(qemuCaps->hostCPU.kvm); - virCPUDefFree(qemuCaps->hostCPU.tcg); + virCPUDefFree(qemuCaps->hostCPU.kvm.full); + virCPUDefFree(qemuCaps->hostCPU.kvm.migratable); + virCPUDefFree(qemuCaps->hostCPU.tcg.full); + virCPUDefFree(qemuCaps->hostCPU.tcg.migratable);
One helper to free...
}
void @@ -2427,9 +2445,9 @@ virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps, virDomainVirtType type) { if (type == VIR_DOMAIN_VIRT_KVM) - return qemuCaps->hostCPU.kvm; + return qemuCaps->hostCPU.kvm.full; else - return qemuCaps->hostCPU.tcg; + return qemuCaps->hostCPU.tcg.full;
This wouldn't need a change with a flat capsCPUModel, just leaving the subsequent patch to handle being able to return the migratable version...
}
@@ -3276,26 +3294,40 @@ virQEMUCapsInitCPUModel(virQEMUCapsPtr qemuCaps, }
+static virCPUDefPtr +virQEMUCapsNewHostCPUModel(void) +{ + virCPUDefPtr cpu; + + if (VIR_ALLOC(cpu) < 0) + return NULL; + + cpu->type = VIR_CPU_TYPE_GUEST; + cpu->mode = VIR_CPU_MODE_CUSTOM; + cpu->match = VIR_CPU_MATCH_EXACT; + cpu->fallback = VIR_CPU_FALLBACK_ALLOW; + + return cpu; +} + + void virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, virCapsPtr caps, virDomainVirtType type) { virCPUDefPtr cpu = NULL; + virCPUDefPtr migCPU = NULL; virCPUDefPtr hostCPU = NULL; + virQEMUCapsCPUModelPtr model; int rc;
if (!caps || !virQEMUCapsGuestIsNative(caps->host.arch, qemuCaps->arch)) return;
- if (VIR_ALLOC(cpu) < 0) + if (!(cpu = virQEMUCapsNewHostCPUModel())) goto error;
- cpu->type = VIR_CPU_TYPE_GUEST; - cpu->mode = VIR_CPU_MODE_CUSTOM; - cpu->match = VIR_CPU_MATCH_EXACT; - cpu->fallback = VIR_CPU_FALLBACK_ALLOW; - if ((rc = virQEMUCapsInitCPUModel(qemuCaps, type, cpu, false)) < 0) { goto error; } else if (rc == 1) { @@ -3309,10 +3341,26 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, goto error; }
+ if (!(migCPU = virQEMUCapsNewHostCPUModel())) + goto error; + + if ((rc = virQEMUCapsInitCPUModel(qemuCaps, type, migCPU, true)) < 0) { + goto error; + } else if (rc == 1) { + VIR_DEBUG("CPU migratibility not provided by QEMU");
migratability (although the dictionary for my email client doesn't like either word).
+ + virCPUDefFree(migCPU); + if (!(migCPU = virCPUCopyMigratable(qemuCaps->arch, cpu))) + goto error; + } + if (type == VIR_DOMAIN_VIRT_KVM) - qemuCaps->hostCPU.kvm = cpu; + model = &qemuCaps->hostCPU.kvm; else - qemuCaps->hostCPU.tcg = cpu; + model = &qemuCaps->hostCPU.tcg; + + model->full = cpu; + model->migratable = migCPU;
If we had a virQEMUCapsSetHostModel this would be included there too hiding from this code...
cleanup: virCPUDefFree(hostCPU); @@ -3320,6 +3368,7 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
error: virCPUDefFree(cpu); + virCPUDefFree(migCPU); virResetLastError(); goto cleanup; } @@ -4067,8 +4116,10 @@ virQEMUCapsReset(virQEMUCapsPtr qemuCaps) qemuCaps->kvmCPUModelInfo = NULL; qemuCaps->tcgCPUModelInfo = NULL;
- virCPUDefFree(qemuCaps->hostCPU.kvm); - virCPUDefFree(qemuCaps->hostCPU.tcg); + virCPUDefFree(qemuCaps->hostCPU.kvm.full); + virCPUDefFree(qemuCaps->hostCPU.kvm.migratable); + virCPUDefFree(qemuCaps->hostCPU.tcg.full); + virCPUDefFree(qemuCaps->hostCPU.tcg.migratable);
This is why I suggested a single API in patch 4... John
memset(&qemuCaps->hostCPU, 0, sizeof(qemuCaps->hostCPU)); }
This will allow us to drop feature filtering from virCPUUpdate where it was just a hack. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 19 ++++++++++++++----- src/qemu/qemu_capabilities.h | 3 ++- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_process.c | 4 ++-- tests/cputest.c | 7 ++++++- 5 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b426a5abc..d4aace56d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2442,12 +2442,20 @@ virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps, virCPUDefPtr virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps, - virDomainVirtType type) + virDomainVirtType type, + bool migratable) { + virQEMUCapsCPUModelPtr model; + if (type == VIR_DOMAIN_VIRT_KVM) - return qemuCaps->hostCPU.kvm.full; + model = &qemuCaps->hostCPU.kvm; else - return qemuCaps->hostCPU.tcg.full; + model = &qemuCaps->hostCPU.tcg; + + if (migratable) + return model->migratable; + else + return model->full; } @@ -2465,7 +2473,7 @@ virQEMUCapsIsCPUModeSupported(virQEMUCapsPtr qemuCaps, virQEMUCapsGuestIsNative(caps->host.arch, qemuCaps->arch); case VIR_CPU_MODE_HOST_MODEL: - return !!virQEMUCapsGetHostModel(qemuCaps, type); + return !!virQEMUCapsGetHostModel(qemuCaps, type, false); case VIR_CPU_MODE_CUSTOM: if (type == VIR_DOMAIN_VIRT_KVM) @@ -5500,7 +5508,8 @@ virQEMUCapsFillDomainCPUCaps(virCapsPtr caps, if (virQEMUCapsIsCPUModeSupported(qemuCaps, caps, domCaps->virttype, VIR_CPU_MODE_HOST_MODEL)) { - virCPUDefPtr cpu = virQEMUCapsGetHostModel(qemuCaps, domCaps->virttype); + virCPUDefPtr cpu = virQEMUCapsGetHostModel(qemuCaps, domCaps->virttype, + false); domCaps->cpu.hostModel = virCPUDefCopy(cpu); } diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index d44682f2a..410fa28b2 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -450,7 +450,8 @@ int virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps, char ***names, size_t *count); virCPUDefPtr virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps, - virDomainVirtType type); + virDomainVirtType type, + bool migratable); bool virQEMUCapsIsCPUModeSupported(virQEMUCapsPtr qemuCaps, virCapsPtr caps, virDomainVirtType type, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 64d2d7105..c1a8ba065 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6886,7 +6886,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, if (def->cpu->mode == VIR_CPU_MODE_CUSTOM) cpuDef = def->cpu; else if (def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) - cpuDef = virQEMUCapsGetHostModel(qemuCaps, def->virtType); + cpuDef = virQEMUCapsGetHostModel(qemuCaps, def->virtType, false); if (cpuDef) { int svm = virCPUCheckFeature(def->os.arch, cpuDef, "svm"); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a20beb13c..07d0883c3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5300,12 +5300,12 @@ qemuProcessUpdateGuestCPU(virDomainDefPtr def, if (def->cpu->check == VIR_CPU_CHECK_PARTIAL && virCPUCompare(caps->host.arch, - virQEMUCapsGetHostModel(qemuCaps, def->virtType), + virQEMUCapsGetHostModel(qemuCaps, def->virtType, false), def->cpu, true) < 0) return -1; if (virCPUUpdate(def->os.arch, def->cpu, - virQEMUCapsGetHostModel(qemuCaps, def->virtType)) < 0) + virQEMUCapsGetHostModel(qemuCaps, def->virtType, true)) < 0) goto cleanup; if (virQEMUCapsGetCPUDefinitions(qemuCaps, def->virtType, diff --git a/tests/cputest.c b/tests/cputest.c index 8c07cf4f6..efa891dc1 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -393,6 +393,7 @@ cpuTestUpdate(const void *arg) const struct data *data = arg; int ret = -1; virCPUDefPtr host = NULL; + virCPUDefPtr migHost = NULL; virCPUDefPtr cpu = NULL; char *result = NULL; @@ -400,7 +401,10 @@ cpuTestUpdate(const void *arg) !(cpu = cpuTestLoadXML(data->arch, data->name))) goto cleanup; - if (virCPUUpdate(host->arch, cpu, host) < 0) + if (!(migHost = virCPUCopyMigratable(data->arch, host))) + goto cleanup; + + if (virCPUUpdate(host->arch, cpu, migHost) < 0) goto cleanup; if (virAsprintf(&result, "%s+%s", data->host, data->name) < 0) @@ -411,6 +415,7 @@ cpuTestUpdate(const void *arg) cleanup: virCPUDefFree(host); virCPUDefFree(cpu); + virCPUDefFree(migHost); VIR_FREE(result); return ret; } -- 2.12.0
On 03/31/2017 01:54 PM, Jiri Denemark wrote:
This will allow us to drop feature filtering from virCPUUpdate where it was just a hack.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 19 ++++++++++++++----- src/qemu/qemu_capabilities.h | 3 ++- src/qemu/qemu_command.c | 2 +- src/qemu/qemu_process.c | 4 ++-- tests/cputest.c | 7 ++++++- 5 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b426a5abc..d4aace56d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2442,12 +2442,20 @@ virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps,
virCPUDefPtr virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps, - virDomainVirtType type) + virDomainVirtType type, + bool migratable) { + virQEMUCapsCPUModelPtr model; + if (type == VIR_DOMAIN_VIRT_KVM) - return qemuCaps->hostCPU.kvm.full; + model = &qemuCaps->hostCPU.kvm; else - return qemuCaps->hostCPU.tcg.full; + model = &qemuCaps->hostCPU.tcg; + + if (migratable) + return model->migratable; + else + return model->full; }
This would have obvious impacts with a flatter model. ACK in general for the algorithm adjustments John
@@ -2465,7 +2473,7 @@ virQEMUCapsIsCPUModeSupported(virQEMUCapsPtr qemuCaps, virQEMUCapsGuestIsNative(caps->host.arch, qemuCaps->arch);
case VIR_CPU_MODE_HOST_MODEL: - return !!virQEMUCapsGetHostModel(qemuCaps, type); + return !!virQEMUCapsGetHostModel(qemuCaps, type, false);
case VIR_CPU_MODE_CUSTOM: if (type == VIR_DOMAIN_VIRT_KVM) @@ -5500,7 +5508,8 @@ virQEMUCapsFillDomainCPUCaps(virCapsPtr caps,
if (virQEMUCapsIsCPUModeSupported(qemuCaps, caps, domCaps->virttype, VIR_CPU_MODE_HOST_MODEL)) { - virCPUDefPtr cpu = virQEMUCapsGetHostModel(qemuCaps, domCaps->virttype); + virCPUDefPtr cpu = virQEMUCapsGetHostModel(qemuCaps, domCaps->virttype, + false); domCaps->cpu.hostModel = virCPUDefCopy(cpu); }
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index d44682f2a..410fa28b2 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -450,7 +450,8 @@ int virQEMUCapsGetCPUDefinitions(virQEMUCapsPtr qemuCaps, char ***names, size_t *count); virCPUDefPtr virQEMUCapsGetHostModel(virQEMUCapsPtr qemuCaps, - virDomainVirtType type); + virDomainVirtType type, + bool migratable); bool virQEMUCapsIsCPUModeSupported(virQEMUCapsPtr qemuCaps, virCapsPtr caps, virDomainVirtType type, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 64d2d7105..c1a8ba065 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6886,7 +6886,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, if (def->cpu->mode == VIR_CPU_MODE_CUSTOM) cpuDef = def->cpu; else if (def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) - cpuDef = virQEMUCapsGetHostModel(qemuCaps, def->virtType); + cpuDef = virQEMUCapsGetHostModel(qemuCaps, def->virtType, false);
if (cpuDef) { int svm = virCPUCheckFeature(def->os.arch, cpuDef, "svm"); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a20beb13c..07d0883c3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5300,12 +5300,12 @@ qemuProcessUpdateGuestCPU(virDomainDefPtr def,
if (def->cpu->check == VIR_CPU_CHECK_PARTIAL && virCPUCompare(caps->host.arch, - virQEMUCapsGetHostModel(qemuCaps, def->virtType), + virQEMUCapsGetHostModel(qemuCaps, def->virtType, false), def->cpu, true) < 0) return -1;
if (virCPUUpdate(def->os.arch, def->cpu, - virQEMUCapsGetHostModel(qemuCaps, def->virtType)) < 0) + virQEMUCapsGetHostModel(qemuCaps, def->virtType, true)) < 0) goto cleanup;
if (virQEMUCapsGetCPUDefinitions(qemuCaps, def->virtType, diff --git a/tests/cputest.c b/tests/cputest.c index 8c07cf4f6..efa891dc1 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -393,6 +393,7 @@ cpuTestUpdate(const void *arg) const struct data *data = arg; int ret = -1; virCPUDefPtr host = NULL; + virCPUDefPtr migHost = NULL; virCPUDefPtr cpu = NULL; char *result = NULL;
@@ -400,7 +401,10 @@ cpuTestUpdate(const void *arg) !(cpu = cpuTestLoadXML(data->arch, data->name))) goto cleanup;
- if (virCPUUpdate(host->arch, cpu, host) < 0) + if (!(migHost = virCPUCopyMigratable(data->arch, host))) + goto cleanup; + + if (virCPUUpdate(host->arch, cpu, migHost) < 0) goto cleanup;
if (virAsprintf(&result, "%s+%s", data->host, data->name) < 0) @@ -411,6 +415,7 @@ cpuTestUpdate(const void *arg) cleanup: virCPUDefFree(host); virCPUDefFree(cpu); + virCPUDefFree(migHost); VIR_FREE(result); return ret; }
Because of the changes done in the previous commit, @host is already a migratable CPU and there's no need to do any additional filtering. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_x86.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index a771b251e..53359ff9b 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2549,8 +2549,7 @@ x86Baseline(virCPUDefPtr *cpus, static int x86UpdateHostModel(virCPUDefPtr guest, - const virCPUDef *host, - virCPUx86MapPtr map) + const virCPUDef *host) { virCPUDefPtr updated = NULL; size_t i; @@ -2559,11 +2558,9 @@ x86UpdateHostModel(virCPUDefPtr guest, if (!(updated = virCPUDefCopyWithoutModel(host))) goto cleanup; - /* Remove non-migratable features by default */ updated->type = VIR_CPU_TYPE_GUEST; updated->mode = VIR_CPU_MODE_CUSTOM; - if (virCPUDefCopyModelFilter(updated, host, true, - x86FeatureIsMigratable, map) < 0) + if (virCPUDefCopyModel(updated, host, true) < 0) goto cleanup; if (guest->vendor_id) { @@ -2627,7 +2624,7 @@ virCPUx86Update(virCPUDefPtr guest, if (guest->mode == VIR_CPU_MODE_HOST_MODEL || guest->match == VIR_CPU_MATCH_MINIMUM) - ret = x86UpdateHostModel(guest, host, map); + ret = x86UpdateHostModel(guest, host); else ret = 0; -- 2.12.0
On 03/31/2017 01:54 PM, Jiri Denemark wrote:
Because of the changes done in the previous commit, @host is already a migratable CPU and there's no need to do any additional filtering.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_x86.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
ACK John
participants (3)
-
Jiri Denemark -
John Ferlan -
Ján Tomko