On Thu, 19 Nov 2020 15:04:08 -0500
Eric Farman <farman@xxxxxxxxxxxxx> wrote:
> On 11/19/20 11:27 AM, Alex Williamson wrote:
> > On Thu, 19 Nov 2020 12:30:26 +0100
> > Cornelia Huck <cohuck@xxxxxxxxxx> wrote:
> >
> >> On Tue, 17 Nov 2020 04:21:38 +0100
> >> Eric Farman <farman@xxxxxxxxxxxxx> wrote:
> >>
> >>> While performing some destructive tests with vfio-ccw, where the
> >>> paths to a device are forcible removed and thus the device itself
> >>> is unreachable, it is rather easy to end up in an endless loop in
> >>> vfio_del_group_dev() due to the lack of a request callback for the
> >>> associated device.
> >>>
> >>> In this example, one MDEV (77c) is used by a guest, while another
> >>> (77b) is not. The symptom is that the iommu is detached from the
> >>> mdev for 77b, but not 77c, until that guest is shutdown:
> >>>
> >>> [ 238.794867] vfio_ccw 0.0.077b: MDEV: Unregistering
> >>> [ 238.794996] vfio_mdev 11f2d2bc-4083-431d-a023-eff72715c4f0: Removing from iommu group 2
> >>> [ 238.795001] vfio_mdev 11f2d2bc-4083-431d-a023-eff72715c4f0: MDEV: detaching iommu
> >>> [ 238.795036] vfio_ccw 0.0.077c: MDEV: Unregistering
> >>> ...silence...
> >>>
> >>> Let's wire in the request call back to the mdev device, so that a hot
> >>> unplug can be (gracefully?) handled by the parent device at the time
> >>> the device is being removed.
> >>
> >> I think it makes a lot of sense to give the vendor driver a way to
> >> handle requests.
> >>
> >>>
> >>> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx>
> >>> ---
> >>> drivers/vfio/mdev/vfio_mdev.c | 11 +++++++++++
> >>> include/linux/mdev.h | 4 ++++
> >>> 2 files changed, 15 insertions(+)
> >>>
> >>> diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> >>> index 30964a4e0a28..2dd243f73945 100644
> >>> --- a/drivers/vfio/mdev/vfio_mdev.c
> >>> +++ b/drivers/vfio/mdev/vfio_mdev.c
> >>> @@ -98,6 +98,16 @@ static int vfio_mdev_mmap(void *device_data, struct vm_area_struct *vma)
> >>> return parent->ops->mmap(mdev, vma);
> >>> }
> >>>
> >>> +static void vfio_mdev_request(void *device_data, unsigned int count)
> >>> +{
> >>> + struct mdev_device *mdev = device_data;
> >>> + struct mdev_parent *parent = mdev->parent;
> >>> +
> >>> + if (unlikely(!parent->ops->request))
> >>
> >> Hm. Do you think that all drivers should implement a ->request()
> >> callback?
> >
> > It's considered optional for bus drivers in vfio-core, obviously
> > mdev-core could enforce presence of this callback, but then we'd break
> > existing out of tree drivers. We don't make guarantees to out of tree
> > drivers, but it feels a little petty. We could instead encourage such
> > support by printing a warning for drivers that register without a
> > request callback.
>
> Coincidentally, I'd considered adding a dev_warn_once() message in
> drivers/vfio/vfio.c:vfio_del_group_dev() when vfio_device->ops->request
> is NULL, and thus we're looping endlessly (and silently). But adding
> this patch and not patch 2 made things silent again, so I left it out.
> Putting a warning when the driver registers seems cool.
If we expect to run into problems without a callback, a warning seems
fine. Then we can also continue to use the (un)likely annotation
without it being weird.
>
> >
> > Minor nit, I tend to prefer:
> >
> > if (callback for thing)
> > call thing
> >
> > Rather than
> >
> > if (!callback for thing)
> > return;
> > call thing
>
> I like it too. I'll set it up that way in v2.
That also would be my preference, although existing code uses the
second pattern.
>
> >
> > Thanks,
> > Alex
> >
> >>
> >>> + return;
> >>> + parent->ops->request(mdev, count);
> >>> +}
> >>> +
> >>> static const struct vfio_device_ops vfio_mdev_dev_ops = {
> >>> .name = "vfio-mdev",
> >>> .open = vfio_mdev_open,
> >
>
