Re: [PATCH 14/14] iommufd/selftest: Add test to verify iommufd preservation

[Samiullah Khawaja <skhawaja@google.com>]

On Mon, Mar 23, 2026 at 03:18:39PM -0700, Vipin Sharma wrote:
>On Tue, Feb 03, 2026 at 10:09:48PM +0000, Samiullah Khawaja wrote:
>> Test iommufd preservation by setting up an iommufd and vfio cdev and
>> preserve it across live update. Test takes VFIO cdev path of a device
>> bound to vfio-pci driver and binds it to an iommufd being preserved. It
>> also preserves the vfio cdev so the iommufd state associated with it is
>> also preserved.
>>
>> The restore path is tested by restoring the preserved vfio cdev only.
>> Test tries to finish the session without restoring iommufd and confirms
>> that it fails.
>
>May be alos add more details that VFIO bind will also fail, it will return EPERM
>due to ...

Agreed will add more detail.
>
>> diff --git a/tools/testing/selftests/iommu/Makefile b/tools/testing/selftests/iommu/Makefile
>> +$(TEST_GEN_PROGS_EXTENDED): %: %.o $(LIBLIVEUPDATE_O)
>> +	$(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $(TARGET_ARCH) $< $(LIBLIVEUPDATE_O) $(LDLIBS) -static -o $@
>
>Why static? User can pass static flag through EXTRA_CFLAGS.

I will remove this.
>
>> diff --git a/tools/testing/selftests/iommu/iommufd_liveupdate.c b/tools/testing/selftests/iommu/iommufd_liveupdate.c
>> +
>> +#define ksft_assert(condition) \
>> +	do { if (!(condition)) \
>> +	ksft_exit_fail_msg("Failed: %s at %s %d: %s\n", \
>> +	#condition, __FILE__, __LINE__, strerror(errno)); } while (0)
>
>Please add indentation to the code.

Will do.
>
>> +
>> +int setup_cdev(const char *vfio_cdev_path)
>
>Nit: s/setup_cdev/open_cdev

Will change this to open_cdev.
>
>Since you are using open_iommufd() name below, this will make it
>consistent as there not setup here apart from opening.
>
>> +int setup_iommufd(int iommufd, int memfd, int cdev_fd, int hwpt_token)
>> +	bind.iommufd = iommufd;
>> +	ret = ioctl(cdev_fd, VFIO_DEVICE_BIND_IOMMUFD, &bind);
>> +	ksft_assert(!ret);
>> +
>> +	ret = ioctl(iommufd, IOMMU_IOAS_ALLOC, &alloc_data);
>> +	ksft_assert(!ret);
>> +
>> +	hwpt_alloc.dev_id = bind.out_devid;
>> +	hwpt_alloc.pt_id = alloc_data.out_ioas_id;
>> +	ret = ioctl(iommufd, IOMMU_HWPT_ALLOC, &hwpt_alloc);
>> +	ksft_assert(!ret);
>> +
>> +	attach_data.pt_id = hwpt_alloc.out_hwpt_id;
>> +	ret = ioctl(cdev_fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &attach_data);
>> +	ksft_assert(!ret);
>> +
>> +	map_file.ioas_id = alloc_data.out_ioas_id;
>> +	ret = ioctl(iommufd, IOMMU_IOAS_MAP_FILE, &map_file);
>> +	ksft_assert(!ret);
>> +
>> +	set_preserve.hwpt_id = attach_data.pt_id;
>> +	ret = ioctl(iommufd, IOMMU_HWPT_LU_SET_PRESERVE, &set_preserve);
>> +	ksft_assert(!ret);
>> +
>> +	return ret;
>> +}
>
>iommufd_utils.h has functions defined for iommufd ioctls. I think we
>should use those functions here and if related to iommufd are not
>present we should add there.

iommufd_utils.h has utilities to use with mock testing, so those cannot
be used here. I will see if I can make separate helper functions for
these and use those here.
>
>> +int main(int argc, char *argv[])
>> +{
>> +	int iommufd, cdev_fd, memfd, luo, session, ret;
>> +	const int token = 0x123456;
>> +	const int cdev_token = 0x654321;
>> +	const int hwpt_token = 0x789012;
>> +	const int memfd_token = 0x890123;
>> +
>> +	if (argc < 2) {
>> +		printf("Usage: ./iommufd_liveupdate <vfio_cdev_path>\n");
>> +		return 1;
>> +	}
>> +
>> +	luo = luo_open_device();
>> +	ksft_assert(luo > 0);
>> +
>> +	session = luo_retrieve_session(luo, "iommufd-test");
>> +	if (session == -ENOENT) {
>> +		session = luo_create_session(luo, "iommufd-test");
>> +
>> +		iommufd = open_iommufd();
>> +		memfd = create_sealed_memfd(SZ_1M);
>> +		cdev_fd = setup_cdev(argv[1]);
>> +
>> +		ret = setup_iommufd(iommufd, memfd, cdev_fd, hwpt_token);
>> +		ksft_assert(!ret);
>> +
>> +		/* Cannot preserve cdev without iommufd */
>> +		ret = luo_session_preserve_fd(session, cdev_fd, cdev_token);
>> +		ksft_assert(ret);
>> +
>> +		/* Cannot preserve iommufd without preserving memfd. */
>> +		ret = luo_session_preserve_fd(session, iommufd, token);
>> +		ksft_assert(ret);
>> +
>> +		ret = luo_session_preserve_fd(session, memfd, memfd_token);
>> +		ksft_assert(!ret);
>> +
>> +		ret = luo_session_preserve_fd(session, iommufd, token);
>> +		ksft_assert(!ret);
>> +
>> +		ret = luo_session_preserve_fd(session, cdev_fd, cdev_token);
>> +		ksft_assert(!ret);
>> +
>
>All of these ksft_assert are hurting my eyes:) I like the approach in
>VFIO where library APIs does validation and test code only check actual
>things needed.
>
>Should we at least create a common function to combine both
>luo_session_preserve() and ksft_assert()?

Agreed. Will rework these.
>

Thanks,
Sami