On Mon, Jul 09, 2018 at 22:56:45 -0500, Chris Venteicher wrote: This is a static function which is not used anywhere yet, you need to move its definition to the patch which will make use of it. Otherwise libvirt would fail to compile at this point. Remember the goal is to be able to compile (and check + syntax-check) libvirt after every single commit. It's not enough if it compiles at the end of a series. ... I would call this variable 'ret' since it is only used to steal the pointer from machine_model and return it. Then, you can even do s/machine_model/model/ if you want. Otherwise the patch looks good. Jirka

Drop "(query-cpu-model-baseline)" from the subject to make it shorter. On Mon, Jul 09, 2018 at 22:56:46 -0500, Chris Venteicher wrote: Please, merge this into a single VIR_DEBUG, otherwise they would be logged on two lines possibly with logs from other threads in between. VIR_DEBUG("model_a->name=%s, model_a->nprops=%lu " "model_b->name=%s, model_b->nprops=%lu", model_a->name, model_a->nprops, model_b->name, model_b->nprops); This function is not called for any capabilities probing, is it? We should normally report an error if QEMU does not support the command and propagate it back to the user who asked for CPU baseline. Not reporting an error is only useful if we don't care if the command is supported by QEMU (i.e., when we probe for capabilities) or when we have a fallback code. Jirka

Not sure if I agree with the renaming here. cpu model is also not migratable". So I think the original naming scheme is fine. On 07/09/2018 11:56 PM, Chris Venteicher wrote: -- Respectfully, - Collin Walling

On Mon, Jul 09, 2018 at 22:56:47 -0500, Chris Venteicher wrote: ... 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. Jirka

These forms modify contents of a qemuMonitorCPUModelInfo structure but do not allocate or free the actual structure. Init - Initialize model name and empty properties within existing structure FreeContents - Free model name and properties within existing structure --- src/qemu/qemu_monitor.c | 35 ++++++++++++++++++++++++++++++++++- src/qemu/qemu_monitor.h | 4 ++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 371aaa15da..2d9297c3a7 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3636,8 +3636,31 @@ qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, } +int +qemuMonitorCPUModelInfoInit(const char *name, qemuMonitorCPUModelInfoPtr model) +{ + int ret = -1; + + if (!model) + goto cleanup; + + model->name = NULL; + model->nprops = 0; + model->props = NULL; + model->props_migratable_valid = false; + + if (VIR_STRDUP(model->name, name) < 0) + goto cleanup; + + ret = 0; + + cleanup: + return ret; +} + + void -qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info) +qemuMonitorCPUModelInfoFreeContents(qemuMonitorCPUModelInfoPtr model_info) { size_t i; @@ -3652,6 +3675,16 @@ qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info) VIR_FREE(model_info->props); VIR_FREE(model_info->name); + + model_info->nprops = 0; + model_info->props_migratable_valid = false; +} + + +void +qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info) +{ + qemuMonitorCPUModelInfoFreeContents(model_info); VIR_FREE(model_info); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 208a7f5d21..0b84a91fbc 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1021,6 +1021,10 @@ int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelInfoPtr *model_info); void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info); +void qemuMonitorCPUModelInfoFreeContents(qemuMonitorCPUModelInfoPtr model_info); + +int qemuMonitorCPUModelInfoInit(const char *name, qemuMonitorCPUModelInfoPtr model) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig); -- 2.17.1

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. --- 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; qemuMonitorCPUModelExpansionType type; virDomainVirtType virtType; virQEMUCapsHostCPUDataPtr cpuData; int ret = -1; + int err = -1; 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; + 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 */ + } + + 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 */ + 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 + */ int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, - bool migratable, - qemuMonitorCPUModelInfoPtr *model_info) + bool migratable_only, + qemuMonitorCPUModelInfoPtr model_info) { VIR_DEBUG("type=%d model_name=%s migratable=%d", - type, model_name, migratable); + type, (model_info ? NULLSTR(model_info->name):"NULL"), + 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)); 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); - 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 */ - 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) 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; + + if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon), QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC, - "host", true, &model) < 0) + true, model) < 0) goto error; if (!(qemuCaps = virQEMUCapsNew())) -- 2.17.1

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; + + 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, -- 2.17.1

On Mon, Jul 09, 2018 at 22:56:50 -0500, Chris Venteicher wrote: The usefulness of this new function depends a question I'll ask in later review of patch 11/11. Single variable per line, please. Redundant parentheses. This should call VIR_REALLOC_N_QUIET and I think the comment is not very useful. s/( /(/ And align this accordingly. Jirka

On 07/09/2018 11:56 PM, Chris Venteicher wrote: It might make sense to rename these to|from "CPUDefS390" or something, since s390x discards the migratability fields and only considers CPU policies enabled and disabled. Or maybe someone with a stronger understanding of other libvirt CPU defs can help make these functions more agnostic. I think this should be set to VIR_TRISTATE_BOOL_ABSENT (which evaluates to the same thing anyway) Set this to false. -- Respectfully, - Collin Walling

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; 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" -- 2.17.1

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) +{ + virQEMUCapsInitQMPCommandPtr cmd = NULL; + virQEMUCapsInitQMPCommandPtr rtn_cmd = NULL; + + VIR_DEBUG("exec =%s", NULLSTR(exec)); + + /* 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; +} + + 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; -- 2.17.1

Quoting Collin Walling (2018-07-11 17:48:54) Seems like a close call here. If folks are ok with spinning up QEMU process twice for a command like hypervisor baseline then... I think this likely becomes a question of where are things headed in terms of relying on QMP commands (or multiple QMP commands) to do specific things in libvirt beyond the scope of querying and caching capabilities. Seems like were moving in the direction of relying on QEMU to track (and calculate based on) the dynamic state of S390 configuration so seemed likely there would be more reliance on QMP commands (individual or multiple) to get things done in libvirt in the future. Those were the reasons that tipped me to pull out the ability to start and maintain QEMU process across multiple QMP commands. Acknowledge I don't have the history here though so welcome discussion and glad to do whatever you think is best.

On 07/09/2018 11:56 PM, Chris Venteicher wrote: "s390x" is not a valid CPU model. Remove the ! from the condition here. Hmmm... since we're fencing who can use baseline based on the architecture, I wonder if it would help to clean things up and have qemuMonitorJSONGetCPUModelBaseline return nonzero if "GenericError" is reported? That would allow you to remove the condition block below, or at the very least clean up the error message to say something like "Either model %s or %s is not supported." -- Respectfully, - Collin Walling

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. --- 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; 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; + } + 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)) { + + 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) + goto cleanup; /* Content Error */ + } 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)) { + + if (!(modelInfo = virQEMUCapsCPUModelInfoFromCPUDef(cpu))) + goto cleanup; + + virCPUDefFree(cpu); /* Null on failure, repopulated on success */ + + 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; + } + + 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; } -- 2.17.1

On 13.07.2018 18:00, Jiri Denemark wrote: 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. 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. The QEMU side will bail out if only one model is passed, so this sounds like the right thing to do. My assumption is that there are not such things (e.g. like vendor). -- Thanks, David / dhildenb

On 18.07.2018 00:39, Collin Walling wrote: 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. 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)" Sure, just CC or ask me and I'm happy to help. -- Thanks, David / dhildenb

On 07/21/2018 12:05 AM, Chris Venteicher wrote: Right... baseline should not be returning any properties paired with false. It constructs a third CPU model with properties that can run on both CPUs. Sending a property paired with "false" in the JSON object is telling QEMU "I want to turn off this feature." The feature will then be omitted from the QMP response. Filtering is not necessary. All properties in baseline or a static expansion response will either be paired with "true" or a feature will be absent. The only occurrence of a property paired with "false" is during a "full" expansion, where QEMU will return ALL features supported by the hypervisor and weather or not that feature is supported on the specified CPU model. Looking back at your code, you'll need to make sure the baseline JSON function captures the props field. If the libvirt command was used with --features flag, then the captured features should be output. Otherwise just output the model name. (Additionally, the --migratable flag won't make a difference for s390). Yes. Yup. This looks right. A "-base" model is a CPU model with a "minimum feature set," so it makes sense to not see any features listed here. AFAIU, a "full" expansion will show *all* features the hypervisor knows about and weather or not that feature can be enabled ("true") on the specified CPU model or not ("false"). All features listed have migration support. -- Respectfully, - Collin Walling