[cc-ing Laine and Andrea if they have a better memory of the time we went from "legacy" passthrough to vfio] On a Monday in 2025, Nathan Chen via Devel wrote:

Implement a new iommufd attribute under hostdevs' PCI subsystem driver that can be used to specify associated iommufd object when launching a qemu VM.

This does not specify which iommufd object it is, just to use the default one. It's perfect for now, we might need a different element if using anything else than iommufd0 starts making sense. Also, I think it should fine not to expose the object in the XML since it has configurable attributes now: # qemu-system-x86_64 -object iommufd,? iommufd options: fd=<string>

Signed-off-by: Nathan Chen <nathanc@nvidia.com> --- docs/formatdomain.rst | 8 ++++++++ src/conf/device_conf.c | 9 +++++++++ src/conf/device_conf.h | 1 + src/conf/schemas/basictypes.rng | 5 +++++ src/qemu/qemu_command.c | 19 +++++++++++++++++++ 5 files changed, 42 insertions(+)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 34dc9c3af7..a5c69dbcf4 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -4845,6 +4845,7 @@ or: device; if PCI ROM loading is disabled through this attribute, attempts to tweak the loading process further using the ``bar`` or ``file`` attributes will be rejected. :since:`Since 4.3.0 (QEMU and KVM only)`. + ``address`` The ``address`` element for USB devices has a ``bus`` and ``device`` attribute to specify the USB bus and device number the device appears at on @@ -4885,6 +4886,13 @@ or: found is "problematic" in some way, the generic vfio-pci driver similarly be forced.

+ The ``<driver>`` element's ``iommufd`` attribute is used to specify + using the iommufd interface to propagate DMA mappings to the kernel, + instead of legacy VFIO. When the attribute is present, an iommufd + object will be created by the resulting qemu command. Libvirt will + open the /dev/iommu and VFIO device cdev, passing the associated + file descriptor numbers to the qemu command. +

Should we resurrect the old attribute and use: <driver name="iommufd"/> The idea being that later in time, when it will no longer make sense to use "legacy" VFIO, we will retire it again. Also, referring to it as "legacy" is both premature (since iommufd does not have the feature parity yet) and confusing in the passage of time.

(Note: :since:`Since 1.0.5`, the ``name`` attribute has been described to be used to select the type of PCI device assignment ("vfio", "kvm", or "xen"), but those values have been mostly diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index c278b81652..88979ecc39 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -60,6 +60,8 @@ int virDeviceHostdevPCIDriverInfoParseXML(xmlNodePtr node, virDeviceHostdevPCIDriverInfo *driver) { + virTristateBool iommufd; + driver->iommufd = false; if (virXMLPropEnum(node, "name", virDeviceHostdevPCIDriverNameTypeFromString, VIR_XML_PROP_NONZERO, @@ -67,6 +69,10 @@ virDeviceHostdevPCIDriverInfoParseXML(xmlNodePtr node, return -1; }

+ if (virXMLPropTristateBool(node, "iommufd", VIR_XML_PROP_NONE, &iommufd) < 0) + return -1; + virTristateBoolToBool(iommufd, &driver->iommufd);

Storing this as 'bool' is losing information. We need to be able to tell whether iommufd was not used because the user did not specify it or whether it was not used because the user explicitly said no for future compatibility reasons. Jano

+ driver->model = virXMLPropString(node, "model"); return 0; } @@ -93,6 +99,9 @@ virDeviceHostdevPCIDriverInfoFormat(virBuffer *buf,

virBufferEscapeString(&driverAttrBuf, " model='%s'", driver->model);

+ if (driver->iommufd) + virBufferAddLit(&driverAttrBuf, " iommufd='yes'"); + virXMLFormatElement(buf, "driver", &driverAttrBuf, NULL); return 0; }

TL;DR of all my rambling below - I think just adding "iommfd='yes'" attribute (as a tristate like Jano suggested) is fine for now, and don't think we should do anything with the name attribute. More details below if you're really in for a read :-) On 11/6/25 7:29 PM, Nathan Chen via Devel wrote:

On 11/6/2025 10:49 AM, Ján Tomko wrote:

...

...

Implement a new iommufd attribute under hostdevs' PCI subsystem driver that can be used to specify associated iommufd object when launching a qemu VM.

This does not specify which iommufd object it is, just to use the default one.

It's perfect for now, we might need a different element if using anything else than iommufd0 starts making sense.

Yeah, I think earlier versions of the patches explicitly gave the iommufd object name used by each device (e.g. literally "iommufd0"), and we deemed that "too much information", recommending to instead just say "use it" or "don't use it", and then later we can add an iommufdIndex or something that would default to 0, and then could contain other values if multiple iommufd objects were needed (and so, e.g., if two devices had "iommufd='yes' iommufdIndex='1'" then they would both be setup to use the same (non-default) iommufd (maybe "iommufd1"). So for right now while we're just supporting a single iommufd object per domain, the current proposed XML should be fine.

...

Also, I think it should fine not to expose the object in the XML since it has configurable attributes now:

I think you mean "*no* configurable attributes"?

...

# qemu-system-x86_64 -object iommufd,? iommufd options:    fd=<string>

Noted, will re-visit if anything else other than iommufd0 makes sense.

...

...

Signed-off-by: Nathan Chen <nathanc@nvidia.com> --- docs/formatdomain.rst           |  8 ++++++++ src/conf/device_conf.c          |  9 +++++++++ src/conf/device_conf.h          |  1 + src/conf/schemas/basictypes.rng |  5 +++++ src/qemu/qemu_command.c         | 19 +++++++++++++++++++ 5 files changed, 42 insertions(+)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 34dc9c3af7..a5c69dbcf4 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -4845,6 +4845,7 @@ or:    device; if PCI ROM loading is disabled through this attribute, attempts to    tweak the loading process further using the ``bar`` or ``file`` attributes    will be rejected. :since:`Since 4.3.0 (QEMU and KVM only)`. + ``address``    The ``address`` element for USB devices has a ``bus`` and ``device``    attribute to specify the USB bus and device number the device appears at on @@ -4885,6 +4886,13 @@ or:    found is "problematic" in some way, the generic vfio-pci driver    similarly be forced.

+   The ``<driver>`` element's ``iommufd`` attribute is used to specify +   using the iommufd interface to propagate DMA mappings to the kernel, +   instead of legacy VFIO. When the attribute is present, an iommufd +   object will be created by the resulting qemu command. Libvirt will +   open the /dev/iommu and VFIO device cdev, passing the associated +   file descriptor numbers to the qemu command. +

Should we resurrect the old attribute and use:    <driver name="iommufd"/>

The idea being that later in time, when it will no longer make sense to use "legacy" VFIO, we will retire it again.

My understanding is that this is still classified as "VFIO device assignment", but just using an iommufd for communication, so it's not "let's do this instead of VFIO", but "let's do VFIO *this* way instead of the other way". Meanwhile, you'd asked earlier about memories of the switch from "legacy KVM" device assignment to VFIO. One thing that's really important to know about that change (when thinking about it as a model to follow for this current change) is that during those days the presence of any PCI device assigned from the host to a guest would render the domain unmigrateable, and so when thinking about the transition of the default from one to the other we didn't need to consider the possibility of migrating a running guest from legacy KVM to VFIO - in order to switch from one to the other you had to shutdown and then restart the domain. We also kept around the "<driver name='kvm'/> nearly a decade longer than it was likely necessary - we only added the ability to manually select at all because someone "closer to customers/users" had insisted we needed a way to switch back to the "old way" if there was a bug in VFIO. But this was never needed - from the very beginning VFIO worked better than legacy KVM, and there were no "missing" features that would require someone to use legacy KVM assignment. I recall removing at least part of the supporting code for legacy KVM assignment several years ago (pretty sure someone else removed the final vestiges) and at the time thinking to myself "all this work that made the code and the configuration more complicated, and made maintenance more complicated and time consuming, only to *never* use it, and then finally remove it 10 years later. How depressing :-/".

...

Also, referring to it as "legacy" is both premature (since iommufd does not have the feature parity yet) and confusing in the passage of time.

I think it would be better to leave it as-is for now, since there are variant VFIO drivers besides vfio-pci that could be assigned to the driver name attribute in tandem with enabling iommufd. Actually a vfio variant driver (other than the variant driver that is automatically discovered as "most appropriate" for the device) is configured with <driver model='blah'/>, not name='blah'.

...

...

   (Note: :since:`Since 1.0.5`, the ``name`` attribute has been    described to be used to select the type of PCI device assignment    ("vfio", "kvm", or "xen"), but those values have been mostly diff --git a/src/conf/device_conf.c b/src/conf/device_conf.c index c278b81652..88979ecc39 100644 --- a/src/conf/device_conf.c +++ b/src/conf/device_conf.c @@ -60,6 +60,8 @@ int virDeviceHostdevPCIDriverInfoParseXML(xmlNodePtr node,                                       virDeviceHostdevPCIDriverInfo *driver) { +    virTristateBool iommufd; +    driver->iommufd = false;     if (virXMLPropEnum(node, "name",                        virDeviceHostdevPCIDriverNameTypeFromString,                        VIR_XML_PROP_NONZERO, @@ -67,6 +69,10 @@ virDeviceHostdevPCIDriverInfoParseXML(xmlNodePtr node,         return -1;     }

+    if (virXMLPropTristateBool(node, "iommufd", VIR_XML_PROP_NONE, &iommufd) < 0) +        return -1; +    virTristateBoolToBool(iommufd, &driver->iommufd);

Storing this as 'bool' is losing information. We need to be able to tell whether iommufd was not used because the user did not specify it or whether it was not used because the user explicitly said no for future compatibility reasons.

+1

That makes sense, I will update it to use virTristateBool instead in the next revision.

-Nathan

On 11/6/2025 11:19 AM, Ján Tomko wrote:

...

Open iommufd FDs from libvirt backend without exposing these FDs to XML users, i.e. one per domain for /dev/iommu and one per iommufd hostdev for /dev/vfio/devices/vfioX, and pass the FD to qemu command line.

The part formatting the object and the part formatting the device should be split.

Sounds good, I will split it into two commits.

...

Signed-off-by: Nathan Chen <nathanc@nvidia.com> --- src/qemu/qemu_command.c |  43 +++++++- src/qemu/qemu_command.h |   3 +- src/qemu/qemu_domain.c  |   8 ++ src/qemu/qemu_domain.h  |   7 ++ src/qemu/qemu_hotplug.c |   2 +- src/qemu/qemu_process.c | 232 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 289 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8fd7527645..740a6970f2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4730,7 +4730,8 @@ qemuBuildVideoCommandLine(virCommand *cmd,

virJSONValue * qemuBuildPCIHostdevDevProps(const virDomainDef *def, -                            virDomainHostdevDef *dev) +                            virDomainHostdevDef *dev, +                            virDomainObj *vm)

Hmm, perhaps exposing the iommufd object in the XML would save us from having to pass this.

We are passing virDomainObj to this function in order to retrieve the you referring to exposing the

...

{     g_autoptr(virJSONValue) props = NULL;     virDomainHostdevSubsysPCI *pcisrc = &dev->source.subsys.u.pci; @@ -4741,6 +4742,13 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,     const char *iommufdId = NULL;     /* 'ramfb' property must be omitted unless it's to be enabled */     bool ramfb = pcisrc->ramfb == VIR_TRISTATE_SWITCH_ON; +    bool useIommufd = false; +    qemuDomainObjPrivate *priv = vm ? vm->privateData : NULL; + +    if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO && +        pcisrc->driver.iommufd) { +        useIommufd = true; +    }

    /* caller has to assign proper passthrough driver name */     switch (pcisrc->driver.name) { @@ -4787,6 +4795,18 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,                               NULL) < 0)         return NULL;

...

addr.domain, pcisrc->addr.bus, +                                                      pcisrc- addr.slot, pcisrc->addr.function);

+    if (useIommufd && priv) { +        g_autofree char *vfioFdName = g_strdup_printf("vfio-%04x: %02x:%02x.%d", +                                                      pcisrc- +

There's no need to duplicate the list of hostdevs which use iommufd in a per-domain hash table.

For storing per-device file descriptors, we have per-device private data.

...

...

vfioDeviceFds, vfioFdName)); +        if (virJSONValueObjectAdd(&props, +                                  "S:fd", g_strdup_printf("%d", vfiofd), +                                  NULL) < 0) +            return NULL; +    }

+        int vfiofd = GPOINTER_TO_INT(g_hash_table_lookup(priv- +     if (qemuBuildDeviceAddressProps(props, def, dev->info) < 0)         return NULL;

@@ -5197,12 +5217,14 @@ qemuBuildAcpiNodesetProps(virCommand *cmd, static int qemuBuildHostdevCommandLine(virCommand *cmd,                             const virDomainDef *def, -                            virQEMUCaps *qemuCaps) +                            virQEMUCaps *qemuCaps, +                            virDomainObj *vm) {     size_t i;     g_autoptr(virJSONValue) props = NULL;     int iommufd = 0;     const char * iommufdId = "iommufd0"; +    qemuDomainObjPrivate *priv = vm->privateData;

    for (i = 0; i < def->nhostdevs; i++) {         virDomainHostdevDef *hostdev = def->hostdevs[i]; @@ -5233,8 +5255,10 @@ qemuBuildHostdevCommandLine(virCommand *cmd,

            if (subsys->u.pci.driver.iommufd && iommufd == 0) {                 iommufd = 1; +                virCommandPassFD(cmd, priv->iommufd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);                 if (qemuMonitorCreateObjectProps(&props, "iommufd",                                                  iommufdId, +                                                 "S:fd", g_strdup_printf("%d", priv->iommufd),                                                  NULL) < 0)                     return -1;

@@ -5245,7 +5269,18 @@ qemuBuildHostdevCommandLine(virCommand *cmd,             if (qemuCommandAddExtDevice(cmd, hostdev->info, def, qemuCaps) < 0)                 return -1;

...

source.subsys.u.pci; +                g_autofree char *vfioFdName = g_strdup_printf("vfio- %04x:%02x:%02x.%d", +                                                              pcisrc- addr.domain, pcisrc->addr.bus, +                                                              pcisrc- addr.slot, pcisrc->addr.function);

-            if (!(devprops = qemuBuildPCIHostdevDevProps(def, hostdev))) +            if (subsys->u.pci.driver.iommufd) { +                virDomainHostdevSubsysPCI *pcisrc = &hostdev- + +                int vfiofd = GPOINTER_TO_INT(g_hash_table_lookup(priv->vfioDeviceFds, vfioFdName)); + +                virCommandPassFD(cmd, vfiofd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);

This would become just:

qemuDomainHostdevPrivate *priv = (qemuDomainHostdevPrivate *)vsock-

...

privateData;

virCommandPassFD(cmd, priv->vfiofd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);

...

+            } + +            if (!(devprops = qemuBuildPCIHostdevDevProps(def, hostdev, vm)))                 return -1;

            if (qemuBuildDeviceCommandlineFromJSON(cmd, devprops, def, qemuCaps) < 0) @@ -10893,7 +10928,7 @@ qemuBuildCommandLine(virDomainObj *vm,     if (qemuBuildRedirdevCommandLine(cmd, def, qemuCaps) < 0)         return NULL;

-    if (qemuBuildHostdevCommandLine(cmd, def, qemuCaps) < 0) +    if (qemuBuildHostdevCommandLine(cmd, def, qemuCaps, vm) < 0)         return NULL;

    if (migrateURI) diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index ad068f1f16..380aac261f 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -180,7 +180,8 @@ qemuBuildThreadContextProps(virJSONValue **tcProps, /* Current, best practice */ virJSONValue * qemuBuildPCIHostdevDevProps(const virDomainDef *def, -                            virDomainHostdevDef *dev); +                            virDomainHostdevDef *dev, +                            virDomainObj *vm);

virJSONValue * qemuBuildRNGDevProps(const virDomainDef *def, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index a42721efad..86640aa3e3 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1953,6 +1953,11 @@ qemuDomainObjPrivateFree(void *data)

    virChrdevFree(priv->devs);

+    if (priv->iommufd >= 0) { +        virEventRemoveHandle(priv->iommufd);

There is no handle to remove (and none is needed). So no need for the condition either.

...

+        priv->iommufd = -1; +    } +     if (priv->pidMonitored >= 0) {         virEventRemoveHandle(priv->pidMonitored);         priv->pidMonitored = -1; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 45fc32a663..cecfed94a7 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -25,6 +25,7 @@ #include <unistd.h> #include <signal.h> #include <sys/stat.h> +#include <dirent.h>

We should not need this in qemu_process.

...

#if WITH_SYS_SYSCALL_H # include <sys/syscall.h> #endif @@ -8091,6 +8092,9 @@ qemuProcessLaunch(virConnectPtr conn,     if (qemuExtDevicesStart(driver, vm, incomingMigrationExtDevices) < 0)         goto cleanup;

+    if (qemuProcessOpenVfioFds(vm) < 0) +        goto cleanup; +     if (!(cmd = qemuBuildCommandLine(vm,                                      incoming ? "defer" : NULL,                                      vmop, @@ -10267,3 +10271,231 @@ qemuProcessHandleNbdkitExit(qemuNbdkitProcess *nbdkit,     qemuProcessEventSubmit(vm, QEMU_PROCESS_EVENT_NBDKIT_EXITED, 0, 0, nbdkit);     virObjectUnlock(vm); } + +/** + * qemuProcessOpenIommuFd: + * @vm: domain object + * @iommuFd: returned file descriptor + * + * Opens /dev/iommu file descriptor for the VM. + * + * Returns: 0 on success, -1 on failure + */ +static int +qemuProcessOpenIommuFd(virDomainObj *vm, int *iommuFd) +{ +    int fd = -1; + +    VIR_DEBUG("Opening IOMMU FD for domain %s", vm->def->name); + +    if ((fd = open("/dev/iommu", O_RDWR | O_CLOEXEC)) < 0) { +        if (errno == ENOENT) { +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", +                           _("IOMMU FD support requires /dev/iommu device")); +        } else { +            virReportSystemError(errno, "%s", +                                 _("cannot open /dev/iommu")); +        } +        return -1; +    } + +    *iommuFd = fd; +    VIR_DEBUG("Opened IOMMU FD %d for domain %s", fd, vm->def->name); +    return 0; +} + +/** + * qemuProcessGetVfioDevicePath: + * @hostdev: host device definition + * @vfioPath: returned VFIO device path + * + * Constructs the VFIO device path for a PCI hostdev. + * + * Returns: 0 on success, -1 on failure + */ +static int +qemuProcessGetVfioDevicePath(virDomainHostdevDef *hostdev,

No need to pass the whole hostdev here. Then this function can live in virpci.c

...

+                             char **vfioPath) +{ +    virPCIDeviceAddress *addr; +    g_autofree char *sysfsPath = NULL; +    DIR *dir = NULL; +    struct dirent *entry = NULL; +    int ret = -1; +

On a Thursday in 2025, Nathan Chen wrote:

On 11/6/2025 11:19 AM, Ján Tomko wrote:

...

...

Open iommufd FDs from libvirt backend without exposing these FDs to XML users, i.e. one per domain for /dev/iommu and one per iommufd hostdev for /dev/vfio/devices/vfioX, and pass the FD to qemu command line.

The part formatting the object and the part formatting the device should be split.

Sounds good, I will split it into two commits. >> Signed-off-by: Nathan Chen <nathanc@nvidia.com>

...

...

--- src/qemu/qemu_command.c |  43 +++++++- src/qemu/qemu_command.h |   3 +- src/qemu/qemu_domain.c  |   8 ++ src/qemu/qemu_domain.h  |   7 ++ src/qemu/qemu_hotplug.c |   2 +- src/qemu/qemu_process.c | 232 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 289 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8fd7527645..740a6970f2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4730,7 +4730,8 @@ qemuBuildVideoCommandLine(virCommand *cmd,

virJSONValue * qemuBuildPCIHostdevDevProps(const virDomainDef *def, -                            virDomainHostdevDef *dev) +                            virDomainHostdevDef *dev, +                            virDomainObj *vm)

Hmm, perhaps exposing the iommufd object in the XML would save us from having to pass this.

We are passing virDomainObj to this function in order to retrieve the FD number, would you mind clarifying how we could avoid passing this by exposing the iommufd object in the XML? It is my understanding that exposing the iommufd object ID would still mean we need pass the virDomainObj.

If it was a separate device, it would have its own data type and own formatting function unrelated to hostdevs.

...

...

{     g_autoptr(virJSONValue) props = NULL;     virDomainHostdevSubsysPCI *pcisrc = &dev->source.subsys.u.pci; @@ -4741,6 +4742,13 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,     const char *iommufdId = NULL;     /* 'ramfb' property must be omitted unless it's to be enabled */     bool ramfb = pcisrc->ramfb == VIR_TRISTATE_SWITCH_ON; +    bool useIommufd = false; +    qemuDomainObjPrivate *priv = vm ? vm->privateData : NULL; + +    if (pcisrc->driver.name == VIR_DEVICE_HOSTDEV_PCI_DRIVER_NAME_VFIO && +        pcisrc->driver.iommufd) { +        useIommufd = true; +    }

    /* caller has to assign proper passthrough driver name */     switch (pcisrc->driver.name) { @@ -4787,6 +4795,18 @@ qemuBuildPCIHostdevDevProps(const virDomainDef *def,                               NULL) < 0)         return NULL;

...

addr.domain, pcisrc->addr.bus, +                                                      pcisrc- addr.slot, pcisrc->addr.function);

+    if (useIommufd && priv) { +        g_autofree char *vfioFdName = g_strdup_printf("vfio-%04x: %02x:%02x.%d", +                                                      pcisrc- +

There's no need to duplicate the list of hostdevs which use iommufd in a per-domain hash table.

For storing per-device file descriptors, we have per-device private data.

...

...

vfioDeviceFds, vfioFdName)); +        if (virJSONValueObjectAdd(&props, +                                  "S:fd", g_strdup_printf("%d", vfiofd), +                                  NULL) < 0) +            return NULL; +    }

+        int vfiofd = GPOINTER_TO_INT(g_hash_table_lookup(priv- +     if (qemuBuildDeviceAddressProps(props, def, dev->info) < 0)         return NULL;

@@ -5197,12 +5217,14 @@ qemuBuildAcpiNodesetProps(virCommand *cmd, static int qemuBuildHostdevCommandLine(virCommand *cmd,                             const virDomainDef *def, -                            virQEMUCaps *qemuCaps) +                            virQEMUCaps *qemuCaps, +                            virDomainObj *vm) {     size_t i;     g_autoptr(virJSONValue) props = NULL;     int iommufd = 0;     const char * iommufdId = "iommufd0"; +    qemuDomainObjPrivate *priv = vm->privateData;

    for (i = 0; i < def->nhostdevs; i++) {         virDomainHostdevDef *hostdev = def->hostdevs[i]; @@ -5233,8 +5255,10 @@ qemuBuildHostdevCommandLine(virCommand *cmd,

            if (subsys->u.pci.driver.iommufd && iommufd == 0) {                 iommufd = 1; +                virCommandPassFD(cmd, priv->iommufd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);                 if (qemuMonitorCreateObjectProps(&props, "iommufd",                                                  iommufdId, +                                                 "S:fd", g_strdup_printf("%d", priv->iommufd),                                                  NULL) < 0)                     return -1;

@@ -5245,7 +5269,18 @@ qemuBuildHostdevCommandLine(virCommand *cmd,             if (qemuCommandAddExtDevice(cmd, hostdev->info, def, qemuCaps) < 0)                 return -1;

...

source.subsys.u.pci; +                g_autofree char *vfioFdName = g_strdup_printf("vfio- %04x:%02x:%02x.%d", +                                                             

-            if (!(devprops = qemuBuildPCIHostdevDevProps(def, hostdev))) +            if (subsys->u.pci.driver.iommufd) { +                virDomainHostdevSubsysPCI *pcisrc = &hostdev- pcisrc- >addr.domain, pcisrc->addr.bus, +                                                              pcisrc- >addr.slot, pcisrc->addr.function); + +                int vfiofd = GPOINTER_TO_INT(g_hash_table_lookup(priv->vfioDeviceFds, vfioFdName)); + +                virCommandPassFD(cmd, vfiofd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);

This would become just:

qemuDomainHostdevPrivate *priv = (qemuDomainHostdevPrivate *)vsock-

...

privateData;

virCommandPassFD(cmd, priv->vfiofd, VIR_COMMAND_PASS_FD_CLOSE_PARENT);

I will proceed with implementing a qemuDomainHostdevPrivate struct and look into the existing implementation for qemuDomainDiskPrivate for reference. I was not able to find a private data attribute in the _virDomainHostdevDef struct definition, but I do see "virObject *privateData;" under the _virDomainDiskDef struct definition - are you aware of any reason behind this, or has it just never been needed for the hostdev struct?

It was added for disks at the time when it was needed. Jano

...

...

+            } + +            if (!(devprops = qemuBuildPCIHostdevDevProps(def, hostdev, vm)))                 return -1;

On a Friday in 2025, Nathan Chen wrote:

On 11/7/2025 4:40 AM, Ján Tomko wrote:

...

On a Thursday in 2025, Nathan Chen wrote:

...

On 11/6/2025 11:19 AM, Ján Tomko wrote:

...

...

Open iommufd FDs from libvirt backend without exposing these FDs to XML users, i.e. one per domain for /dev/iommu and one per iommufd hostdev for /dev/vfio/devices/vfioX, and pass the FD to qemu command line.

The part formatting the object and the part formatting the device should be split.

Sounds good, I will split it into two commits. >> Signed-off-by: Nathan Chen <nathanc@nvidia.com>

...

...

--- src/qemu/qemu_command.c |  43 +++++++- src/qemu/qemu_command.h |   3 +- src/qemu/qemu_domain.c  |   8 ++ src/qemu/qemu_domain.h  |   7 ++ src/qemu/qemu_hotplug.c |   2 +- src/qemu/qemu_process.c | 232 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 289 insertions(+), 6 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8fd7527645..740a6970f2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4730,7 +4730,8 @@ qemuBuildVideoCommandLine(virCommand *cmd,

virJSONValue * qemuBuildPCIHostdevDevProps(const virDomainDef *def, -                            virDomainHostdevDef *dev) +                            virDomainHostdevDef *dev, +                            virDomainObj *vm)

Hmm, perhaps exposing the iommufd object in the XML would save us from having to pass this.

We are passing virDomainObj to this function in order to retrieve the FD number, would you mind clarifying how we could avoid passing this by exposing the iommufd object in the XML? It is my understanding that exposing the iommufd object ID would still mean we need pass the virDomainObj.

If it was a separate device, it would have its own data type and own formatting function unrelated to hostdevs.

That makes sense, like passing a new virDomainIommufdDef struct pointer to a new qemuBuildIommufdDevProps() function. Previously we implemented a virDomainIommufdDef struct as a member under virDomainIOMMUDef [0] to store the iommufd ID and FD number. But we changed the implementation adn XML representation to only be a bool member associated with hostdevs. What are your thoughts on either of the following paths forward?

1. Re-implementing the virDomainIommufdDef struct as a virDomainHostdevDef member, storing the FD numbers as part of a private data struct member in virDomainIommufdDef 2. Storing the FD numbers as part of a private data struct member in virDomainHostdevDef, and avoiding implementing a separate virDomainIommufdDef struct.

Option 2 seems the most straightforward t ome, still allowing us to avoid passing the virDomainObj to qemuBuildPCIHostdevDevPRops(), but please let me know what you think.

Oops, I missed this question. Option 2 sounds better to me. Jano

[0] https://lists.libvirt.org/archives/list/devel@lists.libvirt.org/thread/EASBQ...

Thanks, Nathan

On a Monday in 2025, Nathan Chen via Devel wrote:

Allow access to /dev/iommu and /dev/vfio/devices/vfio* when launching a qemu VM with iommufd feature enabled.

Signed-off-by: Nathan Chen <nathanc@nvidia.com> --- src/qemu/qemu_cgroup.c | 61 ++++++++++++++++++++++++++++ src/qemu/qemu_cgroup.h | 1 + src/qemu/qemu_namespace.c | 44 +++++++++++++++++++++ src/security/security_apparmor.c | 15 +++++++ src/security/security_dac.c | 34 ++++++++++++++++ src/security/security_selinux.c | 34 ++++++++++++++++ src/security/virt-aa-helper.c | 11 +++++- src/util/virpci.c | 68 ++++++++++++++++++++++++++++++++ src/util/virpci.h | 1 + 9 files changed, 268 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 46a7dc1d8b..e15ffd2007 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -461,6 +461,54 @@ qemuTeardownInputCgroup(virDomainObj *vm, }

+int +qemuSetupIommufdCgroup(virDomainObj *vm) +{ + qemuDomainObjPrivate *priv = vm->privateData; + g_autoptr(DIR) dir = NULL; + struct dirent *dent; + g_autofree char *path = NULL; + int iommufd = 0; + size_t i; + + for (i = 0; i < vm->def->nhostdevs; i++) { + if (vm->def->hostdevs[i]->source.subsys.u.pci.driver.iommufd) { + iommufd = 1; + break; + } + } + + if (iommufd == 1) { + if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) + return 0; + if (virDirOpen(&dir, "/dev/vfio/devices") < 0) { + if (errno == ENOENT) + return 0; + return -1; + } + while (virDirRead(dir, &dent, "/dev/vfio/devices") > 0) { + if (STRPREFIX(dent->d_name, "vfio")) { + path = g_strdup_printf("/dev/vfio/devices/%s", dent->d_name); + } + if (path && + qemuCgroupAllowDevicePath(vm, path, + VIR_CGROUP_DEVICE_RW, false) < 0) { + return -1;

This allows all the devices instead of just the ones the VM needs. Also, this is still a hostdev, so it should be done inside qemuSetupHostdevCgroup. Do hostdevs using iommufd also need access to a) /dev/vfio/vfio and b) /dev/vfio/<iommugroup> which were already allowed in qemuSetupHostdevCgroup?

+ } + path = NULL; + }

+ if (virFileExists("/dev/iommu")) + path = g_strdup("/dev/iommu");

No need to check for the existence of the device. If it does not exist, the VM won't start anyway. Also, is it necessary to allow these? libvirt already opened the files and passed file descriptors.

+ if (path && + qemuCgroupAllowDevicePath(vm, path, + VIR_CGROUP_DEVICE_RW, false) < 0) { + return -1; + } + } + return 0; +} + + /** * qemuSetupHostdevCgroup: * vm: domain object @@ -759,6 +807,7 @@ qemuSetupDevicesCgroup(virDomainObj *vm) g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(priv->driver); const char *const *deviceACL = (const char *const *) cfg->cgroupDeviceACL; int rv = -1; + int iommufd = 0; size_t i;

if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_DEVICES)) @@ -836,6 +885,18 @@ qemuSetupDevicesCgroup(virDomainObj *vm) return -1; }

+ for (i = 0; i < vm->def->nhostdevs; i++) { + if (vm->def->hostdevs[i]->source.subsys.u.pci.driver.iommufd) { + iommufd = 1; + break; + } + } +

No need to check this upfront. If /dev/iommu access is necessary, the per-hostdev function qemuSetupHostdevCgroup can add it to the list multiple times, like it already does for /dev/vfio/vfio

+ if (iommufd == 1) { + if (qemuSetupIommufdCgroup(vm) < 0) + return -1; + } + for (i = 0; i < vm->def->nmems; i++) { if (qemuSetupMemoryDevicesCgroup(vm, vm->def->mems[i]) < 0) return -1; diff --git a/src/qemu/qemu_cgroup.h b/src/qemu/qemu_cgroup.h index 3668034cde..bea677ba3c 100644 --- a/src/qemu/qemu_cgroup.h +++ b/src/qemu/qemu_cgroup.h @@ -42,6 +42,7 @@ int qemuSetupHostdevCgroup(virDomainObj *vm, int qemuTeardownHostdevCgroup(virDomainObj *vm, virDomainHostdevDef *dev) G_GNUC_WARN_UNUSED_RESULT; +int qemuSetupIommufdCgroup(virDomainObj *vm); int qemuSetupMemoryDevicesCgroup(virDomainObj *vm, virDomainMemoryDef *mem); int qemuTeardownMemoryDevicesCgroup(virDomainObj *vm, diff --git a/src/qemu/qemu_namespace.c b/src/qemu/qemu_namespace.c index 932777505b..80496f2f0f 100644 --- a/src/qemu/qemu_namespace.c +++ b/src/qemu/qemu_namespace.c @@ -683,6 +683,47 @@ qemuDomainSetupLaunchSecurity(virDomainObj *vm, }

+static int +qemuDomainSetupIommufd(virDomainObj *vm, + GSList **paths) +{ + g_autoptr(DIR) dir = NULL; + struct dirent *dent; + g_autofree char *path = NULL; + int iommufd = 0; + size_t i; + + for (i = 0; i < vm->def->nhostdevs; i++) { + if (vm->def->hostdevs[i]->source.subsys.u.pci.driver.iommufd) { + iommufd = 1; + break; + } + } + + /* Check if iommufd is enabled */ + if (iommufd == 1) { + if (virDirOpen(&dir, "/dev/vfio/devices") < 0) { + if (errno == ENOENT) + return 0; + return -1; + } + while (virDirRead(dir, &dent, "/dev/vfio/devices") > 0) { + if (STRPREFIX(dent->d_name, "vfio")) { + path = g_strdup_printf("/dev/vfio/devices/%s", dent->d_name); + *paths = g_slist_prepend(*paths, g_steal_pointer(&path)); + } + } + path = NULL; + if (virFileExists("/dev/iommu")) + path = g_strdup("/dev/iommu"); + if (path) + *paths = g_slist_prepend(*paths, g_steal_pointer(&path));

Same comments as for cgroups apply here too.

+ } + + return 0; +} + + static int qemuNamespaceMknodPaths(virDomainObj *vm, GSList *paths, @@ -706,6 +747,9 @@ qemuDomainBuildNamespace(virQEMUDriverConfig *cfg, if (qemuDomainSetupAllDisks(vm, &paths) < 0) return -1;

+ if (qemuDomainSetupIommufd(vm, &paths) < 0) + return -1; + if (qemuDomainSetupAllHostdevs(vm, &paths) < 0) return -1;

diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 68ac39611f..0a878fd205 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -856,6 +856,21 @@ AppArmorSetSecurityHostdevLabel(virSecurityManager *mgr, } ret = AppArmorSetSecurityPCILabel(pci, vfioGroupDev, ptr); VIR_FREE(vfioGroupDev); + + if (dev->source.subsys.u.pci.driver.iommufd) { + g_autofree char *vfiofdDev = virPCIDeviceGetIOMMUFDDev(pci); + const char *iommufdDir = "/dev/iommu"; + if (vfiofdDev) { + int ret2 = AppArmorSetSecurityPCILabel(pci, vfiofdDev, ptr); + if (ret2 < 0) + ret = ret2; + ret2 = AppArmorSetSecurityPCILabel(pci, iommufdDir, ptr); + if (ret2 < 0) + ret = ret2; + } else { + return -1; + } + } } else { ret = virPCIDeviceFileIterate(pci, AppArmorSetSecurityPCILabel, ptr); } diff --git a/src/util/virpci.c b/src/util/virpci.c index 90617e69c6..6e6e5e47c0 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2478,6 +2478,74 @@ virPCIDeviceGetIOMMUGroupDev(virPCIDevice *dev) return g_strdup_printf("/dev/vfio/%s", groupFile); }

+/* virPCIDeviceGetIOMMUFDDev - return the name of the device used + * to control this PCI device's group (e.g. "/dev/vfio/devices/vfio15") + */ +char * +virPCIDeviceGetIOMMUFDDev(virPCIDevice *dev) +{ + g_autofree char *path = NULL; + const char *pci_addr = NULL; + g_autoptr(DIR) dir = NULL; + struct dirent *entry; + char *vfiodev = NULL; + + /* Get PCI device address */

No need for this kind of comment - it's obvious from the variable and function names.

+ pci_addr = virPCIDeviceGetName(dev); + if (!pci_addr) + return NULL; + + /* First try: look in PCI device's vfio-dev subdirectory */ + path = g_strdup_printf("/sys/bus/pci/devices/%s/vfio-dev", pci_addr); + + if (virDirOpen(&dir, path) == 1) { + while (virDirRead(dir, &entry, path) > 0) { + if (!g_str_has_prefix(entry->d_name, "vfio")) + continue; + + vfiodev = g_strdup_printf("/dev/vfio/devices/%s", entry->d_name); + break; + } + /* g_autoptr will automatically close dir when it goes out of scope */

This comment is also obvious.

+ dir = NULL;

That does not make dir go out of scope. That's a memory leak. We try not to mix g_auto with manual freeing of variables, so either use two variables or two different scopes. Jano

+ } + + /* Second try: scan /sys/class/vfio-dev for matching device */ + if (!vfiodev) { + g_free(path); + path = g_strdup("/sys/class/vfio-dev"); +