[PATCH v2 0/5] Hypervisor CPU Baseline Cleanups and Fixes
The following patches provide some TLC to the hypervisor CPU baseline handler within the qemu_driver code. #1 checks for the cpu-model-expansion capability before executing the baseline handler since it is used for feature expansion. #2 fixes a styling where a < 0 condition was missing from one of the if (function()) lines for consistency's sake. #3 will check if the cpu definition(s) are valid and contain a model name. #4 checks the cpu definition(s) model names against the model names known by the hypervisor. This patch must come before #5. #5 will allow the baseline command to be ran with a single cpu definition, whereas before the command would simply fail with an unhelpful error message. A CPU model expansion will be performed in this case, which will produce the same result as if the model were actually baselined. Note: without patch #4, #5 can result in a segfault in the case where a single CPU model is provided and the model is not recognized by the hypervisor. This is because cpu model expansion will return 0 and the result will be NULL. Since the QMP response returns "GenericError" in the case of a missing CPU model, or if the command is not supported (and perhaps for other reasons I am unsure of -- the response does not explicitly detail that the CPU model provided was erroneous), we cannot rely on this response always meaning there was a missing CPU model. So, to be safe-and-sure, the CPU model is checked against the list of CPU models known to the hypervisor prior to baselining / expanding (which were retrieved at some point previously during libvirt init). Collin Walling (5): qemu: check for model-expansion cap before baselining qemu: fix one instance of rc check styling in baseline qemu: report error if missing model name when baselining qemu: check if cpu model is supported before baselining qemu: fix error message when baselining with a single cpu src/qemu/qemu_driver.c | 45 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-) -- 2.26.2
Hypervisor-cpu-baseline requires the cpu-model-expansion capability when expanding CPU model features if the --features flag is provided. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1cecef01f7..0b68b33af8 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12563,7 +12563,8 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, (const char **)features, migratable))) goto cleanup; } else if (ARCH_IS_S390(arch) && - virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_BASELINE)) { + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_BASELINE) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) { bool expand_features = (flags & VIR_CONNECT_BASELINE_CPU_EXPAND_FEATURES); if (!(cpu = qemuConnectCPUModelBaseline(qemuCaps, cfg->libDir, -- 2.26.2
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 0b68b33af8..92684a9876 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12470,7 +12470,7 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, if (VIR_ALLOC(baseline) < 0) return NULL; - if (virCPUDefCopyModel(baseline, cpus[0], false)) + if (virCPUDefCopyModel(baseline, cpus[0], false) < 0) return NULL; for (i = 1; i < ncpus; i++) { -- 2.26.2
When executing the hypervisor-cpu-baseline command and the XML file contains a CPU definition without a model name, or an invalid CPU definition, then the commands will fail and return an error message from the QMP response. Let's clean this up by checking for a valid definition and presence of a model name. This code is copied from virCPUBaseline. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 92684a9876..1c5b1dcfee 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12460,6 +12460,19 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, qemuMonitorCPUModelInfoPtr result = NULL; size_t i; + for (i = 0; i < ncpus; i++) { + if (!cpus[i]) { + virReportError(VIR_ERR_INVALID_ARG, + _("invalid CPU definition at index %zu"), i); + return NULL; + } + if (!cpus[i]->model) { + virReportError(VIR_ERR_INVALID_ARG, + _("no CPU model specified at index %zu"), i); + return NULL; + } + } + if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps), libDir, runUid, runGid, false))) return NULL; -- 2.26.2
Check the provided CPU models against the CPU models known by the hypervisor before baselining and print an error if an unrecognized model is found. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1c5b1dcfee..fe572b13e1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12453,12 +12453,13 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, gid_t runGid, bool expand_features, virCPUDefPtr *cpus, - int ncpus) + int ncpus, + virDomainCapsCPUModelsPtr cpuModels) { g_autoptr(qemuProcessQMP) proc = NULL; g_autoptr(virCPUDef) baseline = NULL; qemuMonitorCPUModelInfoPtr result = NULL; - size_t i; + size_t i, j; for (i = 0; i < ncpus; i++) { if (!cpus[i]) { @@ -12471,6 +12472,16 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, _("no CPU model specified at index %zu"), i); return NULL; } + for (j = 0; j < cpuModels->nmodels; j++) { + if (STREQ(cpus[i]->model, cpuModels->models[j].name)) + break; + } + if (j == cpuModels->nmodels) { + virReportError(VIR_ERR_INVALID_ARG, + _("CPU model '%s' not supported by hypervisor"), + cpus[i]->model); + return NULL; + } } if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps), @@ -12582,7 +12593,8 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (!(cpu = qemuConnectCPUModelBaseline(qemuCaps, cfg->libDir, cfg->user, cfg->group, - expand_features, cpus, ncpus))) + expand_features, cpus, ncpus, + cpuModels))) goto cleanup; } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, -- 2.26.2
On Thu, Sep 24, 2020 at 20:22:38 -0400, Collin Walling wrote:
Check the provided CPU models against the CPU models known by the hypervisor before baselining and print an error if an unrecognized model is found.
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1c5b1dcfee..fe572b13e1 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12453,12 +12453,13 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, gid_t runGid, bool expand_features, virCPUDefPtr *cpus, - int ncpus) + int ncpus, + virDomainCapsCPUModelsPtr cpuModels) { g_autoptr(qemuProcessQMP) proc = NULL; g_autoptr(virCPUDef) baseline = NULL; qemuMonitorCPUModelInfoPtr result = NULL; - size_t i; + size_t i, j;
for (i = 0; i < ncpus; i++) { if (!cpus[i]) { @@ -12471,6 +12472,16 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, _("no CPU model specified at index %zu"), i); return NULL; } + for (j = 0; j < cpuModels->nmodels; j++) { + if (STREQ(cpus[i]->model, cpuModels->models[j].name)) + break; + } + if (j == cpuModels->nmodels) {
You can use an existing internal API instead: if (!virDomainCapsCPUModelsGet(cpuModels, cpus[i]->model)) {
+ virReportError(VIR_ERR_INVALID_ARG, + _("CPU model '%s' not supported by hypervisor"), + cpus[i]->model); + return NULL; + } }
if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps),
Jirka
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. This will also ensure the CPU model is sane. Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fe572b13e1..bc823fc585 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12459,6 +12459,7 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, g_autoptr(qemuProcessQMP) proc = NULL; g_autoptr(virCPUDef) baseline = NULL; qemuMonitorCPUModelInfoPtr result = NULL; + qemuMonitorCPUModelExpansionType expansion_type; size_t i, j; for (i = 0; i < ncpus; i++) { @@ -12506,9 +12507,11 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, return NULL; } - if (expand_features) { - if (qemuMonitorGetCPUModelExpansion(proc->mon, - QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL, + if (expand_features || ncpus == 1) { + expansion_type = expand_features ? QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL + : QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC; + + if (qemuMonitorGetCPUModelExpansion(proc->mon, expansion_type, baseline, true, false, &result) < 0) return NULL; -- 2.26.2
Change the commit message to: qemu: allow hypervisor-cpu-baseline with single cpu When executing the hypervisor-cpu-baseline command and if there is only a single CPU definition present in the XML file, then the baseline handler will exit early and 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 handler 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. This will also ensure the CPU model's feature set is sane if any were provided in the file. Signed-off-by: Collin Walling <walling@linux.ibm.com> On 9/24/20 8:22 PM, 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. This will also ensure the CPU model is sane.
Signed-off-by: Collin Walling <walling@linux.ibm.com> --- src/qemu/qemu_driver.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index fe572b13e1..bc823fc585 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12459,6 +12459,7 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, g_autoptr(qemuProcessQMP) proc = NULL; g_autoptr(virCPUDef) baseline = NULL; qemuMonitorCPUModelInfoPtr result = NULL; + qemuMonitorCPUModelExpansionType expansion_type; size_t i, j;
for (i = 0; i < ncpus; i++) { @@ -12506,9 +12507,11 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, return NULL; }
- if (expand_features) { - if (qemuMonitorGetCPUModelExpansion(proc->mon, - QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL, + if (expand_features || ncpus == 1) { + expansion_type = expand_features ? QEMU_MONITOR_CPU_MODEL_EXPANSION_FULL + : QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC; + + if (qemuMonitorGetCPUModelExpansion(proc->mon, expansion_type, baseline, true, false, &result) < 0) return NULL;
-- Regards, Collin Stay safe and stay healthy
Polite ping. Have there been relevant updates elsewhere that I might've missed? Thanks! On 9/24/20 8:22 PM, Collin Walling wrote:
The following patches provide some TLC to the hypervisor CPU baseline handler within the qemu_driver code.
#1 checks for the cpu-model-expansion capability before executing the baseline handler since it is used for feature expansion.
#2 fixes a styling where a < 0 condition was missing from one of the if (function()) lines for consistency's sake.
#3 will check if the cpu definition(s) are valid and contain a model name.
#4 checks the cpu definition(s) model names against the model names known by the hypervisor. This patch must come before #5.
#5 will allow the baseline command to be ran with a single cpu definition, whereas before the command would simply fail with an unhelpful error message. A CPU model expansion will be performed in this case, which will produce the same result as if the model were actually baselined.
Note: without patch #4, #5 can result in a segfault in the case where a single CPU model is provided and the model is not recognized by the hypervisor. This is because cpu model expansion will return 0 and the result will be NULL.
Since the QMP response returns "GenericError" in the case of a missing CPU model, or if the command is not supported (and perhaps for other reasons I am unsure of -- the response does not explicitly detail that the CPU model provided was erroneous), we cannot rely on this response always meaning there was a missing CPU model.
So, to be safe-and-sure, the CPU model is checked against the list of CPU models known to the hypervisor prior to baselining / expanding (which were retrieved at some point previously during libvirt init).
Collin Walling (5): qemu: check for model-expansion cap before baselining qemu: fix one instance of rc check styling in baseline qemu: report error if missing model name when baselining qemu: check if cpu model is supported before baselining qemu: fix error message when baselining with a single cpu
src/qemu/qemu_driver.c | 45 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 8 deletions(-)
-- Regards, Collin Stay safe and stay healthy
On Mon, Oct 26, 2020 at 12:12:40 -0400, Collin Walling wrote:
Polite ping. Have there been relevant updates elsewhere that I might've missed? Thanks!
Oops, sorry about the delay. I replaced the for loop with virDomainCapsCPUModelsGet in patch 4/5 as mentioned in my reply to it and pushed the series to avoid further delays. Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
On 11/24/20 3:05 PM, Jiri Denemark wrote:
On Mon, Oct 26, 2020 at 12:12:40 -0400, Collin Walling wrote:
Polite ping. Have there been relevant updates elsewhere that I might've missed? Thanks!
Oops, sorry about the delay. I replaced the for loop with virDomainCapsCPUModelsGet in patch 4/5 as mentioned in my reply to it and pushed the series to avoid further delays.
Reviewed-by: Jiri Denemark <jdenemar@redhat.com>
No worries at all. Thanks for taking the time to review and push the patches :) Enjoy the holidays! -- Regards, Collin Stay safe and stay healthy
participants (2)
-
Collin Walling -
Jiri Denemark