On Mon, Nov 21, 2016 at 14:11:51 -0500, Jason J. Herne wrote:

Implement compare for s390. Required to test the guest against the host for guest cpu model runnability checking. We always return IDENTICAL to bypass Libvirt's checking. s390 will rely on Qemu to perform the runnability checking.

Implement update for s390. required to support use of cpu "host-model" mode.

Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> --- po/POTFILES.in | 1 + src/cpu/cpu_s390.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/po/POTFILES.in b/po/POTFILES.in index 25867ae..ea70e33 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -46,6 +46,7 @@ src/cpu/cpu.c src/cpu/cpu_arm.c src/cpu/cpu_map.c src/cpu/cpu_ppc64.c +src/cpu/cpu_s390.c src/cpu/cpu_x86.c src/datatypes.c src/driver.c diff --git a/src/cpu/cpu_s390.c b/src/cpu/cpu_s390.c index 04a6bea..0073565 100644 --- a/src/cpu/cpu_s390.c +++ b/src/cpu/cpu_s390.c @@ -71,15 +71,65 @@ s390DataFree(virCPUDataPtr data) VIR_FREE(data); }

+static virCPUCompareResult +virCPUs390Compare(virCPUDefPtr host ATTRIBUTE_UNUSED, + virCPUDefPtr cpu ATTRIBUTE_UNUSED, + bool failMessages ATTRIBUTE_UNUSED)

The indentation is a bit off here.

+{ + return VIR_CPU_COMPARE_IDENTICAL; +}

I know there is no code to detect host CPU, but this essentially means that any guest CPU configuration can be started on any s390 host (I'm talking about KVM, of course). Is this correct or would it make sense to somehow compare the guest CPU with the host model returned by QEMU? Which reminds me I don't think I've ever seen any s390 CPU XML. Could you just add some test cases focused on s390 CPUs to qemuxml2argvtest? And since this series is largely about QEMU capabilities, it would be nice if you could add a few .replies and corresponding .xml files to tests/qemucapabilitiesdata and also use them in domaincapstest? Generating .replies is easy, just run tests/qemucapsprobe /path/to/qemu >caps_2.8.0.s390.replies I think replies from 2.7.0 and 2.8.0 could be enough (unless query-cpu-model-expansion was introduced earlier than in 2.8.0). Note, you may need to regenerate the replies files when query-cpu-model-expansion starts to be used.

+ +static int +virCPUs390Update(virCPUDefPtr guest, + const virCPUDef *host) +{ + virCPUDefPtr updated = NULL; + int ret = -1; + + if (guest->match == VIR_CPU_MATCH_MINIMUM) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Match mode %s not supported"),

All other error messages in this patch start with lower case so I guess this could follow the same pattern.

+ virCPUMatchTypeToString(guest->match)); + goto cleanup; + } +

Are you going to support optional features? If so, the function should update them to require/disable, otherwise I think it should report an error if the guest CPU definition contains them.

+ if (guest->mode != VIR_CPU_MODE_HOST_MODEL) { + ret = 0; + goto cleanup; + } + + if (!host) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("unknown host CPU model")); + goto cleanup; + } + + if (!(updated = virCPUDefCopyWithoutModel(guest))) + goto cleanup; + + updated->mode = VIR_CPU_MODE_CUSTOM; + if (virCPUDefCopyModel(updated, host, true) < 0) + goto cleanup; + + virCPUDefStealModel(guest, updated, false); + guest->mode = VIR_CPU_MODE_CUSTOM; + guest->match = VIR_CPU_MATCH_EXACT;

Are you going to support additional features for host-model CPUs? In other words, does the following XML make sense in s390? <cpu mode='host-model'> <feature name='bla' policy='require'/> <feature name='ble' policy='disable'/> </cpu> If so, the code is insufficient. Otherwise, it's unnecessarily complicated. There's no need to create "updated" CPU model when you don't need to look at the features specified in the original guest CPU definition.

+ ret = 0; + + cleanup: + virCPUDefFree(updated); + return ret; +} + struct cpuArchDriver cpuDriverS390 = { .name = "s390", .arch = archs, .narch = ARRAY_CARDINALITY(archs), - .compare = NULL, + .compare = virCPUs390Compare, .decode = s390Decode, .encode = NULL, .free = s390DataFree, .nodeData = s390NodeData, .baseline = NULL, - .update = NULL, + .update = virCPUs390Update, };

Jirka

On Mon, Nov 21, 2016 at 14:11:52 -0500, Jason J. Herne wrote:

From: "Collin L. Walling" <walling@linux.vnet.ibm.com>

On s390 , the host's features are heavily influenced by not only the host hardware but also by hardware microcode level, host OS version, qemu version and kvm version. In this environment it does not make sense to attempt to report exact host details.

Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> --- src/cpu/cpu_s390.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/src/cpu/cpu_s390.c b/src/cpu/cpu_s390.c index 0073565..2ecbd28 100644 --- a/src/cpu/cpu_s390.c +++ b/src/cpu/cpu_s390.c @@ -48,20 +48,15 @@ s390NodeData(virArch arch)

static int -s390Decode(virCPUDefPtr cpu, +s390Decode(virCPUDefPtr cpu ATTRIBUTE_UNUSED, const virCPUData *data ATTRIBUTE_UNUSED, const char **models ATTRIBUTE_UNUSED, unsigned int nmodels ATTRIBUTE_UNUSED, const char *preferred ATTRIBUTE_UNUSED, unsigned int flags) { - virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES, -1);

- if (cpu->model == NULL && - VIR_STRDUP(cpu->model, "host") < 0) - return -1; - return 0; }

I know it's probably my fault since I (IIRC) suggested this function to do nothing. It should rather just go away completely. And the same applies to s390NodeData. Jirka

On Mon, Nov 21, 2016 at 14:11:53 -0500, Jason J. Herne wrote:

From: "Collin L. Walling" <walling@linux.vnet.ibm.com>

query-cpu-model-expansion is used to get a list of features for a given cpu model name or to get the model and features of the host hardware/environment as seen by Qemu/kvm.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> --- src/qemu/qemu_monitor.c | 60 ++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 22 +++++++++++ src/qemu/qemu_monitor_json.c | 94 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 +++ 4 files changed, 182 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 0bfc1a8..927ef39 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3615,6 +3615,66 @@ qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr cpu)

int +qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, + const char *type, + const char *model_name, + qemuMonitorCPUModelInfoPtr *model_info) +{ + VIR_DEBUG("model_info=%p", model_info);

Printing type and model_name would be even more interesting than model_info pointer.

+ + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONGetCPUModelExpansion(mon, type, model_name, model_info); +} + + +void +qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info) +{ + size_t i; + + if (!model_info) + return; + VIR_FREE(model_info->name); + for (i = 0; i < model_info->nprops; i++) + VIR_FREE(model_info->props[i].name); + VIR_FREE(model_info);

I think this code would be slightly nicer in the following form: if (!model_info) return; for (i = 0; i < model_info->nprops; i++) VIR_FREE(model_info->props[i].name); VIR_FREE(model_info->name); VIR_FREE(model_info);

+} ... diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ef8672c..af50997 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4930,6 +4930,100 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon, return ret; }

+int +qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, + const char *type, + const char *model_name, + qemuMonitorCPUModelInfoPtr *model_info) +{ + int ret = -1; + virJSONValuePtr model; + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + virJSONValuePtr data; + virJSONValuePtr cpu_model; + virJSONValuePtr cpu_props; + qemuMonitorCPUModelInfoPtr newmodel = NULL;

Hmm, would you mind s/newmodel/cpu/ or something similar? I found the "newmodel" name to be quite confusing.

+ char const *cpu_name; + int n;

I think the "n" variable is not really necessary.

+ size_t i; + + *model_info = NULL; + + if (!(model = virJSONValueNewObject())) + goto cleanup; + + if (virJSONValueObjectAppendString(model, "name", model_name) < 0) + goto cleanup; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-expansion", + "s:type", type, + "a:model", model, + NULL))) + goto cleanup; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + goto cleanup; + + if (!(data = virJSONValueObjectGetObject(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-cpu-model-expansion reply was missing return data")); + goto cleanup; + }

Just drop this condition, "return" object is guaranteed to exist if qemuMonitorJSONCheckError() returns zero. It's perfectly OK to do data = virJSONValueObjectGetObject(reply, "return"); if (!(cpu_model = virJSONValueObjectGetObject(data, "model"))) ...

+ + if (!(cpu_model = virJSONValueObjectGetObject(data, "model")))

An error message should be reported here.

+ goto cleanup; + + if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name")))

And here.

+ goto cleanup; + + if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props")))

I believe this should be if (!(cpu_props = virJSONValueObjectGetArray(cpu_model, "props"))) and an error should be reported here.

+ goto cleanup; + + if (VIR_ALLOC(newmodel) < 0) + goto cleanup; + + if (VIR_STRDUP(newmodel->name, cpu_name) < 0) + goto cleanup; + + n = newmodel->nprops = cpu_props->data.object.npairs;

newmodel->nprops = virJSONValueArraySize(cpu_props)

+ + if (VIR_ALLOC_N(newmodel->props, n) < 0) + goto cleanup; + + for (i = 0; i < n; i++) { + const char *prop_name; + bool supported; + + if (!(prop_name = virJSONValueObjectGetKey(cpu_props, i))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-cpu-model-expansion reply data is missing a" + " property name")); + goto cleanup; + } + if (VIR_STRDUP(newmodel->props[i].name, prop_name) < 0) + goto cleanup; + + if (virJSONValueObjectGetBoolean(cpu_props, prop_name, &supported) < 0)

An error should be reported here.

+ goto cleanup; + newmodel->props[i].supported = supported; + } + + ret = 0; + *model_info = newmodel; + newmodel = NULL; + + cleanup: + qemuMonitorCPUModelInfoFree(newmodel); + virJSONValueFree(cmd); + virJSONValueFree(reply); + virJSONValueFree(model); + return ret; +} +

int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, char ***commands) ...

Jirka

On Mon, Nov 21, 2016 at 14:11:54 -0500, Jason J. Herne wrote:

From: "Collin L. Walling" <walling@linux.vnet.ibm.com>

When qmp query-cpu-model-expansion is available probe Qemu for its view of the host model. In kvm environments this can provide a more complete view of the host model because features supported by Qemu and Kvm can be considered.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 109 +++++++++++++++++++++++++++++++++++++++++-- src/qemu/qemu_capabilities.h | 1 + 2 files changed, 105 insertions(+), 5 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index cfd090c..b2c70cf 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -352,6 +352,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "ivshmem-doorbell", /* 240 */ "query-qmp-schema", "gluster.debug_level", + "query-cpu-model-expansion", );

@@ -394,6 +395,8 @@ struct _virQEMUCaps { size_t ngicCapabilities; virGICCapability *gicCapabilities;

+ qemuMonitorCPUModelInfoPtr hostCPUModelInfo; + /* Anything below is not stored in the cache since the values are * re-computed from the other fields or external data sources every * time we probe QEMU or load the results from the cache. @@ -1493,7 +1496,8 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "rtc-reset-reinjection", QEMU_CAPS_RTC_RESET_REINJECTION }, { "migrate-incoming", QEMU_CAPS_INCOMING_DEFER }, { "query-hotpluggable-cpus", QEMU_CAPS_QUERY_HOTPLUGGABLE_CPUS }, - { "query-qmp-schema", QEMU_CAPS_QUERY_QMP_SCHEMA } + { "query-qmp-schema", QEMU_CAPS_QUERY_QMP_SCHEMA }, + { "query-cpu-model-expansion", QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION}, };

struct virQEMUCapsStringFlags virQEMUCapsMigration[] = { @@ -2131,6 +2135,10 @@ virQEMUCapsPtr virQEMUCapsNewCopy(virQEMUCapsPtr qemuCaps) !(ret->hostCPUModel = virCPUDefCopy(qemuCaps->hostCPUModel))) goto error;

+ if (qemuCaps->hostCPUModelInfo && + !(ret->hostCPUModelInfo = qemuMonitorCPUModelInfoCopy(qemuCaps->hostCPUModelInfo))) + goto error; + if (VIR_ALLOC_N(ret->machineTypes, qemuCaps->nmachineTypes) < 0) goto error; ret->nmachineTypes = qemuCaps->nmachineTypes; @@ -2741,6 +2749,18 @@ virQEMUCapsProbeQMPCPUDefinitions(virQEMUCapsPtr qemuCaps, return ret; }

+static int +virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, + qemuMonitorPtr mon)

The indentation is a bit off here.

+{ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION) || + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) + return 0; + + return qemuMonitorGetCPUModelExpansion(mon, "static", "host", + &qemuCaps->hostCPUModelInfo); +} + struct tpmTypeToCaps { int type; virQEMUCapsFlags caps; @@ -2966,6 +2986,29 @@ virQEMUCapsCPUFilterFeatures(const char *name, }

+static int +virQEMUCapsCopyModelFromQEMU(virCPUDefPtr dst, + const qemuMonitorCPUModelInfo *modelInfo) +{ + size_t i; + + if (VIR_STRDUP(dst->model, modelInfo->name) < 0 || + VIR_ALLOC_N(dst->features, modelInfo->nprops) < 0) + return -1; + dst->nfeatures_max = modelInfo->nprops; + dst->nfeatures = 0; + + for (i = 0; i < modelInfo->nprops; i++) { + if (VIR_STRDUP(dst->features[i].name, modelInfo->props[i].name) < 0) + return -1; + dst->features[i].policy = VIR_CPU_FEATURE_REQUIRE;

The qemuMonitorCPUModelInfo structure contains "bool supported" for each feature, so I think the policy should depend on it: if (modelInfo->props[i].supported) dst->features[i].policy = VIR_CPU_FEATURE_REQUIRE; else dst->features[i].policy = VIR_CPU_FEATURE_DISABLE;

+ dst->nfeatures++; + } + + return 0; +}

I'm a bit afraid it's not going to be this easy for x86_64 and I think we should make sure this code is not used for setting host CPU models on architectures we did not check the code with. In other words, I'd expect this function to return -1 on error, 0 on success, and 1 when the caller should fall back to copying host CPU model from caps->host (this would currently happen for arch != s390 or if qemuCaps->hostCPUModelInfo is NULL). Which means the function could just take qemuCaps pointer.

+ + void virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, virCapsPtr caps) @@ -2978,7 +3021,8 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, if (!virQEMUCapsGuestIsNative(caps->host.arch, qemuCaps->arch)) goto error;

- if (caps->host.cpu && caps->host.cpu->model) { + if ((caps->host.cpu && caps->host.cpu->model) + || qemuCaps->hostCPUModelInfo) { if (VIR_ALLOC(cpu) < 0) goto error;

And here, we could just unconditionally initialize cpu, call virQEMUCapsCopyModelFromQEMU, and fall back to the existing code.

@@ -2987,9 +3031,14 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps, cpu->mode = VIR_CPU_MODE_CUSTOM; cpu->match = VIR_CPU_MATCH_EXACT;

- if (virCPUDefCopyModelFilter(cpu, caps->host.cpu, true, - virQEMUCapsCPUFilterFeatures, NULL) < 0) - goto error; + if (qemuCaps->hostCPUModelInfo) { + if (virQEMUCapsCopyModelFromQEMU(cpu, qemuCaps->hostCPUModelInfo) < 0) + goto error; + } else { + if (virCPUDefCopyModelFilter(cpu, caps->host.cpu, true, + virQEMUCapsCPUFilterFeatures, NULL) < 0) + goto error; + } }

qemuCaps->hostCPUModel = cpu; @@ -3031,6 +3080,7 @@ virQEMUCapsLoadCache(virCapsPtr caps, size_t i; int n; xmlNodePtr *nodes = NULL; + xmlNodePtr node = NULL; xmlXPathContextPtr ctxt = NULL; char *str = NULL; long long int l; @@ -3130,6 +3180,37 @@ virQEMUCapsLoadCache(virCapsPtr caps, } VIR_FREE(str);

+ if ((node = virXPathNode("./host/cpu", ctxt)) != NULL) { + xmlNodePtr oldnode = ctxt->node; + ctxt->node = node; + if ((str = virXPathString("string(./model)", ctxt))) { + if (VIR_ALLOC(qemuCaps->hostCPUModelInfo) < 0) + goto cleanup;

So in case ./model element didn't exist (which would actually be an error), qemuCaps->hostCPUModelInfo would happily be accessed even though it was never allocated.

+ if (VIR_STRDUP(qemuCaps->hostCPUModelInfo->name, str) < 0) + goto cleanup; + } + if ((n = virXPathNodeSet("./feature", ctxt, &nodes)) > 0) { + if (VIR_ALLOC_N(qemuCaps->hostCPUModelInfo->props, n) < 0) + goto cleanup; + qemuCaps->hostCPUModelInfo->nprops = n; + for (i = 0; i < n; i++) { + if (!(str = virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing name for a host CPU model" + " feature in QEMU capabilities cache")); + goto cleanup; + } + if (VIR_STRDUP(qemuCaps->hostCPUModelInfo->props[i].name, str) < 0) + goto cleanup; + qemuCaps->hostCPUModelInfo->props[i].supported = true; + } + } + ctxt->node = oldnode; + node = NULL; + VIR_FREE(str); + } + VIR_FREE(nodes); +

Please, move this code into a new function. And I think <host> <cpu> <model>Name</model> <feature name='Feat'/> ... </cpu> </host> is a bit too verbose and nested. What about <hostCPU model='Name'> <feature name='Feat' supported='yes|no'/> ... </hostCPU> Which shows another issue with this code. It expects all features are supported. We also need to store and load the value of props[i].supported.

if ((n = virXPathNodeSet("./cpu", ctxt, &nodes)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse qemu capabilities cpus")); @@ -3310,6 +3391,23 @@ virQEMUCapsFormatCache(virQEMUCapsPtr qemuCaps, virBufferAsprintf(&buf, "<arch>%s</arch>\n", virArchToString(qemuCaps->arch));

+ if (qemuCaps->hostCPUModelInfo) { + virBufferAddLit(&buf, "<host>\n"); + virBufferAdjustIndent(&buf, 2); + virBufferAddLit(&buf, "<cpu>\n"); + virBufferAdjustIndent(&buf, 2); + virBufferAsprintf(&buf, "<model>%s</model>\n", + qemuCaps->hostCPUModelInfo->name); + for (i = 0; i < qemuCaps->hostCPUModelInfo->nprops; i++) { + virBufferAsprintf(&buf, "<feature name='%s'/>\n", + qemuCaps->hostCPUModelInfo->props[i].name); + } + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</cpu>\n"); + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</host>\n"); + } +

And move this into a new function too.

if (qemuCaps->cpuDefinitions) { for (i = 0; i < qemuCaps->cpuDefinitions->nmodels; i++) { virDomainCapsCPUModelPtr cpu = qemuCaps->cpuDefinitions->models + i; @@ -3998,6 +4096,7 @@ virQEMUCapsInitQMPMonitor(virQEMUCapsPtr qemuCaps, goto cleanup; if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_QMP_SCHEMA) && virQEMUCapsProbeQMPSchemaCapabilities(qemuCaps, mon) < 0) + if (virQEMUCapsProbeQMPHostCPU(qemuCaps, mon) < 0) goto cleanup;

Looks like wrong conflict resolution during rebase.

/* 'intel-iommu' shows up as a device since 2.2.0, but can diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 9163572..82e1561 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -387,6 +387,7 @@ typedef enum { QEMU_CAPS_DEVICE_IVSHMEM_DOORBELL, /* -device ivshmem-doorbell */ QEMU_CAPS_QUERY_QMP_SCHEMA, /* query-qmp-schema command */ QEMU_CAPS_GLUSTER_DEBUG_LEVEL, /* -drive gluster.debug_level={0..9} */ + QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION, /* qmp query-cpu-model-expansion */

QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags;

Jirka

On Mon, Nov 21, 2016 at 14:11:55 -0500, Jason J. Herne wrote:

From: "Collin L. Walling" <walling@linux.vnet.ibm.com>

Warn the user when migrating a guest that is using the host-passthrough cpu mode. host-passthrough is not migration safe because the host hypervisor is not attempting to block features that may not exist on the destination host.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> --- src/qemu/qemu_migration.c | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index d4a55d8..f721de7 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -3151,6 +3151,10 @@ qemuMigrationBeginPhase(virQEMUDriverPtr driver, if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_BEGIN3);

+ if (vm->def->cpu && vm->def->cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) + VIR_WARN("cpu mode 'host-passthrough' may fail migration if destination" + " machine is running an older cpu model."); +

The warning is not user visible anyway, so there's no real benefit in adding it. We could document it in formatdomain.html#elementsCPU, though.

if (!qemuMigrationIsAllowed(driver, vm, true, flags)) goto cleanup;

Jirka

On Mon, Nov 21, 2016 at 14:11:56 -0500, Jason J. Herne wrote:

From: "Collin L. Walling" <walling@linux.vnet.ibm.com>

Qemu has abandoned the +/-feature syntax in favor of key=value. Some architectures (s390) do not support +/-feature. So we update libvirt to handle both formats.

If we detect a sufficiently new Qemu (indicated by support for qmp query-cpu-model-expansion) we use key=value else we fall back to +/-feature.

Signed-off-by: Collin L. Walling <walling@linux.vnet.ibm.com> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4a5fce3..1f2da19 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6580,12 +6580,18 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver, switch ((virCPUFeaturePolicy) cpu->features[i].policy) { case VIR_CPU_FEATURE_FORCE: case VIR_CPU_FEATURE_REQUIRE: - virBufferAsprintf(buf, ",+%s", cpu->features[i].name); + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) + virBufferAsprintf(buf, ",%s=on", cpu->features[i].name); + else + virBufferAsprintf(buf, ",+%s", cpu->features[i].name); break;

case VIR_CPU_FEATURE_DISABLE: case VIR_CPU_FEATURE_FORBID: - virBufferAsprintf(buf, ",-%s", cpu->features[i].name); + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) + virBufferAsprintf(buf, ",%s=off", cpu->features[i].name); + else + virBufferAsprintf(buf, ",-%s", cpu->features[i].name); break;

It would be nice to add some tests to qemuxml2argvtest and let them use QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION capability.

case VIR_CPU_FEATURE_OPTIONAL:

Jirka