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

[Samiullah Khawaja <skhawaja@google.com>]

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 <skhawaja@google.com>
>> ---
>>  .../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