Re: [libvirt] [PATCH v2 2/3] qemu: Implement the ccf-assist pSeries feature

[Cole Robinson]

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 <danielhb...@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 <crobi...@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 @@
      &lt;tseg unit='MiB'&gt;48&lt;/tseg&gt;
    &lt;/smm&gt;
    &lt;htm state='on'/&gt;
+  &lt;ccf-assist state='on'/&gt;
    &lt;msrs unknown='ignore'/&gt;
  &lt;/features&gt;
  ...</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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list