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

[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@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;
> >   }