[libvirt] [PATCH v5 00/15] CPU Model Baseline and Comparison for s390x
Note: since I've made some changes to a lot of these patches / split up some patches, I've decided to hold off on adding any r-b's in case there is a specific change that someone does not agree with. Changelog: - Properly refactored code from CPU model expansion function - Introduced a cleanup patch for CPU model expansion function - Introduced patches that modifies the refactored code to suit needs for baseline/comparison - CPU expansion function now accepts a virCPUDefPtr - Removed props parsing from CPU model comparison (they weren't used) - Cleaner error reporting when baselining/comparing with erroneous CPU models / features - Various cleanups based on feedback ___ To run these patches, execute the virsh hypervisor-cpu-compare or hypervisor-cpu-baseline commands and pass an XML file describing one or more CPU definition. You can use the definition from virsh domcapabilities or from a guest XML. There is no need extract it from the file and place it a new one, as the XML parser will look specifically for the CPU tags. ___ These patches hookup the virsh hypervisor-cpu-compare/baseline commands for the s390x architecture. They take an XML file describing some CPU definitions and passes the data to QEMU, where the actual CPU model comparison / baseline calculation is handled (available since QEMU 2.8.5). When baselining CPU models with the --features argument, s390x will report a full CPU model expansion. Thanks. Collin Walling (15): qemu_monitor: refactor cpu model expansion qemu_monitor: expansion cleanups qemu_monitor: use cpu def instead of char for expansion qemu_monitor: add features to CPU model for QMP command qemu_monitor: allow cpu props to be optional qemu_monitor: make qemuMonitorJSONParseCPUModelData command-agnostic qemu_monitor: implement query-cpu-model-baseline qemu_capabilities: introduce QEMU_CAPS_QUERY_CPU_MODEL_BASELINE qemu_driver: hook up query-cpu-model-baseline qemu_driver: expand cpu features after baseline qemu_monitor: implement query-cpu-model-comparison qemu_capabilities: introduce QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON cpu_conf: xml to cpu definition parse helper qemu_driver: hook up query-cpu-model-comparison qemu_driver: improve comparison/baseline error reporting src/conf/cpu_conf.c | 29 +++ src/conf/cpu_conf.h | 5 + src/cpu/cpu.c | 14 +- src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 21 +- src/qemu/qemu_capabilities.h | 4 + src/qemu/qemu_driver.c | 215 +++++++++++++++++ src/qemu/qemu_monitor.c | 39 +++- src/qemu/qemu_monitor.h | 13 +- src/qemu/qemu_monitor_json.c | 279 +++++++++++++++++------ src/qemu/qemu_monitor_json.h | 17 +- tests/cputest.c | 11 +- tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 2 + tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 2 + 19 files changed, 573 insertions(+), 89 deletions(-) -- 2.7.4
Refactor some code in qemuMonitorJSONGetCPUModelExpansion to be later used for the comparison and baseline functions. Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_monitor_json.c | 143 ++++++++++++++++++++++++++++--------------- 1 file changed, 94 insertions(+), 49 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e4404f0..7219d14 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5780,6 +5780,96 @@ qemuMonitorJSONParseCPUModelProperty(const char *key, return 0; } + +static virJSONValuePtr +qemuMonitorJSONMakeCPUModel(const char *model_name, + bool migratable) +{ + virJSONValuePtr model = NULL; + virJSONValuePtr props = NULL; + + if (!(model = virJSONValueNewObject())) + goto error; + + if (virJSONValueObjectAppendString(model, "name", model_name) < 0) + goto error; + + if (!migratable) { + if (!(props = virJSONValueNewObject()) || + virJSONValueObjectAppendBoolean(props, "migratable", false) < 0 || + virJSONValueObjectAppend(model, "props", props) < 0) + goto error; + props = NULL; + } + + return model; + + error: + virJSONValueFree(model); + virJSONValueFree(props); + return NULL; +} + + +static int +qemuMonitorJSONParseCPUModelData(virJSONValuePtr data, + virJSONValuePtr *cpu_model, + virJSONValuePtr *cpu_props, + const char **cpu_name) +{ + if (!(*cpu_model = virJSONValueObjectGetObject(data, "model"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-cpu-model-expansion reply data was missing 'model'")); + return -1; + } + + if (!(*cpu_name = virJSONValueObjectGetString(*cpu_model, "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-cpu-model-expansion reply data was missing 'name'")); + return -1; + } + + if (!(*cpu_props = virJSONValueObjectGetObject(*cpu_model, "props"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-cpu-model-expansion reply data was missing 'props'")); + return -1; + } + + return 0; +} + + +static int +qemuMonitorJSONParseCPUModel(const char *cpu_name, + virJSONValuePtr cpu_props, + qemuMonitorCPUModelInfoPtr *model_info) +{ + qemuMonitorCPUModelInfoPtr machine_model = NULL; + int ret = -1; + + if (VIR_ALLOC(machine_model) < 0) + goto cleanup; + + if (VIR_STRDUP(machine_model->name, cpu_name) < 0) + goto cleanup; + + if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0) + goto cleanup; + + if (virJSONValueObjectForeachKeyValue(cpu_props, + qemuMonitorJSONParseCPUModelProperty, + machine_model) < 0) + goto cleanup; + + VIR_STEAL_PTR(*model_info, machine_model); + ret = 0; + + cleanup: + qemuMonitorCPUModelInfoFree(machine_model); + return ret; +} + + int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, @@ -5789,32 +5879,19 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, { int ret = -1; virJSONValuePtr model = NULL; - virJSONValuePtr props = NULL; virJSONValuePtr cmd = NULL; virJSONValuePtr reply = NULL; virJSONValuePtr data; virJSONValuePtr cpu_model; virJSONValuePtr cpu_props; - qemuMonitorCPUModelInfoPtr machine_model = NULL; char const *cpu_name; const char *typeStr = ""; *model_info = NULL; - if (!(model = virJSONValueNewObject())) - goto cleanup; - - if (virJSONValueObjectAppendString(model, "name", model_name) < 0) + if (!(model = qemuMonitorJSONMakeCPUModel(model_name, migratable))) goto cleanup; - if (!migratable) { - if (!(props = virJSONValueNewObject()) || - virJSONValueObjectAppendBoolean(props, "migratable", false) < 0 || - virJSONValueObjectAppend(model, "props", props) < 0) - goto cleanup; - props = NULL; - } - retry: switch (type) { case QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC: @@ -5850,11 +5927,9 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, data = virJSONValueObjectGetObject(reply, "return"); - if (!(cpu_model = virJSONValueObjectGetObject(data, "model"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-cpu-model-expansion reply data was missing 'model'")); + if (qemuMonitorJSONParseCPUModelData(data, + &cpu_model, &cpu_props, &cpu_name) < 0) goto cleanup; - } /* QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL requests "full" expansion * on the result of the initial "static" expansion. @@ -5869,42 +5944,12 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, goto retry; } - if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-cpu-model-expansion reply data was missing 'name'")); - goto cleanup; - } - - if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-cpu-model-expansion reply data was missing 'props'")); - goto cleanup; - } - - if (VIR_ALLOC(machine_model) < 0) - goto cleanup; - - if (VIR_STRDUP(machine_model->name, cpu_name) < 0) - goto cleanup; - - if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0) - goto cleanup; - - if (virJSONValueObjectForeachKeyValue(cpu_props, - qemuMonitorJSONParseCPUModelProperty, - machine_model) < 0) - goto cleanup; - - ret = 0; - *model_info = machine_model; - machine_model = NULL; + ret = qemuMonitorJSONParseCPUModel(cpu_name, cpu_props, model_info); cleanup: - qemuMonitorCPUModelInfoFree(machine_model); virJSONValueFree(cmd); virJSONValueFree(reply); virJSONValueFree(model); - virJSONValueFree(props); return ret; } -- 2.7.4
On Thu, Sep 19, 2019 at 16:24:52 -0400, Collin Walling wrote:
Refactor some code in qemuMonitorJSONGetCPUModelExpansion to be later used for the comparison and baseline functions.
Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> --- src/qemu/qemu_monitor_json.c | 143 ++++++++++++++++++++++++++++--------------- 1 file changed, 94 insertions(+), 49 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
With refactoring most of the expansion function, let's take care of some additional cleanups. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_monitor_json.c | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 7219d14..3282593 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5877,20 +5877,19 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, bool migratable, qemuMonitorCPUModelInfoPtr *model_info) { - int ret = -1; - virJSONValuePtr model = NULL; - virJSONValuePtr cmd = NULL; - virJSONValuePtr reply = NULL; + VIR_AUTOPTR(virJSONValue) model = NULL; + VIR_AUTOPTR(virJSONValue) cmd = NULL; + VIR_AUTOPTR(virJSONValue) reply = NULL; virJSONValuePtr data; virJSONValuePtr cpu_model; - virJSONValuePtr cpu_props; - char const *cpu_name; + virJSONValuePtr cpu_props = NULL; + const char *cpu_name = ""; const char *typeStr = ""; *model_info = NULL; if (!(model = qemuMonitorJSONMakeCPUModel(model_name, migratable))) - goto cleanup; + return -1; retry: switch (type) { @@ -5908,35 +5907,33 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, "s:type", typeStr, "a:model", &model, NULL))) - goto cleanup; + return -1; if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) - goto cleanup; + return -1; /* Even though query-cpu-model-expansion is advertised by query-commands it * may just return GenericError if it is not implemented for the requested * guest architecture or it is not supported in the host environment. */ - if (qemuMonitorJSONHasError(reply, "GenericError")) { - ret = 0; - goto cleanup; - } + if (qemuMonitorJSONHasError(reply, "GenericError")) + return 0; if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) - goto cleanup; + return -1; data = virJSONValueObjectGetObject(reply, "return"); if (qemuMonitorJSONParseCPUModelData(data, &cpu_model, &cpu_props, &cpu_name) < 0) - goto cleanup; + return -1; /* QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL requests "full" expansion * on the result of the initial "static" expansion. */ if (type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL) { if (!(model = virJSONValueCopy(cpu_model))) - goto cleanup; + return -1; virJSONValueFree(cmd); virJSONValueFree(reply); @@ -5944,13 +5941,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, goto retry; } - ret = qemuMonitorJSONParseCPUModel(cpu_name, cpu_props, model_info); - - cleanup: - virJSONValueFree(cmd); - virJSONValueFree(reply); - virJSONValueFree(model); - return ret; + return qemuMonitorJSONParseCPUModel(cpu_name, cpu_props, model_info); } -- 2.7.4
On Thu, Sep 19, 2019 at 16:24:53 -0400, Collin Walling wrote:
With refactoring most of the expansion function, let's take care of some additional cleanups.
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_monitor_json.c | 37 ++++++++++++++----------------------- 1 file changed, 14 insertions(+), 23 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
When expanding a CPU model via query-cpu-model-expansion, any features that were a part of the original model are discarded. For exmaple, when expanding modelA with features f1, f2, a full expansion may reveal feature f3, but the expanded model will not include f1 or f2. Let's pass a virCPUDefPtr to the expansion function in preparation for taking features into consideration. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_capabilities.c | 9 +++++++-- src/qemu/qemu_monitor.c | 7 +++---- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 8 ++++---- src/qemu/qemu_monitor_json.h | 2 +- tests/cputest.c | 7 ++++++- 6 files changed, 22 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 9b19930..bc0ac3d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2497,6 +2497,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, qemuMonitorCPUModelInfoPtr nonMigratable = NULL; virHashTablePtr hash = NULL; const char *model; + virCPUDefPtr cpu; qemuMonitorCPUModelExpansionType type; virDomainVirtType virtType; int ret = -1; @@ -2512,6 +2513,9 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, model = "host"; } + if (VIR_ALLOC(cpu) < 0 || VIR_STRDUP(cpu->model, model) < 0) + goto cleanup; + /* Some x86_64 features defined in cpu_map.xml use spelling which differ * from the one preferred by QEMU. Static expansion would give us only the * preferred spelling. With new QEMU we always use the QEMU's canonical @@ -2525,12 +2529,12 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, else type = QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC; - if (qemuMonitorGetCPUModelExpansion(mon, type, model, true, &modelInfo) < 0) + if (qemuMonitorGetCPUModelExpansion(mon, type, cpu, true, &modelInfo) < 0) goto cleanup; /* Try to check migratability of each feature. */ if (modelInfo && - qemuMonitorGetCPUModelExpansion(mon, type, model, false, + qemuMonitorGetCPUModelExpansion(mon, type, cpu, false, &nonMigratable) < 0) goto cleanup; @@ -2574,6 +2578,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, virHashFree(hash); qemuMonitorCPUModelInfoFree(nonMigratable); qemuMonitorCPUModelInfoFree(modelInfo); + virCPUDefFree(cpu); return ret; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index aa230b3..7b454c2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3716,16 +3716,15 @@ qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr cpu) int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, + virCPUDefPtr cpu, bool migratable, qemuMonitorCPUModelInfoPtr *model_info) { - VIR_DEBUG("type=%d model_name=%s migratable=%d", - type, model_name, migratable); + VIR_DEBUG("type=%d cpu=%p migratable=%d", type, cpu, migratable); QEMU_CHECK_MONITOR(mon); - return qemuMonitorJSONGetCPUModelExpansion(mon, type, model_name, + return qemuMonitorJSONGetCPUModelExpansion(mon, type, cpu, migratable, model_info); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 70000a1..672b4f9 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1157,7 +1157,7 @@ typedef enum { int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, + virCPUDefPtr cpu, bool migratable, qemuMonitorCPUModelInfoPtr *model_info); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3282593..3c6c330 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5782,7 +5782,7 @@ qemuMonitorJSONParseCPUModelProperty(const char *key, static virJSONValuePtr -qemuMonitorJSONMakeCPUModel(const char *model_name, +qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu, bool migratable) { virJSONValuePtr model = NULL; @@ -5791,7 +5791,7 @@ qemuMonitorJSONMakeCPUModel(const char *model_name, if (!(model = virJSONValueNewObject())) goto error; - if (virJSONValueObjectAppendString(model, "name", model_name) < 0) + if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0) goto error; if (!migratable) { @@ -5873,7 +5873,7 @@ qemuMonitorJSONParseCPUModel(const char *cpu_name, int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, + virCPUDefPtr cpu, bool migratable, qemuMonitorCPUModelInfoPtr *model_info) { @@ -5888,7 +5888,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, *model_info = NULL; - if (!(model = qemuMonitorJSONMakeCPUModel(model_name, migratable))) + if (!(model = qemuMonitorJSONMakeCPUModel(cpu, migratable))) return -1; retry: diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 61e64e8..bdccd36 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -390,7 +390,7 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon, int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, - const char *model_name, + virCPUDefPtr cpu, bool migratable, qemuMonitorCPUModelInfoPtr *model_info) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5); diff --git a/tests/cputest.c b/tests/cputest.c index 7037bcc..3c17ed4 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -481,6 +481,7 @@ cpuTestMakeQEMUCaps(const struct data *data) virQEMUCapsPtr qemuCaps = NULL; qemuMonitorTestPtr testMon = NULL; qemuMonitorCPUModelInfoPtr model = NULL; + virCPUDefPtr cpu = NULL; char *json = NULL; if (virAsprintf(&json, "%s/cputestdata/%s-cpuid-%s.json", @@ -490,9 +491,12 @@ cpuTestMakeQEMUCaps(const struct data *data) if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true))) goto error; + if (VIR_ALLOC(cpu) < 0 || VIR_STRDUP(cpu->model, "host") < 0) + goto cleanup; + if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon), QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC, - "host", true, &model) < 0) + cpu, true, &model) < 0) goto error; if (!(qemuCaps = virQEMUCapsNew())) @@ -515,6 +519,7 @@ cpuTestMakeQEMUCaps(const struct data *data) cleanup: qemuMonitorCPUModelInfoFree(model); qemuMonitorTestFree(testMon); + virCPUDefFree(cpu); VIR_FREE(json); return qemuCaps; -- 2.7.4
On Thu, Sep 19, 2019 at 16:24:54 -0400, Collin Walling wrote:
When expanding a CPU model via query-cpu-model-expansion, any features that were a part of the original model are discarded. For exmaple, when expanding modelA with features f1, f2, a full expansion may reveal feature f3, but the expanded model will not include f1 or f2.
Let's pass a virCPUDefPtr to the expansion function in preparation for taking features into consideration.
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_capabilities.c | 9 +++++++-- src/qemu/qemu_monitor.c | 7 +++---- src/qemu/qemu_monitor.h | 2 +- src/qemu/qemu_monitor_json.c | 8 ++++---- src/qemu/qemu_monitor_json.h | 2 +- tests/cputest.c | 7 ++++++- 6 files changed, 22 insertions(+), 13 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
query-cpu-model-baseline/comparison will accept a list of features as part of the command. Since CPUs may be defined with CPU feature policies, let's parse it to the appropriate boolean that the QMP command expects. A feature that is set to required, force, or if it is a hypervisor CPU feature (-1), then set the property value to true. Otherwise (optional, disabled) set the value to false. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_monitor_json.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 3c6c330..77113c0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5787,6 +5787,7 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu, { virJSONValuePtr model = NULL; virJSONValuePtr props = NULL; + size_t i; if (!(model = virJSONValueNewObject())) goto error; @@ -5794,12 +5795,31 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu, if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0) goto error; - if (!migratable) { - if (!(props = virJSONValueNewObject()) || - virJSONValueObjectAppendBoolean(props, "migratable", false) < 0 || - virJSONValueObjectAppend(model, "props", props) < 0) + if (cpu->nfeatures || !migratable) { + if (!(props = virJSONValueNewObject())) + goto error; + + for (i = 0; i < cpu->nfeatures; i++) { + char *name = cpu->features[i].name; + bool enabled = false; + + /* policy may be reported as -1 if the CPU def is a host model */ + if (cpu->features[i].policy == VIR_CPU_FEATURE_REQUIRE || + cpu->features[i].policy == VIR_CPU_FEATURE_FORCE || + cpu->features[i].policy == -1) + enabled = true; + + if (virJSONValueObjectAppendBoolean(props, name, enabled) < 0) + goto error; + } + + if (!migratable && + virJSONValueObjectAppendBoolean(props, "migratable", false) < 0) { + goto error; + } + + if (virJSONValueObjectAppend(model, "props", props) < 0) goto error; - props = NULL; } return model; -- 2.7.4
On Thu, Sep 19, 2019 at 16:24:55 -0400, Collin Walling wrote:
query-cpu-model-baseline/comparison will accept a list of features as part of the command. Since CPUs may be defined with CPU feature policies, let's parse it to the appropriate boolean that the QMP command expects.
A feature that is set to required, force, or if it is a hypervisor CPU feature (-1), then set the property value to true. Otherwise (optional, disabled) set the value to false.
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_monitor_json.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Some older s390 CPU models (e.g. z900) will not report props as a response from query-cpu-model-expansion. As such, we should make the props field optional when parsing the return data from the QMP response. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_capabilities.c | 10 ++++++++-- src/qemu/qemu_monitor.c | 4 +++- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 24 ++++++++++++++++-------- src/qemu/qemu_monitor_json.h | 3 ++- tests/cputest.c | 6 +++++- 6 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index bc0ac3d..3020530 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -2500,6 +2500,7 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, virCPUDefPtr cpu; qemuMonitorCPUModelExpansionType type; virDomainVirtType virtType; + bool fail_no_props = true; int ret = -1; if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) @@ -2529,12 +2530,17 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps, else type = QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC; - if (qemuMonitorGetCPUModelExpansion(mon, type, cpu, true, &modelInfo) < 0) + /* Older s390 models do not report a feature set */ + if (ARCH_IS_S390(qemuCaps->arch)) + fail_no_props = false; + + if (qemuMonitorGetCPUModelExpansion(mon, type, cpu, true, fail_no_props, + &modelInfo) < 0) goto cleanup; /* Try to check migratability of each feature. */ if (modelInfo && - qemuMonitorGetCPUModelExpansion(mon, type, cpu, false, + qemuMonitorGetCPUModelExpansion(mon, type, cpu, false, fail_no_props, &nonMigratable) < 0) goto cleanup; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 7b454c2..6bda620 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3718,6 +3718,7 @@ qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, virCPUDefPtr cpu, bool migratable, + bool fail_no_props, qemuMonitorCPUModelInfoPtr *model_info) { VIR_DEBUG("type=%d cpu=%p migratable=%d", type, cpu, migratable); @@ -3725,7 +3726,8 @@ qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, QEMU_CHECK_MONITOR(mon); return qemuMonitorJSONGetCPUModelExpansion(mon, type, cpu, - migratable, model_info); + migratable, fail_no_props, + model_info); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 672b4f9..c9cb3bd 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1159,6 +1159,7 @@ int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, virCPUDefPtr cpu, bool migratable, + bool fail_no_props, qemuMonitorCPUModelInfoPtr *model_info); void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 77113c0..ddcf425 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5833,6 +5833,7 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu, static int qemuMonitorJSONParseCPUModelData(virJSONValuePtr data, + bool fail_no_props, virJSONValuePtr *cpu_model, virJSONValuePtr *cpu_props, const char **cpu_name) @@ -5849,7 +5850,8 @@ qemuMonitorJSONParseCPUModelData(virJSONValuePtr data, return -1; } - if (!(*cpu_props = virJSONValueObjectGetObject(*cpu_model, "props"))) { + if (!(*cpu_props = virJSONValueObjectGetObject(*cpu_model, "props")) && + fail_no_props) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("query-cpu-model-expansion reply data was missing 'props'")); return -1; @@ -5873,13 +5875,17 @@ qemuMonitorJSONParseCPUModel(const char *cpu_name, if (VIR_STRDUP(machine_model->name, cpu_name) < 0) goto cleanup; - if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0) - goto cleanup; + if (cpu_props) { + size_t nprops = virJSONValueObjectKeysNumber(cpu_props); - if (virJSONValueObjectForeachKeyValue(cpu_props, - qemuMonitorJSONParseCPUModelProperty, - machine_model) < 0) - goto cleanup; + if (VIR_ALLOC_N(machine_model->props, nprops) < 0) + goto cleanup; + + if (virJSONValueObjectForeachKeyValue(cpu_props, + qemuMonitorJSONParseCPUModelProperty, + machine_model) < 0) + goto cleanup; + } VIR_STEAL_PTR(*model_info, machine_model); ret = 0; @@ -5895,6 +5901,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, virCPUDefPtr cpu, bool migratable, + bool fail_no_props, qemuMonitorCPUModelInfoPtr *model_info) { VIR_AUTOPTR(virJSONValue) model = NULL; @@ -5945,7 +5952,8 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, data = virJSONValueObjectGetObject(reply, "return"); if (qemuMonitorJSONParseCPUModelData(data, - &cpu_model, &cpu_props, &cpu_name) < 0) + fail_no_props, &cpu_model, &cpu_props, + &cpu_name) < 0) return -1; /* QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL requests "full" expansion diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index bdccd36..682710f 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -392,8 +392,9 @@ int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, virCPUDefPtr cpu, bool migratable, + bool fail_no_props, qemuMonitorCPUModelInfoPtr *model_info) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5); + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(6); int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, char ***commands) diff --git a/tests/cputest.c b/tests/cputest.c index 3c17ed4..e3937c6 100644 --- a/tests/cputest.c +++ b/tests/cputest.c @@ -482,6 +482,7 @@ cpuTestMakeQEMUCaps(const struct data *data) qemuMonitorTestPtr testMon = NULL; qemuMonitorCPUModelInfoPtr model = NULL; virCPUDefPtr cpu = NULL; + bool fail_no_props = true; char *json = NULL; if (virAsprintf(&json, "%s/cputestdata/%s-cpuid-%s.json", @@ -494,9 +495,12 @@ cpuTestMakeQEMUCaps(const struct data *data) if (VIR_ALLOC(cpu) < 0 || VIR_STRDUP(cpu->model, "host") < 0) goto cleanup; + if (ARCH_IS_S390(data->arch)) + fail_no_props = false; + if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon), QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC, - cpu, true, &model) < 0) + cpu, true, fail_no_props, &model) < 0) goto error; if (!(qemuCaps = virQEMUCapsNew())) -- 2.7.4
On Thu, Sep 19, 2019 at 16:24:56 -0400, Collin Walling wrote:
Some older s390 CPU models (e.g. z900) will not report props as a response from query-cpu-model-expansion. As such, we should make the props field optional when parsing the return data from the QMP response.
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_capabilities.c | 10 ++++++++-- src/qemu/qemu_monitor.c | 4 +++- src/qemu/qemu_monitor.h | 1 + src/qemu/qemu_monitor_json.c | 24 ++++++++++++++++-------- src/qemu/qemu_monitor_json.h | 3 ++- tests/cputest.c | 6 +++++- 6 files changed, 35 insertions(+), 13 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Modify the error messages in qemuMonitorJSONParseCPUModelData to print the command name provided to the function. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_monitor_json.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index ddcf425..13ee323 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5833,27 +5833,28 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu, static int qemuMonitorJSONParseCPUModelData(virJSONValuePtr data, + const char *cmd_name, bool fail_no_props, virJSONValuePtr *cpu_model, virJSONValuePtr *cpu_props, const char **cpu_name) { if (!(*cpu_model = virJSONValueObjectGetObject(data, "model"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-cpu-model-expansion reply data was missing 'model'")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s reply data was missing 'model'"), cmd_name); return -1; } if (!(*cpu_name = virJSONValueObjectGetString(*cpu_model, "name"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-cpu-model-expansion reply data was missing 'name'")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s reply data was missing 'name'"), cmd_name); return -1; } if (!(*cpu_props = virJSONValueObjectGetObject(*cpu_model, "props")) && fail_no_props) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-cpu-model-expansion reply data was missing 'props'")); + virReportError(VIR_ERR_INTERNAL_ERROR, + _("%s reply data was missing 'props'"), cmd_name); return -1; } @@ -5951,7 +5952,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, data = virJSONValueObjectGetObject(reply, "return"); - if (qemuMonitorJSONParseCPUModelData(data, + if (qemuMonitorJSONParseCPUModelData(data, "query-cpu-model-expansion", fail_no_props, &cpu_model, &cpu_props, &cpu_name) < 0) return -1; -- 2.7.4
On Thu, Sep 19, 2019 at 16:24:57 -0400, Collin Walling wrote:
Modify the error messages in qemuMonitorJSONParseCPUModelData to print the command name provided to the function.
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_monitor_json.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Interfaces with QEMU to baseline CPU models. The command takes two CPU models, A and B, that are given a model name and an optional list of CPU features. Through the query-cpu-model-baseline command issued via QMP, a result is produced that contains a new baselined CPU model that is guaranteed to run on both A and B. Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> --- src/qemu/qemu_monitor.c | 14 ++++++++++++++ src/qemu/qemu_monitor.h | 5 +++++ src/qemu/qemu_monitor_json.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++++ 4 files changed, 67 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 6bda620..834eaf2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3731,6 +3731,20 @@ qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, } +int +qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, + virCPUDefPtr cpu_a, + virCPUDefPtr cpu_b, + qemuMonitorCPUModelInfoPtr *baseline) +{ + VIR_DEBUG("cpu_a=%p cpu_b=%p", cpu_a, cpu_b); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONGetCPUModelBaseline(mon, cpu_a, cpu_b, baseline); +} + + void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index c9cb3bd..8974ba8 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1164,6 +1164,11 @@ int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon, void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info); +int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, + virCPUDefPtr cpu_a, + virCPUDefPtr cpu_b, + qemuMonitorCPUModelInfoPtr *baseline); + qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 13ee323..679531d 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5974,6 +5974,48 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, } +int +qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, + virCPUDefPtr cpu_a, + virCPUDefPtr cpu_b, + qemuMonitorCPUModelInfoPtr *baseline) +{ + VIR_AUTOPTR(virJSONValue) model_a = NULL; + VIR_AUTOPTR(virJSONValue) model_b = NULL; + VIR_AUTOPTR(virJSONValue) cmd = NULL; + VIR_AUTOPTR(virJSONValue) reply = NULL; + virJSONValuePtr data; + virJSONValuePtr cpu_model; + virJSONValuePtr cpu_props = NULL; + const char *cpu_name = ""; + + if (!(model_a = qemuMonitorJSONMakeCPUModel(cpu_a, true)) || + !(model_b = qemuMonitorJSONMakeCPUModel(cpu_b, true))) + return -1; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-baseline", + "a:modela", &model_a, + "a:modelb", &model_b, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + return -1; + + data = virJSONValueObjectGetObject(reply, "return"); + + if (qemuMonitorJSONParseCPUModelData(data, "query-cpu-model-baseline", + false, &cpu_model, &cpu_props, + &cpu_name) < 0) + return -1; + + return qemuMonitorJSONParseCPUModel(cpu_name, cpu_props, baseline); +} + + int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, char ***commands) { diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 682710f..77ea41b 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -396,6 +396,12 @@ int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelInfoPtr *model_info) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(6); +int qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, + virCPUDefPtr cpu_a, + virCPUDefPtr cpu_b, + qemuMonitorCPUModelInfoPtr *baseline) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, char ***commands) ATTRIBUTE_NONNULL(2); -- 2.7.4
On Thu, Sep 19, 2019 at 16:24:58 -0400, Collin Walling wrote:
Interfaces with QEMU to baseline CPU models. The command takes two CPU models, A and B, that are given a model name and an optional list of CPU features. Through the query-cpu-model-baseline command issued via QMP, a result is produced that contains a new baselined CPU model that is guaranteed to run on both A and B.
Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> --- src/qemu/qemu_monitor.c | 14 ++++++++++++++ src/qemu/qemu_monitor.h | 5 +++++ src/qemu/qemu_monitor_json.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++++ 4 files changed, 67 insertions(+)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This capability enables baselining of CPU models via QMP. Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 + 9 files changed, 10 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 3020530..6fa8354 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -539,6 +539,7 @@ VIR_ENUM_IMPL(virQEMUCaps, "migration-file-drop-cache", "net-socket-dgram", "dbus-vmstate", + "query-cpu-model-baseline", ); @@ -985,6 +986,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "blockdev-del", QEMU_CAPS_BLOCKDEV_DEL }, { "query-current-machine", QEMU_CAPS_QUERY_CURRENT_MACHINE }, { "block-dirty-bitmap-merge", QEMU_CAPS_BITMAP_MERGE }, + { "query-cpu-model-baseline", QEMU_CAPS_QUERY_CPU_MODEL_BASELINE }, }; struct virQEMUCapsStringFlags virQEMUCapsMigration[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 54f9115..2c10e00 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -520,6 +520,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_MIGRATION_FILE_DROP_CACHE, /* migration with disk cache on is safe for type='file' disks */ QEMU_CAPS_NET_SOCKET_DGRAM, /* -net socket,fd= with dgram socket */ QEMU_CAPS_DBUS_VMSTATE, /* -object dbus-vmstate */ + QEMU_CAPS_QUERY_CPU_MODEL_BASELINE, /* qmp query-cpu-model-baseline */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml index 21b918f..e6f7e28 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml @@ -103,6 +103,7 @@ <flag name='egl-headless'/> <flag name='zpci'/> <flag name='iothread.poll-max-ns'/> + <flag name='query-cpu-model-baseline'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100805</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml index 6cb997d..48d7742 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -110,6 +110,7 @@ <flag name='egl-headless'/> <flag name='zpci'/> <flag name='iothread.poll-max-ns'/> + <flag name='query-cpu-model-baseline'/> <version>2011000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100806</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 2930381..381abd5 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -121,6 +121,7 @@ <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> + <flag name='query-cpu-model-baseline'/> <version>2012000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100807</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml index 5a2b740..9516a3b 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml @@ -92,6 +92,7 @@ <flag name='sdl-gl'/> <flag name='vhost-vsock'/> <flag name='zpci'/> + <flag name='query-cpu-model-baseline'/> <version>2007093</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100764</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml index 72ae100..cf8bbb2 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml @@ -97,6 +97,7 @@ <flag name='vhost-vsock'/> <flag name='zpci'/> <flag name='iothread.poll-max-ns'/> + <flag name='query-cpu-model-baseline'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100765</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml index d511377..40b4a2b 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml @@ -123,6 +123,7 @@ <flag name='memory-backend-memfd.hugetlb'/> <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> + <flag name='query-cpu-model-baseline'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100757</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml index 6cad000..4909bcc 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml @@ -133,6 +133,7 @@ <flag name='nbd-bitmap'/> <flag name='migration-file-drop-cache'/> <flag name='net-socket-dgram'/> + <flag name='query-cpu-model-baseline'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100758</microcodeVersion> -- 2.7.4
This command is hooked into the virsh hypervisor-cpu-baseline command. The CPU models provided in the XML sent to the command will be baselined via the query-cpu-model-baseline QMP command. The resulting CPU model will be reported. Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> --- src/qemu/qemu_driver.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1e041a8..2a5a3ca 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13810,6 +13810,83 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, } +static int +qemuConnectStealCPUModelFromInfo(virCPUDefPtr dst, + qemuMonitorCPUModelInfoPtr *src) +{ + qemuMonitorCPUModelInfoPtr info = *src; + size_t i; + int ret = 0; + + virCPUDefFreeModel(dst); + + VIR_STEAL_PTR(dst->model, info->name); + + for (i = 0; i < info->nprops; i++) { + char *name = info->props[i].name; + + if (info->props[i].type == QEMU_MONITOR_CPU_PROPERTY_BOOLEAN && + info->props[i].value.boolean && + virCPUDefAddFeature(dst, name, VIR_CPU_FEATURE_REQUIRE) < 0) { + virCPUDefFree(dst); + ret = -1; + break; + } + } + + qemuMonitorCPUModelInfoFree(info); + *src = NULL; + return ret; +} + + +static virCPUDefPtr +qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, + const char *libDir, + uid_t runUid, + gid_t runGid, + int ncpus, + virCPUDefPtr *cpus) +{ + qemuProcessQMPPtr proc; + virCPUDefPtr baseline = NULL; + qemuMonitorCPUModelInfoPtr result = NULL; + size_t i; + + if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps), + libDir, runUid, runGid, false))) + goto cleanup; + + if (qemuProcessQMPStart(proc) < 0) + goto cleanup; + + if (VIR_ALLOC(baseline) < 0) + goto error; + + if (virCPUDefCopyModel(baseline, cpus[0], false)) + goto error; + + for (i = 1; i < ncpus; i++) { + + if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline, + cpus[i], &result) < 0) + goto error; + + /* result is freed regardless of this function's success */ + if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) + goto error; + } + + cleanup: + qemuProcessQMPFree(proc); + return baseline; + + error: + virCPUDefFree(baseline); + goto cleanup; +} + + static char * qemuConnectBaselineHypervisorCPU(virConnectPtr conn, const char *emulator, @@ -13821,6 +13898,7 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); virCPUDefPtr *cpus = NULL; virQEMUCapsPtr qemuCaps = NULL; virArch arch; @@ -13875,6 +13953,16 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels, (const char **)features, migratable))) goto cleanup; + + } else if (ARCH_IS_S390(arch) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_BASELINE)) { + + if (!(cpu = qemuConnectCPUModelBaseline(qemuCaps, cfg->libDir, + cfg->user, cfg->group, + ncpus, + cpus))) + goto cleanup; + } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("computing baseline hypervisor CPU is not supported " -- 2.7.4
On Thu, Sep 19, 2019 at 16:25:00 -0400, Collin Walling wrote:
This command is hooked into the virsh hypervisor-cpu-baseline command. The CPU models provided in the XML sent to the command will be baselined via the query-cpu-model-baseline QMP command. The resulting CPU model will be reported.
Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> --- src/qemu/qemu_driver.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1e041a8..2a5a3ca 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13810,6 +13810,83 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, }
+static int +qemuConnectStealCPUModelFromInfo(virCPUDefPtr dst, + qemuMonitorCPUModelInfoPtr *src) +{ + qemuMonitorCPUModelInfoPtr info = *src; + size_t i; + int ret = 0;
We usually initialize ret to -1 and set it to zero at the very end when everything is done rather than changing it to -1 on each error path.
+ + virCPUDefFreeModel(dst); + + VIR_STEAL_PTR(dst->model, info->name); + + for (i = 0; i < info->nprops; i++) { + char *name = info->props[i].name; + + if (info->props[i].type == QEMU_MONITOR_CPU_PROPERTY_BOOLEAN && + info->props[i].value.boolean && + virCPUDefAddFeature(dst, name, VIR_CPU_FEATURE_REQUIRE) < 0) { + virCPUDefFree(dst);
This would cause double free in the caller.
+ ret = -1; + break; + } + } +
Adding cleanup label here would be better.
+ qemuMonitorCPUModelInfoFree(info); + *src = NULL;
You can avoid this by using VIR_STEAL_PTR(info, *src) at the beginning.
+ return ret; +} + + +static virCPUDefPtr +qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, + const char *libDir, + uid_t runUid, + gid_t runGid, + int ncpus, + virCPUDefPtr *cpus)
I think I mentioned in my previous review (probably not in this exact context, though) we usually pass an array followed by the number of elements.
+{ + qemuProcessQMPPtr proc; + virCPUDefPtr baseline = NULL; + qemuMonitorCPUModelInfoPtr result = NULL; + size_t i; + + if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps), + libDir, runUid, runGid, false))) + goto cleanup; + + if (qemuProcessQMPStart(proc) < 0) + goto cleanup; + + if (VIR_ALLOC(baseline) < 0) + goto error; + + if (virCPUDefCopyModel(baseline, cpus[0], false)) + goto error; + + for (i = 1; i < ncpus; i++) { +
Extra empty line.
+ if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline, + cpus[i], &result) < 0) + goto error; + + /* result is freed regardless of this function's success */
I think this comment would be better placed as a documentation of the new function.
+ if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) + goto error; + } +
You could steal from baseline to ret, free baseline unconditionally and return ret to get rid of the error label.
+ cleanup: + qemuProcessQMPFree(proc); + return baseline; + + error: + virCPUDefFree(baseline); + goto cleanup; +} + + static char * qemuConnectBaselineHypervisorCPU(virConnectPtr conn, const char *emulator, @@ -13821,6 +13898,7 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); virCPUDefPtr *cpus = NULL; virQEMUCapsPtr qemuCaps = NULL; virArch arch; @@ -13875,6 +13953,16 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels, (const char **)features, migratable))) goto cleanup; + + } else if (ARCH_IS_S390(arch) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_BASELINE)) { + + if (!(cpu = qemuConnectCPUModelBaseline(qemuCaps, cfg->libDir, + cfg->user, cfg->group, + ncpus, + cpus))) + goto cleanup; +
Three extra empty lines in this hunk.
} else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("computing baseline hypervisor CPU is not supported "
With the following patch squashed in Reviewed-by: Jiri Denemark <jdenemar@redhat.com> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b25e554358..b772a920e3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13739,32 +13739,41 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, } +/** + * qemuConnectStealCPUModelFromInfo: + * + * Consumes @src and replaces the content of @dst with CPU model name and + * features from @src. When this function returns (both with success or + * failure), @src is freed. + */ static int qemuConnectStealCPUModelFromInfo(virCPUDefPtr dst, qemuMonitorCPUModelInfoPtr *src) { - qemuMonitorCPUModelInfoPtr info = *src; + qemuMonitorCPUModelInfoPtr info; size_t i; - int ret = 0; + int ret = -1; virCPUDefFreeModel(dst); + VIR_STEAL_PTR(info, *src); VIR_STEAL_PTR(dst->model, info->name); for (i = 0; i < info->nprops; i++) { char *name = info->props[i].name; - if (info->props[i].type == QEMU_MONITOR_CPU_PROPERTY_BOOLEAN && - info->props[i].value.boolean && - virCPUDefAddFeature(dst, name, VIR_CPU_FEATURE_REQUIRE) < 0) { - virCPUDefFree(dst); - ret = -1; - break; - } + if (info->props[i].type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN || + !info->props[i].value.boolean) + continue; + + if (virCPUDefAddFeature(dst, name, VIR_CPU_FEATURE_REQUIRE) < 0) + goto cleanup; } + ret = 0; + + cleanup: qemuMonitorCPUModelInfoFree(info); - *src = NULL; return ret; } @@ -13774,10 +13783,11 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, const char *libDir, uid_t runUid, gid_t runGid, - int ncpus, - virCPUDefPtr *cpus) + virCPUDefPtr *cpus, + int ncpus) { qemuProcessQMPPtr proc; + virCPUDefPtr ret = NULL; virCPUDefPtr baseline = NULL; qemuMonitorCPUModelInfoPtr result = NULL; size_t i; @@ -13790,29 +13800,26 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, goto cleanup; if (VIR_ALLOC(baseline) < 0) - goto error; + goto cleanup; if (virCPUDefCopyModel(baseline, cpus[0], false)) - goto error; + goto cleanup; for (i = 1; i < ncpus; i++) { - if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline, cpus[i], &result) < 0) - goto error; + goto cleanup; - /* result is freed regardless of this function's success */ if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) - goto error; + goto cleanup; } + VIR_STEAL_PTR(ret, baseline); + cleanup: qemuProcessQMPFree(proc); - return baseline; - - error: virCPUDefFree(baseline); - goto cleanup; + return ret; } @@ -13882,16 +13889,12 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels, (const char **)features, migratable))) goto cleanup; - } else if (ARCH_IS_S390(arch) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_BASELINE)) { - if (!(cpu = qemuConnectCPUModelBaseline(qemuCaps, cfg->libDir, cfg->user, cfg->group, - ncpus, - cpus))) + cpus, ncpus))) goto cleanup; - } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("computing baseline hypervisor CPU is not supported "
On 10/2/19 9:52 AM, Jiri Denemark wrote:
On Thu, Sep 19, 2019 at 16:25:00 -0400, Collin Walling wrote:
This command is hooked into the virsh hypervisor-cpu-baseline command. The CPU models provided in the XML sent to the command will be baselined via the query-cpu-model-baseline QMP command. The resulting CPU model will be reported.
Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> --- src/qemu/qemu_driver.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)
Thank you for the reviews and for providing a cleanup patch for this one. I will tend to these ASAP. -- Respectfully, - Collin Walling
Perform a full CPU model expansion on the result of the baselined model name when the features flag is present. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2a5a3ca..93f1767 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13845,6 +13845,7 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, const char *libDir, uid_t runUid, gid_t runGid, + bool expand_features, int ncpus, virCPUDefPtr *cpus) { @@ -13877,6 +13878,16 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, goto error; } + if (expand_features) { + if (qemuMonitorGetCPUModelExpansion(proc->mon, + QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL, + baseline, true, false, &result) < 0) + goto error; + + if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) + goto error; + } + cleanup: qemuProcessQMPFree(proc); return baseline; @@ -13957,9 +13968,11 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, } else if (ARCH_IS_S390(arch) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_BASELINE)) { + bool expand_features = (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES); + if (!(cpu = qemuConnectCPUModelBaseline(qemuCaps, cfg->libDir, cfg->user, cfg->group, - ncpus, + expand_features, ncpus, cpus))) goto cleanup; -- 2.7.4
On Thu, Sep 19, 2019 at 16:25:01 -0400, Collin Walling wrote:
Perform a full CPU model expansion on the result of the baselined model name when the features flag is present.
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Interfaces with QEMU to compare CPU models. The command takes two CPU models, A and B, that are given a model name and an optional list of CPU features. Through the query-cpu-model-comparison command issued via QMP, a result is produced that contains the comparison evaluation string (identical, superset, subset, incompatible). The list of properties (aka CPU features) that is returned from the QMP response is ignored. Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> --- src/qemu/qemu_monitor.c | 14 ++++++++++++++ src/qemu/qemu_monitor.h | 5 +++++ src/qemu/qemu_monitor_json.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++++ 4 files changed, 67 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 834eaf2..76b5547 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3745,6 +3745,20 @@ qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, } +int +qemuMonitorGetCPUModelComparison(qemuMonitorPtr mon, + virCPUDefPtr cpu_a, + virCPUDefPtr cpu_b, + char **result) +{ + VIR_DEBUG("cpu_a=%p cpu_b=%p", cpu_a, cpu_b); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONGetCPUModelComparison(mon, cpu_a, cpu_b, result); +} + + void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8974ba8..6f46931 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1169,6 +1169,11 @@ int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, virCPUDefPtr cpu_b, qemuMonitorCPUModelInfoPtr *baseline); +int qemuMonitorGetCPUModelComparison(qemuMonitorPtr mon, + virCPUDefPtr cpu_a, + virCPUDefPtr cpu_b, + char **result); + qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 679531d..d8f7668 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -6016,6 +6016,48 @@ qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, } +int +qemuMonitorJSONGetCPUModelComparison(qemuMonitorPtr mon, + virCPUDefPtr cpu_a, + virCPUDefPtr cpu_b, + char **result) +{ + VIR_AUTOPTR(virJSONValue) model_a = NULL; + VIR_AUTOPTR(virJSONValue) model_b = NULL; + VIR_AUTOPTR(virJSONValue) cmd = NULL; + VIR_AUTOPTR(virJSONValue) reply = NULL; + const char *data_result; + virJSONValuePtr data; + + if (!(model_a = qemuMonitorJSONMakeCPUModel(cpu_a, true)) || + !(model_b = qemuMonitorJSONMakeCPUModel(cpu_b, true))) + return -1; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-comparison", + "a:modela", &model_a, + "a:modelb", &model_b, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, reply) < 0) + return -1; + + data = virJSONValueObjectGetObject(reply, "return"); + + if (!(data_result = virJSONValueObjectGetString(data, "result"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-cpu-model-comparison reply data was missing " + "'result'")); + return -1; + } + + return VIR_STRDUP(*result, data_result); +} + + int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, char ***commands) { diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 77ea41b..52b24f0 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -402,6 +402,12 @@ int qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, qemuMonitorCPUModelInfoPtr *baseline) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); +int qemuMonitorJSONGetCPUModelComparison(qemuMonitorPtr mon, + virCPUDefPtr cpu_a, + virCPUDefPtr cpu_b, + char **result) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, char ***commands) ATTRIBUTE_NONNULL(2); -- 2.7.4
On Thu, Sep 19, 2019 at 16:25:02 -0400, Collin Walling wrote:
Interfaces with QEMU to compare CPU models. The command takes two CPU models, A and B, that are given a model name and an optional list of CPU features. Through the query-cpu-model-comparison command issued via QMP, a result is produced that contains the comparison evaluation string (identical, superset, subset, incompatible).
The list of properties (aka CPU features) that is returned from the QMP response is ignored.
Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> --- src/qemu/qemu_monitor.c | 14 ++++++++++++++ src/qemu/qemu_monitor.h | 5 +++++ src/qemu/qemu_monitor_json.c | 42 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 6 ++++++ 4 files changed, 67 insertions(+)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This capability enables comparison of CPU models via QMP. Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 3 +++ tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml | 1 + 9 files changed, 14 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 6fa8354..b89bcea 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -540,6 +540,9 @@ VIR_ENUM_IMPL(virQEMUCaps, "net-socket-dgram", "dbus-vmstate", "query-cpu-model-baseline", + + /* 340 */ + "query-cpu-model-comparison", ); @@ -987,6 +990,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "query-current-machine", QEMU_CAPS_QUERY_CURRENT_MACHINE }, { "block-dirty-bitmap-merge", QEMU_CAPS_BITMAP_MERGE }, { "query-cpu-model-baseline", QEMU_CAPS_QUERY_CPU_MODEL_BASELINE }, + { "query-cpu-model-comparison", QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON }, }; struct virQEMUCapsStringFlags virQEMUCapsMigration[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 2c10e00..2a274f9 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -522,6 +522,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */ QEMU_CAPS_DBUS_VMSTATE, /* -object dbus-vmstate */ QEMU_CAPS_QUERY_CPU_MODEL_BASELINE, /* qmp query-cpu-model-baseline */ + /* 340 */ + QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON, /* qmp query-cpu-model-comparison */ + QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml index e6f7e28..058eb32 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml @@ -104,6 +104,7 @@ <flag name='zpci'/> <flag name='iothread.poll-max-ns'/> <flag name='query-cpu-model-baseline'/> + <flag name='query-cpu-model-comparison'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100805</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml index 48d7742..4256131 100644 --- a/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.11.0.s390x.xml @@ -111,6 +111,7 @@ <flag name='zpci'/> <flag name='iothread.poll-max-ns'/> <flag name='query-cpu-model-baseline'/> + <flag name='query-cpu-model-comparison'/> <version>2011000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100806</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml index 381abd5..48be96d 100644 --- a/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.12.0.s390x.xml @@ -122,6 +122,7 @@ <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> <flag name='query-cpu-model-baseline'/> + <flag name='query-cpu-model-comparison'/> <version>2012000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100807</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml index 9516a3b..5b04416 100644 --- a/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml @@ -93,6 +93,7 @@ <flag name='vhost-vsock'/> <flag name='zpci'/> <flag name='query-cpu-model-baseline'/> + <flag name='query-cpu-model-comparison'/> <version>2007093</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100764</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml index cf8bbb2..d5feb7b 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml @@ -98,6 +98,7 @@ <flag name='zpci'/> <flag name='iothread.poll-max-ns'/> <flag name='query-cpu-model-baseline'/> + <flag name='query-cpu-model-comparison'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100765</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml index 40b4a2b..27befc4 100644 --- a/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_3.0.0.s390x.xml @@ -124,6 +124,7 @@ <flag name='iothread.poll-max-ns'/> <flag name='memory-backend-file.align'/> <flag name='query-cpu-model-baseline'/> + <flag name='query-cpu-model-comparison'/> <version>3000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100757</microcodeVersion> diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml index 4909bcc..dc98baf 100644 --- a/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_4.0.0.s390x.xml @@ -134,6 +134,7 @@ <flag name='migration-file-drop-cache'/> <flag name='net-socket-dgram'/> <flag name='query-cpu-model-baseline'/> + <flag name='query-cpu-model-comparison'/> <version>4000000</version> <kvmVersion>0</kvmVersion> <microcodeVersion>39100758</microcodeVersion> -- 2.7.4
Implement an XML to virCPUDefPtr helper that handles the ctxt prerequisite for virCPUDefParseXML. This does not alter any functionality. Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> --- src/conf/cpu_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/cpu_conf.h | 5 +++++ src/cpu/cpu.c | 14 +------------- src/libvirt_private.syms | 1 + 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/conf/cpu_conf.c b/src/conf/cpu_conf.c index 7d16a05..a6bb9ea 100644 --- a/src/conf/cpu_conf.c +++ b/src/conf/cpu_conf.c @@ -268,6 +268,35 @@ virCPUDefCopy(const virCPUDef *cpu) } +int +virCPUDefParseXMLString(const char *xml, + virCPUType type, + virCPUDefPtr *cpu) +{ + xmlDocPtr doc = NULL; + xmlXPathContextPtr ctxt = NULL; + int ret = -1; + + if (!xml) { + virReportError(VIR_ERR_INVALID_ARG, "%s", _("missing CPU definition")); + goto cleanup; + } + + if (!(doc = virXMLParseStringCtxt(xml, _("(CPU_definition)"), &ctxt))) + goto cleanup; + + if (virCPUDefParseXML(ctxt, NULL, type, cpu) < 0) + goto cleanup; + + ret = 0; + + cleanup: + xmlFreeDoc(doc); + xmlXPathFreeContext(ctxt); + return ret; +} + + /* * Parses CPU definition XML from a node pointed to by @xpath. If @xpath is * NULL, the current node of @ctxt is used (i.e., it is a shortcut to "."). diff --git a/src/conf/cpu_conf.h b/src/conf/cpu_conf.h index 19ce816..30904fa 100644 --- a/src/conf/cpu_conf.h +++ b/src/conf/cpu_conf.h @@ -183,6 +183,11 @@ virCPUDefPtr virCPUDefCopyWithoutModel(const virCPUDef *cpu); int +virCPUDefParseXMLString(const char *xml, + virCPUType type, + virCPUDefPtr *cpu); + +int virCPUDefParseXML(xmlXPathContextPtr ctxt, const char *xpath, virCPUType mode, diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index b89462c..2278d79 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -111,31 +111,19 @@ virCPUCompareXML(virArch arch, const char *xml, bool failIncompatible) { - xmlDocPtr doc = NULL; - xmlXPathContextPtr ctxt = NULL; virCPUDefPtr cpu = NULL; virCPUCompareResult ret = VIR_CPU_COMPARE_ERROR; VIR_DEBUG("arch=%s, host=%p, xml=%s", virArchToString(arch), host, NULLSTR(xml)); - if (!xml) { - virReportError(VIR_ERR_INVALID_ARG, "%s", _("missing CPU definition")); - goto cleanup; - } - - if (!(doc = virXMLParseStringCtxt(xml, _("(CPU_definition)"), &ctxt))) - goto cleanup; - - if (virCPUDefParseXML(ctxt, NULL, VIR_CPU_TYPE_AUTO, &cpu) < 0) + if (virCPUDefParseXMLString(xml, VIR_CPU_TYPE_AUTO, &cpu) < 0) goto cleanup; ret = virCPUCompare(arch, host, cpu, failIncompatible); cleanup: virCPUDefFree(cpu); - xmlXPathFreeContext(ctxt); - xmlFreeDoc(doc); return ret; } diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4865eda..9850664 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -101,6 +101,7 @@ virCPUDefIsEqual; virCPUDefListFree; virCPUDefListParse; virCPUDefParseXML; +virCPUDefParseXMLString; virCPUDefStealModel; virCPUDefUpdateFeature; virCPUModeTypeToString; -- 2.7.4
On Thu, Sep 19, 2019 at 16:25:04 -0400, Collin Walling wrote:
Implement an XML to virCPUDefPtr helper that handles the ctxt prerequisite for virCPUDefParseXML.
This does not alter any functionality.
Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> --- src/conf/cpu_conf.c | 29 +++++++++++++++++++++++++++++ src/conf/cpu_conf.h | 5 +++++ src/cpu/cpu.c | 14 +------------- src/libvirt_private.syms | 1 + 4 files changed, 36 insertions(+), 13 deletions(-)
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
This command is hooked into the virsh hypervisor-cpu-compare command. As such, the CPU model XML provided to the command will be compared to the hypervisor CPU contained in the QEMU capabilities file for the appropriate QEMU binary (for s390x, this CPU definition can be observed via virsh domcapabilities). QMP will report that the XML CPU is either identical to, a subset of, or incompatible with the hypervisor CPU. s390 can also report that the XML CPU is a "superset" of the hypervisor CPU. This response is presented as incompatible, as this CPU model would not be able to run on the hypervisor. Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 93f1767..153b2f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13703,6 +13703,45 @@ qemuConnectCompareCPU(virConnectPtr conn, } +static virCPUCompareResult +qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps, + const char *libDir, + uid_t runUid, + gid_t runGid, + virCPUDefPtr cpu_a, + virCPUDefPtr cpu_b, + bool failIncompatible) +{ + qemuProcessQMPPtr proc = NULL; + char *result = NULL; + int ret = VIR_CPU_COMPARE_ERROR; + + if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps), + libDir, runUid, runGid, false))) + goto cleanup; + + if (qemuProcessQMPStart(proc) < 0) + goto cleanup; + + if (qemuMonitorGetCPUModelComparison(proc->mon, cpu_a, cpu_b, &result) < 0) + goto cleanup; + + if (STREQ(result, "identical")) + ret = VIR_CPU_COMPARE_IDENTICAL; + else if (STREQ(result, "superset")) + ret = VIR_CPU_COMPARE_SUPERSET; + else if (failIncompatible) + virReportError(VIR_ERR_CPU_INCOMPATIBLE, NULL); + else + ret = VIR_CPU_COMPARE_INCOMPATIBLE; + + cleanup: + VIR_FREE(result); + qemuProcessQMPFree(proc); + return ret; +} + + static int qemuConnectCompareHypervisorCPU(virConnectPtr conn, const char *emulator, @@ -13714,9 +13753,11 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn, { int ret = VIR_CPU_COMPARE_ERROR; virQEMUDriverPtr driver = conn->privateData; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); virQEMUCapsPtr qemuCaps = NULL; bool failIncompatible; virCPUDefPtr hvCPU; + virCPUDefPtr cpu; virArch arch; virDomainVirtType virttype; @@ -13751,6 +13792,16 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn, if (ARCH_IS_X86(arch)) { ret = virCPUCompareXML(arch, hvCPU, xmlCPU, failIncompatible); + + } else if (ARCH_IS_S390(arch) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON)) { + + if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, &cpu) < 0) + goto cleanup; + + ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir, + cfg->user, cfg->group, + hvCPU, cpu, failIncompatible); } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("comparing with the hypervisor CPU is not supported " @@ -13758,6 +13809,7 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn, } cleanup: + virCPUDefFree(cpu); virObjectUnref(qemuCaps); return ret; } -- 2.7.4
On Thu, Sep 19, 2019 at 16:25:05 -0400, Collin Walling wrote:
This command is hooked into the virsh hypervisor-cpu-compare command. As such, the CPU model XML provided to the command will be compared to the hypervisor CPU contained in the QEMU capabilities file for the appropriate QEMU binary (for s390x, this CPU definition can be observed via virsh domcapabilities).
QMP will report that the XML CPU is either identical to, a subset of, or incompatible with the hypervisor CPU. s390 can also report that the XML CPU is a "superset" of the hypervisor CPU. This response is presented as incompatible, as this CPU model would not be able to run on the hypervisor.
Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> --- src/qemu/qemu_driver.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 93f1767..153b2f2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c ... @@ -13714,9 +13753,11 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn, { int ret = VIR_CPU_COMPARE_ERROR; virQEMUDriverPtr driver = conn->privateData; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); virQEMUCapsPtr qemuCaps = NULL; bool failIncompatible; virCPUDefPtr hvCPU; + virCPUDefPtr cpu;
This needs to be initialized to NULL.
virArch arch; virDomainVirtType virttype;
@@ -13751,6 +13792,16 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn,
if (ARCH_IS_X86(arch)) { ret = virCPUCompareXML(arch, hvCPU, xmlCPU, failIncompatible); + + } else if (ARCH_IS_S390(arch) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON)) { +
Some extra empty lines.
+ if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, &cpu) < 0) + goto cleanup; + + ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir, + cfg->user, cfg->group, + hvCPU, cpu, failIncompatible); } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("comparing with the hypervisor CPU is not supported " @@ -13758,6 +13809,7 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn, }
cleanup: + virCPUDefFree(cpu); virObjectUnref(qemuCaps); return ret; }
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
Providing an erroneous CPU definition in the XML file provided to the hypervisor-cpu-compare/baseline command will result in a verbose internal error. Let's add some sanity checking before executing the QMP commands to provide a cleaner message if something is wrong with the CPU model or features. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 153b2f2..6298c48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13703,6 +13703,55 @@ qemuConnectCompareCPU(virConnectPtr conn, } +static int +qemuConnectCheckCPUModel(qemuMonitorPtr mon, + virCPUDefPtr cpu, + bool reportError) +{ + qemuMonitorCPUModelInfoPtr modelInfo = NULL; + qemuMonitorCPUModelExpansionType type; + size_t i, j; + int ret = -1; + + /* Collect CPU model name and features known by QEMU */ + type = QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL; + if (qemuMonitorGetCPUModelExpansion(mon, type, cpu, + true, false, &modelInfo) < 0) + goto cleanup; + + /* Sanity check CPU model */ + if (!modelInfo) { + if (reportError) + virReportError(VIR_ERR_CPU_INCOMPATIBLE, + _("Unknown CPU model: %s"), cpu->model); + goto cleanup; + } + + /* Sanity check CPU features */ + for (i = 0; i < cpu->nfeatures; i++) { + const char *feat = cpu->features[i].name; + + for (j = 0; j < modelInfo->nprops; j++) { + const char *prop = modelInfo->props[j].name; + if (STREQ(feat, prop)) + break; + } + + if (j == modelInfo->nprops) { + if (reportError) + virReportError(VIR_ERR_CPU_INCOMPATIBLE, + _("Unknown CPU feature: %s"), feat); + goto cleanup; + } + } + ret = 0; + + cleanup: + qemuMonitorCPUModelInfoFree(modelInfo); + return ret; +} + + static virCPUCompareResult qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps, const char *libDir, @@ -13723,6 +13772,13 @@ qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps, if (qemuProcessQMPStart(proc) < 0) goto cleanup; + if (qemuConnectCheckCPUModel(proc->mon, cpu_a, failIncompatible) || + qemuConnectCheckCPUModel(proc->mon, cpu_b, failIncompatible)) { + if (!failIncompatible) + ret = VIR_CPU_COMPARE_INCOMPATIBLE; + goto cleanup; + } + if (qemuMonitorGetCPUModelComparison(proc->mon, cpu_a, cpu_b, &result) < 0) goto cleanup; @@ -13913,6 +13969,9 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, if (qemuProcessQMPStart(proc) < 0) goto cleanup; + if (qemuConnectCheckCPUModel(proc->mon, cpus[0], true)) + goto cleanup; + if (VIR_ALLOC(baseline) < 0) goto error; @@ -13921,6 +13980,9 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, for (i = 1; i < ncpus; i++) { + if (qemuConnectCheckCPUModel(proc->mon, cpus[i], true)) + goto error; + if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline, cpus[i], &result) < 0) goto error; -- 2.7.4
On Thu, Sep 19, 2019 at 16:25:06 -0400, Collin Walling wrote:
Providing an erroneous CPU definition in the XML file provided to the hypervisor-cpu-compare/baseline command will result in a verbose internal error. Let's add some sanity checking before executing the QMP commands to provide a cleaner message if something is wrong with the CPU model or features.
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 153b2f2..6298c48 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13703,6 +13703,55 @@ qemuConnectCompareCPU(virConnectPtr conn, }
+static int +qemuConnectCheckCPUModel(qemuMonitorPtr mon, + virCPUDefPtr cpu, + bool reportError) +{ + qemuMonitorCPUModelInfoPtr modelInfo = NULL; + qemuMonitorCPUModelExpansionType type; + size_t i, j; + int ret = -1; + + /* Collect CPU model name and features known by QEMU */ + type = QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL; + if (qemuMonitorGetCPUModelExpansion(mon, type, cpu, + true, false, &modelInfo) < 0) + goto cleanup; + + /* Sanity check CPU model */ + if (!modelInfo) { + if (reportError) + virReportError(VIR_ERR_CPU_INCOMPATIBLE, + _("Unknown CPU model: %s"), cpu->model);
This is not really related to CPU compatibility. Except for taking a CPU model (or feature below) from a newer QEMU and comparing it on an old one. But this is indistinguishable from an incorrect input. For this reason and for consistency among all architectures we should just report this as a normal error (e.g., VIR_ERR_CONFIG_UNSUPPORTED).
+ goto cleanup; + } + + /* Sanity check CPU features */ + for (i = 0; i < cpu->nfeatures; i++) { + const char *feat = cpu->features[i].name; + + for (j = 0; j < modelInfo->nprops; j++) { + const char *prop = modelInfo->props[j].name; + if (STREQ(feat, prop)) + break; + } + + if (j == modelInfo->nprops) { + if (reportError) + virReportError(VIR_ERR_CPU_INCOMPATIBLE, + _("Unknown CPU feature: %s"), feat);
Ditto.
+ goto cleanup; + } + } + ret = 0; + + cleanup: + qemuMonitorCPUModelInfoFree(modelInfo); + return ret; +} + + static virCPUCompareResult qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps, const char *libDir, @@ -13723,6 +13772,13 @@ qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps, if (qemuProcessQMPStart(proc) < 0) goto cleanup;
+ if (qemuConnectCheckCPUModel(proc->mon, cpu_a, failIncompatible) || + qemuConnectCheckCPUModel(proc->mon, cpu_b, failIncompatible)) {
That said, you don't need to pass failIncompatible to qemuConnectCheckCPUModel...
+ if (!failIncompatible) + ret = VIR_CPU_COMPARE_INCOMPATIBLE;
and you can drop this conditional.
+ goto cleanup; + } + if (qemuMonitorGetCPUModelComparison(proc->mon, cpu_a, cpu_b, &result) < 0) goto cleanup;
@@ -13913,6 +13969,9 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, if (qemuProcessQMPStart(proc) < 0) goto cleanup;
+ if (qemuConnectCheckCPUModel(proc->mon, cpus[0], true)) + goto cleanup; + if (VIR_ALLOC(baseline) < 0) goto error;
@@ -13921,6 +13980,9 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps,
for (i = 1; i < ncpus; i++) {
+ if (qemuConnectCheckCPUModel(proc->mon, cpus[i], true)) + goto error; +
Also in all four cases you should use qemuConnectCheckCPUModel() < 0 check. Jirka
On 19.09.19 22:24, Collin Walling wrote:
Note: since I've made some changes to a lot of these patches / split up some patches, I've decided to hold off on adding any r-b's in case there is a specific change that someone does not agree with.
Changelog:
- Properly refactored code from CPU model expansion function - Introduced a cleanup patch for CPU model expansion function - Introduced patches that modifies the refactored code to suit needs for baseline/comparison - CPU expansion function now accepts a virCPUDefPtr - Removed props parsing from CPU model comparison (they weren't used) - Cleaner error reporting when baselining/comparing with erroneous CPU models / features - Various cleanups based on feedback ___
To run these patches, execute the virsh hypervisor-cpu-compare or hypervisor-cpu-baseline commands and pass an XML file describing one or more CPU definition. You can use the definition from virsh domcapabilities or from a guest XML. There is no need extract it from the file and place it a new one, as the XML parser will look specifically for the CPU tags. ___
These patches hookup the virsh hypervisor-cpu-compare/baseline commands for the s390x architecture. They take an XML file describing some CPU definitions and passes the data to QEMU, where the actual CPU model comparison / baseline calculation is handled (available since QEMU 2.8.5).
When baselining CPU models with the --features argument, s390x will report a full CPU model expansion.
Thanks.
Nice to see this make progress after all these years I integrated support into QEMU :) -- Thanks, David / dhildenb
On Thu, Sep 19, 2019 at 16:24:51 -0400, Collin Walling wrote:
Note: since I've made some changes to a lot of these patches / split up some patches, I've decided to hold off on adding any r-b's in case there is a specific change that someone does not agree with.
Changelog:
- Properly refactored code from CPU model expansion function - Introduced a cleanup patch for CPU model expansion function - Introduced patches that modifies the refactored code to suit needs for baseline/comparison - CPU expansion function now accepts a virCPUDefPtr - Removed props parsing from CPU model comparison (they weren't used) - Cleaner error reporting when baselining/comparing with erroneous CPU models / features - Various cleanups based on feedback
Thanks. All but 15/15 are acked now and I will push them as soon as we leave the pre-release freeze period. The patch 15/15 needs a little bit of tweaking, but it is not essential and it can be pushed later as a follow-up patch. Alternatively, if you want to work on it while we are frozen, you can use the s390-cpu-apis branch (based on current master) of my staging repo https://gitlab.com/jirkade/libvirt.git Jirka
On 10/2/19 11:48 AM, Jiri Denemark wrote:
On Thu, Sep 19, 2019 at 16:24:51 -0400, Collin Walling wrote:
Note: since I've made some changes to a lot of these patches / split up some patches, I've decided to hold off on adding any r-b's in case there is a specific change that someone does not agree with.
Changelog:
- Properly refactored code from CPU model expansion function - Introduced a cleanup patch for CPU model expansion function - Introduced patches that modifies the refactored code to suit needs for baseline/comparison - CPU expansion function now accepts a virCPUDefPtr - Removed props parsing from CPU model comparison (they weren't used) - Cleaner error reporting when baselining/comparing with erroneous CPU models / features - Various cleanups based on feedback
Thanks. All but 15/15 are acked now and I will push them as soon as we leave the pre-release freeze period. The patch 15/15 needs a little bit of tweaking, but it is not essential and it can be pushed later as a follow-up patch. Alternatively, if you want to work on it while we are frozen, you can use the s390-cpu-apis branch (based on current master) of my staging repo https://gitlab.com/jirkade/libvirt.git
Jirka
Much appreciated. I'll fix 15/15 and post it as a follow-up. Thank you for your time and offering to take on the cleanups.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Respectfully, - Collin Walling
On Wed, Oct 02, 2019 at 17:48:47 +0200, Jiri Denemark wrote:
On Thu, Sep 19, 2019 at 16:24:51 -0400, Collin Walling wrote:
Note: since I've made some changes to a lot of these patches / split up some patches, I've decided to hold off on adding any r-b's in case there is a specific change that someone does not agree with.
Changelog:
- Properly refactored code from CPU model expansion function - Introduced a cleanup patch for CPU model expansion function - Introduced patches that modifies the refactored code to suit needs for baseline/comparison - CPU expansion function now accepts a virCPUDefPtr - Removed props parsing from CPU model comparison (they weren't used) - Cleaner error reporting when baselining/comparing with erroneous CPU models / features - Various cleanups based on feedback
Thanks. All but 15/15 are acked now and I will push them as soon as we leave the pre-release freeze period. The patch 15/15 needs a little bit of tweaking, but it is not essential and it can be pushed later as a follow-up patch. Alternatively, if you want to work on it while we are frozen, you can use the s390-cpu-apis branch (based on current master) of my staging repo https://gitlab.com/jirkade/libvirt.git
Patches 1-14 were pushed. Jirka
participants (3)
-
Collin Walling -
David Hildenbrand -
Jiri Denemark