> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Wednesday, March 29, 2023 11:50 PM
>
> On Wed, 29 Mar 2023 09:41:26 +0000
> "Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:
>
> > > From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
> > > Sent: Wednesday, March 29, 2023 11:14 AM
> > >
> > > > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > > > Sent: Wednesday, March 29, 2023 12:00 AM
> > > >
> > > >
> > > > Personally I don't like the suggestion to fail with -EPERM if the user
> > > > doesn't own all the affected devices. This isn't a "probe if I can do
> > > > a reset" ioctl, it's a "provide information about the devices affected
> > > > by a reset to know how to call the hot-reset ioctl". We're returning
> > > > the bdf to the cdev version of this ioctl for exactly this debugging
> > > > purpose when the devices are not owned, that becomes useless if we give
> > > > up an return -EPERM if ownership doesn't align.
> > >
> > > Jason's suggestion makes sense for returning the case of returning dev_id
> > > as dev_id is local to iommufd. If there are devices in the same dev_set are
> > > opened by multiple users, multiple iommufd would be used. Then the
> > > dev_id would have overlap. e.g. a dev_set has three devices. Device A and
> > > B are opened by the current user as cdev, dev_id #1 and #2 are generated.
> > > While device C opened by another user as cdev, dev_id #n is generated for
> > > it. If dev_id #n happens to be #1, then user gets two info entries that have
> > > the same dev_id.
> > >
> >
> > In Alex's proposal you'll set a invalid dev_id for device C so the user can
> > still get the info for diagnostic purpose instead of seeing an -EPERM error.
>
> Yes, we shouldn't be reporting dev_ids outside of the user's iommufd
> context.
Following Alex's suggestion, here are two commits to extend existing _INFO
to report dev_id.
>From ad5c81366813c5effd707a0b5f5e797f5fdb3115 Mon Sep 17 00:00:00 2001
From: Yi Liu <yi.l.liu@xxxxxxxxx>
Date: Thu, 30 Mar 2023 05:29:36 -0700
Subject: [PATCH] vfio: Mark cdev usage in vfio_device
There are users that needs to check if vfio_device is opened as cdev.
e.g. vfio-pci.
Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
---
drivers/vfio/device_cdev.c | 2 ++
include/linux/vfio.h | 14 ++++++++++++++
2 files changed, 16 insertions(+)
diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
index b5de997bff6d..56f3bbe34680 100644
--- a/drivers/vfio/device_cdev.c
+++ b/drivers/vfio/device_cdev.c
@@ -66,6 +66,7 @@ void vfio_device_cdev_close(struct vfio_device_file *df)
return;
mutex_lock(&device->dev_set->lock);
+ device->cdev_opened = false;
vfio_device_close(df);
vfio_device_put_kvm(device);
if (df->iommufd)
@@ -180,6 +181,7 @@ long vfio_device_ioctl_bind_iommufd(struct vfio_device_file *df,
* read/write/mmap
*/
smp_store_release(&df->access_granted, true);
+ device->cdev_opened = true;
mutex_unlock(&device->dev_set->lock);
return 0;
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 1367605d617c..86efc1640940 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -58,6 +58,7 @@ struct vfio_device {
struct device device; /* device.kref covers object life circle */
#if IS_ENABLED(CONFIG_VFIO_DEVICE_CDEV)
struct cdev cdev;
+ bool cdev_opened;
#endif
refcount_t refcount; /* user count on registered device*/
unsigned int open_count;
@@ -167,6 +168,19 @@ static inline int vfio_iommufd_physical_devid(struct vfio_device *vdev, u32 *id)
((void (*)(struct vfio_device *vdev)) NULL)
#endif
+#if IS_ENABLED(CONFIG_VFIO_DEVICE_CDEV)
+static inline bool vfio_device_cdev_opened(struct vfio_device *device)
+{
+ lockdep_assert_held(&device->dev_set->lock);
+ return device->cdev_opened;
+}
+#else
+static inline bool vfio_device_cdev_opened(struct vfio_device *device)
+{
+ return false;
+}
+#endif
+
/**
* @migration_set_state: Optional callback to change the migration state for
* devices that support migration. It's mandatory for
--
2.34.1
>From f796cafd6c51e49adcf76352dc1daf14712c3a48 Mon Sep 17 00:00:00 2001
From: Yi Liu <yi.l.liu@xxxxxxxxx>
Date: Thu, 30 Mar 2023 05:44:45 -0700
Subject: [PATCH] vfio/pci: Report dev_id in VFIO_DEVICE_GET_PCI_HOT_RESET_INFO
for the devices opened as cdev.
Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
---
drivers/vfio/pci/vfio_pci_core.c | 59 ++++++++++++++++++++++++++++----
include/uapi/linux/vfio.h | 6 +++-
2 files changed, 57 insertions(+), 8 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 19f5b075d70a..49e0981037f7 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -767,6 +767,20 @@ static int vfio_pci_get_irq_count(struct vfio_pci_core_device *vdev, int irq_typ
return 0;
}
+static struct vfio_device *
+vfio_pci_find_device_in_devset(struct vfio_device_set *dev_set,
+ struct pci_dev *pdev)
+{
+ struct vfio_device *cur;
+
+ lockdep_assert_held(&dev_set->lock);
+
+ list_for_each_entry(cur, &dev_set->device_list, dev_set_list)
+ if (cur->dev == &pdev->dev)
+ return cur;
+ return NULL;
+}
+
static int vfio_pci_count_devs(struct pci_dev *pdev, void *data)
{
(*(int *)data)++;
@@ -776,13 +790,20 @@ static int vfio_pci_count_devs(struct pci_dev *pdev, void *data)
struct vfio_pci_fill_info {
int max;
int cur;
+ bool require_devid;
+ struct iommufd_ctx *iommufd;
+ struct vfio_device_set *dev_set;
struct vfio_pci_dependent_device *devices;
};
static int vfio_pci_fill_devs(struct pci_dev *pdev, void *data)
{
struct vfio_pci_fill_info *fill = data;
+ struct vfio_device_set *dev_set = fill->dev_set;
struct iommu_group *iommu_group;
+ struct vfio_device *vdev;
+
+ lockdep_assert_held(&dev_set->lock);
if (fill->cur == fill->max)
return -EAGAIN; /* Something changed, try again */
@@ -791,7 +812,24 @@ static int vfio_pci_fill_devs(struct pci_dev *pdev, void *data)
if (!iommu_group)
return -EPERM; /* Cannot reset non-isolated devices */
- fill->devices[fill->cur].group_id = iommu_group_id(iommu_group);
+ /*
+ * If dev_id is needed, fill in the dev_id field, otherwise
+ * fill in group_id.
+ */
+ if (fill->require_devid) {
+ /*
+ * Report the devices that are opened as cdev and have
+ * the same iommufd with the fill->iommufd. Otherwise,
+ * just fill in an IOMMUFD_INVALID_ID.
+ */
+ vdev = vfio_pci_find_device_in_devset(dev_set, pdev);
+ if (vdev && !vfio_device_cdev_opened(vdev) &&
+ fill->iommufd == vfio_iommufd_physical_ictx(vdev))
+ vfio_iommufd_physical_devid(vdev, &fill->devices[fill->cur].dev_id);
+ fill->devices[fill->cur].dev_id = IOMMUFD_INVALID_ID;
+ } else {
+ fill->devices[fill->cur].group_id = iommu_group_id(iommu_group);
+ }
fill->devices[fill->cur].segment = pci_domain_nr(pdev->bus);
fill->devices[fill->cur].bus = pdev->bus->number;
fill->devices[fill->cur].devfn = pdev->devfn;
@@ -1230,17 +1269,25 @@ static int vfio_pci_ioctl_get_pci_hot_reset_info(
return -ENOMEM;
fill.devices = devices;
+ fill.dev_set = vdev->vdev.dev_set;
+ mutex_lock(&vdev->vdev.dev_set->lock);
+ if (vfio_device_cdev_opened(&vdev->vdev))
+ fill.require_devid = true;
ret = vfio_pci_for_each_slot_or_bus(vdev->pdev, vfio_pci_fill_devs,
&fill, slot);
+ mutex_unlock(&vdev->vdev.dev_set->lock);
/*
* If a device was removed between counting and filling, we may come up
* short of fill.max. If a device was added, we'll have a return of
* -EAGAIN above.
*/
- if (!ret)
+ if (!ret) {
hdr.count = fill.cur;
+ if (fill.require_devid)
+ hdr.flags = VFIO_PCI_HOT_RESET_FLAG_IOMMUFD_DEV_ID;
+ }
reset_info_exit:
if (copy_to_user(arg, &hdr, minsz))
@@ -2346,12 +2393,10 @@ static bool vfio_dev_in_files(struct vfio_pci_core_device *vdev,
static int vfio_pci_is_device_in_set(struct pci_dev *pdev, void *data)
{
struct vfio_device_set *dev_set = data;
- struct vfio_device *cur;
- list_for_each_entry(cur, &dev_set->device_list, dev_set_list)
- if (cur->dev == &pdev->dev)
- return 0;
- return -EBUSY;
+ lockdep_assert_held(&dev_set->lock);
+
+ return vfio_pci_find_device_in_devset(dev_set, pdev) ? 0 : -EBUSY;
}
/*
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 53c72e26ecd3..3fcbc84d51ba 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -743,7 +743,10 @@ enum {
* -enospc = insufficient buffer, -enodev = unsupported for device.
*/
struct vfio_pci_dependent_device {
- __u32 group_id;
+ union {
+ __u32 group_id;
+ __u32 dev_id;
+ };
__u16 segment;
__u8 bus;
__u8 devfn; /* Use PCI_SLOT/PCI_FUNC */
@@ -752,6 +755,7 @@ struct vfio_pci_dependent_device {
struct vfio_pci_hot_reset_info {
__u32 argsz;
__u32 flags;
+#define VFIO_PCI_HOT_RESET_FLAG_IOMMUFD_DEV_ID (1 << 0)
__u32 count;
struct vfio_pci_dependent_device devices[];
};
--
2.34.1
Regards,
Yi Liu
