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

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

 



On 5/19/22 12:23 PM, Tony Krowiak wrote:
I made a few comments, but other than that this looks good to
me:

Reviewed-by: Tony Krowiak <akrowiak@xxxxxxxxxxxxx>


...

I'm not sure what version of the code on which the patch was rebased,

Was on top of Jason's vfio_group_locking series, but now would apply on vfio-next since Alex pulled that series in to vfio-next.

but in the
latest master branch from our repository the kvm_get_kvm(kvm) function is
called inside of the if block below. I'm fine with moving outside of the block, but
I don't see a corresponding removal of it from inside the block.

Yeah, I didn't notice those there. v3 will simply remove my get/put additions and leave yours as-is.

...

vfio_ap_mdev_group_notifier;
-    events = VFIO_GROUP_NOTIFY_SET_KVM;
+    if (!vdev->kvm)
+        return -EPERM;

Perhaps -EINVAL or -EFAULT?


Whichever you'd prefer?  If I don't hear back I'll just use -EINVAL in v3.

-    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;
@@ -1415,12 +1400,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;
  }
@@ -1431,8 +1415,6 @@ static void vfio_ap_mdev_close_device(struct vfio_device *vdev)
      vfio_unregister_notifier(vdev, VFIO_IOMMU_NOTIFY,
                   &matrix_mdev->iommu_notifier);
-    vfio_unregister_notifier(vdev, VFIO_GROUP_NOTIFY,
-                 &matrix_mdev->group_notifier);
      vfio_ap_mdev_unset_kvm(matrix_mdev);

I'm not sure if this matters, but the vfio_ap_mdev_unset_kvm(matrix_mdev)
function uses the KVM pointer stored in matrix_mdev->kvm. I can't imagine
the KVM pointer stored in vdev->kvm being different than matrix_mdev->kvm,

With this patch matrix_mdev->kvm is set from the value in vdev->kvm during vfio_ap_mdev_set_kvm (basically, doing the work that the notifier was doing but instead of getting it from notifier data get it from the vfio_device)

but thought I should point it out. Previously, this function was called by the notifier handler which did not have access to the KVM pointer which is why it
was retrieved from matrix_mdev->kvm. Even if the vdev->kvm and
matrix_mdev->kvm did not match, we should probably go ahead and call
the unset function anyway to remove access to AP resources for the guest and
reset the queues.





[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