On 05/25/2018 12:39 PM, Filip Alac wrote:

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=967231

Signed-off-by: Filip Alac <filipalac@gmail.com> --- docs/schemas/capability.rng | 11 +++++++++++ src/conf/capabilities.c | 8 ++++++++ src/conf/capabilities.h | 5 +++++ src/libvirt_private.syms | 1 + src/qemu/qemu_capabilities.c | 4 ++++ src/util/virpci.c | 19 +++++++++++++++++++ src/util/virpci.h | 2 ++ 7 files changed, 50 insertions(+)

diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng index e1ab5c224..7e7d8fbc7 100644 --- a/docs/schemas/capability.rng +++ b/docs/schemas/capability.rng @@ -39,6 +39,9 @@ <optional> <ref name='power_management'/> </optional> + <optional> + <ref name='iommu_support'/> + </optional> <optional> <ref name='migration'/> </optional> @@ -148,6 +151,14 @@ </element> </define>

+ <define name='iommu_support'> + <optional> + <attribute name='support'> + <ref name='virYesNo'/> + </attribute> + </optional> + </define>

This does not make much sense. This defines an optional attribute 'support' but does not say to which element.

+ <define name='migration'> <element name='migration_features'> <optional> diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index dd2fc77f9..eb387916f 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1020,6 +1020,8 @@ virCapabilitiesFormatXML(virCapsPtr caps) } virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</power_management>\n"); + virBufferAsprintf(&buf, "<iommu support='%s'/>\n", + caps->host.iommu ? "yes" : "no");

This doesn't make much sense. This block starts with: if (caps->host.powerMgmt) { So you only print the <iommu/> iff power mgmt is available. These are orthogonal features and have nothing in common.

} else { /* The host does not support any PM feature. */ virBufferAddLit(&buf, "<power_management/>\n"); @@ -1743,3 +1745,9 @@ virCapabilitiesInitCaches(virCapsPtr caps) virBitmapFree(cpus); return ret; } + +int +virCapabilitiesHostInitIOMMU(virCapsPtr caps) +{ + return caps->host.iommu = virPCIHasIOMMU(); +}

Whoa, this is very unsafe. I don't quite see why virCapabilitiesHostInitIOMMU() should fail when host doesn't have IOMMU. In fact, it's quite the opposite because false is 0 and true is 1.

diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index f0a06a24d..4d41363a3 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -183,6 +183,7 @@ struct _virCapsHost { int nPagesSize; /* size of pagesSize array */ unsigned int *pagesSize; /* page sizes support on the system */ unsigned char host_uuid[VIR_UUID_BUFLEN]; + int iommu;

You declare this int even though you use it only to store a boolean.

};

typedef int (*virDomainDefNamespaceParse)(xmlDocPtr, xmlNodePtr, @@ -327,4 +328,8 @@ void virCapsHostCacheBankFree(virCapsHostCacheBankPtr ptr);

int virCapabilitiesInitCaches(virCapsPtr caps);

+int virCapabilitiesInitCaches(virCapsPtr caps);

No need to copy this line ;-)

+ +int virCapabilitiesHostInitIOMMU(virCapsPtr caps); + #endif /* __VIR_CAPABILITIES_H */ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a97b7fe22..258d02962 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -58,6 +58,7 @@ virCapabilitiesFreeMachines; virCapabilitiesFreeNUMAInfo; virCapabilitiesGetCpusForNodemask; virCapabilitiesGetNodeInfo; +virCapabilitiesHostInitIOMMU; virCapabilitiesHostSecModelAddBaseLabel; virCapabilitiesInitCaches; virCapabilitiesInitNUMA;

You should expose virPCIHasIOMMU too. After all, you're exposing it in header file.

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 8a63db5f4..552d5452d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -948,6 +948,10 @@ virQEMUCapsInit(virFileCachePtr cache) if (virCapabilitiesInitPages(caps) < 0) VIR_WARN("Failed to get pages info");

+ /* Add IOMMU info */ + if (virCapabilitiesHostInitIOMMU(caps) < 0) + VIR_WARN("Failed to get iommmu info"); +

In the XML you placed <iommu/> between <power_management/> and <migration_features/>. Might be worth honouring that order here too.

/* Add domain migration transport URIs */ virCapabilitiesAddHostMigrateTransport(caps, "tcp"); virCapabilitiesAddHostMigrateTransport(caps, "rdma"); diff --git a/src/util/virpci.c b/src/util/virpci.c index 8d0236666..c88b13c97 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -3288,3 +3288,22 @@ virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev) VIR_FREE(dev->link_sta); VIR_FREE(dev); } +

See how the rest of functions in the file is separated by two empty lines? Might be worth preserving that.

+bool +virPCIHasIOMMU(void) +{ + struct stat sb; + + /* We can only check on newer kernels with iommu groups & vfio */ + if (stat("/sys/kernel/iommu_groups", &sb) < 0) + return false; + + if (!S_ISDIR(sb.st_mode)) + return false; + + /* Check if folder is empty */ + if (sb.st_nlink <= 2) + return false; + + return true; +}

This is similar to qemuHostdevHostSupportsPassthroughVFIO() which got me thinking, do we even need this patch? I mean, we already kind of expose if host is VFIO capable: /domainCapabilities/hostdev/enum[name='pciBackend'/value/vfio. Michal

On 05/25/2018 12:39 PM, Filip Alac wrote:

Signed-off-by: Filip Alac <filipalac@gmail.com> --- docs/news.xml | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml index 693d4a373..babf13379 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -93,6 +93,14 @@ secret-event, pool-event and nodedev-event) </description> </change> + <change> + <summary> + capabilities: Provide info about host IOMMU support + </summary> + <description> + Capabilities XML now provide information about host IOMMU support. + </description> + </change> </section> <section title="Bug fixes"> </section>

This looks okay. Michal