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

[Raghavendra Rao Ananta]

On Wed, Mar 11, 2026 at 4:57 PM Vipin Sharma <vipinsh@xxxxxxxxxx> 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@xxxxxxxxxx/ .
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