[libvirt] [PATCHv1 0/7] Baseline CPU model using QEMU QMP exchanges
Some architectures (S390) depend on QEMU to compute baseline CPU model. Interacting with QEMU requires starting the QEMU process and completing one or more query-cpu-model-baseline QMP exchanges with QEMU. This patch set depends on qemuMonitorGetCPUModelBaseline function exposed by "query-cpu-model-baseline QMP Command" patch set discussed previously on libvir-list. See "s390x CPU models: exposing features" patch set on Qemu-devel for discussion of QEMU aspects. This is part of resolution of: https://bugzilla.redhat.com/show_bug.cgi?id=1511999 Signed-off-by: Chris Venteicher <cventeic@redhat.com> Chris Venteicher (7): qemu_monitor_json: Properties optional in QMP JSON for CPUModelInfo qemu_capabilities: CPUModelInfo: XML/QMP format conversion qemu_capabilities: Start and connect to QEMU qemu_capabilities: Baseline CPUModel via QEMU qemu_capabilities: Find QEMU binary for S390 arch qemu_capabilities: qmperr pointer tracked in QMPCommand qemu_driver: Baseline CPU model using QEMU src/qemu/qemu_capabilities.c | 362 ++++++++++++++++++++++++++++++++++++++++++- src/qemu/qemu_capabilities.h | 9 ++ src/qemu/qemu_driver.c | 29 ++++ src/qemu/qemu_monitor_json.c | 34 ++-- 4 files changed, 414 insertions(+), 20 deletions(-) -- 2.14.1
Allow case where props not present in JSON for CPUModelInfo. Check for NULL input. Update comments to show JSON examples for more typical S390x usecase. --- src/qemu/qemu_monitor_json.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index cb17cf53bc..92db267353 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5358,7 +5358,7 @@ qemuMonitorJSONParseCPUModelProperty(const char *key, } -/* model_json: {"name": "IvyBridge", "props": {}} +/* model_json: {"name": "z13-base", "props": {}} */ static virJSONValuePtr qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model) @@ -5367,6 +5367,9 @@ qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model) virJSONValuePtr model_json = NULL; size_t i; + if (!model) + goto cleanup; + if (!(cpu_props = virJSONValueNewObject())) goto cleanup; @@ -5408,7 +5411,7 @@ qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model) } -/* model_json: {"name": "IvyBridge", "props": {}} +/* model_json: {"name": "z13-base", "props": {}} */ static qemuMonitorCPUModelInfoPtr qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model) @@ -5424,26 +5427,22 @@ qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model) goto cleanup; } - if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Parsed JSON reply 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 ((cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) { + if (VIR_ALLOC_N(machine_model->props, + virJSONValueObjectKeysNumber(cpu_props)) < 0) + goto cleanup; - if (virJSONValueObjectForeachKeyValue(cpu_props, - qemuMonitorJSONParseCPUModelProperty, - machine_model) < 0) - goto cleanup; + if (virJSONValueObjectForeachKeyValue(cpu_props, + qemuMonitorJSONParseCPUModelProperty, + machine_model) < 0) + goto cleanup; + } VIR_STEAL_PTR(model, machine_model); @@ -5586,11 +5585,8 @@ qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup; - /* Urgh, some QEMU architectures have query-cpu-model-baseline - * command but return 'GenericError' with string "Not supported", - * instead of simply omitting the command entirely - */ if (qemuMonitorJSONHasError(reply, "GenericError")) { + /* QEMU does not support query-cpu-model-baseline or cpu model */ ret = 0; goto cleanup; } -- 2.14.1
I'd recommend including this patch with your other qemu_monitor_json patch series. Let's wait until we see Jiri's patches before moving forward with discussion. On 05/05/2018 01:48 PM, Chris Venteicher wrote:
Allow case where props not present in JSON for CPUModelInfo. Check for NULL input. Update comments to show JSON examples for more typical S390x usecase. --- src/qemu/qemu_monitor_json.c | 34 +++++++++++++++------------------- 1 file changed, 15 insertions(+), 19 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index cb17cf53bc..92db267353 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5358,7 +5358,7 @@ qemuMonitorJSONParseCPUModelProperty(const char *key, }
-/* model_json: {"name": "IvyBridge", "props": {}} +/* model_json: {"name": "z13-base", "props": {}} */ static virJSONValuePtr qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model) @@ -5367,6 +5367,9 @@ qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model) virJSONValuePtr model_json = NULL; size_t i;
+ if (!model) + goto cleanup; + if (!(cpu_props = virJSONValueNewObject())) goto cleanup;
@@ -5408,7 +5411,7 @@ qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model) }
-/* model_json: {"name": "IvyBridge", "props": {}} +/* model_json: {"name": "z13-base", "props": {}} */ static qemuMonitorCPUModelInfoPtr qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model) @@ -5424,26 +5427,22 @@ qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model) goto cleanup; }
- if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Parsed JSON reply 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 ((cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) { + if (VIR_ALLOC_N(machine_model->props, + virJSONValueObjectKeysNumber(cpu_props)) < 0) + goto cleanup;
- if (virJSONValueObjectForeachKeyValue(cpu_props, - qemuMonitorJSONParseCPUModelProperty, - machine_model) < 0) - goto cleanup; + if (virJSONValueObjectForeachKeyValue(cpu_props, + qemuMonitorJSONParseCPUModelProperty, + machine_model) < 0) + goto cleanup; + }
VIR_STEAL_PTR(model, machine_model);
@@ -5586,11 +5585,8 @@ qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) goto cleanup;
- /* Urgh, some QEMU architectures have query-cpu-model-baseline - * command but return 'GenericError' with string "Not supported", - * instead of simply omitting the command entirely - */ if (qemuMonitorJSONHasError(reply, "GenericError")) { + /* QEMU does not support query-cpu-model-baseline or cpu model */ ret = 0; goto cleanup; }
-- Respectfully, - Collin Walling
Functions converting directly between virsh XML and QMEU QMP forms of CPUModelInfo. --- src/qemu/qemu_capabilities.c | 159 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 159 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 0265d83028..afce3eb2b7 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -3341,6 +3341,165 @@ virQEMUCapsLoadCache(virArch hostArch, } +/* <cpu> + * <arch>s390x</arch> + * <model>z13-base</model> + * <feature name='xxx'/> + * ... + * <feature name='yyy'/> + * </cpu> + * + * returns 0, (*model), Success + * returns 0, !(*model), Content not found + * returns -1, !(*model), Failure + */ +static int +virQEMUCapsCPUModelInfoFromXML(xmlXPathContextPtr ctxt, + const char *xpath, + qemuMonitorCPUModelInfoPtr *model) +{ + size_t i; + int ret = -1; + qemuMonitorCPUModelInfoPtr cpuModel = NULL; + xmlNodePtr *nodes = NULL; + xmlNodePtr oldnode = ctxt->node; + + VIR_DEBUG("xpath =%s", NULLSTR(xpath)); + + *model = NULL; + + /* optional xpath query for cpu node with nonerror exit on miss */ + if (xpath && !(ctxt->node = virXPathNode(xpath, ctxt))) { + ret = 0; + goto cleanup; + } + + if (!virXMLNodeNameEqual(ctxt->node, "cpu")) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("XML does not contain expected 'cpu' element")); + goto cleanup; + } + + if (VIR_ALLOC(cpuModel) < 0) + goto cleanup; + + if (!(cpuModel->name = virXPathString("string(./model[1])", ctxt))) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("Missing CPU model name")); + goto cleanup; + } + + cpuModel->nprops = virXPathNodeSet("./feature", ctxt, &nodes); + + if (VIR_ALLOC_N(cpuModel->props, cpuModel->nprops) < 0) + goto cleanup; + + for (i = 0; i < cpuModel->nprops; i++) { + qemuMonitorCPUPropertyPtr prop = &(cpuModel->props[i]); + + prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN; + prop->value.boolean = true; + prop->migratable = VIR_TRISTATE_BOOL_ABSENT; + + if (!(prop->name = virXMLPropString(nodes[i], "name"))) { + virReportError(VIR_ERR_XML_ERROR, "%s", _("Invalid CPU feature name")); + goto cleanup; + } + } + + *model = qemuMonitorCPUModelInfoCopy(cpuModel); + + ret = 0; + + VIR_DEBUG("model->name = %s", NULLSTR((*model)->name)); + + cleanup: + ctxt->node = oldnode; + VIR_FREE(nodes); + qemuMonitorCPUModelInfoFree(cpuModel); + return ret; +} + + +/* + * returns 0, (*model), Success + * returns 0, !(*model), Content not found + * returns -1, !(*model), Failure + */ +static int +virQEMUCapsLoadCPUModelInfoFromXMLString(const char *xml, + const char *xpath, + qemuMonitorCPUModelInfoPtr *model) +{ + int ret = -1; + + xmlDocPtr doc = NULL; + xmlXPathContextPtr ctxt = NULL; + + *model = NULL; + + VIR_DEBUG("input = %s", NULLSTR(xml)); + + if (!(doc = virXMLParseStringCtxt(xml, _("(CPU_definition)"), &ctxt))) { + virReportError(VIR_ERR_INVALID_ARG, "%s", _("missing CPU definition")); + goto cleanup; + } + + if (virQEMUCapsCPUModelInfoFromXML(ctxt, xpath, model) < 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("XML does not contain expected 'cpu' element")); + goto cleanup; + } + + ret = 0; + + cleanup: + xmlXPathFreeContext(ctxt); + xmlFreeDoc(doc); + + return ret; +} + + +/* <cpu match='exact'> + * <model>z13-base</model> + * <feature policy='require' name='xxx'/> + * <feature policy='require' name='yyy'/> + * </cpu> + */ +static void +virQEMUCapsCPUModelInfoToXML(qemuMonitorCPUModelInfoPtr model, virBufferPtr buf) +{ + size_t i; + + if (!model) + return; + + virBufferAddLit(buf, "<cpu match='exact'>\n"); + + virBufferAdjustIndent(buf, 2); + + virBufferAsprintf(buf, "<model>%s</model>\n", model->name); + + for (i = 0; i < model->nprops; i++) { + qemuMonitorCPUPropertyPtr prop = &(model->props[i]); + + virBufferAsprintf(buf, "<feature policy='require' name='%s'/>\n", + prop->name); + } + + virBufferAdjustIndent(buf, -2); + + virBufferAddLit(buf, "</cpu>\n"); +} + + +/* + * <hostCPU type='%s' model='%s' migratability='%s'> + * <property name='%s' type='%s' value='%s'/> + * ... + * <property name='%s' type='%s' value='%s'/> + * </hostCPU> + */ static void virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps, virBufferPtr buf, -- 2.14.1
On Sat, May 05, 2018 at 12:48:44 -0500, Chris Venteicher wrote:
Functions converting directly between virsh XML and QMEU QMP forms of CPUModelInfo.
The XML parsing formatting code is in src/conf/cpu_conf.c and there's no need to duplicate it here. The translation should just work between virCPUDef and qemuMonitorCPUModelInfo and use the APIs from cpu_conf.c for XML conversion. Jirka
Start and connect to QEMU so QMP commands can be performed. 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 | 59 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index afce3eb2b7..097985cbe7 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4303,6 +4303,65 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, } +/* Start and connect to QEMU so QMP commands can be performed. + */ +static virQEMUCapsInitQMPCommandPtr +virQEMUCapsSpinUpQemu(const char *exec, + const char *libDir, uid_t runUid, gid_t runGid, + bool forceTCG) +{ + virQEMUCapsInitQMPCommandPtr cmd = NULL; + virQEMUCapsInitQMPCommandPtr rtn_cmd = NULL; + char *binary = NULL; + + if (exec) { + if (VIR_STRDUP(binary, exec) < 0) + goto cleanup; + } else { + /* Check for existence of base emulator, or alternate base + * which can be used with magic cpu choice + */ + virArch arch = virArchFromHost(); + binary = virQEMUCapsFindBinaryForArch(arch, arch); + } + + VIR_DEBUG("binary=%s", binary); + + /* Make sure the binary we are about to try exec'ing exists. + * Technically we could catch the exec() failure, but that's + * in a sub-process so it's hard to feed back a useful error. + */ + if (!virFileIsExecutable(binary)) { + virReportSystemError(errno, _("QEMU binary %s is not executable"), + binary); + goto cleanup; + } + + if (!(cmd = virQEMUCapsInitQMPCommandNew(binary, libDir, + runUid, runGid, NULL))) + goto cleanup; + + if ((virQEMUCapsInitQMPCommandRun(cmd, forceTCG)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error starting QEMU")); + goto cleanup; + } + + if (!(cmd->mon)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Error connecting to QEMU")); + goto cleanup; + } + + VIR_STEAL_PTR(rtn_cmd, cmd); + + cleanup: + virQEMUCapsInitQMPCommandFree(cmd); + VIR_FREE(binary); + + return rtn_cmd; +} + + static int virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, const char *libDir, -- 2.14.1
On Sat, May 05, 2018 at 12:48:45 -0500, Chris Venteicher wrote:
Start and connect to QEMU so QMP commands can be performed.
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 | 59 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index afce3eb2b7..097985cbe7 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4303,6 +4303,65 @@ virQEMUCapsInitQMPCommandRun(virQEMUCapsInitQMPCommandPtr cmd, }
+/* Start and connect to QEMU so QMP commands can be performed. + */ +static virQEMUCapsInitQMPCommandPtr +virQEMUCapsSpinUpQemu(const char *exec, + const char *libDir, uid_t runUid, gid_t runGid, + bool forceTCG)
This wrapper should have a different signature (accepting qemuCaps), and probably a different name which would suggest it's for onetime QMP commands. I'm not sure what the right name would be, though :-)
+{ + virQEMUCapsInitQMPCommandPtr cmd = NULL; + virQEMUCapsInitQMPCommandPtr rtn_cmd = NULL; + char *binary = NULL; + + if (exec) { + if (VIR_STRDUP(binary, exec) < 0) + goto cleanup; + } else { + /* Check for existence of base emulator, or alternate base + * which can be used with magic cpu choice + */ + virArch arch = virArchFromHost(); + binary = virQEMUCapsFindBinaryForArch(arch, arch); + } + + VIR_DEBUG("binary=%s", binary);
We have a cache of QEMU capabilities for all binaries we found so there's no need to try to look for them again.
+ + /* Make sure the binary we are about to try exec'ing exists. + * Technically we could catch the exec() failure, but that's + * in a sub-process so it's hard to feed back a useful error. + */ + if (!virFileIsExecutable(binary)) { + virReportSystemError(errno, _("QEMU binary %s is not executable"), + binary); + goto cleanup; + }
Getting qemuCaps from the cache will also make sure the binary is executable.
+ + if (!(cmd = virQEMUCapsInitQMPCommandNew(binary, libDir, + runUid, runGid, NULL))) + goto cleanup;
This will use capabilities.monitor.sock and capabilities.pidfile for all processes. We either need to serialize this code with caps probing and also serialize all threads running baseline/compare QMP commands, or we need to refactor virQEMUCapsInitQMPCommandNew a bit so that it can be used simultaneously by several threads.
+ + if ((virQEMUCapsInitQMPCommandRun(cmd, forceTCG)) < 0) {
No need for extra () since there's no assignment here.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Error starting QEMU"));
This would override any useful error already set by virQEMUCapsInitQMPCommandRun.
+ goto cleanup; + } + + if (!(cmd->mon)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Error connecting to QEMU")); + goto cleanup; + }
We should just check virQEMUCapsInitQMPCommandRun returned 0 and fail otherwise.
+ + VIR_STEAL_PTR(rtn_cmd, cmd); + + cleanup: + virQEMUCapsInitQMPCommandFree(cmd); + VIR_FREE(binary); + + return rtn_cmd; +} + + static int virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, const char *libDir, -- 2.14.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Spinup QEMU instance and complete QMP query-cpu-model-baseline exchanges to determine CPUModel Baseline. query-cpu-model-baseline only compares two CPUModels so multiple exchanges are needed to evaluate more than two CPUModels. --- src/qemu/qemu_capabilities.c | 132 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_capabilities.h | 9 +++ 2 files changed, 141 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 097985cbe7..9ffbf91a90 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5122,3 +5122,135 @@ virQEMUCapsSetMicrocodeVersion(virQEMUCapsPtr qemuCaps, { qemuCaps->microcodeVersion = microcodeVersion; } + + +/* in: + * <host> + * <cpu> + * <arch>s390x</arch> + * <model>z13-base</model> + * <feature name='xxx'/> + * ... + * <feature name='yyy'/> + * </cpu> + * <cpu> + * <arch>s390x</arch> + * <model>z114-base</model> + * <feature name='xxx'/> + * ... + * <feature name='yyy'/> + * </cpu> + * </host> + * + * out: + * <cpu match='exact'> + * <model>z13-base</model> + * <feature policy='require' name='xxx'/> + * <feature policy='require' name='yyy'/> + * </cpu> + * + * (ret==0) && (baseline.len == 0) if baseline not supported by QEMU + */ +int +virQEMUCapsQMPBaselineCPUModel(char *exec, + const char *libDir, + uid_t runUid, + gid_t runGid, + const char **xmlCPUs, + unsigned int ncpus, + bool migratable, + virBufferPtr baseline) +{ + qemuMonitorCPUModelInfoPtr model_baseline = NULL; + qemuMonitorCPUModelInfoPtr new_model_baseline = NULL; + qemuMonitorCPUModelInfoPtr model_b = NULL; + virQEMUCapsInitQMPCommandPtr cmd = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + bool forceTCG = false; + int ret = -1; + size_t i; + + VIR_DEBUG("ncpus=%i, migratable=%i", ncpus, migratable); + + if (!(cmd = virQEMUCapsSpinUpQemu(exec, libDir, runUid, runGid, forceTCG))) + goto cleanup; + + if (!(cmd->mon)) { + /* Not connected to QEMU (monitor) */ + VIR_DEBUG("QEMU failed to initialize error=%s", virGetLastErrorMessage()); + goto cleanup; + } + + /* QEMU requires qmp_capabilities exchange first */ + if (qemuMonitorSetCapabilities(cmd->mon) < 0) { + VIR_DEBUG("Failed to set monitor capabilities %s", + virGetLastErrorMessage()); + goto cleanup; + } + + if (!xmlCPUs && ncpus != 0) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("nonzero ncpus doesn't match with NULL xmlCPUs")); + goto cleanup; + } + + if (ncpus < 2) { + virReportError(VIR_ERR_INVALID_ARG, "%s", _("baseline < 2 CPUs ")); + goto cleanup; + } + + if (virQEMUCapsLoadCPUModelInfoFromXMLString(xmlCPUs[0], "//cpu", + &model_baseline) < 0) + goto cleanup; + + VIR_DEBUG("xmlCPUs[0] = %s", xmlCPUs[0]); + + for (i = 1; i < ncpus; i++) { + + VIR_DEBUG("xmlCPUs[%lu] = %s", i, xmlCPUs[i]); + + if (virQEMUCapsLoadCPUModelInfoFromXMLString(xmlCPUs[i], "//cpu", &model_b) < 0) + goto cleanup; + + if (!model_baseline || !model_b) { + virReportError(VIR_ERR_INVALID_ARG, "%s", _("xmlCPU without content")); + goto cleanup; + } + + if (qemuMonitorGetCPUModelBaseline(cmd->mon, model_baseline, + model_b, &new_model_baseline) < 0) + goto cleanup; + + if (!new_model_baseline) { + VIR_DEBUG("QEMU didn't support baseline"); + ret = 0; + goto cleanup; + } + + qemuMonitorCPUModelInfoFree(model_baseline); + qemuMonitorCPUModelInfoFree(model_b); + + model_b = NULL; + + model_baseline = new_model_baseline; + } + + VIR_DEBUG("model_baseline->name = %s", NULLSTR(model_baseline->name)); + + virQEMUCapsCPUModelInfoToXML(model_baseline, &buf); + + if (virBufferUse(&buf)) + virBufferAddBuffer(baseline, &buf); + + ret = 0; + + cleanup: + virQEMUCapsInitQMPCommandFree(cmd); + + qemuMonitorCPUModelInfoFree(model_baseline); + qemuMonitorCPUModelInfoFree(model_b); + + VIR_DEBUG("baseline = %s", virBufferCurrentContent(baseline)); + + return ret; +} diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 8da18a8063..2d3e8ce2ad 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -536,6 +536,15 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps, void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps, const char *machineType); +int virQEMUCapsQMPBaselineCPUModel(char *exec, + const char *libDir, + uid_t runUid, + gid_t runGid, + const char **xmlCPUs, + unsigned int ncpus, + bool migratable, + virBufferPtr baseline); + virFileCachePtr virQEMUCapsCacheNew(const char *libDir, const char *cacheDir, uid_t uid, -- 2.14.1
On Sat, May 05, 2018 at 12:48:46 -0500, Chris Venteicher wrote:
Spinup QEMU instance and complete QMP query-cpu-model-baseline exchanges to determine CPUModel Baseline.
query-cpu-model-baseline only compares two CPUModels so multiple exchanges are needed to evaluate more than two CPUModels. --- src/qemu/qemu_capabilities.c | 132 +++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_capabilities.h | 9 +++ 2 files changed, 141 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 097985cbe7..9ffbf91a90 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -5122,3 +5122,135 @@ virQEMUCapsSetMicrocodeVersion(virQEMUCapsPtr qemuCaps, { qemuCaps->microcodeVersion = microcodeVersion; } + + +/* in: + * <host> + * <cpu> + * <arch>s390x</arch> + * <model>z13-base</model> + * <feature name='xxx'/> + * ... + * <feature name='yyy'/> + * </cpu> + * <cpu> + * <arch>s390x</arch> + * <model>z114-base</model> + * <feature name='xxx'/> + * ... + * <feature name='yyy'/> + * </cpu> + * </host> + * + * out: + * <cpu match='exact'> + * <model>z13-base</model> + * <feature policy='require' name='xxx'/> + * <feature policy='require' name='yyy'/> + * </cpu> + * + * (ret==0) && (baseline.len == 0) if baseline not supported by QEMU + */ +int +virQEMUCapsQMPBaselineCPUModel(char *exec, + const char *libDir, + uid_t runUid, + gid_t runGid, + const char **xmlCPUs, + unsigned int ncpus, + bool migratable, + virBufferPtr baseline) +{ + qemuMonitorCPUModelInfoPtr model_baseline = NULL; + qemuMonitorCPUModelInfoPtr new_model_baseline = NULL; + qemuMonitorCPUModelInfoPtr model_b = NULL; + virQEMUCapsInitQMPCommandPtr cmd = NULL; + virBuffer buf = VIR_BUFFER_INITIALIZER; + bool forceTCG = false; + int ret = -1; + size_t i; + + VIR_DEBUG("ncpus=%i, migratable=%i", ncpus, migratable); + + if (!(cmd = virQEMUCapsSpinUpQemu(exec, libDir, runUid, runGid, forceTCG))) + goto cleanup; + + if (!(cmd->mon)) {
This test will never be true.
+ /* Not connected to QEMU (monitor) */ + VIR_DEBUG("QEMU failed to initialize error=%s", virGetLastErrorMessage()); + goto cleanup; + } + + /* QEMU requires qmp_capabilities exchange first */ + if (qemuMonitorSetCapabilities(cmd->mon) < 0) { + VIR_DEBUG("Failed to set monitor capabilities %s", + virGetLastErrorMessage()); + goto cleanup; + }
This should be moved inside virQEMUCapsSpinUpQemu. And the rest of the function needs to be changed to work on CPUDefs already parsed by the caller. Jirka
S390 uses qemu-kvm in /usr/libexec. --- src/qemu/qemu_capabilities.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 9ffbf91a90..ac7569679c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -687,6 +687,11 @@ virQEMUCapsFindBinaryForArch(virArch hostarch, goto out; } + /* Fourth attempt: try 'qemu-kvm' */ + if ((ret = virQEMUCapsFindBinary("%s", "/usr/libexec/qemu-kvm")) != NULL) + goto out; + + out: return ret; } -- 2.14.1
On Sat, May 05, 2018 at 12:48:47 -0500, Chris Venteicher wrote:
S390 uses qemu-kvm in /usr/libexec.
That's incorrect, /usr/libexec/qemu-kvm is the native binary on RHEL/CentOS. That is it's arch matches host arch.
--- src/qemu/qemu_capabilities.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 9ffbf91a90..ac7569679c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -687,6 +687,11 @@ virQEMUCapsFindBinaryForArch(virArch hostarch, goto out; }
+ /* Fourth attempt: try 'qemu-kvm' */ + if ((ret = virQEMUCapsFindBinary("%s", "/usr/libexec/qemu-kvm")) != NULL) + goto out; + + out: return ret; }
NACK, this is handled elsewhere.
On 05/17/2018 04:41 PM, Jiri Denemark wrote:
On Sat, May 05, 2018 at 12:48:47 -0500, Chris Venteicher wrote:
S390 uses qemu-kvm in /usr/libexec.
That's incorrect, /usr/libexec/qemu-kvm is the native binary on RHEL/CentOS. That is it's arch matches host arch.
--- src/qemu/qemu_capabilities.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 9ffbf91a90..ac7569679c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -687,6 +687,11 @@ virQEMUCapsFindBinaryForArch(virArch hostarch, goto out; }
+ /* Fourth attempt: try 'qemu-kvm' */ + if ((ret = virQEMUCapsFindBinary("%s", "/usr/libexec/qemu-kvm")) != NULL) + goto out; + + out: return ret; }
NACK, this is handled elsewhere.
This is going to sound silly since the string "redhat" is found everywhere in these emails, but do you think it's worthwhile to also add the ability to probe the other native binaries that are used by other distros? For example (and I believe is why you mention "this is handled elsewhere") virQEMUCapsInitGuest (src/qemu/qemu_capabilities.c) will check for the different "kvmbins" for RHEL, Fedora, and Debian/Ubuntu. A little bit of work would allow us to reuse this to get the appropriate qemu caps for the other distros. A "virQEMUCapsFindNativeBinary" or something like that comes to mind. Just a thought.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Respectfully, - Collin Walling
On Thu, May 17, 2018 at 17:02:23 -0400, Collin Walling wrote:
On 05/17/2018 04:41 PM, Jiri Denemark wrote:
On Sat, May 05, 2018 at 12:48:47 -0500, Chris Venteicher wrote:
S390 uses qemu-kvm in /usr/libexec.
That's incorrect, /usr/libexec/qemu-kvm is the native binary on RHEL/CentOS. That is it's arch matches host arch.
--- src/qemu/qemu_capabilities.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 9ffbf91a90..ac7569679c 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -687,6 +687,11 @@ virQEMUCapsFindBinaryForArch(virArch hostarch, goto out; }
+ /* Fourth attempt: try 'qemu-kvm' */ + if ((ret = virQEMUCapsFindBinary("%s", "/usr/libexec/qemu-kvm")) != NULL) + goto out; + + out: return ret; }
NACK, this is handled elsewhere.
This is going to sound silly since the string "redhat" is found everywhere in these emails, but do you think it's worthwhile to also add the ability to probe the other native binaries that are used by other distros?
Sure, but do you know any other distro which would use another stupid non-standard location and name for QEMU binaries? I hope they would all use upstream names and locations, i.e., /usr/bin/qemu-system-$ARCH or /usr/libexec/qemu-kvm if they are derived from RHEL. If there are other options, I guess we should handle them. Currently, we probe for /usr/libexec/qemu-kvm, qemu-kvm, and kvm in virQEMUCapsInitGuest and we could easily extend this list if needed. Jirka
Allow QEMU process to be started without requirement for caller to maintain handle to qmperr pointer. The handle to qmperr pointer can be stored in QMPCommand structure (new way) stored in calling function's stack (original way). --- 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 ac7569679c..431378cc02 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4110,6 +4110,7 @@ struct _virQEMUCapsInitQMPCommand { uid_t runUid; gid_t runGid; char **qmperr; + char *qmperr_internal; char *monarg; char *monpath; char *pidfile; @@ -4187,7 +4188,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.14.1
On Sat, May 05, 2018 at 12:48:48 -0500, Chris Venteicher wrote:
Allow QEMU process to be started without requirement for caller to maintain handle to qmperr pointer.
The handle to qmperr pointer can be stored in QMPCommand structure (new way) stored in calling function's stack (original way). --- 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 ac7569679c..431378cc02 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -4110,6 +4110,7 @@ struct _virQEMUCapsInitQMPCommand { uid_t runUid; gid_t runGid; char **qmperr; + char *qmperr_internal; char *monarg; char *monpath; char *pidfile; @@ -4187,7 +4188,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"
It's not really clear what you're trying to solve with this patch and why it is needed. Jirka
virsh cpu-baseline determines baseline from QEMU for some architectures and libvirt for others. Skip the QEMU attempt to save time on architectures QEMU baseline does not support. --- src/qemu/qemu_driver.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 83fc191085..6096d46ab5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13166,6 +13166,15 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, virCPUDefPtr baseline = NULL; virCPUDefPtr cpu = NULL; char *cpustr = NULL; + virBuffer baseline_xml = VIR_BUFFER_INITIALIZER; + virArch arch = virArchFromHost(); + + VIR_DEBUG("ncpus=%i, flags=0x%x", ncpus, flags); + + virQEMUDriverPtr driver = conn->privateData; + virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + + bool migratable = !!(flags & VIR_CONNECT_BASELINE_CPU_MIGRATABLE); virCheckFlags(VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES | VIR_CONNECT_BASELINE_CPU_MIGRATABLE, NULL); @@ -13173,6 +13182,23 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, if (virConnectBaselineCPUEnsureACL(conn) < 0) goto cleanup; + + if (!ARCH_IS_S390(arch)) + goto libvirt_method; /* Avoid starting QEMU */ + + /* Try QEMU method + */ + if (virQEMUCapsQMPBaselineCPUModel(NULL, + cfg->libDir, cfg->user, cfg->group, + xmlCPUs, ncpus, migratable, &baseline_xml) < 0) { + /* Content Error */ + goto cleanup; + } + + if ((cpustr = virBufferContentAndReset(&baseline_xml))) + goto done; + + libvirt_method: if (!(cpus = virCPUDefListParse(xmlCPUs, ncpus, VIR_CPU_TYPE_HOST))) goto cleanup; @@ -13199,6 +13225,9 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, virCPUDefFree(baseline); virCPUDefFree(cpu); + done: + VIR_DEBUG("qemu cpustr = %s", NULLSTR(cpustr)); + return cpustr; } -- 2.14.1
On Sat, May 05, 2018 at 12:48:49 -0500, Chris Venteicher wrote:
virsh cpu-baseline determines baseline from QEMU for some architectures and libvirt for others.
Skip the QEMU attempt to save time on architectures QEMU baseline does not support.
This is not the right direction. As explained by me and Daniel in response to Collin, we need a new public API for this functionality. I'm already working on new CPU related APIs and I hope to send them soon. Jirka
On Sat, May 05, 2018 at 12:48:42 -0500, Chris Venteicher wrote:
Some architectures (S390) depend on QEMU to compute baseline CPU model.
Interacting with QEMU requires starting the QEMU process and completing one or more query-cpu-model-baseline QMP exchanges with QEMU.
This patch set depends on qemuMonitorGetCPUModelBaseline function exposed by "query-cpu-model-baseline QMP Command" patch set discussed previously on libvir-list.
Since patch 1/7 changes some code introduced in the series implementing query-cpu-model-baseline support, please send patches for both series at once next time and (as already suggested by Collin) squash the changes to the patches which introduced the code you're fixing here in 1/7. Overall, I think the approach of making the monitor API work on CPUModelInfo is better than the one using CPUDef because the monitor code in general does not have all data it could potentially need to perform the translation. Jirka
On 05/17/2018 04:15 PM, Jiri Denemark wrote:
On Sat, May 05, 2018 at 12:48:42 -0500, Chris Venteicher wrote:
Some architectures (S390) depend on QEMU to compute baseline CPU model.
Interacting with QEMU requires starting the QEMU process and completing one or more query-cpu-model-baseline QMP exchanges with QEMU.
This patch set depends on qemuMonitorGetCPUModelBaseline function exposed by "query-cpu-model-baseline QMP Command" patch set discussed previously on libvir-list.
Since patch 1/7 changes some code introduced in the series implementing query-cpu-model-baseline support, please send patches for both series at once next time and (as already suggested by Collin) squash the changes to the patches which introduced the code you're fixing here in 1/7.
Overall, I think the approach of making the monitor API work on CPUModelInfo is better than the one using CPUDef because the monitor code in general does not have all data it could potentially need to perform the translation.
Jirka
I can agree with this. qemuMonitorCPUModelInfo is simpler and has everything qmp needs. I also concur with your response to 2/7. Translating ModelInfo -> CPUDef and using the existing CPUDef -> XML functions is the way to go. Since we're getting a closer to an agreement on how this API should work, I could respin my comparison patches with what we've learned thus far to see how they look. Otherwise, I have no problem waiting until we come up with something more definitive -- I have plenty to keep me busy in the meantime :)
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Respectfully, - Collin Walling
Quoting Collin Walling (2018-05-17 16:07:14)
On 05/17/2018 04:15 PM, Jiri Denemark wrote:
On Sat, May 05, 2018 at 12:48:42 -0500, Chris Venteicher wrote:
Some architectures (S390) depend on QEMU to compute baseline CPU model.
Interacting with QEMU requires starting the QEMU process and completing one or more query-cpu-model-baseline QMP exchanges with QEMU.
This patch set depends on qemuMonitorGetCPUModelBaseline function exposed by "query-cpu-model-baseline QMP Command" patch set discussed previously on libvir-list.
Since patch 1/7 changes some code introduced in the series implementing query-cpu-model-baseline support, please send patches for both series at once next time and (as already suggested by Collin) squash the changes to the patches which introduced the code you're fixing here in 1/7.
Overall, I think the approach of making the monitor API work on CPUModelInfo is better than the one using CPUDef because the monitor code in general does not have all data it could potentially need to perform the translation.
Jirka
I can agree with this. qemuMonitorCPUModelInfo is simpler and has everything qmp needs.
I also concur with your response to 2/7. Translating ModelInfo -> CPUDef and using the existing CPUDef -> XML functions is the way to go.
Since we're getting a closer to an agreement on how this API should work, I could respin my comparison patches with what we've learned thus far to see how they look. Otherwise, I have no problem waiting until we come up with something more definitive -- I have plenty to keep me busy in the meantime :)
Thanks for the feedback. Most of the comments make immediate sense. The rest I will need a few days in the code to dig deeper to understand. Jirka, - Looks like S390 support for baseline / compare still needs to be added to your "new cpu related apis". (Exchanging QMP messages so QEMU can do the baseline / compare operation.) The baseline patches I am working on should add S390 support to your new cpu related apis. - Looks like S390 support should still be added to virsh cpu-baseline (and cpu-compare) but there are dependencies on the changes made in the "new cpu related apis" patch set. Collin, This is my main task right now but the code is new to me so it might take a week or two to get to a new patch set for baseline. Thanks, Chris
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Respectfully, - Collin Walling
On Sat, May 19, 2018 at 08:31:50 -0500, Chris Venteicher wrote:
Most of the comments make immediate sense. The rest I will need a few days in the code to dig deeper to understand.
Jirka, - Looks like S390 support for baseline / compare still needs to be added to your "new cpu related apis". (Exchanging QMP messages so QEMU can do the baseline / compare operation.) The baseline patches I am working on should add S390 support to your new cpu related apis.
Yes, the s390 code would just fit in a new "else if (ARCH_IS_S390(arch))" branch in qemuConnectCompareHypervisorCPU.
- Looks like S390 support should still be added to virsh cpu-baseline (and cpu-compare) but there are dependencies on the changes made in the "new cpu related apis" patch set.
No, the old APIs won't get any support for s390. Jirka
participants (3)
-
Chris Venteicher -
Collin Walling -
Jiri Denemark