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

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

On 2017年02月21日 04:53, Alex Williamson wrote:
> On Sun, 19 Feb 2017 22:47:08 +0800
> Lan Tianyu <tianyu.lan@intel.com> 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
> 
> How is this value determined?

The length of fault info array is defined by manually.
It's hard to estimate how many fault info entry will be queued in the
array.

> 
>> +
>>  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];
>> +	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++;
>> +
>> +	if (iommu->iommu_fault_fd)
>> +		eventfd_signal(iommu->iommu_fault_fd, 1);
>> +
>> +	mutex_unlock(&iommu->fault_lock);
>> +	return 0;
>> +}
> 
> 
> This notifier is never registered as any sort of IOMMU fault reporting
> infrastructure is currently imaginary, and the iommu development list
> isn't cc'd.

Yes, I should cc IOMM maillist. I thought this patchset was to determine
new VFIO API and so...

> 
>> +
>>  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;
>> +};
> 
> 
> A common fault reporting structure would need to be agreed upon by all
> parties, ie. everyone that current supporting the IOMMU API.  As used
> in QEMU, the fault_reason here appears to be very VT-d specific.

Fault reason number will be platform specific and it'd hard to use a
common definition to show fault reason number of all platforms. I wonder
whether we can define a vendor field in the fault info and
consumer can use the vendor field to deal with platform specific info.
-- 
Best regards
Tianyu Lan