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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <aQvzNZU9x9gmFzH3@google.com>
Date: Thu, 6 Nov 2025 01:00:37 +0000
From: David Matlack <dmatlack@...gle.com>
To: Raghavendra Rao Ananta <rananta@...gle.com>
Cc: Alex Williamson <alex@...zbot.org>,
	Alex Williamson <alex.williamson@...hat.com>,
	Josh Hilke <jrhilke@...gle.com>, kvm@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] vfio: selftests: Add tests to validate SR-IOV UAPI

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.