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

[David Matlack <dmatlack@google.com>]

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.

> -static void vfio_pci_container_setup(struct vfio_pci_device *device, const char *bdf)
> +static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,

Let's name this vfio_pci_group_get_device_fd() since it's getting the
device FD from the group using ioctl(VFIO_GROUP_GET_DEVICE_FD).

> +					      const char *bdf,
> +					      const char *vf_token)

There's an extra space before these arguments. Align them all vertically
with the first argument.

I noticed this throughout this patch, so please fix throughout the whole
series in v2.

Also please run checkpatch.pl. It will catch things like this for you.

  CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
  #78: FILE: tools/testing/selftests/vfio/lib/vfio_pci_device.c:335:
  +static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
  +                                             const char *bdf,

> +{
> +	char *arg = (char *) bdf;

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;

> +
> +	/*
> +	 * If a vf_token exists, argument to VFIO_GROUP_GET_DEVICE_FD
> +	 * will be in the form of the following example:
> +	 * "0000:04:10.0 vf_token=bd8d9d2b-5a5f-4f5a-a211-f591514ba1f3"
> +	 */
> +	if (vf_token) {
> +		size_t sz = strlen(bdf) + strlen(" "VF_TOKEN_ARG) +
> +			    strlen(vf_token) + 1;
> +
> +		arg = calloc(1, sz);
> +		VFIO_ASSERT_NOT_NULL(arg);
> +
> +		snprintf(arg, sz, "%s %s%s", bdf, VF_TOKEN_ARG, vf_token);
> +	}

UUIDs are 16 bytes so I think we could create a pretty reasonably fixed
size array to hold the argument and simplify this code, make it more
self-documenting, and eliminate VF_TOKEN_ARG. This is test code, so we
can make the array bigger in the future if we need to.  Keeping the code
simple is more important IMO.

static void vfio_pci_container_get_device_fd(struct vfio_pci_device *device,
                                             const char *bdf,
                                             const char *vf_token)
{
        char arg[64];

        if (vf_token)
                snprintf(arg, ARRAY_SIZE(arg), "%s vf_token=%s", bdf, vf_token);
        else
                snprintf(arg, ARRAY_SIZE(arg), "%s", bdf);

        device->fd = ioctl(device->group_fd, VFIO_GROUP_GET_DEVICE_FD, arg);
        VFIO_ASSERT_GE(device->fd, 0);
}

And to protect against writing off the end of arg, we can introduce a
snprintf_assert() in a separate commit. There are actually a few
snprintf() calls in vfio_pci_device.c that would be nice to convert to
snprintf_assert().

#define snprintf_assert(_s, _size, _fmt, ...) do {                      \
        int __ret = snprintf(_s, _size, _fmt, ##__VA_ARGS__);           \
        VFIO_ASSERT_LT(__ret, _size);                                   \
} while (0)

snprintf_assert() could be added in a precursor commit to this one.

> +static void vfio_device_bind_iommufd(int device_fd, int iommufd, const char *vf_token)
>  {
>  	struct vfio_device_bind_iommufd args = {
>  		.argsz = sizeof(args),
>  		.iommufd = iommufd,
>  	};
> +	uuid_t token_uuid = {0};
> +
> +	if (vf_token) {
> +		VFIO_ASSERT_EQ(uuid_parse(vf_token, token_uuid), 0);
> +		args.flags = VFIO_DEVICE_BIND_FLAG_TOKEN;

Maybe make this |= instead of = ? I had to double-check that this wasn't
overwriting any flags set above this. Obviously it's not since this is a
small function, but |= would make that super obvious.