[PATCH v1 1/2] qemu: driver: use model name "host" for mode "host-passthrough"
When executing the hypervisor-cpu-compare/baseline commands and the XML file contains a CPU definition using host-passthrough and no model name, the commands will fail and return an error message from the QMP response. Let's fix this by checking for host-passthrough and a missing model name after the CPU definition has been converted from XML. If these conditions are matched, then the CPU definition's model name will be set to "host". Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 41 +++++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1cecef01f7..427d2419f3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12281,6 +12281,26 @@ qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps, } +static int +qemuConnectCheckCPUModel(virCPUDefPtr cpu) +{ + if (!cpu->model) { + /* + * On some architectures a model name is never present + * for the host-passthrough mode, so default it to "host" + */ + if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) { + cpu->model = g_strdup("host"); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cpu parameter is missing a model name")); + return -1; + } + } + return 0; +} + + static int qemuConnectCompareHypervisorCPU(virConnectPtr conn, const char *emulator, @@ -12336,15 +12356,9 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn, if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, &cpu) < 0) goto cleanup; - if (!cpu->model) { - if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) { - cpu->model = g_strdup("host"); - } else { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("cpu parameter is missing a model name")); - goto cleanup; - } - } + if (qemuConnectCheckCPUModel(cpu) < 0) + goto cleanup; + ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir, cfg->user, cfg->group, hvCPU, cpu, failIncompatible); @@ -12470,10 +12484,17 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, if (VIR_ALLOC(baseline) < 0) return NULL; - if (virCPUDefCopyModel(baseline, cpus[0], false)) + if (qemuConnectCheckCPUModel(cpus[0]) < 0) + return NULL; + + if (virCPUDefCopyModel(baseline, cpus[0], false) < 0) return NULL; for (i = 1; i < ncpus; i++) { + + if (qemuConnectCheckCPUModel(cpus[i]) < 0) + return NULL; + if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline, cpus[i], &result) < 0) return NULL; -- 2.26.2
When executing the hypervisor-cpu-baseline command and if there is only a single CPU definition present in the XML file, then libvirt will print an unhelpful message: "error: An error occurred, but the cause is unknown" This is due to no CPU definition ever being "baselined", since the API expects at least two CPU models. Let's fix this by performing a CPU model expansion on the single CPU definition and returning the result to the caller. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 427d2419f3..97a960a769 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12472,6 +12472,7 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, g_autoptr(qemuProcessQMP) proc = NULL; g_autoptr(virCPUDef) baseline = NULL; qemuMonitorCPUModelInfoPtr result = NULL; + qemuMonitorCPUModelExpansionType expansion_type; size_t i; if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps), @@ -12503,15 +12504,15 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, return NULL; } - if (expand_features) { - if (qemuMonitorGetCPUModelExpansion(proc->mon, - QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL, - baseline, true, false, &result) < 0) - return NULL; + expansion_type = expand_features ? QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL + : QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC; - if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) - return NULL; - } + if (qemuMonitorGetCPUModelExpansion(proc->mon, expansion_type, + baseline, true, false, &result) < 0) + return NULL; + + if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) + return NULL; return g_steal_pointer(&baseline); } -- 2.26.2
On Wed, Sep 23, 2020 at 22:26:29 -0400, Collin Walling wrote:
When executing the hypervisor-cpu-baseline command and if there is only a single CPU definition present in the XML file, then libvirt will print an unhelpful message:
"error: An error occurred, but the cause is unknown"
This is due to no CPU definition ever being "baselined", since the API expects at least two CPU models.
Let's fix this by performing a CPU model expansion on the single CPU definition and returning the result to the caller.
Ah, so when host-passthrough CPU is passed to the baseline API, we should report an error. So this patch is actually trying to handle single CPU definition with a non-empty <model> element specified. Good, as the API is of course supposed to work in this case. It should basically return the CPU definition passed to it with unsupported features disabled. Normally, expansion should only be done when requested by the corresponding API flag. The simplest fix would be just returning the original CPU definition if only one was passed to baseline. But the result might not be the correct one as it could include unsupported features. So is static expansion the thing that will return the expected result? If so, this patch is mostly correct...
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 427d2419f3..97a960a769 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12472,6 +12472,7 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, g_autoptr(qemuProcessQMP) proc = NULL; g_autoptr(virCPUDef) baseline = NULL; qemuMonitorCPUModelInfoPtr result = NULL; + qemuMonitorCPUModelExpansionType expansion_type; size_t i;
if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps), @@ -12503,15 +12504,15 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, return NULL; }
- if (expand_features) {
While full expansion has to always be performed on the baseline result, static expansion should only be called when ncpus == 1. In other words, rather than removing this condition, you should change it to if (expand_features || ncpus == 1) {
- if (qemuMonitorGetCPUModelExpansion(proc->mon, - QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL, - baseline, true, false, &result) < 0) - return NULL; + expansion_type = expand_features ? QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL + : QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC;
- if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) - return NULL; - } + if (qemuMonitorGetCPUModelExpansion(proc->mon, expansion_type, + baseline, true, false, &result) < 0) + return NULL; + + if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) + return NULL;
return g_steal_pointer(&baseline); }
Jirka
On 9/24/20 3:30 AM, Jiri Denemark wrote:
On Wed, Sep 23, 2020 at 22:26:29 -0400, Collin Walling wrote:
When executing the hypervisor-cpu-baseline command and if there is only a single CPU definition present in the XML file, then libvirt will print an unhelpful message:
"error: An error occurred, but the cause is unknown"
This is due to no CPU definition ever being "baselined", since the API expects at least two CPU models.
Let's fix this by performing a CPU model expansion on the single CPU definition and returning the result to the caller.
Ah, so when host-passthrough CPU is passed to the baseline API, we should report an error. So this patch is actually trying to handle single CPU definition with a non-empty <model> element specified. Good, as the API is of course supposed to work in this case. It should basically return the CPU definition passed to it with unsupported features disabled.
Normally, expansion should only be done when requested by the corresponding API flag. The simplest fix would be just returning the original CPU definition if only one was passed to baseline. But the result might not be the correct one as it could include unsupported
Right. In fact, if the model cannot be baselined (due to only a single CPU provided to the API), and we DON'T call expansion here, the result will just be the same verbatim XML that the user provided... which may or may not be correct... Calling expansion on the model will guarantee a sane response.
features. So is static expansion the thing that will return the expected result? If so, this patch is mostly correct...
Precisely. A static expansion will return the CPU model and a non-exhaustive list of features associated with the model.
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 427d2419f3..97a960a769 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12472,6 +12472,7 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, g_autoptr(qemuProcessQMP) proc = NULL; g_autoptr(virCPUDef) baseline = NULL; qemuMonitorCPUModelInfoPtr result = NULL; + qemuMonitorCPUModelExpansionType expansion_type; size_t i;
if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps), @@ -12503,15 +12504,15 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, return NULL; }
- if (expand_features) {
While full expansion has to always be performed on the baseline result, static expansion should only be called when ncpus == 1. In other words, rather than removing this condition, you should change it to
if (expand_features || ncpus == 1) {
Sounds good. I'll replace patch #1 with something that simply checks for a missing model name within baseline and prints a helpful error, and make the appropriate changes to this one.
- if (qemuMonitorGetCPUModelExpansion(proc->mon, - QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL, - baseline, true, false, &result) < 0) - return NULL; + expansion_type = expand_features ? QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL + : QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC;
- if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) - return NULL; - } + if (qemuMonitorGetCPUModelExpansion(proc->mon, expansion_type, + baseline, true, false, &result) < 0) + return NULL; + + if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) + return NULL;
return g_steal_pointer(&baseline); }
Jirka
Thanks! -- Regards, Collin Stay safe and stay healthy
On Wed, Sep 23, 2020 at 22:26:28 -0400, Collin Walling wrote:
When executing the hypervisor-cpu-compare/baseline commands and the XML file contains a CPU definition using host-passthrough and no model name, the commands will fail and return an error message from the QMP response.
That's the expected and correct behavior, only the error message is should be better and reported directly by libvirt.
Let's fix this by checking for host-passthrough and a missing model name after the CPU definition has been converted from XML. If these conditions are matched, then the CPU definition's model name will be set to "host".
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 41 +++++++++++++++++++++++++++++++---------- 1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1cecef01f7..427d2419f3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12281,6 +12281,26 @@ qemuConnectCPUModelComparison(virQEMUCapsPtr qemuCaps, }
+static int +qemuConnectCheckCPUModel(virCPUDefPtr cpu) +{ + if (!cpu->model) { + /* + * On some architectures a model name is never present + * for the host-passthrough mode, so default it to "host" + */ + if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) { + cpu->model = g_strdup("host"); + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cpu parameter is missing a model name")); + return -1; + } + } + return 0; +} + + static int qemuConnectCompareHypervisorCPU(virConnectPtr conn, const char *emulator, ... @@ -12470,10 +12484,17 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, if (VIR_ALLOC(baseline) < 0) return NULL;
- if (virCPUDefCopyModel(baseline, cpus[0], false)) + if (qemuConnectCheckCPUModel(cpus[0]) < 0) + return NULL; + + if (virCPUDefCopyModel(baseline, cpus[0], false) < 0) return NULL;
for (i = 1; i < ncpus; i++) { + + if (qemuConnectCheckCPUModel(cpus[i]) < 0) + return NULL; + if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline, cpus[i], &result) < 0) return NULL;
As explained in the "qemu: substitute missing model name for host-passthrough" thread, we should only make sure all CPUs passed to the baseline API have a non-empty <model> element. Jirka
participants (2)
-
Collin Walling -
Jiri Denemark