Re: [RFC PATCH 2/3] VFIO: Add IOMMU fault notifier callback

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

On Sun, Feb 19, 2017 at 10:47:08PM +0800, Lan Tianyu wrote:
> This patch is to add callback to handle fault event reported by
> IOMMU driver. Callback stores fault into an array and notify userspace
> via eventfd to read fault info.
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 30 ++++++++++++++++++++++++++++++
>  include/linux/iommu.h           |  7 +++++++
>  include/uapi/linux/vfio.h       |  7 +++++++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 46674ea..dc434a3 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -56,6 +56,8 @@
>  MODULE_PARM_DESC(disable_hugepages,
>  		 "Disable VFIO IOMMU support for IOMMU hugepages.");
>  
> +#define NR_IOMMU_FAULT_INFO	10
> +
>  struct vfio_iommu {
>  	struct list_head	domain_list;
>  	struct vfio_domain	*external_domain; /* domain for external user */
> @@ -64,6 +66,9 @@ struct vfio_iommu {
>  	struct blocking_notifier_head notifier;
>  	struct eventfd_ctx	*iommu_fault_fd;
>  	struct mutex            fault_lock;
> +	struct vfio_iommu_fault_info fault_info[NR_IOMMU_FAULT_INFO];

What if you run out of this space? Userspace will not
see any more faults. What will cause progress to happen?


> +	struct blocking_notifier_head iommu_fault_notifier;
> +	u8			fault_count;
>  	bool			v2;
>  	bool			nesting;
>  };
> @@ -1456,6 +1461,7 @@ static void *vfio_iommu_type1_open(unsigned long arg)
>  	iommu->dma_list = RB_ROOT;
>  	mutex_init(&iommu->lock);
>  	mutex_init(&iommu->fault_lock);
> +	iommu->fault_count = 0;
>  	BLOCKING_INIT_NOTIFIER_HEAD(&iommu->notifier);
>  
>  	return iommu;
> @@ -1516,6 +1522,30 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
>  	return ret;
>  }
>  
> +static int vfio_iommu_fault_event_notifier(struct notifier_block *nb,
> +					   struct iommu_fault_info *fault_info,
> +					   void *data)
> +{
> +	struct vfio_iommu *iommu = data;
> +	struct vfio_iommu_fault_info *info;
> +
> +	mutex_lock(&iommu->fault_lock);
> +
> +	info = &iommu->fault_info[iommu->fault_count];
> +	info->addr = fault_info->addr;
> +	info->sid = fault_info->sid;
> +	info->fault_reason = fault_info->fault_reason;
> +	info->is_write = fault_info->is_write;
> +
> +	iommu->fault_count++;

Will corrupt memory once array overflows NR_IOMMU_FAULT_INFO.


> +
> +	if (iommu->iommu_fault_fd)
> +		eventfd_signal(iommu->iommu_fault_fd, 1);
> +
> +	mutex_unlock(&iommu->fault_lock);
> +	return 0;
> +}
> +
>  static long vfio_iommu_type1_ioctl(void *iommu_data,
>  				   unsigned int cmd, unsigned long arg)
>  {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 0ff5111..b6a7bdb 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -86,6 +86,13 @@ struct iommu_domain {
>  	void *iova_cookie;
>  };
>  
> +struct iommu_fault_info {
> +	__u64	addr;
> +	__u16   sid;
> +	__u8    fault_reason;
> +	__u8	is_write:1;
> +};
> +
>  enum iommu_cap {
>  	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU can enforce cache coherent DMA
>  					   transactions */
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 8616334..da359dd 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -562,6 +562,13 @@ struct vfio_iommu_type1_set_fault_eventfd {
>  
>  #define VFIO_IOMMU_SET_FAULT_EVENTFD	_IO(VFIO_TYPE, VFIO_BASE + 17)
>  
> +struct vfio_iommu_fault_info {
> +	__u64	addr;
> +	__u16   sid;

It's not clear it's userspace's business to know the sid.  It normally
does not care once management has bound vfio to a device. You should use
a device identifier that makes sense.


> +	__u8    fault_reason;
> +	__u8	is_write:1;
> +};
> +
>  /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
>  
>  /*
> -- 
> 1.8.3.1