Re: [RFC PATCH 3/3] VFIO: Add new cmd for user space to get IOMMU fault info

[Alex Williamson <alex.williamson@redhat.com>]

On Sun, 19 Feb 2017 22:47:09 +0800
Lan Tianyu <tianyu.lan@intel.com> wrote:

> This patch is to introduce cmd VFIO_IOMMU_GET_FAULT_INFO cmd to return
> fault info reported by IOMMU driver.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 31 +++++++++++++++++++++++++++++++
>  include/uapi/linux/vfio.h       | 15 +++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index dc434a3..58e6689 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1657,6 +1657,37 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>  		mutex_unlock(&iommu->fault_lock);
>  
>  		return ret;
> +	} else if (cmd == VFIO_IOMMU_GET_FAULT_INFO) {
> +		struct vfio_iommu_type1_get_fault_info info;
> +		int fault_size = sizeof(struct vfio_iommu_fault_info);
> +		int ret;
> +
> +		minsz = offsetofend(struct vfio_iommu_type1_get_fault_info,
> +				    count);
> +		if (copy_from_user(&info, (void __user *)arg, minsz))
> +			return -EFAULT;
> +
> +		if (info.argsz < minsz)
> +			return -EINVAL;
> +
> +		mutex_lock(&iommu->fault_lock);
> +		info.count = iommu->fault_count;
> +
> +		if (info.argsz < sizeof(info) +
> +		    iommu->fault_count * fault_size)
> +			ret = -ENOSPC;
> +
> +		if (copy_to_user((void __user *)arg, &info, minsz))
> +			ret = -EFAULT;
> +
> +		if (!ret & !copy_to_user((void __user *)(arg + minsz),
> +		    iommu->fault_info, info.count * fault_size))
> +			iommu->fault_count = 0;
> +		else if (ret != ENOSPC)
> +			ret = -EFAULT;
> +
> +		mutex_unlock(&iommu->fault_lock);
> +		return ret;
>  	}
>  
>  	return -ENOTTY;
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index da359dd..e6fd86f 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -562,6 +562,11 @@ struct vfio_iommu_type1_set_fault_eventfd {
>  
>  #define VFIO_IOMMU_SET_FAULT_EVENTFD	_IO(VFIO_TYPE, VFIO_BASE + 17)
>  
> +/*
> + * VFIO_IOMMU_GET_FAULT_INFO		_IO(VFIO_TYPE, VFIO_BASE + 18)
> + *
> + * Return IOMMU fault info to userspace.
> + */
>  struct vfio_iommu_fault_info {
>  	__u64	addr;
>  	__u16   sid;
> @@ -569,6 +574,16 @@ struct vfio_iommu_fault_info {
>  	__u8	is_write:1;
>  };
>  
> +struct vfio_iommu_type1_get_fault_info {
> +	__u32	argsz;
> +	__u32   flags;
> +	__u32	count;
> +	struct vfio_iommu_fault_info fault_info[];
> +};

I wonder if adding a region to the container would be better for
reporting this.  It could at least act more like hardware reports the
errors, ie. head and tail pointers at known offsets with split
ownership between the kernel and user, possibly the ability to mmap
the region address space directly into QEMU memory.

> +
> +#define VFIO_IOMMU_GET_FAULT_INFO	_IO(VFIO_TYPE, VFIO_BASE + 18)
> +
> +
>  /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
>  
>  /*