From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lan Tianyu Subject: Re: [RFC PATCH 2/3] VFIO: Add IOMMU fault notifier callback Date: Tue, 21 Feb 2017 14:13:04 +0800 Message-ID: <370e5de8-cc3b-0da7-a829-debae7538432@intel.com> References: <1487515629-13815-1-git-send-email-tianyu.lan@intel.com> <1487515629-13815-3-git-send-email-tianyu.lan@intel.com> <20170221075030-mutt-send-email-mst@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, kevin.tian@intel.com, jan.kiszka@siemens.com, jasowang@redhat.com, peterx@redhat.com, david@gibson.dropbear.id.au, alex.williamson@redhat.com, yi.l.liu@intel.com To: "Michael S. Tsirkin" Return-path: Received: from mga07.intel.com ([134.134.136.100]:63976 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751043AbdBUGTs (ORCPT ); Tue, 21 Feb 2017 01:19:48 -0500 In-Reply-To: <20170221075030-mutt-send-email-mst@kernel.org> Sender: kvm-owner@vger.kernel.org List-ID: 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 >> --- >> 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