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.
Minor nit, I tend to prefer:
if (callback for thing)
call thing
Rather than
if (!callback for thing)
return;
call thing
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,
