From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936446AbcKNToD (ORCPT ); Mon, 14 Nov 2016 14:44:03 -0500 Received: from mx1.redhat.com ([209.132.183.28]:45338 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932174AbcKNToB (ORCPT ); Mon, 14 Nov 2016 14:44:01 -0500 Date: Mon, 14 Nov 2016 12:43:59 -0700 From: Alex Williamson To: Kirti Wankhede Cc: , , , , , , , , Subject: Re: [PATCH v12 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver_ops Message-ID: <20161114124359.18162de8@t450s.home> In-Reply-To: <1479138156-28905-6-git-send-email-kwankhede@nvidia.com> References: <1479138156-28905-1-git-send-email-kwankhede@nvidia.com> <1479138156-28905-6-git-send-email-kwankhede@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Mon, 14 Nov 2016 19:44:01 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 14 Nov 2016 21:12:19 +0530 Kirti Wankhede wrote: > Added APIs for pining and unpining set of pages. These call back into > backend iommu module to actually pin and unpin pages. > Added two new callback functions to struct vfio_iommu_driver_ops. Backend > IOMMU module that supports pining and unpinning pages for mdev devices > should provide these functions. > > Renamed static functions in vfio_type1_iommu.c to resolve conflicts > > Signed-off-by: Kirti Wankhede > Signed-off-by: Neo Jia > Change-Id: Ia7417723aaae86bec2959ad9ae6c2915ddd340e0 > --- > drivers/vfio/vfio.c | 103 ++++++++++++++++++++++++++++++++++++++++ > drivers/vfio/vfio_iommu_type1.c | 20 ++++---- > include/linux/vfio.h | 12 ++++- > 3 files changed, 124 insertions(+), 11 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index 2e83bdf007fe..7dcfbca2016a 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -1799,6 +1799,109 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset) > } > EXPORT_SYMBOL_GPL(vfio_info_cap_shift); > > + > +/* > + * Pin a set of guest PFNs and return their associated host PFNs for local > + * domain only. > + * @dev [in] : device > + * @user_pfn [in]: array of user/guest PFNs to be unpinned. Number of user/guest > + * PFNs should not be greater than PAGE_SIZE. > + * @npage [in] :count of elements in array. This count should not be greater > + * than PAGE_SIZE. > + * @prot [in] : protection flags > + * @phys_pfn[out] : array of host PFNs > + * Return error or number of pages pinned. > + */ > +int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, int npage, > + int prot, unsigned long *phys_pfn) > +{ > + struct vfio_container *container; > + struct vfio_group *group; > + struct vfio_iommu_driver *driver; > + int ret; > + > + if (!dev || !user_pfn || !phys_pfn || !npage) > + return -EINVAL; > + > + if (npage >= PAGE_SIZE) > + return -E2BIG; This misses the point of using PAGE_SIZE. The concern is that previously we were allowing (nearly) arbitrarily large arrays to be passed around. The agreement as I understood it would be that the array itself would be sized up to a maximum of PAGE_SIZE, which means the number of entries cannot exceed PAGE_SIZE/sizeof(*user_pfn) (ie. 512 of x86). I also suggested that we should have a #define for this so that vendor drivers can actually chunk their calls into allowable sizes if they need to and not need to guess the limit, ex. include/linux/vfio.h #define VFIO_PAGE_PINNING_MAX_ENTRIES (PAGE_SIZE / sizeof(unsigned long)) If we wanted a simple limit to the number of entries per call, there would be no reason to have it based on PAGE_SIZE. Thanks, Alex > + > + group = vfio_group_get_from_dev(dev); > + if (IS_ERR(group)) > + return PTR_ERR(group); > + > + ret = vfio_group_add_container_user(group); > + if (ret) > + goto err_pin_pages; > + > + container = group->container; > + down_read(&container->group_lock); > + > + driver = container->iommu_driver; > + if (likely(driver && driver->ops->pin_pages)) > + ret = driver->ops->pin_pages(container->iommu_data, user_pfn, > + npage, prot, phys_pfn); > + else > + ret = -ENOTTY; > + > + up_read(&container->group_lock); > + vfio_group_try_dissolve_container(group); > + > +err_pin_pages: > + vfio_group_put(group); > + return ret; > +} > +EXPORT_SYMBOL(vfio_pin_pages); > + > +/* > + * Unpin set of host PFNs for local domain only. > + * @dev [in] : device > + * @user_pfn [in]: array of user/guest PFNs to be unpinned. Number of user/guest > + * PFNs should not be greater than PAGE_SIZE. > + * @npage [in] :count of elements in array. This count should not be greater > + * than PAGE_SIZE. > + * Return error or number of pages unpinned. > + */ > +int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, int npage) > +{ > + struct vfio_container *container; > + struct vfio_group *group; > + struct vfio_iommu_driver *driver; > + int ret; > + > + if (!dev || !user_pfn || !npage) > + return -EINVAL; > + > + if (npage >= PAGE_SIZE) > + return -E2BIG; > + > + group = vfio_group_get_from_dev(dev); > + if (IS_ERR(group)) > + return PTR_ERR(group); > + > + ret = vfio_group_add_container_user(group); > + if (ret) > + goto err_unpin_pages; > + > + container = group->container; > + down_read(&container->group_lock); > + > + driver = container->iommu_driver; > + if (likely(driver && driver->ops->unpin_pages)) > + ret = driver->ops->unpin_pages(container->iommu_data, user_pfn, > + npage); > + else > + ret = -ENOTTY; > + > + up_read(&container->group_lock); > + vfio_group_try_dissolve_container(group); > + > +err_unpin_pages: > + vfio_group_put(group); > + return ret; > +} > +EXPORT_SYMBOL(vfio_unpin_pages); > + > /** > * Module/class support > */ > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 2ba19424e4a1..9f3d58d3dfaf 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -259,8 +259,8 @@ static int vaddr_get_pfn(unsigned long vaddr, int prot, unsigned long *pfn) > * the iommu can only map chunks of consecutive pfns anyway, so get the > * first page and all consecutive pages with the same locking. > */ > -static long vfio_pin_pages(unsigned long vaddr, long npage, > - int prot, unsigned long *pfn_base) > +static long vfio_pin_pages_remote(unsigned long vaddr, long npage, > + int prot, unsigned long *pfn_base) > { > unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > bool lock_cap = capable(CAP_IPC_LOCK); > @@ -318,8 +318,8 @@ static long vfio_pin_pages(unsigned long vaddr, long npage, > return i; > } > > -static long vfio_unpin_pages(unsigned long pfn, long npage, > - int prot, bool do_accounting) > +static long vfio_unpin_pages_remote(unsigned long pfn, long npage, > + int prot, bool do_accounting) > { > unsigned long unlocked = 0; > long i; > @@ -382,9 +382,9 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma) > if (WARN_ON(!unmapped)) > break; > > - unlocked += vfio_unpin_pages(phys >> PAGE_SHIFT, > - unmapped >> PAGE_SHIFT, > - dma->prot, false); > + unlocked += vfio_unpin_pages_remote(phys >> PAGE_SHIFT, > + unmapped >> PAGE_SHIFT, > + dma->prot, false); > iova += unmapped; > > cond_resched(); > @@ -613,8 +613,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu, > > while (size) { > /* Pin a contiguous chunk of memory */ > - npage = vfio_pin_pages(vaddr + dma->size, > - size >> PAGE_SHIFT, prot, &pfn); > + npage = vfio_pin_pages_remote(vaddr + dma->size, > + size >> PAGE_SHIFT, prot, &pfn); > if (npage <= 0) { > WARN_ON(!npage); > ret = (int)npage; > @@ -624,7 +624,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu, > /* Map it! */ > ret = vfio_iommu_map(iommu, iova + dma->size, pfn, npage, prot); > if (ret) { > - vfio_unpin_pages(pfn, npage, prot, true); > + vfio_unpin_pages_remote(pfn, npage, prot, true); > break; > } > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 0ecae0b1cd34..86f507d0f585 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -75,7 +75,11 @@ struct vfio_iommu_driver_ops { > struct iommu_group *group); > void (*detach_group)(void *iommu_data, > struct iommu_group *group); > - > + int (*pin_pages)(void *iommu_data, unsigned long *user_pfn, > + int npage, int prot, > + unsigned long *phys_pfn); > + int (*unpin_pages)(void *iommu_data, > + unsigned long *user_pfn, int npage); > }; > > extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops); > @@ -127,6 +131,12 @@ static inline long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group, > } > #endif /* CONFIG_EEH */ > > +extern int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, > + int npage, int prot, unsigned long *phys_pfn); > + > +extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, > + int npage); > + > /* > * IRQfd - generic > */ From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46970) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6NAl-0000za-UY for qemu-devel@nongnu.org; Mon, 14 Nov 2016 14:44:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6NAg-00071Z-7F for qemu-devel@nongnu.org; Mon, 14 Nov 2016 14:44:07 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47192) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c6NAf-00070K-Ut for qemu-devel@nongnu.org; Mon, 14 Nov 2016 14:44:02 -0500 Date: Mon, 14 Nov 2016 12:43:59 -0700 From: Alex Williamson Message-ID: <20161114124359.18162de8@t450s.home> In-Reply-To: <1479138156-28905-6-git-send-email-kwankhede@nvidia.com> References: <1479138156-28905-1-git-send-email-kwankhede@nvidia.com> <1479138156-28905-6-git-send-email-kwankhede@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v12 05/22] vfio iommu: Added pin and unpin callback functions to vfio_iommu_driver_ops List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kirti Wankhede Cc: pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, kevin.tian@intel.com, jike.song@intel.com, bjsdjshi@linux.vnet.ibm.com, linux-kernel@vger.kernel.org On Mon, 14 Nov 2016 21:12:19 +0530 Kirti Wankhede wrote: > Added APIs for pining and unpining set of pages. These call back into > backend iommu module to actually pin and unpin pages. > Added two new callback functions to struct vfio_iommu_driver_ops. Backend > IOMMU module that supports pining and unpinning pages for mdev devices > should provide these functions. > > Renamed static functions in vfio_type1_iommu.c to resolve conflicts > > Signed-off-by: Kirti Wankhede > Signed-off-by: Neo Jia > Change-Id: Ia7417723aaae86bec2959ad9ae6c2915ddd340e0 > --- > drivers/vfio/vfio.c | 103 ++++++++++++++++++++++++++++++++++++++++ > drivers/vfio/vfio_iommu_type1.c | 20 ++++---- > include/linux/vfio.h | 12 ++++- > 3 files changed, 124 insertions(+), 11 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index 2e83bdf007fe..7dcfbca2016a 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -1799,6 +1799,109 @@ void vfio_info_cap_shift(struct vfio_info_cap *caps, size_t offset) > } > EXPORT_SYMBOL_GPL(vfio_info_cap_shift); > > + > +/* > + * Pin a set of guest PFNs and return their associated host PFNs for local > + * domain only. > + * @dev [in] : device > + * @user_pfn [in]: array of user/guest PFNs to be unpinned. Number of user/guest > + * PFNs should not be greater than PAGE_SIZE. > + * @npage [in] :count of elements in array. This count should not be greater > + * than PAGE_SIZE. > + * @prot [in] : protection flags > + * @phys_pfn[out] : array of host PFNs > + * Return error or number of pages pinned. > + */ > +int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, int npage, > + int prot, unsigned long *phys_pfn) > +{ > + struct vfio_container *container; > + struct vfio_group *group; > + struct vfio_iommu_driver *driver; > + int ret; > + > + if (!dev || !user_pfn || !phys_pfn || !npage) > + return -EINVAL; > + > + if (npage >= PAGE_SIZE) > + return -E2BIG; This misses the point of using PAGE_SIZE. The concern is that previously we were allowing (nearly) arbitrarily large arrays to be passed around. The agreement as I understood it would be that the array itself would be sized up to a maximum of PAGE_SIZE, which means the number of entries cannot exceed PAGE_SIZE/sizeof(*user_pfn) (ie. 512 of x86). I also suggested that we should have a #define for this so that vendor drivers can actually chunk their calls into allowable sizes if they need to and not need to guess the limit, ex. include/linux/vfio.h #define VFIO_PAGE_PINNING_MAX_ENTRIES (PAGE_SIZE / sizeof(unsigned long)) If we wanted a simple limit to the number of entries per call, there would be no reason to have it based on PAGE_SIZE. Thanks, Alex > + > + group = vfio_group_get_from_dev(dev); > + if (IS_ERR(group)) > + return PTR_ERR(group); > + > + ret = vfio_group_add_container_user(group); > + if (ret) > + goto err_pin_pages; > + > + container = group->container; > + down_read(&container->group_lock); > + > + driver = container->iommu_driver; > + if (likely(driver && driver->ops->pin_pages)) > + ret = driver->ops->pin_pages(container->iommu_data, user_pfn, > + npage, prot, phys_pfn); > + else > + ret = -ENOTTY; > + > + up_read(&container->group_lock); > + vfio_group_try_dissolve_container(group); > + > +err_pin_pages: > + vfio_group_put(group); > + return ret; > +} > +EXPORT_SYMBOL(vfio_pin_pages); > + > +/* > + * Unpin set of host PFNs for local domain only. > + * @dev [in] : device > + * @user_pfn [in]: array of user/guest PFNs to be unpinned. Number of user/guest > + * PFNs should not be greater than PAGE_SIZE. > + * @npage [in] :count of elements in array. This count should not be greater > + * than PAGE_SIZE. > + * Return error or number of pages unpinned. > + */ > +int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, int npage) > +{ > + struct vfio_container *container; > + struct vfio_group *group; > + struct vfio_iommu_driver *driver; > + int ret; > + > + if (!dev || !user_pfn || !npage) > + return -EINVAL; > + > + if (npage >= PAGE_SIZE) > + return -E2BIG; > + > + group = vfio_group_get_from_dev(dev); > + if (IS_ERR(group)) > + return PTR_ERR(group); > + > + ret = vfio_group_add_container_user(group); > + if (ret) > + goto err_unpin_pages; > + > + container = group->container; > + down_read(&container->group_lock); > + > + driver = container->iommu_driver; > + if (likely(driver && driver->ops->unpin_pages)) > + ret = driver->ops->unpin_pages(container->iommu_data, user_pfn, > + npage); > + else > + ret = -ENOTTY; > + > + up_read(&container->group_lock); > + vfio_group_try_dissolve_container(group); > + > +err_unpin_pages: > + vfio_group_put(group); > + return ret; > +} > +EXPORT_SYMBOL(vfio_unpin_pages); > + > /** > * Module/class support > */ > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 2ba19424e4a1..9f3d58d3dfaf 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -259,8 +259,8 @@ static int vaddr_get_pfn(unsigned long vaddr, int prot, unsigned long *pfn) > * the iommu can only map chunks of consecutive pfns anyway, so get the > * first page and all consecutive pages with the same locking. > */ > -static long vfio_pin_pages(unsigned long vaddr, long npage, > - int prot, unsigned long *pfn_base) > +static long vfio_pin_pages_remote(unsigned long vaddr, long npage, > + int prot, unsigned long *pfn_base) > { > unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT; > bool lock_cap = capable(CAP_IPC_LOCK); > @@ -318,8 +318,8 @@ static long vfio_pin_pages(unsigned long vaddr, long npage, > return i; > } > > -static long vfio_unpin_pages(unsigned long pfn, long npage, > - int prot, bool do_accounting) > +static long vfio_unpin_pages_remote(unsigned long pfn, long npage, > + int prot, bool do_accounting) > { > unsigned long unlocked = 0; > long i; > @@ -382,9 +382,9 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma) > if (WARN_ON(!unmapped)) > break; > > - unlocked += vfio_unpin_pages(phys >> PAGE_SHIFT, > - unmapped >> PAGE_SHIFT, > - dma->prot, false); > + unlocked += vfio_unpin_pages_remote(phys >> PAGE_SHIFT, > + unmapped >> PAGE_SHIFT, > + dma->prot, false); > iova += unmapped; > > cond_resched(); > @@ -613,8 +613,8 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu, > > while (size) { > /* Pin a contiguous chunk of memory */ > - npage = vfio_pin_pages(vaddr + dma->size, > - size >> PAGE_SHIFT, prot, &pfn); > + npage = vfio_pin_pages_remote(vaddr + dma->size, > + size >> PAGE_SHIFT, prot, &pfn); > if (npage <= 0) { > WARN_ON(!npage); > ret = (int)npage; > @@ -624,7 +624,7 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu, > /* Map it! */ > ret = vfio_iommu_map(iommu, iova + dma->size, pfn, npage, prot); > if (ret) { > - vfio_unpin_pages(pfn, npage, prot, true); > + vfio_unpin_pages_remote(pfn, npage, prot, true); > break; > } > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index 0ecae0b1cd34..86f507d0f585 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -75,7 +75,11 @@ struct vfio_iommu_driver_ops { > struct iommu_group *group); > void (*detach_group)(void *iommu_data, > struct iommu_group *group); > - > + int (*pin_pages)(void *iommu_data, unsigned long *user_pfn, > + int npage, int prot, > + unsigned long *phys_pfn); > + int (*unpin_pages)(void *iommu_data, > + unsigned long *user_pfn, int npage); > }; > > extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops); > @@ -127,6 +131,12 @@ static inline long vfio_spapr_iommu_eeh_ioctl(struct iommu_group *group, > } > #endif /* CONFIG_EEH */ > > +extern int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, > + int npage, int prot, unsigned long *phys_pfn); > + > +extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, > + int npage); > + > /* > * IRQfd - generic > */