[libvirt] [PATCHv3 1/6] conf: add <irqchip mode> to <features>
John Ferlan
jferlan at redhat.com
Fri Apr 28 17:26:12 UTC 2017
On 04/26/2017 04:29 AM, Ján Tomko wrote:
> Add a new <irqchip> element with a mode attribute.
>
> Possible values are off, split or on.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1427005
> ---
> docs/formatdomain.html.in | 10 +++++++
> docs/schemas/domaincommon.rng | 16 ++++++++++
> src/conf/domain_conf.c | 34 +++++++++++++++++++++-
> src/conf/domain_conf.h | 12 ++++++++
> .../qemuxml2argv-intel-iommu-irqchip.xml | 29 ++++++++++++++++++
> .../qemuxml2xmlout-intel-iommu-irqchip.xml | 1 +
> tests/qemuxml2xmltest.c | 1 +
> 7 files changed, 102 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml
> create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index e31a271..4b92a0f 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -1643,6 +1643,7 @@
> </kvm>
> <pvspinlock state='on'/>
> <gic version='2'/>
> + <irqchip mode='split'/>
>
> </features>
> ...</pre>
> @@ -1804,6 +1805,15 @@
> for hypervisor to decide.
> <span class="since">Since 2.1.0</span>
> </dd>
> + <dt><code>irqchip</code></dt>
> + <dd>Tune the in-kernel irqchip. Possible values for the
> + <code>mode</code> attribute are:
> + <code>on</code>, <code>split</code> and <code>off</code>.
> + <code>split</code> is useful for using interrupt remapping
> + with the <a href="#elementsIommu">IOMMU device</a>.
> + The default is left for hypervisor to decide.
> + <span class="since">Since 3.3.0</span> (QEMU only)
> + </dd>
> </dl>
>
You asked in your response to my comments from the v2 posting "What
would you say?"
Well, that's what I was expecting someone to tell me. If I'm reading the
documentation and see this, I might wonder whether I need this and what
it's used for. What does on, split, and off actually do. Reading
literally just what you have here makes it seem more related to IOMMU.
Hence why I asked. I'm certainly "hardware terminology wording
challenged" - glad that Peter/Paolo provided some insights.
After thinking about it a bit, I think no matter what we end up calling
it as long as we can inform the reader/consumer of our documentation
that this relates to the QEMU 'kernel_irqchip' setting which is an
architecture and machine model specific setting to determine where to
handle interrupts. (that's about as generic as comes to mind). That
should give enough context to have the curious user to search on
"kernel_irqchip"...
I understand there's concern about being too specific, but since you're
indicating this is a QEMU only feature - I think some QEMU specifics
could be added.
What's still not clear to me is how this setting affects IOMMU. I'm
"assuming" that "up to this point" there isn't an impact. But I do see
code that specifically checks if interrupt remapping is enabled, then
irqchip must be off or split. Something that could be added with the
docs when patch 5/6 are added.
Hopefully this helps. I'm fine with the code. If you change the name of
the feature to attempt to be more generic - that's fine.
John
> <h3><a name="elementsTime">Time keeping</a></h3>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index eb4b0f7..7772a91 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -4514,6 +4514,9 @@
> </optional>
> </element>
> </optional>
> + <optional>
> + <ref name="irqchip"/>
> + </optional>
> </interleave>
> </element>
> </optional>
> @@ -4689,6 +4692,19 @@
> </element>
> </define>
>
> + <define name="irqchip">
> + <element name="irqchip">
> + <attribute name="mode">
> + <choice>
> + <value>off</value>
> + <value>split</value>
> + <value>on</value>
> + </choice>
> + </attribute>
> + <empty/>
> + </element>
> + </define>
> +
> <define name="address">
> <element name="address">
> <choice>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index fe9b7c7..73fa268 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -140,7 +140,8 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST,
> "pmu",
> "vmport",
> "gic",
> - "smm")
> + "smm",
> + "irqchip")
>
> VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST,
> "default",
> @@ -857,6 +858,12 @@ VIR_ENUM_IMPL(virDomainLoader,
> "rom",
> "pflash")
>
> +VIR_ENUM_IMPL(virDomainIRQChip,
> + VIR_DOMAIN_IRQCHIP_LAST,
> + "off",
> + "split",
> + "on")
> +
> /* Internal mapping: subset of block job types that can be present in
> * <mirror> XML (remaining types are not two-phase). */
> VIR_ENUM_DECL(virDomainBlockJob)
> @@ -17521,6 +17528,24 @@ virDomainDefParseXML(xmlDocPtr xml,
> ctxt->node = node;
> break;
>
> + case VIR_DOMAIN_FEATURE_IRQCHIP:
> + node = ctxt->node;
> + ctxt->node = nodes[i];
> + tmp = virXPathString("string(./@mode)", ctxt);
> + if (tmp) {
> + int value = virDomainIRQChipTypeFromString(tmp);
> + if (value < 0) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("Unknown irqchip mode: %s"),
> + tmp);
> + goto error;
> + }
> + def->irqchip = value;
> + def->features[val] = VIR_TRISTATE_SWITCH_ON;
> + }
> + ctxt->node = node;
> + break;
> +
> /* coverity[dead_error_begin] */
> case VIR_DOMAIN_FEATURE_LAST:
> break;
> @@ -24601,6 +24626,13 @@ virDomainDefFormatInternal(virDomainDefPtr def,
> }
> break;
>
> + case VIR_DOMAIN_FEATURE_IRQCHIP:
> + if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
> + virBufferAsprintf(buf, "<irqchip mode='%s'/>\n",
> + virDomainIRQChipTypeToString(def->irqchip));
> + }
> + break;
> +
> /* coverity[dead_error_begin] */
> case VIR_DOMAIN_FEATURE_LAST:
> break;
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 3b6b174..61af012 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1673,6 +1673,7 @@ typedef enum {
> VIR_DOMAIN_FEATURE_VMPORT,
> VIR_DOMAIN_FEATURE_GIC,
> VIR_DOMAIN_FEATURE_SMM,
> + VIR_DOMAIN_FEATURE_IRQCHIP,
>
> VIR_DOMAIN_FEATURE_LAST
> } virDomainFeature;
> @@ -1812,6 +1813,16 @@ struct _virDomainLoaderDef {
>
> void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
>
> +typedef enum {
> + VIR_DOMAIN_IRQCHIP_OFF = 0,
> + VIR_DOMAIN_IRQCHIP_SPLIT,
> + VIR_DOMAIN_IRQCHIP_ON,
> +
> + VIR_DOMAIN_IRQCHIP_LAST
> +} virDomainIRQChip;
> +
> +VIR_ENUM_DECL(virDomainIRQChip);
> +
> /* Operating system configuration data & machine / arch */
> typedef struct _virDomainOSDef virDomainOSDef;
> typedef virDomainOSDef *virDomainOSDefPtr;
> @@ -2261,6 +2272,7 @@ struct _virDomainDef {
> unsigned int hyperv_spinlocks;
> virGICVersion gic_version;
> char *hyperv_vendor_id;
> + virDomainIRQChip irqchip;
>
> /* These options are of type virTristateSwitch: ON = keep, OFF = drop */
> int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml
> new file mode 100644
> index 0000000..cc895af
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml
> @@ -0,0 +1,29 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219100</memory>
> + <currentMemory unit='KiB'>219100</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='x86_64' machine='q35'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <features>
> + <irqchip mode='split'/>
> + </features>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-x86_64</emulator>
> + <controller type='pci' index='0' model='pcie-root'/>
> + <controller type='sata' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x1f' function='0x2'/>
> + </controller>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='none'/>
> + <iommu model='intel'/>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml
> new file mode 120000
> index 0000000..58a0199
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml
> @@ -0,0 +1 @@
> +../qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml
> \ No newline at end of file
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 2dccde7..40425f5 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -1122,6 +1122,7 @@ mymain(void)
> DO_TEST("intel-iommu-machine",
> QEMU_CAPS_MACHINE_OPT,
> QEMU_CAPS_MACHINE_IOMMU);
> + DO_TEST("intel-iommu-irqchip", NONE);
>
> DO_TEST("cpu-check-none", NONE);
> DO_TEST("cpu-check-partial", NONE);
>
More information about the libvir-list
mailing list