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

[Andrea Bolognani]

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.

-- 
Andrea Bolognani / Red Hat / Virtualization