Re: [RFC PATCH 1/3] VFIO: Add new cmd to receive eventfd from userspace to notify IOMMU fault event

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Feb 19, 2017 at 10:47:07PM +0800, Lan Tianyu wrote:
> This patch is to receive IOMMU fault eventfd and IOMMU fault event flag
> from userspace. VFIO should register IOMMU fault event handler according
> fault event flag which designates what kind of fault events need to be
> reported.  When VFIO get IOMMU fault event from IOMMU driver, it should
> notify userspace via received fd.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 45 +++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/vfio.h       | 15 ++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index b3cc33f..46674ea 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -38,6 +38,7 @@
>  #include <linux/workqueue.h>
>  #include <linux/mdev.h>
>  #include <linux/notifier.h>
> +#include <linux/eventfd.h>
>  
>  #define DRIVER_VERSION  "0.2"
>  #define DRIVER_AUTHOR   "Alex Williamson <alex.williamson@redhat.com>"
> @@ -61,6 +62,8 @@ struct vfio_iommu {
>  	struct mutex		lock;
>  	struct rb_root		dma_list;
>  	struct blocking_notifier_head notifier;
> +	struct eventfd_ctx	*iommu_fault_fd;
> +	struct mutex            fault_lock;
>  	bool			v2;
>  	bool			nesting;
>  };
> @@ -1452,6 +1455,7 @@ static void *vfio_iommu_type1_open(unsigned long arg)
>  	INIT_LIST_HEAD(&iommu->domain_list);
>  	iommu->dma_list = RB_ROOT;
>  	mutex_init(&iommu->lock);
> +	mutex_init(&iommu->fault_lock);
>  	BLOCKING_INIT_NOTIFIER_HEAD(&iommu->notifier);
>  
>  	return iommu;
> @@ -1582,6 +1586,47 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>  
>  		return copy_to_user((void __user *)arg, &unmap, minsz) ?
>  			-EFAULT : 0;
> +	} else if (cmd == VFIO_IOMMU_SET_FAULT_EVENTFD) {
> +		struct vfio_iommu_type1_set_fault_eventfd eventfd;
> +		int fd;
> +		int ret;
> +
> +		minsz = offsetofend(struct vfio_iommu_type1_set_fault_eventfd,
> +				    fd);
> +
> +		if (copy_from_user(&eventfd, (void __user *)arg, minsz))
> +			return -EFAULT;
> +
> +		if (eventfd.argsz < minsz)
> +			return -EINVAL;
> +
> +
> +		mutex_lock(&iommu->fault_lock);
> +
> +		fd = eventfd.fd;
> +		if (fd == -1) {
> +			if (iommu->iommu_fault_fd)
> +				eventfd_ctx_put(iommu->iommu_fault_fd);
> +			iommu->iommu_fault_fd = NULL;
> +			ret = 0;
> +		} else if (fd >= 0) {
> +			struct eventfd_ctx *ctx;
> +
> +			ctx = eventfd_ctx_fdget(fd);
> +			if (IS_ERR(ctx))
> +				return PTR_ERR(ctx);
> +
> +			if (ctx)
> +				eventfd_ctx_put(ctx);
> +
> +			iommu->iommu_fault_fd = ctx;
> +			ret = 0;
> +		} else
> +			ret = -EINVAL;
> +
> +		mutex_unlock(&iommu->fault_lock);
> +
> +		return ret;
>  	}
>  
>  	return -ENOTTY;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 519eff3..8616334 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -547,6 +547,21 @@ struct vfio_iommu_type1_dma_unmap {
>  #define VFIO_IOMMU_ENABLE	_IO(VFIO_TYPE, VFIO_BASE + 15)
>  #define VFIO_IOMMU_DISABLE	_IO(VFIO_TYPE, VFIO_BASE + 16)
>  
> +/*
> + * VFIO_IOMMU_SET_FAULT_EVENT_FD	_IO(VFIO_TYPE, VFIO_BASE + 17)
> + *
> + * Receive eventfd from userspace to notify fault event from IOMMU.
> + */
> +struct vfio_iommu_type1_set_fault_eventfd {
> +	__u32	argsz;
> +	__u32   flags;
> +/* What IOMMU Fault events should be reported. */
> +#define VFIO_IOMMU_UR_FAULT_WITHOUT_PASID (1 << 0)
> +	__s32	fd;			/* Eventfd from user space */
> +};
> +
> +#define VFIO_IOMMU_SET_FAULT_EVENTFD	_IO(VFIO_TYPE, VFIO_BASE + 17)
> +
>  /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */

How about simply doing

        VFIO_PCI_MSIX_IRQ_INDEX,
        VFIO_PCI_ERR_IRQ_INDEX,
        VFIO_PCI_REQ_IRQ_INDEX,
+       VFIO_PCI_IOMMU_UR_FAULT_WITHOUT_PASID_IRQ_INDEX,
        VFIO_PCI_NUM_IRQS


instead?

This way you don't need to write a whole new kernel/userspace
interface.

>  /*
> -- 
> 1.8.3.1