Messages in this thread | | | Date | Fri, 20 May 2022 10:09:58 -0400 | Subject | Re: [PATCH v3 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM | From | Matthew Rosato <> |
| |
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@linux.ibm.com> >> Reviewed-by: Kevin Tian <kevin.tian@intel.com> >> Reviewed-by: Christoph Hellwig <hch@lst.de> >> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >> --- >> 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; >> }
| |