Messages in this thread | | | Date | Fri, 20 May 2022 16:59:05 -0400 | Subject | Re: [PATCH v3 1/1] vfio: remove VFIO_GROUP_NOTIFY_SET_KVM | From | Tony Krowiak <> |
| |
On 5/20/22 10:09 AM, Matthew Rosato wrote: > 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)
You are correct, the VFIO_GROUP_NOTIFY_SET_KVM notifier will get triggered when it is registered; however, you may have pointed out a flaw in the previous version of the code. I'm guessing this notifier is not triggered when it is unregistered, so unless the guest is terminated due to a non-zero return code from the open_device callback, it will have access to the AP queues. In hindsight, we probably should have registered the IOMMU notifier first.
You make a valid point about this being an edge case and I don't think it's critical, so feel free to keep it as-is.
My r-b still stands.
> >> >>> @@ -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; >>> } >
| |