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

[Lan Tianyu <tianyu.lan@intel.com>]

On 2017年02月21日 13:55, Michael S. Tsirkin wrote:
> 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?


When userspace gets fault info via new VFIO cmd, the fault_info in arry
will be clear. If userspace doesn't get fault info after triggering
fault event fd, the surplus fault info will be ignored.

> 
> 
>> +	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.


Yes, I miss check of NR_IOMMU_FAULT_INFO Here. Thanks.

> 
> 
>> +
>> +	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.

Yes, How about "bdf"?

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


-- 
Best regards
Tianyu Lan