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@xxxxxxxxx>
>> ---
>> 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
