[libvirt] [PATCH v2 2/3] qemu: Implement the ccf-assist pSeries feature
Daniel Henrique Barboza
danielhb413 at gmail.com
Thu Oct 10 14:23:08 UTC 2019
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 at 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 at 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
More information about the libvir-list
mailing list