[libvirt] [PATCH v3 2/3] qemu: Implement the HTM pSeries feature

[John Ferlan]

On 07/02/2018 04:19 AM, Andrea Bolognani wrote:
> On Sat, 2018-06-30 at 07:23 -0400, John Ferlan wrote:
>> On 06/26/2018 06:21 AM, Andrea Bolognani wrote:
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1525599
>>> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
>>> ---
>>>  docs/formatdomain.html.in                     |  8 ++++++++
>>>  docs/schemas/domaincommon.rng                 |  5 +++++
>>>  src/conf/domain_conf.c                        | 19 ++++++++++++++++++
>>>  src/conf/domain_conf.h                        |  1 +
>>>  src/qemu/qemu_command.c                       | 20 +++++++++++++++++++
>>>  src/qemu/qemu_domain.c                        | 13 ++++++++++++
>>>  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, 71 insertions(+), 1 deletion(-)
>>
>> Even though it's QEMU/KVM only - the conf/docs/xml2xml should be
>> separated from the qemu/xml2argv changes.
> 
> Can do.
> 
>>> @@ -2162,6 +2163,13 @@
>>>        <dd>Enable QEMU vmcoreinfo device to let the guest kernel save debug
>>>            details. <span class="since">Since 4.4.0</span> (QEMU only)
>>>        </dd>
>>> +      <dt><code>htm</code></dt>
>>> +      <dd>Configure HTM (Hardware Transational Memory) 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 4.5.0</span> (QEMU/KVM only)
>>
>> This'll miss 4.5, so change to 4.6 before pushing
> 
> Sure.
> 
>>> +        case VIR_DOMAIN_FEATURE_HTM:
>>> +            if (!(tmp = virXMLPropString(nodes[i], "state"))) {
>>> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>>> +                               _("missing state attribute '%s' of feature '%s'"),
>>> +                               tmp, virDomainFeatureTypeToString(val));
>>> +                goto error;
>>> +            }
>>> +            if ((def->features[val] = virTristateSwitchTypeFromString(tmp)) < 0) {
>>
>> [1] checked here, so that...
>>
>>> +    if (def->features[VIR_DOMAIN_FEATURE_HTM] != VIR_TRISTATE_SWITCH_ABSENT) {
>>> +        const char *str;
>>> +
>>> +        if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_CAP_HTM)) {
>>> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>>> +                           _("HTM configuration is not supported by this "
>>> +                             "QEMU binary"));
>>> +            goto cleanup;
>>> +        }
>>> +
>>> +        str = virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM]);
>>> +        if (!str) {
>>
>> [1] ... this is unnecessary due to virDomainDefParseXML check.
> 
> I strongly dislike the practice of skipping checks in a function
> with the rationale that they're already performed somewhere else in
> the code base: libvirt is in a state of constant flux, and as stuff
> gets moved around you might very well find yourself no longer as
> shielded from invalid values as you thought you were. Local sanity
> checks should IMHO always be performed locally.
> 
> tl;dr I'd really rather keep this in.
> 

That's fine - I don't mind the check, just pointing out it should be
unnecessary. You can leave it in and the R-by still stands.

John

htm vs. hpt, yep the tla's and seeing pseries with memory related
functionality for both certainly led me down that path...