7:22 p.m.
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
7:22 p.m.
New subject: [PATCH v2 1/5] qemu: check for model-expansion cap before baselining
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(a)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
7:22 p.m.
New subject: [PATCH v2 2/5] qemu: fix one instance of rc check styling in baseline
Signed-off-by: Collin Walling <walling(a)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
7:22 p.m.
New subject: [PATCH v2 3/5] qemu: report error if missing model name when baselining
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(a)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
7:22 p.m.
New subject: [PATCH v2 4/5] qemu: check if cpu model is supported before baselining
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(a)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
1:59 p.m.
New subject: [PATCH v2 4/5] qemu: check if cpu model is supported before baselining
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(a)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
7:22 p.m.
New subject: [PATCH v2 5/5] qemu: fix error message when baselining with a single cpu
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(a)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
7:29 p.m.
New subject: [PATCH v2 5/5] qemu: fix error message when baselining with a single cpu
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(a)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(a)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
11:12 a.m.
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
2:05 p.m.
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(a)redhat.com>
2:58 p.m.
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(a)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
1459
days inactive
1519
days old
10 comments
2 participants
participants (2)
-
Collin Walling -
Jiri Denemark