linux-kernel - Re: [PATCH 4/4] vfio: selftests: Add tests to validate SR-IOV UAPI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aQzcQ0fJd-aCRThS@google.com>
Date: Thu, 6 Nov 2025 17:34:59 +0000
From: David Matlack <dmatlack@...gle.com>
To: Raghavendra Rao Ananta <rananta@...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 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().

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

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

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

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