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

[John Ferlan <jferlan@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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.

[...]


> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 0d68596991..73db315c4f 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1929,6 +1929,7 @@
>    &lt;smm state='on'&gt;
>      &lt;tseg unit='MiB'&gt;48&lt;/tseg&gt;
>    &lt;/smm&gt;
> +  &lt;htm state='on'/&gt;
>  &lt;/features&gt;
>  ...</pre>
>  
> @@ -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

> +      </dd>
>      </dl>
>  
>      <h3><a id="elementsTime">Time keeping</a></h3>

[...]

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d8cb7f37f3..ea01d08a3c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -152,6 +152,7 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST,
>                "ioapic",
>                "hpt",
>                "vmcoreinfo",
> +              "htm",
>  );
>  
>  VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST,
> @@ -19827,6 +19828,22 @@ virDomainDefParseXML(xmlDocPtr xml,
>              }
>              break;
>  
> +        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...

> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("unknown state attribute '%s' of feature '%s'"),
> +                               tmp, virDomainFeatureTypeToString(val));
> +                goto error;
> +            }
> +            VIR_FREE(tmp);
> +            break;
> +

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index a357d2199c..5342c08f19 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -7291,6 +7291,26 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
>          }
>      }
>  
> +    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.


> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("Invalid setting for HTM state"));
> +            goto cleanup;
> +        }
> +
> +        virBufferAsprintf(&buf, ",cap-htm=%s", str);

Just remove @str, and directly call/use:

virTristateSwitchTypeToString(def->features[VIR_DOMAIN_FEATURE_HTM])

> +    }
> +
>      if (cpu && cpu->model &&
>          cpu->mode == VIR_CPU_MODE_HOST_MODEL &&
>          qemuDomainIsPSeries(def) &&

[...]

with the adjustments and splitting into two patches (I trust you have
that capability),

Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>

John

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