On 03/23/2017 11:26 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
Shouldn't this just go on the "last" patch in the series. IDC really,
but if you're going to add to all patches, then all patches should have
it listed.
---
docs/formatdomain.html.in | 10 +++++++
docs/schemas/domaincommon.rng | 16 ++++++++++
src/conf/domain_conf.c | 35 +++++++++++++++++++++-
src/conf/domain_conf.h | 13 ++++++++
.../qemuxml2argv-intel-iommu-irqchip.xml | 29 ++++++++++++++++++
.../qemuxml2xmlout-intel-iommu-irqchip.xml | 29 ++++++++++++++++++
tests/qemuxml2xmltest.c | 2 ++
7 files changed, 133 insertions(+), 1 deletion(-)
create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml
create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 4a3123e..32a5f18 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -1627,6 +1627,7 @@
</kvm>
<pvspinlock state='on'/>
<gic version='2'/>
+ <irqchip mode='split'/>
</features>
...</pre>
@@ -1788,6 +1789,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>
Did you somehow know this wouldn't be reviewed in time for 3.2.0? ;-)
Once I got to patch 4 & 7, I began to wonder if support there was
dependence upon the value being "split" or is "on" also acceptable.
It
would seem "off" and not present wouldn't allow intremap or caching to
work.
At the very least whatever it is that allows the -device intel-iommu to
be present in QEMU 2.7 (if I read virQEMUCapsInitQMPMonitor correctly)
instead of ",iommu=on" would also seem to be a requirement. I know we
don't want to put versions there, but whatever it is that is required
should be listed (at least while it's still fresh in your mind).
</dl>
<h3><a name="elementsTime">Time keeping</a></h3>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index fbedc9b..8ba3902 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -4489,6 +4489,9 @@
</optional>
</element>
</optional>
+ <optional>
+ <ref name="irqchip"/>
+ </optional>
</interleave>
</element>
</optional>
@@ -4664,6 +4667,19 @@
</element>
</define>
+ <define name="irqchip">
+ <element name="irqchip">
+ <attribute name="mode">
+ <choice>
should there be a "default" (or undefined - see below) to match the enum
impl list? or just left empty since there's a features tristate set on
parse (and should be used to control whether it gets formatted).
+ <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 6bbc6a2..ffc6a68 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -139,7 +139,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",
@@ -855,6 +856,13 @@ VIR_ENUM_IMPL(virDomainLoader,
"rom",
"pflash")
+VIR_ENUM_IMPL(virDomainIRQChip,
+ VIR_DOMAIN_IRQCHIP_LAST,
+ "",
Should this be "undefined"? Or just go away since there's a tristate to
manage the parse and (not yet) format.
+ "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)
@@ -17278,6 +17286,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;
@@ -24337,6 +24363,13 @@ virDomainDefFormatInternal(virDomainDefPtr def,
}
break;
+ case VIR_DOMAIN_FEATURE_IRQCHIP:
This is where I would expect to see a check like other features for "if
(def->features[i] != VIR_TRISTATE_SWITCH_ABSENT)" (or == ON) since you
set def->features[val] = VIR_TRISTATE_SWITCH_ON; above.
Using the tristate removes the need to have that default/undefined or ""
value.
+ if (def->irqchip) {
+ 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 8e6d874..762e64e 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1662,6 +1662,7 @@ typedef enum {
VIR_DOMAIN_FEATURE_VMPORT,
VIR_DOMAIN_FEATURE_GIC,
VIR_DOMAIN_FEATURE_SMM,
+ VIR_DOMAIN_FEATURE_IRQCHIP,
VIR_DOMAIN_FEATURE_LAST
} virDomainFeature;
@@ -1801,6 +1802,17 @@ struct _virDomainLoaderDef {
void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
+typedef enum {
+ VIR_DOMAIN_IRQCHIP_DEFAULT = 0,
I think your description would make this UNDEFINED... Of course the
tristate setting makes this value unnecessary.
+ VIR_DOMAIN_IRQCHIP_OFF,
+ 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;
@@ -2250,6 +2262,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..98e4bba
--- /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</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 100644
index 0000000..98e4bba
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-intel-iommu-irqchip.xml
Since there's no difference between this and
tests/qemuxml2argvdata/qemuxml2argv-intel-iommu-irqchip.xml this should
thus follow the example of other outdata files using a link to the
argvdata file (the gic files provide examples.
Impacts subsequent patches that need to change both files.
@@ -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</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/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 266b9c0..8afd830 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1124,6 +1124,8 @@ mymain(void)
DO_TEST("intel-iommu-machine",
QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACHINE_IOMMU);
+ DO_TEST("intel-iommu-irqchip",
+ QEMU_CAPS_DEVICE_INTEL_IOMMU);
Should this also set QEMU_CAPS_MACHINE_OPT and
QEMU_CAPS_DEVICE_INTEL_IOMMU too? Although I suppose it only the caps
only matter on xml2argv...
DO_TEST("cpu-check-none", NONE);
DO_TEST("cpu-check-partial", NONE);
Conditional ACK w/ suggested adjustments - I'm open to keeping the
"undefined" entry, but I think it should be removed.
John