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

[Vipin Sharma <vipinsh@google.com>]

On Thu, Apr 02, 2026 at 05:30:59PM +0000, Raghavendra Rao Ananta wrote:
> diff --git a/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c b/tools/testing/selftests/vfio/vfio_pci_sriov_uapi_test.c
> +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;

I will recommend to return the error code and pass struct
vfio_pci_device **out_dev in the arguments. This seems more natural
compared to having a last argument as an ret value which is checked in
the caller.

> +
> +/*
> + * PF's token is always set with UUID_1 and VF's token is rotated with
> + * various tokens (including UUID_1 and NULL).

Nit: s/UUID_1/UUID_2

> + * 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);
> +	ASSERT_EQ(ret, 0);
> +
> +	vf = device_init(vf_bdf, iommu, variant->vf_token, &ret);
> +	ASSERT_VF_CREATION(ret);

ASSERT_VF_CREATION() name is confusing, as it is asserting both success
and failure ret value based on the variant passed.

I will recommend to rename it to ASSERT_COND_VF_CREATION(), or, may be
create a wrapper function to check if current test is a UUID_1 variant
or not, and then directly the assert needed.

> +/*
> + * After setting a token on the PF, validate if the VF can still set the
> + * expected token.
> + */

This comment seems incorrect. VF doesn't set the token, it just provides
the token which is set on a PF.

May be a comment can be "After closing the PF, validate VF access still
needs the right token.

> +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);

I am assuming because of this, you cannot move device_init and
device_cleanup calls to FIXTURE_SETUP and FIXTURE_TEARDOWN respectively.

Can we just start this test with device_cleanup(), then do init with
UUID_2? This will allow to reduce the code in all of the tests by moving
things to corresponding setup and teardown functions. WDYT?

> +
> +static void vf_setup(void)
> +{
> +	char *vf_driver;
> +	int nr_vfs;
> +
> +	nr_vfs = sysfs_sriov_totalvfs_get(pf_bdf);
> +	if (nr_vfs <= 0)
> +		ksft_exit_skip("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)
> +		ksft_exit_skip("SR-IOV already configured for the PF: %s\n", pf_bdf);

Why would we want to skip if VFs are already enabled. Just
set it to 0 if it is already there and set it to 1 unconditionally after
that.