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 -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
