* [PATCH v6 1/8] vfio: selftests: Add -Wall and -Werror to the Makefile
2026-03-03 19:38 [PATCH v6 0/8] vfio: selftest: Add SR-IOV UAPI test Raghavendra Rao Ananta
@ 2026-03-03 19:38 ` Raghavendra Rao Ananta
2026-03-03 19:38 ` [PATCH v6 2/8] vfio: selftests: Introduce snprintf_assert() Raghavendra Rao Ananta
` (6 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-03 19:38 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Vipin Sharma, Josh Hilke, kvm, linux-kernel,
Raghavendra Rao Ananta
Add the compiler flags, -Wall and -Werror, to catch all the build
warnings and flag them as a build error, respectively. This is to
ensure that no obvious programmer errors are introduced. We can
add -Wno-* flags in the future to ignore specific warnings as necesasry.
Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
tools/testing/selftests/vfio/Makefile | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/vfio/Makefile b/tools/testing/selftests/vfio/Makefile
index 8e90e409e91d8..6a9ac6dd32cb6 100644
--- a/tools/testing/selftests/vfio/Makefile
+++ b/tools/testing/selftests/vfio/Makefile
@@ -23,6 +23,7 @@ include lib/libvfio.mk
CFLAGS += -I$(top_srcdir)/tools/include
CFLAGS += -MD
+CFLAGS += -Wall -Werror
CFLAGS += $(EXTRA_CFLAGS)
LDFLAGS += -pthread
--
2.53.0.473.g4a7958ca14-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v6 2/8] vfio: selftests: Introduce snprintf_assert()
2026-03-03 19:38 [PATCH v6 0/8] vfio: selftest: Add SR-IOV UAPI test Raghavendra Rao Ananta
2026-03-03 19:38 ` [PATCH v6 1/8] vfio: selftests: Add -Wall and -Werror to the Makefile Raghavendra Rao Ananta
@ 2026-03-03 19:38 ` Raghavendra Rao Ananta
2026-03-03 19:38 ` [PATCH v6 3/8] vfio: selftests: Introduce a sysfs lib Raghavendra Rao Ananta
` (5 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-03 19:38 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Vipin Sharma, Josh Hilke, kvm, linux-kernel,
Raghavendra Rao Ananta
Introduce snprintf_assert() to protect the users of snprintf() to fail
if the requested operation was truncated due to buffer limits. VFIO
tests and libraries, including a new sysfs library that will be introduced
by an upcoming patch, rely quite heavily on snprintf()s to build PCI
sysfs paths. Having a protection against this will be helpful to prevent
false test failures.
Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
.../vfio/lib/include/libvfio/assert.h | 5 +++++
.../selftests/vfio/lib/vfio_pci_device.c | 8 +++----
.../selftests/vfio/vfio_dma_mapping_test.c | 6 +++---
.../selftests/vfio/vfio_pci_device_test.c | 21 ++++++++++---------
4 files changed, 23 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/vfio/lib/include/libvfio/assert.h b/tools/testing/selftests/vfio/lib/include/libvfio/assert.h
index f4ebd122d9b6f..77b68c7129a64 100644
--- a/tools/testing/selftests/vfio/lib/include/libvfio/assert.h
+++ b/tools/testing/selftests/vfio/lib/include/libvfio/assert.h
@@ -51,4 +51,9 @@
VFIO_ASSERT_EQ(__ret, 0, "ioctl(%s, %s, %s) returned %d\n", #_fd, #_op, #_arg, __ret); \
} while (0)
+#define snprintf_assert(_s, _size, _fmt, ...) do { \
+ int __ret = snprintf(_s, _size, _fmt, ##__VA_ARGS__); \
+ VFIO_ASSERT_LT(__ret, _size); \
+} while (0)
+
#endif /* SELFTESTS_VFIO_LIB_INCLUDE_LIBVFIO_ASSERT_H */
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index 4e5871f1ebc3b..1538d2b30ae59 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -209,7 +209,7 @@ static unsigned int vfio_pci_get_group_from_dev(const char *bdf)
unsigned int group;
int ret;
- snprintf(sysfs_path, PATH_MAX, "%s/%s/iommu_group", PCI_SYSFS_PATH, bdf);
+ snprintf_assert(sysfs_path, PATH_MAX, "%s/%s/iommu_group", PCI_SYSFS_PATH, bdf);
ret = readlink(sysfs_path, dev_iommu_group_path, sizeof(dev_iommu_group_path));
VFIO_ASSERT_NE(ret, -1, "Failed to get the IOMMU group for device: %s\n", bdf);
@@ -229,7 +229,7 @@ static void vfio_pci_group_setup(struct vfio_pci_device *device, const char *bdf
int group;
group = vfio_pci_get_group_from_dev(bdf);
- snprintf(group_path, sizeof(group_path), "/dev/vfio/%d", group);
+ snprintf_assert(group_path, sizeof(group_path), "/dev/vfio/%d", group);
device->group_fd = open(group_path, O_RDWR);
VFIO_ASSERT_GE(device->group_fd, 0, "open(%s) failed\n", group_path);
@@ -300,7 +300,7 @@ const char *vfio_pci_get_cdev_path(const char *bdf)
cdev_path = calloc(PATH_MAX, 1);
VFIO_ASSERT_NOT_NULL(cdev_path);
- snprintf(dir_path, sizeof(dir_path), "/sys/bus/pci/devices/%s/vfio-dev/", bdf);
+ snprintf_assert(dir_path, sizeof(dir_path), "/sys/bus/pci/devices/%s/vfio-dev/", bdf);
dir = opendir(dir_path);
VFIO_ASSERT_NOT_NULL(dir, "Failed to open directory %s\n", dir_path);
@@ -310,7 +310,7 @@ const char *vfio_pci_get_cdev_path(const char *bdf)
if (strncmp("vfio", entry->d_name, 4))
continue;
- snprintf(cdev_path, PATH_MAX, "/dev/vfio/devices/%s", entry->d_name);
+ snprintf_assert(cdev_path, PATH_MAX, "/dev/vfio/devices/%s", entry->d_name);
break;
}
diff --git a/tools/testing/selftests/vfio/vfio_dma_mapping_test.c b/tools/testing/selftests/vfio/vfio_dma_mapping_test.c
index abb170bdcef7e..7d0de8c79de13 100644
--- a/tools/testing/selftests/vfio/vfio_dma_mapping_test.c
+++ b/tools/testing/selftests/vfio/vfio_dma_mapping_test.c
@@ -44,9 +44,9 @@ static int intel_iommu_mapping_get(const char *bdf, u64 iova,
FILE *file;
char *rest;
- snprintf(iommu_mapping_path, sizeof(iommu_mapping_path),
- "/sys/kernel/debug/iommu/intel/%s/domain_translation_struct",
- bdf);
+ snprintf_assert(iommu_mapping_path, sizeof(iommu_mapping_path),
+ "/sys/kernel/debug/iommu/intel/%s/domain_translation_struct",
+ bdf);
printf("Searching for IOVA 0x%lx in %s\n", iova, iommu_mapping_path);
diff --git a/tools/testing/selftests/vfio/vfio_pci_device_test.c b/tools/testing/selftests/vfio/vfio_pci_device_test.c
index 7c0fe8ce3a61f..93c11fd5e0818 100644
--- a/tools/testing/selftests/vfio/vfio_pci_device_test.c
+++ b/tools/testing/selftests/vfio/vfio_pci_device_test.c
@@ -39,16 +39,17 @@ FIXTURE_TEARDOWN(vfio_pci_device_test)
iommu_cleanup(self->iommu);
}
-#define read_pci_id_from_sysfs(_file) ({ \
- char __sysfs_path[PATH_MAX]; \
- char __buf[32]; \
- int __fd; \
- \
- snprintf(__sysfs_path, PATH_MAX, "/sys/bus/pci/devices/%s/%s", device_bdf, _file); \
- ASSERT_GT((__fd = open(__sysfs_path, O_RDONLY)), 0); \
- ASSERT_GT(read(__fd, __buf, ARRAY_SIZE(__buf)), 0); \
- ASSERT_EQ(0, close(__fd)); \
- (u16)strtoul(__buf, NULL, 0); \
+#define read_pci_id_from_sysfs(_file) ({ \
+ char __sysfs_path[PATH_MAX]; \
+ char __buf[32]; \
+ int __fd; \
+ \
+ snprintf_assert(__sysfs_path, PATH_MAX, "/sys/bus/pci/devices/%s/%s", \
+ device_bdf, _file); \
+ ASSERT_GT((__fd = open(__sysfs_path, O_RDONLY)), 0); \
+ ASSERT_GT(read(__fd, __buf, ARRAY_SIZE(__buf)), 0); \
+ ASSERT_EQ(0, close(__fd)); \
+ (u16)strtoul(__buf, NULL, 0); \
})
TEST_F(vfio_pci_device_test, config_space_read_write)
--
2.53.0.473.g4a7958ca14-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v6 3/8] vfio: selftests: Introduce a sysfs lib
2026-03-03 19:38 [PATCH v6 0/8] vfio: selftest: Add SR-IOV UAPI test Raghavendra Rao Ananta
2026-03-03 19:38 ` [PATCH v6 1/8] vfio: selftests: Add -Wall and -Werror to the Makefile Raghavendra Rao Ananta
2026-03-03 19:38 ` [PATCH v6 2/8] vfio: selftests: Introduce snprintf_assert() Raghavendra Rao Ananta
@ 2026-03-03 19:38 ` Raghavendra Rao Ananta
2026-03-04 22:53 ` David Matlack
2026-03-09 22:06 ` Vipin Sharma
2026-03-03 19:38 ` [PATCH v6 4/8] vfio: selftests: Extend container/iommufd setup for passing vf_token Raghavendra Rao Ananta
` (4 subsequent siblings)
7 siblings, 2 replies; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-03 19:38 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Vipin Sharma, Josh Hilke, kvm, linux-kernel,
Raghavendra Rao Ananta
Introduce a sysfs library to handle the common reads/writes to the
PCI sysfs files, for example, getting the total number of VFs supported
by the device via /sys/bus/pci/devices/$BDF/sriov_totalvfs. The library
will be used in the upcoming test patch to configure the VFs for a given
PF device.
Opportunistically, move vfio_pci_get_group_from_dev() to this library as
it falls under the same bucket. Rename it to sysfs_iommu_group_get() to
align with other function names.
Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
.../selftests/vfio/lib/include/libvfio.h | 1 +
.../vfio/lib/include/libvfio/sysfs.h | 12 ++
tools/testing/selftests/vfio/lib/libvfio.mk | 1 +
tools/testing/selftests/vfio/lib/sysfs.c | 144 ++++++++++++++++++
.../selftests/vfio/lib/vfio_pci_device.c | 22 +--
5 files changed, 159 insertions(+), 21 deletions(-)
create mode 100644 tools/testing/selftests/vfio/lib/include/libvfio/sysfs.h
create mode 100644 tools/testing/selftests/vfio/lib/sysfs.c
diff --git a/tools/testing/selftests/vfio/lib/include/libvfio.h b/tools/testing/selftests/vfio/lib/include/libvfio.h
index 1b6da54cc2cb7..07862b470777b 100644
--- a/tools/testing/selftests/vfio/lib/include/libvfio.h
+++ b/tools/testing/selftests/vfio/lib/include/libvfio.h
@@ -5,6 +5,7 @@
#include <libvfio/assert.h>
#include <libvfio/iommu.h>
#include <libvfio/iova_allocator.h>
+#include <libvfio/sysfs.h>
#include <libvfio/vfio_pci_device.h>
#include <libvfio/vfio_pci_driver.h>
diff --git a/tools/testing/selftests/vfio/lib/include/libvfio/sysfs.h b/tools/testing/selftests/vfio/lib/include/libvfio/sysfs.h
new file mode 100644
index 0000000000000..c48d5ef00ba6f
--- /dev/null
+++ b/tools/testing/selftests/vfio/lib/include/libvfio/sysfs.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef SELFTESTS_VFIO_LIB_INCLUDE_LIBVFIO_SYSFS_H
+#define SELFTESTS_VFIO_LIB_INCLUDE_LIBVFIO_SYSFS_H
+
+int sysfs_sriov_totalvfs_get(const char *bdf);
+int sysfs_sriov_numvfs_get(const char *bdf);
+void sysfs_sriov_numvfs_set(const char *bdfs, int numvfs);
+char *sysfs_sriov_vf_bdf_get(const char *pf_bdf, int i);
+unsigned int sysfs_iommu_group_get(const char *bdf);
+char *sysfs_driver_get(const char *bdf);
+
+#endif /* SELFTESTS_VFIO_LIB_INCLUDE_LIBVFIO_SYSFS_H */
diff --git a/tools/testing/selftests/vfio/lib/libvfio.mk b/tools/testing/selftests/vfio/lib/libvfio.mk
index 9f47bceed16f4..b7857319c3f1f 100644
--- a/tools/testing/selftests/vfio/lib/libvfio.mk
+++ b/tools/testing/selftests/vfio/lib/libvfio.mk
@@ -6,6 +6,7 @@ LIBVFIO_SRCDIR := $(selfdir)/vfio/lib
LIBVFIO_C := iommu.c
LIBVFIO_C += iova_allocator.c
LIBVFIO_C += libvfio.c
+LIBVFIO_C += sysfs.c
LIBVFIO_C += vfio_pci_device.c
LIBVFIO_C += vfio_pci_driver.c
diff --git a/tools/testing/selftests/vfio/lib/sysfs.c b/tools/testing/selftests/vfio/lib/sysfs.c
new file mode 100644
index 0000000000000..1fec6c7a7fce7
--- /dev/null
+++ b/tools/testing/selftests/vfio/lib/sysfs.c
@@ -0,0 +1,144 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <fcntl.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <string.h>
+#include <linux/limits.h>
+
+#include <libvfio.h>
+
+static int sysfs_val_get_int(const char *component, const char *name,
+ const char *file)
+{
+ char path[PATH_MAX];
+ char buf[32];
+ int ret;
+ int fd;
+
+ snprintf_assert(path, PATH_MAX, "/sys/bus/pci/%s/%s/%s", component, name, file);
+ fd = open(path, O_RDONLY);
+ if (fd < 0)
+ return fd;
+
+ VFIO_ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);
+ VFIO_ASSERT_EQ(close(fd), 0);
+
+ errno = 0;
+ ret = strtol(buf, NULL, 0);
+ VFIO_ASSERT_EQ(errno, 0, "sysfs path \"%s\" is not an integer: \"%s\"\n", path, buf);
+
+ return ret;
+}
+
+static void sysfs_val_set(const char *component, const char *name,
+ const char *file, const char *val)
+{
+ char path[PATH_MAX];
+ int fd;
+
+ snprintf_assert(path, PATH_MAX, "/sys/bus/pci/%s/%s/%s", component, name, file);
+ VFIO_ASSERT_GT(fd = open(path, O_WRONLY), 0);
+
+ VFIO_ASSERT_EQ(write(fd, val, strlen(val)), strlen(val));
+ VFIO_ASSERT_EQ(close(fd), 0);
+}
+
+static int sysfs_device_val_get(const char *bdf, const char *file)
+{
+ return sysfs_val_get_int("devices", bdf, file);
+}
+
+static void sysfs_device_val_set(const char *bdf, const char *file, const char *val)
+{
+ sysfs_val_set("devices", bdf, file, val);
+}
+
+static void sysfs_device_val_set_int(const char *bdf, const char *file, int val)
+{
+ char val_str[32];
+
+ snprintf_assert(val_str, sizeof(val_str), "%d", val);
+ sysfs_device_val_set(bdf, file, val_str);
+}
+
+int sysfs_sriov_totalvfs_get(const char *bdf)
+{
+ return sysfs_device_val_get(bdf, "sriov_totalvfs");
+}
+
+int sysfs_sriov_numvfs_get(const char *bdf)
+{
+ return sysfs_device_val_get(bdf, "sriov_numvfs");
+}
+
+void sysfs_sriov_numvfs_set(const char *bdf, int numvfs)
+{
+ sysfs_device_val_set_int(bdf, "sriov_numvfs", numvfs);
+}
+
+char *sysfs_sriov_vf_bdf_get(const char *pf_bdf, int i)
+{
+ char vf_path[PATH_MAX];
+ char path[PATH_MAX];
+ char *out_vf_bdf;
+ int ret;
+
+ out_vf_bdf = calloc(16, sizeof(char));
+ VFIO_ASSERT_NOT_NULL(out_vf_bdf);
+
+ snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/virtfn%d", pf_bdf, i);
+
+ ret = readlink(path, vf_path, PATH_MAX);
+ VFIO_ASSERT_NE(ret, -1);
+ vf_path[ret] = '\0';
+
+ ret = sscanf(basename(vf_path), "%s", out_vf_bdf);
+ VFIO_ASSERT_EQ(ret, 1);
+
+ return out_vf_bdf;
+}
+
+unsigned int sysfs_iommu_group_get(const char *bdf)
+{
+ char dev_iommu_group_path[PATH_MAX];
+ char path[PATH_MAX];
+ unsigned int group;
+ int ret;
+
+ snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/iommu_group", bdf);
+
+ ret = readlink(path, dev_iommu_group_path, sizeof(dev_iommu_group_path));
+ VFIO_ASSERT_NE(ret, -1, "Failed to get the IOMMU group for device: %s\n", bdf);
+ dev_iommu_group_path[ret] = '\0';
+
+ ret = sscanf(basename(dev_iommu_group_path), "%u", &group);
+ VFIO_ASSERT_EQ(ret, 1, "Failed to get the IOMMU group for device: %s\n", bdf);
+
+ return group;
+}
+
+char *sysfs_driver_get(const char *bdf)
+{
+ char driver_path[PATH_MAX];
+ char path[PATH_MAX];
+ char *out_driver;
+ int ret;
+
+ out_driver = calloc(64, sizeof(char));
+ VFIO_ASSERT_NOT_NULL(out_driver);
+
+ snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/driver", bdf);
+ ret = readlink(path, driver_path, PATH_MAX);
+ if (ret == -1) {
+ free(out_driver);
+
+ if (errno == ENOENT)
+ return NULL;
+
+ VFIO_FAIL("Failed to read %s\n", path);
+ }
+ driver_path[ret] = '\0';
+
+ strcpy(out_driver, basename(driver_path));
+ return out_driver;
+}
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index 1538d2b30ae59..82f255f0486dc 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -25,8 +25,6 @@
#include "kselftest.h"
#include <libvfio.h>
-#define PCI_SYSFS_PATH "/sys/bus/pci/devices"
-
static void vfio_pci_irq_set(struct vfio_pci_device *device,
u32 index, u32 vector, u32 count, int *fds)
{
@@ -202,24 +200,6 @@ void vfio_pci_device_reset(struct vfio_pci_device *device)
ioctl_assert(device->fd, VFIO_DEVICE_RESET, NULL);
}
-static unsigned int vfio_pci_get_group_from_dev(const char *bdf)
-{
- char dev_iommu_group_path[PATH_MAX] = {0};
- char sysfs_path[PATH_MAX] = {0};
- unsigned int group;
- int ret;
-
- snprintf_assert(sysfs_path, PATH_MAX, "%s/%s/iommu_group", PCI_SYSFS_PATH, bdf);
-
- ret = readlink(sysfs_path, dev_iommu_group_path, sizeof(dev_iommu_group_path));
- VFIO_ASSERT_NE(ret, -1, "Failed to get the IOMMU group for device: %s\n", bdf);
-
- ret = sscanf(basename(dev_iommu_group_path), "%u", &group);
- VFIO_ASSERT_EQ(ret, 1, "Failed to get the IOMMU group for device: %s\n", bdf);
-
- return group;
-}
-
static void vfio_pci_group_setup(struct vfio_pci_device *device, const char *bdf)
{
struct vfio_group_status group_status = {
@@ -228,7 +208,7 @@ static void vfio_pci_group_setup(struct vfio_pci_device *device, const char *bdf
char group_path[32];
int group;
- group = vfio_pci_get_group_from_dev(bdf);
+ group = sysfs_iommu_group_get(bdf);
snprintf_assert(group_path, sizeof(group_path), "/dev/vfio/%d", group);
device->group_fd = open(group_path, O_RDWR);
--
2.53.0.473.g4a7958ca14-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 3/8] vfio: selftests: Introduce a sysfs lib
2026-03-03 19:38 ` [PATCH v6 3/8] vfio: selftests: Introduce a sysfs lib Raghavendra Rao Ananta
@ 2026-03-04 22:53 ` David Matlack
2026-03-31 21:27 ` Raghavendra Rao Ananta
2026-03-09 22:06 ` Vipin Sharma
1 sibling, 1 reply; 21+ messages in thread
From: David Matlack @ 2026-03-04 22:53 UTC (permalink / raw)
To: Raghavendra Rao Ananta
Cc: Alex Williamson, Vipin Sharma, Josh Hilke, kvm, linux-kernel
On 2026-03-03 07:38 PM, Raghavendra Rao Ananta wrote:
> + ret = readlink(path, vf_path, PATH_MAX);
> + VFIO_ASSERT_NE(ret, -1);
> + vf_path[ret] = '\0';
...
> + ret = readlink(path, dev_iommu_group_path, sizeof(dev_iommu_group_path));
> + VFIO_ASSERT_NE(ret, -1, "Failed to get the IOMMU group for device: %s\n", bdf);
> + dev_iommu_group_path[ret] = '\0';
...
> + ret = readlink(path, driver_path, PATH_MAX);
> + if (ret == -1) {
> + free(out_driver);
> +
> + if (errno == ENOENT)
> + return NULL;
> +
> + VFIO_FAIL("Failed to read %s\n", path);
> + }
> + driver_path[ret] = '\0';
These can all write off the end of the buffer if ret == PATH_MAX.
Also there is an inconsistency in these calls. 2 use hard-coded PATH_MAX
while the other uses sizeof().
Perhaps add a wrapper to handle all the details?
#define readlink_safe(path, buf) ({ \
int __ret = readlink(_path, _buf, sizeof(_buf) - 1); \
if (__ret != -1) \
_buf[__ret] = 0; \
__ret;
})
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 3/8] vfio: selftests: Introduce a sysfs lib
2026-03-04 22:53 ` David Matlack
@ 2026-03-31 21:27 ` Raghavendra Rao Ananta
0 siblings, 0 replies; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-31 21:27 UTC (permalink / raw)
To: David Matlack
Cc: Alex Williamson, Vipin Sharma, Josh Hilke, kvm, linux-kernel
On Wed, Mar 4, 2026 at 2:53 PM David Matlack <dmatlack@google.com> wrote:
>
> On 2026-03-03 07:38 PM, Raghavendra Rao Ananta wrote:
>
> > + ret = readlink(path, vf_path, PATH_MAX);
> > + VFIO_ASSERT_NE(ret, -1);
> > + vf_path[ret] = '\0';
>
> ...
>
> > + ret = readlink(path, dev_iommu_group_path, sizeof(dev_iommu_group_path));
> > + VFIO_ASSERT_NE(ret, -1, "Failed to get the IOMMU group for device: %s\n", bdf);
> > + dev_iommu_group_path[ret] = '\0';
>
> ...
>
> > + ret = readlink(path, driver_path, PATH_MAX);
> > + if (ret == -1) {
> > + free(out_driver);
> > +
> > + if (errno == ENOENT)
> > + return NULL;
> > +
> > + VFIO_FAIL("Failed to read %s\n", path);
> > + }
> > + driver_path[ret] = '\0';
>
> These can all write off the end of the buffer if ret == PATH_MAX.
>
> Also there is an inconsistency in these calls. 2 use hard-coded PATH_MAX
> while the other uses sizeof().
>
> Perhaps add a wrapper to handle all the details?
>
> #define readlink_safe(path, buf) ({ \
> int __ret = readlink(_path, _buf, sizeof(_buf) - 1); \
> if (__ret != -1) \
> _buf[__ret] = 0; \
> __ret;
> })
Yeah, good idea. I'll consider this in v7.
Thank you.
Raghavendra
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v6 3/8] vfio: selftests: Introduce a sysfs lib
2026-03-03 19:38 ` [PATCH v6 3/8] vfio: selftests: Introduce a sysfs lib Raghavendra Rao Ananta
2026-03-04 22:53 ` David Matlack
@ 2026-03-09 22:06 ` Vipin Sharma
2026-03-31 21:34 ` Raghavendra Rao Ananta
1 sibling, 1 reply; 21+ messages in thread
From: Vipin Sharma @ 2026-03-09 22:06 UTC (permalink / raw)
To: Raghavendra Rao Ananta
Cc: David Matlack, Alex Williamson, Josh Hilke, kvm, linux-kernel
On 2026-03-03 19:38:17, Raghavendra Rao Ananta wrote:
> +char *sysfs_sriov_vf_bdf_get(const char *pf_bdf, int i)
> +{
> + char vf_path[PATH_MAX];
> + char path[PATH_MAX];
> + char *out_vf_bdf;
> + int ret;
> +
> + out_vf_bdf = calloc(16, sizeof(char));
A comment of /* ../0000:00:00.0 */ would be nice to tell why 16 is
chosen.
> + VFIO_ASSERT_NOT_NULL(out_vf_bdf);
> +
> + snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/virtfn%d", pf_bdf, i);
> +
> + ret = readlink(path, vf_path, PATH_MAX);
> + VFIO_ASSERT_NE(ret, -1);
> + vf_path[ret] = '\0';
Can we just initialize vf_path to {0} at the beginning and not worry
here?
> +
> + ret = sscanf(basename(vf_path), "%s", out_vf_bdf);
> + VFIO_ASSERT_EQ(ret, 1);
> +
> + return out_vf_bdf;
> +}
> +
> +unsigned int sysfs_iommu_group_get(const char *bdf)
> +{
> + char dev_iommu_group_path[PATH_MAX];
> + char path[PATH_MAX];
> + unsigned int group;
Why unsigned int? In kernel iommu_group id is int itself. Caller of this
function also casting it to int.
> + int ret;
> +
> + snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/iommu_group", bdf);
> +
> + ret = readlink(path, dev_iommu_group_path, sizeof(dev_iommu_group_path));
> + VFIO_ASSERT_NE(ret, -1, "Failed to get the IOMMU group for device: %s\n", bdf);
> + dev_iommu_group_path[ret] = '\0';
Same, initialize at beginning.
> +
> + ret = sscanf(basename(dev_iommu_group_path), "%u", &group);
Can we combine these operations into a single function? Seems like vfio,
iommu_group and driver all use the same pattern.
> + VFIO_ASSERT_EQ(ret, 1, "Failed to get the IOMMU group for device: %s\n", bdf);
> +
> + return group;
> +}
> +
> +char *sysfs_driver_get(const char *bdf)
> +{
> + char driver_path[PATH_MAX];
> + char path[PATH_MAX];
> + char *out_driver;
> + int ret;
> +
> + out_driver = calloc(64, sizeof(char));
Comment on why 64?
> + VFIO_ASSERT_NOT_NULL(out_driver);
> +
> + snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/driver", bdf);
> + ret = readlink(path, driver_path, PATH_MAX);
> + if (ret == -1) {
> + free(out_driver);
> +
> + if (errno == ENOENT)
> + return NULL;
> +
> + VFIO_FAIL("Failed to read %s\n", path);
> + }
> + driver_path[ret] = '\0';
Same, initialize at beginning.
> +
> + strcpy(out_driver, basename(driver_path));
They both have different lengths. Should we use strncpy()?
> + return out_driver;
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 3/8] vfio: selftests: Introduce a sysfs lib
2026-03-09 22:06 ` Vipin Sharma
@ 2026-03-31 21:34 ` Raghavendra Rao Ananta
0 siblings, 0 replies; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-31 21:34 UTC (permalink / raw)
To: Vipin Sharma
Cc: David Matlack, Alex Williamson, Josh Hilke, kvm, linux-kernel
On Mon, Mar 9, 2026 at 3:06 PM Vipin Sharma <vipinsh@google.com> wrote:
>
> On 2026-03-03 19:38:17, Raghavendra Rao Ananta wrote:
> > +char *sysfs_sriov_vf_bdf_get(const char *pf_bdf, int i)
> > +{
> > + char vf_path[PATH_MAX];
> > + char path[PATH_MAX];
> > + char *out_vf_bdf;
> > + int ret;
> > +
> > + out_vf_bdf = calloc(16, sizeof(char));
>
> A comment of /* ../0000:00:00.0 */ would be nice to tell why 16 is
> chosen.
>
Sure, will add it in v7.
> > + VFIO_ASSERT_NOT_NULL(out_vf_bdf);
> > +
> > + snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/virtfn%d", pf_bdf, i);
> > +
> > + ret = readlink(path, vf_path, PATH_MAX);
> > + VFIO_ASSERT_NE(ret, -1);
> > + vf_path[ret] = '\0';
>
> Can we just initialize vf_path to {0} at the beginning and not worry
> here?
>
Hmm, we can but we don't want to fill 4K (PATH_MAX) worth of 0s for
every function call. Hence, I preferred to set it after the call to
readlink(). This also seems to be the common practise, at least by
looking in the "tools/" dir. With David's suggestion of introducing
readlink_safe(), the caller need not even worry about this. Same
response to the similar comments below.
> > +
> > + ret = sscanf(basename(vf_path), "%s", out_vf_bdf);
> > + VFIO_ASSERT_EQ(ret, 1);
> > +
> > + return out_vf_bdf;
> > +}
> > +
> > +unsigned int sysfs_iommu_group_get(const char *bdf)
> > +{
> > + char dev_iommu_group_path[PATH_MAX];
> > + char path[PATH_MAX];
> > + unsigned int group;
>
> Why unsigned int? In kernel iommu_group id is int itself. Caller of this
> function also casting it to int.
>
Ah, it should be 'int'. I guess I copied the original code as is. I'll
update it in v7.
> > + int ret;
> > +
> > + snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/iommu_group", bdf);
> > +
> > + ret = readlink(path, dev_iommu_group_path, sizeof(dev_iommu_group_path));
> > + VFIO_ASSERT_NE(ret, -1, "Failed to get the IOMMU group for device: %s\n", bdf);
> > + dev_iommu_group_path[ret] = '\0';
>
> Same, initialize at beginning.
>
> > +
> > + ret = sscanf(basename(dev_iommu_group_path), "%u", &group);
>
> Can we combine these operations into a single function? Seems like vfio,
> iommu_group and driver all use the same pattern.
>
I think we can, at least for the vf_driver and the iommu_group. The
'sysfs_driver_get' handles things slightly differently. I'll update in
v7.
> > + VFIO_ASSERT_EQ(ret, 1, "Failed to get the IOMMU group for device: %s\n", bdf);
> > +
> > + return group;
> > +}
> > +
> > +char *sysfs_driver_get(const char *bdf)
> > +{
> > + char driver_path[PATH_MAX];
> > + char path[PATH_MAX];
> > + char *out_driver;
> > + int ret;
> > +
> > + out_driver = calloc(64, sizeof(char));
>
> Comment on why 64?
>
Will do in v7.
> > + VFIO_ASSERT_NOT_NULL(out_driver);
> > +
> > + snprintf_assert(path, PATH_MAX, "/sys/bus/pci/devices/%s/driver", bdf);
> > + ret = readlink(path, driver_path, PATH_MAX);
> > + if (ret == -1) {
> > + free(out_driver);
> > +
> > + if (errno == ENOENT)
> > + return NULL;
> > +
> > + VFIO_FAIL("Failed to read %s\n", path);
> > + }
> > + driver_path[ret] = '\0';
>
> Same, initialize at beginning.
>
> > +
> > + strcpy(out_driver, basename(driver_path));
>
> They both have different lengths. Should we use strncpy()?
>
The assumption was that 'out_driver' is allocated to accommodate a max
driver length and should fit any driver name. But I can switch to
strncpy() to make this more clear in v7.
Thank you.
Raghavendra
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v6 4/8] vfio: selftests: Extend container/iommufd setup for passing vf_token
2026-03-03 19:38 [PATCH v6 0/8] vfio: selftest: Add SR-IOV UAPI test Raghavendra Rao Ananta
` (2 preceding siblings ...)
2026-03-03 19:38 ` [PATCH v6 3/8] vfio: selftests: Introduce a sysfs lib Raghavendra Rao Ananta
@ 2026-03-03 19:38 ` Raghavendra Rao Ananta
2026-03-03 19:38 ` [PATCH v6 5/8] vfio: selftests: Expose more vfio_pci_device functions Raghavendra Rao Ananta
` (3 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-03 19:38 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Vipin Sharma, Josh Hilke, kvm, linux-kernel,
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 functions,
vfio_pci_iommufd_setup() and vfio_pci_container_setup(), to accept
vf_token as an (optional) argument and handle the necessary setup.
No functional changes are expected.
Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
tools/testing/selftests/vfio/Makefile | 2 +
.../selftests/vfio/lib/vfio_pci_device.c | 46 +++++++++++++++----
2 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/tools/testing/selftests/vfio/Makefile b/tools/testing/selftests/vfio/Makefile
index 6a9ac6dd32cb6..f27ed18070f14 100644
--- a/tools/testing/selftests/vfio/Makefile
+++ b/tools/testing/selftests/vfio/Makefile
@@ -28,6 +28,8 @@ CFLAGS += $(EXTRA_CFLAGS)
LDFLAGS += -pthread
+LDLIBS += -luuid
+
$(TEST_GEN_PROGS): %: %.o $(LIBVFIO_O)
$(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) $< $(LIBVFIO_O) $(LDLIBS) -o $@
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index 82f255f0486dc..dc8b37df8d1f1 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -22,6 +22,8 @@
#include <linux/types.h>
#include <linux/vfio.h>
+#include <uuid/uuid.h>
+
#include "kselftest.h"
#include <libvfio.h>
@@ -220,7 +222,27 @@ static void vfio_pci_group_setup(struct vfio_pci_device *device, const char *bdf
ioctl_assert(device->group_fd, VFIO_GROUP_SET_CONTAINER, &device->iommu->container_fd);
}
-static void vfio_pci_container_setup(struct vfio_pci_device *device, const char *bdf)
+static void vfio_pci_group_get_device_fd(struct vfio_pci_device *device,
+ const char *bdf, const char *vf_token)
+{
+ char arg[64];
+
+ /*
+ * 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)
+ snprintf_assert(arg, ARRAY_SIZE(arg), "%s vf_token=%s", bdf, vf_token);
+ else
+ snprintf_assert(arg, ARRAY_SIZE(arg), "%s", bdf);
+
+ device->fd = ioctl(device->group_fd, VFIO_GROUP_GET_DEVICE_FD, 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)
{
struct iommu *iommu = device->iommu;
unsigned long iommu_type = iommu->mode->iommu_type;
@@ -238,8 +260,7 @@ static void vfio_pci_container_setup(struct vfio_pci_device *device, const char
*/
(void)ioctl(iommu->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_group_get_device_fd(device, bdf, vf_token);
}
static void vfio_pci_device_setup(struct vfio_pci_device *device)
@@ -300,12 +321,20 @@ const char *vfio_pci_get_cdev_path(const char *bdf)
return cdev_path;
}
-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;
+
+ 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);
}
@@ -320,7 +349,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);
@@ -328,7 +358,7 @@ 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_bind_iommufd(device->fd, device->iommu->iommufd, vf_token);
vfio_device_attach_iommufd_pt(device->fd, device->iommu->ioas_id);
}
@@ -344,9 +374,9 @@ struct vfio_pci_device *vfio_pci_device_init(const char *bdf, struct iommu *iomm
device->bdf = bdf;
if (iommu->mode->container_path)
- vfio_pci_container_setup(device, bdf);
+ vfio_pci_container_setup(device, bdf, NULL);
else
- vfio_pci_iommufd_setup(device, bdf);
+ vfio_pci_iommufd_setup(device, bdf, NULL);
vfio_pci_device_setup(device);
vfio_pci_driver_probe(device);
--
2.53.0.473.g4a7958ca14-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v6 5/8] vfio: selftests: Expose more vfio_pci_device functions
2026-03-03 19:38 [PATCH v6 0/8] vfio: selftest: Add SR-IOV UAPI test Raghavendra Rao Ananta
` (3 preceding siblings ...)
2026-03-03 19:38 ` [PATCH v6 4/8] vfio: selftests: Extend container/iommufd setup for passing vf_token Raghavendra Rao Ananta
@ 2026-03-03 19:38 ` Raghavendra Rao Ananta
2026-03-03 19:38 ` [PATCH v6 6/8] vfio: selftests: Add helper to set/override a vf_token Raghavendra Rao Ananta
` (2 subsequent siblings)
7 siblings, 0 replies; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-03 19:38 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Vipin Sharma, Josh Hilke, kvm, linux-kernel,
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>
Reviewed-by: David Matlack <dmatlack@google.com>
---
.../lib/include/libvfio/vfio_pci_device.h | 7 +++
.../selftests/vfio/lib/vfio_pci_device.c | 47 ++++++++++++++-----
2 files changed, 42 insertions(+), 12 deletions(-)
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 2858885a89bbb..898de032fed5a 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
@@ -122,4 +122,11 @@ static inline bool vfio_pci_device_match(struct vfio_pci_device *device,
const char *vfio_pci_get_cdev_path(const char *bdf);
+void vfio_pci_group_setup(struct vfio_pci_device *device, const char *bdf);
+void __vfio_pci_group_get_device_fd(struct vfio_pci_device *device,
+ const char *bdf, const char *vf_token);
+void vfio_container_set_iommu(struct vfio_pci_device *device);
+void vfio_pci_cdev_open(struct vfio_pci_device *device, const char *bdf);
+int __vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token);
+
#endif /* SELFTESTS_VFIO_LIB_INCLUDE_LIBVFIO_VFIO_PCI_DEVICE_H */
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index dc8b37df8d1f1..3123ba591f088 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -202,7 +202,7 @@ void vfio_pci_device_reset(struct vfio_pci_device *device)
ioctl_assert(device->fd, VFIO_DEVICE_RESET, NULL);
}
-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),
@@ -222,8 +222,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->iommu->container_fd);
}
-static void vfio_pci_group_get_device_fd(struct vfio_pci_device *device,
- const char *bdf, const char *vf_token)
+void __vfio_pci_group_get_device_fd(struct vfio_pci_device *device,
+ const char *bdf, const char *vf_token)
{
char arg[64];
@@ -238,18 +238,21 @@ static void vfio_pci_group_get_device_fd(struct vfio_pci_device *device,
snprintf_assert(arg, ARRAY_SIZE(arg), "%s", bdf);
device->fd = ioctl(device->group_fd, VFIO_GROUP_GET_DEVICE_FD, arg);
+}
+
+static void vfio_pci_group_get_device_fd(struct vfio_pci_device *device,
+ const char *bdf, const char *vf_token)
+{
+ __vfio_pci_group_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)
{
struct iommu *iommu = device->iommu;
unsigned long iommu_type = iommu->mode->iommu_type;
int ret;
- vfio_pci_group_setup(device, bdf);
-
ret = ioctl(iommu->container_fd, VFIO_CHECK_EXTENSION, iommu_type);
VFIO_ASSERT_GT(ret, 0, "VFIO IOMMU type %lu not supported\n", iommu_type);
@@ -259,7 +262,13 @@ static void vfio_pci_container_setup(struct vfio_pci_device *device,
* because the IOMMU type is already set.
*/
(void)ioctl(iommu->container_fd, VFIO_SET_IOMMU, (void *)iommu_type);
+}
+static void vfio_pci_container_setup(struct vfio_pci_device *device,
+ const char *bdf, const char *vf_token)
+{
+ vfio_pci_group_setup(device, bdf);
+ vfio_container_set_iommu(device);
vfio_pci_group_get_device_fd(device, bdf, vf_token);
}
@@ -321,8 +330,7 @@ const char *vfio_pci_get_cdev_path(const char *bdf)
return cdev_path;
}
-static void vfio_device_bind_iommufd(int device_fd, int iommufd,
- const char *vf_token)
+int __vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token)
{
struct vfio_device_bind_iommufd args = {
.argsz = sizeof(args),
@@ -336,7 +344,18 @@ static void vfio_device_bind_iommufd(int device_fd, int iommufd,
args.token_uuid_ptr = (u64)token_uuid;
}
- ioctl_assert(device_fd, VFIO_DEVICE_BIND_IOMMUFD, &args);
+ if (ioctl(device_fd, VFIO_DEVICE_BIND_IOMMUFD, &args))
+ return -errno;
+
+ return 0;
+}
+
+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 void vfio_device_attach_iommufd_pt(int device_fd, u32 pt_id)
@@ -349,15 +368,19 @@ 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_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);
+}
+static void vfio_pci_iommufd_setup(struct vfio_pci_device *device,
+ const char *bdf, const char *vf_token)
+{
+ vfio_pci_cdev_open(device, bdf);
vfio_device_bind_iommufd(device->fd, device->iommu->iommufd, vf_token);
vfio_device_attach_iommufd_pt(device->fd, device->iommu->ioas_id);
}
--
2.53.0.473.g4a7958ca14-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v6 6/8] vfio: selftests: Add helper to set/override a vf_token
2026-03-03 19:38 [PATCH v6 0/8] vfio: selftest: Add SR-IOV UAPI test Raghavendra Rao Ananta
` (4 preceding siblings ...)
2026-03-03 19:38 ` [PATCH v6 5/8] vfio: selftests: Expose more vfio_pci_device functions Raghavendra Rao Ananta
@ 2026-03-03 19:38 ` Raghavendra Rao Ananta
2026-03-03 19:38 ` [PATCH v6 7/8] vfio: selftests: Add helpers to alloc/free vfio_pci_device Raghavendra Rao Ananta
2026-03-03 19:38 ` [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI Raghavendra Rao Ananta
7 siblings, 0 replies; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-03 19:38 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Vipin Sharma, Josh Hilke, kvm, linux-kernel,
Raghavendra Rao Ananta
Add a helper function, vfio_device_set_vf_token(), to set or override a
vf_token. 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. Hence, add an API to utilize this
functionality from the test code. The subsequent commit will use this to
test the functionality of this method to set the vf_token.
Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
.../lib/include/libvfio/vfio_pci_device.h | 2 ++
.../selftests/vfio/lib/vfio_pci_device.c | 34 +++++++++++++++++++
2 files changed, 36 insertions(+)
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 898de032fed5a..4ebdc00e20fca 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
@@ -129,4 +129,6 @@ void vfio_container_set_iommu(struct vfio_pci_device *device);
void vfio_pci_cdev_open(struct vfio_pci_device *device, const char *bdf);
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_LIBVFIO_VFIO_PCI_DEVICE_H */
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index 3123ba591f088..4673b148f8c44 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -113,6 +113,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_pci_region_get(struct vfio_pci_device *device, int index,
struct vfio_region_info *info)
{
--
2.53.0.473.g4a7958ca14-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v6 7/8] vfio: selftests: Add helpers to alloc/free vfio_pci_device
2026-03-03 19:38 [PATCH v6 0/8] vfio: selftest: Add SR-IOV UAPI test Raghavendra Rao Ananta
` (5 preceding siblings ...)
2026-03-03 19:38 ` [PATCH v6 6/8] vfio: selftests: Add helper to set/override a vf_token Raghavendra Rao Ananta
@ 2026-03-03 19:38 ` Raghavendra Rao Ananta
2026-03-03 19:38 ` [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI Raghavendra Rao Ananta
7 siblings, 0 replies; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-03 19:38 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Vipin Sharma, Josh Hilke, kvm, linux-kernel,
Raghavendra Rao Ananta
Add a helper, vfio_pci_device_alloc(), to allocate 'struct
vfio_pci_device'. The subsequent test patch will utilize this
to get the struct with very minimal initialization done.
Internally, let vfio_pci_device_init() also make use of this
function and later do the full initialization.
Symmetrically, add a free variant, vfio_pci_device_free(),
to be used in a similar fashion.
No functional change intended.
Signed-off-by: Raghavendra Rao Ananta <rananta@google.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
.../vfio/lib/include/libvfio/vfio_pci_device.h | 2 ++
.../selftests/vfio/lib/vfio_pci_device.c | 18 ++++++++++++++++--
2 files changed, 18 insertions(+), 2 deletions(-)
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 4ebdc00e20fca..3eabead717bbd 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
@@ -38,6 +38,8 @@ struct vfio_pci_device {
#define dev_info(_dev, _fmt, ...) printf("%s: " _fmt, (_dev)->bdf, ##__VA_ARGS__)
#define dev_err(_dev, _fmt, ...) fprintf(stderr, "%s: " _fmt, (_dev)->bdf, ##__VA_ARGS__)
+struct vfio_pci_device *vfio_pci_device_alloc(const char *bdf, struct iommu *iommu);
+void vfio_pci_device_free(struct vfio_pci_device *device);
struct vfio_pci_device *vfio_pci_device_init(const char *bdf, struct iommu *iommu);
void vfio_pci_device_cleanup(struct vfio_pci_device *device);
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index 4673b148f8c44..4ff76970e3791 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -419,7 +419,7 @@ static void vfio_pci_iommufd_setup(struct vfio_pci_device *device,
vfio_device_attach_iommufd_pt(device->fd, device->iommu->ioas_id);
}
-struct vfio_pci_device *vfio_pci_device_init(const char *bdf, struct iommu *iommu)
+struct vfio_pci_device *vfio_pci_device_alloc(const char *bdf, struct iommu *iommu)
{
struct vfio_pci_device *device;
@@ -430,6 +430,20 @@ struct vfio_pci_device *vfio_pci_device_init(const char *bdf, struct iommu *iomm
device->iommu = iommu;
device->bdf = bdf;
+ return device;
+}
+
+void vfio_pci_device_free(struct vfio_pci_device *device)
+{
+ free(device);
+}
+
+struct vfio_pci_device *vfio_pci_device_init(const char *bdf, struct iommu *iommu)
+{
+ struct vfio_pci_device *device;
+
+ device = vfio_pci_device_alloc(bdf, iommu);
+
if (iommu->mode->container_path)
vfio_pci_container_setup(device, bdf, NULL);
else
@@ -462,5 +476,5 @@ void vfio_pci_device_cleanup(struct vfio_pci_device *device)
if (device->group_fd)
VFIO_ASSERT_EQ(close(device->group_fd), 0);
- free(device);
+ vfio_pci_device_free(device);
}
--
2.53.0.473.g4a7958ca14-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI
2026-03-03 19:38 [PATCH v6 0/8] vfio: selftest: Add SR-IOV UAPI test Raghavendra Rao Ananta
` (6 preceding siblings ...)
2026-03-03 19:38 ` [PATCH v6 7/8] vfio: selftests: Add helpers to alloc/free vfio_pci_device Raghavendra Rao Ananta
@ 2026-03-03 19:38 ` Raghavendra Rao Ananta
2026-03-11 23:57 ` Vipin Sharma
7 siblings, 1 reply; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-03 19:38 UTC (permalink / raw)
To: David Matlack, Alex Williamson
Cc: Vipin Sharma, Josh Hilke, kvm, linux-kernel,
Raghavendra Rao Ananta
Add a selftest, vfio_pci_sriov_uapi_test.c, 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>
Reviewed-by: David Matlack <dmatlack@google.com>
---
tools/testing/selftests/vfio/Makefile | 1 +
.../selftests/vfio/vfio_pci_sriov_uapi_test.c | 200 ++++++++++++++++++
2 files changed, 201 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 f27ed18070f14..d0c8cea53eb54 100644
--- a/tools/testing/selftests/vfio/Makefile
+++ b/tools/testing/selftests/vfio/Makefile
@@ -12,6 +12,7 @@ TEST_GEN_PROGS += vfio_iommufd_setup_test
TEST_GEN_PROGS += vfio_pci_device_test
TEST_GEN_PROGS += vfio_pci_device_init_perf_test
TEST_GEN_PROGS += vfio_pci_driver_test
+TEST_GEN_PROGS += vfio_pci_sriov_uapi_test
TEST_FILES += scripts/cleanup.sh
TEST_FILES += scripts/lib.sh
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..9cfbecccb759f
--- /dev/null
+++ b/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c
@@ -0,0 +1,200 @@
+// 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 <libvfio.h>
+
+#include "../kselftest_harness.h"
+
+#define UUID_1 "52ac9bff-3a88-4fbd-901a-0d767c3b6c97"
+#define UUID_2 "88594674-90a0-47a9-aea8-9d9b352ac08a"
+
+static const char *pf_bdf;
+
+static int container_setup(struct vfio_pci_device *device, const char *bdf,
+ const char *vf_token)
+{
+ vfio_pci_group_setup(device, bdf);
+ vfio_container_set_iommu(device);
+ __vfio_pci_group_get_device_fd(device, bdf, vf_token);
+
+ /* The device fd will be -1 in case of mismatched tokens */
+ return (device->fd < 0);
+}
+
+static int iommufd_setup(struct vfio_pci_device *device, const char *bdf,
+ const char *vf_token)
+{
+ vfio_pci_cdev_open(device, bdf);
+ return __vfio_device_bind_iommufd(device->fd,
+ device->iommu->iommufd, vf_token);
+}
+
+static struct vfio_pci_device *device_init(const char *bdf, struct iommu *iommu,
+ const char *vf_token, int *out_ret)
+{
+ struct vfio_pci_device *device = vfio_pci_device_alloc(bdf, iommu);
+
+ if (iommu->mode->container_path)
+ *out_ret = container_setup(device, bdf, vf_token);
+ else
+ *out_ret = iommufd_setup(device, bdf, vf_token);
+
+ return device;
+}
+
+static void device_cleanup(struct vfio_pci_device *device)
+{
+ if (device->fd > 0)
+ VFIO_ASSERT_EQ(close(device->fd), 0);
+
+ if (device->group_fd)
+ VFIO_ASSERT_EQ(close(device->group_fd), 0);
+
+ vfio_pci_device_free(device);
+}
+
+FIXTURE(vfio_pci_sriov_uapi_test) {
+ char *vf_bdf;
+};
+
+FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
+{
+ char *vf_driver;
+ int nr_vfs;
+
+ nr_vfs = sysfs_sriov_totalvfs_get(pf_bdf);
+ if (nr_vfs <= 0)
+ SKIP(return, "SR-IOV may not be supported by the PF: %s\n", pf_bdf);
+
+ nr_vfs = sysfs_sriov_numvfs_get(pf_bdf);
+ if (nr_vfs != 0)
+ SKIP(return, "SR-IOV already configured for the PF: %s\n", pf_bdf);
+
+ /* Create only one VF for testing */
+ sysfs_sriov_numvfs_set(pf_bdf, 1);
+ self->vf_bdf = sysfs_sriov_vf_bdf_get(pf_bdf, 0);
+
+ /*
+ * The VF inherits the driver from the PF.
+ * Ensure this is 'vfio-pci' before proceeding.
+ */
+ vf_driver = sysfs_driver_get(self->vf_bdf);
+ ASSERT_NE(vf_driver, NULL);
+ ASSERT_EQ(strcmp(vf_driver, "vfio-pci"), 0);
+ free(vf_driver);
+
+ printf("Created 1 VF (%s) under the PF: %s\n", self->vf_bdf, pf_bdf);
+}
+
+FIXTURE_TEARDOWN(vfio_pci_sriov_uapi_test)
+{
+ free(self->vf_bdf);
+ sysfs_sriov_numvfs_set(pf_bdf, 0);
+}
+
+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 || 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)
+{
+ struct vfio_pci_device *pf;
+ struct vfio_pci_device *vf;
+ struct iommu *iommu;
+ int ret;
+
+ iommu = iommu_init(variant->iommu_mode);
+ pf = device_init(pf_bdf, iommu, UUID_1, &ret);
+ vf = device_init(self->vf_bdf, iommu, variant->vf_token, &ret);
+
+ ASSERT_VF_CREATION(ret);
+
+ device_cleanup(vf);
+ device_cleanup(pf);
+ iommu_cleanup(iommu);
+}
+
+/*
+ * 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)
+{
+ struct vfio_pci_device *pf;
+ struct vfio_pci_device *vf;
+ struct iommu *iommu;
+ int ret;
+
+ iommu = iommu_init(variant->iommu_mode);
+ pf = device_init(pf_bdf, iommu, UUID_1, &ret);
+ device_cleanup(pf);
+
+ vf = device_init(self->vf_bdf, iommu, variant->vf_token, &ret);
+
+ ASSERT_VF_CREATION(ret);
+
+ device_cleanup(vf);
+ iommu_cleanup(iommu);
+}
+
+/*
+ * After PF device init, override the existing token and validate if the newly
+ * set token is the one that's active.
+ */
+TEST_F(vfio_pci_sriov_uapi_test, override_token)
+{
+ struct vfio_pci_device *pf;
+ struct vfio_pci_device *vf;
+ struct iommu *iommu;
+ int ret;
+
+ iommu = iommu_init(variant->iommu_mode);
+ pf = device_init(pf_bdf, iommu, UUID_2, &ret);
+ vfio_device_set_vf_token(pf->fd, UUID_1);
+
+ vf = device_init(self->vf_bdf, iommu, variant->vf_token, &ret);
+
+ ASSERT_VF_CREATION(ret);
+
+ device_cleanup(vf);
+ device_cleanup(pf);
+ iommu_cleanup(iommu);
+}
+
+int main(int argc, char *argv[])
+{
+ pf_bdf = vfio_selftests_get_bdf(&argc, argv);
+ return test_harness_run(argc, argv);
+}
--
2.53.0.473.g4a7958ca14-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI
2026-03-03 19:38 ` [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI Raghavendra Rao Ananta
@ 2026-03-11 23:57 ` Vipin Sharma
2026-03-31 23:53 ` Raghavendra Rao Ananta
0 siblings, 1 reply; 21+ messages in thread
From: Vipin Sharma @ 2026-03-11 23:57 UTC (permalink / raw)
To: Raghavendra Rao Ananta
Cc: David Matlack, Alex Williamson, Josh Hilke, kvm, linux-kernel
On 2026-03-03 19:38:22, Raghavendra Rao Ananta wrote:
> +static int container_setup(struct vfio_pci_device *device, const char *bdf,
> + const char *vf_token)
> +{
> + vfio_pci_group_setup(device, bdf);
> + vfio_container_set_iommu(device);
> + __vfio_pci_group_get_device_fd(device, bdf, vf_token);
> +
> + /* The device fd will be -1 in case of mismatched tokens */
> + return (device->fd < 0);
> +}
> +
This is one is exactly similar to vfio_pci_container_setup() except you
are calling __vfio_pci_group_get_device_fd() which doesn't do assertion.
Any reason to not create __vfio_pci_container_setup() in the library and
just write assertion in vfio_pci_container_setup()?
> +static int iommufd_setup(struct vfio_pci_device *device, const char *bdf,
> + const char *vf_token)
> +{
> + vfio_pci_cdev_open(device, bdf);
> + return __vfio_device_bind_iommufd(device->fd,
> + device->iommu->iommufd, vf_token);
> +}
I see these are also similar to the code in library with some
differences. May be we can refactor library code and reuse it here.
One option can be to split vfio_pci_iommufd_setup() in two parts where
first part is what your are doing above and second part is attaching PT.
> +
> +static struct vfio_pci_device *device_init(const char *bdf, struct iommu *iommu,
> + const char *vf_token, int *out_ret)
> +{
> + struct vfio_pci_device *device = vfio_pci_device_alloc(bdf, iommu);
> +
> + if (iommu->mode->container_path)
> + *out_ret = container_setup(device, bdf, vf_token);
> + else
> + *out_ret = iommufd_setup(device, bdf, vf_token);
> +
> + return device;
> +}
Same for this one, vfio_pci_device_init() can be composed of two parts,
first one being alloc and container/iommufd setup and second device
setup and driver probe.
My reasoning for this is to avoid having separate code for these common
flows and avoid them diverging down the line.
> +
> +static void device_cleanup(struct vfio_pci_device *device)
> +{
> + if (device->fd > 0)
> + VFIO_ASSERT_EQ(close(device->fd), 0);
> +
> + if (device->group_fd)
> + VFIO_ASSERT_EQ(close(device->group_fd), 0);
> +
> + vfio_pci_device_free(device);
> +}
> +
> +FIXTURE(vfio_pci_sriov_uapi_test) {
> + char *vf_bdf;
Nit: Can we just use an array like char vf_bdf[16]?
Just not sure why we need to do alloc and free for each run.
> +};
> +
> +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> +{
> + char *vf_driver;
> + int nr_vfs;
> +
> + nr_vfs = sysfs_sriov_totalvfs_get(pf_bdf);
> + if (nr_vfs <= 0)
> + SKIP(return, "SR-IOV may not be supported by the PF: %s\n", pf_bdf);
> +
> + nr_vfs = sysfs_sriov_numvfs_get(pf_bdf);
> + if (nr_vfs != 0)
> + SKIP(return, "SR-IOV already configured for the PF: %s\n", pf_bdf);
> +
If there is a test failure (ASSERT) then fixture cleanup will not run
leaving SR-IOV enabled and all subsequent tests will skip.
Since this test is specific to SR-IOV and user is intentionally passing
a device to test, I think we can just reset the VFs to 0 before
proceeding instead of skipping.
> + /* Create only one VF for testing */
> + sysfs_sriov_numvfs_set(pf_bdf, 1);
> + self->vf_bdf = sysfs_sriov_vf_bdf_get(pf_bdf, 0);
> +
> + /*
> + * The VF inherits the driver from the PF.
> + * Ensure this is 'vfio-pci' before proceeding.
> + */
> + vf_driver = sysfs_driver_get(self->vf_bdf);
> + ASSERT_NE(vf_driver, NULL);
> + ASSERT_EQ(strcmp(vf_driver, "vfio-pci"), 0);
> + free(vf_driver);
> +
> + printf("Created 1 VF (%s) under the PF: %s\n", self->vf_bdf, pf_bdf);
> +}
> +
> +FIXTURE_TEARDOWN(vfio_pci_sriov_uapi_test)
> +{
> + free(self->vf_bdf);
> + sysfs_sriov_numvfs_set(pf_bdf, 0);
> +}
> +
> +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 || 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)
> +{
> + struct vfio_pci_device *pf;
> + struct vfio_pci_device *vf;
> + struct iommu *iommu;
> + int ret;
> +
> + iommu = iommu_init(variant->iommu_mode);
Can this and cleanup go in fixture setup?
> + pf = device_init(pf_bdf, iommu, UUID_1, &ret);
We should assert here as well and in other functions.
> + vf = device_init(self->vf_bdf, iommu, variant->vf_token, &ret);
> +
> + ASSERT_VF_CREATION(ret);
> +
> + device_cleanup(vf);
> + device_cleanup(pf);
> + iommu_cleanup(iommu);
> +}
> +
> +/*
> + * 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)
> +{
> + struct vfio_pci_device *pf;
> + struct vfio_pci_device *vf;
> + struct iommu *iommu;
> + int ret;
> +
> + iommu = iommu_init(variant->iommu_mode);
> + pf = device_init(pf_bdf, iommu, UUID_1, &ret);
> + device_cleanup(pf);
> +
> + vf = device_init(self->vf_bdf, iommu, variant->vf_token, &ret);
> +
> + ASSERT_VF_CREATION(ret);
> +
> + device_cleanup(vf);
> + iommu_cleanup(iommu);
> +}
> +
> +/*
> + * After PF device init, override the existing token and validate if the newly
> + * set token is the one that's active.
> + */
> +TEST_F(vfio_pci_sriov_uapi_test, override_token)
> +{
> + struct vfio_pci_device *pf;
> + struct vfio_pci_device *vf;
> + struct iommu *iommu;
> + int ret;
> +
> + iommu = iommu_init(variant->iommu_mode);
> + pf = device_init(pf_bdf, iommu, UUID_2, &ret);
> + vfio_device_set_vf_token(pf->fd, UUID_1);
> +
> + vf = device_init(self->vf_bdf, iommu, variant->vf_token, &ret);
> +
> + ASSERT_VF_CREATION(ret);
> +
> + device_cleanup(vf);
> + device_cleanup(pf);
> + iommu_cleanup(iommu);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + pf_bdf = vfio_selftests_get_bdf(&argc, argv);
> + return test_harness_run(argc, argv);
> +}
> --
> 2.53.0.473.g4a7958ca14-goog
>
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI
2026-03-11 23:57 ` Vipin Sharma
@ 2026-03-31 23:53 ` Raghavendra Rao Ananta
2026-04-01 0:10 ` David Matlack
0 siblings, 1 reply; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-03-31 23:53 UTC (permalink / raw)
To: Vipin Sharma
Cc: David Matlack, Alex Williamson, Josh Hilke, kvm, linux-kernel
On Wed, Mar 11, 2026 at 4:57 PM Vipin Sharma <vipinsh@google.com> wrote:
>
> On 2026-03-03 19:38:22, Raghavendra Rao Ananta wrote:
> > +static int container_setup(struct vfio_pci_device *device, const char *bdf,
> > + const char *vf_token)
> > +{
> > + vfio_pci_group_setup(device, bdf);
> > + vfio_container_set_iommu(device);
> > + __vfio_pci_group_get_device_fd(device, bdf, vf_token);
> > +
> > + /* The device fd will be -1 in case of mismatched tokens */
> > + return (device->fd < 0);
> > +}
> > +
>
> This is one is exactly similar to vfio_pci_container_setup() except you
> are calling __vfio_pci_group_get_device_fd() which doesn't do assertion.
>
> Any reason to not create __vfio_pci_container_setup() in the library and
> just write assertion in vfio_pci_container_setup()?
>
Yes, the test is specifically interested in skipping the 'device->fd'
assertion out of the many assertions that vfio_pci_container_setup()
performs. Hence, there's no point in creating
__vfio_pci_container_setup() that skips only this part, as it could be
confusing. If there are more usecases of similar assertions being
skipped, we can consider implementing the functions as needed in the
lib.
> > +static int iommufd_setup(struct vfio_pci_device *device, const char *bdf,
> > + const char *vf_token)
> > +{
> > + vfio_pci_cdev_open(device, bdf);
> > + return __vfio_device_bind_iommufd(device->fd,
> > + device->iommu->iommufd, vf_token);
> > +}
>
> I see these are also similar to the code in library with some
> differences. May be we can refactor library code and reuse it here.
>
> One option can be to split vfio_pci_iommufd_setup() in two parts where
> first part is what your are doing above and second part is attaching PT.
>
I think my above statement applies here too. Moreover,
vfio_pci_iommufd_setup() seems to integrate the three calls really
well, even in terms of readability. Splitting out only
vfio_device_attach_iommufd_pt() (or an other function) just for the
sake of this test didn't make much sense. Hence, I retained the flow.
But again, if we have usecases in the future, we can split it.
> > +
> > +static struct vfio_pci_device *device_init(const char *bdf, struct iommu *iommu,
> > + const char *vf_token, int *out_ret)
> > +{
> > + struct vfio_pci_device *device = vfio_pci_device_alloc(bdf, iommu);
> > +
> > + if (iommu->mode->container_path)
> > + *out_ret = container_setup(device, bdf, vf_token);
> > + else
> > + *out_ret = iommufd_setup(device, bdf, vf_token);
> > +
> > + return device;
> > +}
>
> Same for this one, vfio_pci_device_init() can be composed of two parts,
> first one being alloc and container/iommufd setup and second device
> setup and driver probe.
>
> My reasoning for this is to avoid having separate code for these common
> flows and avoid them diverging down the line.
>
Similar comment to the one above. I avoided refactoring the library
code just to make one-off assertions relevant only to this test.
> > +
> > +static void device_cleanup(struct vfio_pci_device *device)
> > +{
> > + if (device->fd > 0)
> > + VFIO_ASSERT_EQ(close(device->fd), 0);
> > +
> > + if (device->group_fd)
> > + VFIO_ASSERT_EQ(close(device->group_fd), 0);
> > +
> > + vfio_pci_device_free(device);
> > +}
> > +
> > +FIXTURE(vfio_pci_sriov_uapi_test) {
> > + char *vf_bdf;
>
> Nit: Can we just use an array like char vf_bdf[16]?
> Just not sure why we need to do alloc and free for each run.
>
We can. In fact, I initially had that (for getting the driver path
though in v2), but this felt a bit cleaner (suggested by David) and
similar to how we handle most kernel allocations. This way the caller
doesn't have to guess a size, while the library knows an appropriate
size.
> > +};
> > +
> > +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> > +{
> > + char *vf_driver;
> > + int nr_vfs;
> > +
> > + nr_vfs = sysfs_sriov_totalvfs_get(pf_bdf);
> > + if (nr_vfs <= 0)
> > + SKIP(return, "SR-IOV may not be supported by the PF: %s\n", pf_bdf);
> > +
> > + nr_vfs = sysfs_sriov_numvfs_get(pf_bdf);
> > + if (nr_vfs != 0)
> > + SKIP(return, "SR-IOV already configured for the PF: %s\n", pf_bdf);
> > +
>
> If there is a test failure (ASSERT) then fixture cleanup will not run
> leaving SR-IOV enabled and all subsequent tests will skip.
>
> Since this test is specific to SR-IOV and user is intentionally passing
> a device to test, I think we can just reset the VFs to 0 before
> proceeding instead of skipping.
>
The idea was to eliminate the assumption that if 'sriov_numvfs'
returns > 0, someone else might be using it. This is based on David's
suggestion: https://lore.kernel.org/all/aQzcQ0fJd-aCRThS@google.com/ .
Moreover, the device might be in a partial state. So, even if the next
test proceeds, it could fail due to other reasons.
But I see your point about cleanup on failure. I think this is a
common issue across all tests. Was the expectation that we rely on
cleanup.sh to take care of all the cleanups, regardless of test
pass/failure?
> > + /* Create only one VF for testing */
> > + sysfs_sriov_numvfs_set(pf_bdf, 1);
> > + self->vf_bdf = sysfs_sriov_vf_bdf_get(pf_bdf, 0);
> > +
> > + /*
> > + * The VF inherits the driver from the PF.
> > + * Ensure this is 'vfio-pci' before proceeding.
> > + */
> > + vf_driver = sysfs_driver_get(self->vf_bdf);
> > + ASSERT_NE(vf_driver, NULL);
> > + ASSERT_EQ(strcmp(vf_driver, "vfio-pci"), 0);
> > + free(vf_driver);
> > +
> > + printf("Created 1 VF (%s) under the PF: %s\n", self->vf_bdf, pf_bdf);
> > +}
> > +
> > +FIXTURE_TEARDOWN(vfio_pci_sriov_uapi_test)
> > +{
> > + free(self->vf_bdf);
> > + sysfs_sriov_numvfs_set(pf_bdf, 0);
> > +}
> > +
> > +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 || 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)
> > +{
> > + struct vfio_pci_device *pf;
> > + struct vfio_pci_device *vf;
> > + struct iommu *iommu;
> > + int ret;
> > +
> > + iommu = iommu_init(variant->iommu_mode);
>
> Can this and cleanup go in fixture setup?
>
Isn't 'variant' inaccessible from FIXUTURE_SETUP()?
> > + pf = device_init(pf_bdf, iommu, UUID_1, &ret);
>
> We should assert here as well and in other functions.
>
Good point. I'll add it in v7.
Thank you.
Raghavendra
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI
2026-03-31 23:53 ` Raghavendra Rao Ananta
@ 2026-04-01 0:10 ` David Matlack
2026-04-01 23:46 ` Raghavendra Rao Ananta
0 siblings, 1 reply; 21+ messages in thread
From: David Matlack @ 2026-04-01 0:10 UTC (permalink / raw)
To: Raghavendra Rao Ananta
Cc: Vipin Sharma, Alex Williamson, Josh Hilke, kvm, linux-kernel
On Tue, Mar 31, 2026 at 4:53 PM Raghavendra Rao Ananta
<rananta@google.com> wrote:
> > > +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> > > +{
> > > + char *vf_driver;
> > > + int nr_vfs;
> > > +
> > > + nr_vfs = sysfs_sriov_totalvfs_get(pf_bdf);
> > > + if (nr_vfs <= 0)
> > > + SKIP(return, "SR-IOV may not be supported by the PF: %s\n", pf_bdf);
> > > +
> > > + nr_vfs = sysfs_sriov_numvfs_get(pf_bdf);
> > > + if (nr_vfs != 0)
> > > + SKIP(return, "SR-IOV already configured for the PF: %s\n", pf_bdf);
> > > +
> >
> > If there is a test failure (ASSERT) then fixture cleanup will not run
> > leaving SR-IOV enabled and all subsequent tests will skip.
> >
> > Since this test is specific to SR-IOV and user is intentionally passing
> > a device to test, I think we can just reset the VFs to 0 before
> > proceeding instead of skipping.
> >
> The idea was to eliminate the assumption that if 'sriov_numvfs'
> returns > 0, someone else might be using it. This is based on David's
> suggestion: https://lore.kernel.org/all/aQzcQ0fJd-aCRThS@google.com/ .
> Moreover, the device might be in a partial state. So, even if the next
> test proceeds, it could fail due to other reasons.
>
> But I see your point about cleanup on failure. I think this is a
> common issue across all tests. Was the expectation that we rely on
> cleanup.sh to take care of all the cleanups, regardless of test
> pass/failure?
I think we rely on the fact that test cases run in sub-processes and
so if there is a failure in fixture setup then the child process exits
and all vfio and iommufd fds are cleaned up. So the device is still
"clean" for the next test.
However if we are modifying system state like the number of VFs
enabled on a device, I think Vipin is right we need to clean up after
ourselves if the error occurs in fixture setup.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI
2026-04-01 0:10 ` David Matlack
@ 2026-04-01 23:46 ` Raghavendra Rao Ananta
2026-04-01 23:50 ` David Matlack
0 siblings, 1 reply; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-04-01 23:46 UTC (permalink / raw)
To: David Matlack
Cc: Vipin Sharma, Alex Williamson, Josh Hilke, kvm, linux-kernel
On Tue, Mar 31, 2026 at 5:11 PM David Matlack <dmatlack@google.com> wrote:
>
> On Tue, Mar 31, 2026 at 4:53 PM Raghavendra Rao Ananta
> <rananta@google.com> wrote:
>
> > > > +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> > > > +{
> > > > + char *vf_driver;
> > > > + int nr_vfs;
> > > > +
> > > > + nr_vfs = sysfs_sriov_totalvfs_get(pf_bdf);
> > > > + if (nr_vfs <= 0)
> > > > + SKIP(return, "SR-IOV may not be supported by the PF: %s\n", pf_bdf);
> > > > +
> > > > + nr_vfs = sysfs_sriov_numvfs_get(pf_bdf);
> > > > + if (nr_vfs != 0)
> > > > + SKIP(return, "SR-IOV already configured for the PF: %s\n", pf_bdf);
> > > > +
> > >
> > > If there is a test failure (ASSERT) then fixture cleanup will not run
> > > leaving SR-IOV enabled and all subsequent tests will skip.
> > >
> > > Since this test is specific to SR-IOV and user is intentionally passing
> > > a device to test, I think we can just reset the VFs to 0 before
> > > proceeding instead of skipping.
> > >
> > The idea was to eliminate the assumption that if 'sriov_numvfs'
> > returns > 0, someone else might be using it. This is based on David's
> > suggestion: https://lore.kernel.org/all/aQzcQ0fJd-aCRThS@google.com/ .
> > Moreover, the device might be in a partial state. So, even if the next
> > test proceeds, it could fail due to other reasons.
> >
> > But I see your point about cleanup on failure. I think this is a
> > common issue across all tests. Was the expectation that we rely on
> > cleanup.sh to take care of all the cleanups, regardless of test
> > pass/failure?
>
> I think we rely on the fact that test cases run in sub-processes and
> so if there is a failure in fixture setup then the child process exits
> and all vfio and iommufd fds are cleaned up. So the device is still
> "clean" for the next test.
>
> However if we are modifying system state like the number of VFs
> enabled on a device, I think Vipin is right we need to clean up after
> ourselves if the error occurs in fixture setup.
In that case, since the FIXTURE_TEARDOWN() is not invoked for a
failure in FIXTURE_SETUP(), the test must install its own cleaning
handler. Using conventional error handling with goto labels might also
be tricky because the sysfs lib contains ASSERTs. Even if we do reset
sriov_nrvfs and exit, since the failure is in FIXTURE_SETUP(), it's
highly likely that it's going to fail again.
Since we need the VF creation only once, shall we simply do this setup
in main()?
int main()
{
pf_bdf = vfio_selftests_get_bdf(&argc, argv);
vf_bdf = setup_vf(pf_bdf); // Do all the sysfs stuff here
ret = test_harness_run(argc, argv);
destroy_vf();
return ret;
}
Another possible way is to expose the VF from setup.sh itself and pass
it as an arg to the test.
WDYT?
Thank you.
Raghavendra
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI
2026-04-01 23:46 ` Raghavendra Rao Ananta
@ 2026-04-01 23:50 ` David Matlack
2026-04-02 0:17 ` Raghavendra Rao Ananta
0 siblings, 1 reply; 21+ messages in thread
From: David Matlack @ 2026-04-01 23:50 UTC (permalink / raw)
To: Raghavendra Rao Ananta
Cc: Vipin Sharma, Alex Williamson, Josh Hilke, kvm, linux-kernel
On Wed, Apr 1, 2026 at 4:46 PM Raghavendra Rao Ananta
<rananta@google.com> wrote:
>
> On Tue, Mar 31, 2026 at 5:11 PM David Matlack <dmatlack@google.com> wrote:
> >
> > On Tue, Mar 31, 2026 at 4:53 PM Raghavendra Rao Ananta
> > <rananta@google.com> wrote:
> >
> > > > > +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> > > > > +{
> > > > > + char *vf_driver;
> > > > > + int nr_vfs;
> > > > > +
> > > > > + nr_vfs = sysfs_sriov_totalvfs_get(pf_bdf);
> > > > > + if (nr_vfs <= 0)
> > > > > + SKIP(return, "SR-IOV may not be supported by the PF: %s\n", pf_bdf);
> > > > > +
> > > > > + nr_vfs = sysfs_sriov_numvfs_get(pf_bdf);
> > > > > + if (nr_vfs != 0)
> > > > > + SKIP(return, "SR-IOV already configured for the PF: %s\n", pf_bdf);
> > > > > +
> > > >
> > > > If there is a test failure (ASSERT) then fixture cleanup will not run
> > > > leaving SR-IOV enabled and all subsequent tests will skip.
> > > >
> > > > Since this test is specific to SR-IOV and user is intentionally passing
> > > > a device to test, I think we can just reset the VFs to 0 before
> > > > proceeding instead of skipping.
> > > >
> > > The idea was to eliminate the assumption that if 'sriov_numvfs'
> > > returns > 0, someone else might be using it. This is based on David's
> > > suggestion: https://lore.kernel.org/all/aQzcQ0fJd-aCRThS@google.com/ .
> > > Moreover, the device might be in a partial state. So, even if the next
> > > test proceeds, it could fail due to other reasons.
> > >
> > > But I see your point about cleanup on failure. I think this is a
> > > common issue across all tests. Was the expectation that we rely on
> > > cleanup.sh to take care of all the cleanups, regardless of test
> > > pass/failure?
> >
> > I think we rely on the fact that test cases run in sub-processes and
> > so if there is a failure in fixture setup then the child process exits
> > and all vfio and iommufd fds are cleaned up. So the device is still
> > "clean" for the next test.
> >
> > However if we are modifying system state like the number of VFs
> > enabled on a device, I think Vipin is right we need to clean up after
> > ourselves if the error occurs in fixture setup.
>
> In that case, since the FIXTURE_TEARDOWN() is not invoked for a
> failure in FIXTURE_SETUP(), the test must install its own cleaning
> handler. Using conventional error handling with goto labels might also
> be tricky because the sysfs lib contains ASSERTs. Even if we do reset
> sriov_nrvfs and exit, since the failure is in FIXTURE_SETUP(), it's
> highly likely that it's going to fail again.
>
> Since we need the VF creation only once, shall we simply do this setup
> in main()?
>
> int main()
> {
> pf_bdf = vfio_selftests_get_bdf(&argc, argv);
> vf_bdf = setup_vf(pf_bdf); // Do all the sysfs stuff here
> ret = test_harness_run(argc, argv);
> destroy_vf();
>
> return ret;
> }
>
> Another possible way is to expose the VF from setup.sh itself and pass
> it as an arg to the test.
>
> WDYT?
Creating and destroying VFs in main() outside of the test harness
sounds like a good solution to me.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI
2026-04-01 23:50 ` David Matlack
@ 2026-04-02 0:17 ` Raghavendra Rao Ananta
2026-04-02 16:02 ` David Matlack
0 siblings, 1 reply; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-04-02 0:17 UTC (permalink / raw)
To: David Matlack
Cc: Vipin Sharma, Alex Williamson, Josh Hilke, kvm, linux-kernel
On Wed, Apr 1, 2026 at 4:51 PM David Matlack <dmatlack@google.com> wrote:
>
> On Wed, Apr 1, 2026 at 4:46 PM Raghavendra Rao Ananta
> <rananta@google.com> wrote:
> >
> > On Tue, Mar 31, 2026 at 5:11 PM David Matlack <dmatlack@google.com> wrote:
> > >
> > > On Tue, Mar 31, 2026 at 4:53 PM Raghavendra Rao Ananta
> > > <rananta@google.com> wrote:
> > >
> > > > > > +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> > > > > > +{
> > > > > > + char *vf_driver;
> > > > > > + int nr_vfs;
> > > > > > +
> > > > > > + nr_vfs = sysfs_sriov_totalvfs_get(pf_bdf);
> > > > > > + if (nr_vfs <= 0)
> > > > > > + SKIP(return, "SR-IOV may not be supported by the PF: %s\n", pf_bdf);
> > > > > > +
> > > > > > + nr_vfs = sysfs_sriov_numvfs_get(pf_bdf);
> > > > > > + if (nr_vfs != 0)
> > > > > > + SKIP(return, "SR-IOV already configured for the PF: %s\n", pf_bdf);
> > > > > > +
> > > > >
> > > > > If there is a test failure (ASSERT) then fixture cleanup will not run
> > > > > leaving SR-IOV enabled and all subsequent tests will skip.
> > > > >
> > > > > Since this test is specific to SR-IOV and user is intentionally passing
> > > > > a device to test, I think we can just reset the VFs to 0 before
> > > > > proceeding instead of skipping.
> > > > >
> > > > The idea was to eliminate the assumption that if 'sriov_numvfs'
> > > > returns > 0, someone else might be using it. This is based on David's
> > > > suggestion: https://lore.kernel.org/all/aQzcQ0fJd-aCRThS@google.com/ .
> > > > Moreover, the device might be in a partial state. So, even if the next
> > > > test proceeds, it could fail due to other reasons.
> > > >
> > > > But I see your point about cleanup on failure. I think this is a
> > > > common issue across all tests. Was the expectation that we rely on
> > > > cleanup.sh to take care of all the cleanups, regardless of test
> > > > pass/failure?
> > >
> > > I think we rely on the fact that test cases run in sub-processes and
> > > so if there is a failure in fixture setup then the child process exits
> > > and all vfio and iommufd fds are cleaned up. So the device is still
> > > "clean" for the next test.
> > >
> > > However if we are modifying system state like the number of VFs
> > > enabled on a device, I think Vipin is right we need to clean up after
> > > ourselves if the error occurs in fixture setup.
> >
> > In that case, since the FIXTURE_TEARDOWN() is not invoked for a
> > failure in FIXTURE_SETUP(), the test must install its own cleaning
> > handler. Using conventional error handling with goto labels might also
> > be tricky because the sysfs lib contains ASSERTs. Even if we do reset
> > sriov_nrvfs and exit, since the failure is in FIXTURE_SETUP(), it's
> > highly likely that it's going to fail again.
> >
> > Since we need the VF creation only once, shall we simply do this setup
> > in main()?
> >
> > int main()
> > {
> > pf_bdf = vfio_selftests_get_bdf(&argc, argv);
> > vf_bdf = setup_vf(pf_bdf); // Do all the sysfs stuff here
> > ret = test_harness_run(argc, argv);
> > destroy_vf();
> >
> > return ret;
> > }
> >
> > Another possible way is to expose the VF from setup.sh itself and pass
> > it as an arg to the test.
> >
> > WDYT?
>
> Creating and destroying VFs in main() outside of the test harness
> sounds like a good solution to me.
Gah.. thinking this through, since we use ASSERTs in sysfs setup, we'd
still have problems with 'goto label' like error handling. If it's not
getting too complicated, I can set up an exit handler (say via
exitat()) and destroy VFs there.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI
2026-04-02 0:17 ` Raghavendra Rao Ananta
@ 2026-04-02 16:02 ` David Matlack
2026-04-02 17:33 ` Raghavendra Rao Ananta
0 siblings, 1 reply; 21+ messages in thread
From: David Matlack @ 2026-04-02 16:02 UTC (permalink / raw)
To: Raghavendra Rao Ananta
Cc: Vipin Sharma, Alex Williamson, Josh Hilke, kvm, linux-kernel
On Wed, Apr 1, 2026 at 5:17 PM Raghavendra Rao Ananta
<rananta@google.com> wrote:
>
> On Wed, Apr 1, 2026 at 4:51 PM David Matlack <dmatlack@google.com> wrote:
> >
> > On Wed, Apr 1, 2026 at 4:46 PM Raghavendra Rao Ananta
> > <rananta@google.com> wrote:
> > >
> > > On Tue, Mar 31, 2026 at 5:11 PM David Matlack <dmatlack@google.com> wrote:
> > > >
> > > > On Tue, Mar 31, 2026 at 4:53 PM Raghavendra Rao Ananta
> > > > <rananta@google.com> wrote:
> > > >
> > > > > > > +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> > > > > > > +{
> > > > > > > + char *vf_driver;
> > > > > > > + int nr_vfs;
> > > > > > > +
> > > > > > > + nr_vfs = sysfs_sriov_totalvfs_get(pf_bdf);
> > > > > > > + if (nr_vfs <= 0)
> > > > > > > + SKIP(return, "SR-IOV may not be supported by the PF: %s\n", pf_bdf);
> > > > > > > +
> > > > > > > + nr_vfs = sysfs_sriov_numvfs_get(pf_bdf);
> > > > > > > + if (nr_vfs != 0)
> > > > > > > + SKIP(return, "SR-IOV already configured for the PF: %s\n", pf_bdf);
> > > > > > > +
> > > > > >
> > > > > > If there is a test failure (ASSERT) then fixture cleanup will not run
> > > > > > leaving SR-IOV enabled and all subsequent tests will skip.
> > > > > >
> > > > > > Since this test is specific to SR-IOV and user is intentionally passing
> > > > > > a device to test, I think we can just reset the VFs to 0 before
> > > > > > proceeding instead of skipping.
> > > > > >
> > > > > The idea was to eliminate the assumption that if 'sriov_numvfs'
> > > > > returns > 0, someone else might be using it. This is based on David's
> > > > > suggestion: https://lore.kernel.org/all/aQzcQ0fJd-aCRThS@google.com/ .
> > > > > Moreover, the device might be in a partial state. So, even if the next
> > > > > test proceeds, it could fail due to other reasons.
> > > > >
> > > > > But I see your point about cleanup on failure. I think this is a
> > > > > common issue across all tests. Was the expectation that we rely on
> > > > > cleanup.sh to take care of all the cleanups, regardless of test
> > > > > pass/failure?
> > > >
> > > > I think we rely on the fact that test cases run in sub-processes and
> > > > so if there is a failure in fixture setup then the child process exits
> > > > and all vfio and iommufd fds are cleaned up. So the device is still
> > > > "clean" for the next test.
> > > >
> > > > However if we are modifying system state like the number of VFs
> > > > enabled on a device, I think Vipin is right we need to clean up after
> > > > ourselves if the error occurs in fixture setup.
> > >
> > > In that case, since the FIXTURE_TEARDOWN() is not invoked for a
> > > failure in FIXTURE_SETUP(), the test must install its own cleaning
> > > handler. Using conventional error handling with goto labels might also
> > > be tricky because the sysfs lib contains ASSERTs. Even if we do reset
> > > sriov_nrvfs and exit, since the failure is in FIXTURE_SETUP(), it's
> > > highly likely that it's going to fail again.
> > >
> > > Since we need the VF creation only once, shall we simply do this setup
> > > in main()?
> > >
> > > int main()
> > > {
> > > pf_bdf = vfio_selftests_get_bdf(&argc, argv);
> > > vf_bdf = setup_vf(pf_bdf); // Do all the sysfs stuff here
> > > ret = test_harness_run(argc, argv);
> > > destroy_vf();
> > >
> > > return ret;
> > > }
> > >
> > > Another possible way is to expose the VF from setup.sh itself and pass
> > > it as an arg to the test.
> > >
> > > WDYT?
> >
> > Creating and destroying VFs in main() outside of the test harness
> > sounds like a good solution to me.
>
> Gah.. thinking this through, since we use ASSERTs in sysfs setup, we'd
> still have problems with 'goto label' like error handling. If it's not
> getting too complicated, I can set up an exit handler (say via
> exitat()) and destroy VFs there.
That sounds reasonable. Doing the setup in a child process would be
another viable approach.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v6 8/8] vfio: selftests: Add tests to validate SR-IOV UAPI
2026-04-02 16:02 ` David Matlack
@ 2026-04-02 17:33 ` Raghavendra Rao Ananta
0 siblings, 0 replies; 21+ messages in thread
From: Raghavendra Rao Ananta @ 2026-04-02 17:33 UTC (permalink / raw)
To: David Matlack
Cc: Vipin Sharma, Alex Williamson, Josh Hilke, kvm, linux-kernel
On Thu, Apr 2, 2026 at 9:02 AM David Matlack <dmatlack@google.com> wrote:
>
> On Wed, Apr 1, 2026 at 5:17 PM Raghavendra Rao Ananta
> <rananta@google.com> wrote:
> >
> > On Wed, Apr 1, 2026 at 4:51 PM David Matlack <dmatlack@google.com> wrote:
> > >
> > > On Wed, Apr 1, 2026 at 4:46 PM Raghavendra Rao Ananta
> > > <rananta@google.com> wrote:
> > > >
> > > > On Tue, Mar 31, 2026 at 5:11 PM David Matlack <dmatlack@google.com> wrote:
> > > > >
> > > > > On Tue, Mar 31, 2026 at 4:53 PM Raghavendra Rao Ananta
> > > > > <rananta@google.com> wrote:
> > > > >
> > > > > > > > +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> > > > > > > > +{
> > > > > > > > + char *vf_driver;
> > > > > > > > + int nr_vfs;
> > > > > > > > +
> > > > > > > > + nr_vfs = sysfs_sriov_totalvfs_get(pf_bdf);
> > > > > > > > + if (nr_vfs <= 0)
> > > > > > > > + SKIP(return, "SR-IOV may not be supported by the PF: %s\n", pf_bdf);
> > > > > > > > +
> > > > > > > > + nr_vfs = sysfs_sriov_numvfs_get(pf_bdf);
> > > > > > > > + if (nr_vfs != 0)
> > > > > > > > + SKIP(return, "SR-IOV already configured for the PF: %s\n", pf_bdf);
> > > > > > > > +
> > > > > > >
> > > > > > > If there is a test failure (ASSERT) then fixture cleanup will not run
> > > > > > > leaving SR-IOV enabled and all subsequent tests will skip.
> > > > > > >
> > > > > > > Since this test is specific to SR-IOV and user is intentionally passing
> > > > > > > a device to test, I think we can just reset the VFs to 0 before
> > > > > > > proceeding instead of skipping.
> > > > > > >
> > > > > > The idea was to eliminate the assumption that if 'sriov_numvfs'
> > > > > > returns > 0, someone else might be using it. This is based on David's
> > > > > > suggestion: https://lore.kernel.org/all/aQzcQ0fJd-aCRThS@google.com/ .
> > > > > > Moreover, the device might be in a partial state. So, even if the next
> > > > > > test proceeds, it could fail due to other reasons.
> > > > > >
> > > > > > But I see your point about cleanup on failure. I think this is a
> > > > > > common issue across all tests. Was the expectation that we rely on
> > > > > > cleanup.sh to take care of all the cleanups, regardless of test
> > > > > > pass/failure?
> > > > >
> > > > > I think we rely on the fact that test cases run in sub-processes and
> > > > > so if there is a failure in fixture setup then the child process exits
> > > > > and all vfio and iommufd fds are cleaned up. So the device is still
> > > > > "clean" for the next test.
> > > > >
> > > > > However if we are modifying system state like the number of VFs
> > > > > enabled on a device, I think Vipin is right we need to clean up after
> > > > > ourselves if the error occurs in fixture setup.
> > > >
> > > > In that case, since the FIXTURE_TEARDOWN() is not invoked for a
> > > > failure in FIXTURE_SETUP(), the test must install its own cleaning
> > > > handler. Using conventional error handling with goto labels might also
> > > > be tricky because the sysfs lib contains ASSERTs. Even if we do reset
> > > > sriov_nrvfs and exit, since the failure is in FIXTURE_SETUP(), it's
> > > > highly likely that it's going to fail again.
> > > >
> > > > Since we need the VF creation only once, shall we simply do this setup
> > > > in main()?
> > > >
> > > > int main()
> > > > {
> > > > pf_bdf = vfio_selftests_get_bdf(&argc, argv);
> > > > vf_bdf = setup_vf(pf_bdf); // Do all the sysfs stuff here
> > > > ret = test_harness_run(argc, argv);
> > > > destroy_vf();
> > > >
> > > > return ret;
> > > > }
> > > >
> > > > Another possible way is to expose the VF from setup.sh itself and pass
> > > > it as an arg to the test.
> > > >
> > > > WDYT?
> > >
> > > Creating and destroying VFs in main() outside of the test harness
> > > sounds like a good solution to me.
> >
> > Gah.. thinking this through, since we use ASSERTs in sysfs setup, we'd
> > still have problems with 'goto label' like error handling. If it's not
> > getting too complicated, I can set up an exit handler (say via
> > exitat()) and destroy VFs there.
>
> That sounds reasonable. Doing the setup in a child process would be
> another viable approach.
Yeah, the latter approach is also a good idea. But I went with the
former in v7 so we would set up and destroy the VF only once, instead
of 45 times (once per sub-test), which also increases testing time
slightly.
Thank you.
Raghavendra
^ permalink raw reply [flat|nested] 21+ messages in thread