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

[Vipin Sharma <vipinsh@google.com>]

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
>