linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] vfio/pci: print vfio-device name to fdinfo
@ 2025-06-23 21:02 Alex Mastro
  2025-06-23 21:50 ` Keith Busch
  2025-06-23 22:18 ` Alex Williamson
  0 siblings, 2 replies; 11+ messages in thread
From: Alex Mastro @ 2025-06-23 21:02 UTC (permalink / raw)
  To: Alex Williamson
  Cc: peterx, kbusch, kvm, linux-kernel, Jason Gunthorpe, Alex Mastro

Print the PCI device name to a vfio device's fdinfo. This enables tools
to query which device is associated with a given vfio device fd. It's
inspired by eventfd's printing of "eventfd-id" (fs/eventfd.c), which
lsof uses to format the NAME column (e.g. "[eventfd:7278]").

This results in output like below:

$ cat /proc/"$process_using_vfio"/fdinfo/"$vfio_device_fd" | grep vfio
vfio-device-name: 0000:c6:00.0

Signed-off-by: Alex Mastro <amastro@fb.com>
---
Hello, this is my first patch submission to vfio, and linux. We would
like our tools to be able to query the PCI device name for a given
vfio-device fd by inspecting a process's open file descriptors. It is
inspired by eventfd's id printing, which is nicely formatted by lsof in
the NAME column.

I am not sure to what extent this should be generalized, so I opted
to put as little policy as possible into vfio_main.c, and have each
vfio_device_fops implement what it means to show_fdinfo. The only
implementer is vfio_pci_ops in this change.

Alternatively, if we wanted to normalize show_fdinfo formatting, this
could instead hoist the print formatting up into vfio_main.c, and call
an optional vfio_device_ops->instance_name() to get the name. I opted
not to do this here due to unfamiliarity with other vfio drivers, but am
open to changing it.

I noticed that other vfio_device_fops are guarded by checks on
vfio_device_file.access_granted. From what I can tell, that shouldn't
be required here, since a vfio pci device is guaranteed to be
able to print its name (due to existence of vfio_device.pdev) after
vfio_device_ops.init() construction.

This change rooted on the for-linus branch of linux-vfio [1].

[1] https://github.com/awilliam/linux-vfio
---
 drivers/vfio/pci/vfio_pci.c | 14 ++++++++++++++
 drivers/vfio/vfio_main.c    | 15 +++++++++++++++
 include/linux/vfio.h        |  2 ++
 3 files changed, 31 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 5ba39f7623bb..b682766127ab 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -21,6 +21,7 @@
 #include <linux/mutex.h>
 #include <linux/notifier.h>
 #include <linux/pm_runtime.h>
+#include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 #include <linux/uaccess.h>
@@ -125,6 +126,16 @@ static int vfio_pci_open_device(struct vfio_device *core_vdev)
 	return 0;
 }
 
+#ifdef CONFIG_PROC_FS
+static void vfio_pci_core_show_fdinfo(struct vfio_device *core_vdev, struct seq_file *m)
+{
+	struct vfio_pci_core_device *vdev =
+		container_of(core_vdev, struct vfio_pci_core_device, vdev);
+
+	seq_printf(m, "vfio-device-name: %s\n", pci_name(vdev->pdev));
+}
+#endif
+
 static const struct vfio_device_ops vfio_pci_ops = {
 	.name		= "vfio-pci",
 	.init		= vfio_pci_core_init_dev,
@@ -138,6 +149,9 @@ static const struct vfio_device_ops vfio_pci_ops = {
 	.mmap		= vfio_pci_core_mmap,
 	.request	= vfio_pci_core_request,
 	.match		= vfio_pci_core_match,
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo	= vfio_pci_core_show_fdinfo,
+#endif
 	.bind_iommufd	= vfio_iommufd_physical_bind,
 	.unbind_iommufd	= vfio_iommufd_physical_unbind,
 	.attach_ioas	= vfio_iommufd_physical_attach_ioas,
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 1fd261efc582..e02504247da8 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -28,6 +28,7 @@
 #include <linux/pseudo_fs.h>
 #include <linux/rwsem.h>
 #include <linux/sched.h>
+#include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/stat.h>
 #include <linux/string.h>
@@ -1354,6 +1355,17 @@ static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
 	return device->ops->mmap(device, vma);
 }
 
+#ifdef CONFIG_PROC_FS
+static void vfio_device_show_fdinfo(struct seq_file *m, struct file *filep)
+{
+	struct vfio_device_file *df = filep->private_data;
+	struct vfio_device *device = df->device;
+
+	if (device->ops->show_fdinfo)
+		device->ops->show_fdinfo(device, m);
+}
+#endif
+
 const struct file_operations vfio_device_fops = {
 	.owner		= THIS_MODULE,
 	.open		= vfio_device_fops_cdev_open,
@@ -1363,6 +1375,9 @@ const struct file_operations vfio_device_fops = {
 	.unlocked_ioctl	= vfio_device_fops_unl_ioctl,
 	.compat_ioctl	= compat_ptr_ioctl,
 	.mmap		= vfio_device_fops_mmap,
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo	= vfio_device_show_fdinfo,
+#endif
 };
 
 static struct vfio_device *vfio_device_from_file(struct file *file)
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 707b00772ce1..54076045a44f 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -16,6 +16,7 @@
 #include <linux/cdev.h>
 #include <uapi/linux/vfio.h>
 #include <linux/iova_bitmap.h>
+#include <linux/seq_file.h>
 
 struct kvm;
 struct iommufd_ctx;
@@ -135,6 +136,7 @@ struct vfio_device_ops {
 	void	(*dma_unmap)(struct vfio_device *vdev, u64 iova, u64 length);
 	int	(*device_feature)(struct vfio_device *device, u32 flags,
 				  void __user *arg, size_t argsz);
+	void	(*show_fdinfo)(struct vfio_device *device, struct seq_file *m);
 };
 
 #if IS_ENABLED(CONFIG_IOMMUFD)

---
base-commit: c1d9dac0db168198b6f63f460665256dedad9b6e
change-id: 20250623-vfio-fdinfo-767e75a1496a

Best regards,
-- 
Alex Mastro <amastro@fb.com>


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] vfio/pci: print vfio-device name to fdinfo
  2025-06-23 21:02 [PATCH] vfio/pci: print vfio-device name to fdinfo Alex Mastro
@ 2025-06-23 21:50 ` Keith Busch
  2025-06-23 22:18 ` Alex Williamson
  1 sibling, 0 replies; 11+ messages in thread
From: Keith Busch @ 2025-06-23 21:50 UTC (permalink / raw)
  To: Alex Mastro; +Cc: Alex Williamson, peterx, kvm, linux-kernel, Jason Gunthorpe

On Mon, Jun 23, 2025 at 02:02:38PM -0700, Alex Mastro wrote:
> Print the PCI device name to a vfio device's fdinfo. This enables tools
> to query which device is associated with a given vfio device fd. It's
> inspired by eventfd's printing of "eventfd-id" (fs/eventfd.c), which
> lsof uses to format the NAME column (e.g. "[eventfd:7278]").
> 
> This results in output like below:
> 
> $ cat /proc/"$process_using_vfio"/fdinfo/"$vfio_device_fd" | grep vfio
> vfio-device-name: 0000:c6:00.0

procfs' fdinfo sounds like the right interface to append this attribute,
so looks looks good to me!

Reviewed-by: Keith Busch <kbusch@kernel.org>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vfio/pci: print vfio-device name to fdinfo
  2025-06-23 21:02 [PATCH] vfio/pci: print vfio-device name to fdinfo Alex Mastro
  2025-06-23 21:50 ` Keith Busch
@ 2025-06-23 22:18 ` Alex Williamson
  2025-06-23 23:14   ` Alex Mastro
  2025-06-24  0:56   ` Jason Gunthorpe
  1 sibling, 2 replies; 11+ messages in thread
From: Alex Williamson @ 2025-06-23 22:18 UTC (permalink / raw)
  To: Alex Mastro; +Cc: peterx, kbusch, kvm, linux-kernel, Jason Gunthorpe

On Mon, 23 Jun 2025 14:02:38 -0700
Alex Mastro <amastro@fb.com> wrote:

> Print the PCI device name to a vfio device's fdinfo. This enables tools
> to query which device is associated with a given vfio device fd. It's
> inspired by eventfd's printing of "eventfd-id" (fs/eventfd.c), which
> lsof uses to format the NAME column (e.g. "[eventfd:7278]").
> 
> This results in output like below:
> 
> $ cat /proc/"$process_using_vfio"/fdinfo/"$vfio_device_fd" | grep vfio
> vfio-device-name: 0000:c6:00.0
> 
> Signed-off-by: Alex Mastro <amastro@fb.com>
> ---
> Hello, this is my first patch submission to vfio, and linux. We would
> like our tools to be able to query the PCI device name for a given
> vfio-device fd by inspecting a process's open file descriptors. It is
> inspired by eventfd's id printing, which is nicely formatted by lsof in
> the NAME column.
> 
> I am not sure to what extent this should be generalized, so I opted
> to put as little policy as possible into vfio_main.c, and have each
> vfio_device_fops implement what it means to show_fdinfo. The only
> implementer is vfio_pci_ops in this change.
> 
> Alternatively, if we wanted to normalize show_fdinfo formatting, this
> could instead hoist the print formatting up into vfio_main.c, and call
> an optional vfio_device_ops->instance_name() to get the name. I opted
> not to do this here due to unfamiliarity with other vfio drivers, but am
> open to changing it.

TBH, I don't think we need a callback, just use dev_name() in
vfio_main.  The group interface always requires the name, in some cases
it can require further information, but we seem to have forgotten that
in the cdev interface anyway :-\

> I noticed that other vfio_device_fops are guarded by checks on
> vfio_device_file.access_granted. From what I can tell, that shouldn't
> be required here, since a vfio pci device is guaranteed to be
> able to print its name (due to existence of vfio_device.pdev) after
> vfio_device_ops.init() construction.

Yep, we shouldn't need any extra locking.
 
> This change rooted on the for-linus branch of linux-vfio [1].
> 
> [1] https://github.com/awilliam/linux-vfio
> ---
>  drivers/vfio/pci/vfio_pci.c | 14 ++++++++++++++
>  drivers/vfio/vfio_main.c    | 15 +++++++++++++++
>  include/linux/vfio.h        |  2 ++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 5ba39f7623bb..b682766127ab 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -21,6 +21,7 @@
>  #include <linux/mutex.h>
>  #include <linux/notifier.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/seq_file.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
> @@ -125,6 +126,16 @@ static int vfio_pci_open_device(struct vfio_device *core_vdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PROC_FS
> +static void vfio_pci_core_show_fdinfo(struct vfio_device *core_vdev, struct seq_file *m)
> +{
> +	struct vfio_pci_core_device *vdev =
> +		container_of(core_vdev, struct vfio_pci_core_device, vdev);
> +
> +	seq_printf(m, "vfio-device-name: %s\n", pci_name(vdev->pdev));
> +}
> +#endif
> +
>  static const struct vfio_device_ops vfio_pci_ops = {
>  	.name		= "vfio-pci",
>  	.init		= vfio_pci_core_init_dev,
> @@ -138,6 +149,9 @@ static const struct vfio_device_ops vfio_pci_ops = {
>  	.mmap		= vfio_pci_core_mmap,
>  	.request	= vfio_pci_core_request,
>  	.match		= vfio_pci_core_match,
> +#ifdef CONFIG_PROC_FS
> +	.show_fdinfo	= vfio_pci_core_show_fdinfo,
> +#endif
>  	.bind_iommufd	= vfio_iommufd_physical_bind,
>  	.unbind_iommufd	= vfio_iommufd_physical_unbind,
>  	.attach_ioas	= vfio_iommufd_physical_attach_ioas,
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 1fd261efc582..e02504247da8 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -28,6 +28,7 @@
>  #include <linux/pseudo_fs.h>
>  #include <linux/rwsem.h>
>  #include <linux/sched.h>
> +#include <linux/seq_file.h>

Only where we're doing the seq_print do we need to include this.

I think you're missing an update to Documentation/filesystems/proc.rst
as well.

>  #include <linux/slab.h>
>  #include <linux/stat.h>
>  #include <linux/string.h>
> @@ -1354,6 +1355,17 @@ static int vfio_device_fops_mmap(struct file *filep, struct vm_area_struct *vma)
>  	return device->ops->mmap(device, vma);
>  }
>  
> +#ifdef CONFIG_PROC_FS
> +static void vfio_device_show_fdinfo(struct seq_file *m, struct file *filep)
> +{
> +	struct vfio_device_file *df = filep->private_data;
> +	struct vfio_device *device = df->device;
> +
> +	if (device->ops->show_fdinfo)
> +		device->ops->show_fdinfo(device, m);
> +}
> +#endif
> +
>  const struct file_operations vfio_device_fops = {
>  	.owner		= THIS_MODULE,
>  	.open		= vfio_device_fops_cdev_open,
> @@ -1363,6 +1375,9 @@ const struct file_operations vfio_device_fops = {
>  	.unlocked_ioctl	= vfio_device_fops_unl_ioctl,
>  	.compat_ioctl	= compat_ptr_ioctl,
>  	.mmap		= vfio_device_fops_mmap,
> +#ifdef CONFIG_PROC_FS
> +	.show_fdinfo	= vfio_device_show_fdinfo,
> +#endif
>  };
>  
>  static struct vfio_device *vfio_device_from_file(struct file *file)
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 707b00772ce1..54076045a44f 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -16,6 +16,7 @@
>  #include <linux/cdev.h>
>  #include <uapi/linux/vfio.h>
>  #include <linux/iova_bitmap.h>
> +#include <linux/seq_file.h>
>  
>  struct kvm;
>  struct iommufd_ctx;
> @@ -135,6 +136,7 @@ struct vfio_device_ops {
>  	void	(*dma_unmap)(struct vfio_device *vdev, u64 iova, u64 length);
>  	int	(*device_feature)(struct vfio_device *device, u32 flags,
>  				  void __user *arg, size_t argsz);
> +	void	(*show_fdinfo)(struct vfio_device *device, struct seq_file *m);

Were we to keep this, the comment would need to be updated to describe
it.  Thanks,

Alex

>  };
>  
>  #if IS_ENABLED(CONFIG_IOMMUFD)
> 
> ---
> base-commit: c1d9dac0db168198b6f63f460665256dedad9b6e
> change-id: 20250623-vfio-fdinfo-767e75a1496a
> 
> Best regards,


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vfio/pci: print vfio-device name to fdinfo
  2025-06-23 22:18 ` Alex Williamson
@ 2025-06-23 23:14   ` Alex Mastro
  2025-06-24  0:56   ` Jason Gunthorpe
  1 sibling, 0 replies; 11+ messages in thread
From: Alex Mastro @ 2025-06-23 23:14 UTC (permalink / raw)
  To: Alex Williamson
  Cc: Alex Mastro, peterx, kbusch, kvm, linux-kernel, Jason Gunthorpe

Hi Alex, thanks for the prompt feedback.

On Mon, 23 Jun 2025 16:18:31 -0600 Alex Williamson <alex.williamson@redhat.com> wrote:
> TBH, I don't think we need a callback, just use dev_name() in
> vfio_main.  The group interface always requires the name, in some cases
> it can require further information, but we seem to have forgotten that
> in the cdev interface anyway :-\

Sounds good -- I'll follow up on this in v2.

> > +#include <linux/seq_file.h>
> 
> Only where we're doing the seq_print do we need to include this.

Ah, I only added it due to the direct reference to `struct seq_file *` in this
file, but will keep this in mind. I do see that it's transitively visible via
vfio.h's include.

> I think you're missing an update to Documentation/filesystems/proc.rst
> as well.

TIL about this documentation (section 3.8), but it looks to be focused more on
common fs/* pieces like eventfd, signalfd, and such. I didn't see any drivers/*
precedence, but would not be opposed to being the first. Do you think it still
makes sense to add there?

Thanks,
Alex

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vfio/pci: print vfio-device name to fdinfo
  2025-06-23 22:18 ` Alex Williamson
  2025-06-23 23:14   ` Alex Mastro
@ 2025-06-24  0:56   ` Jason Gunthorpe
  2025-06-24 16:23     ` Alex Williamson
  2025-06-24 16:35     ` Alex Mastro
  1 sibling, 2 replies; 11+ messages in thread
From: Jason Gunthorpe @ 2025-06-24  0:56 UTC (permalink / raw)
  To: Alex Williamson; +Cc: Alex Mastro, peterx, kbusch, kvm, linux-kernel

On Mon, Jun 23, 2025 at 04:18:31PM -0600, Alex Williamson wrote:
> > Alternatively, if we wanted to normalize show_fdinfo formatting, this
> > could instead hoist the print formatting up into vfio_main.c, and call
> > an optional vfio_device_ops->instance_name() to get the name. I opted
> > not to do this here due to unfamiliarity with other vfio drivers, but am
> > open to changing it.
> 
> TBH, I don't think we need a callback, just use dev_name() in
> vfio_main.

IMHO this should really be the name of /dev/vfio/XX file and not
something made up like event fd uses.

The file was opened via /dev/vfio/XX, that is what lsof should report..

For the legacy route this effectively gives you the iommu group.

For the new route this will give you the struct device.

The userspace can deduce more information, like the actual PCI BDF, by
mapping the name through sysfs.

I would have guessed this is already happening automatically as part
of the cdev mechanism? Maybe we broken it when we changed the inode to
use unmap mapping range?

> The group interface always requires the name, in some cases
> it can require further information, but we seem to have forgotten that
> in the cdev interface anyway :-\

?

Jason

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vfio/pci: print vfio-device name to fdinfo
  2025-06-24  0:56   ` Jason Gunthorpe
@ 2025-06-24 16:23     ` Alex Williamson
  2025-06-24 18:12       ` Jason Gunthorpe
  2025-06-24 16:35     ` Alex Mastro
  1 sibling, 1 reply; 11+ messages in thread
From: Alex Williamson @ 2025-06-24 16:23 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: Alex Mastro, peterx, kbusch, kvm, linux-kernel

On Mon, 23 Jun 2025 21:56:05 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Mon, Jun 23, 2025 at 04:18:31PM -0600, Alex Williamson wrote:
> > > Alternatively, if we wanted to normalize show_fdinfo formatting, this
> > > could instead hoist the print formatting up into vfio_main.c, and call
> > > an optional vfio_device_ops->instance_name() to get the name. I opted
> > > not to do this here due to unfamiliarity with other vfio drivers, but am
> > > open to changing it.  
> > 
> > TBH, I don't think we need a callback, just use dev_name() in
> > vfio_main.  
> 
> IMHO this should really be the name of /dev/vfio/XX file and not
> something made up like event fd uses.
> 
> The file was opened via /dev/vfio/XX, that is what lsof should report..
> 
> For the legacy route this effectively gives you the iommu group.

We don't need fdinfo for this, right?  The group is clearly visible in
/proc/PID/fd:

# ls -l | grep vfio
lrwx------. 1 qemu qemu 64 Jun 24 09:27 32 -> /dev/vfio/16
lrwx------. 1 qemu qemu 64 Jun 24 09:27 33 -> /dev/vfio/vfio
lrwx------. 1 qemu qemu 64 Jun 24 09:27 34 -> anon_inode:kvm-vfio
lrwx------. 1 qemu qemu 64 Jun 24 09:27 35 -> anon_inode:[vfio-device]
lrwx------. 1 qemu qemu 64 Jun 24 09:27 38 -> /dev/vfio/2
lrwx------. 1 qemu qemu 64 Jun 24 09:27 39 -> anon_inode:[vfio-device]
lrwx------. 1 qemu qemu 64 Jun 24 09:27 44 -> anon_inode:[vfio-device]
lrwx------. 1 qemu qemu 64 Jun 24 09:27 49 -> /dev/vfio/12
lrwx------. 1 qemu qemu 64 Jun 24 09:27 50 -> anon_inode:[vfio-device]
lrwx------. 1 qemu qemu 64 Jun 24 09:27 55 -> /dev/vfio/4
lrwx------. 1 qemu qemu 64 Jun 24 09:27 56 -> anon_inode:[vfio-device]

An iommufd/vfio-cdev VM even more clearly shows the devices:

# ls -l | grep vfio
lrwx------. 1 root root 64 Jun 24 10:06 23 -> /dev/vfio/devices/vfio7
lrwx------. 1 root root 64 Jun 24 10:06 24 -> anon_inode:kvm-vfio
lrwx------. 1 root root 64 Jun 24 10:06 30 -> /dev/vfio/devices/vfio8

I think we're specifically trying to gain visibility to the
anon_inode:[vfio-device] in the legacy case.

The @name passed to anon_inode_getfile_fmode() is described as the name
of the "class", which is why I think we used the static
"[vfio-device]", but I see KVM breaks the mold, adding the vcpu_id:

	snprintf(name, sizeof(name), "kvm-vcpu-stats:%d", vcpu->vcpu_id);

We could do something similar, but maybe fdinfo is the better option,
and if it is then dev_name() seems like the useful thing to add there
(though we could add more than one thing).

> For the new route this will give you the struct device.
> 
> The userspace can deduce more information, like the actual PCI BDF, by
> mapping the name through sysfs.

I don't really know what the rules are here, whether we're able to
report information for convenience or we should strive for the absolute
most concise reference.  cdev and group information is available
without fdinfo.

> I would have guessed this is already happening automatically as part
> of the cdev mechanism? Maybe we broken it when we changed the inode to
> use unmap mapping range?
> 
> > The group interface always requires the name, in some cases
> > it can require further information, but we seem to have forgotten that
> > in the cdev interface anyway :-\  
> 
> ?

I don't recall if or how we accounted for the concept of vf_tokens in
the cdev model and I don't see evidence that we did.  For instance
vfio_pci_validate_vf_token() is only called from vfio_pci_core_match(),
which is called as match through the vfio_device_ops, but only from
vfio_group_ioctl_get_device_fd().  So using cdev, it appears we don't
have the same opt-in requirement when using a VF where the PF is
managed by a vfio-pci userspace driver.  Thanks,

Alex


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vfio/pci: print vfio-device name to fdinfo
  2025-06-24  0:56   ` Jason Gunthorpe
  2025-06-24 16:23     ` Alex Williamson
@ 2025-06-24 16:35     ` Alex Mastro
  2025-06-24 18:16       ` Jason Gunthorpe
  1 sibling, 1 reply; 11+ messages in thread
From: Alex Mastro @ 2025-06-24 16:35 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Alex Mastro, Alex Williamson, peterx, kbusch, kvm, linux-kernel

On Mon, 23 Jun 2025 21:56:05 -0300 Jason Gunthorpe <jgg@ziepe.ca> wrote:
> For the legacy route this effectively gives you the iommu group.

This is true, but
- There could be multiple devices per group, and I can't see a good way to
  determine exactly which one is in use. (we happen to have one device per
  group, so this particular concern is somewhat moot for us)
- In our use case, we vend the vfio device fd via SCM_RIGHTS to other processes
  which did not open the group fd. We keep track of this internally, but it's an
  imperfect solution, and we'd like a more fundamental way to query the kernel
  for this.

> For the new route this will give you the struct device.
> 
> The userspace can deduce more information, like the actual PCI BDF, by
> mapping the name through sysfs.

In the vfio cdev case, <pci sysfs path>/vfio-dev/X tells you the
/dev/vfio/devices/X mapping, but is there a straightforward way to map in the
opposite direction?

I'm open to other approaches to solving the issue, but making an additive change
to fdinfo seemed innocuous enough, and hopefully unlikely to be incompatible
with future direction of this subsystem.

Thanks,
Alex

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vfio/pci: print vfio-device name to fdinfo
  2025-06-24 16:23     ` Alex Williamson
@ 2025-06-24 18:12       ` Jason Gunthorpe
  0 siblings, 0 replies; 11+ messages in thread
From: Jason Gunthorpe @ 2025-06-24 18:12 UTC (permalink / raw)
  To: Alex Williamson; +Cc: Alex Mastro, peterx, kbusch, kvm, linux-kernel

On Tue, Jun 24, 2025 at 10:23:03AM -0600, Alex Williamson wrote:

> I think we're specifically trying to gain visibility to the
> anon_inode:[vfio-device] in the legacy case.

Ah, I see.. 

> The @name passed to anon_inode_getfile_fmode() is described as the name
> of the "class", which is why I think we used the static
> "[vfio-device]", but I see KVM breaks the mold, adding the vcpu_id:
> 
> 	snprintf(name, sizeof(name), "kvm-vcpu-stats:%d", vcpu->vcpu_id);
> 
> We could do something similar, but maybe fdinfo is the better option,
> and if it is then dev_name() seems like the useful thing to add there
> (though we could add more than one thing).

I wouldn't encode a sysfspath (which is what you really need for a
device name) in the [] section.. fdinfo makes sense for that, but I
would return the full sysfs path to the device from the core code
rather than try to return just the BDF for PCI.

Prefix /sys/ and then userspace can inspect the directory for whatever
information it needs.

It could also return a %d within the [] that indicated which group it
was for. In most system that will tell you the device anyhow since
groups are singular.

> I don't recall if or how we accounted for the concept of vf_tokens in
> the cdev model and I don't see evidence that we did.  For instance
> vfio_pci_validate_vf_token() is only called from vfio_pci_core_match(),
> which is called as match through the vfio_device_ops, but only from
> vfio_group_ioctl_get_device_fd().  So using cdev, it appears we don't
> have the same opt-in requirement when using a VF where the PF is
> managed by a vfio-pci userspace driver.  Thanks,

Hmm.. I can't recall.

I wrote a small patch to correct this, I will post it.

Jason

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vfio/pci: print vfio-device name to fdinfo
  2025-06-24 16:35     ` Alex Mastro
@ 2025-06-24 18:16       ` Jason Gunthorpe
  2025-06-24 18:53         ` Alex Mastro
  0 siblings, 1 reply; 11+ messages in thread
From: Jason Gunthorpe @ 2025-06-24 18:16 UTC (permalink / raw)
  To: Alex Mastro; +Cc: Alex Williamson, peterx, kbusch, kvm, linux-kernel

On Tue, Jun 24, 2025 at 09:35:58AM -0700, Alex Mastro wrote:

> > The userspace can deduce more information, like the actual PCI BDF, by
> > mapping the name through sysfs.
> 
> In the vfio cdev case, <pci sysfs path>/vfio-dev/X tells you the
> /dev/vfio/devices/X mapping, but is there a straightforward way to map in the
> opposite direction?

There will be a symlink under /sys/class/vfio-xx/XX pointing to the
<pci sysfs path>/vfio-dev/X directory

And another symlink under /sys/dev/char/XX:XX doing the same.

Jason

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vfio/pci: print vfio-device name to fdinfo
  2025-06-24 18:16       ` Jason Gunthorpe
@ 2025-06-24 18:53         ` Alex Mastro
  2025-06-24 18:58           ` Jason Gunthorpe
  0 siblings, 1 reply; 11+ messages in thread
From: Alex Mastro @ 2025-06-24 18:53 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Alex Mastro, Alex Williamson, peterx, kbusch, kvm, linux-kernel

On Tue, 24 Jun 2025 15:16:16 -0300 Jason Gunthorpe <jgg@ziepe.ca> wrote:
> There will be a symlink under /sys/class/vfio-xx/XX pointing to the
> <pci sysfs path>/vfio-dev/X directory
> 
> And another symlink under /sys/dev/char/XX:XX doing the same.

Got it, thanks. The issue does seem solved for the vfio cdev case, then.

Alex


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] vfio/pci: print vfio-device name to fdinfo
  2025-06-24 18:53         ` Alex Mastro
@ 2025-06-24 18:58           ` Jason Gunthorpe
  0 siblings, 0 replies; 11+ messages in thread
From: Jason Gunthorpe @ 2025-06-24 18:58 UTC (permalink / raw)
  To: Alex Mastro; +Cc: Alex Williamson, peterx, kbusch, kvm, linux-kernel

On Tue, Jun 24, 2025 at 11:53:25AM -0700, Alex Mastro wrote:
> On Tue, 24 Jun 2025 15:16:16 -0300 Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > There will be a symlink under /sys/class/vfio-xx/XX pointing to the
> > <pci sysfs path>/vfio-dev/X directory
> > 
> > And another symlink under /sys/dev/char/XX:XX doing the same.
> 
> Got it, thanks. The issue does seem solved for the vfio cdev case, then.

I wonder if we could arrange things so that if the cdev path is turned
on then the device FD created by the ioctl would report the same
/dev/vfio/X information? I've never looked at how this works, but
maybe it is easy?

Even if userspace is not using the cdev it does exist and is still
logically affiliated with the ioctl FD.

Then we'd have a nice consistent user experience.

Jason

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-06-24 18:58 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-23 21:02 [PATCH] vfio/pci: print vfio-device name to fdinfo Alex Mastro
2025-06-23 21:50 ` Keith Busch
2025-06-23 22:18 ` Alex Williamson
2025-06-23 23:14   ` Alex Mastro
2025-06-24  0:56   ` Jason Gunthorpe
2025-06-24 16:23     ` Alex Williamson
2025-06-24 18:12       ` Jason Gunthorpe
2025-06-24 16:35     ` Alex Mastro
2025-06-24 18:16       ` Jason Gunthorpe
2025-06-24 18:53         ` Alex Mastro
2025-06-24 18:58           ` Jason Gunthorpe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).