| Message ID | 20201117032139.50988-2-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/2] vfio-mdev: Wire in a request handler for mdev parent | expand |
On Tue, 17 Nov 2020 04:21:38 +0100 Eric Farman <farman@linux.ibm.com> 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@linux.ibm.com> > --- > 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? > + return; > + parent->ops->request(mdev, count); > +} > + > static const struct vfio_device_ops vfio_mdev_dev_ops = { > .name = "vfio-mdev", > .open = vfio_mdev_open,
On 11/19/20 6:30 AM, Cornelia Huck wrote: > On Tue, 17 Nov 2020 04:21:38 +0100 > Eric Farman <farman@linux.ibm.com> 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@linux.ibm.com> >> --- >> 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? Hrm... Good question. Don't know the profile of other drivers; so maybe the unlikely() is unecessary. But probably need to check that parent is not NULL also, in case things are really in the weeds. > >> + return; >> + parent->ops->request(mdev, count); >> +} >> + >> static const struct vfio_device_ops vfio_mdev_dev_ops = { >> .name = "vfio-mdev", >> .open = vfio_mdev_open, >
On Tue, 17 Nov 2020 04:21:38 +0100 Eric Farman <farman@linux.ibm.com> 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. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > 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)) > + return; > + parent->ops->request(mdev, count); > +} > + > static const struct vfio_device_ops vfio_mdev_dev_ops = { > .name = "vfio-mdev", > .open = vfio_mdev_open, > @@ -106,6 +116,7 @@ static const struct vfio_device_ops vfio_mdev_dev_ops = { > .read = vfio_mdev_read, > .write = vfio_mdev_write, > .mmap = vfio_mdev_mmap, > + .request = vfio_mdev_request, > }; > > static int vfio_mdev_probe(struct device *dev) > diff --git a/include/linux/mdev.h b/include/linux/mdev.h > index 0ce30ca78db0..0ed88be1f4bb 100644 > --- a/include/linux/mdev.h > +++ b/include/linux/mdev.h > @@ -72,6 +72,9 @@ struct device *mdev_get_iommu_device(struct device *dev); > * @mmap: mmap callback > * @mdev: mediated device structure > * @vma: vma structure > + * @request: request callback In include/linux/vfio.h it is documented like * @request: Request for the bus driver to release the device Can we add 'to release' here as well? IMHO, when one requests, one needs to say what is requested. So I would expect a function called request() to have a parameter (direct or indirect) that expresses, what is requested. But this does not seem to be the case here. Or did I miss it? Well it's called request() and not request_removal() in vfio, so I believe it's only consistent to keep calling it request(). But I do think we should at least document what is actually requested. Otherwise LGTM! > + * @mdev: mediated device structure > + * @count: request sequence number > * Parent device that support mediated device should be registered with mdev > * module with mdev_parent_ops structure. > **/ > @@ -92,6 +95,7 @@ struct mdev_parent_ops { > long (*ioctl)(struct mdev_device *mdev, unsigned int cmd, > unsigned long arg); > int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma); > + void (*request)(struct mdev_device *mdev, unsigned int count); > }; > > /* interface for exporting mdev supported type attributes */
On Thu, 19 Nov 2020 12:30:26 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > > +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? @Tony: What do you think, does vfio_ap need something like this? BTW how is this supposed to work in a setup where the one parent has may children (like vfio_ap or the gpu slice and dice usecases). After giving this some thought I'm under the impression, I don't get the full picture yet. Regards, Halil
On Thu, 19 Nov 2020 12:30:26 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Tue, 17 Nov 2020 04:21:38 +0100 > Eric Farman <farman@linux.ibm.com> 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@linux.ibm.com> > > --- > > 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,
On 11/19/20 11:27 AM, Alex Williamson wrote: > On Thu, 19 Nov 2020 12:30:26 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Tue, 17 Nov 2020 04:21:38 +0100 >> Eric Farman <farman@linux.ibm.com> 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@linux.ibm.com> >>> --- >>> 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. > > 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. > > 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, >
On Thu, 19 Nov 2020 16:56:11 +0100 Halil Pasic <pasic@linux.ibm.com> wrote: > On Thu, 19 Nov 2020 12:30:26 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > > > > +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? > > @Tony: What do you think, does vfio_ap need something like this? > > BTW how is this supposed to work in a setup where the one parent > has may children (like vfio_ap or the gpu slice and dice usecases). I'd think that the driver would either keep some kind of reference counting (do something when the last child is gone), notifies all other children as well, or leaves the decision to userspace. Probably highly depends on the driver. > > After giving this some thought I'm under the impression, I don't > get the full picture yet. > > Regards, > Halil >
On Thu, 19 Nov 2020 15:04:08 -0500 Eric Farman <farman@linux.ibm.com> wrote: > On 11/19/20 11:27 AM, Alex Williamson wrote: > > On Thu, 19 Nov 2020 12:30:26 +0100 > > Cornelia Huck <cohuck@redhat.com> wrote: > > > >> On Tue, 17 Nov 2020 04:21:38 +0100 > >> Eric Farman <farman@linux.ibm.com> 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@linux.ibm.com> > >>> --- > >>> 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, > > >
On 11/19/20 10:56 AM, Halil Pasic wrote: > On Thu, 19 Nov 2020 12:30:26 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > >>> +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? > @Tony: What do you think, does vfio_ap need something like this? > > BTW how is this supposed to work in a setup where the one parent > has may children (like vfio_ap or the gpu slice and dice usecases). > > After giving this some thought I'm under the impression, I don't > get the full picture yet. Eric Farman touched base with me on Friday to discuss this, but I was on my way out the door for an appointment. He is off this week; so, the bottom line for me is that I don't have even a piece of the picture here and therefore don't have enough info to speculate on whether vfio_ap needs something like this. > > Regards, > Halil
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)) + return; + parent->ops->request(mdev, count); +} + static const struct vfio_device_ops vfio_mdev_dev_ops = { .name = "vfio-mdev", .open = vfio_mdev_open, @@ -106,6 +116,7 @@ static const struct vfio_device_ops vfio_mdev_dev_ops = { .read = vfio_mdev_read, .write = vfio_mdev_write, .mmap = vfio_mdev_mmap, + .request = vfio_mdev_request, }; static int vfio_mdev_probe(struct device *dev) diff --git a/include/linux/mdev.h b/include/linux/mdev.h index 0ce30ca78db0..0ed88be1f4bb 100644 --- a/include/linux/mdev.h +++ b/include/linux/mdev.h @@ -72,6 +72,9 @@ struct device *mdev_get_iommu_device(struct device *dev); * @mmap: mmap callback * @mdev: mediated device structure * @vma: vma structure + * @request: request callback + * @mdev: mediated device structure + * @count: request sequence number * Parent device that support mediated device should be registered with mdev * module with mdev_parent_ops structure. **/ @@ -92,6 +95,7 @@ struct mdev_parent_ops { long (*ioctl)(struct mdev_device *mdev, unsigned int cmd, unsigned long arg); int (*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma); + void (*request)(struct mdev_device *mdev, unsigned int count); }; /* interface for exporting mdev supported type attributes */
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. Signed-off-by: Eric Farman <farman@linux.ibm.com> --- drivers/vfio/mdev/vfio_mdev.c | 11 +++++++++++ include/linux/mdev.h | 4 ++++ 2 files changed, 15 insertions(+)