[libvirt] [PATCH 4/9] conf: add <driver intremap> to <iommu>
John Ferlan
jferlan at redhat.com
Tue Mar 28 18:54:23 UTC 2017
On 03/23/2017 11:26 AM, Ján Tomko wrote:
> Add a new attribute to control interrupt remapping.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1427005
> ---
> docs/formatdomain.html.in | 22 ++++++++++++-
> docs/schemas/domaincommon.rng | 9 +++++
> src/conf/domain_conf.c | 38 +++++++++++++++++++---
> src/conf/domain_conf.h | 1 +
> .../qemuxml2argv-intel-iommu-irqchip.xml | 4 ++-
> .../qemuxml2xmlout-intel-iommu-irqchip.xml | 4 ++-
> 6 files changed, 71 insertions(+), 7 deletions(-)
>
awful name "intremap", but just following QEMU...
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 32a5f18..6b196d4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7285,7 +7285,9 @@ qemu-kvm -net nic,model=? /dev/null
> <pre>
> ...
> <devices>
> - <iommu model='intel'/>
> + <iommu model='intel'>
> + <driver intremap='on'/>
> + </iommu>
> </devices>
> ...
> </pre>
> @@ -7296,6 +7298,24 @@ qemu-kvm -net nic,model=? /dev/null
> Currently only the <code>intel</code> model is supported.
> </p>
> </dd>
> + <dt><code>driver</code></dt>
> + <dd>
> + <p>
> + The <code>driver</code> subelement can be used to configure
> + additional options:
> + </p>
> + <dl>
> + <dt><code>intremap</code></dt>
> + <dd>
> + <p>
> + The <code>intremap</code> attribute with possible values
> + <code>on</code> and <code>off</code> can be used to
> + turn on interrupt remapping. <span class="since">Since 3.3.0</span>
> + (QEMU only)
> + </p>
> + </dd>
> + </dl>
> + </dd>
If I read patch1 correctly, support of this feature is dependent upon
the 'irqchip' feature being set to (at least, it seems) "split".
Whether "on" would be valid or not is not clear.
In any case, since there is a dependency, it should be listed here and a
doc link/reference to the irqchip feature description?
Since patch7 only adds support for this when there's a "-device
intel-iommu" and not when there's ",iommu=on", there's also another
dependency that needs to be described.
> </dl>
>
> <h3><a name="seclabel">Security label</a></h3>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 8ba3902..68f3680 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -3889,6 +3889,15 @@
> <attribute name="model">
> <value>intel</value>
> </attribute>
> + <optional>
> + <element name="driver">
> + <optional>
> + <attribute name="intremap">
> + <ref name="virOnOff"/>
> + </attribute>
> + </optional>
> + </element>
> + </optional>
> </element>
> </define>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 1245fdd..da0c9f0 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13909,12 +13909,16 @@ virDomainMemoryDefParseXML(xmlNodePtr memdevNode,
>
>
> static virDomainIOMMUDefPtr
> -virDomainIOMMUDefParseXML(xmlNodePtr node)
> +virDomainIOMMUDefParseXML(xmlNodePtr node,
> + xmlXPathContextPtr ctxt)
> {
> virDomainIOMMUDefPtr iommu = NULL, ret = NULL;
> + xmlNodePtr save = ctxt->node;
> char *tmp = NULL;
> int val;
>
> + ctxt->node = node;
> +
> if (VIR_ALLOC(iommu) < 0)
> goto cleanup;
>
> @@ -13931,10 +13935,20 @@ virDomainIOMMUDefParseXML(xmlNodePtr node)
>
> iommu->model = val;
>
> + VIR_FREE(tmp);
> + if ((tmp = virXPathString("string(./driver/@intremap)", ctxt))) {
> + if ((val = virTristateSwitchTypeFromString(tmp)) < 0) {
> + virReportError(VIR_ERR_XML_ERROR, _("unknown intremap value: %s"), tmp);
> + goto cleanup;
> + }
> + iommu->intremap = val;
> + }
> +
> ret = iommu;
> iommu = NULL;
>
> cleanup:
> + ctxt->node = save;
> VIR_FREE(iommu);
> VIR_FREE(tmp);
> return ret;
> @@ -14087,7 +14101,7 @@ virDomainDeviceDefParse(const char *xmlStr,
> goto error;
> break;
> case VIR_DOMAIN_DEVICE_IOMMU:
> - if (!(dev->data.iommu = virDomainIOMMUDefParseXML(node)))
> + if (!(dev->data.iommu = virDomainIOMMUDefParseXML(node, ctxt)))
> goto error;
> break;
> case VIR_DOMAIN_DEVICE_NONE:
> @@ -18202,7 +18216,7 @@ virDomainDefParseXML(xmlDocPtr xml,
> }
>
> if (n > 0) {
> - if (!(def->iommu = virDomainIOMMUDefParseXML(nodes[0])))
> + if (!(def->iommu = virDomainIOMMUDefParseXML(nodes[0], ctxt)))
> goto error;
> }
> VIR_FREE(nodes);
> @@ -23846,8 +23860,24 @@ static void
> virDomainIOMMUDefFormat(virBufferPtr buf,
> virDomainIOMMUDefPtr iommu)
> {
> - virBufferAsprintf(buf, "<iommu model='%s'/>\n",
> + virBuffer childBuf = VIR_BUFFER_INITIALIZER;
> +
> + virBufferAdjustIndent(&childBuf, virBufferGetIndent(buf, false) + 2);
> +
> + if (iommu->intremap) {
Since intremap is not a boolean or a pointer and while it may seem
superfluous ... I'd like to see != VIR_TRISTATE_SWITCH_ABSENT to make it
more obvious of the comparison.
> + virBufferAsprintf(&childBuf, "<driver intremap='%s'/>\n",
> + virTristateSwitchTypeToString(iommu->intremap));
> + }
> +
> + virBufferAsprintf(buf, "<iommu model='%s'",
> virDomainIOMMUModelTypeToString(iommu->model));
> + if (virBufferUse(&childBuf)) {
> + virBufferAddLit(buf, ">\n");
> + virBufferAddBuffer(buf, &childBuf);
> + virBufferAddLit(buf, "</iommu>\n");
> + } else {
> + virBufferAddLit(buf, "/>\n");
> + }
> }
>
>
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 762e64e..ad8ae2d 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -2203,6 +2203,7 @@ typedef enum {
>
> struct _virDomainIOMMUDef {
> virDomainIOMMUModel model;
> + virTristateSwitch intremap;
> };
> /*
> * Guest VM main configuration
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml
> index 98e4bba..3cef53d 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml
> @@ -24,6 +24,8 @@
> <input type='mouse' bus='ps2'/>
> <input type='keyboard' bus='ps2'/>
> <memballoon model='none'/>
> - <iommu model='intel'/>
> + <iommu model='intel'>
> + <driver intremap='on'/>
> + </iommu>
> </devices>
> </domain>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml
> index 98e4bba..3cef53d 100644
> --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml
> @@ -24,6 +24,8 @@
> <input type='mouse' bus='ps2'/>
> <input type='keyboard' bus='ps2'/>
> <memballoon model='none'/>
> - <iommu model='intel'/>
> + <iommu model='intel'>
> + <driver intremap='on'/>
> + </iommu>
> </devices>
> </domain>
>
FWIW: could end up with a merge conflict if you use the link from
outdata to argvdata, but at least you don't change in two places.
ACK w/ updated docs,
John
More information about the libvir-list
mailing list