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

[Raghavendra Rao Ananta]

On Mon, Apr 13, 2026 at 11:08 AM Vipin Sharma <vipinsh@xxxxxxxxxx> wrote:
>
> 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.
>
The idea was to keep it in line with vfio_pci_device_init(), so I
returned 'struct vfio_pci_device *'. But, it's not a big deal. I've
changed it to return an 'int' if that's what we prefer.

> > +
> > +/*
> > + * 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
>
We rotate with UUID_1, UUID_2, and NULL. I've adjusted the comment to
incude all three.

> > + * 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?
>
Yes it was intentionally kept this way for the 'override_token' test.
Also see the previous 'pf_early_close' test that performs a premature
cleanup of the PF. To accommodate these (and any future TEST_F()s we
may want to add) based on your suggestion, we'd have to create
special/conditional statements across the tests and I'd like to avoid
that if possible. The current setup clearly shows what each test
does/requires.

> > +
> > +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.
>
This actually goes back to a previous discussion with David where we
agreed to avoid such situations. For instance, what if the device is
already in use elsewhere.

Other than these, I've addressed other comments.

Thank you.
Raghavendra