[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