On Fri, May 08, 2026 at 06:17:19PM +0000, David Matlack wrote:
On 2026-05-05 10:14 PM, Samiullah Khawaja wrote:
IOMMUFD allows creating multiple IOAS and HWPTs under one iommufd, Add
API to init a struct iommu using an already opened iommufd. The API
internally creates a new IOAS and also a new HWPT as an option based on
the flags passed to the function.
Signed-off-by: Samiullah Khawaja <[email protected]>
---
.../vfio/lib/include/libvfio/iommu.h | 5 ++
.../lib/include/libvfio/vfio_pci_device.h | 2 +
tools/testing/selftests/vfio/lib/iommu.c | 62 +++++++++++++++++--
.../selftests/vfio/lib/vfio_pci_device.c | 22 ++++++-
4 files changed, 84 insertions(+), 7 deletions(-)
[snip]
int __iommu_map(struct iommu *iommu, struct dma_region *region);
diff --git a/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
b/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
index 2858885a89bb..1143ceb6a9b8 100644
--- a/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
+++ b/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
@@ -19,6 +19,7 @@ struct vfio_pci_device {
const char *bdf;
int fd;
int group_fd;
+ u32 dev_id;
Please introduce an initialize this field in its own patch and, in the
commit message, explain why it is being added and how it will be used.
Agreed. Will do in next revision.
struct iommu *iommu;
@@ -65,6 +66,7 @@ void vfio_pci_config_access(struct vfio_pci_device *device,
bool write,
#define vfio_pci_config_writew(_d, _o, _v) vfio_pci_config_write(_d, _o, _v,
u16)
#define vfio_pci_config_writel(_d, _o, _v) vfio_pci_config_write(_d, _o, _v,
u32)
+void vfio_pci_device_attach_iommu(struct vfio_pci_device *device, struct iommu
*iommu);
void vfio_pci_irq_enable(struct vfio_pci_device *device, u32 index,
u32 vector, int count);
void vfio_pci_irq_disable(struct vfio_pci_device *device, u32 index);
diff --git a/tools/testing/selftests/vfio/lib/iommu.c
b/tools/testing/selftests/vfio/lib/iommu.c
index 035dac069d60..644c049cf9f0 100644
--- a/tools/testing/selftests/vfio/lib/iommu.c
+++ b/tools/testing/selftests/vfio/lib/iommu.c
@@ -408,6 +408,18 @@ struct iommu_iova_range *iommu_iova_ranges(struct iommu
*iommu, u32 *nranges)
return ranges;
}
[snip]
+struct iommu *iommufd_iommu_init(int iommufd, u32 dev_id, u32 flags)
+{
+ struct iommu *iommu;
+
+ iommu = iommu_alloc("iommufd");
Use MODE_IOMMUFD instead of string literal.
Agreed. Will update this.
+
+ iommu->iommufd = dup(iommufd);
+ VFIO_ASSERT_GT(iommu->iommufd, 0);
+
+ iommu->ioas_id = iommufd_ioas_alloc(iommu->iommufd);
Please avoid duplicating this code. e.g. Something like this:
Agreed. Will update this.
diff --git a/tools/testing/selftests/vfio/lib/iommu.c
b/tools/testing/selftests/vfio/lib/iommu.c
index 644c049cf9f0..4909cda5c840 100644
--- a/tools/testing/selftests/vfio/lib/iommu.c
+++ b/tools/testing/selftests/vfio/lib/iommu.c
@@ -443,6 +443,19 @@ static struct iommu *iommu_alloc(const char *iommu_mode)
return iommu;
}
+static void iommufd_init(struct iommu *iommu, int iommufd)
+{
+ /*
+ * Require device->iommufd to be >0 so that a simple non-0 check can be
+ * used to check if iommufd is enabled. In practice open() will never
+ * return 0 unless stdin is closed.
+ */
+ VFIO_ASSERT_GT(iommufd, 0);
+
+ iommu->iommufd = iommufd;
+ iommu->ioas_id = iommufd_ioas_alloc(iommu->iommufd);
+}
+
struct iommu *iommu_init(const char *iommu_mode)
{
const char *container_path;
@@ -459,15 +472,7 @@ struct iommu *iommu_init(const char *iommu_mode)
version = ioctl(iommu->container_fd, VFIO_GET_API_VERSION);
VFIO_ASSERT_EQ(version, VFIO_API_VERSION, "Unsupported version:
%d\n", version);
} else {
- /*
- * Require device->iommufd to be >0 so that a simple non-0
check can be
- * used to check if iommufd is enabled. In practice open() will
never
- * return 0 unless stdin is closed.
- */
- iommu->iommufd = open("/dev/iommu", O_RDWR);
- VFIO_ASSERT_GT(iommu->iommufd, 0);
-
- iommu->ioas_id = iommufd_ioas_alloc(iommu->iommufd);
+ iommufd_init(iommu, open("/dev/iommu", O_RDWR));
}
return iommu;
@@ -478,11 +483,7 @@ struct iommu *iommufd_iommu_init(int iommufd, u32 dev_id,
u32 flags)
struct iommu *iommu;
iommu = iommu_alloc("iommufd");
-
- iommu->iommufd = dup(iommufd);
- VFIO_ASSERT_GT(iommu->iommufd, 0);
-
- iommu->ioas_id = iommufd_ioas_alloc(iommu->iommufd);
+ iommufd_init(iommu, dup(iommufd));
if (flags & IOMMUFD_IOMMU_INIT_CREATE_PT)
iommu->hwpt_id = iommufd_hwpt_alloc(iommu, dev_id);
+
+ if (flags & IOMMUFD_IOMMU_INIT_CREATE_PT)
+ iommu->hwpt_id = iommufd_hwpt_alloc(iommu, dev_id);
Does this need to be part of iommufd_iommu_init()? Maybe it would be
better to expose a separate helper to allocate a HWPT for a given struct
iommu *.
Hmm.. that is interesting.. I think we can have following immediate
possibilities when creating a struct iommu (there will be more down the
road, but can be handled in similar way):
- create using struct iommu with a new IOAS.
- create using struct iommu with existing IOAS but new HWPT.
- create using struct iommu with new IOAS and new HWPT.
I think I should probably add following flags:
IOMMUFD_IOMMU_INIT_CREATE_IOPT (IO Page Table) or _PT
IOMMUFD_IOMMU_INIT_CREATE_IOAS (IO address Space) or _AS
At least one of those should be required when creating new struct iommu
from an existing one. WDYT?
This would enable use of HWPTs in iommus constructed by iommu_init() as
well, which someone may want to do in the future.
If you go this route, please introduce this in its own commit.
Both flags can be introduced in same commit or should be separate?
+
+ return iommu;
+}
+
+static void iommufd_cleanup(struct iommu *iommu)
+{
+ struct iommu_destroy args = {
+ .size = sizeof(args),
+ };
+
+ if (iommu->hwpt_id) {
+ args.id = iommu->hwpt_id;
+ ioctl_assert(iommu->iommufd, IOMMU_DESTROY, &args);
+ }
+
+ args.id = iommu->ioas_id;
+ ioctl_assert(iommu->iommufd, IOMMU_DESTROY, &args);
Please create a helper function to do IOMMU_DESTROY ioctl. Then here you
can just do:
if (iommu->hwpt_id)
iommufd_iommu_destroy(iommu, iommu->hwpt_id);
iommfd_iommu_destroy(iommu, iommu->ioas_id);
Looks great. Will do.
+
+ VFIO_ASSERT_EQ(close(iommu->iommufd), 0);
+}
+
void iommu_cleanup(struct iommu *iommu)
{
if (iommu->iommufd)
- VFIO_ASSERT_EQ(close(iommu->iommufd), 0);
+ iommufd_cleanup(iommu);
else
VFIO_ASSERT_EQ(close(iommu->container_fd), 0);
[snip]
+void vfio_pci_device_attach_iommu(struct vfio_pci_device *device, struct iommu
*iommu)
+{
+ u32 pt_id = iommu->ioas_id;
+
+ /* Only iommufd supports changing struct iommu attachments */
+ VFIO_ASSERT_TRUE(iommu->iommufd);
+
+ if (iommu->hwpt_id)
+ pt_id = iommu->hwpt_id;
nit: This can be folded into the variable declaration.
const u32 pt_id = iommu->hwpt_id ?: iommu->ioas_id;
Agreed
+
+ VFIO_ASSERT_NE(pt_id, 0);
Why is this check needed?
I think it is redundant. I will remove it.
+ vfio_device_attach_iommufd_pt(device->fd, pt_id);
+ device->iommu = iommu;
+}
Please introduce vfio_pci_device_attach_iommu() in its own patch.
Agreed. Will do.
+
static void vfio_pci_iommufd_setup(struct vfio_pci_device *device, const char
*bdf)
{
const char *cdev_path = vfio_pci_get_cdev_path(bdf);
@@ -350,8 +366,8 @@ static void vfio_pci_iommufd_setup(struct vfio_pci_device
*device, const char *b
VFIO_ASSERT_GE(device->fd, 0);
free((void *)cdev_path);
- vfio_device_bind_iommufd(device->fd, device->iommu->iommufd);
- vfio_device_attach_iommufd_pt(device->fd, device->iommu->ioas_id);
+ device->dev_id = vfio_device_bind_iommufd(device->fd,
device->iommu->iommufd);
+ vfio_pci_device_attach_iommu(device, device->iommu);
}
struct vfio_pci_device *vfio_pci_device_init(const char *bdf, struct iommu
*iommu)
--
2.54.0.545.g6539524ca2-goog
Thanks,
Sami
