Quoting Collin Walling (2018-07-11 18:25:00)

Not sure if I agree with the renaming here.

From what I understand, "if a cpu property is not migratable, then the cpu model is also not migratable".

So I think the original naming scheme is fine.

Very much welcome the feedback here from folks with experience... Got to say though as a newbie I spent a crazy amount of time trying to sort out what migratability in qemuMonitorCPUModelInfo and migratable in qemuMonitorCPUProperty actually mean. qemuMonitorCPUModelInfo looks like it should just be a libvirt structure encapsulating the CPUModelInfo in QMP messages. But the CPUModelInfo in QMP messages doesn't contain migratability or migratable. Turns out migratability and (prop) migratable are something libvirt overloads in CPUModelInfo with that's only used when storing host cpu capabilities in virQEMUCapsHostCPUData. Moreover, the mechanism that virQEMUCapsProbeQMPHostCPU uses to get QMP to enumerate both migratable and non-migratable Host CPU properties only seems to work for some archs like X86. Other archs will only enumerate the migratable host properties. So as best I can tell the "migratablity" variable in the qemuMonitorCPUModelInfo structure only means we were able to enumerate both migratable and non-migratable properties of the Host CPU (only when stored in virQEMUCapsHostCPUData) on a limited set of arch like X86. In fact if Host CPU's "migratability" is set to true it likely means the Host CPU's CPUModelInfo contains non-migratable properties in addition to migratable properties. So that is what I was trying to get at renaming 'migratability' to 'props_migratable_valid'. Trying to clarify that in the HostCPU case we were able to enumerate some non-migratable features and correspondingly indicate what properties are and are not migratable in the Host CPU. With that said I would be glad to defer to those with libvirt experience here to tell me if I missed something, we should not change the name, or some other additional cleanup is in order.

On 07/09/2018 11:56 PM, Chris Venteicher wrote:

...

Renamed variable in CPUModelInfo such that props_migratable_valid is true when properties in CPUModelInfo have been updated to accurately indicate if property is / isn't migratable.

Property migratability is not returned directly in QMP messages but rather is sometimes calculated within Libvirt by other means and then stored in CPUModelInfo properties by Libvirt. props_migratable_valid is set to true when this calculation has been done by Libvirt. --- src/qemu/qemu_capabilities.c | 10 +++++----- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor.h | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c7da916f9a..3d78e2e29b 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2410,7 +2410,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, } }

- modelInfo->migratability = true; + modelInfo->props_migratable_valid = true; }

VIR_STEAL_PTR(cpuData->info, modelInfo); @@ -2465,7 +2465,7 @@ virQEMUCapsGetCPUFeatures(virQEMUCapsPtr qemuCaps, }

VIR_STEAL_PTR(*features, list); - if (migratable && !data->info->migratability) + if (migratable && !data->info->props_migratable_valid) ret = 1; else ret = 0; @@ -2864,7 +2864,7 @@ virQEMUCapsInitCPUModel(virQEMUCapsPtr qemuCaps, virQEMUCapsHostCPUDataPtr cpuData = virQEMUCapsGetHostCPUData(qemuCaps, type); int ret = 1;

- if (migratable && cpuData->info && !cpuData->info->migratability) + if (migratable && cpuData->info && !cpuData->info->props_migratable_valid) return 1;

if (ARCH_IS_S390(qemuCaps->arch)) { @@ -3047,7 +3047,7 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps, _("invalid migratability value for host CPU model")); goto cleanup; } - hostCPU->migratability = val == VIR_TRISTATE_BOOL_YES; + hostCPU->props_migratable_valid = val == VIR_TRISTATE_BOOL_YES; VIR_FREE(str);

ctxt->node = hostCPUNode; @@ -3540,7 +3540,7 @@ virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps, virBufferAsprintf(buf, "<hostCPU type='%s' model='%s' migratability='%s'>\n", typeStr, model->name, - model->migratability ? "yes" : "no"); + model->props_migratable_valid ? "yes" : "no"); virBufferAdjustIndent(buf, 2);

for (i = 0; i < model->nprops; i++) { diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index a3278c018e..371aaa15da 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3671,7 +3671,7 @@ qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig) if (VIR_STRDUP(copy->name, orig->name) < 0) goto error;

- copy->migratability = orig->migratability; + copy->props_migratable_valid = orig->props_migratable_valid; copy->nprops = orig->nprops;

for (i = 0; i < orig->nprops; i++) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 18b59be985..208a7f5d21 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1005,7 +1005,7 @@ struct _qemuMonitorCPUModelInfo { char *name; size_t nprops; qemuMonitorCPUPropertyPtr props; - bool migratability; + bool props_migratable_valid; };

typedef enum {

-- Respectfully, - Collin Walling

Quoting Jiri Denemark (2018-07-12 06:59:18)

On Mon, Jul 09, 2018 at 22:56:47 -0500, Chris Venteicher wrote:

...

Renamed variable in CPUModelInfo such that props_migratable_valid is true when properties in CPUModelInfo have been updated to accurately indicate if property is / isn't migratable.

Property migratability is not returned directly in QMP messages but rather is sometimes calculated within Libvirt by other means and then stored in CPUModelInfo properties by Libvirt. props_migratable_valid is set to true when this calculation has been done by Libvirt. --- src/qemu/qemu_capabilities.c | 10 +++++----- src/qemu/qemu_monitor.c | 2 +- src/qemu/qemu_monitor.h | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)

...

...

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 18b59be985..208a7f5d21 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1005,7 +1005,7 @@ struct _qemuMonitorCPUModelInfo { char *name; size_t nprops; qemuMonitorCPUPropertyPtr props; - bool migratability; + bool props_migratable_valid; };

I don't see a reason for renaming the variable. The new name is uglier and may be confusing in exactly the same way you found migratability to be confusing. Just add a comment which would explain that the migratability tells whether we can use the prop->migratable value to check if a particular feature is migratable.

I still have some concerns about the use of the migratability (props_migratability_valid) variable. There seems to be a very narrow case where the CPUModelInfo is non-migratable (contains non-migratable props) ... arch:X86 + name:host. In all other cases, specific model names (ex IvyBridge) or other Archs, the CPUModelInfo is by default migratable yet the variable migratability (props_migratable_valid) is false. There seems to be multiple cases in existing code base where the CPUModelInfo is actually migratable but is treated as non-migratable because the migratability (props_migratability_valid) is false. Not sure if this is really a problem or if I am just missing something yet. Seems like the most efficient thing is to try to sort it out in a stand alone patch that's easy to give feedback on. Regardless, I will try to keep the naming consistent (migratability) and use a comment if needed. Chris

Jirka

On Mon, Jul 09, 2018 at 22:56:49 -0500, Chris Venteicher wrote:

Send both model name and a set of features/properties to QEMU for expansion rather than just the model name.

Required to expand name+props models of the form computed by baseline into fully expanded (all props/features listed) output.

This patch is doing too much at once and is quite hard to review.

--- src/qemu/qemu_capabilities.c | 42 +++++++++++++++++----- src/qemu/qemu_monitor.c | 38 ++++++++++++++++---- src/qemu/qemu_monitor.h | 5 ++- src/qemu/qemu_monitor_json.c | 69 ++++++++++++++++++++++++------------ src/qemu/qemu_monitor_json.h | 7 ++-- tests/cputest.c | 7 +++- 6 files changed, 122 insertions(+), 46 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 3d78e2e29b..72ab012a78 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2343,23 +2343,32 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, qemuMonitorCPUModelInfoPtr modelInfo = NULL; qemuMonitorCPUModelInfoPtr nonMigratable = NULL; virHashTablePtr hash = NULL; - const char *model; + const char *model_name;

Why? If we really want to do this, it should go into a separate patch explaining why it is needed.

qemuMonitorCPUModelExpansionType type; virDomainVirtType virtType; virQEMUCapsHostCPUDataPtr cpuData; int ret = -1; + int err = -1;

We usually call such variable 'rc' and leave it uninitialized.

if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) return 0;

if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) { virtType = VIR_DOMAIN_VIRT_QEMU; - model = "max"; + model_name = "max"; } else { virtType = VIR_DOMAIN_VIRT_KVM; - model = "host"; + model_name = "host"; }

+ if ((VIR_ALLOC(modelInfo) < 0) || + (VIR_ALLOC(nonMigratable) < 0)) + goto cleanup; + + if ((qemuMonitorCPUModelInfoInit(model_name, modelInfo) < 0) || + (qemuMonitorCPUModelInfoInit(model_name, nonMigratable) < 0)) + goto cleanup;

Redundant parentheses. But anyway, VIR_ALLOC + qemuMonitorCPUModelInfoInit combo should be replaced with a single qemuMonitorCPUModelInfoNew function which would do both. As a bonus point, you could never end up with a non-NULL modelInfo structure with name == NULL.

+ cpuData = virQEMUCapsGetHostCPUData(qemuCaps, virtType);

/* Some x86_64 features defined in cpu_map.xml use spelling which differ @@ -2372,16 +2381,31 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, else type = QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC;

- if (qemuMonitorGetCPUModelExpansion(mon, type, model, true, &modelInfo) < 0) + if ((err = qemuMonitorGetCPUModelExpansion(mon, type, true, modelInfo)) < 0) goto cleanup;

- /* Try to check migratability of each feature. */ - if (modelInfo && - qemuMonitorGetCPUModelExpansion(mon, type, model, false, - &nonMigratable) < 0) + if (err == 1) { + ret = 0; /* Qemu can't do expansion 1, exit without error */ + goto cleanup; /* We don't have info so don't update cpuData->info */ + }

It's not very clear to me why qemuMonitorGetCPUModelExpansion needs to report 1. A separate patch with an explanation would be better.

+ + if ((err = qemuMonitorGetCPUModelExpansion(mon, type, false, nonMigratable)) < 0) goto cleanup;

- if (nonMigratable) { + /* Try to check migratability of each feature */ + /* Expansion 1 sets migratable features true + * Expansion 2 sets migratable and non-migratable features true + * (non-migratable set true only in some archs like X86) + * + * If delta between Expansion 1 and 2 exists... + * - both migratable and non-migratable features set prop->value = true + * - migratable features set prop->migatable = VIR_TRISTATE_BOOL_YES + * - non-migratable features set prop->migatable = VIR_TRISTATE_BOOL_NO + */ + if (err == 0) { + /* Expansion 2 succeded + * Qemu expanded both migratable and nonMigratable features */

This comment seems redundant. It's pretty clear from the code that both expansions were successful at this point.

+ qemuMonitorCPUPropertyPtr prop; qemuMonitorCPUPropertyPtr nmProp; size_t i; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 2d9297c3a7..91b946c8b4 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3619,20 +3619,46 @@ qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr cpu) }

+/* + * type static: + * Expand to static base model + delta property changes + * Returned model is invariant and migration safe + * + * model_info->name = base model name + * model_info->props = features to +/- to base model to achive model_name + * + * type full: + * Expand model to enumerate all properties + * Returned model isn't guaranteed to be invariant or migration safe. + * + * model_info->name = base model name + * model_info->props = features to +/- to empty set to achive model_name + * + * type static_full: + * Expand to static base model + delta property changes (pass 0) + * Expand model to enumerate all properties (pass 1) + * Returned model is invariant and migration safe + * + * model_info->name = base model name + * model_info->props = features to +/- to empty set to achive model_name + * + * migratable_only: + * true: QEMU excludes non-migratable features + * false: QEMU includes non-migratable features for some archs like X86 + */

Function description in our public API format and using real qemuMonitorCPUModelExpansionType values would be a lot better.

int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, - bool migratable, - qemuMonitorCPUModelInfoPtr *model_info) + bool migratable_only,

Why do you need to rename migratable as migratable_only? The migratable bool selects whether the CPU model returned by QEMU has to be migratable or not. In any case it would be a separate change.

+ qemuMonitorCPUModelInfoPtr model_info)

If you keep the double pointer there, you could remove some hacks and comments to explain these hacks in the JSON monitor implementation. And you wouldn't need to introduce qemuMonitorCPUModelInfoFreeContents.

{ VIR_DEBUG("type=%d model_name=%s migratable=%d", - type, model_name, migratable); + type, (model_info ? NULLSTR(model_info->name):"NULL"),

The function is not supposed to be called with model_info == NULL, the check here is useless. Not to mention that if model_info was NULL, qemuMonitorJSONGetCPUModelExpansion would segfault anyway. And I believe the model_info->name can't be NULL either. All this would of course change with the double pointer and additionally this function should check a proper value was passed in.

+ migratable_only);

QEMU_CHECK_MONITOR(mon);

- return qemuMonitorJSONGetCPUModelExpansion(mon, type, model_name, - migratable, model_info); + return qemuMonitorJSONGetCPUModelExpansion(mon, type, migratable_only, model_info); }

diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 0b84a91fbc..6b4b527512 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1016,9 +1016,8 @@ typedef enum {

int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, - bool migratable, - qemuMonitorCPUModelInfoPtr *model_info); + bool migratable_only, + qemuMonitorCPUModelInfoPtr model_info);

void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info); void qemuMonitorCPUModelInfoFreeContents(qemuMonitorCPUModelInfoPtr model_info); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 90d43eee97..9b681f4592 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5390,8 +5390,11 @@ qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model) } }

- ignore_value(virJSONValueObjectCreate(&model_json, "s:name", model->name, - "a:props", &cpu_props, NULL)); + if (model->nprops > 0) + ignore_value(virJSONValueObjectCreate(&model_json, "s:name", model->name, + "a:props", &cpu_props, NULL)); + else + ignore_value(virJSONValueObjectCreate(&model_json, "s:name", model->name, NULL));

Just don't fill in cpu_props at all and then you can use "A:props" to cover both cases. Anyway, this can be done in a separate patch.

cleanup: virJSONValueFree(cpu_props); @@ -5440,38 +5443,52 @@ qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model) return model; }

+ +/* return: + * -1 - Execution Failure + * 0 - Success + * 1 - Qemu unable to do expansion leaving "model" unmodified + */ int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, - bool migratable, - qemuMonitorCPUModelInfoPtr *model_info) + bool migratable_only, + qemuMonitorCPUModelInfoPtr model) { int ret = -1; - virJSONValuePtr model = NULL; - virJSONValuePtr props = NULL; + virJSONValuePtr json_model = NULL; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; virJSONValuePtr data; virJSONValuePtr cpu_model; + qemuMonitorCPUModelInfoPtr expanded_model = NULL; + qemuMonitorCPUModelInfoPtr model_info = NULL; const char *typeStr = "";

- *model_info = NULL; + if (!(model_info = qemuMonitorCPUModelInfoCopy(model))) + return -1; + + qemuMonitorCPUModelInfoFreeContents(model);

We usually don't touch output parameters until we know the function succeeded. Double pointer would let you comply with this.

- if (!(model = virJSONValueNewObject())) - goto cleanup; + if (!migratable_only) { + /* Add property to input CPUModelInfo causing QEMU to include + * non-migratable properties for some architectures like X86 */

This comment doesn't really belong here. It's the caller's decision to run this command with appropriate arguments for each architecture.

- if (virJSONValueObjectAppendString(model, "name", model_name) < 0) - goto cleanup; + qemuMonitorCPUProperty prop; + prop.type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN; + prop.value.boolean = false; + prop.migratable = false; + + if (VIR_STRDUP(prop.name, "migratable") < 0) + goto cleanup;

- if (!migratable) { - if (!(props = virJSONValueNewObject()) || - virJSONValueObjectAppendBoolean(props, "migratable", false) < 0 || - virJSONValueObjectAppend(model, "props", props) < 0) + if (VIR_APPEND_ELEMENT(model_info->props, model_info->nprops, prop) < 0)

You'd leak prop.name here.

goto cleanup; - props = NULL; }

+ if (!(json_model = qemuMonitorJSONBuildCPUModelInfoToJSON(model_info))) + goto cleanup; + retry: switch (type) { case QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC: @@ -5486,7 +5503,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,

if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-expansion", "s:type", typeStr, - "a:model", &model, + "a:model", &json_model, NULL))) goto cleanup;

@@ -5498,7 +5515,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, * guest architecture or it is not supported in the host environment. */ if (qemuMonitorJSONHasError(reply, "GenericError")) { - ret = 0; + ret = 1; goto cleanup; }

@@ -5517,7 +5534,9 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, * on the result of the initial "static" expansion. */ if (type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL) { - if (!(model = virJSONValueCopy(cpu_model))) + virJSONValueFree(json_model); + + if (!(json_model = virJSONValueCopy(cpu_model))) goto cleanup;

virJSONValueFree(cmd); @@ -5526,16 +5545,20 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, goto retry; }

- if (!(*model_info = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model))) + if (!(expanded_model = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model))) goto cleanup;

+ *model = *expanded_model; /* overwrite contents */ + ret = 0;

cleanup: + VIR_FREE(expanded_model); /* Free structure but not reused contents */ + qemuMonitorCPUModelInfoFreeContents(model_info); + virJSONValueFree(cmd); virJSONValueFree(reply); - virJSONValueFree(model); - virJSONValueFree(props); + virJSONValueFree(json_model); return ret; }

diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 73e1cb6ace..9950483c5c 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -362,10 +362,9 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,

int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, - bool migratable, - qemuMonitorCPUModelInfoPtr *model_info) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5); + bool migratable_only, + qemuMonitorCPUModelInfoPtr model_info) + ATTRIBUTE_NONNULL(4);

int qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, qemuMonitorCPUModelInfoPtr model_a, diff --git a/tests/cputest.c b/tests/cputest.c index baf2b3c648..27727aa29e 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -495,9 +495,14 @@ cpuTestMakeQEMUCaps(const struct data *data) if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true))) goto error;

+ if ((VIR_ALLOC(model) < 0) || + (qemuMonitorCPUModelInfoInit("host", model) < 0)) + goto cleanup; + +

One empty line would be enough.

if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon), QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC, - "host", true, &model) < 0) + true, model) < 0) goto error;

if (!(qemuCaps = virQEMUCapsNew()))

Jirka

On 07/09/2018 11:56 PM, Chris Venteicher wrote:

Filter out cpu properties in qemuMonitorCPUModelInfo structure based on boolean value of true or false.

Goal is to form a list of "enabled" or "disabled" properties.

Required to convert between cpu model feature / property lists that indicate if property is or isn't include in model and the form of cpu model feature / property lists that only enumerate properties that are actually included in the model. --- src/qemu/qemu_monitor.c | 29 +++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 4 ++++ 2 files changed, 33 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 91b946c8b4..dd8510fbab 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3766,6 +3766,35 @@ qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig) return NULL; }

+ +/* Squash CPU Model Info property list + * removing props of type boolean matching value */ +void +qemuMonitorCPUModelInfoRemovePropByBoolValue(qemuMonitorCPUModelInfoPtr model, + bool value) +{ + qemuMonitorCPUPropertyPtr src, dst; + size_t i, dst_size = 0;

Nit: rename "dst_size" to something like "dst_props" or "dst_nprops"... something to better reflect that this value represents the number of props in dst.

+ + for (i = 0; i < model->nprops; i++) { + src = &(model->props[i]); + dst = &(model->props[dst_size]); + + if ((src->type == QEMU_MONITOR_CPU_PROPERTY_BOOLEAN) && + (src->value.boolean == value)) + continue; + + *dst = *src; + + dst_size++; + } + + model->nprops = dst_size; + + ignore_value(VIR_REALLOC_N(model->props, dst_size)); /* not fatal */ +} + + int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, qemuMonitorCPUModelInfoPtr model_a, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 6b4b527512..9841ab230c 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1028,6 +1028,10 @@ int qemuMonitorCPUModelInfoInit(const char *name, qemuMonitorCPUModelInfoPtr mod qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig);

+void qemuMonitorCPUModelInfoRemovePropByBoolValue( qemuMonitorCPUModelInfoPtr model, + bool value) + ATTRIBUTE_NONNULL(1); + int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, qemuMonitorCPUModelInfoPtr model_a, qemuMonitorCPUModelInfoPtr model_b,

-- Respectfully, - Collin Walling

On Mon, Jul 09, 2018 at 22:56:51 -0500, Chris Venteicher wrote:

Bi-directional conversion functions. Converts model / name and features / properties between the two structures.

Created reusable functions to bridge the internal (virCpuDef) and

s/virCpuDef/virCPUDef/

QEMU/QMP specific structures for describing CPU Models. --- src/qemu/qemu_capabilities.c | 146 +++++++++++++++++++++++++++++------ src/qemu/qemu_capabilities.h | 3 + 2 files changed, 127 insertions(+), 22 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 72ab012a78..d3f2317a1d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2758,7 +2758,8 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps, virCPUDefPtr cpu, bool migratable) { - size_t i; + virCPUDefPtr tmp = NULL; + int ret = -1;

if (!modelInfo) { if (type == VIR_DOMAIN_VIRT_KVM) { @@ -2771,32 +2772,21 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps, return 2; }

- if (VIR_STRDUP(cpu->model, modelInfo->name) < 0 || - VIR_ALLOC_N(cpu->features, modelInfo->nprops) < 0) - return -1; + if (!(tmp = virQEMUCapsCPUModelInfoToCPUDef(migratable, modelInfo))) + goto cleanup;

- cpu->nfeatures_max = modelInfo->nprops; - cpu->nfeatures = 0; + /* Free then copy over model, vendor, vendor_id and features */ + virCPUDefFreeModel(cpu);

- for (i = 0; i < modelInfo->nprops; i++) { - virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures; - qemuMonitorCPUPropertyPtr prop = modelInfo->props + i; - - if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN) - continue; + if (virCPUDefCopyModel(cpu, tmp, true) < 0) + goto cleanup;

You can replace the virCPUDefFreeModel/virCPUDefCopyModel combo with a single virCPUDefStealModel.

- if (VIR_STRDUP(feature->name, prop->name) < 0) - return -1; + ret = 0;

- 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++; - } + cleanup: + virCPUDefFree(tmp);

- return 0; + return ret; }

@@ -3547,6 +3537,118 @@ virQEMUCapsLoadCache(virArch hostArch, return ret; }

+/* virCPUDef model => qemuMonitorCPUModelInfo name + * virCPUDef features => qemuMonitorCPUModelInfo boolean properties */ +qemuMonitorCPUModelInfoPtr +virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef) +{ + size_t i; + qemuMonitorCPUModelInfoPtr cpuModel = NULL; + qemuMonitorCPUModelInfoPtr ret = NULL; + + if (!cpuDef || (VIR_ALLOC(cpuModel) < 0)) + goto cleanup; + + VIR_DEBUG("cpuDef->model = %s", NULLSTR(cpuDef->model)); + + if (VIR_STRDUP(cpuModel->name, cpuDef->model) < 0 || + VIR_ALLOC_N(cpuModel->props, cpuDef->nfeatures) < 0) + goto cleanup; + + /* no visibility on migratability of properties / features */ + cpuModel->props_migratable_valid = false;

I don't think there's any need to set this explicitly as this is the default value.

+ + cpuModel->nprops = 0;

This is also the default value, but since we actually care about the value and increment it later, it's useful to explicitly set it anyway. (In contrast to the migratability bool above)

+ + for (i = 0; i < cpuDef->nfeatures; i++) { + qemuMonitorCPUPropertyPtr prop = &(cpuModel->props[cpuModel->nprops]); + virCPUFeatureDefPtr feature = &(cpuDef->features[i]); + + if (!feature || !(feature->name)) + continue;

feature cannot be NULL at this point.

+ + if (VIR_STRDUP(prop->name, feature->name) < 0) + goto cleanup; + + prop->migratable = VIR_TRISTATE_BOOL_ABSENT;

No need to set this explicitly.

+ prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN; + + switch (feature->policy) { + case -1: /* policy undefined */ + case VIR_CPU_FEATURE_FORCE: + case VIR_CPU_FEATURE_REQUIRE: + prop->value.boolean = true; + break; + + case VIR_CPU_FEATURE_FORBID: + case VIR_CPU_FEATURE_DISABLE: + case VIR_CPU_FEATURE_OPTIONAL: + case VIR_CPU_FEATURE_LAST: + prop->value.boolean = false; + break; + }

I don't see any value in using switch since the compiler won't warn us in case of missing case anyway because feature->policy is int. I'd just write it as if (feature->policy == -1 || feature->policy == VIR_CPU_FEATURE_FORCE || feature->policy == VIR_CPU_FEATURE_REQUIRE) prop->value.boolean = true; or perhaps even better using direct assignment prop->value.boolean = feature->policy == -1 || feature->policy == VIR_CPU_FEATURE_FORCE || feature->policy == VIR_CPU_FEATURE_REQUIRE;

+ + cpuModel->nprops += 1;

cpuModel->nprops++;

+ } + + VIR_STEAL_PTR(ret, cpuModel); + + cleanup: + qemuMonitorCPUModelInfoFree(cpuModel); + return ret; +} + + +/* qemuMonitorCPUModelInfo name => virCPUDef model + * qemuMonitorCPUModelInfo boolean properties => virCPUDef features + * + * migratable_only true: mark non-migratable features as disabled + * false: allow all features as required + */ +virCPUDefPtr +virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, qemuMonitorCPUModelInfoPtr model)

One parameter per line. And I think "migratable" would be as descriptive as "migratable_only" but shorter.

+{ + virCPUDefPtr cpu = NULL; + virCPUDefPtr ret = NULL; + size_t i; + + if (!model || (VIR_ALLOC(cpu) < 0)) + goto cleanup;

This would return an error without reporting any error if model == NULL. But do we really need to check for that? And redundant parentheses again.

+ + VIR_DEBUG("model->name= %s", NULLSTR(model->name)); + + if (VIR_STRDUP(cpu->model, model->name) < 0 || + VIR_ALLOC_N(cpu->features, model->nprops) < 0) + goto cleanup; + + cpu->nfeatures_max = model->nprops; + cpu->nfeatures = 0; + + for (i = 0; i < model->nprops; i++) { + virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures; + qemuMonitorCPUPropertyPtr prop = model->props + i; + + if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN) + continue; + + if (VIR_STRDUP(feature->name, prop->name) < 0) + goto cleanup; + + if (!prop->value.boolean || + (migratable_only && prop->migratable == VIR_TRISTATE_BOOL_NO))

Again, I don't see a reason for renaming migratable to migratable_only.

+ feature->policy = VIR_CPU_FEATURE_DISABLE; + else + feature->policy = VIR_CPU_FEATURE_REQUIRE; + + cpu->nfeatures++; + } + + VIR_STEAL_PTR(ret, cpu); + + cleanup: + virCPUDefFree(cpu); + return ret; +}

static void virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index a048a1cf02..d5cd486295 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -564,6 +564,9 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps, void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps, const char *machineType);

+qemuMonitorCPUModelInfoPtr virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef); +virCPUDefPtr virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, qemuMonitorCPUModelInfoPtr model);

One parameter per line. Anyway somehow I feel these new functions do not belong to qemu_capabilities.h. But I'm not sure what would be the better place for them yet. Jirka

On Mon, Jul 09, 2018 at 22:56:52 -0500, Chris Venteicher wrote:

Previously QMPCommandPtr (handle for issuing QMP commands) required an external char * qmperr to persist over the lifespan of QMPCommand to expose a QMP error string outside of QMPCommand.

Before this change, an external char *qmperr had to be maintained between calls to virQEMUCapsInitQMPCommandNew and virQEMUCapsInitQMPCommandAbort.

This change allows the qmperr pointer to be maintained within the QMPCommand structure avoiding the need to track and maintain the qmperr across a sometimes multi-function call lifespan of a QMPCommand.

Q) Should we eliminate external *qmperr completely as there seems to be no current use of qmperr outside the lifespan of QMPCommand in which an internal qmperr char pointer can persist and be used by anywhere we have access to the QMPCommandPtr? --- src/qemu/qemu_capabilities.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d3f2317a1d..f33152ec40 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4272,6 +4272,7 @@ struct _virQEMUCapsInitQMPCommand { uid_t runUid; gid_t runGid; char **qmperr; + char *qmperr_internal;

I'd probably call it qmperrBuffer or something similar to make it more obvious it's the buffer for qmperr.

char *monarg; char *monpath; char *pidfile; @@ -4349,7 +4350,11 @@ virQEMUCapsInitQMPCommandNew(char *binary,

cmd->runUid = runUid; cmd->runGid = runGid; - cmd->qmperr = qmperr; + + if (qmperr) + cmd->qmperr = qmperr; /* external storage */ + else + cmd->qmperr = &cmd->qmperr_internal; /* cmd internal storage */

/* the ".sock" sufix is important to avoid a possible clash with a qemu * domain called "capabilities"

You don't free the internal buffer anywhere. Jirka

I understand your approach, but I do not 100% agree with exposing the QEMU command stuff outside of qemu_capabilities. In patch 10/11, you have a function defined in qemu_caps for baselining. I'd spawn / kill your QEMU process within there. For when the features flag is set (requiring you to call expansion), I'd have a new function in qemu_caps that follows a similar process for cpu expansion. This would lead to libvirt spawning / killing at most 2 separate instances of QEMU (if --features is present) during hypervisor-cpu-baseline, which I do not believe is all that harmful. On 07/09/2018 11:56 PM, Chris Venteicher wrote:

Commit makes starting a single persistent QEMU instance possible for use over multiple independent QMP commands without starting and stopping QEMU for each QMP command or command type.

Commit allows functions outside qemu_capabilities (ex. qemuConnectBaselineHypervisorCPU in qemu_driver) requiring multiple different QMP messages to be sent to QEMU to issue those requests over a single QMP Command Channel without starting and stopping QEMU for each independent QMP Command usage.

Commit moves following to global scope so parent function can maintain QEMU instance over multiple QMP commands / command types: virQEMUCapsInitQMPCommand virQEMUCapsInitQMPCommandFree

Commit Introduces virQEMUCapsNewQMPCommandConnection to Start and connect to QEMU so QMP commands can be performed.

The new reusable function isolates code for starting QEMU and establishing Monitor connections from code for obtaining capabilities so that arbitrary QMP commands can be exchanged with QEMU. --- src/qemu/qemu_capabilities.c | 61 +++++++++++++++++++++++------------- src/qemu/qemu_capabilities.h | 26 +++++++++++++++ 2 files changed, 66 insertions(+), 21 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index f33152ec40..6f8983384a 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4265,25 +4265,6 @@ virQEMUCapsInitQMPMonitorTCG(virQEMUCapsPtr qemuCaps ATTRIBUTE_UNUSED, }

-typedef struct _virQEMUCapsInitQMPCommand virQEMUCapsInitQMPCommand; -typedef virQEMUCapsInitQMPCommand *virQEMUCapsInitQMPCommandPtr; -struct _virQEMUCapsInitQMPCommand { - char *binary; - uid_t runUid; - gid_t runGid; - char **qmperr; - char *qmperr_internal; - char *monarg; - char *monpath; - char *pidfile; - virCommandPtr cmd; - qemuMonitorPtr mon; - virDomainChrSourceDef config; - pid_t pid; - virDomainObjPtr vm; -}; - - static void virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd) { @@ -4318,7 +4299,7 @@ virQEMUCapsInitQMPCommandAbort(virQEMUCapsInitQMPCommandPtr cmd) }

-static void +void virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd) { if (!cmd) @@ -4334,7 +4315,7 @@ virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd)

static virQEMUCapsInitQMPCommandPtr -virQEMUCapsInitQMPCommandNew(char *binary, +virQEMUCapsInitQMPCommandNew(const char *binary, const char *libDir, uid_t runUid, gid_t runGid, @@ -4475,6 +4456,44 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, }

+/* Start and connect to QEMU so QMP commands can be performed. + */ +virQEMUCapsInitQMPCommandPtr +virQEMUCapsNewQMPCommandConnection(const char *exec, + const char *libDir, uid_t runUid, gid_t runGid, + bool forceTCG)

Nit: line up the 2nd and 3rd line of params with the 1st

+{ + virQEMUCapsInitQMPCommandPtr cmd = NULL; + virQEMUCapsInitQMPCommandPtr rtn_cmd = NULL; + + VIR_DEBUG("exec =%s", NULLSTR(exec));

Nit: remove space between exec and =

+ + /* Allocate and initialize QMPCommand structure */ + if (!(cmd = virQEMUCapsInitQMPCommandNew(exec, libDir, + runUid, runGid, NULL))) + goto cleanup; + + /* Spawn QEMU and establish connection for QMP commands */ + if (virQEMUCapsInitQMPCommandRun(cmd, forceTCG) != 0) + goto cleanup; + + /* Exit capabilities negotiation mode and enter QEMU command mode + * by issuing qmp_capabilities command to QEMU */ + if (qemuMonitorSetCapabilities(cmd->mon) < 0) { + VIR_DEBUG("Failed to set monitor capabilities %s", + virGetLastErrorMessage()); + goto cleanup; + } + + VIR_STEAL_PTR(rtn_cmd, cmd); + + cleanup: + virQEMUCapsInitQMPCommandFree(cmd); + + return rtn_cmd; +}

Other than the above nits, the function looks good to me.

+ + static int virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, const char *libDir, diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index d5cd486295..7be636d739 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -490,6 +490,32 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags;

+typedef struct _virQEMUCapsInitQMPCommand virQEMUCapsInitQMPCommand; +typedef virQEMUCapsInitQMPCommand *virQEMUCapsInitQMPCommandPtr; + +struct _virQEMUCapsInitQMPCommand { + char *binary; + uid_t runUid; + gid_t runGid; + char **qmperr; + char *qmperr_internal; + char *monarg; + char *monpath; + char *pidfile; + virCommandPtr cmd; + qemuMonitorPtr mon; + virDomainChrSourceDef config; + pid_t pid; + virDomainObjPtr vm; +}; + +virQEMUCapsInitQMPCommandPtr +virQEMUCapsNewQMPCommandConnection(const char *exec, + const char *libDir, uid_t runUid, gid_t runGid, + bool forceTCG); + +void virQEMUCapsInitQMPCommandFree(virQEMUCapsInitQMPCommandPtr cmd); + typedef struct _virQEMUCaps virQEMUCaps; typedef virQEMUCaps *virQEMUCapsPtr;

-- Respectfully, - Collin Walling

Drop "(baseline using QEMU)" from Subject to make it shorter. On Mon, Jul 09, 2018 at 22:56:54 -0500, Chris Venteicher wrote:

Baseline cpu model using QEMU/QMP query-cpu-model-baseline

query-cpu-model-baseline only compares two CPUModels so multiple exchanges are needed to evaluate more than two CPUModels. --- src/qemu/qemu_capabilities.c | 85 ++++++++++++++++++++++++++++++++++++ src/qemu/qemu_capabilities.h | 4 ++ 2 files changed, 89 insertions(+)

This code has nothing to do with QEMU capabilities, it should be implemented in qemu_driver.c.

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6f8983384a..e0bf78fbba 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5424,3 +5424,88 @@ virQEMUCapsStripMachineAliases(virQEMUCapsPtr qemuCaps) for (i = 0; i < qemuCaps->nmachineTypes; i++) VIR_FREE(qemuCaps->machineTypes[i].alias); } + + +/* in: + * cpus[0]->model = "z13-base"; + * cpus[0]->features[0].name = "xxx"; + * cpus[0]->features[1].name = "yyy"; + * *** + * cpus[n]->model = "s390x"; + * cpus[n]->features[0].name = "xxx"; + * cpus[n]->features[1].name = "yyy"; + * + * out: + * *baseline->model = "s390x"; + * *baseline->features[0].name = "yyy"; + * + * (ret==0) && (*baseline==NULL) if a QEMU rejects model name or baseline command + */ +int +virQEMUCapsQMPBaselineCPUModel(virQEMUCapsInitQMPCommandPtr cmd, + virCPUDefPtr *cpus, + virCPUDefPtr *baseline) +{ + qemuMonitorCPUModelInfoPtr model_baseline = NULL; + qemuMonitorCPUModelInfoPtr new_model_baseline = NULL; + qemuMonitorCPUModelInfoPtr next_model = NULL;

One space between the type and the variable name would be enough.

+ bool migratable_only = true; + int ret = -1; + size_t i; + + *baseline = NULL; + + if (!cpus || !cpus[0] || !cpus[1]) { + virReportError(VIR_ERR_INVALID_ARG, "%s", _("less than 2 cpus")); + goto cleanup; + }

I guess you're going to special case the single CPU use case in the caller...

+ + for (i = 0; !cpus[i]; i++) { /* last element in cpus == NULL */

As already mentioned by Collin, this can't really work unless you remove '!'.

+ virCPUDefPtr cpu = cpus[i]; + + VIR_DEBUG("cpu[%lu]->model = %s", i, NULLSTR(cpu->model)); + + if (!(next_model = virQEMUCapsCPUModelInfoFromCPUDef(cpu))) { + virReportError(VIR_ERR_INVALID_ARG, "%s", _("cpu without content"));

This would override any error reported by virQEMUCapsCPUModelInfoFromCPUDef.

+ goto cleanup; + } + + if (i == 0) { + model_baseline = next_model; + continue; + }

Alternatively you could just call virQEMUCapsCPUModelInfoFromCPUDef once before the for loop to set model_baseline and start the for loop at index 1.

+ + if (qemuMonitorGetCPUModelBaseline(cmd->mon, model_baseline, + next_model, &new_model_baseline) < 0) + goto cleanup; + + if (!new_model_baseline) { + virReportError(VIR_ERR_INVALID_ARG, + _("QEMU doesn't support baseline or recognize model %s or %s"), + model_baseline->name, + next_model->name); + ret = 0;

This doesn't make a lot of sense. It's a real error so why you'd return 0? And the error message should be reported by qemuMonitorGetCPUModelBaseline.

+ goto cleanup; + } + + qemuMonitorCPUModelInfoFree(model_baseline); + qemuMonitorCPUModelInfoFree(next_model); + + next_model = NULL; + + model_baseline = new_model_baseline; + } + + if (!(*baseline = virQEMUCapsCPUModelInfoToCPUDef(migratable_only, model_baseline))) + goto cleanup; + + VIR_DEBUG("baseline->model = %s", NULLSTR((*baseline)->model));

How could the model name be NULL at this point?

+ + ret = 0; + + cleanup: + qemuMonitorCPUModelInfoFree(model_baseline); + qemuMonitorCPUModelInfoFree(next_model); + + return ret; +} diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 7be636d739..d49c8b32ec 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -593,6 +593,10 @@ void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps, qemuMonitorCPUModelInfoPtr virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef); virCPUDefPtr virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, qemuMonitorCPUModelInfoPtr model);

+int virQEMUCapsQMPBaselineCPUModel(virQEMUCapsInitQMPCommandPtr cmd, + virCPUDefPtr *cpus, + virCPUDefPtr *baseline); + virFileCachePtr virQEMUCapsCacheNew(const char *libDir, const char *cacheDir, uid_t uid,

Jirka

On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:

Transient S390 configurations require using QEMU to compute CPU Model Baseline and to do CPU Feature Expansion.

Start and use a single QEMU instance to do both the baseline and expansion transactions required by BaselineHypervisorCPU.

CPU Feature Expansion uses true / false to indicate if property is/isn't included in model. Baseline only returns property list where all enumerated properties are included.

So are you saying on s390 there's no chance there would be a CPU model with some feature which is included in the CPU model disabled for some reason? Sounds too good to be true :-) (This is the question I referred to in one of my replies to the other patches.) Most of the code you added in this patch is indented by three spaces while we use four spaces in libvirt.

--- src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a35e04a85..6c6107f077 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, virArch arch; virDomainVirtType virttype; virDomainCapsCPUModelsPtr cpuModels; - bool migratable; + bool migratable_only;

Why? The bool says the user specified VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable CPU back. What does the "_only" part mean? This API does not return several CPUs, it only returns a single one and it's either migratable or not.

virCPUDefPtr cpu = NULL; char *cpustr = NULL; char **features = NULL; + virQEMUCapsInitQMPCommandPtr cmd = NULL; + bool forceTCG = false; + qemuMonitorCPUModelInfoPtr modelInfo = NULL;

virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES | VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL); @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0) goto cleanup;

- migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE); - if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO))) goto cleanup;

@@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (!qemuCaps) goto cleanup;

+ /* QEMU can enumerate non-migratable cpu model features for some archs like x86 + * migratable_only == true: ask for and include only migratable features + * migratable_only == false: ask for and include all features + */ + migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE); + + if (ARCH_IS_S390(arch)) { + /* QEMU for S390 arch only enumerates migratable features + * No reason to explicitly ask QEMU for or include non-migratable features + */ + migratable_only = true; + } +

And what if they come up with some features which are not migratable in the future? I don't think there's any reason for this API to mess with the value. The code should just provide the same CPU in both cases for s390.

if (!(cpuModels = virQEMUCapsGetCPUDefinitions(qemuCaps, virttype)) || cpuModels->nmodels == 0) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -13437,18 +13451,31 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,

if (ARCH_IS_X86(arch)) { int rc = virQEMUCapsGetCPUFeatures(qemuCaps, virttype, - migratable, &features); + migratable_only, &features); if (rc < 0) goto cleanup; if (features && rc == 0) { /* We got only migratable features from QEMU if we asked for them, * no further filtering in virCPUBaseline is desired. */ - migratable = false; + migratable_only = false; }

if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels, - (const char **)features, migratable))) + (const char **)features, migratable_only))) goto cleanup; + } else if (ARCH_IS_S390(arch)) { +

No need for this extra empty line.

+ const char *binary = virQEMUCapsGetBinary(qemuCaps); + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + if (!(cmd = virQEMUCapsNewQMPCommandConnection(binary, cfg->libDir, + cfg->user, cfg->group, + forceTCG))) + goto cleanup; + + if ((virQEMUCapsQMPBaselineCPUModel(cmd, cpus, &cpu) < 0) || !cpu)

Hmm, I think you should only call this when the user passed more than one CPU. This API is expected to work even with a single CPU in which case it just expands it if the corresponding flag was set.

+ goto cleanup; /* Content Error */

What did you want to say with the comment? I think you can safely drop it.

+

And this one either.

} else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("computing baseline hypervisor CPU is not supported " @@ -13458,9 +13485,36 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,

cpu->fallback = VIR_CPU_FALLBACK_FORBID;

- if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) && - virCPUExpandFeatures(arch, cpu) < 0) - goto cleanup; + if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) { + if (ARCH_IS_X86(arch)) { + if (virCPUExpandFeatures(arch, cpu) < 0) + goto cleanup; + } else if (ARCH_IS_S390(arch)) { +

Extra empty line.

+ if (!(modelInfo = virQEMUCapsCPUModelInfoFromCPUDef(cpu))) + goto cleanup; + + virCPUDefFree(cpu); /* Null on failure, repopulated on success */

I think it would be better to drop the comment and just do it: cpu = NULL; Oh and since this goes from CPUDef to modelInfo and back, you may actually need to do some extra work to persist some parts of the original CPU definitions which get lost during the translation (e.g., the CPU vendor) if it's applicable for s390. We have to do similar stuff for x86 too when we translate CPUDef into CPUID bits and back.

+ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Feature Expansion not supported with this QEMU binary")); + goto cleanup; + }

Is this necessary? Can't qemuMonitorGetCPUModelExpansion just report the error when it tries to call an unsupported QMP command? Or is the error message confusing?

+ + if (qemuMonitorGetCPUModelExpansion(cmd->mon, + QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL, + migratable_only, modelInfo) < 0) + goto cleanup; + + /* Expansion enumerates all features + * Baseline reply enumerates only in-model (true) features */ + qemuMonitorCPUModelInfoRemovePropByBoolValue(modelInfo, false); + + if (!(cpu = virQEMUCapsCPUModelInfoToCPUDef(migratable_only, modelInfo))) + goto cleanup; + } + }

cpustr = virCPUDefFormat(cpu, NULL);

@@ -13469,6 +13523,8 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, virCPUDefFree(cpu); virObjectUnref(qemuCaps); virStringListFree(features); + virQEMUCapsInitQMPCommandFree(cmd); + qemuMonitorCPUModelInfoFree(modelInfo);

return cpustr; }

Jirka

On 07/17/2018 05:01 PM, David Hildenbrand wrote:

On 13.07.2018 18:00, Jiri Denemark wrote:

...

On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:

...

Transient S390 configurations require using QEMU to compute CPU Model Baseline and to do CPU Feature Expansion.

Start and use a single QEMU instance to do both the baseline and expansion transactions required by BaselineHypervisorCPU.

CPU Feature Expansion uses true / false to indicate if property is/isn't included in model. Baseline only returns property list where all enumerated properties are included.

So are you saying on s390 there's no chance there would be a CPU model with some feature which is included in the CPU model disabled for some reason? Sounds too good to be true :-) (This is the question I referred to in one of my replies to the other patches.)

Giving some background information: When we expand/baseline CPU models, we always expand them to the "-base" variants of our CPU models, which contain some set of features we expect to be around in all sane configurations ("minimal feature set").

It is very unlikely that we ever have something like "z14-base,featx=off", but it could happen - When using an emulator (TCG) - When running nested and the guest hypervisor is started with such a strange CPU model - When something in the HW is very wrong or eventually removed in the future (unlikely but possible)

On some very weird inputs to a baseline request, such a strange model can also be the result. But it is very unusual.

I assume something like "baseline z14-base,featx=off with z14-base" will result in "z14-base,featx=off", too.

That's correct. A CPU model with a feature disabled that is baseline with a CPU model with that same feature enabled will omit that feature in the QMP response. e.g. if z14-base has features x, y, z then "baseline z14-base,featx=off with z14-base" will result in "z14-base,featy=on,featz=on" Since baseline will just report a base cpu and its minimal feature set... I wonder if it makes sense for libvirt to just report the features resulting from the baseline command instead of later calling expansion?

...

Most of the code you added in this patch is indented by three spaces while we use four spaces in libvirt.

...

--- src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a35e04a85..6c6107f077 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, virArch arch; virDomainVirtType virttype; virDomainCapsCPUModelsPtr cpuModels; - bool migratable; + bool migratable_only;

Why? The bool says the user specified VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable CPU back. What does the "_only" part mean? This API does not return several CPUs, it only returns a single one and it's either migratable or not.

...

virCPUDefPtr cpu = NULL; char *cpustr = NULL; char **features = NULL; + virQEMUCapsInitQMPCommandPtr cmd = NULL; + bool forceTCG = false; + qemuMonitorCPUModelInfoPtr modelInfo = NULL;

virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES | VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL); @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0) goto cleanup;

- migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE); - if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO))) goto cleanup;

@@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (!qemuCaps) goto cleanup;

+ /* QEMU can enumerate non-migratable cpu model features for some archs like x86 + * migratable_only == true: ask for and include only migratable features + * migratable_only == false: ask for and include all features + */ + migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE); + + if (ARCH_IS_S390(arch)) { + /* QEMU for S390 arch only enumerates migratable features + * No reason to explicitly ask QEMU for or include non-migratable features + */ + migratable_only = true; + } +

And what if they come up with some features which are not migratable in the future? I don't think there's any reason for this API to mess with the value. The code should just provide the same CPU in both cases for s390.

s390x usually only provides features if they are migratable. Could it happen it the future that we have something that cannot be migrated? Possible but very very unlikely. We cared about migration (even for nested support) right from the beginning. As of now, we have no features that are supported by QEMU that cannot be migrated - in contrast to e.g. x86 (e.g. VMX/SVM). Every new feature has to be whitelisted in QEMU - and we only allow to do so if migration is in place for it.

You're a gold mine on this kind of information. I may have to pester you about some CPU model related stuff in the future :)

...

...

if (!(cpuModels = virQEMUCapsGetCPUDefinitions(qemuCaps, virttype)) || cpuModels->nmodels == 0) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, @@ -13437,18 +13451,31 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,

if (ARCH_IS_X86(arch)) { int rc = virQEMUCapsGetCPUFeatures(qemuCaps, virttype, - migratable, &features); + migratable_only, &features); if (rc < 0) goto cleanup; if (features && rc == 0) { /* We got only migratable features from QEMU if we asked for them, * no further filtering in virCPUBaseline is desired. */ - migratable = false; + migratable_only = false; }

if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels, - (const char **)features, migratable))) + (const char **)features, migratable_only))) goto cleanup; + } else if (ARCH_IS_S390(arch)) { +

No need for this extra empty line.

...

+ const char *binary = virQEMUCapsGetBinary(qemuCaps); + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + if (!(cmd = virQEMUCapsNewQMPCommandConnection(binary, cfg->libDir, + cfg->user, cfg->group, + forceTCG))) + goto cleanup; + + if ((virQEMUCapsQMPBaselineCPUModel(cmd, cpus, &cpu) < 0) || !cpu)

Hmm, I think you should only call this when the user passed more than one CPU. This API is expected to work even with a single CPU in which case it just expands it if the corresponding flag was set.

The QEMU side will bail out if only one model is passed, so this sounds like the right thing to do.

...

...

+ goto cleanup; /* Content Error */

What did you want to say with the comment? I think you can safely drop it.

...

+

And this one either.

...

} else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("computing baseline hypervisor CPU is not supported " @@ -13458,9 +13485,36 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn,

cpu->fallback = VIR_CPU_FALLBACK_FORBID;

- if ((flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) && - virCPUExpandFeatures(arch, cpu) < 0) - goto cleanup; + if (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES) { + if (ARCH_IS_X86(arch)) { + if (virCPUExpandFeatures(arch, cpu) < 0) + goto cleanup; + } else if (ARCH_IS_S390(arch)) { +

Extra empty line.

...

+ if (!(modelInfo = virQEMUCapsCPUModelInfoFromCPUDef(cpu))) + goto cleanup; + + virCPUDefFree(cpu); /* Null on failure, repopulated on success */

I think it would be better to drop the comment and just do it:

cpu = NULL;

Oh and since this goes from CPUDef to modelInfo and back, you may actually need to do some extra work to persist some parts of the original CPU definitions which get lost during the translation (e.g., the CPU vendor) if it's applicable for s390. We have to do similar stuff for x86 too when we translate CPUDef into CPUID bits and back.

My assumption is that there are not such things (e.g. like vendor).

Correct. AFAIK and from what I've seen, s390 only cares about the arch, model, and features data with regards to cpu models and libvirt. -- Respectfully, - Collin Walling

Quoting David Hildenbrand (2018-07-18 02:26:24)

On 18.07.2018 00:39, Collin Walling wrote:

...

On 07/17/2018 05:01 PM, David Hildenbrand wrote:

...

On 13.07.2018 18:00, Jiri Denemark wrote:

...

On Mon, Jul 09, 2018 at 22:56:55 -0500, Chris Venteicher wrote:

...

Transient S390 configurations require using QEMU to compute CPU Model Baseline and to do CPU Feature Expansion.

Start and use a single QEMU instance to do both the baseline and expansion transactions required by BaselineHypervisorCPU.

CPU Feature Expansion uses true / false to indicate if property is/isn't included in model. Baseline only returns property list where all enumerated properties are included.

So are you saying on s390 there's no chance there would be a CPU model with some feature which is included in the CPU model disabled for some reason? Sounds too good to be true :-) (This is the question I referred to in one of my replies to the other patches.)

Giving some background information: When we expand/baseline CPU models, we always expand them to the "-base" variants of our CPU models, which contain some set of features we expect to be around in all sane configurations ("minimal feature set").

It is very unlikely that we ever have something like "z14-base,featx=off", but it could happen - When using an emulator (TCG) - When running nested and the guest hypervisor is started with such a strange CPU model - When something in the HW is very wrong or eventually removed in the future (unlikely but possible)

On some very weird inputs to a baseline request, such a strange model can also be the result. But it is very unusual.

I assume something like "baseline z14-base,featx=off with z14-base" will result in "z14-base,featx=off", too.

That's correct. A CPU model with a feature disabled that is baseline with a CPU model with that same feature enabled will omit that feature in the QMP response.

e.g. if z14-base has features x, y, z then

"baseline z14-base,featx=off with z14-base" will result in "z14-base,featy=on,featz=on"

I am runing tests on both S390 and X86 (hacked QEMU to enable baseline). I don't see a "false" property in the baseline response in any of the logs. I did try to slip a "zpci":false into the query-cpu-model-baseline but I still don't get a false in the response. Here is the request/response for reference. {"execute":"query-cpu-model-baseline", "arguments":{"modela":{"name":"z14"}, "modelb":{"name":"z13","props":{"msa5":true,"exrl":true,"zpci":false}}}, "id":"libvirt-2"} {"return": {"model": {"name": "z13-base","props": {"aen": true, "aefsi": true, "msa5": true, "msa4": true, "msa3": true, "msa2": true, "msa1": true, "sthyi": true, "edat": true, "ri": true, "edat2": true, "vx": true, "ipter": true, "esop": true, "cte": true, "sea_esop2": true, "te": true, "cmm": true}}}, "id": "libvirt-2"}

Usually we try to not chose a model with stripped off base features ("we try to produce a model that looks sane"), but instead fallback to some very ancient CPU model. E.g.

{ "execute": "query-cpu-model-baseline", "arguments" : { "modela": { "name": "z14-base", "props": {"msa" : false}}, "modelb": { "name": "z14"}} }

-> {"return": {"model": {"name": "z800-base", "props": {"etf2": true, "ldisp": true}}}}

We might want to change that behavior in the future however (or maybe it already is like this for some corner cases) - assume some base feature gets dropped by HW in a new CPU generation. We don't always want to fallback to a z900 or so when baselining. So one should assume that we can have disabled features here.

Especially as there is a BUG in QEMU I'll have to fix:

{ "execute": "query-cpu-model-baseline", "arguments" : { "modela": { "name": "z14-base", "props": {"esan3" : false}}, "modelb": { "name": "z14"}} }

-> Segmentation fault

This would have to produce a model with esan3 disabled (very very unlikely to ever happen in real life :) )

The result should be something like {"model": {"name": "z900-base", "props": {"esan3": false}}} or an error that they cannot be baselined.

Seems like were saying I should be filtering out or otherwise property excluding any false properties that are returned. Please correct if I have this wrong. I currently do filtering / exclusion on the result of expansion but seems like I should be doing filtering on baseline output too if we don't do the expansion just in case baseline would return a false property for some reason.

...

Since baseline will just report a base cpu and its minimal feature set... I wonder if it makes sense for libvirt to just report the features resulting from the baseline command instead of later calling expansion?

Yes it does and the docs say:

"Baseline two CPU models, creating a compatible third model. The created model will always be a static, migration-safe CPU model (see "static" CPU model expansion for details)"

...

...

...

Most of the code you added in this patch is indented by three spaces while we use four spaces in libvirt.

...

--- src/qemu/qemu_driver.c | 74 +++++++++++++++++++++++++++++++++++++----- 1 file changed, 65 insertions(+), 9 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 9a35e04a85..6c6107f077 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13400,10 +13400,13 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, virArch arch; virDomainVirtType virttype; virDomainCapsCPUModelsPtr cpuModels; - bool migratable; + bool migratable_only;

Why? The bool says the user specified VIR_CONNECT_BASELINE_CPU_MIGRATABLE which means they want a migratable CPU back. What does the "_only" part mean? This API does not return several CPUs, it only returns a single one and it's either migratable or not.

...

virCPUDefPtr cpu = NULL; char *cpustr = NULL; char **features = NULL; + virQEMUCapsInitQMPCommandPtr cmd = NULL; + bool forceTCG = false; + qemuMonitorCPUModelInfoPtr modelInfo = NULL;

virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES | VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL); @@ -13411,8 +13414,6 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (virConnectBaselineHypervisorCPUEnsureACL(conn) < 0) goto cleanup;

- migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE); - if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_AUTO))) goto cleanup;

@@ -13425,6 +13426,19 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (!qemuCaps) goto cleanup;

+ /* QEMU can enumerate non-migratable cpu model features for some archs like x86 + * migratable_only == true: ask for and include only migratable features + * migratable_only == false: ask for and include all features + */ + migratable_only = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE); + + if (ARCH_IS_S390(arch)) { + /* QEMU for S390 arch only enumerates migratable features + * No reason to explicitly ask QEMU for or include non-migratable features + */ + migratable_only = true; + } +

And what if they come up with some features which are not migratable in the future? I don't think there's any reason for this API to mess with the value. The code should just provide the same CPU in both cases for s390.

s390x usually only provides features if they are migratable. Could it happen it the future that we have something that cannot be migrated? Possible but very very unlikely. We cared about migration (even for nested support) right from the beginning. As of now, we have no features that are supported by QEMU that cannot be migrated - in contrast to e.g. x86 (e.g. VMX/SVM). Every new feature has to be whitelisted in QEMU - and we only allow to do so if migration is in place for it.

A problem here is if I set migratable_only (or migratable) to false then

Here is my understanding: 1) query-cpu-model-baseline will only return migratable properties. 2) query-cpu-model-expansion on S390 only returns migratable properties. In fact, here is what happens if you ask S390 for non-migratable features too: {"execute":"query-cpu-model-expansion", "arguments":{"type":"static","model":{"name":"host","props":{"migratable":false}}},"id":"libvirt-45"} Line [{"id": "libvirt-45", "error": {"class": "GenericError", "desc": "Property '.migratable' not found"}}] 3) There seem to be two usecases for expansion A/ Enumrate migratable properties in the baseline model B/ Enumerate both migratable and nonmigratable props in baseline model. B/ Doesn't work on S390 but A/ does. Here is what A/ looks like: {"execute":"query-cpu-model-baseline", "arguments":{"modela":{"name":"z13-base"},"modelb":{"name":"z13-base"}}, "id":"libvirt-4"} Line [{"return": {"model": {"name": "z13-base"}}, "id": "libvirt-4"}] *** {"execute":"query-cpu-model-expansion", "arguments":{"type":"full","model":{"name":"z13-base"}},"id":"libvirt-5"} Line [{"return": {"model": {"name": "z13-base", "props": {"pfmfi": false, "exrl": true, "stfle45": true, "cmma": false, "dateh2": true, "aen": false, "gen13ptff": true, "dateh": true, "cmmnt": false, "iacc2": true, "parseh": true, "csst": true, "idter": false, "idtes": true, "msa": true, "aefsi": false, "hpma2": false, "csst2": true, "csske": true, "mepoch": false, "msa8": false, "msa7": false, "msa6": false, "msa5": false, "msa4": false, "msa3": false, "msa2": false, "msa1": false, "sthyi": false, "stckf": true, "stfle": true, "edat": false, "etf3": true, "etf2": true, "hfpm": true, "ri": false, "edat2": false, "hfpue": true, "dfp": true, "mvcos": true, "sprogp": true, "sigpif": false, "ldisphp": true, "vx": false, "ipter": false, "emon": true, "cei": false, "cmpsceh": true, "ginste": true, "dfppc": true, "dfpzc": true, "dfphp": true, "stfle49": true, "mepochptff": false, "opc": false, "asnlxr": true, "gpereh": false, "minste2": false, "vxeh": false, "vxpd": false, "esop": false, "ectg": true, "ib": false, "siif": false, "tsi": false, "tpei": false, "esan3": true, "fpe": true, "ibs": false, "zarch": true, "stfle53": true, "sief2": false, "eimm": true, "iep": false, "irbm": false, "srs": true, "kss": false, "cte": false, "ais": false, "fpseh": true, "ltlbc": true, "ldisp": true, "bpb": false, "64bscao": false, "ctop": false, "gs": false, "sema": false, "etf3eh": true, "etf2eh": true, "eec": false, "ppa15": false, "zpci": false, "nonqks": true, "sea_esop2": false, "pfpo": true, "te": false, "cmm": false, "tods": true, "plo": true, "gsls": false, "skey": false}}}, "id": "libvirt-5"}] property "migratable":false is added to query-cpu-model-expansion. If X86 && model "name":"host" (limted of names) you get a successfull result. For all other archs and specific model names like (z13, IvyBridge) you get this: Line [{"id": "libvirt-45","error": {"class": "GenericError", "desc": "Property '.migratable' not found"}}] So unless something changes on the QEMU side you get nothing if you try to get query-cpu-model-expansion to enumerate non-migratable features outside the X86 + name: host type of usecases.

...

You're a gold mine on this kind of information. I may have to pester you about some CPU model related stuff in the future :)

Sure, just CC or ask me and I'm happy to help.

--

Thanks,

David / dhildenb