3:06 p.m.
changes in v2:
- rebased with newer master (412cc0f403)
- changed the 'news' text to be in the v5.9.0 section
Hi,
This short series exposes, implements and documents a
pSeries guest capability called Count Cache Flush Assist
(ccf-assist), which was added in the Linux kernel since
5.1 and in QEMU since 4.0.0.
The reason why this capability needs to exposed via Libvirt
is because it is defaulted to 'off' in QEMU to not break
migration compatibility in the Power 9 processor class. However,
there is a performance gain in activating it, thus the user
might want to choose less migration options for more power.
More details can be found in the commit message of patch 1.
v1: https://www.redhat.com/archives/libvir-list/2019-September/msg00649.html
Daniel Henrique Barboza (3):
qemu: Add capability for the ccf-assist pSeries feature
qemu: Implement the ccf-assist pSeries feature
news: Update for the ccf-assist pSeries feature
docs/formatdomain.html.in | 9 +++++++++
docs/news.xml | 9 +++++++++
docs/schemas/domaincommon.rng | 5 +++++
src/conf/domain_conf.c | 4 ++++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
src/qemu/qemu_command.c | 20 +++++++++++++++++++
src/qemu/qemu_domain.c | 1 +
.../qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 +
tests/qemuxml2argvdata/pseries-features.args | 2 +-
tests/qemuxml2argvdata/pseries-features.xml | 1 +
tests/qemuxml2argvtest.c | 1 +
tests/qemuxml2xmloutdata/pseries-features.xml | 1 +
tests/qemuxml2xmltest.c | 1 +
15 files changed, 58 insertions(+), 1 deletion(-)
--
2.21.0
3:06 p.m.
New subject: [libvirt] [PATCH v2 1/3] qemu: Add capability for the ccf-assist pSeries feature
Linux kernel 5.1 added a new PPC KVM capability named
KVM_PPC_CPU_CHAR_BCCTR_FLUSH_ASSIST, which is exposed to the QEMU guest
since QEMU commit 8ff43ee404d under a new sPAPR capability called
SPAPR_CAP_CCF_ASSIST. This cap indicates whether the processor supports
hardware acceleration for the count cache flush workaround, which
is a software workaround that flushes the count cache on context
switch. If the processor has this hardware acceleration, the software
flush can be shortened, resulting in performance gain.
This hardware acceleration is defaulted to 'off' in QEMU. The reason
is that earlier versions of the Power 9 processor didn't support
it (it is available on Power 9 DD2.3 and newer), and defaulting this
option to 'on' would break migration compatibility between the Power 9
processor class.
However, the user running a P9 DD2.3+ hypervisor might want to create
guests with ccf-assist=on, accepting the downside of only being able
to migrate them only between other P9 DD2.3+ hosts running upstream
kernel 5.1+, to get a performance boost.
This patch adds this new capability to Libvirt, with the name of
QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_capabilities.c | 2 ++
src/qemu/qemu_capabilities.h | 1 +
tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml | 1 +
3 files changed, 4 insertions(+)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 46a056340b..b73534c0d1 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -545,6 +545,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"incremental-backup",
"query-cpu-model-baseline",
"query-cpu-model-comparison",
+ "machine.pseries.cap-ccf-assist",
);
@@ -1440,6 +1441,7 @@ static struct virQEMUCapsStringFlags
virQEMUCapsMachinePropsPSeries[] = {
{ "cap-hpt-max-page-size", QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE
},
{ "cap-htm", QEMU_CAPS_MACHINE_PSERIES_CAP_HTM },
{ "cap-nested-hv", QEMU_CAPS_MACHINE_PSERIES_CAP_NESTED_HV },
+ { "cap-ccf-assist", QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST },
};
static struct virQEMUCapsStringFlags virQEMUCapsMachinePropsVirt[] = {
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index 72da3691f2..3da8e3d0f0 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -526,6 +526,7 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check
*/
QEMU_CAPS_INCREMENTAL_BACKUP, /* incremental backup is supported */
QEMU_CAPS_QUERY_CPU_MODEL_BASELINE, /* qmp query-cpu-model-baseline */
QEMU_CAPS_QUERY_CPU_MODEL_COMPARISON, /* qmp query-cpu-model-comparison */
+ QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST, /* -machine pseries.cap-ccf-assist */
QEMU_CAPS_LAST /* this must always be the last item */
} virQEMUCapsFlags;
diff --git a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
index 9ea6f4d046..5edd2c1c31 100644
--- a/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
+++ b/tests/qemucapabilitiesdata/caps_4.0.0.ppc64.xml
@@ -170,6 +170,7 @@
<flag name='nbd-bitmap'/>
<flag name='bochs-display'/>
<flag name='migration-file-drop-cache'/>
+ <flag name='machine.pseries.cap-ccf-assist'/>
<version>4000000</version>
<kvmVersion>0</kvmVersion>
<microcodeVersion>42900758</microcodeVersion>
--
2.21.0
3:06 p.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: Implement the ccf-assist pSeries feature
This patch adds the implementation of the ccf-assist pSeries
feature, based on the QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST
capability that was added in the previous patch.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
docs/formatdomain.html.in | 9 +++++++++
docs/schemas/domaincommon.rng | 5 +++++
src/conf/domain_conf.c | 4 ++++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_command.c | 20 +++++++++++++++++++
src/qemu/qemu_domain.c | 1 +
tests/qemuxml2argvdata/pseries-features.args | 2 +-
tests/qemuxml2argvdata/pseries-features.xml | 1 +
tests/qemuxml2argvtest.c | 1 +
tests/qemuxml2xmloutdata/pseries-features.xml | 1 +
tests/qemuxml2xmltest.c | 1 +
11 files changed, 45 insertions(+), 1 deletion(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 647f96f651..059781a0c3 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2059,6 +2059,7 @@
<tseg unit='MiB'>48</tseg>
</smm>
<htm state='on'/>
+ <ccf-assist state='on'/>
<msrs unknown='ignore'/>
</features>
...</pre>
@@ -2357,6 +2358,14 @@
will not be ignored.
<span class="since">Since 5.1.0</span> (bhyve only)
</dd>
+ <dt><code>ccf-assist</code></dt>
+ <dd>Configure ccf-assist (Count Cache Flush Assist) availability for
+ pSeries guests.
+ Possible values for the <code>state</code> attribute
+ are <code>on</code> and <code>off</code>. If the
attribute is not
+ defined, the hypervisor default will be used.
+ <span class="since">Since 5.8.0</span> (QEMU/KVM only)
+ </dd>
</dl>
<h3><a id="elementsTime">Time keeping</a></h3>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 40eb4a2d75..0d6ff86044 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -5120,6 +5120,11 @@
<optional>
<ref name="msrs"/>
</optional>
+ <optional>
+ <element name="ccf-assist">
+ <ref name="featurestate"/>
+ </element>
+ </optional>
</interleave>
</element>
</optional>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index a53cd6a725..dd2e7fb6b9 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -171,6 +171,7 @@ VIR_ENUM_IMPL(virDomainFeature,
"htm",
"nested-hv",
"msrs",
+ "ccf-assist",
);
VIR_ENUM_IMPL(virDomainCapabilitiesPolicy,
@@ -20396,6 +20397,7 @@ virDomainDefParseXML(xmlDocPtr xml,
case VIR_DOMAIN_FEATURE_HTM:
case VIR_DOMAIN_FEATURE_NESTED_HV:
+ case VIR_DOMAIN_FEATURE_CCF_ASSIST:
if (!(tmp = virXMLPropString(nodes[i], "state"))) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("missing state attribute '%s' of feature
'%s'"),
@@ -22621,6 +22623,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
case VIR_DOMAIN_FEATURE_VMCOREINFO:
case VIR_DOMAIN_FEATURE_HTM:
case VIR_DOMAIN_FEATURE_NESTED_HV:
+ case VIR_DOMAIN_FEATURE_CCF_ASSIST:
if (src->features[i] != dst->features[i]) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("State of feature '%s' differs: "
@@ -28187,6 +28190,7 @@ virDomainDefFormatFeatures(virBufferPtr buf,
case VIR_DOMAIN_FEATURE_VMPORT:
case VIR_DOMAIN_FEATURE_HTM:
case VIR_DOMAIN_FEATURE_NESTED_HV:
+ case VIR_DOMAIN_FEATURE_CCF_ASSIST:
switch ((virTristateSwitch) def->features[i]) {
case VIR_TRISTATE_SWITCH_LAST:
case VIR_TRISTATE_SWITCH_ABSENT:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 2884af49d8..e8662e4bc5 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1755,6 +1755,7 @@ typedef enum {
VIR_DOMAIN_FEATURE_HTM,
VIR_DOMAIN_FEATURE_NESTED_HV,
VIR_DOMAIN_FEATURE_MSRS,
+ VIR_DOMAIN_FEATURE_CCF_ASSIST,
VIR_DOMAIN_FEATURE_LAST
} virDomainFeature;
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index cbf25d5f07..3bd841919d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7409,6 +7409,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
virBufferAsprintf(&buf, ",cap-nested-hv=%s", str);
}
+ if (def->features[VIR_DOMAIN_FEATURE_CCF_ASSIST] != VIR_TRISTATE_SWITCH_ABSENT) {
+ const char *str;
+
+ if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("ccf-assist configuration is not supported by this
"
+ "QEMU binary"));
+ return -1;
+ }
+
+ str =
virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_CCF_ASSIST]);
+ if (!str) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Invalid setting for ccf-assist state"));
+ return -1;
+ }
+
+ virBufferAsprintf(&buf, ",cap-ccf-assist=%s", str);
+ }
+
if (cpu && cpu->model &&
cpu->mode == VIR_CPU_MODE_HOST_MODEL &&
qemuDomainIsPSeries(def) &&
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index b4175a846e..ecbe21449c 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4730,6 +4730,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def,
case VIR_DOMAIN_FEATURE_HPT:
case VIR_DOMAIN_FEATURE_HTM:
case VIR_DOMAIN_FEATURE_NESTED_HV:
+ case VIR_DOMAIN_FEATURE_CCF_ASSIST:
if (def->features[i] != VIR_TRISTATE_SWITCH_ABSENT &&
!qemuDomainIsPSeries(def)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
diff --git a/tests/qemuxml2argvdata/pseries-features.args
b/tests/qemuxml2argvdata/pseries-features.args
index 9fde54b37a..7aa357a54e 100644
--- a/tests/qemuxml2argvdata/pseries-features.args
+++ b/tests/qemuxml2argvdata/pseries-features.args
@@ -11,7 +11,7 @@ QEMU_AUDIO_DRV=none \
-name guest \
-S \
-machine pseries,accel=tcg,usb=off,dump-guest-core=off,resize-hpt=required,\
-cap-hpt-max-page-size=1048576k,cap-htm=on,cap-nested-hv=off \
+cap-hpt-max-page-size=1048576k,cap-htm=on,cap-nested-hv=off,cap-ccf-assist=on \
-m 512 \
-realtime mlock=off \
-smp 1,sockets=1,cores=1,threads=1 \
diff --git a/tests/qemuxml2argvdata/pseries-features.xml
b/tests/qemuxml2argvdata/pseries-features.xml
index 6f7d32b065..8ccc1b73d8 100644
--- a/tests/qemuxml2argvdata/pseries-features.xml
+++ b/tests/qemuxml2argvdata/pseries-features.xml
@@ -12,6 +12,7 @@
</hpt>
<htm state='on'/>
<nested-hv state='off'/>
+ <ccf-assist state='on'/>
</features>
<devices>
<emulator>/usr/bin/qemu-system-ppc64</emulator>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index db79301f0e..14a2a140dc 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1960,6 +1960,7 @@ mymain(void)
QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE,
QEMU_CAPS_MACHINE_PSERIES_CAP_HTM,
QEMU_CAPS_MACHINE_PSERIES_CAP_NESTED_HV,
+ QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST,
QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT);
DO_TEST_FAILURE("pseries-features",
QEMU_CAPS_DEVICE_SPAPR_PCI_HOST_BRIDGE);
diff --git a/tests/qemuxml2xmloutdata/pseries-features.xml
b/tests/qemuxml2xmloutdata/pseries-features.xml
index 7e12bc9c03..a5df840394 100644
--- a/tests/qemuxml2xmloutdata/pseries-features.xml
+++ b/tests/qemuxml2xmloutdata/pseries-features.xml
@@ -14,6 +14,7 @@
</hpt>
<htm state='on'/>
<nested-hv state='off'/>
+ <ccf-assist state='on'/>
</features>
<clock offset='utc'/>
<on_poweroff>destroy</on_poweroff>
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index d5c66d8791..b9364f942f 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -608,6 +608,7 @@ mymain(void)
QEMU_CAPS_MACHINE_PSERIES_CAP_HPT_MAX_PAGE_SIZE,
QEMU_CAPS_MACHINE_PSERIES_CAP_HTM,
QEMU_CAPS_MACHINE_PSERIES_CAP_NESTED_HV,
+ QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST,
QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT);
DO_TEST("pseries-serial-native",
--
2.21.0
5:06 p.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: Implement the ccf-assist pSeries feature
On 10/8/19 4:06 PM, Daniel Henrique Barboza wrote:
This patch adds the implementation of the ccf-assist pSeries
feature, based on the QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST
capability that was added in the previous patch.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
docs/formatdomain.html.in | 9 +++++++++
docs/schemas/domaincommon.rng | 5 +++++
src/conf/domain_conf.c | 4 ++++
src/conf/domain_conf.h | 1 +
src/qemu/qemu_command.c | 20 +++++++++++++++++++
src/qemu/qemu_domain.c | 1 +
tests/qemuxml2argvdata/pseries-features.args | 2 +-
tests/qemuxml2argvdata/pseries-features.xml | 1 +
tests/qemuxml2argvtest.c | 1 +
tests/qemuxml2xmloutdata/pseries-features.xml | 1 +
tests/qemuxml2xmltest.c | 1 +
11 files changed, 45 insertions(+), 1 deletion(-)
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
And pushed.
One general comment, I find it helpful to put the XML addition in the
commit message, and the cover letter. Makes it easier for reviewers, and
also easier for grepping commit logs for an XML property which is
occasionally useful.
A couple comments below though
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 647f96f651..059781a0c3 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -2059,6 +2059,7 @@
<tseg unit='MiB'>48</tseg>
</smm>
<htm state='on'/>
+ <ccf-assist state='on'/>
<msrs unknown='ignore'/>
</features>
...</pre>
@@ -2357,6 +2358,14 @@
will not be ignored.
<span class="since">Since 5.1.0</span> (bhyve only)
</dd>
+ <dt><code>ccf-assist</code></dt>
+ <dd>Configure ccf-assist (Count Cache Flush Assist) availability for
+ pSeries guests.
+ Possible values for the <code>state</code> attribute
+ are <code>on</code> and <code>off</code>. If the
attribute is not
+ defined, the hypervisor default will be used.
+ <span class="since">Since 5.8.0</span> (QEMU/KVM only)
+ </dd>
</dl>
I changed the version reference to 5.9.0 too
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index cbf25d5f07..3bd841919d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7409,6 +7409,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
virBufferAsprintf(&buf, ",cap-nested-hv=%s", str);
}
+ if (def->features[VIR_DOMAIN_FEATURE_CCF_ASSIST] != VIR_TRISTATE_SWITCH_ABSENT)
{
+ const char *str;
+
I know you were just follow the pattern of the rest of the function here
so I didn't object. But, these two error checks should not be here.
+ if (!virQEMUCapsGet(qemuCaps,
QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("ccf-assist configuration is not supported by this
"
+ "QEMU binary"));
+ return -1;
+ }
+
This is a validation check, throwing an error if an unsupported qemu
config was requested. This should be part of the
qemuDomainDefValidateFeatures in qemu_domain.c, which already has
several other feature validation checks.
Basically every qemuCaps validation check in qemu_command.c CLI building
is a candidate for moving to something triggered from qemuDomainDefValidate.
The benefits of moving to validate time is that XML is rejected by
'virsh define' rather than at 'virsh start' time. It also makes it
easier to follow the cli building code, and makes it easier to verify
qemu_command.c test suite code coverage for the important stuff like
covering every CLI option. It's also a good intermediate step for
sharing validation with domain capabilities building, like is done
presently for rng models.
The features stuff here is a good place to start. I know you've been
sending cleanup patches lately; if working on this is something you want
to do, I'm happy to provide timely reviews (that's my sales pitch :) )
+ str =
virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_CCF_ASSIST]);
+ if (!str) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+ _("Invalid setting for ccf-assist state"));
+ return -1;
+ }
+
+ virBufferAsprintf(&buf, ",cap-ccf-assist=%s", str);
+ }
+
This check isn't really required IMO. The only time we should hit !str
is if some there's some internal bug which doesn't deserve an explicit
error message. Better to just use NULLSTR(str) and let the (null) hit
qemu command line which will hopefully fail. Either way it's a bug that
shouldn't happen in practice, so the similar checks in this function can
be removed IMO
Thanks,
Cole
9:23 a.m.
New subject: [libvirt] [PATCH v2 2/3] qemu: Implement the ccf-assist pSeries feature
On 10/9/19 7:06 PM, Cole Robinson wrote:
On 10/8/19 4:06 PM, Daniel Henrique Barboza wrote:
> This patch adds the implementation of the ccf-assist pSeries
> feature, based on the QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST
> capability that was added in the previous patch.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
> ---
> docs/formatdomain.html.in | 9 +++++++++
> docs/schemas/domaincommon.rng | 5 +++++
> src/conf/domain_conf.c | 4 ++++
> src/conf/domain_conf.h | 1 +
> src/qemu/qemu_command.c | 20 +++++++++++++++++++
> src/qemu/qemu_domain.c | 1 +
> tests/qemuxml2argvdata/pseries-features.args | 2 +-
> tests/qemuxml2argvdata/pseries-features.xml | 1 +
> tests/qemuxml2argvtest.c | 1 +
> tests/qemuxml2xmloutdata/pseries-features.xml | 1 +
> tests/qemuxml2xmltest.c | 1 +
> 11 files changed, 45 insertions(+), 1 deletion(-)
>
Reviewed-by: Cole Robinson <crobinso(a)redhat.com>
And pushed.
One general comment, I find it helpful to put the XML addition in the
commit message, and the cover letter. Makes it easier for reviewers,
and also easier for grepping commit logs for an XML property which is
occasionally useful.
A couple comments below though
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 647f96f651..059781a0c3 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2059,6 +2059,7 @@
> <tseg unit='MiB'>48</tseg>
> </smm>
> <htm state='on'/>
> + <ccf-assist state='on'/>
> <msrs unknown='ignore'/>
> </features>
> ...</pre>
> @@ -2357,6 +2358,14 @@
> will not be ignored.
> <span class="since">Since 5.1.0</span> (bhyve
only)
> </dd>
> + <dt><code>ccf-assist</code></dt>
> + <dd>Configure ccf-assist (Count Cache Flush Assist)
> availability for
> + pSeries guests.
> + Possible values for the <code>state</code> attribute
> + are <code>on</code> and <code>off</code>. If the
attribute
> is not
> + defined, the hypervisor default will be used.
> + <span class="since">Since 5.8.0</span> (QEMU/KVM
only)
> + </dd>
> </dl>
I changed the version reference to 5.9.0 too
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index cbf25d5f07..3bd841919d 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7409,6 +7409,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
> virBufferAsprintf(&buf, ",cap-nested-hv=%s", str);
> }
> + if (def->features[VIR_DOMAIN_FEATURE_CCF_ASSIST] !=
> VIR_TRISTATE_SWITCH_ABSENT) {
> + const char *str;
> +
I know you were just follow the pattern of the rest of the function
here so I didn't object. But, these two error checks should not be here.
> + if (!virQEMUCapsGet(qemuCaps,
> QEMU_CAPS_MACHINE_PSERIES_CAP_CCF_ASSIST)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("ccf-assist configuration is not
> supported by this "
> + "QEMU binary"));
> + return -1;
> + }
> +
This is a validation check, throwing an error if an unsupported qemu
config was requested. This should be part of the
qemuDomainDefValidateFeatures in qemu_domain.c, which already has
several other feature validation checks.
Basically every qemuCaps validation check in qemu_command.c CLI
building is a candidate for moving to something triggered from
qemuDomainDefValidate.
The benefits of moving to validate time is that XML is rejected by
'virsh define' rather than at 'virsh start' time. It also makes it
easier to follow the cli building code, and makes it easier to verify
qemu_command.c test suite code coverage for the important stuff like
covering every CLI option. It's also a good intermediate step for
sharing validation with domain capabilities building, like is done
presently for rng models.
The features stuff here is a good place to start. I know you've been
sending cleanup patches lately; if working on this is something you
want to do, I'm happy to provide timely reviews (that's my sales pitch
:) )
hehehe yeah, I use these cleanups as an opportunity to try to learn more
about the
code and so on.
I'll follow your suggestion and send patches to move these verifications
from
qemu_command.c to qemu_domain:qemuDomainDefValidate, hopefully
soon(C).
Thanks,
DHB
> + str =
> virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_CCF_ASSIST]);
> + if (!str) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("Invalid setting for ccf-assist state"));
> + return -1;
> + }
> +
> + virBufferAsprintf(&buf, ",cap-ccf-assist=%s", str);
> + }
> +
This check isn't really required IMO. The only time we should hit !str
is if some there's some internal bug which doesn't deserve an explicit
error message. Better to just use NULLSTR(str) and let the (null) hit
qemu command line which will hopefully fail. Either way it's a bug
that shouldn't happen in practice, so the similar checks in this
function can be removed IMO
Thanks,
Cole
3:06 p.m.
New subject: [libvirt] [PATCH v2 3/3] news: Update for the ccf-assist pSeries feature
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
docs/news.xml | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/docs/news.xml b/docs/news.xml
index d9c402e6bb..b8002368ae 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -41,6 +41,15 @@
<libvirt>
<release version="v5.9.0" date="unreleased">
<section title="New features">
+ <change>
+ <summary>
+ qemu: Implement the ccf-assist pSeries feature
+ </summary>
+ <description>
+ Users can now decide whether ccf-assist (Count Cache Flush Assist)
+ support should be available to pSeries guests.
+ </description>
+ </change>
</section>
<section title="Improvements">
</section>
--
2.21.0
2038
days inactive
2040
days old
5 comments
2 participants
participants (2)
-
Cole Robinson -
Daniel Henrique Barboza