Re: [PATCH 1/4] vfio: selftests: Add support for passing vf_token in device init

[David Matlack <dmatlack@google.com>]

On 2025-11-06 09:56 PM, Raghavendra Rao Ananta wrote:
> On Thu, Nov 6, 2025 at 5:22 AM David Matlack <dmatlack@google.com> wrote:
> > On 2025-11-04 12:35 AM, Raghavendra Rao Ananta wrote:
> >
> > > -struct vfio_pci_device *vfio_pci_device_init(const char *bdf, const char *iommu_mode);
> > > +struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > > +                                           const char *iommu_mode,
> > > +                                           const char *vf_token);
> >
> > Vipin is also looking at adding an optional parameter to
> > vfio_pci_device_init():
> > https://lore.kernel.org/kvm/20251018000713.677779-20-vipinsh@google.com/
> >
> > I am wondering if we should support an options struct for such
> > parameters. e.g. something like this
> >
> > diff --git a/tools/testing/selftests/vfio/lib/include/vfio_util.h b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> > index b01068d98fda..cee837fe561c 100644
> > --- a/tools/testing/selftests/vfio/lib/include/vfio_util.h
> > +++ b/tools/testing/selftests/vfio/lib/include/vfio_util.h
> > @@ -160,6 +160,10 @@ struct vfio_pci_driver {
> >         int msi;
> >  };
> >
> > +struct vfio_pci_device_options {
> > +       const char *vf_token;
> > +};
> > +
> >  struct vfio_pci_device {
> >         int fd;
> >
> > @@ -202,9 +206,18 @@ const char *vfio_pci_get_cdev_path(const char *bdf);
> >
> >  extern const char *default_iommu_mode;
> >
> > -struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > -                                             const char *iommu_mode,
> > -                                             const char *vf_token);
> > +struct vfio_pci_device *__vfio_pci_device_init(const char *bdf,
> > +                                              const char *iommu_mode,
> > +                                              const struct vfio_pci_device_options *options);
> > +
> > +static inline struct vfio_pci_device *vfio_pci_device_init(const char *bdf,
> > +                                                          const char *iommu_mode)
> > +{
> > +       static const struct vfio_pci_device_options default_options = {};
> > +
> > +       return __vfio_pci_device_init(bdf, iommu_mode, &default_options);
> > +}
> > +
> >
> > This will avoid you having to update every test.
> >
> > You can create a helper function in vfio_pci_sriov_uapi_test.c to call
> > __vfio_pci_device_init() and abstract away the options stuff from your
> > test.
> >
> I like the idea of an optional expandable struct. I'll implement this in v2.

Just to make sure we're on the same page: I don't think you need to add
this in v2 since you don't need to call vfio_pci_device_init(). For the
inner functions that you want to call from your test, passing vf_token
directly makes more sense IMO. vfio_pci_device_init() will just pass in
NULL to those functions for vf_token by default.

If/when we want to pass vf_token to vfio_pci_device_init() we can add
the options struct.

> > No space necessary after a cast. This is another one checkpatch.pl will
> > catch for you.
> >
> >   CHECK:SPACING: No space is necessary after a cast
> >   #81: FILE: tools/testing/selftests/vfio/lib/vfio_pci_device.c:338:
> >   +       char *arg = (char *) bdf;
> >
> Actually, I did run checkpatch.pl on the entire series as:
> .$ ./scripts/checkpatch.pl *.patch
> 
> I didn't see any of these warnings. Are there any other options to consider?

Ah, I run with a few additional options. That's probably why we are
seeing different output. Here's what I have in my .bashrc:

function checkpatch() {
        scripts/checkpatch.pl \
                -q \
                --strict \
                --codespell \
                --no-signoff \
                --show-types \
                --ignore gerrit_change_id,FILE_PATH_CHANGES,NOT_UNIFIED_DIFF \
                --no-summary \
                "$@"
}