[RFC PATCH 0/2] Connect request callback to mdev and vfio-ccw

[RFC PATCH 0/2] Connect request callback to mdev and vfio-ccw

[Eric Farman]

There is a situation where removing all the paths from a device
connected via mdev and vfio-ccw can cause some difficulty.
Using the "chchp -c 0 xx" command to all paths will cause the
device to be removed from the configuration, and any guest
filesystem that is relying on that device will encounter errors.
Interestingly, the last chchp command will actually fail to
return to a shell prompt, and subsequent commands (another
chchp to bring the paths back online, chzdev, etc.) will also
hang because of the outstanding chchp.

The last chchp command drives to vfio_ccw_sch_remove() for every
affected mediated device, and ultimately enters an infinite loop
in vfio_del_group_dev(). This loop is broken when the guest goes
away, which in this case doesn't occur until the guest is shutdown.
This drives vfio_ccw_mdev_release() and thus vfio_device_release()
to wake up the vfio_del_group_dev() thread.

There is also a callback mechanism called "request" to ask a
driver (and perhaps user) to release the device, but this is not
implemented for mdev. So this adds one to that point, and then
wire it to vfio-ccw to pass it along to userspace. This will
gracefully drive the unplug code, and everything behaves nicely.

Despite the testing that was occurring, this doesn't appear related
to the vfio-ccw channel path handling code. I can reproduce this with
an older kernel/QEMU, which makes sense because the above behavior is
driven from the subchannel event codepaths and not the chpid events.
Because of that, I didn't flag anything with a Fixes tag, since it's
seemingly been this way forever.

Eric Farman (2):
  vfio-mdev: Wire in a request handler for mdev parent
  vfio-ccw: Wire in the request callback

 drivers/s390/cio/vfio_ccw_ops.c     | 26 ++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_private.h |  4 ++++
 drivers/vfio/mdev/vfio_mdev.c       | 11 +++++++++++
 include/linux/mdev.h                |  4 ++++
 include/uapi/linux/vfio.h           |  1 +
 5 files changed, 46 insertions(+)

-- 
2.17.1

[RFC PATCH 1/2] vfio-mdev: Wire in a request handler for mdev parent

[Eric Farman]

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
+ *			@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 */
-- 
2.17.1

[RFC PATCH 2/2] vfio-ccw: Wire in the request callback

[Eric Farman]

The device is being unplugged, so pass the request to userspace to
ask for a graceful cleanup. This should free up the thread that
would otherwise loop waiting for the device to be fully released.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c     | 26 ++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_private.h |  4 ++++
 include/uapi/linux/vfio.h           |  1 +
 3 files changed, 31 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 8b3ed5b45277..68106be4ba7a 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -394,6 +394,7 @@ static int vfio_ccw_mdev_get_irq_info(struct vfio_irq_info *info)
 	switch (info->index) {
 	case VFIO_CCW_IO_IRQ_INDEX:
 	case VFIO_CCW_CRW_IRQ_INDEX:
+	case VFIO_CCW_REQ_IRQ_INDEX:
 		info->count = 1;
 		info->flags = VFIO_IRQ_INFO_EVENTFD;
 		break;
@@ -424,6 +425,9 @@ static int vfio_ccw_mdev_set_irqs(struct mdev_device *mdev,
 	case VFIO_CCW_CRW_IRQ_INDEX:
 		ctx = &private->crw_trigger;
 		break;
+	case VFIO_CCW_REQ_IRQ_INDEX:
+		ctx = &private->req_trigger;
+		break;
 	default:
 		return -EINVAL;
 	}
@@ -607,6 +611,27 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
 	}
 }
 
+/* Request removal of the device*/
+static void vfio_ccw_mdev_request(struct mdev_device *mdev, unsigned int count)
+{
+	struct vfio_ccw_private *private = dev_get_drvdata(mdev_parent_dev(mdev));
+
+	if (!private)
+		return;
+
+	if (private->req_trigger) {
+		if (!(count % 10))
+			dev_notice_ratelimited(mdev_dev(private->mdev),
+					       "Relaying device request to user (#%u)\n",
+					       count);
+
+		eventfd_signal(private->req_trigger, 1);
+	} else if (count == 0) {
+		dev_notice(mdev_dev(private->mdev),
+			   "No device request channel registered, blocked until released by user\n");
+	}
+}
+
 static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
 	.owner			= THIS_MODULE,
 	.supported_type_groups  = mdev_type_groups,
@@ -617,6 +642,7 @@ static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
 	.read			= vfio_ccw_mdev_read,
 	.write			= vfio_ccw_mdev_write,
 	.ioctl			= vfio_ccw_mdev_ioctl,
+	.request		= vfio_ccw_mdev_request,
 };
 
 int vfio_ccw_mdev_reg(struct subchannel *sch)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 8723156b29ea..b2c762eb42b9 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -84,7 +84,10 @@ struct vfio_ccw_crw {
  * @irb: irb info received from interrupt
  * @scsw: scsw info
  * @io_trigger: eventfd ctx for signaling userspace I/O results
+ * @crw_trigger: eventfd ctx for signaling userspace CRW information
+ * @req_trigger: eventfd ctx for signaling userspace to return device
  * @io_work: work for deferral process of I/O handling
+ * @crw_work: work for deferral process of CRW handling
  */
 struct vfio_ccw_private {
 	struct subchannel	*sch;
@@ -108,6 +111,7 @@ struct vfio_ccw_private {
 
 	struct eventfd_ctx	*io_trigger;
 	struct eventfd_ctx	*crw_trigger;
+	struct eventfd_ctx	*req_trigger;
 	struct work_struct	io_work;
 	struct work_struct	crw_work;
 } __aligned(8);
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 2f313a238a8f..d1812777139f 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -820,6 +820,7 @@ enum {
 enum {
 	VFIO_CCW_IO_IRQ_INDEX,
 	VFIO_CCW_CRW_IRQ_INDEX,
+	VFIO_CCW_REQ_IRQ_INDEX,
 	VFIO_CCW_NUM_IRQS
 };
 
-- 
2.17.1

Re: [RFC PATCH 1/2] vfio-mdev: Wire in a request handler for mdev parent

[Cornelia Huck]

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,

Re: [RFC PATCH 2/2] vfio-ccw: Wire in the request callback

[Cornelia Huck]

On Tue, 17 Nov 2020 04:21:39 +0100
Eric Farman <farman@linux.ibm.com> wrote:

> The device is being unplugged, so pass the request to userspace to
> ask for a graceful cleanup. This should free up the thread that
> would otherwise loop waiting for the device to be fully released.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_ops.c     | 26 ++++++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_private.h |  4 ++++
>  include/uapi/linux/vfio.h           |  1 +
>  3 files changed, 31 insertions(+)
> 

(...)

> @@ -607,6 +611,27 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
>  	}
>  }
>  
> +/* Request removal of the device*/
> +static void vfio_ccw_mdev_request(struct mdev_device *mdev, unsigned int count)
> +{
> +	struct vfio_ccw_private *private = dev_get_drvdata(mdev_parent_dev(mdev));
> +
> +	if (!private)
> +		return;
> +
> +	if (private->req_trigger) {
> +		if (!(count % 10))
> +			dev_notice_ratelimited(mdev_dev(private->mdev),
> +					       "Relaying device request to user (#%u)\n",
> +					       count);
> +
> +		eventfd_signal(private->req_trigger, 1);
> +	} else if (count == 0) {
> +		dev_notice(mdev_dev(private->mdev),
> +			   "No device request channel registered, blocked until released by user\n");
> +	}
> +}

This looks like the vfio-pci request handler, so probably good :)

Still have to read up on the QEMU side, but a LGTM for now.

Re: [RFC PATCH 2/2] vfio-ccw: Wire in the request callback

[Cornelia Huck]

On Thu, 19 Nov 2020 12:43:26 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 17 Nov 2020 04:21:39 +0100
> Eric Farman <farman@linux.ibm.com> wrote:
> 
> > The device is being unplugged, so pass the request to userspace to
> > ask for a graceful cleanup. This should free up the thread that
> > would otherwise loop waiting for the device to be fully released.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  drivers/s390/cio/vfio_ccw_ops.c     | 26 ++++++++++++++++++++++++++
> >  drivers/s390/cio/vfio_ccw_private.h |  4 ++++
> >  include/uapi/linux/vfio.h           |  1 +
> >  3 files changed, 31 insertions(+)
> >   
> 
> (...)
> 
> > @@ -607,6 +611,27 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device *mdev,
> >  	}
> >  }
> >  
> > +/* Request removal of the device*/
> > +static void vfio_ccw_mdev_request(struct mdev_device *mdev, unsigned int count)
> > +{
> > +	struct vfio_ccw_private *private = dev_get_drvdata(mdev_parent_dev(mdev));
> > +
> > +	if (!private)
> > +		return;
> > +
> > +	if (private->req_trigger) {
> > +		if (!(count % 10))
> > +			dev_notice_ratelimited(mdev_dev(private->mdev),
> > +					       "Relaying device request to user (#%u)\n",
> > +					       count);
> > +
> > +		eventfd_signal(private->req_trigger, 1);
> > +	} else if (count == 0) {
> > +		dev_notice(mdev_dev(private->mdev),
> > +			   "No device request channel registered, blocked until released by user\n");
> > +	}
> > +}  
> 
> This looks like the vfio-pci request handler, so probably good :)
> 
> Still have to read up on the QEMU side, but a LGTM for now.

And now that I've looked at the QEMU code:

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [RFC PATCH 1/2] vfio-mdev: Wire in a request handler for mdev parent

[Eric Farman]

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,
> 

Re: [RFC PATCH 1/2] vfio-mdev: Wire in a request handler for mdev parent

[Halil Pasic]

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 */

Re: [RFC PATCH 1/2] vfio-mdev: Wire in a request handler for mdev parent

[Halil Pasic]

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

Re: [RFC PATCH 1/2] vfio-mdev: Wire in a request handler for mdev parent

[Alex Williamson]

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,  

Re: [RFC PATCH 1/2] vfio-mdev: Wire in a request handler for mdev parent

[Eric Farman]

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,
> 

Re: [RFC PATCH 1/2] vfio-mdev: Wire in a request handler for mdev parent

[Cornelia Huck]

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
> 

Re: [RFC PATCH 1/2] vfio-mdev: Wire in a request handler for mdev parent

[Cornelia Huck]

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,  
> >   
> 

Re: [RFC PATCH 1/2] vfio-mdev: Wire in a request handler for mdev parent

[Tony Krowiak]

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