Re: [PATCH v3 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/20/22 9:56 AM, Tony Krowiak wrote:


On 5/19/22 2:33 PM, Matthew Rosato wrote:
Rather than relying on a notifier for associating the KVM with
the group, let's assume that the association has already been
made prior to device_open.  The first time a device is opened
associate the group KVM with the device.

This fixes a user-triggerable oops in GVT.

Reviewed-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>
Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Signed-off-by: Matthew Rosato <mjrosato@xxxxxxxxxxxxx>
---
  drivers/gpu/drm/i915/gvt/gtt.c        |  4 +-
  drivers/gpu/drm/i915/gvt/gvt.h        |  3 -
  drivers/gpu/drm/i915/gvt/kvmgt.c      | 82 ++++++--------------------
  drivers/s390/crypto/vfio_ap_ops.c     | 35 ++---------
  drivers/s390/crypto/vfio_ap_private.h |  3 -
  drivers/vfio/vfio.c                   | 83 ++++++++++-----------------
  include/linux/vfio.h                  |  6 +-
  7 files changed, 57 insertions(+), 159 deletions(-)


diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index e8914024f5b1..a7d2a95796d3 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1284,25 +1284,6 @@ static void vfio_ap_mdev_unset_kvm(struct ap_matrix_mdev *matrix_mdev)
      }
  }
-static int vfio_ap_mdev_group_notifier(struct notifier_block *nb,
-                       unsigned long action, void *data)
-{
-    int notify_rc = NOTIFY_OK;
-    struct ap_matrix_mdev *matrix_mdev;
-
-    if (action != VFIO_GROUP_NOTIFY_SET_KVM)
-        return NOTIFY_OK;
-
-    matrix_mdev = container_of(nb, struct ap_matrix_mdev, group_notifier);
-
-    if (!data)
-        vfio_ap_mdev_unset_kvm(matrix_mdev);
-    else if (vfio_ap_mdev_set_kvm(matrix_mdev, data))
-        notify_rc = NOTIFY_DONE;
-
-    return notify_rc;
-}
-
  static struct vfio_ap_queue *vfio_ap_find_queue(int apqn)
  {
      struct device *dev;
@@ -1402,11 +1383,10 @@ static int vfio_ap_mdev_open_device(struct vfio_device *vdev)
      unsigned long events;
      int ret;
-    matrix_mdev->group_notifier.notifier_call = vfio_ap_mdev_group_notifier;
-    events = VFIO_GROUP_NOTIFY_SET_KVM;
+    if (!vdev->kvm)
+        return -EINVAL;
-    ret = vfio_register_notifier(vdev, VFIO_GROUP_NOTIFY, &events,
-                     &matrix_mdev->group_notifier);
+    ret = vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm);
      if (ret)
          return ret;

I'm sorry I didn't see this with my last review, but maybe move the call
to vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm) after the successful
registration of the IOMMU notifier? This way you won't be plugging AP queues
into the guest only to remove them if the registration fails.

This is a pretty edge error case, and the vfio_ap_mdev_unset_kvm(matrix_mdev) call at err_kvm should do the proper cleanup, right? I guess I'm wondering if it's really any different than the prior code which would have registered the VFIO_GROUP_NOTIFY_SET_KVM first, which would have immediately triggered the notifier since the KVM was already registered to the group, meaning it would haved called vfio_ap_mdev_group_notifier->vfio_ap_mdev_set_kvm anyway (see vfio_register_group_notifier, the "The attaching of kvm and vfio_group might already happen..." comment)


@@ -1415,12 +1395,11 @@ static int vfio_ap_mdev_open_device(struct vfio_device *vdev)
      ret = vfio_register_notifier(vdev, VFIO_IOMMU_NOTIFY, &events,
                       &matrix_mdev->iommu_notifier);
      if (ret)
-        goto out_unregister_group;
+        goto err_kvm;
      return 0;
-out_unregister_group:
-    vfio_unregister_notifier(vdev, VFIO_GROUP_NOTIFY,
-                 &matrix_mdev->group_notifier);
+err_kvm:
+    vfio_ap_mdev_unset_kvm(matrix_mdev);
      return ret;
  }




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux