Re: [RFC PATCH 1/3] vfio: selftests: Add support of creating iommus from iommufd

[Samiullah Khawaja <skhawaja@google.com>]

On Wed, Oct 21, 2026 at 10:19:34PM +0000, David Matlack wrote:
>On 2026-09-25 01:13 AM, Samiullah Khawaja wrote:
>> Add API to init a struct iommu using an already opened iommufd instance
>> and attach devices to it.
>>
>> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
>> ---
>>  .../vfio/lib/include/libvfio/iommu.h          |  2 +
>>  .../lib/include/libvfio/vfio_pci_device.h     |  2 +
>>  tools/testing/selftests/vfio/lib/iommu.c      | 60 +++++++++++++++++--
>>  .../selftests/vfio/lib/vfio_pci_device.c      | 25 ++++++--
>>  4 files changed, 80 insertions(+), 9 deletions(-)
>>
>> diff --git a/tools/testing/selftests/vfio/lib/include/libvfio/iommu.h b/tools/testing/selftests/vfio/lib/include/libvfio/iommu.h
>> index e9a3386a4719..62f1e8affd2b 100644
>> --- a/tools/testing/selftests/vfio/lib/include/libvfio/iommu.h
>> +++ b/tools/testing/selftests/vfio/lib/include/libvfio/iommu.h
>> @@ -29,10 +29,12 @@ struct iommu {
>>  	int container_fd;
>>  	int iommufd;
>>  	u32 ioas_id;
>> +	u32 hwpt_id;
>>  	struct list_head dma_regions;
>>  };
>>
>>  struct iommu *iommu_init(const char *iommu_mode);
>> +struct iommu *iommufd_iommu_init(int iommufd, u32 dev_id);
>>  void iommu_cleanup(struct iommu *iommu);
>>
>>  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 e19bd94b8dd2..4dcc23bc9b78 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;
>>
>>  	struct iommu *iommu;
>>
>> @@ -82,6 +83,7 @@ static inline void vfio_pci_cmd_clear(struct vfio_pci_device *device, u16 bits)
>>  	vfio_pci_config_writew(device, PCI_COMMAND, cmd & ~bits);
>>  }
>>
>> +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 b6f3c5c84e01..6d2d5c82813c 100644
>> --- a/tools/testing/selftests/vfio/lib/iommu.c
>> +++ b/tools/testing/selftests/vfio/lib/iommu.c
>> @@ -405,6 +405,18 @@ struct iommu_iova_range *iommu_iova_ranges(struct iommu *iommu, u32 *nranges)
>>  	return ranges;
>>  }
>>
>> +static u32 iommufd_hwpt_alloc(struct iommu *iommu, u32 dev_id)
>> +{
>> +	struct iommu_hwpt_alloc args = {
>> +		.size = sizeof(args),
>> +		.pt_id = iommu->ioas_id,
>> +		.dev_id = dev_id,
>> +	};
>> +
>> +	ioctl_assert(iommu->iommufd, IOMMU_HWPT_ALLOC, &args);
>> +	return args.out_hwpt_id;
>> +}
>> +
>>  static u32 iommufd_ioas_alloc(int iommufd)
>>  {
>>  	struct iommu_ioas_alloc args = {
>> @@ -415,17 +427,25 @@ static u32 iommufd_ioas_alloc(int iommufd)
>>  	return args.out_ioas_id;
>>  }
>>
>> -struct iommu *iommu_init(const char *iommu_mode)
>> +static struct iommu *iommu_alloc(const char *iommu_mode)
>>  {
>> -	const char *container_path;
>>  	struct iommu *iommu;
>> -	int version;
>>
>>  	iommu = calloc_assert(1, sizeof(*iommu));
>>
>>  	INIT_LIST_HEAD(&iommu->dma_regions);
>>
>>  	iommu->mode = lookup_iommu_mode(iommu_mode);
>> +	return iommu;
>> +}
>> +
>> +struct iommu *iommu_init(const char *iommu_mode)
>> +{
>> +	const char *container_path;
>> +	struct iommu *iommu;
>> +	int version;
>> +
>> +	iommu = iommu_alloc(iommu_mode);
>>
>>  	container_path = iommu->mode->container_path;
>>  	if (container_path) {
>> @@ -449,10 +469,42 @@ struct iommu *iommu_init(const char *iommu_mode)
>>  	return iommu;
>>  }
>>
>> +struct iommu *iommufd_iommu_init(int iommufd, u32 dev_id)
>
>I think this is a good opportunity to start adding kernel-doc comments.
>It's not very obvious how and when to use both iommufd_iommu_init() and
>vfio_pci_device_attach_iommu(). A kernel-doc comment would help clarify
>that.

Agreed. I will add it.
>
>> +{
>> +	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);
>> +	iommu->hwpt_id = iommufd_hwpt_alloc(iommu, dev_id);
>> +
>> +	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 put IOMMU_DESTROY into its own helper so this can be:
>
>if (iommu->hwpt_id)
>        iommufd_iommu_destroy(iommu, iommu->hwpt_id);
>
>iommufd_iommu_destroy(iommu, iommu->ioas_id);

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);
>>
>> diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
>> index 774d1e90ec0e..81d3879d064f 100644
>> --- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
>> +++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
>> @@ -469,15 +469,15 @@ int __vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token)
>>  	if (ioctl(device_fd, VFIO_DEVICE_BIND_IOMMUFD, &args))
>>  		return -errno;
>>
>> -	return 0;
>> +	return args.out_devid;
>
>This breaks vfio_pci_sriov_uapi_test for all iommufd variants (9/45
>fail). iommufd_setup() in that test returns the result of
>__vfio_device_bind_iommufd() directly, and the
>fixture/ASSERT_COND_VF_CREATION() treat any non-zero value as failure.
>
>Something like the following is needed to fix it:
>
>diff --git a/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c b/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c
>--- a/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c
>+++ b/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c
>@@ -32,9 +32,14 @@ static int container_setup(struct vfio_pci_device *device, const char *bdf,
> static int iommufd_setup(struct vfio_pci_device *device, const char *bdf,
>		  const char *vf_token)
> {
>+    int ret;
>+
>     vfio_pci_cdev_open(device, bdf);
>-    return __vfio_device_bind_iommufd(device->fd,
>-                      device->iommu->iommufd, vf_token);
>+    ret = __vfio_device_bind_iommufd(device->fd,
>+                     device->iommu->iommufd, vf_token);
>+
>+    /* On success, __vfio_device_bind_iommufd() returns the device ID. */
>+    return ret < 0 ? ret : 0;
> }
>
> static int device_init(const char *bdf, struct iommu *iommu,

Will update.
>
>>  }
>>
>> -static void vfio_device_bind_iommufd(int device_fd, int iommufd,
>> -				     const char *vf_token)
>> +int vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token)
>>  {
>>  	int ret = __vfio_device_bind_iommufd(device_fd, iommufd, vf_token);
>>
>> -	VFIO_ASSERT_EQ(ret, 0, "Failed VFIO_DEVICE_BIND_IOMMUFD ioctl\n");
>> +	VFIO_ASSERT_GE(ret, 0, "Failed VFIO_DEVICE_BIND_IOMMUFD ioctl\n");
>> +	return ret;
>>  }
>>
>>  static void vfio_device_attach_iommufd_pt(int device_fd, u32 pt_id)
>> @@ -503,10 +503,25 @@ static void vfio_pci_iommufd_setup(struct vfio_pci_device *device,
>>  				   const char *bdf, const char *vf_token)
>>  {
>>  	vfio_pci_cdev_open(device, bdf);
>> -	vfio_device_bind_iommufd(device->fd, device->iommu->iommufd, vf_token);
>> +	device->dev_id = vfio_device_bind_iommufd(device->fd, device->iommu->iommufd, vf_token);
>>  	vfio_device_attach_iommufd_pt(device->fd, device->iommu->ioas_id);
>>  }
>>
>> +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;
>> +
>> +	VFIO_ASSERT_NE(pt_id, 0);
>
>Is this assert needed? (Will vfio_device_attach_iommufd_pt() not handle
>this appropriately?)

It should be covered by the assert for the ioctl. I will remove this.
>
>> +	vfio_device_attach_iommufd_pt(device->fd, pt_id);
>> +	device->iommu = iommu;
>> +}
>> +
>>  struct vfio_pci_device *vfio_pci_device_alloc(const char *bdf, struct iommu *iommu)
>>  {
>>  	struct vfio_pci_device *device;
>> --
>> 2.56.0.rc1.315.gc6ed9934b7-goog
>>

Thanks,
Sami