[PATCH 0/4] vfio: selftest: Add SR-IOV UAPI test

[PATCH 0/4] vfio: selftest: Add SR-IOV UAPI test

[Raghavendra Rao Ananta]

Hello,

This series adds a vfio selftest, vfio_pci_sriov_uapi_test.c, to get some
coverage on SR-IOV UAPI handling. Specifically, it includes the
following cases that iterates over all the iommu modes:
 - Setting correct/incorrect/NULL tokens during device init.
 - Close the PF device immediately after setting the token.
 - Change/override the PF's token after device init.

The test takes care of creating/setting up the VF device, and hence, it
can be executed like any other test, simply by passing the PF's BDF to
run.sh. For example,

./run.sh -d 0000:16:00.1 -- ./vfio_pci_sriov_uapi_test
+ echo "0" > /sys/bus/pci/devices/0000:16:00.1/sriov_numvfsdddd
+ echo "vfio-pci" > /sys/bus/pci/devices/0000:16:00.1/driver_override
+ echo "0000:16:00.1" > /sys/bus/pci/drivers/vfio-pci/bind

TAP version 13
1..45
Starting 45 tests from 15 test cases.
  RUN  vfio_pci_sriov_uapi_test.vfio_type1_iommu_same_uuid.init_token_match
    OK vfio_pci_sriov_uapi_test.vfio_type1_iommu_same_uuid.init_token_match
ok 1 vfio_pci_sriov_uapi_test.vfio_type1_iommu_same_uuid.init_token_match
  RUN vfio_pci_sriov_uapi_test.vfio_type1_iommu_same_uuid.pf_early_close
   OK vfio_pci_sriov_uapi_test.vfio_type1_iommu_same_uuid.pf_early_close
ok 2 vfio_pci_sriov_uapi_test.vfio_type1_iommu_same_uuid.pf_early_close
  RUN vfio_pci_sriov_uapi_test.vfio_type1_iommu_same_uuid.override_token
   OK vfio_pci_sriov_uapi_test.vfio_type1_iommu_same_uuid.override_token
[...]
  RUN vfio_pci_sriov_uapi_test.iommufd_null_uuid.override_token ...
   OK vfio_pci_sriov_uapi_test.iommufd_null_uuid.override_token
ok 45 vfio_pci_sriov_uapi_test.iommufd_null_uuid.override_token
PASSED: 45 / 45 tests passed.

The series this dependent on another series that provides fixes in the
IOMMUFD's vf_token handling [1].

Thank you.
Raghavendra

[1]: https://lore.kernel.org/all/20251031170603.2260022-1-rananta@google.com/

Raghavendra Rao Ananta (4):
  vfio: selftests: Add support for passing vf_token in device init
  vfio: selftests: Export vfio_pci_device functions
  vfio: selftests: Add helper to set/override a vf_token
  vfio: selftests: Add tests to validate SR-IOV UAPI

 tools/testing/selftests/vfio/Makefile         |   1 +
 .../selftests/vfio/lib/include/vfio_util.h    |  19 +-
 tools/testing/selftests/vfio/lib/libvfio.mk   |   4 +-
 .../selftests/vfio/lib/vfio_pci_device.c      | 151 ++++++++++--
 .../selftests/vfio/vfio_dma_mapping_test.c    |   2 +-
 .../selftests/vfio/vfio_pci_device_test.c     |   4 +-
 .../selftests/vfio/vfio_pci_driver_test.c     |   4 +-
 .../selftests/vfio/vfio_pci_sriov_uapi_test.c | 220 ++++++++++++++++++
 8 files changed, 377 insertions(+), 28 deletions(-)
 create mode 100644 tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c


base-commit: 211ddde0823f1442e4ad052a2f30f050145ccada
-- 
2.51.2.997.g839fc31de9-goog

[PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[Raghavendra Rao Ananta]

A UUID is normally set as a vf_token to correspond the VFs with the
PFs, if they are both bound by the vfio-pci driver. This is true for
iommufd-based approach and container-based approach. The token can be
set either during device creation (VFIO_GROUP_GET_DEVICE_FD) in
container-based approach or during iommu bind (VFIO_DEVICE_BIND_IOMMUFD)
in the iommu-fd case. Hence, extend the vfio_pci_device_init() helper to
accept vf_token during device setup.

The tests depending on vfio_pci_device_init() are adjusted accordingly
and no functional changes are expected. A later patch will add tests
that passes actual token to test the UAPI.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../selftests/vfio/lib/include/vfio_util.h    |  4 +-
 tools/testing/selftests/vfio/lib/libvfio.mk   |  4 +-
 .../selftests/vfio/lib/vfio_pci_device.c      | 60 ++++++++++++++++---
 .../selftests/vfio/vfio_dma_mapping_test.c    |  2 +-
 .../selftests/vfio/vfio_pci_device_test.c     |  4 +-
 .../selftests/vfio/vfio_pci_driver_test.c     |  4 +-
 6 files changed, 62 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/vfio/lib/include/vfio_util.h b/tools/testing/selftests/vfio/lib/include/vfio_util.h
index ed31606e01b78..b01068d98fdab 100644
--- a/tools/testing/selftests/vfio/lib/include/vfio_util.h
+++ b/tools/testing/selftests/vfio/lib/include/vfio_util.h
@@ -202,7 +202,9 @@ const char *vfio_pci_get_cdev_path(const char *bdf);
 
 extern const char *default_iommu_mode;
 
-struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_mode);
+struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
+					      const char *iommu_mode,
+					      const char *vf_token);
 void vfio_pci_device_cleanup(struct vfio_pci_device *device);
 void vfio_pci_device_reset(struct vfio_pci_device *device);
 
diff --git a/tools/testing/selftests/vfio/lib/libvfio.mk b/tools/testing/selftests/vfio/lib/libvfio.mk
index 5d11c3a89a28e..2dc85c41ffb4b 100644
--- a/tools/testing/selftests/vfio/lib/libvfio.mk
+++ b/tools/testing/selftests/vfio/lib/libvfio.mk
@@ -18,7 +18,9 @@ $(shell mkdir -p $(LIBVFIO_O_DIRS))
 
 CFLAGS += -I$(VFIO_DIR)/lib/include
 
+LDLIBS += -luuid
+
 $(LIBVFIO_O): $(OUTPUT)/%.o : $(VFIO_DIR)/%.c
-	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< -o $@
+	$(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c $< $(LDLIBS) -o $@
 
 EXTRA_CLEAN += $(LIBVFIO_O)
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index 0921b2451ba5c..3f7be8d371d06 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -11,6 +11,7 @@
 #include <sys/mman.h>
 
 #include <uapi/linux/types.h>
+#include <uuid/uuid.h>
 #include <linux/limits.h>
 #include <linux/mman.h>
 #include <linux/types.h>
@@ -22,6 +23,8 @@
 
 #define PCI_SYSFS_PATH	"/sys/bus/pci/devices"
 
+#define VF_TOKEN_ARG "vf_token="
+
 #define ioctl_assert(_fd, _op, _arg) do {						       \
 	void *__arg = (_arg);								       \
 	int __ret = ioctl((_fd), (_op), (__arg));					       \
@@ -328,7 +331,37 @@ static void vfio_pci_group_setup(struct vfio_pci_device *device, const char *bdf
 	ioctl_assert(device->group_fd, VFIO_GROUP_SET_CONTAINER, &device->container_fd);
 }
 
-static void vfio_pci_container_setup(struct vfio_pci_device *device, const char *bdf)
+static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
+					      const char *bdf,
+					      const char *vf_token)
+{
+	char *arg = (char *) bdf;
+
+	/*
+	 * If a vf_token exists, argument to VFIO_GROUP_GET_DEVICE_FD
+	 * will be in the form of the following example:
+	 * "0000:04:10.0 vf_token=bd8d9d2b-5a5f-4f5a-a211-f591514ba1f3"
+	 */
+	if (vf_token) {
+		size_t sz = strlen(bdf) + strlen(" "VF_TOKEN_ARG) +
+			    strlen(vf_token) + 1;
+
+		arg = calloc(1, sz);
+		VFIO_ASSERT_NOT_NULL(arg);
+
+		snprintf(arg, sz, "%s %s%s", bdf, VF_TOKEN_ARG, vf_token);
+	}
+
+	device->fd = ioctl(device->group_fd, VFIO_GROUP_GET_DEVICE_FD, arg);
+
+	if (vf_token)
+		free((void *) arg);
+
+	VFIO_ASSERT_GE(device->fd, 0);
+}
+
+static void vfio_pci_container_setup(struct vfio_pci_device *device,
+				      const char *bdf, const char *vf_token)
 {
 	unsigned long iommu_type = device->iommu_mode->iommu_type;
 	const char *path = device->iommu_mode->container_path;
@@ -348,8 +381,7 @@ static void vfio_pci_container_setup(struct vfio_pci_device *device, const char
 
 	ioctl_assert(device->container_fd, VFIO_SET_IOMMU, (void *)iommu_type);
 
-	device->fd = ioctl(device->group_fd, VFIO_GROUP_GET_DEVICE_FD, bdf);
-	VFIO_ASSERT_GE(device->fd, 0);
+	vfio_pci_container_get_device_fd(device, bdf, vf_token);
 }
 
 static void vfio_pci_device_setup(struct vfio_pci_device *device)
@@ -456,12 +488,19 @@ static const struct vfio_iommu_mode *lookup_iommu_mode(const char *iommu_mode)
 	VFIO_FAIL("Unrecognized IOMMU mode: %s\n", iommu_mode);
 }
 
-static void vfio_device_bind_iommufd(int device_fd, int iommufd)
+static void vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token)
 {
 	struct vfio_device_bind_iommufd args = {
 		.argsz = sizeof(args),
 		.iommufd = iommufd,
 	};
+	uuid_t token_uuid = {0};
+
+	if (vf_token) {
+		VFIO_ASSERT_EQ(uuid_parse(vf_token, token_uuid), 0);
+		args.flags = VFIO_DEVICE_BIND_FLAG_TOKEN;
+		args.token_uuid_ptr = (u64) token_uuid;
+	}
 
 	ioctl_assert(device_fd, VFIO_DEVICE_BIND_IOMMUFD, &args);
 }
@@ -486,7 +525,8 @@ static void vfio_device_attach_iommufd_pt(int device_fd, u32 pt_id)
 	ioctl_assert(device_fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &args);
 }
 
-static void vfio_pci_iommufd_setup(struct vfio_pci_device *device, const char *bdf)
+static void vfio_pci_iommufd_setup(struct vfio_pci_device *device,
+				    const char *bdf, const char *vf_token)
 {
 	const char *cdev_path = vfio_pci_get_cdev_path(bdf);
 
@@ -502,12 +542,14 @@ static void vfio_pci_iommufd_setup(struct vfio_pci_device *device, const char *b
 	device->iommufd = open("/dev/iommu", O_RDWR);
 	VFIO_ASSERT_GT(device->iommufd, 0);
 
-	vfio_device_bind_iommufd(device->fd, device->iommufd);
+	vfio_device_bind_iommufd(device->fd, device->iommufd, vf_token);
 	device->ioas_id = iommufd_ioas_alloc(device->iommufd);
 	vfio_device_attach_iommufd_pt(device->fd, device->ioas_id);
 }
 
-struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_mode)
+struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
+					      const char *iommu_mode,
+					      const char *vf_token)
 {
 	struct vfio_pci_device *device;
 
@@ -519,9 +561,9 @@ struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_
 	device->iommu_mode = lookup_iommu_mode(iommu_mode);
 
 	if (device->iommu_mode->container_path)
-		vfio_pci_container_setup(device, bdf);
+		vfio_pci_container_setup(device, bdf, vf_token);
 	else
-		vfio_pci_iommufd_setup(device, bdf);
+		vfio_pci_iommufd_setup(device, bdf, vf_token);
 
 	vfio_pci_device_setup(device);
 	vfio_pci_driver_probe(device);
diff --git a/tools/testing/selftests/vfio/vfio_dma_mapping_test.c b/tools/testing/selftests/vfio/vfio_dma_mapping_test.c
index ab19c54a774da..3c53b808f7f87 100644
--- a/tools/testing/selftests/vfio/vfio_dma_mapping_test.c
+++ b/tools/testing/selftests/vfio/vfio_dma_mapping_test.c
@@ -114,7 +114,7 @@ FIXTURE_VARIANT_ADD_ALL_IOMMU_MODES(anonymous_hugetlb_1gb, SZ_1G, MAP_HUGETLB |
 
 FIXTURE_SETUP(vfio_dma_mapping_test)
 {
-	self->device = vfio_pci_device_init(device_bdf, variant->iommu_mode);
+	self->device = vfio_pci_device_init(device_bdf, variant->iommu_mode, NULL);
 }
 
 FIXTURE_TEARDOWN(vfio_dma_mapping_test)
diff --git a/tools/testing/selftests/vfio/vfio_pci_device_test.c b/tools/testing/selftests/vfio/vfio_pci_device_test.c
index 7a270698e4d24..ebf7fd3d1cf70 100644
--- a/tools/testing/selftests/vfio/vfio_pci_device_test.c
+++ b/tools/testing/selftests/vfio/vfio_pci_device_test.c
@@ -28,7 +28,7 @@ FIXTURE(vfio_pci_device_test) {
 
 FIXTURE_SETUP(vfio_pci_device_test)
 {
-	self->device = vfio_pci_device_init(device_bdf, default_iommu_mode);
+	self->device = vfio_pci_device_init(device_bdf, default_iommu_mode, NULL);
 }
 
 FIXTURE_TEARDOWN(vfio_pci_device_test)
@@ -116,7 +116,7 @@ FIXTURE_VARIANT_ADD(vfio_pci_irq_test, msix) {
 
 FIXTURE_SETUP(vfio_pci_irq_test)
 {
-	self->device = vfio_pci_device_init(device_bdf, default_iommu_mode);
+	self->device = vfio_pci_device_init(device_bdf, default_iommu_mode, NULL);
 }
 
 FIXTURE_TEARDOWN(vfio_pci_irq_test)
diff --git a/tools/testing/selftests/vfio/vfio_pci_driver_test.c b/tools/testing/selftests/vfio/vfio_pci_driver_test.c
index 2dbd70b7db627..cfbaa05dda884 100644
--- a/tools/testing/selftests/vfio/vfio_pci_driver_test.c
+++ b/tools/testing/selftests/vfio/vfio_pci_driver_test.c
@@ -71,7 +71,7 @@ FIXTURE_SETUP(vfio_pci_driver_test)
 {
 	struct vfio_pci_driver *driver;
 
-	self->device = vfio_pci_device_init(device_bdf, variant->iommu_mode);
+	self->device = vfio_pci_device_init(device_bdf, variant->iommu_mode, NULL);
 
 	driver = &self->device->driver;
 
@@ -233,7 +233,7 @@ int main(int argc, char *argv[])
 
 	device_bdf = vfio_selftests_get_bdf(&argc, argv);
 
-	device = vfio_pci_device_init(device_bdf, default_iommu_mode);
+	device = vfio_pci_device_init(device_bdf, default_iommu_mode, NULL);
 	if (!device->driver.ops) {
 		fprintf(stderr, "No driver found for device %s\n", device_bdf);
 		return KSFT_SKIP;
-- 
2.51.2.997.g839fc31de9-goog

[PATCH 2/4] vfio: selftests: Export vfio_pci_device functions

[Raghavendra Rao Ananta]

Refactor and make the functions called under device initialization
public. A later patch adds a test that calls these functions to validate
the UAPI of SR-IOV devices. Opportunistically, to test the success
and failure cases of the UAPI, split the functions dealing with
VFIO_GROUP_GET_DEVICE_FD and VFIO_DEVICE_BIND_IOMMUFD into a core
function and another one that asserts the ioctl. The former will be
used for testing the SR-IOV UAPI, hence only export these.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../selftests/vfio/lib/include/vfio_util.h    | 13 ++++
 .../selftests/vfio/lib/vfio_pci_device.c      | 75 +++++++++++++------
 2 files changed, 67 insertions(+), 21 deletions(-)

diff --git a/tools/testing/selftests/vfio/lib/include/vfio_util.h b/tools/testing/selftests/vfio/lib/include/vfio_util.h
index b01068d98fdab..2baf12a888e67 100644
--- a/tools/testing/selftests/vfio/lib/include/vfio_util.h
+++ b/tools/testing/selftests/vfio/lib/include/vfio_util.h
@@ -294,4 +294,17 @@ void vfio_pci_driver_memcpy_start(struct vfio_pci_device *device,
 int vfio_pci_driver_memcpy_wait(struct vfio_pci_device *device);
 void vfio_pci_driver_send_msi(struct vfio_pci_device *device);
 
+const char *iommu_mode_container_path(const char *iommu_mode);
+const struct vfio_iommu_mode *lookup_iommu_mode(const char *iommu_mode);
+
+void vfio_container_open(struct vfio_pci_device *device);
+void vfio_pci_group_setup(struct vfio_pci_device *device, const char *bdf);
+void vfio_container_set_iommu(struct vfio_pci_device *device);
+void __vfio_container_get_device_fd(struct vfio_pci_device *device,
+				    const char *bdf, const char *vf_token);
+
+void vfio_pci_iommufd_cdev_open(struct vfio_pci_device *device, const char *bdf);
+void vfio_pci_iommufd_iommudev_open(struct vfio_pci_device *device);
+int __vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token);
+
 #endif /* SELFTESTS_VFIO_LIB_INCLUDE_VFIO_UTIL_H */
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index 3f7be8d371d06..7a1547e365870 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -311,7 +311,7 @@ static unsigned int vfio_pci_get_group_from_dev(const char *bdf)
 	return group;
 }
 
-static void vfio_pci_group_setup(struct vfio_pci_device *device, const char *bdf)
+void vfio_pci_group_setup(struct vfio_pci_device *device, const char *bdf)
 {
 	struct vfio_group_status group_status = {
 		.argsz = sizeof(group_status),
@@ -331,9 +331,8 @@ static void vfio_pci_group_setup(struct vfio_pci_device *device, const char *bdf
 	ioctl_assert(device->group_fd, VFIO_GROUP_SET_CONTAINER, &device->container_fd);
 }
 
-static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
-					      const char *bdf,
-					      const char *vf_token)
+void __vfio_container_get_device_fd(struct vfio_pci_device *device,
+				     const char *bdf, const char *vf_token)
 {
 	char *arg = (char *) bdf;
 
@@ -356,32 +355,46 @@ static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
 
 	if (vf_token)
 		free((void *) arg);
+}
 
+static void vfio_container_get_device_fd(struct vfio_pci_device *device,
+					  const char *bdf,
+					  const char *vf_token)
+{
+	__vfio_container_get_device_fd(device, bdf, vf_token);
 	VFIO_ASSERT_GE(device->fd, 0);
 }
 
-static void vfio_pci_container_setup(struct vfio_pci_device *device,
-				      const char *bdf, const char *vf_token)
+void vfio_container_set_iommu(struct vfio_pci_device *device)
 {
 	unsigned long iommu_type = device->iommu_mode->iommu_type;
+	int ret;
+
+	ret = ioctl(device->container_fd, VFIO_CHECK_EXTENSION, iommu_type);
+	VFIO_ASSERT_GT(ret, 0, "VFIO IOMMU type %lu not supported\n", iommu_type);
+
+	ioctl_assert(device->container_fd, VFIO_SET_IOMMU, (void *)iommu_type);
+}
+
+void vfio_container_open(struct vfio_pci_device *device)
+{
 	const char *path = device->iommu_mode->container_path;
 	int version;
-	int ret;
 
 	device->container_fd = open(path, O_RDWR);
 	VFIO_ASSERT_GE(device->container_fd, 0, "open(%s) failed\n", path);
 
 	version = ioctl(device->container_fd, VFIO_GET_API_VERSION);
 	VFIO_ASSERT_EQ(version, VFIO_API_VERSION, "Unsupported version: %d\n", version);
+}
 
+static void vfio_pci_container_setup(struct vfio_pci_device *device,
+				      const char *bdf, const char *vf_token)
+{
+	vfio_container_open(device);
 	vfio_pci_group_setup(device, bdf);
-
-	ret = ioctl(device->container_fd, VFIO_CHECK_EXTENSION, iommu_type);
-	VFIO_ASSERT_GT(ret, 0, "VFIO IOMMU type %lu not supported\n", iommu_type);
-
-	ioctl_assert(device->container_fd, VFIO_SET_IOMMU, (void *)iommu_type);
-
-	vfio_pci_container_get_device_fd(device, bdf, vf_token);
+	vfio_container_set_iommu(device);
+	vfio_container_get_device_fd(device, bdf, vf_token);
 }
 
 static void vfio_pci_device_setup(struct vfio_pci_device *device)
@@ -471,7 +484,7 @@ static const struct vfio_iommu_mode iommu_modes[] = {
 
 const char *default_iommu_mode = "iommufd";
 
-static const struct vfio_iommu_mode *lookup_iommu_mode(const char *iommu_mode)
+const struct vfio_iommu_mode *lookup_iommu_mode(const char *iommu_mode)
 {
 	int i;
 
@@ -488,7 +501,12 @@ static const struct vfio_iommu_mode *lookup_iommu_mode(const char *iommu_mode)
 	VFIO_FAIL("Unrecognized IOMMU mode: %s\n", iommu_mode);
 }
 
-static void vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token)
+const char *iommu_mode_container_path(const char *iommu_mode)
+{
+	return lookup_iommu_mode(iommu_mode)->container_path;
+}
+
+int __vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token)
 {
 	struct vfio_device_bind_iommufd args = {
 		.argsz = sizeof(args),
@@ -502,7 +520,14 @@ static void vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_
 		args.token_uuid_ptr = (u64) token_uuid;
 	}
 
-	ioctl_assert(device_fd, VFIO_DEVICE_BIND_IOMMUFD, &args);
+	return ioctl(device_fd, VFIO_DEVICE_BIND_IOMMUFD, &args);
+}
+
+static void 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");
 }
 
 static u32 iommufd_ioas_alloc(int iommufd)
@@ -525,23 +550,31 @@ static void vfio_device_attach_iommufd_pt(int device_fd, u32 pt_id)
 	ioctl_assert(device_fd, VFIO_DEVICE_ATTACH_IOMMUFD_PT, &args);
 }
 
-static void vfio_pci_iommufd_setup(struct vfio_pci_device *device,
-				    const char *bdf, const char *vf_token)
+void vfio_pci_iommufd_cdev_open(struct vfio_pci_device *device, const char *bdf)
 {
 	const char *cdev_path = vfio_pci_get_cdev_path(bdf);
 
 	device->fd = open(cdev_path, O_RDWR);
 	VFIO_ASSERT_GE(device->fd, 0);
 	free((void *)cdev_path);
+}
 
+void vfio_pci_iommufd_iommudev_open(struct vfio_pci_device *device)
+{
 	/*
 	 * 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.
 	 */
-	device->iommufd = open("/dev/iommu", O_RDWR);
-	VFIO_ASSERT_GT(device->iommufd, 0);
+	 device->iommufd = open("/dev/iommu", O_RDWR);
+	 VFIO_ASSERT_GT(device->iommufd, 0);
+}
 
+static void vfio_pci_iommufd_setup(struct vfio_pci_device *device,
+				    const char *bdf, const char *vf_token)
+{
+	vfio_pci_iommufd_cdev_open(device, bdf);
+	vfio_pci_iommufd_iommudev_open(device);
 	vfio_device_bind_iommufd(device->fd, device->iommufd, vf_token);
 	device->ioas_id = iommufd_ioas_alloc(device->iommufd);
 	vfio_device_attach_iommufd_pt(device->fd, device->ioas_id);
-- 
2.51.2.997.g839fc31de9-goog

[PATCH 3/4] vfio: selftests: Add helper to set/override a vf_token

[Raghavendra Rao Ananta]

Not only at init, but a vf_token can also be set via the
VFIO_DEVICE_FEATURE ioctl, by setting the
VFIO_DEVICE_FEATURE_PCI_VF_TOKEN flag. Add an API to utilize this
functionality from the test code.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 .../selftests/vfio/lib/include/vfio_util.h    |  2 ++
 .../selftests/vfio/lib/vfio_pci_device.c      | 34 +++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/tools/testing/selftests/vfio/lib/include/vfio_util.h b/tools/testing/selftests/vfio/lib/include/vfio_util.h
index 2baf12a888e67..f0a12646456a9 100644
--- a/tools/testing/selftests/vfio/lib/include/vfio_util.h
+++ b/tools/testing/selftests/vfio/lib/include/vfio_util.h
@@ -307,4 +307,6 @@ void vfio_pci_iommufd_cdev_open(struct vfio_pci_device *device, const char *bdf)
 void vfio_pci_iommufd_iommudev_open(struct vfio_pci_device *device);
 int __vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token);
 
+void vfio_device_set_vf_token(int fd, const char *vf_token);
+
 #endif /* SELFTESTS_VFIO_LIB_INCLUDE_VFIO_UTIL_H */
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index 7a1547e365870..c0fa4e27a96ef 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -144,6 +144,40 @@ static void vfio_pci_irq_get(struct vfio_pci_device *device, u32 index,
 	ioctl_assert(device->fd, VFIO_DEVICE_GET_IRQ_INFO, irq_info);
 }
 
+static int vfio_device_feature_ioctl(int fd, u32 flags, void *data,
+				     size_t data_size)
+{
+	u8 buffer[sizeof(struct vfio_device_feature) + data_size] = {};
+	struct vfio_device_feature *feature = (void *)buffer;
+
+	memcpy(feature->data, data, data_size);
+
+	feature->argsz = sizeof(buffer);
+	feature->flags = flags;
+
+	return ioctl(fd, VFIO_DEVICE_FEATURE, feature);
+}
+
+static void vfio_device_feature_set(int fd, u16 feature, void *data, size_t data_size)
+{
+	u32 flags = VFIO_DEVICE_FEATURE_SET | feature;
+	int ret;
+
+	ret = vfio_device_feature_ioctl(fd, flags, data, data_size);
+	VFIO_ASSERT_EQ(ret, 0, "Failed to set feature %u\n", feature);
+}
+
+void vfio_device_set_vf_token(int fd, const char *vf_token)
+{
+	uuid_t token_uuid = {0};
+
+	VFIO_ASSERT_NOT_NULL(vf_token, "vf_token is NULL");
+	VFIO_ASSERT_EQ(uuid_parse(vf_token, token_uuid), 0);
+
+	vfio_device_feature_set(fd, VFIO_DEVICE_FEATURE_PCI_VF_TOKEN,
+				token_uuid, sizeof(uuid_t));
+}
+
 static void vfio_iommu_dma_map(struct vfio_pci_device *device,
 			       struct vfio_dma_region *region)
 {
-- 
2.51.2.997.g839fc31de9-goog

[PATCH 4/4] vfio: selftests: Add tests to validate SR-IOV UAPI

[Raghavendra Rao Ananta]

Add a test to validate the SR-IOV UAPI, including the following cases,
iterating over all the IOMMU modes currently supported:
 - Setting correct/incorrect/NULL tokens during device init.
 - Close the PF device immediately after setting the token.
 - Change/override the PF's token after device init.

Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
---
 tools/testing/selftests/vfio/Makefile         |   1 +
 .../selftests/vfio/vfio_pci_sriov_uapi_test.c | 220 ++++++++++++++++++
 2 files changed, 221 insertions(+)
 create mode 100644 tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c

diff --git a/tools/testing/selftests/vfio/Makefile b/tools/testing/selftests/vfio/Makefile
index 324ba0175a333..e9fad0acf4d13 100644
--- a/tools/testing/selftests/vfio/Makefile
+++ b/tools/testing/selftests/vfio/Makefile
@@ -3,6 +3,7 @@ TEST_GEN_PROGS += vfio_dma_mapping_test
 TEST_GEN_PROGS += vfio_iommufd_setup_test
 TEST_GEN_PROGS += vfio_pci_device_test
 TEST_GEN_PROGS += vfio_pci_driver_test
+TEST_GEN_PROGS += vfio_pci_sriov_uapi_test
 TEST_PROGS_EXTENDED := run.sh
 include ../lib.mk
 include lib/libvfio.mk
diff --git a/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c b/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c
new file mode 100644
index 0000000000000..4d63359053230
--- /dev/null
+++ b/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <sys/ioctl.h>
+#include <linux/limits.h>
+
+#include <vfio_util.h>
+
+#include "../kselftest_harness.h"
+
+#define PCI_SYSFS_PATH "/sys/bus/pci/devices"
+
+#define UUID_1 "52ac9bff-3a88-4fbd-901a-0d767c3b6c97"
+#define UUID_2 "88594674-90a0-47a9-aea8-9d9b352ac08a"
+
+static const char *pf_dev_bdf;
+static char vf_dev_bdf[16];
+
+struct vfio_pci_device *pf_device;
+struct vfio_pci_device *vf_device;
+
+static void test_vfio_pci_container_setup(struct vfio_pci_device *device,
+					   const char *bdf,
+					   const char *vf_token)
+{
+	vfio_container_open(device);
+	vfio_pci_group_setup(device, bdf);
+	vfio_container_set_iommu(device);
+	__vfio_container_get_device_fd(device, bdf, vf_token);
+}
+
+static int test_vfio_pci_iommufd_setup(struct vfio_pci_device *device,
+					const char *bdf, const char *vf_token)
+{
+	vfio_pci_iommufd_cdev_open(device, bdf);
+	vfio_pci_iommufd_iommudev_open(device);
+	return __vfio_device_bind_iommufd(device->fd, device->iommufd, vf_token);
+}
+
+static struct vfio_pci_device *test_vfio_pci_device_init(const char *bdf,
+							  const char *iommu_mode,
+							  const char *vf_token,
+							  int *out_ret)
+{
+	struct vfio_pci_device *device;
+
+	device = calloc(1, sizeof(*device));
+	VFIO_ASSERT_NOT_NULL(device);
+
+	device->iommu_mode = lookup_iommu_mode(iommu_mode);
+
+	if (iommu_mode_container_path(iommu_mode)) {
+		test_vfio_pci_container_setup(device, bdf, vf_token);
+		/* The device fd will be -1 in case of mismatched tokens */
+		*out_ret = (device->fd < 0);
+	} else {
+		*out_ret = test_vfio_pci_iommufd_setup(device, bdf, vf_token);
+	}
+
+	return device;
+}
+
+static void test_vfio_pci_device_cleanup(struct vfio_pci_device *device)
+{
+	if (device->fd > 0)
+		VFIO_ASSERT_EQ(close(device->fd), 0);
+
+	if (device->iommufd) {
+		VFIO_ASSERT_EQ(close(device->iommufd), 0);
+	} else {
+		VFIO_ASSERT_EQ(close(device->group_fd), 0);
+		VFIO_ASSERT_EQ(close(device->container_fd), 0);
+	}
+
+	free(device);
+}
+
+FIXTURE(vfio_pci_sriov_uapi_test) {};
+
+FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
+{
+	char vf_path[PATH_MAX] = {0};
+	char path[PATH_MAX] = {0};
+	unsigned int nr_vfs;
+	char buf[32] = {0};
+	int ret;
+	int fd;
+
+	/* Check if SR-IOV is supported by the device */
+	snprintf(path, PATH_MAX, "%s/%s/sriov_totalvfs", PCI_SYSFS_PATH, pf_dev_bdf);
+	fd = open(path, O_RDONLY);
+	if (fd < 0) {
+		fprintf(stderr, "SR-IOV may not be supported by the device\n");
+		exit(KSFT_SKIP);
+	}
+
+	ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);
+	ASSERT_EQ(close(fd), 0);
+	nr_vfs = strtoul(buf, NULL, 0);
+	if (nr_vfs < 0) {
+		fprintf(stderr, "SR-IOV may not be supported by the device\n");
+		exit(KSFT_SKIP);
+	}
+
+	/* Setup VFs, if already not done */
+	snprintf(path, PATH_MAX, "%s/%s/sriov_numvfs", PCI_SYSFS_PATH, pf_dev_bdf);
+	ASSERT_GT(fd = open(path, O_RDWR), 0);
+	ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);
+	nr_vfs = strtoul(buf, NULL, 0);
+	if (nr_vfs == 0)
+		ASSERT_EQ(write(fd, "1", 1), 1);
+	ASSERT_EQ(close(fd), 0);
+
+	/* Get the BDF of the first VF */
+	snprintf(path, PATH_MAX, "%s/%s/virtfn0", PCI_SYSFS_PATH, pf_dev_bdf);
+	ret = readlink(path, vf_path, PATH_MAX);
+	ASSERT_NE(ret, -1);
+	ret = sscanf(basename(vf_path), "%s", vf_dev_bdf);
+	ASSERT_EQ(ret, 1);
+}
+
+FIXTURE_TEARDOWN(vfio_pci_sriov_uapi_test)
+{
+}
+
+FIXTURE_VARIANT(vfio_pci_sriov_uapi_test) {
+	const char *iommu_mode;
+	char *vf_token;
+};
+
+#define FIXTURE_VARIANT_ADD_IOMMU_MODE(_iommu_mode, _name, _vf_token)		\
+FIXTURE_VARIANT_ADD(vfio_pci_sriov_uapi_test, _iommu_mode ## _ ## _name) {	\
+	.iommu_mode = #_iommu_mode,						\
+	.vf_token = (_vf_token),						\
+}
+
+FIXTURE_VARIANT_ADD_ALL_IOMMU_MODES(same_uuid, UUID_1);
+FIXTURE_VARIANT_ADD_ALL_IOMMU_MODES(diff_uuid, UUID_2);
+FIXTURE_VARIANT_ADD_ALL_IOMMU_MODES(null_uuid, NULL);
+
+/*
+ * PF's token is always set with UUID_1 and VF's token is rotated with
+ * various tokens (including UUID_1 and NULL).
+ * This asserts if the VF device is successfully created for a match
+ * in the token or actually fails during a mismatch.
+ */
+#define ASSERT_VF_CREATION(_ret) do {				\
+	if (variant->vf_token == NULL ||			\
+	    strcmp(UUID_1, variant->vf_token)) {		\
+	    ASSERT_NE((_ret), 0);				\
+	} else {						\
+	    ASSERT_EQ((_ret), 0);				\
+	}							\
+} while (0)
+
+/*
+ * Validate if the UAPI handles correctly and incorrectly set token on the VF.
+ */
+TEST_F(vfio_pci_sriov_uapi_test, init_token_match)
+{
+	int ret;
+
+	pf_device = test_vfio_pci_device_init(pf_dev_bdf, variant->iommu_mode,
+					      UUID_1, &ret);
+	vf_device = test_vfio_pci_device_init(vf_dev_bdf, variant->iommu_mode,
+					      variant->vf_token, &ret);
+
+	ASSERT_VF_CREATION(ret);
+
+	test_vfio_pci_device_cleanup(vf_device);
+	test_vfio_pci_device_cleanup(pf_device);
+}
+
+/*
+ * After setting a token on the PF, validate if the VF can still set the
+ * expected token.
+ */
+TEST_F(vfio_pci_sriov_uapi_test, pf_early_close)
+{
+	int ret;
+
+	pf_device = test_vfio_pci_device_init(pf_dev_bdf, variant->iommu_mode,
+					      UUID_1, &ret);
+	test_vfio_pci_device_cleanup(pf_device);
+
+	vf_device = test_vfio_pci_device_init(vf_dev_bdf, variant->iommu_mode,
+					      variant->vf_token, &ret);
+
+	ASSERT_VF_CREATION(ret);
+
+	test_vfio_pci_device_cleanup(vf_device);
+}
+
+/*
+ * After PF device init, override the exsiting token and validate if the newly
+ * set token is the one that's active.
+ */
+TEST_F(vfio_pci_sriov_uapi_test, override_token)
+{
+	int ret;
+
+	pf_device = test_vfio_pci_device_init(pf_dev_bdf, variant->iommu_mode,
+					      UUID_2, &ret);
+	vfio_device_set_vf_token(pf_device->fd, UUID_1);
+
+	vf_device = test_vfio_pci_device_init(vf_dev_bdf, variant->iommu_mode,
+					      variant->vf_token, &ret);
+
+	ASSERT_VF_CREATION(ret);
+
+	test_vfio_pci_device_cleanup(vf_device);
+	test_vfio_pci_device_cleanup(pf_device);
+}
+
+int main(int argc, char *argv[])
+{
+	pf_dev_bdf = vfio_selftests_get_bdf(&argc, argv);
+	return test_harness_run(argc, argv);
+}
-- 
2.51.2.997.g839fc31de9-goog

Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[David Matlack]

On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:

> -struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_mode);
> +struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> +					      const char *iommu_mode,
> +					      const char *vf_token);

Vipin is also looking at adding an optional parameter to
vfio_pci_device_init():
https://lore.kernel.org/kvm/20251018000713.677779-20-vipinsh@google.com/

I am wondering if we should support an options struct for such
parameters. e.g. something like this

diff --git a/tools/testing/selftests/vfio/lib/include/vfio_util.h b/tools/testing/selftests/vfio/lib/include/vfio_util.h
index b01068d98fda..cee837fe561c 100644
--- a/tools/testing/selftests/vfio/lib/include/vfio_util.h
+++ b/tools/testing/selftests/vfio/lib/include/vfio_util.h
@@ -160,6 +160,10 @@ struct vfio_pci_driver {
        int msi;
 };

+struct vfio_pci_device_options {
+       const char *vf_token;
+};
+
 struct vfio_pci_device {
        int fd;

@@ -202,9 +206,18 @@ const char *vfio_pci_get_cdev_path(const char *bdf);

 extern const char *default_iommu_mode;

-struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
-                                             const char *iommu_mode,
-                                             const char *vf_token);
+struct vfio_pci_device *__vfio_pci_device_init(const char *bdf,
+                                              const char *iommu_mode,
+                                              const struct vfio_pci_device_options *options);
+
+static inline struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
+                                                          const char *iommu_mode)
+{
+       static const struct vfio_pci_device_options default_options = {};
+
+       return __vfio_pci_device_init(bdf, iommu_mode, &default_options);
+}
+

This will avoid you having to update every test.

You can create a helper function in vfio_pci_sriov_uapi_test.c to call
__vfio_pci_device_init() and abstract away the options stuff from your
test.

> -static void vfio_pci_container_setup(struct vfio_pci_device *device, const char *bdf)
> +static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,

Let's name this vfio_pci_group_get_device_fd() since it's getting the
device FD from the group using ioctl(VFIO_GROUP_GET_DEVICE_FD).

> +					      const char *bdf,
> +					      const char *vf_token)

There's an extra space before these arguments. Align them all vertically
with the first argument.

I noticed this throughout this patch, so please fix throughout the whole
series in v2.

Also please run checkpatch.pl. It will catch things like this for you.

  CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
  #78: FILE: tools/testing/selftests/vfio/lib/vfio_pci_device.c:335:
  +static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
  +                                             const char *bdf,

> +{
> +	char *arg = (char *) bdf;

No space necessary after a cast. This is another one checkpatch.pl will
catch for you.

  CHECK:SPACING: No space is necessary after a cast
  #81: FILE: tools/testing/selftests/vfio/lib/vfio_pci_device.c:338:
  +       char *arg = (char *) bdf;

> +
> +	/*
> +	 * If a vf_token exists, argument to VFIO_GROUP_GET_DEVICE_FD
> +	 * will be in the form of the following example:
> +	 * "0000:04:10.0 vf_token=bd8d9d2b-5a5f-4f5a-a211-f591514ba1f3"
> +	 */
> +	if (vf_token) {
> +		size_t sz = strlen(bdf) + strlen(" "VF_TOKEN_ARG) +
> +			    strlen(vf_token) + 1;
> +
> +		arg = calloc(1, sz);
> +		VFIO_ASSERT_NOT_NULL(arg);
> +
> +		snprintf(arg, sz, "%s %s%s", bdf, VF_TOKEN_ARG, vf_token);
> +	}

UUIDs are 16 bytes so I think we could create a pretty reasonably fixed
size array to hold the argument and simplify this code, make it more
self-documenting, and eliminate VF_TOKEN_ARG. This is test code, so we
can make the array bigger in the future if we need to.  Keeping the code
simple is more important IMO.

static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
                                             const char *bdf,
                                             const char *vf_token)
{
        char arg[64];

        if (vf_token)
                snprintf(arg, ARRAY_SIZE(arg), "%s vf_token=%s", bdf, vf_token);
        else
                snprintf(arg, ARRAY_SIZE(arg), "%s", bdf);

        device->fd = ioctl(device->group_fd, VFIO_GROUP_GET_DEVICE_FD, arg);
        VFIO_ASSERT_GE(device->fd, 0);
}

And to protect against writing off the end of arg, we can introduce a
snprintf_assert() in a separate commit. There are actually a few
snprintf() calls in vfio_pci_device.c that would be nice to convert to
snprintf_assert().

#define snprintf_assert(_s, _size, _fmt, ...) do {                      \
        int __ret = snprintf(_s, _size, _fmt, ##__VA_ARGS__);           \
        VFIO_ASSERT_LT(__ret, _size);                                   \
} while (0)

snprintf_assert() could be added in a precursor commit to this one.

> +static void vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token)
>  {
>  	struct vfio_device_bind_iommufd args = {
>  		.argsz = sizeof(args),
>  		.iommufd = iommufd,
>  	};
> +	uuid_t token_uuid = {0};
> +
> +	if (vf_token) {
> +		VFIO_ASSERT_EQ(uuid_parse(vf_token, token_uuid), 0);
> +		args.flags = VFIO_DEVICE_BIND_FLAG_TOKEN;

Maybe make this |= instead of = ? I had to double-check that this wasn't
overwriting any flags set above this. Obviously it's not since this is a
small function, but |= would make that super obvious.

Re: [PATCH 3/4] vfio: selftests: Add helper to set/override a vf_token

[David Matlack]

On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> Not only at init, but a vf_token can also be set via the
> VFIO_DEVICE_FEATURE ioctl, by setting the
> VFIO_DEVICE_FEATURE_PCI_VF_TOKEN flag. Add an API to utilize this
> functionality from the test code.

Say what the commit does first. Then add context (e.g.  compare/contrast
to other ways of setting the VF token).

Also please add a sentence about how this will be used in a subsequent
commit, since there are no callers in this commit.

> +void vfio_device_set_vf_token(int fd, const char *vf_token)
> +{
> +	uuid_t token_uuid = {0};
> +
> +	VFIO_ASSERT_NOT_NULL(vf_token, "vf_token is NULL");

nit: The help message here is not needed. It will be very obvious that
vf_token is NULL if this assert fires :)

Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[David Matlack]

On 2025-11-05 11:52 PM, David Matlack wrote:
> On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> 
> > -struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_mode);
> > +struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > +					      const char *iommu_mode,
> > +					      const char *vf_token);
> 
> Vipin is also looking at adding an optional parameter to
> vfio_pci_device_init():
> https://lore.kernel.org/kvm/20251018000713.677779-20-vipinsh@google.com/
> 
> I am wondering if we should support an options struct for such
> parameters. e.g. something like this

Wait, patch 4 doesn't even use vfio_pci_device_init(). Do we need this
commit? It seems like we just need some of the inner functions to have
support for vf_token.

Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[David Matlack]

On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:

> diff --git a/tools/testing/selftests/vfio/lib/libvfio.mk b/tools/testing/selftests/vfio/lib/libvfio.mk
> index 5d11c3a89a28e..2dc85c41ffb4b 100644
> --- a/tools/testing/selftests/vfio/lib/libvfio.mk
> +++ b/tools/testing/selftests/vfio/lib/libvfio.mk
> @@ -18,7 +18,9 @@ $(shell mkdir -p $(LIBVFIO_O_DIRS))
>  
>  CFLAGS += -I$(VFIO_DIR)/lib/include
>  
> +LDLIBS += -luuid

I wonder if we really need this dependency. VFIO and IOMMUFD just expect
a 16 byte character array. That is easy enough to represent. The other
part we use is uuid_parse(), but I don't know if selftests need to do
that validation. We can let VFIO and IOMMUFD validate the UUID as they
see fit and return an error if they aren't happy with it. i.e. We do not
need to duplicate validation in the test.

Re: [PATCH 2/4] vfio: selftests: Export vfio_pci_device functions

[David Matlack]

On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> Refactor and make the functions called under device initialization
> public. A later patch adds a test that calls these functions to validate
> the UAPI of SR-IOV devices. Opportunistically, to test the success
> and failure cases of the UAPI, split the functions dealing with
> VFIO_GROUP_GET_DEVICE_FD and VFIO_DEVICE_BIND_IOMMUFD into a core
> function and another one that asserts the ioctl. The former will be
> used for testing the SR-IOV UAPI, hence only export these.

I have a series that separates the IOMMU initialization and fields from
struct vfio_pci_device. I suspect that will make what you are trying to
do a lot easier.

https://lore.kernel.org/kvm/20251008232531.1152035-1-dmatlack@google.com/

Can you take a look at it and see if it would simplifying things for
you? Reviews would be very appreciated if so :)

Re: [PATCH 4/4] vfio: selftests: Add tests to validate SR-IOV UAPI

[David Matlack]

On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:

> +static const char *pf_dev_bdf;
> +static char vf_dev_bdf[16];

vf_dev_bdf can be part of the test fixture instead of a global variable.
pf_dev_bdf should be the only global variable since we have to get it
from main() into the text fixture.

> +
> +struct vfio_pci_device *pf_device;
> +struct vfio_pci_device *vf_device;

These can be local variables in the places they are used.

> +
> +static void test_vfio_pci_container_setup(struct vfio_pci_device *device,
> +					   const char *bdf,
> +					   const char *vf_token)
> +{
> +	vfio_container_open(device);
> +	vfio_pci_group_setup(device, bdf);
> +	vfio_container_set_iommu(device);
> +	__vfio_container_get_device_fd(device, bdf, vf_token);
> +}
> +
> +static int test_vfio_pci_iommufd_setup(struct vfio_pci_device *device,
> +					const char *bdf, const char *vf_token)
> +{
> +	vfio_pci_iommufd_cdev_open(device, bdf);
> +	vfio_pci_iommufd_iommudev_open(device);
> +	return __vfio_device_bind_iommufd(device->fd, device->iommufd, vf_token);
> +}
> +
> +static struct vfio_pci_device *test_vfio_pci_device_init(const char *bdf,
> +							  const char *iommu_mode,
> +							  const char *vf_token,
> +							  int *out_ret)
> +{
> +	struct vfio_pci_device *device;
> +
> +	device = calloc(1, sizeof(*device));
> +	VFIO_ASSERT_NOT_NULL(device);
> +
> +	device->iommu_mode = lookup_iommu_mode(iommu_mode);
> +
> +	if (iommu_mode_container_path(iommu_mode)) {
> +		test_vfio_pci_container_setup(device, bdf, vf_token);
> +		/* The device fd will be -1 in case of mismatched tokens */
> +		*out_ret = (device->fd < 0);

Maybe just return device->fd from test_vfio_pci_container_setup() so
this can be:

  *out_ret = test_vfio_pci_container_setup(device, bdf, vf_token);

and then you can drop the curly braces.

> +	} else {
> +		*out_ret = test_vfio_pci_iommufd_setup(device, bdf, vf_token);
> +	}
> +
> +	return device;
> +}
> +
> +static void test_vfio_pci_device_cleanup(struct vfio_pci_device *device)
> +{
> +	if (device->fd > 0)
> +		VFIO_ASSERT_EQ(close(device->fd), 0);
> +
> +	if (device->iommufd) {
> +		VFIO_ASSERT_EQ(close(device->iommufd), 0);
> +	} else {
> +		VFIO_ASSERT_EQ(close(device->group_fd), 0);
> +		VFIO_ASSERT_EQ(close(device->container_fd), 0);
> +	}
> +
> +	free(device);
> +}
> +
> +FIXTURE(vfio_pci_sriov_uapi_test) {};
> +
> +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> +{
> +	char vf_path[PATH_MAX] = {0};
> +	char path[PATH_MAX] = {0};
> +	unsigned int nr_vfs;
> +	char buf[32] = {0};
> +	int ret;
> +	int fd;
> +
> +	/* Check if SR-IOV is supported by the device */
> +	snprintf(path, PATH_MAX, "%s/%s/sriov_totalvfs", PCI_SYSFS_PATH, pf_dev_bdf);

nit: Personally I would just hard-code the sysfs path instead of using
PCI_SYSFS_PATH. I think the code is more readable and more succinct that
way. And sysfs should be a stable ABI.

> +	fd = open(path, O_RDONLY);
> +	if (fd < 0) {
> +		fprintf(stderr, "SR-IOV may not be supported by the device\n");
> +		exit(KSFT_SKIP);

Use SKIP() for this:

if (fd < 0)
        SKIP(return, "SR-IOV is not supported by the device\n");

Ditto below.

> +	}
> +
> +	ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);
> +	ASSERT_EQ(close(fd), 0);
> +	nr_vfs = strtoul(buf, NULL, 0);
> +	if (nr_vfs < 0) {
> +		fprintf(stderr, "SR-IOV may not be supported by the device\n");
> +		exit(KSFT_SKIP);
> +	}
> +
> +	/* Setup VFs, if already not done */

Before creating VFs, should we disable auto-probing so the VFs don't get
bound to some other random driver (write 0 to sriov_drivers_autoprobe)?

> +	snprintf(path, PATH_MAX, "%s/%s/sriov_numvfs", PCI_SYSFS_PATH, pf_dev_bdf);
> +	ASSERT_GT(fd = open(path, O_RDWR), 0);
> +	ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);
> +	nr_vfs = strtoul(buf, NULL, 0);
> +	if (nr_vfs == 0)

If VFs are already enabled, shouldn't the test fail or skip?

> +		ASSERT_EQ(write(fd, "1", 1), 1);
> +	ASSERT_EQ(close(fd), 0);
> +
> +	/* Get the BDF of the first VF */
> +	snprintf(path, PATH_MAX, "%s/%s/virtfn0", PCI_SYSFS_PATH, pf_dev_bdf);
> +	ret = readlink(path, vf_path, PATH_MAX);
> +	ASSERT_NE(ret, -1);
> +	ret = sscanf(basename(vf_path), "%s", vf_dev_bdf);
> +	ASSERT_EQ(ret, 1);

What ensures the created VF is bound to vfio-pci?

> +}
> +
> +FIXTURE_TEARDOWN(vfio_pci_sriov_uapi_test)
> +{
> +}

FIXTURE_TEARDOWN() should undo what FIXTURE_SETUP() did, i.e. write 0 to
sriov_numvfs. Otherwise running this test will leave behind SR-IOV
enabled on the PF.

You could also make this the users problem (the user has to provide a PF
with 1 VF where both PF and VF are bound to vfio-pci). But I think it
would be nice to make the test work automatically given a PF if we can.

Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[Raghavendra Rao Ananta]

Hi David,

On Thu, Nov 6, 2025 at 5:22 AM David Matlack <dmatlack@google.com> wrote:
>
> On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
>
> > -struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_mode);
> > +struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > +                                           const char *iommu_mode,
> > +                                           const char *vf_token);
>
> Vipin is also looking at adding an optional parameter to
> vfio_pci_device_init():
> https://lore.kernel.org/kvm/20251018000713.677779-20-vipinsh@google.com/
>
> I am wondering if we should support an options struct for such
> parameters. e.g. something like this
>
> diff --git a/tools/testing/selftests/vfio/lib/include/vfio_util.h b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> index b01068d98fda..cee837fe561c 100644
> --- a/tools/testing/selftests/vfio/lib/include/vfio_util.h
> +++ b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> @@ -160,6 +160,10 @@ struct vfio_pci_driver {
>         int msi;
>  };
>
> +struct vfio_pci_device_options {
> +       const char *vf_token;
> +};
> +
>  struct vfio_pci_device {
>         int fd;
>
> @@ -202,9 +206,18 @@ const char *vfio_pci_get_cdev_path(const char *bdf);
>
>  extern const char *default_iommu_mode;
>
> -struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> -                                             const char *iommu_mode,
> -                                             const char *vf_token);
> +struct vfio_pci_device *__vfio_pci_device_init(const char *bdf,
> +                                              const char *iommu_mode,
> +                                              const struct vfio_pci_device_options *options);
> +
> +static inline struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> +                                                          const char *iommu_mode)
> +{
> +       static const struct vfio_pci_device_options default_options = {};
> +
> +       return __vfio_pci_device_init(bdf, iommu_mode, &default_options);
> +}
> +
>
> This will avoid you having to update every test.
>
> You can create a helper function in vfio_pci_sriov_uapi_test.c to call
> __vfio_pci_device_init() and abstract away the options stuff from your
> test.
>
I like the idea of an optional expandable struct. I'll implement this in v2.

> > -static void vfio_pci_container_setup(struct vfio_pci_device *device, const char *bdf)
> > +static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
>
> Let's name this vfio_pci_group_get_device_fd() since it's getting the
> device FD from the group using ioctl(VFIO_GROUP_GET_DEVICE_FD).
>
> > +                                           const char *bdf,
> > +                                           const char *vf_token)
>
> There's an extra space before these arguments. Align them all vertically
> with the first argument.
>
> I noticed this throughout this patch, so please fix throughout the whole
> series in v2.
>
I may have to fix my editor to display this right. Thanks for catching this.

> Also please run checkpatch.pl. It will catch things like this for you.
>
>   CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
>   #78: FILE: tools/testing/selftests/vfio/lib/vfio_pci_device.c:335:
>   +static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
>   +                                             const char *bdf,
>
> > +{
> > +     char *arg = (char *) bdf;
>
> No space necessary after a cast. This is another one checkpatch.pl will
> catch for you.
>
>   CHECK:SPACING: No space is necessary after a cast
>   #81: FILE: tools/testing/selftests/vfio/lib/vfio_pci_device.c:338:
>   +       char *arg = (char *) bdf;
>
Actually, I did run checkpatch.pl on the entire series as:
.$ ./scripts/checkpatch.pl *.patch

I didn't see any of these warnings. Are there any other options to consider?

> > +
> > +     /*
> > +      * If a vf_token exists, argument to VFIO_GROUP_GET_DEVICE_FD
> > +      * will be in the form of the following example:
> > +      * "0000:04:10.0 vf_token=bd8d9d2b-5a5f-4f5a-a211-f591514ba1f3"
> > +      */
> > +     if (vf_token) {
> > +             size_t sz = strlen(bdf) + strlen(" "VF_TOKEN_ARG) +
> > +                         strlen(vf_token) + 1;
> > +
> > +             arg = calloc(1, sz);
> > +             VFIO_ASSERT_NOT_NULL(arg);
> > +
> > +             snprintf(arg, sz, "%s %s%s", bdf, VF_TOKEN_ARG, vf_token);
> > +     }
>
> UUIDs are 16 bytes so I think we could create a pretty reasonably fixed
> size array to hold the argument and simplify this code, make it more
> self-documenting, and eliminate VF_TOKEN_ARG. This is test code, so we
> can make the array bigger in the future if we need to.  Keeping the code
> simple is more important IMO.
>
> static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
>                                              const char *bdf,
>                                              const char *vf_token)
> {
>         char arg[64];
>
>         if (vf_token)
>                 snprintf(arg, ARRAY_SIZE(arg), "%s vf_token=%s", bdf, vf_token);
>         else
>                 snprintf(arg, ARRAY_SIZE(arg), "%s", bdf);
>
>         device->fd = ioctl(device->group_fd, VFIO_GROUP_GET_DEVICE_FD, arg);
>         VFIO_ASSERT_GE(device->fd, 0);
> }
>
Yeah, this is a good idea! I'll implement it in v2.

> And to protect against writing off the end of arg, we can introduce a
> snprintf_assert() in a separate commit. There are actually a few
> snprintf() calls in vfio_pci_device.c that would be nice to convert to
> snprintf_assert().
>
> #define snprintf_assert(_s, _size, _fmt, ...) do {                      \
>         int __ret = snprintf(_s, _size, _fmt, ##__VA_ARGS__);           \
>         VFIO_ASSERT_LT(__ret, _size);                                   \
> } while (0)
>
> snprintf_assert() could be added in a precursor commit to this one.
>
Sounds good. I'll add a commit for this and convert all the
snprintf()s to snprintf_assert()s.

> > +static void vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token)
> >  {
> >       struct vfio_device_bind_iommufd args = {
> >               .argsz = sizeof(args),
> >               .iommufd = iommufd,
> >       };
> > +     uuid_t token_uuid = {0};
> > +
> > +     if (vf_token) {
> > +             VFIO_ASSERT_EQ(uuid_parse(vf_token, token_uuid), 0);
> > +             args.flags = VFIO_DEVICE_BIND_FLAG_TOKEN;
>
> Maybe make this |= instead of = ? I had to double-check that this wasn't
> overwriting any flags set above this. Obviously it's not since this is a
> small function, but |= would make that super obvious.
Oh yes, absolutely!

Thank you.
Raghavendra

Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[Raghavendra Rao Ananta]

On Thu, Nov 6, 2025 at 5:42 AM David Matlack <dmatlack@google.com> wrote:
>
> On 2025-11-05 11:52 PM, David Matlack wrote:
> > On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> >
> > > -struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_mode);
> > > +struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > > +                                         const char *iommu_mode,
> > > +                                         const char *vf_token);
> >
> > Vipin is also looking at adding an optional parameter to
> > vfio_pci_device_init():
> > https://lore.kernel.org/kvm/20251018000713.677779-20-vipinsh@google.com/
> >
> > I am wondering if we should support an options struct for such
> > parameters. e.g. something like this
>
> Wait, patch 4 doesn't even use vfio_pci_device_init(). Do we need this
> commit? It seems like we just need some of the inner functions to have
> support for vf_token.

Gah, that's my bad. I changed the approach later but forgot to revert
the API. I'll fix it in v2.

Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[Raghavendra Rao Ananta]

On Thu, Nov 6, 2025 at 5:44 AM David Matlack <dmatlack@google.com> wrote:
>
> On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
>
> > diff --git a/tools/testing/selftests/vfio/lib/libvfio.mk b/tools/testing/selftests/vfio/lib/libvfio.mk
> > index 5d11c3a89a28e..2dc85c41ffb4b 100644
> > --- a/tools/testing/selftests/vfio/lib/libvfio.mk
> > +++ b/tools/testing/selftests/vfio/lib/libvfio.mk
> > @@ -18,7 +18,9 @@ $(shell mkdir -p $(LIBVFIO_O_DIRS))
> >
> >  CFLAGS += -I$(VFIO_DIR)/lib/include
> >
> > +LDLIBS += -luuid
>
> I wonder if we really need this dependency. VFIO and IOMMUFD just expect
> a 16 byte character array. That is easy enough to represent. The other
> part we use is uuid_parse(), but I don't know if selftests need to do
> that validation. We can let VFIO and IOMMUFD validate the UUID as they
> see fit and return an error if they aren't happy with it. i.e. We do not
> need to duplicate validation in the test.

Unfortunately, VFIO interface accepts UUID in multiple formats. For
VFIO_DEVICE_FEATURE and VFIO_DEVICE_BIND_IOMMUFD it accepts a
'u8[16]', but for VFIO_GROUP_GET_DEVICE_FD, we must present it as a
string. Is there an issue with the inclusion of an external library (I
think I've seen others in tools/ use it).

Thank you.
Raghavendra

Re: [PATCH 2/4] vfio: selftests: Export vfio_pci_device functions

[Raghavendra Rao Ananta]

On Thu, Nov 6, 2025 at 6:12 AM David Matlack <dmatlack@google.com> wrote:
>
> On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> > Refactor and make the functions called under device initialization
> > public. A later patch adds a test that calls these functions to validate
> > the UAPI of SR-IOV devices. Opportunistically, to test the success
> > and failure cases of the UAPI, split the functions dealing with
> > VFIO_GROUP_GET_DEVICE_FD and VFIO_DEVICE_BIND_IOMMUFD into a core
> > function and another one that asserts the ioctl. The former will be
> > used for testing the SR-IOV UAPI, hence only export these.
>
> I have a series that separates the IOMMU initialization and fields from
> struct vfio_pci_device. I suspect that will make what you are trying to
> do a lot easier.
>
> https://lore.kernel.org/kvm/20251008232531.1152035-1-dmatlack@google.com/
>
Nice! I'll take a look at it. By the way, how do we normally deal with
dependencies among series? Do we simply mention it in the
cover-letter?

> Can you take a look at it and see if it would simplifying things for
> you? Reviews would be very appreciated if so :)
Absolutely! Sorry, I have it on my TODO list to review the changes,
but didn't get a chance. I'll get to it soon. Thanks for the reminder
:)

Re: [PATCH 3/4] vfio: selftests: Add helper to set/override a vf_token

[Raghavendra Rao Ananta]

On Thu, Nov 6, 2025 at 5:31 AM David Matlack <dmatlack@google.com> wrote:
>
> On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> > Not only at init, but a vf_token can also be set via the
> > VFIO_DEVICE_FEATURE ioctl, by setting the
> > VFIO_DEVICE_FEATURE_PCI_VF_TOKEN flag. Add an API to utilize this
> > functionality from the test code.
>
> Say what the commit does first. Then add context (e.g.  compare/contrast
> to other ways of setting the VF token).
>
> Also please add a sentence about how this will be used in a subsequent
> commit, since there are no callers in this commit.
>
> > +void vfio_device_set_vf_token(int fd, const char *vf_token)
> > +{
> > +     uuid_t token_uuid = {0};
> > +
> > +     VFIO_ASSERT_NOT_NULL(vf_token, "vf_token is NULL");
>
> nit: The help message here is not needed. It will be very obvious that
> vf_token is NULL if this assert fires :)
I'll apply these suggestions in v2.

Thank you.
Raghavendra

Re: [PATCH 4/4] vfio: selftests: Add tests to validate SR-IOV UAPI

[Raghavendra Rao Ananta]

On Thu, Nov 6, 2025 at 6:30 AM David Matlack <dmatlack@google.com> wrote:
>
> On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
>
> > +static const char *pf_dev_bdf;
> > +static char vf_dev_bdf[16];
>
> vf_dev_bdf can be part of the test fixture instead of a global variable.
> pf_dev_bdf should be the only global variable since we have to get it
> from main() into the text fixture.
>
My understading is placing vars in FIXTURE() is helpful to get an
access across various other FIXTURE_*() and TEST*() functions. Out of
curiosity, is there an advantage here vs having them global?

> > +
> > +struct vfio_pci_device *pf_device;
> > +struct vfio_pci_device *vf_device;
>
> These can be local variables in the places they are used.
>
I was a bit greedy to save a few lines, as they are reassigned in
every TEST_F() anyway. Is there any advantage by making them local?

> > +
> > +static void test_vfio_pci_container_setup(struct vfio_pci_device *device,
> > +                                        const char *bdf,
> > +                                        const char *vf_token)
> > +{
> > +     vfio_container_open(device);
> > +     vfio_pci_group_setup(device, bdf);
> > +     vfio_container_set_iommu(device);
> > +     __vfio_container_get_device_fd(device, bdf, vf_token);
> > +}
> > +
> > +static int test_vfio_pci_iommufd_setup(struct vfio_pci_device *device,
> > +                                     const char *bdf, const char *vf_token)
> > +{
> > +     vfio_pci_iommufd_cdev_open(device, bdf);
> > +     vfio_pci_iommufd_iommudev_open(device);
> > +     return __vfio_device_bind_iommufd(device->fd, device->iommufd, vf_token);
> > +}
> > +
> > +static struct vfio_pci_device *test_vfio_pci_device_init(const char *bdf,
> > +                                                       const char *iommu_mode,
> > +                                                       const char *vf_token,
> > +                                                       int *out_ret)
> > +{
> > +     struct vfio_pci_device *device;
> > +
> > +     device = calloc(1, sizeof(*device));
> > +     VFIO_ASSERT_NOT_NULL(device);
> > +
> > +     device->iommu_mode = lookup_iommu_mode(iommu_mode);
> > +
> > +     if (iommu_mode_container_path(iommu_mode)) {
> > +             test_vfio_pci_container_setup(device, bdf, vf_token);
> > +             /* The device fd will be -1 in case of mismatched tokens */
> > +             *out_ret = (device->fd < 0);
>
> Maybe just return device->fd from test_vfio_pci_container_setup() so
> this can be:
>
>   *out_ret = test_vfio_pci_container_setup(device, bdf, vf_token);
>
> and then you can drop the curly braces.
>
Makes sense. I'll do it in v2.

> > +     } else {
> > +             *out_ret = test_vfio_pci_iommufd_setup(device, bdf, vf_token);
> > +     }
> > +
> > +     return device;
> > +}
> > +
> > +static void test_vfio_pci_device_cleanup(struct vfio_pci_device *device)
> > +{
> > +     if (device->fd > 0)
> > +             VFIO_ASSERT_EQ(close(device->fd), 0);
> > +
> > +     if (device->iommufd) {
> > +             VFIO_ASSERT_EQ(close(device->iommufd), 0);
> > +     } else {
> > +             VFIO_ASSERT_EQ(close(device->group_fd), 0);
> > +             VFIO_ASSERT_EQ(close(device->container_fd), 0);
> > +     }
> > +
> > +     free(device);
> > +}
> > +
> > +FIXTURE(vfio_pci_sriov_uapi_test) {};
> > +
> > +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> > +{
> > +     char vf_path[PATH_MAX] = {0};
> > +     char path[PATH_MAX] = {0};
> > +     unsigned int nr_vfs;
> > +     char buf[32] = {0};
> > +     int ret;
> > +     int fd;
> > +
> > +     /* Check if SR-IOV is supported by the device */
> > +     snprintf(path, PATH_MAX, "%s/%s/sriov_totalvfs", PCI_SYSFS_PATH, pf_dev_bdf);
>
> nit: Personally I would just hard-code the sysfs path instead of using
> PCI_SYSFS_PATH. I think the code is more readable and more succinct that
> way. And sysfs should be a stable ABI.
>
Sure.

> > +     fd = open(path, O_RDONLY);
> > +     if (fd < 0) {
> > +             fprintf(stderr, "SR-IOV may not be supported by the device\n");
> > +             exit(KSFT_SKIP);
>
> Use SKIP() for this:
>
Sure.
> if (fd < 0)
>         SKIP(return, "SR-IOV is not supported by the device\n");
>
> Ditto below.
>
> > +     }
> > +
> > +     ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);
> > +     ASSERT_EQ(close(fd), 0);
> > +     nr_vfs = strtoul(buf, NULL, 0);
> > +     if (nr_vfs < 0) {
> > +             fprintf(stderr, "SR-IOV may not be supported by the device\n");
> > +             exit(KSFT_SKIP);
> > +     }
> > +
> > +     /* Setup VFs, if already not done */
>
> Before creating VFs, should we disable auto-probing so the VFs don't get
> bound to some other random driver (write 0 to sriov_drivers_autoprobe)?
>
Good idea. I'll make this change in v2.

> > +     snprintf(path, PATH_MAX, "%s/%s/sriov_numvfs", PCI_SYSFS_PATH, pf_dev_bdf);
> > +     ASSERT_GT(fd = open(path, O_RDWR), 0);
> > +     ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);
> > +     nr_vfs = strtoul(buf, NULL, 0);
> > +     if (nr_vfs == 0)
>
> If VFs are already enabled, shouldn't the test fail or skip?
>
My idea was to simply "steal" the device that was already created and
use it. Do we want to skip it, as you suggested?

> > +             ASSERT_EQ(write(fd, "1", 1), 1);
> > +     ASSERT_EQ(close(fd), 0);
> > +
> > +     /* Get the BDF of the first VF */
> > +     snprintf(path, PATH_MAX, "%s/%s/virtfn0", PCI_SYSFS_PATH, pf_dev_bdf);
> > +     ret = readlink(path, vf_path, PATH_MAX);
> > +     ASSERT_NE(ret, -1);
> > +     ret = sscanf(basename(vf_path), "%s", vf_dev_bdf);
> > +     ASSERT_EQ(ret, 1);
>
> What ensures the created VF is bound to vfio-pci?
>
Good point. I'll explicitly bind it to vfio-pci, if it isn't already.

> > +}
> > +
> > +FIXTURE_TEARDOWN(vfio_pci_sriov_uapi_test)
> > +{
> > +}
>
> FIXTURE_TEARDOWN() should undo what FIXTURE_SETUP() did, i.e. write 0 to
> sriov_numvfs. Otherwise running this test will leave behind SR-IOV
> enabled on the PF.
>
I had this originally, but then realized that run.sh aready resets the
sriov_numvfs to its original value. We can do it here too, if you'd
like to keep the symmetry and make the test self-sufficient. With some
of your other suggestions, I may have to do some more cleanup here
now.

> You could also make this the users problem (the user has to provide a PF
> with 1 VF where both PF and VF are bound to vfio-pci). But I think it
> would be nice to make the test work automatically given a PF if we can.
Let's go with the latter, assuming it doesn't get too complicated
(currently, the setup part seems bigger than the actual test :) )

Re: [PATCH 2/4] vfio: selftests: Export vfio_pci_device functions

[David Matlack]

On 2025-11-06 10:13 PM, Raghavendra Rao Ananta wrote:
> On Thu, Nov 6, 2025 at 6:12 AM David Matlack <dmatlack@google.com> wrote:
> >
> > On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> > > Refactor and make the functions called under device initialization
> > > public. A later patch adds a test that calls these functions to validate
> > > the UAPI of SR-IOV devices. Opportunistically, to test the success
> > > and failure cases of the UAPI, split the functions dealing with
> > > VFIO_GROUP_GET_DEVICE_FD and VFIO_DEVICE_BIND_IOMMUFD into a core
> > > function and another one that asserts the ioctl. The former will be
> > > used for testing the SR-IOV UAPI, hence only export these.
> >
> > I have a series that separates the IOMMU initialization and fields from
> > struct vfio_pci_device. I suspect that will make what you are trying to
> > do a lot easier.
> >
> > https://lore.kernel.org/kvm/20251008232531.1152035-1-dmatlack@google.com/
> >
> Nice! I'll take a look at it. By the way, how do we normally deal with
> dependencies among series? Do we simply mention it in the
> cover-letter?

What I usually do is:

 - Mention in the cover letter that this series applies on top of
   another series, and provide a lore link to the exact series version
   it's on top of.

 - Upload your series (with the dependent series) somewhere pulic like
   GitHub so that it's easy for reviewers to check out your code without
   having to apply multiple different series of patches. Include a link
   to this in your cover letter too.

See this series from Vipin for an example of doing this:

  https://lore.kernel.org/kvm/20251018000713.677779-1-vipinsh@google.com/

> > Can you take a look at it and see if it would simplifying things for
> > you? Reviews would be very appreciated if so :)
> Absolutely! Sorry, I have it on my TODO list to review the changes,
> but didn't get a chance. I'll get to it soon. Thanks for the reminder
> :)

Thanks!

Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[David Matlack]

On 2025-11-06 10:06 PM, Raghavendra Rao Ananta wrote:
> On Thu, Nov 6, 2025 at 5:44 AM David Matlack <dmatlack@google.com> wrote:
> >
> > On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> >
> > > diff --git a/tools/testing/selftests/vfio/lib/libvfio.mk b/tools/testing/selftests/vfio/lib/libvfio.mk
> > > index 5d11c3a89a28e..2dc85c41ffb4b 100644
> > > --- a/tools/testing/selftests/vfio/lib/libvfio.mk
> > > +++ b/tools/testing/selftests/vfio/lib/libvfio.mk
> > > @@ -18,7 +18,9 @@ $(shell mkdir -p $(LIBVFIO_O_DIRS))
> > >
> > >  CFLAGS += -I$(VFIO_DIR)/lib/include
> > >
> > > +LDLIBS += -luuid
> >
> > I wonder if we really need this dependency. VFIO and IOMMUFD just expect
> > a 16 byte character array. That is easy enough to represent. The other
> > part we use is uuid_parse(), but I don't know if selftests need to do
> > that validation. We can let VFIO and IOMMUFD validate the UUID as they
> > see fit and return an error if they aren't happy with it. i.e. We do not
> > need to duplicate validation in the test.
> 
> Unfortunately, VFIO interface accepts UUID in multiple formats. For
> VFIO_DEVICE_FEATURE and VFIO_DEVICE_BIND_IOMMUFD it accepts a
> 'u8[16]', but for VFIO_GROUP_GET_DEVICE_FD, we must present it as a
> string. Is there an issue with the inclusion of an external library (I
> think I've seen others in tools/ use it).

Ack. In that case, depending on libuuid SGTM.

I mistakenly thought VFIO always took the UUID as a string and
uuid_parse() was just being used to sanity check the format. Thanks for
clarifying.

Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[David Matlack]

On 2025-11-06 09:56 PM, Raghavendra Rao Ananta wrote:
> On Thu, Nov 6, 2025 at 5:22 AM David Matlack <dmatlack@google.com> wrote:
> > On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> >
> > > -struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_mode);
> > > +struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > > +                                           const char *iommu_mode,
> > > +                                           const char *vf_token);
> >
> > Vipin is also looking at adding an optional parameter to
> > vfio_pci_device_init():
> > https://lore.kernel.org/kvm/20251018000713.677779-20-vipinsh@google.com/
> >
> > I am wondering if we should support an options struct for such
> > parameters. e.g. something like this
> >
> > diff --git a/tools/testing/selftests/vfio/lib/include/vfio_util.h b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> > index b01068d98fda..cee837fe561c 100644
> > --- a/tools/testing/selftests/vfio/lib/include/vfio_util.h
> > +++ b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> > @@ -160,6 +160,10 @@ struct vfio_pci_driver {
> >         int msi;
> >  };
> >
> > +struct vfio_pci_device_options {
> > +       const char *vf_token;
> > +};
> > +
> >  struct vfio_pci_device {
> >         int fd;
> >
> > @@ -202,9 +206,18 @@ const char *vfio_pci_get_cdev_path(const char *bdf);
> >
> >  extern const char *default_iommu_mode;
> >
> > -struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > -                                             const char *iommu_mode,
> > -                                             const char *vf_token);
> > +struct vfio_pci_device *__vfio_pci_device_init(const char *bdf,
> > +                                              const char *iommu_mode,
> > +                                              const struct vfio_pci_device_options *options);
> > +
> > +static inline struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > +                                                          const char *iommu_mode)
> > +{
> > +       static const struct vfio_pci_device_options default_options = {};
> > +
> > +       return __vfio_pci_device_init(bdf, iommu_mode, &default_options);
> > +}
> > +
> >
> > This will avoid you having to update every test.
> >
> > You can create a helper function in vfio_pci_sriov_uapi_test.c to call
> > __vfio_pci_device_init() and abstract away the options stuff from your
> > test.
> >
> I like the idea of an optional expandable struct. I'll implement this in v2.

Just to make sure we're on the same page: I don't think you need to add
this in v2 since you don't need to call vfio_pci_device_init(). For the
inner functions that you want to call from your test, passing vf_token
directly makes more sense IMO. vfio_pci_device_init() will just pass in
NULL to those functions for vf_token by default.

If/when we want to pass vf_token to vfio_pci_device_init() we can add
the options struct.

> > No space necessary after a cast. This is another one checkpatch.pl will
> > catch for you.
> >
> >   CHECK:SPACING: No space is necessary after a cast
> >   #81: FILE: tools/testing/selftests/vfio/lib/vfio_pci_device.c:338:
> >   +       char *arg = (char *) bdf;
> >
> Actually, I did run checkpatch.pl on the entire series as:
> .$ ./scripts/checkpatch.pl *.patch
> 
> I didn't see any of these warnings. Are there any other options to consider?

Ah, I run with a few additional options. That's probably why we are
seeing different output. Here's what I have in my .bashrc:

function checkpatch() {
        scripts/checkpatch.pl \
                -q \
                --strict \
                --codespell \
                --no-signoff \
                --show-types \
                --ignore gerrit_change_id,FILE_PATH_CHANGES,NOT_UNIFIED_DIFF \
                --no-summary \
                "$@"
}

Re: [PATCH 4/4] vfio: selftests: Add tests to validate SR-IOV UAPI

[David Matlack]

On 2025-11-06 10:35 PM, Raghavendra Rao Ananta wrote:
> On Thu, Nov 6, 2025 at 6:30 AM David Matlack <dmatlack@google.com> wrote:
> >
> > On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> >
> > > +static const char *pf_dev_bdf;
> > > +static char vf_dev_bdf[16];
> >
> > vf_dev_bdf can be part of the test fixture instead of a global variable.
> > pf_dev_bdf should be the only global variable since we have to get it
> > from main() into the text fixture.
> >
> My understading is placing vars in FIXTURE() is helpful to get an
> access across various other FIXTURE_*() and TEST*() functions. Out of
> curiosity, is there an advantage here vs having them global?

Global variables are just generally a bad design pattern. IMO, only
variables that truly need to be global should be global.

The only variable that needs to be global is pf_dev_bdf.

Since vf_dev_bdf needs to be accessed within FIXTURE_SETUP(),
FIXTURE_TEARDOWN(), and TEST_F(), then FIXTURE() is the right home for
it. The whole point of FIXTURE() is to hold state for each TEST_F().

> 
> > > +
> > > +struct vfio_pci_device *pf_device;
> > > +struct vfio_pci_device *vf_device;
> >
> > These can be local variables in the places they are used.
> >
> I was a bit greedy to save a few lines, as they are reassigned in
> every TEST_F() anyway. Is there any advantage by making them local?

It's easy to mess up global variables. And also when reading the code it
is confusing to see a global variable that does not need to be global.
It makes me think I must be missing something.

As a general practice I think it's good to limit the scope of variables
to the minimum scope they are needed.

> > > +     snprintf(path, PATH_MAX, "%s/%s/sriov_numvfs", PCI_SYSFS_PATH, pf_dev_bdf);
> > > +     ASSERT_GT(fd = open(path, O_RDWR), 0);
> > > +     ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);
> > > +     nr_vfs = strtoul(buf, NULL, 0);
> > > +     if (nr_vfs == 0)
> >
> > If VFs are already enabled, shouldn't the test fail or skip?
> >
> My idea was to simply "steal" the device that was already created and
> use it. Do we want to skip it, as you suggested?

If a VF already exists it might be bound to a different driver, and may
be in use by something else. I think the only safe thing to do is to
bail if a VF already exists. If the test creates the VF, then it knows
that it owns it.

> > > +FIXTURE_TEARDOWN(vfio_pci_sriov_uapi_test)
> > > +{
> > > +}
> >
> > FIXTURE_TEARDOWN() should undo what FIXTURE_SETUP() did, i.e. write 0 to
> > sriov_numvfs. Otherwise running this test will leave behind SR-IOV
> > enabled on the PF.
> >
> I had this originally, but then realized that run.sh aready resets the
> sriov_numvfs to its original value. We can do it here too, if you'd
> like to keep the symmetry and make the test self-sufficient. With some
> of your other suggestions, I may have to do some more cleanup here
> now.

I think the test should return the PF back to the state it was in at the
start of the test. That way the test doesn't "leak" changes it made. The
best way to do that is to clean up in FIXTURE_TEARDOWN(). There might be
some other test that wants to run using the PF before run.sh does its
cleanup work.

> > You could also make this the users problem (the user has to provide a PF
> > with 1 VF where both PF and VF are bound to vfio-pci). But I think it
> > would be nice to make the test work automatically given a PF if we can.
> Let's go with the latter, assuming it doesn't get too complicated
> (currently, the setup part seems bigger than the actual test :) )

Let's create helpers for all the sysfs operations under lib.

e.g. tools/testing/selftests/vfio/lib/sysfs.c:

  int sysfs_get_sriov_totalvfs(const char *bdf);
  void sysfs_set_sriov_numvfs(const char *bdfs, int numvfs);
  ...

That will greatly simplify the amount of code in this test, and I think
it's highly likely we re-use those functions in other tests. And even if
we don't, it's nice to encapsulate all the sysfs code in one place for
readability and maintainability.

If you do this I think there's also some sysfs stuff in
vfio_pci_device.c that you can also pull out into this helper file.

Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[Raghavendra Rao Ananta]

On Thu, Nov 6, 2025 at 10:47 PM David Matlack <dmatlack@google.com> wrote:
>
> On 2025-11-06 09:56 PM, Raghavendra Rao Ananta wrote:
> > On Thu, Nov 6, 2025 at 5:22 AM David Matlack <dmatlack@google.com> wrote:
> > > On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> > >
> > > > -struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_mode);
> > > > +struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > > > +                                           const char *iommu_mode,
> > > > +                                           const char *vf_token);
> > >
> > > Vipin is also looking at adding an optional parameter to
> > > vfio_pci_device_init():
> > > https://lore.kernel.org/kvm/20251018000713.677779-20-vipinsh@google.com/
> > >
> > > I am wondering if we should support an options struct for such
> > > parameters. e.g. something like this
> > >
> > > diff --git a/tools/testing/selftests/vfio/lib/include/vfio_util.h b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> > > index b01068d98fda..cee837fe561c 100644
> > > --- a/tools/testing/selftests/vfio/lib/include/vfio_util.h
> > > +++ b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> > > @@ -160,6 +160,10 @@ struct vfio_pci_driver {
> > >         int msi;
> > >  };
> > >
> > > +struct vfio_pci_device_options {
> > > +       const char *vf_token;
> > > +};
> > > +
> > >  struct vfio_pci_device {
> > >         int fd;
> > >
> > > @@ -202,9 +206,18 @@ const char *vfio_pci_get_cdev_path(const char *bdf);
> > >
> > >  extern const char *default_iommu_mode;
> > >
> > > -struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > > -                                             const char *iommu_mode,
> > > -                                             const char *vf_token);
> > > +struct vfio_pci_device *__vfio_pci_device_init(const char *bdf,
> > > +                                              const char *iommu_mode,
> > > +                                              const struct vfio_pci_device_options *options);
> > > +
> > > +static inline struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > > +                                                          const char *iommu_mode)
> > > +{
> > > +       static const struct vfio_pci_device_options default_options = {};
> > > +
> > > +       return __vfio_pci_device_init(bdf, iommu_mode, &default_options);
> > > +}
> > > +
> > >
> > > This will avoid you having to update every test.
> > >
> > > You can create a helper function in vfio_pci_sriov_uapi_test.c to call
> > > __vfio_pci_device_init() and abstract away the options stuff from your
> > > test.
> > >
> > I like the idea of an optional expandable struct. I'll implement this in v2.
>
> Just to make sure we're on the same page: I don't think you need to add
> this in v2 since you don't need to call vfio_pci_device_init(). For the
> inner functions that you want to call from your test, passing vf_token
> directly makes more sense IMO. vfio_pci_device_init() will just pass in
> NULL to those functions for vf_token by default.
>
> If/when we want to pass vf_token to vfio_pci_device_init() we can add
> the options struct.
>
Yes, of course. I'll avoid it until we have a need.

> > > No space necessary after a cast. This is another one checkpatch.pl will
> > > catch for you.
> > >
> > >   CHECK:SPACING: No space is necessary after a cast
> > >   #81: FILE: tools/testing/selftests/vfio/lib/vfio_pci_device.c:338:
> > >   +       char *arg = (char *) bdf;
> > >
> > Actually, I did run checkpatch.pl on the entire series as:
> > .$ ./scripts/checkpatch.pl *.patch
> >
> > I didn't see any of these warnings. Are there any other options to consider?
>
> Ah, I run with a few additional options. That's probably why we are
> seeing different output. Here's what I have in my .bashrc:
>
> function checkpatch() {
>         scripts/checkpatch.pl \
>                 -q \
>                 --strict \
>                 --codespell \
>                 --no-signoff \
>                 --show-types \
>                 --ignore gerrit_change_id,FILE_PATH_CHANGES,NOT_UNIFIED_DIFF \
>                 --no-summary \
>                 "$@"
> }
Thanks! I'll give this a shot!

Re: [PATCH 4/4] vfio: selftests: Add tests to validate SR-IOV UAPI

[Raghavendra Rao Ananta]

On Thu, Nov 6, 2025 at 11:05 PM David Matlack <dmatlack@google.com> wrote:
>
> On 2025-11-06 10:35 PM, Raghavendra Rao Ananta wrote:
> > On Thu, Nov 6, 2025 at 6:30 AM David Matlack <dmatlack@google.com> wrote:
> > >
> > > On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> > >
> > > > +static const char *pf_dev_bdf;
> > > > +static char vf_dev_bdf[16];
> > >
> > > vf_dev_bdf can be part of the test fixture instead of a global variable.
> > > pf_dev_bdf should be the only global variable since we have to get it
> > > from main() into the text fixture.
> > >
> > My understading is placing vars in FIXTURE() is helpful to get an
> > access across various other FIXTURE_*() and TEST*() functions. Out of
> > curiosity, is there an advantage here vs having them global?
>
> Global variables are just generally a bad design pattern. IMO, only
> variables that truly need to be global should be global.
>
> The only variable that needs to be global is pf_dev_bdf.
>
> Since vf_dev_bdf needs to be accessed within FIXTURE_SETUP(),
> FIXTURE_TEARDOWN(), and TEST_F(), then FIXTURE() is the right home for
> it. The whole point of FIXTURE() is to hold state for each TEST_F().
>
Sounds good. I'll move them into FIXTURE().

> >
> > > > +
> > > > +struct vfio_pci_device *pf_device;
> > > > +struct vfio_pci_device *vf_device;
> > >
> > > These can be local variables in the places they are used.
> > >
> > I was a bit greedy to save a few lines, as they are reassigned in
> > every TEST_F() anyway. Is there any advantage by making them local?
>
> It's easy to mess up global variables. And also when reading the code it
> is confusing to see a global variable that does not need to be global.
> It makes me think I must be missing something.
>
> As a general practice I think it's good to limit the scope of variables
> to the minimum scope they are needed.
>
Agreed. I prefer min scope too, but I guess my habit of using global
variables in other tests and avoiding passing pointers around led me
to use it here. I'll move it to a local scope.

> > > > +     snprintf(path, PATH_MAX, "%s/%s/sriov_numvfs", PCI_SYSFS_PATH, pf_dev_bdf);
> > > > +     ASSERT_GT(fd = open(path, O_RDWR), 0);
> > > > +     ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);
> > > > +     nr_vfs = strtoul(buf, NULL, 0);
> > > > +     if (nr_vfs == 0)
> > >
> > > If VFs are already enabled, shouldn't the test fail or skip?
> > >
> > My idea was to simply "steal" the device that was already created and
> > use it. Do we want to skip it, as you suggested?
>
> If a VF already exists it might be bound to a different driver, and may
> be in use by something else. I think the only safe thing to do is to
> bail if a VF already exists. If the test creates the VF, then it knows
> that it owns it.
>
Makes sense. Let's skip in that case.

> > > > +FIXTURE_TEARDOWN(vfio_pci_sriov_uapi_test)
> > > > +{
> > > > +}
> > >
> > > FIXTURE_TEARDOWN() should undo what FIXTURE_SETUP() did, i.e. write 0 to
> > > sriov_numvfs. Otherwise running this test will leave behind SR-IOV
> > > enabled on the PF.
> > >
> > I had this originally, but then realized that run.sh aready resets the
> > sriov_numvfs to its original value. We can do it here too, if you'd
> > like to keep the symmetry and make the test self-sufficient. With some
> > of your other suggestions, I may have to do some more cleanup here
> > now.
>
> I think the test should return the PF back to the state it was in at the
> start of the test. That way the test doesn't "leak" changes it made. The
> best way to do that is to clean up in FIXTURE_TEARDOWN(). There might be
> some other test that wants to run using the PF before run.sh does its
> cleanup work.
>
Sure, I'll clean up everything that the test does in FIXTURE_SETUP().

> > > You could also make this the users problem (the user has to provide a PF
> > > with 1 VF where both PF and VF are bound to vfio-pci). But I think it
> > > would be nice to make the test work automatically given a PF if we can.
> > Let's go with the latter, assuming it doesn't get too complicated
> > (currently, the setup part seems bigger than the actual test :) )
>
> Let's create helpers for all the sysfs operations under lib.
>
> e.g. tools/testing/selftests/vfio/lib/sysfs.c:
>
>   int sysfs_get_sriov_totalvfs(const char *bdf);
>   void sysfs_set_sriov_numvfs(const char *bdfs, int numvfs);
>   ...
>
> That will greatly simplify the amount of code in this test, and I think
> it's highly likely we re-use those functions in other tests. And even if
> we don't, it's nice to encapsulate all the sysfs code in one place for
> readability and maintainability.
>
> If you do this I think there's also some sysfs stuff in
> vfio_pci_device.c that you can also pull out into this helper file.
Good idea. I'll create this lib.

Thank you.
Raghavendra