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

[Vipin Sharma]

On Mon, May 04, 2026 at 10:51:41AM -0700, Raghavendra Rao Ananta wrote:
> 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:
> > > +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.
> 

I think if you make device_cleanup() handle already cleaned up device as
no-op this should avoid any special handling.

iommu_init() and device_init() with UUID_1 will be used by all three
tests in FIXTURE_SETUP(). Their corresponding cleanup will be in
FIXTURE_TEARDOWN().

This way only pf_early_close() will have extra call of
device_cleanup(pf).

Can you tell more about special/conditional statements which each test
will have to write? Currently, we don't have any tests like that, we can
revisit it when we see that kind of issue.


> > > +
> > > +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.
> 

I think this is what you are referring to:
https://lore.kernel.org/all/aQzcQ0fJd-aCRThS@xxxxxxxxxx
--------
	> > > +     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?
	> >
	> My idea was to simply "steal" the device that was already created and
	> use it. Do we want to skip it, as you suggested?

	If a VF already exists it might be bound to a different driver, and may
	be in use by something else. I think the only safe thing to do is to
	bail if a VF already exists. If the test creates the VF, then it knows
	that it owns it.
--------

If we are running a test harness, and one test failed without clearing
up VFs then all the following tests will pay penalty this way. As this
is a selftest code, device assigned to it is for testing not for some
production work at the same time, I am not able to see why it will be a
unsafe.

My recommendation will be to reset and not skip, I don't think there are
any practical risks here. I will leave it to you to make a final
decision on this.