[PATCH 0/3] s390/vfio-ap: fix hang when mdev attached to guest is removed

[PATCH 0/3] s390/vfio-ap: fix hang when mdev attached to guest is removed

[Tony Krowiak]

When a user attempts to remove a vfio-ap mediated device attached to a
guest, the operation hangs until the mdev's fd is closed by the guest
(i.e., the hostdev is detached or the guest is shut down). This patch 
series provides kernel-side code that allows userspace to set up a 
communication channel that will allow the vfio_ap device driver to notify 
userspace when a request to release the mdev is received, so that userspace
can close the mdev fd and avoid the hang. The patch series provides the 
following:  

1. Introduces code to handle the VFIO_DEVICE_GET_IRQ_INFO and 
   VFIO_DEVICE_SET_IRQS ioctl calls to set the eventfd_ctx for signaling a
   device request to userspace. 

2. Wires up the VFIO bus driver callback to request a release of the mdev.
   When invoked, the vfio_ap device driver will use the eventfd_ctx set up
   in #1 to signal a request to userspace to release the mdev.


Note:
----
If a user subsequently attempts to restart the guest or re-attach the mdev,
the operation will fail with a message indicating the domain is already
active. This is a libvirt problem resolved with the following commit:

commit ebd004a03dbd ("security: do not remember/recall labels for VFIO 
MDEVs") 

Tony Krowiak (3):
  vfio: ap: realize the VFIO_DEVICE_GET_IRQ_INFO ioctl
  vfio: ap: realize the VFIO_DEVICE_SET_IRQS ioctl
  s390/vfio-ap: Wire in the vfio_device_ops request callback

 drivers/s390/crypto/vfio_ap_ops.c     | 134 +++++++++++++++++++++++++-
 drivers/s390/crypto/vfio_ap_private.h |   3 +
 include/uapi/linux/vfio.h             |   9 ++
 3 files changed, 145 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH 1/3] vfio: ap: realize the VFIO_DEVICE_GET_IRQ_INFO ioctl

[Tony Krowiak]

Realize the VFIO_DEVICE_GET_IRQ_INFO ioctl to retrieve the information for
the VFIO device request IRQ.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 30 +++++++++++++++++++++++++++++-
 include/uapi/linux/vfio.h         |  9 +++++++++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index cfbcb864ab63..35cd90eee937 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1750,7 +1750,32 @@ static int vfio_ap_mdev_get_device_info(unsigned long arg)
 
 	info.flags = VFIO_DEVICE_FLAGS_AP | VFIO_DEVICE_FLAGS_RESET;
 	info.num_regions = 0;
-	info.num_irqs = 0;
+	info.num_irqs = VFIO_AP_NUM_IRQS;
+
+	return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
+}
+
+static ssize_t vfio_ap_get_irq_info(unsigned long arg)
+{
+	unsigned long minsz;
+	struct vfio_irq_info info;
+
+	minsz = offsetofend(struct vfio_irq_info, count);
+
+	if (copy_from_user(&info, (void __user *)arg, minsz))
+		return -EFAULT;
+
+	if (info.argsz < minsz || info.index >= VFIO_AP_NUM_IRQS)
+		return -EINVAL;
+
+	switch (info.index) {
+	case VFIO_AP_REQ_IRQ_INDEX:
+		info.count = 1;
+		info.flags = VFIO_IRQ_INFO_EVENTFD;
+		break;
+	default:
+		return -EINVAL;
+	}
 
 	return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
 }
@@ -1770,6 +1795,9 @@ static ssize_t vfio_ap_mdev_ioctl(struct vfio_device *vdev,
 	case VFIO_DEVICE_RESET:
 		ret = vfio_ap_mdev_reset_queues(&matrix_mdev->qtable);
 		break;
+	case VFIO_DEVICE_GET_IRQ_INFO:
+			ret = vfio_ap_get_irq_info(arg);
+			break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 0552e8dcf0cb..b71276bd7f91 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -646,6 +646,15 @@ enum {
 	VFIO_CCW_NUM_IRQS
 };
 
+/*
+ * The vfio-ap bus driver makes use of the following IRQ index mapping.
+ * Unimplemented IRQ types return a count of zero.
+ */
+enum {
+	VFIO_AP_REQ_IRQ_INDEX,
+	VFIO_AP_NUM_IRQS
+};
+
 /**
  * VFIO_DEVICE_GET_PCI_HOT_RESET_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 12,
  *					      struct vfio_pci_hot_reset_info)
-- 
2.31.1

[PATCH 2/3] vfio: ap: realize the VFIO_DEVICE_SET_IRQS ioctl

[Tony Krowiak]

Realize the VFIO_DEVICE_SET_IRQS ioctl to set an eventfd file descriptor
to be used by the vfio_ap device driver to signal a device request to
userspace.

Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c     | 83 +++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_private.h |  3 +
 2 files changed, 86 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 35cd90eee937..44f159136891 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -716,6 +716,7 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
 	ret = vfio_register_emulated_iommu_dev(&matrix_mdev->vdev);
 	if (ret)
 		goto err_put_vdev;
+	matrix_mdev->req_trigger = NULL;
 	dev_set_drvdata(&mdev->dev, matrix_mdev);
 	mutex_lock(&matrix_dev->mdevs_lock);
 	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
@@ -1780,6 +1781,85 @@ static ssize_t vfio_ap_get_irq_info(unsigned long arg)
 	return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
 }
 
+static int vfio_ap_irq_set_init(struct vfio_irq_set *irq_set, unsigned long arg)
+{
+	int ret;
+	size_t data_size;
+	unsigned long minsz;
+
+	minsz = offsetofend(struct vfio_irq_set, count);
+
+	if (copy_from_user(irq_set, (void __user *)arg, minsz))
+		return -EFAULT;
+
+	ret = vfio_set_irqs_validate_and_prepare(irq_set, 1, VFIO_AP_NUM_IRQS,
+						 &data_size);
+	if (ret)
+		return ret;
+
+	if (!(irq_set->flags & VFIO_IRQ_SET_ACTION_TRIGGER))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int vfio_ap_set_request_irq(struct ap_matrix_mdev *matrix_mdev,
+				   unsigned long arg)
+{
+	s32 fd;
+	void __user *data;
+	unsigned long minsz;
+	struct eventfd_ctx *req_trigger;
+
+	minsz = offsetofend(struct vfio_irq_set, count);
+	data = (void __user *)(arg + minsz);
+
+	if (get_user(fd, (s32 __user *)data))
+		return -EFAULT;
+
+	if (fd == -1) {
+		if (matrix_mdev->req_trigger)
+			eventfd_ctx_put(matrix_mdev->req_trigger);
+		matrix_mdev->req_trigger = NULL;
+	} else if (fd >= 0) {
+		req_trigger = eventfd_ctx_fdget(fd);
+		if (IS_ERR(req_trigger))
+			return PTR_ERR(req_trigger);
+
+		if (matrix_mdev->req_trigger)
+			eventfd_ctx_put(matrix_mdev->req_trigger);
+
+		matrix_mdev->req_trigger = req_trigger;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int vfio_ap_set_irqs(struct ap_matrix_mdev *matrix_mdev,
+			    unsigned long arg)
+{
+	int ret;
+	struct vfio_irq_set irq_set;
+
+	ret = vfio_ap_irq_set_init(&irq_set, arg);
+	if (ret)
+		return ret;
+
+	switch (irq_set.flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
+	case VFIO_IRQ_SET_DATA_EVENTFD:
+		switch (irq_set.index) {
+		case VFIO_AP_REQ_IRQ_INDEX:
+			return vfio_ap_set_request_irq(matrix_mdev, arg);
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
 static ssize_t vfio_ap_mdev_ioctl(struct vfio_device *vdev,
 				    unsigned int cmd, unsigned long arg)
 {
@@ -1798,6 +1878,9 @@ static ssize_t vfio_ap_mdev_ioctl(struct vfio_device *vdev,
 	case VFIO_DEVICE_GET_IRQ_INFO:
 			ret = vfio_ap_get_irq_info(arg);
 			break;
+	case VFIO_DEVICE_SET_IRQS:
+		ret = vfio_ap_set_irqs(matrix_mdev, arg);
+		break;
 	default:
 		ret = -EOPNOTSUPP;
 		break;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 976a65f32e7d..4642bbdbd1b2 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -15,6 +15,7 @@
 #include <linux/types.h>
 #include <linux/mdev.h>
 #include <linux/delay.h>
+#include <linux/eventfd.h>
 #include <linux/mutex.h>
 #include <linux/kvm_host.h>
 #include <linux/vfio.h>
@@ -103,6 +104,7 @@ struct ap_queue_table {
  *		PQAP(AQIC) instruction.
  * @mdev:	the mediated device
  * @qtable:	table of queues (struct vfio_ap_queue) assigned to the mdev
+ * @req_trigger eventfd ctx for signaling userspace to return a device
  * @apm_add:	bitmap of APIDs added to the host's AP configuration
  * @aqm_add:	bitmap of APQIs added to the host's AP configuration
  * @adm_add:	bitmap of control domain numbers added to the host's AP
@@ -117,6 +119,7 @@ struct ap_matrix_mdev {
 	crypto_hook pqap_hook;
 	struct mdev_device *mdev;
 	struct ap_queue_table qtable;
+	struct eventfd_ctx *req_trigger;
 	DECLARE_BITMAP(apm_add, AP_DEVICES);
 	DECLARE_BITMAP(aqm_add, AP_DOMAINS);
 	DECLARE_BITMAP(adm_add, AP_DOMAINS);
-- 
2.31.1

[PATCH 3/3] s390/vfio-ap: Wire in the vfio_device_ops request callback

[Tony Krowiak]

The mdev device is being removed, 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: Tony Krowiak <akrowiak@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 44f159136891..a8f58e133e6e 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -1736,6 +1736,26 @@ static void vfio_ap_mdev_close_device(struct vfio_device *vdev)
 	vfio_ap_mdev_unset_kvm(matrix_mdev);
 }
 
+static void vfio_ap_mdev_request(struct vfio_device *vdev, unsigned int count)
+{
+	struct device *dev = vdev->dev;
+	struct ap_matrix_mdev *matrix_mdev;
+
+	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
+
+	if (matrix_mdev->req_trigger) {
+		if (!(count % 10))
+			dev_notice_ratelimited(dev,
+					       "Relaying device request to user (#%u)\n",
+					       count);
+
+		eventfd_signal(matrix_mdev->req_trigger, 1);
+	} else if (count == 0) {
+		dev_notice(dev,
+			   "No device request registered, blocked until released by user\n");
+	}
+}
+
 static int vfio_ap_mdev_get_device_info(unsigned long arg)
 {
 	unsigned long minsz;
@@ -1955,6 +1975,7 @@ static const struct vfio_device_ops vfio_ap_matrix_dev_ops = {
 	.bind_iommufd = vfio_iommufd_emulated_bind,
 	.unbind_iommufd = vfio_iommufd_emulated_unbind,
 	.attach_ioas = vfio_iommufd_emulated_attach_ioas,
+	.request = vfio_ap_mdev_request
 };
 
 static struct mdev_driver vfio_ap_matrix_driver = {
-- 
2.31.1

Re: [PATCH 1/3] vfio: ap: realize the VFIO_DEVICE_GET_IRQ_INFO ioctl

[Cédric Le Goater]

On 5/31/23 00:35, Tony Krowiak wrote:
> Realize the VFIO_DEVICE_GET_IRQ_INFO ioctl to retrieve the information for
> the VFIO device request IRQ.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 30 +++++++++++++++++++++++++++++-
>   include/uapi/linux/vfio.h         |  9 +++++++++
>   2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index cfbcb864ab63..35cd90eee937 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1750,7 +1750,32 @@ static int vfio_ap_mdev_get_device_info(unsigned long arg)
>   
>   	info.flags = VFIO_DEVICE_FLAGS_AP | VFIO_DEVICE_FLAGS_RESET;
>   	info.num_regions = 0;
> -	info.num_irqs = 0;
> +	info.num_irqs = VFIO_AP_NUM_IRQS;
> +
> +	return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
> +}
> +
> +static ssize_t vfio_ap_get_irq_info(unsigned long arg)
> +{
> +	unsigned long minsz;
> +	struct vfio_irq_info info;
> +
> +	minsz = offsetofend(struct vfio_irq_info, count);
> +
> +	if (copy_from_user(&info, (void __user *)arg, minsz))
> +		return -EFAULT;
> +
> +	if (info.argsz < minsz || info.index >= VFIO_AP_NUM_IRQS)
> +		return -EINVAL;
> +
> +	switch (info.index) {
> +	case VFIO_AP_REQ_IRQ_INDEX:
> +		info.count = 1;
> +		info.flags = VFIO_IRQ_INFO_EVENTFD;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
>   
>   	return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
>   }
> @@ -1770,6 +1795,9 @@ static ssize_t vfio_ap_mdev_ioctl(struct vfio_device *vdev,
>   	case VFIO_DEVICE_RESET:
>   		ret = vfio_ap_mdev_reset_queues(&matrix_mdev->qtable);
>   		break;
> +	case VFIO_DEVICE_GET_IRQ_INFO:
> +			ret = vfio_ap_get_irq_info(arg);
> +			break;
>   	default:
>   		ret = -EOPNOTSUPP;
>   		break;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 0552e8dcf0cb..b71276bd7f91 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -646,6 +646,15 @@ enum {
>   	VFIO_CCW_NUM_IRQS
>   };
>   
> +/*
> + * The vfio-ap bus driver makes use of the following IRQ index mapping.
> + * Unimplemented IRQ types return a count of zero.
> + */
> +enum {
> +	VFIO_AP_REQ_IRQ_INDEX,
> +	VFIO_AP_NUM_IRQS
> +};
> +
>   /**
>    * VFIO_DEVICE_GET_PCI_HOT_RESET_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 12,
>    *					      struct vfio_pci_hot_reset_info)

Re: [PATCH 2/3] vfio: ap: realize the VFIO_DEVICE_SET_IRQS ioctl

[Cédric Le Goater]

On 5/31/23 00:35, Tony Krowiak wrote:
> Realize the VFIO_DEVICE_SET_IRQS ioctl to set an eventfd file descriptor
> to be used by the vfio_ap device driver to signal a device request to
> userspace.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   drivers/s390/crypto/vfio_ap_ops.c     | 83 +++++++++++++++++++++++++++
>   drivers/s390/crypto/vfio_ap_private.h |  3 +
>   2 files changed, 86 insertions(+)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 35cd90eee937..44f159136891 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -716,6 +716,7 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
>   	ret = vfio_register_emulated_iommu_dev(&matrix_mdev->vdev);
>   	if (ret)
>   		goto err_put_vdev;
> +	matrix_mdev->req_trigger = NULL;
>   	dev_set_drvdata(&mdev->dev, matrix_mdev);
>   	mutex_lock(&matrix_dev->mdevs_lock);
>   	list_add(&matrix_mdev->node, &matrix_dev->mdev_list);
> @@ -1780,6 +1781,85 @@ static ssize_t vfio_ap_get_irq_info(unsigned long arg)
>   	return copy_to_user((void __user *)arg, &info, minsz) ? -EFAULT : 0;
>   }
>   
> +static int vfio_ap_irq_set_init(struct vfio_irq_set *irq_set, unsigned long arg)
> +{
> +	int ret;
> +	size_t data_size;
> +	unsigned long minsz;
> +
> +	minsz = offsetofend(struct vfio_irq_set, count);
> +
> +	if (copy_from_user(irq_set, (void __user *)arg, minsz))
> +		return -EFAULT;
> +
> +	ret = vfio_set_irqs_validate_and_prepare(irq_set, 1, VFIO_AP_NUM_IRQS,
> +						 &data_size);
> +	if (ret)
> +		return ret;
> +
> +	if (!(irq_set->flags & VFIO_IRQ_SET_ACTION_TRIGGER))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int vfio_ap_set_request_irq(struct ap_matrix_mdev *matrix_mdev,
> +				   unsigned long arg)
> +{
> +	s32 fd;
> +	void __user *data;
> +	unsigned long minsz;
> +	struct eventfd_ctx *req_trigger;
> +
> +	minsz = offsetofend(struct vfio_irq_set, count);
> +	data = (void __user *)(arg + minsz);
> +
> +	if (get_user(fd, (s32 __user *)data))
> +		return -EFAULT;
> +
> +	if (fd == -1) {
> +		if (matrix_mdev->req_trigger)
> +			eventfd_ctx_put(matrix_mdev->req_trigger);
> +		matrix_mdev->req_trigger = NULL;
> +	} else if (fd >= 0) {
> +		req_trigger = eventfd_ctx_fdget(fd);
> +		if (IS_ERR(req_trigger))
> +			return PTR_ERR(req_trigger);
> +
> +		if (matrix_mdev->req_trigger)
> +			eventfd_ctx_put(matrix_mdev->req_trigger);
> +
> +		matrix_mdev->req_trigger = req_trigger;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vfio_ap_set_irqs(struct ap_matrix_mdev *matrix_mdev,
> +			    unsigned long arg)
> +{
> +	int ret;
> +	struct vfio_irq_set irq_set;
> +
> +	ret = vfio_ap_irq_set_init(&irq_set, arg);
> +	if (ret)
> +		return ret;
> +
> +	switch (irq_set.flags & VFIO_IRQ_SET_DATA_TYPE_MASK) {
> +	case VFIO_IRQ_SET_DATA_EVENTFD:
> +		switch (irq_set.index) {
> +		case VFIO_AP_REQ_IRQ_INDEX:
> +			return vfio_ap_set_request_irq(matrix_mdev, arg);
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>   static ssize_t vfio_ap_mdev_ioctl(struct vfio_device *vdev,
>   				    unsigned int cmd, unsigned long arg)
>   {
> @@ -1798,6 +1878,9 @@ static ssize_t vfio_ap_mdev_ioctl(struct vfio_device *vdev,
>   	case VFIO_DEVICE_GET_IRQ_INFO:
>   			ret = vfio_ap_get_irq_info(arg);
>   			break;
> +	case VFIO_DEVICE_SET_IRQS:
> +		ret = vfio_ap_set_irqs(matrix_mdev, arg);
> +		break;
>   	default:
>   		ret = -EOPNOTSUPP;
>   		break;
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 976a65f32e7d..4642bbdbd1b2 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -15,6 +15,7 @@
>   #include <linux/types.h>
>   #include <linux/mdev.h>
>   #include <linux/delay.h>
> +#include <linux/eventfd.h>
>   #include <linux/mutex.h>
>   #include <linux/kvm_host.h>
>   #include <linux/vfio.h>
> @@ -103,6 +104,7 @@ struct ap_queue_table {
>    *		PQAP(AQIC) instruction.
>    * @mdev:	the mediated device
>    * @qtable:	table of queues (struct vfio_ap_queue) assigned to the mdev
> + * @req_trigger eventfd ctx for signaling userspace to return a device
>    * @apm_add:	bitmap of APIDs added to the host's AP configuration
>    * @aqm_add:	bitmap of APQIs added to the host's AP configuration
>    * @adm_add:	bitmap of control domain numbers added to the host's AP
> @@ -117,6 +119,7 @@ struct ap_matrix_mdev {
>   	crypto_hook pqap_hook;
>   	struct mdev_device *mdev;
>   	struct ap_queue_table qtable;
> +	struct eventfd_ctx *req_trigger;
>   	DECLARE_BITMAP(apm_add, AP_DEVICES);
>   	DECLARE_BITMAP(aqm_add, AP_DOMAINS);
>   	DECLARE_BITMAP(adm_add, AP_DOMAINS);

Re: [PATCH 3/3] s390/vfio-ap: Wire in the vfio_device_ops request callback

[Cédric Le Goater]

On 5/31/23 00:35, Tony Krowiak wrote:
> The mdev device is being removed, 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: Tony Krowiak <akrowiak@linux.ibm.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44f159136891..a8f58e133e6e 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -1736,6 +1736,26 @@ static void vfio_ap_mdev_close_device(struct vfio_device *vdev)
>   	vfio_ap_mdev_unset_kvm(matrix_mdev);
>   }
>   
> +static void vfio_ap_mdev_request(struct vfio_device *vdev, unsigned int count)
> +{
> +	struct device *dev = vdev->dev;
> +	struct ap_matrix_mdev *matrix_mdev;
> +
> +	matrix_mdev = container_of(vdev, struct ap_matrix_mdev, vdev);
> +
> +	if (matrix_mdev->req_trigger) {
> +		if (!(count % 10))
> +			dev_notice_ratelimited(dev,
> +					       "Relaying device request to user (#%u)\n",
> +					       count);
> +
> +		eventfd_signal(matrix_mdev->req_trigger, 1);
> +	} else if (count == 0) {
> +		dev_notice(dev,
> +			   "No device request registered, blocked until released by user\n");
> +	}
> +}
> +
>   static int vfio_ap_mdev_get_device_info(unsigned long arg)
>   {
>   	unsigned long minsz;
> @@ -1955,6 +1975,7 @@ static const struct vfio_device_ops vfio_ap_matrix_dev_ops = {
>   	.bind_iommufd = vfio_iommufd_emulated_bind,
>   	.unbind_iommufd = vfio_iommufd_emulated_unbind,
>   	.attach_ioas = vfio_iommufd_emulated_attach_ioas,
> +	.request = vfio_ap_mdev_request
>   };
>   
>   static struct mdev_driver vfio_ap_matrix_driver = {

Re: [PATCH 1/3] vfio: ap: realize the VFIO_DEVICE_GET_IRQ_INFO ioctl

[Anthony Krowiak]

On 5/31/23 8:54 AM, Cédric Le Goater wrote:
> Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thank you for the review.

Re: [PATCH 1/3] vfio: ap: realize the VFIO_DEVICE_GET_IRQ_INFO ioctl

[Cédric Le Goater]

On 5/31/23 15:05, Anthony Krowiak wrote:
> 
> 
> On 5/31/23 8:54 AM, Cédric Le Goater wrote:
>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> 
> Thank you for the review.
> 

I also ran a few tests with the QEMU part on guests with passthrough
crypto devices. This is probably a v6.5 candidate.

C.

Re: [PATCH 0/3] s390/vfio-ap: fix hang when mdev attached to guest is removed

[Matthew Rosato]

On 5/30/23 6:35 PM, Tony Krowiak wrote:
> When a user attempts to remove a vfio-ap mediated device attached to a
> guest, the operation hangs until the mdev's fd is closed by the guest
> (i.e., the hostdev is detached or the guest is shut down). This patch 
> series provides kernel-side code that allows userspace to set up a 
> communication channel that will allow the vfio_ap device driver to notify 
> userspace when a request to release the mdev is received, so that userspace
> can close the mdev fd and avoid the hang. The patch series provides the 
> following:  
> 
> 1. Introduces code to handle the VFIO_DEVICE_GET_IRQ_INFO and 
>    VFIO_DEVICE_SET_IRQS ioctl calls to set the eventfd_ctx for signaling a
>    device request to userspace. 
> 
> 2. Wires up the VFIO bus driver callback to request a release of the mdev.
>    When invoked, the vfio_ap device driver will use the eventfd_ctx set up
>    in #1 to signal a request to userspace to release the mdev.
> 
> 
> Note:
> ----
> If a user subsequently attempts to restart the guest or re-attach the mdev,
> the operation will fail with a message indicating the domain is already
> active. This is a libvirt problem resolved with the following commit:
> 
> commit ebd004a03dbd ("security: do not remember/recall labels for VFIO 
> MDEVs") 

For the series: 

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>


I also did some testing using the companion qemu series at
https://lore.kernel.org/qemu-devel/20230530225544.280031-1-akrowiak@linux.ibm.com

Before kernel+qemu changes:
1. mdevctl start -u <uuid>, where <uuid> references a vfio-ap mdev
2. start a qemu guest with <uuid> attached
3. mdvectl stop -u <uuid>
4. -mdevctl will now hang indefinitely; the mdev remains in-use by the guest-
Note: detaching the device or powering off the guest will allow the mdevctl command to complete.

After kernel+qemu changes:
1. mdevctl start -u <uuid>, where <uuid> references a vfio-ap mdev
2. start a qemu guest with <uuid> attached
3. mdvectl stop -u <uuid>
4. -device is detached from the guest and stopped-
5. Using a libvirt that includes ebd004a03dbd I also verified that the mdev can be started again and re-attached to the running guest without error.


> 
> Tony Krowiak (3):
>   vfio: ap: realize the VFIO_DEVICE_GET_IRQ_INFO ioctl
>   vfio: ap: realize the VFIO_DEVICE_SET_IRQS ioctl
>   s390/vfio-ap: Wire in the vfio_device_ops request callback
> 
>  drivers/s390/crypto/vfio_ap_ops.c     | 134 +++++++++++++++++++++++++-
>  drivers/s390/crypto/vfio_ap_private.h |   3 +
>  include/uapi/linux/vfio.h             |   9 ++
>  3 files changed, 145 insertions(+), 1 deletion(-)
> 

Re: [PATCH 0/3] s390/vfio-ap: fix hang when mdev attached to guest is removed

[Matthew Rosato]

On 5/30/23 6:35 PM, Tony Krowiak wrote:
> When a user attempts to remove a vfio-ap mediated device attached to a
> guest, the operation hangs until the mdev's fd is closed by the guest
> (i.e., the hostdev is detached or the guest is shut down). This patch 
> series provides kernel-side code that allows userspace to set up a 
> communication channel that will allow the vfio_ap device driver to notify 
> userspace when a request to release the mdev is received, so that userspace
> can close the mdev fd and avoid the hang. The patch series provides the 
> following:  
> 
> 1. Introduces code to handle the VFIO_DEVICE_GET_IRQ_INFO and 
>    VFIO_DEVICE_SET_IRQS ioctl calls to set the eventfd_ctx for signaling a
>    device request to userspace. 
> 
> 2. Wires up the VFIO bus driver callback to request a release of the mdev.
>    When invoked, the vfio_ap device driver will use the eventfd_ctx set up
>    in #1 to signal a request to userspace to release the mdev.
> 

As to how this series eventually reaches master...  It touches both s390 and vfio.  

@Alex/@s390 maintainers -- I suggest it go through s390 given the diffstat, it's almost completely in s390 drivers code.  However there is a uapi hit to vfio.h (in patch 1) that should get at least an ACK from Alex beforehand.

Re: [PATCH 0/3] s390/vfio-ap: fix hang when mdev attached to guest is removed

[Anthony Krowiak]

On 5/31/23 10:48 AM, Matthew Rosato wrote:
> On 5/30/23 6:35 PM, Tony Krowiak wrote:
>> When a user attempts to remove a vfio-ap mediated device attached to a
>> guest, the operation hangs until the mdev's fd is closed by the guest
>> (i.e., the hostdev is detached or the guest is shut down). This patch
>> series provides kernel-side code that allows userspace to set up a
>> communication channel that will allow the vfio_ap device driver to notify
>> userspace when a request to release the mdev is received, so that userspace
>> can close the mdev fd and avoid the hang. The patch series provides the
>> following:
>>
>> 1. Introduces code to handle the VFIO_DEVICE_GET_IRQ_INFO and
>>     VFIO_DEVICE_SET_IRQS ioctl calls to set the eventfd_ctx for signaling a
>>     device request to userspace.
>>
>> 2. Wires up the VFIO bus driver callback to request a release of the mdev.
>>     When invoked, the vfio_ap device driver will use the eventfd_ctx set up
>>     in #1 to signal a request to userspace to release the mdev.
>>
>>
>> Note:
>> ----
>> If a user subsequently attempts to restart the guest or re-attach the mdev,
>> the operation will fail with a message indicating the domain is already
>> active. This is a libvirt problem resolved with the following commit:
>>
>> commit ebd004a03dbd ("security: do not remember/recall labels for VFIO
>> MDEVs")
> 
> For the series:
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Thanks for the review.

> 
> 
> I also did some testing using the companion qemu series at
> https://lore.kernel.org/qemu-devel/20230530225544.280031-1-akrowiak@linux.ibm.com

Shall I credit you with Tested-by also?

> 
> Before kernel+qemu changes:
> 1. mdevctl start -u <uuid>, where <uuid> references a vfio-ap mdev
> 2. start a qemu guest with <uuid> attached
> 3. mdvectl stop -u <uuid>
> 4. -mdevctl will now hang indefinitely; the mdev remains in-use by the guest-
> Note: detaching the device or powering off the guest will allow the mdevctl command to complete.
> 
> After kernel+qemu changes:
> 1. mdevctl start -u <uuid>, where <uuid> references a vfio-ap mdev
> 2. start a qemu guest with <uuid> attached
> 3. mdvectl stop -u <uuid>
> 4. -device is detached from the guest and stopped-
> 5. Using a libvirt that includes ebd004a03dbd I also verified that the mdev can be started again and re-attached to the running guest without error.
> 
> 
>>
>> Tony Krowiak (3):
>>    vfio: ap: realize the VFIO_DEVICE_GET_IRQ_INFO ioctl
>>    vfio: ap: realize the VFIO_DEVICE_SET_IRQS ioctl
>>    s390/vfio-ap: Wire in the vfio_device_ops request callback
>>
>>   drivers/s390/crypto/vfio_ap_ops.c     | 134 +++++++++++++++++++++++++-
>>   drivers/s390/crypto/vfio_ap_private.h |   3 +
>>   include/uapi/linux/vfio.h             |   9 ++
>>   3 files changed, 145 insertions(+), 1 deletion(-)
>>
> 

Re: [PATCH 0/3] s390/vfio-ap: fix hang when mdev attached to guest is removed

[Matthew Rosato]

On 6/1/23 8:15 AM, Anthony Krowiak wrote:
> 
> 
> On 5/31/23 10:48 AM, Matthew Rosato wrote:

>> I also did some testing using the companion qemu series at
>> https://lore.kernel.org/qemu-devel/20230530225544.280031-1-akrowiak@linux.ibm.com
> 
> Shall I credit you with Tested-by also?
> 

Sure.

Thanks,
Matt

Re: [PATCH 1/3] vfio: ap: realize the VFIO_DEVICE_GET_IRQ_INFO ioctl

[Alex Williamson]

On Tue, 30 May 2023 18:35:36 -0400
Tony Krowiak <akrowiak@linux.ibm.com> wrote:

> Realize the VFIO_DEVICE_GET_IRQ_INFO ioctl to retrieve the information for
> the VFIO device request IRQ.
> 
> Signed-off-by: Tony Krowiak <akrowiak@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 30 +++++++++++++++++++++++++++++-
>  include/uapi/linux/vfio.h         |  9 +++++++++
>  2 files changed, 38 insertions(+), 1 deletion(-)

Acked-by: Alex Williamson <alex.williamson@redhat.com>

Re: [PATCH 0/3] s390/vfio-ap: fix hang when mdev attached to guest is removed

[Alex Williamson]

On Wed, 31 May 2023 10:51:54 -0400
Matthew Rosato <mjrosato@linux.ibm.com> wrote:

> On 5/30/23 6:35 PM, Tony Krowiak wrote:
> > When a user attempts to remove a vfio-ap mediated device attached to a
> > guest, the operation hangs until the mdev's fd is closed by the guest
> > (i.e., the hostdev is detached or the guest is shut down). This patch 
> > series provides kernel-side code that allows userspace to set up a 
> > communication channel that will allow the vfio_ap device driver to notify 
> > userspace when a request to release the mdev is received, so that userspace
> > can close the mdev fd and avoid the hang. The patch series provides the 
> > following:  
> > 
> > 1. Introduces code to handle the VFIO_DEVICE_GET_IRQ_INFO and 
> >    VFIO_DEVICE_SET_IRQS ioctl calls to set the eventfd_ctx for signaling a
> >    device request to userspace. 
> > 
> > 2. Wires up the VFIO bus driver callback to request a release of the mdev.
> >    When invoked, the vfio_ap device driver will use the eventfd_ctx set up
> >    in #1 to signal a request to userspace to release the mdev.
> >   
> 
> As to how this series eventually reaches master...  It touches both s390 and vfio.  
> 
> @Alex/@s390 maintainers -- I suggest it go through s390 given the
> diffstat, it's almost completely in s390 drivers code.  However there
> is a uapi hit to vfio.h (in patch 1) that should get at least an ACK
> from Alex beforehand.

Ack'd, I'll expect this to go through the s390 tree.  Thanks,

Alex

Re: [PATCH 0/3] s390/vfio-ap: fix hang when mdev attached to guest is removed

[Alexander Gordeev]

On Thu, Jun 01, 2023 at 02:47:22PM -0600, Alex Williamson wrote:
...
> > As to how this series eventually reaches master...  It touches both s390 and vfio.  
> > 
> > @Alex/@s390 maintainers -- I suggest it go through s390 given the
> > diffstat, it's almost completely in s390 drivers code.  However there
> > is a uapi hit to vfio.h (in patch 1) that should get at least an ACK
> > from Alex beforehand.
> 
> Ack'd, I'll expect this to go through the s390 tree.  Thanks,

Applied, thanks!

> Alex