Message-ID: <CAJHc60ycPfeba0hjiHLTgFO2JAjPsuWzHhJqVbqOTEaOPfNy_A@mail.gmail.com>
Date: Thu, 6 Nov 2025 22:35: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 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?

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

> > +
> > +static void test_vfio_pci_container_setup(struct vfio_pci_device *device,
> > +                                        const char *bdf,
> > +                                        const char *vf_token)
> > +{
> > +     vfio_container_open(device);
> > +     vfio_pci_group_setup(device, bdf);
> > +     vfio_container_set_iommu(device);
> > +     __vfio_container_get_device_fd(device, bdf, vf_token);
> > +}
> > +
> > +static int test_vfio_pci_iommufd_setup(struct vfio_pci_device *device,
> > +                                     const char *bdf, const char *vf_token)
> > +{
> > +     vfio_pci_iommufd_cdev_open(device, bdf);
> > +     vfio_pci_iommufd_iommudev_open(device);
> > +     return __vfio_device_bind_iommufd(device->fd, device->iommufd, vf_token);
> > +}
> > +
> > +static struct vfio_pci_device *test_vfio_pci_device_init(const char *bdf,
> > +                                                       const char *iommu_mode,
> > +                                                       const char *vf_token,
> > +                                                       int *out_ret)
> > +{
> > +     struct vfio_pci_device *device;
> > +
> > +     device = calloc(1, sizeof(*device));
> > +     VFIO_ASSERT_NOT_NULL(device);
> > +
> > +     device->iommu_mode = lookup_iommu_mode(iommu_mode);
> > +
> > +     if (iommu_mode_container_path(iommu_mode)) {
> > +             test_vfio_pci_container_setup(device, bdf, vf_token);
> > +             /* The device fd will be -1 in case of mismatched tokens */
> > +             *out_ret = (device->fd < 0);
>
> Maybe just return device->fd from test_vfio_pci_container_setup() so
> this can be:
>
>   *out_ret = test_vfio_pci_container_setup(device, bdf, vf_token);
>
> and then you can drop the curly braces.
>
Makes sense. I'll do it in v2.

> > +     } else {
> > +             *out_ret = test_vfio_pci_iommufd_setup(device, bdf, vf_token);
> > +     }
> > +
> > +     return device;
> > +}
> > +
> > +static void test_vfio_pci_device_cleanup(struct vfio_pci_device *device)
> > +{
> > +     if (device->fd > 0)
> > +             VFIO_ASSERT_EQ(close(device->fd), 0);
> > +
> > +     if (device->iommufd) {
> > +             VFIO_ASSERT_EQ(close(device->iommufd), 0);
> > +     } else {
> > +             VFIO_ASSERT_EQ(close(device->group_fd), 0);
> > +             VFIO_ASSERT_EQ(close(device->container_fd), 0);
> > +     }
> > +
> > +     free(device);
> > +}
> > +
> > +FIXTURE(vfio_pci_sriov_uapi_test) {};
> > +
> > +FIXTURE_SETUP(vfio_pci_sriov_uapi_test)
> > +{
> > +     char vf_path[PATH_MAX] = {0};
> > +     char path[PATH_MAX] = {0};
> > +     unsigned int nr_vfs;
> > +     char buf[32] = {0};
> > +     int ret;
> > +     int fd;
> > +
> > +     /* Check if SR-IOV is supported by the device */
> > +     snprintf(path, PATH_MAX, "%s/%s/sriov_totalvfs", PCI_SYSFS_PATH, pf_dev_bdf);
>
> nit: Personally I would just hard-code the sysfs path instead of using
> PCI_SYSFS_PATH. I think the code is more readable and more succinct that
> way. And sysfs should be a stable ABI.
>
Sure.

> > +     fd = open(path, O_RDONLY);
> > +     if (fd < 0) {
> > +             fprintf(stderr, "SR-IOV may not be supported by the device\n");
> > +             exit(KSFT_SKIP);
>
> Use SKIP() for this:
>
Sure.
> if (fd < 0)
>         SKIP(return, "SR-IOV is not supported by the device\n");
>
> Ditto below.
>
> > +     }
> > +
> > +     ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);
> > +     ASSERT_EQ(close(fd), 0);
> > +     nr_vfs = strtoul(buf, NULL, 0);
> > +     if (nr_vfs < 0) {
> > +             fprintf(stderr, "SR-IOV may not be supported by the device\n");
> > +             exit(KSFT_SKIP);
> > +     }
> > +
> > +     /* Setup VFs, if already not done */
>
> Before creating VFs, should we disable auto-probing so the VFs don't get
> bound to some other random driver (write 0 to sriov_drivers_autoprobe)?
>
Good idea. I'll make this change in v2.

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

> > +             ASSERT_EQ(write(fd, "1", 1), 1);
> > +     ASSERT_EQ(close(fd), 0);
> > +
> > +     /* Get the BDF of the first VF */
> > +     snprintf(path, PATH_MAX, "%s/%s/virtfn0", PCI_SYSFS_PATH, pf_dev_bdf);
> > +     ret = readlink(path, vf_path, PATH_MAX);
> > +     ASSERT_NE(ret, -1);
> > +     ret = sscanf(basename(vf_path), "%s", vf_dev_bdf);
> > +     ASSERT_EQ(ret, 1);
>
> What ensures the created VF is bound to vfio-pci?
>
Good point. I'll explicitly bind it to vfio-pci, if it isn't already.

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

> 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 :) )

Powered by blists - more mailing lists