Message-ID: <CAJHc60y-0ea=7_WExNzcVNYWkAP43507puJfOEir1r4ezv3CUQ@mail.gmail.com>
Date: Fri, 7 Nov 2025 08:26:15 +0530
From: Raghavendra Rao Ananta <rananta@...gle.com>
To: David Matlack <dmatlack@...gle.com>
Cc: Alex Williamson <alex@...zbot.org>, Alex Williamson <alex.williamson@...hat.com>, 
	Josh Hilke <jrhilke@...gle.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] vfio: selftests: Add tests to validate SR-IOV UAPI

On Thu, Nov 6, 2025 at 11:05 PM David Matlack <dmatlack@...gle.com> wrote:
>
> On 2025-11-06 10:35 PM, Raghavendra Rao Ananta wrote:
> > On Thu, Nov 6, 2025 at 6:30 AM David Matlack <dmatlack@...gle.com> wrote:
> > >
> > > On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> > >
> > > > +static const char *pf_dev_bdf;
> > > > +static char vf_dev_bdf[16];
> > >
> > > vf_dev_bdf can be part of the test fixture instead of a global variable.
> > > pf_dev_bdf should be the only global variable since we have to get it
> > > from main() into the text fixture.
> > >
> > My understading is placing vars in FIXTURE() is helpful to get an
> > access across various other FIXTURE_*() and TEST*() functions. Out of
> > curiosity, is there an advantage here vs having them global?
>
> Global variables are just generally a bad design pattern. IMO, only
> variables that truly need to be global should be global.
>
> The only variable that needs to be global is pf_dev_bdf.
>
> Since vf_dev_bdf needs to be accessed within FIXTURE_SETUP(),
> FIXTURE_TEARDOWN(), and TEST_F(), then FIXTURE() is the right home for
> it. The whole point of FIXTURE() is to hold state for each TEST_F().
>
Sounds good. I'll move them into FIXTURE().

> >
> > > > +
> > > > +struct vfio_pci_device *pf_device;
> > > > +struct vfio_pci_device *vf_device;
> > >
> > > These can be local variables in the places they are used.
> > >
> > I was a bit greedy to save a few lines, as they are reassigned in
> > every TEST_F() anyway. Is there any advantage by making them local?
>
> It's easy to mess up global variables. And also when reading the code it
> is confusing to see a global variable that does not need to be global.
> It makes me think I must be missing something.
>
> As a general practice I think it's good to limit the scope of variables
> to the minimum scope they are needed.
>
Agreed. I prefer min scope too, but I guess my habit of using global
variables in other tests and avoiding passing pointers around led me
to use it here. I'll move it to a local scope.

> > > > +     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.
>
Makes sense. Let's skip in that case.

> > > > +FIXTURE_TEARDOWN(vfio_pci_sriov_uapi_test)
> > > > +{
> > > > +}
> > >
> > > FIXTURE_TEARDOWN() should undo what FIXTURE_SETUP() did, i.e. write 0 to
> > > sriov_numvfs. Otherwise running this test will leave behind SR-IOV
> > > enabled on the PF.
> > >
> > I had this originally, but then realized that run.sh aready resets the
> > sriov_numvfs to its original value. We can do it here too, if you'd
> > like to keep the symmetry and make the test self-sufficient. With some
> > of your other suggestions, I may have to do some more cleanup here
> > now.
>
> I think the test should return the PF back to the state it was in at the
> start of the test. That way the test doesn't "leak" changes it made. The
> best way to do that is to clean up in FIXTURE_TEARDOWN(). There might be
> some other test that wants to run using the PF before run.sh does its
> cleanup work.
>
Sure, I'll clean up everything that the test does in FIXTURE_SETUP().

> > > You could also make this the users problem (the user has to provide a PF
> > > with 1 VF where both PF and VF are bound to vfio-pci). But I think it
> > > would be nice to make the test work automatically given a PF if we can.
> > Let's go with the latter, assuming it doesn't get too complicated
> > (currently, the setup part seems bigger than the actual test :) )
>
> Let's create helpers for all the sysfs operations under lib.
>
> e.g. tools/testing/selftests/vfio/lib/sysfs.c:
>
>   int sysfs_get_sriov_totalvfs(const char *bdf);
>   void sysfs_set_sriov_numvfs(const char *bdfs, int numvfs);
>   ...
>
> That will greatly simplify the amount of code in this test, and I think
> it's highly likely we re-use those functions in other tests. And even if
> we don't, it's nice to encapsulate all the sysfs code in one place for
> readability and maintainability.
>
> If you do this I think there's also some sysfs stuff in
> vfio_pci_device.c that you can also pull out into this helper file.
Good idea. I'll create this lib.

Thank you.
Raghavendra

Powered by blists - more mailing lists