* [PATCH 0/2] use vfio_iova_rw() to read/write IOVAs from CPU side @ 2020-01-03 1:00 Yan Zhao 2020-01-03 1:02 ` [PATCH 1/2] vfio: introduce vfio_iova_rw to read/write a range of IOVAs Yan Zhao 2020-01-03 1:03 ` [PATCH 2/2] drm/i915/gvt: subsitute kvm_read/write_guest with vfio_iova_rw Yan Zhao 0 siblings, 2 replies; 6+ messages in thread From: Yan Zhao @ 2020-01-03 1:00 UTC (permalink / raw) To: alex.williamson, zhenyuw Cc: kvm, linux-kernel, intel-gvt, pbonzini, kevin.tian, Yan Zhao When device models read/write memory pointed by a range of IOVAs, it is better to use vfio interfaces. And because this read/write is from CPUs, there's no need to call vfio_pin_pages() to pin those memory. patch 1 introduces interface vfio_iova_rw() in vfio to read/write userspace IOVAs without pinning userspace pages. patch 2 let gvt switch from kvm side rw interface to vfio_iova_rw(). Yan Zhao (2): vfio: introduce vfio_iova_rw to read/write a range of IOVAs drm/i915/gvt: subsitute kvm_read/write_guest with vfio_iova_rw drivers/gpu/drm/i915/gvt/kvmgt.c | 26 +++------- drivers/vfio/vfio.c | 45 ++++++++++++++++++ drivers/vfio/vfio_iommu_type1.c | 81 ++++++++++++++++++++++++++++++++ include/linux/vfio.h | 5 ++ 4 files changed, 138 insertions(+), 19 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] vfio: introduce vfio_iova_rw to read/write a range of IOVAs 2020-01-03 1:00 [PATCH 0/2] use vfio_iova_rw() to read/write IOVAs from CPU side Yan Zhao @ 2020-01-03 1:02 ` Yan Zhao 2020-01-09 18:16 ` Alex Williamson 2020-01-03 1:03 ` [PATCH 2/2] drm/i915/gvt: subsitute kvm_read/write_guest with vfio_iova_rw Yan Zhao 1 sibling, 1 reply; 6+ messages in thread From: Yan Zhao @ 2020-01-03 1:02 UTC (permalink / raw) To: alex.williamson Cc: zhenyuw, kvm, linux-kernel, intel-gvt, pbonzini, kevin.tian, Yan Zhao vfio_iova_rw will read/write a range of userspace memory (starting form device iova to iova + len -1) into a kenrel buffer without pinning the userspace memory. TODO: vfio needs to mark the iova dirty if vfio_iova_rw(write) is called. Cc: Kevin Tian <kevin.tian@intel.com> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- drivers/vfio/vfio.c | 45 ++++++++++++++++++ drivers/vfio/vfio_iommu_type1.c | 81 +++++++++++++++++++++++++++++++++ include/linux/vfio.h | 5 ++ 3 files changed, 131 insertions(+) diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index c8482624ca34..36e91e647ed5 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -1961,6 +1961,51 @@ int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, int npage) } EXPORT_SYMBOL(vfio_unpin_pages); +/* + * Read/Write a range of userspace IOVAs for a device into/from a kernel + * buffer without pinning the userspace memory + * @dev [in] : device + * @iova [in] : base IOVA of a userspace buffer + * @data [in] : pointer to kernel buffer + * @len [in] : kernel buffer length + * @write : indicate read or write + * Return error on failure or 0 on success. + */ +int vfio_iova_rw(struct device *dev, unsigned long iova, void *data, + unsigned long len, bool write) +{ + struct vfio_container *container; + struct vfio_group *group; + struct vfio_iommu_driver *driver; + int ret = 0; + + if (!dev || !data || len <= 0) + return -EINVAL; + + group = vfio_group_get_from_dev(dev); + if (!group) + return -ENODEV; + + ret = vfio_group_add_container_user(group); + if (ret) + goto out; + + container = group->container; + driver = container->iommu_driver; + + if (likely(driver && driver->ops->iova_rw)) + ret = driver->ops->iova_rw(container->iommu_data, + iova, data, len, write); + else + ret = -ENOTTY; + + vfio_group_try_dissolve_container(group); +out: + vfio_group_put(group); + return ret; +} +EXPORT_SYMBOL(vfio_iova_rw); + static int vfio_register_iommu_notifier(struct vfio_group *group, unsigned long *events, struct notifier_block *nb) diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 2ada8e6cdb88..aee191077235 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -27,6 +27,7 @@ #include <linux/iommu.h> #include <linux/module.h> #include <linux/mm.h> +#include <linux/mmu_context.h> #include <linux/rbtree.h> #include <linux/sched/signal.h> #include <linux/sched/mm.h> @@ -2326,6 +2327,85 @@ static int vfio_iommu_type1_unregister_notifier(void *iommu_data, return blocking_notifier_chain_unregister(&iommu->notifier, nb); } +static int next_segment(unsigned long len, int offset) +{ + if (len > PAGE_SIZE - offset) + return PAGE_SIZE - offset; + else + return len; +} + +static int vfio_iommu_type1_rw_iova_seg(struct vfio_iommu *iommu, + unsigned long iova, void *data, + unsigned long seg_len, + unsigned long offset, + bool write) +{ + struct mm_struct *mm; + unsigned long vaddr; + struct vfio_dma *dma; + bool kthread = current->mm == NULL; + int ret = 0; + + dma = vfio_find_dma(iommu, iova, PAGE_SIZE); + if (!dma) + return -EINVAL; + + mm = get_task_mm(dma->task); + + if (!mm) + return -ENODEV; + + if (kthread) + use_mm(mm); + else if (current->mm != mm) { + ret = -EINVAL; + goto out; + } + + vaddr = dma->vaddr + iova - dma->iova + offset; + + ret = write ? __copy_to_user((void __user *)vaddr, + data, seg_len) : + __copy_from_user(data, (void __user *)vaddr, + seg_len); + if (ret) + ret = -EFAULT; + + if (kthread) + unuse_mm(mm); +out: + mmput(mm); + return ret; +} + +static int vfio_iommu_type1_iova_rw(void *iommu_data, unsigned long iova, + void *data, unsigned long len, bool write) +{ + struct vfio_iommu *iommu = iommu_data; + int offset = iova & ~PAGE_MASK; + int seg_len; + int ret = 0; + + iova = iova & PAGE_MASK; + + mutex_lock(&iommu->lock); + while ((seg_len = next_segment(len, offset)) > 0) { + ret = vfio_iommu_type1_rw_iova_seg(iommu, iova, data, + seg_len, offset, write); + if (ret) + break; + + offset = 0; + len -= seg_len; + data += seg_len; + iova += PAGE_SIZE; + } + + mutex_unlock(&iommu->lock); + return ret; +} + static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = { .name = "vfio-iommu-type1", .owner = THIS_MODULE, @@ -2338,6 +2418,7 @@ static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = { .unpin_pages = vfio_iommu_type1_unpin_pages, .register_notifier = vfio_iommu_type1_register_notifier, .unregister_notifier = vfio_iommu_type1_unregister_notifier, + .iova_rw = vfio_iommu_type1_iova_rw, }; static int __init vfio_iommu_type1_init(void) diff --git a/include/linux/vfio.h b/include/linux/vfio.h index e42a711a2800..7bf18a31bbcf 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -82,6 +82,8 @@ struct vfio_iommu_driver_ops { struct notifier_block *nb); int (*unregister_notifier)(void *iommu_data, struct notifier_block *nb); + int (*iova_rw)(void *iommu_data, unsigned long iova, + void *data, unsigned long len, bool write); }; extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops); @@ -107,6 +109,9 @@ extern int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, int npage); +extern int vfio_iova_rw(struct device *dev, unsigned long iova, void *data, + unsigned long len, bool write); + /* each type has independent events */ enum vfio_notify_type { VFIO_IOMMU_NOTIFY = 0, -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vfio: introduce vfio_iova_rw to read/write a range of IOVAs 2020-01-03 1:02 ` [PATCH 1/2] vfio: introduce vfio_iova_rw to read/write a range of IOVAs Yan Zhao @ 2020-01-09 18:16 ` Alex Williamson 2020-01-10 3:24 ` Yan Zhao 0 siblings, 1 reply; 6+ messages in thread From: Alex Williamson @ 2020-01-09 18:16 UTC (permalink / raw) To: Yan Zhao; +Cc: zhenyuw, kvm, linux-kernel, pbonzini, kevin.tian On Thu, 2 Jan 2020 20:02:17 -0500 Yan Zhao <yan.y.zhao@intel.com> wrote: > vfio_iova_rw will read/write a range of userspace memory (starting form > device iova to iova + len -1) into a kenrel buffer without pinning the > userspace memory. > > TODO: vfio needs to mark the iova dirty if vfio_iova_rw(write) is > called. > > Cc: Kevin Tian <kevin.tian@intel.com> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > --- > drivers/vfio/vfio.c | 45 ++++++++++++++++++ > drivers/vfio/vfio_iommu_type1.c | 81 +++++++++++++++++++++++++++++++++ > include/linux/vfio.h | 5 ++ > 3 files changed, 131 insertions(+) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index c8482624ca34..36e91e647ed5 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -1961,6 +1961,51 @@ int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, int npage) > } > EXPORT_SYMBOL(vfio_unpin_pages); > > +/* > + * Read/Write a range of userspace IOVAs for a device into/from a kernel > + * buffer without pinning the userspace memory > + * @dev [in] : device > + * @iova [in] : base IOVA of a userspace buffer > + * @data [in] : pointer to kernel buffer > + * @len [in] : kernel buffer length > + * @write : indicate read or write > + * Return error on failure or 0 on success. > + */ > +int vfio_iova_rw(struct device *dev, unsigned long iova, void *data, > + unsigned long len, bool write) Shouldn't iova be a dma_addr_t and len be a size_t? AIUI this function performs the equivalent behavior of the device itself performing a DMA. Hmm, should the interface be named vfio_dma_rw()? > +{ > + struct vfio_container *container; > + struct vfio_group *group; > + struct vfio_iommu_driver *driver; > + int ret = 0; > + > + if (!dev || !data || len <= 0) > + return -EINVAL; > + > + group = vfio_group_get_from_dev(dev); > + if (!group) > + return -ENODEV; > + > + ret = vfio_group_add_container_user(group); > + if (ret) > + goto out; > + > + container = group->container; > + driver = container->iommu_driver; > + > + if (likely(driver && driver->ops->iova_rw)) > + ret = driver->ops->iova_rw(container->iommu_data, > + iova, data, len, write); > + else > + ret = -ENOTTY; > + > + vfio_group_try_dissolve_container(group); > +out: > + vfio_group_put(group); > + return ret; > +} > +EXPORT_SYMBOL(vfio_iova_rw); > + > static int vfio_register_iommu_notifier(struct vfio_group *group, > unsigned long *events, > struct notifier_block *nb) > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 2ada8e6cdb88..aee191077235 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -27,6 +27,7 @@ > #include <linux/iommu.h> > #include <linux/module.h> > #include <linux/mm.h> > +#include <linux/mmu_context.h> > #include <linux/rbtree.h> > #include <linux/sched/signal.h> > #include <linux/sched/mm.h> > @@ -2326,6 +2327,85 @@ static int vfio_iommu_type1_unregister_notifier(void *iommu_data, > return blocking_notifier_chain_unregister(&iommu->notifier, nb); > } > > +static int next_segment(unsigned long len, int offset) > +{ > + if (len > PAGE_SIZE - offset) > + return PAGE_SIZE - offset; > + else > + return len; > +} > + > +static int vfio_iommu_type1_rw_iova_seg(struct vfio_iommu *iommu, > + unsigned long iova, void *data, > + unsigned long seg_len, > + unsigned long offset, > + bool write) > +{ > + struct mm_struct *mm; > + unsigned long vaddr; > + struct vfio_dma *dma; > + bool kthread = current->mm == NULL; > + int ret = 0; > + > + dma = vfio_find_dma(iommu, iova, PAGE_SIZE); > + if (!dma) > + return -EINVAL; > + > + mm = get_task_mm(dma->task); > + > + if (!mm) > + return -ENODEV; > + > + if (kthread) > + use_mm(mm); > + else if (current->mm != mm) { > + ret = -EINVAL; > + goto out; > + } > + > + vaddr = dma->vaddr + iova - dma->iova + offset; Parenthesis here would be useful and might prevent overflow, ie: dma->vaddr + (iova - dma->iova) + offset > + > + ret = write ? __copy_to_user((void __user *)vaddr, > + data, seg_len) : > + __copy_from_user(data, (void __user *)vaddr, > + seg_len); > + if (ret) > + ret = -EFAULT; > + > + if (kthread) > + unuse_mm(mm); > +out: > + mmput(mm); > + return ret; > +} > + > +static int vfio_iommu_type1_iova_rw(void *iommu_data, unsigned long iova, > + void *data, unsigned long len, bool write) > +{ > + struct vfio_iommu *iommu = iommu_data; > + int offset = iova & ~PAGE_MASK; > + int seg_len; > + int ret = 0; > + > + iova = iova & PAGE_MASK; > + > + mutex_lock(&iommu->lock); > + while ((seg_len = next_segment(len, offset)) > 0) { > + ret = vfio_iommu_type1_rw_iova_seg(iommu, iova, data, > + seg_len, offset, write); Why do we need to split operations at page boundaries? It seems really inefficient that at each page crossing we need to lookup the vfio_dma again (probably the same one), switch to the mm (probably the same one), and perform another copy_{to,from}_user() when potentially have everything we need to perform a larger copy. Thanks, Alex > + if (ret) > + break; > + > + offset = 0; > + len -= seg_len; > + data += seg_len; > + iova += PAGE_SIZE; > + } > + > + mutex_unlock(&iommu->lock); > + return ret; > +} > + > static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = { > .name = "vfio-iommu-type1", > .owner = THIS_MODULE, > @@ -2338,6 +2418,7 @@ static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = { > .unpin_pages = vfio_iommu_type1_unpin_pages, > .register_notifier = vfio_iommu_type1_register_notifier, > .unregister_notifier = vfio_iommu_type1_unregister_notifier, > + .iova_rw = vfio_iommu_type1_iova_rw, > }; > > static int __init vfio_iommu_type1_init(void) > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > index e42a711a2800..7bf18a31bbcf 100644 > --- a/include/linux/vfio.h > +++ b/include/linux/vfio.h > @@ -82,6 +82,8 @@ struct vfio_iommu_driver_ops { > struct notifier_block *nb); > int (*unregister_notifier)(void *iommu_data, > struct notifier_block *nb); > + int (*iova_rw)(void *iommu_data, unsigned long iova, > + void *data, unsigned long len, bool write); > }; > > extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops); > @@ -107,6 +109,9 @@ extern int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, > extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, > int npage); > > +extern int vfio_iova_rw(struct device *dev, unsigned long iova, void *data, > + unsigned long len, bool write); > + > /* each type has independent events */ > enum vfio_notify_type { > VFIO_IOMMU_NOTIFY = 0, ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vfio: introduce vfio_iova_rw to read/write a range of IOVAs 2020-01-09 18:16 ` Alex Williamson @ 2020-01-10 3:24 ` Yan Zhao 0 siblings, 0 replies; 6+ messages in thread From: Yan Zhao @ 2020-01-10 3:24 UTC (permalink / raw) To: Alex Williamson; +Cc: zhenyuw, kvm, linux-kernel, pbonzini, Tian, Kevin On Fri, Jan 10, 2020 at 02:16:36AM +0800, Alex Williamson wrote: > On Thu, 2 Jan 2020 20:02:17 -0500 > Yan Zhao <yan.y.zhao@intel.com> wrote: > > > vfio_iova_rw will read/write a range of userspace memory (starting form > > device iova to iova + len -1) into a kenrel buffer without pinning the > > userspace memory. > > > > TODO: vfio needs to mark the iova dirty if vfio_iova_rw(write) is > > called. > > > > Cc: Kevin Tian <kevin.tian@intel.com> > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > --- > > drivers/vfio/vfio.c | 45 ++++++++++++++++++ > > drivers/vfio/vfio_iommu_type1.c | 81 +++++++++++++++++++++++++++++++++ > > include/linux/vfio.h | 5 ++ > > 3 files changed, 131 insertions(+) > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > index c8482624ca34..36e91e647ed5 100644 > > --- a/drivers/vfio/vfio.c > > +++ b/drivers/vfio/vfio.c > > @@ -1961,6 +1961,51 @@ int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, int npage) > > } > > EXPORT_SYMBOL(vfio_unpin_pages); > > > > +/* > > + * Read/Write a range of userspace IOVAs for a device into/from a kernel > > + * buffer without pinning the userspace memory > > + * @dev [in] : device > > + * @iova [in] : base IOVA of a userspace buffer > > + * @data [in] : pointer to kernel buffer > > + * @len [in] : kernel buffer length > > + * @write : indicate read or write > > + * Return error on failure or 0 on success. > > + */ > > +int vfio_iova_rw(struct device *dev, unsigned long iova, void *data, > > + unsigned long len, bool write) > > Shouldn't iova be a dma_addr_t and len be a size_t? AIUI this function > performs the equivalent behavior of the device itself performing a DMA. > Hmm, should the interface be named vfio_dma_rw()? > ok. will rename the interface to vfio_dma_rw(). thanks :) > > +{ > > + struct vfio_container *container; > > + struct vfio_group *group; > > + struct vfio_iommu_driver *driver; > > + int ret = 0; > > + > > + if (!dev || !data || len <= 0) > > + return -EINVAL; > > + > > + group = vfio_group_get_from_dev(dev); > > + if (!group) > > + return -ENODEV; > > + > > + ret = vfio_group_add_container_user(group); > > + if (ret) > > + goto out; > > + > > + container = group->container; > > + driver = container->iommu_driver; > > + > > + if (likely(driver && driver->ops->iova_rw)) > > + ret = driver->ops->iova_rw(container->iommu_data, > > + iova, data, len, write); > > + else > > + ret = -ENOTTY; > > + > > + vfio_group_try_dissolve_container(group); > > +out: > > + vfio_group_put(group); > > + return ret; > > +} > > +EXPORT_SYMBOL(vfio_iova_rw); > > + > > static int vfio_register_iommu_notifier(struct vfio_group *group, > > unsigned long *events, > > struct notifier_block *nb) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > > index 2ada8e6cdb88..aee191077235 100644 > > --- a/drivers/vfio/vfio_iommu_type1.c > > +++ b/drivers/vfio/vfio_iommu_type1.c > > @@ -27,6 +27,7 @@ > > #include <linux/iommu.h> > > #include <linux/module.h> > > #include <linux/mm.h> > > +#include <linux/mmu_context.h> > > #include <linux/rbtree.h> > > #include <linux/sched/signal.h> > > #include <linux/sched/mm.h> > > @@ -2326,6 +2327,85 @@ static int vfio_iommu_type1_unregister_notifier(void *iommu_data, > > return blocking_notifier_chain_unregister(&iommu->notifier, nb); > > } > > > > +static int next_segment(unsigned long len, int offset) > > +{ > > + if (len > PAGE_SIZE - offset) > > + return PAGE_SIZE - offset; > > + else > > + return len; > > +} > > + > > +static int vfio_iommu_type1_rw_iova_seg(struct vfio_iommu *iommu, > > + unsigned long iova, void *data, > > + unsigned long seg_len, > > + unsigned long offset, > > + bool write) > > +{ > > + struct mm_struct *mm; > > + unsigned long vaddr; > > + struct vfio_dma *dma; > > + bool kthread = current->mm == NULL; > > + int ret = 0; > > + > > + dma = vfio_find_dma(iommu, iova, PAGE_SIZE); > > + if (!dma) > > + return -EINVAL; > > + > > + mm = get_task_mm(dma->task); > > + > > + if (!mm) > > + return -ENODEV; > > + > > + if (kthread) > > + use_mm(mm); > > + else if (current->mm != mm) { > > + ret = -EINVAL; > > + goto out; > > + } > > + > > + vaddr = dma->vaddr + iova - dma->iova + offset; > > Parenthesis here would be useful and might prevent overflow, ie: > > dma->vaddr + (iova - dma->iova) + offset > Yes, thanks for pointing it out! > > + > > + ret = write ? __copy_to_user((void __user *)vaddr, > > + data, seg_len) : > > + __copy_from_user(data, (void __user *)vaddr, > > + seg_len); > > + if (ret) > > + ret = -EFAULT; > > + > > + if (kthread) > > + unuse_mm(mm); > > +out: > > + mmput(mm); > > + return ret; > > +} > > + > > +static int vfio_iommu_type1_iova_rw(void *iommu_data, unsigned long iova, > > + void *data, unsigned long len, bool write) > > +{ > > + struct vfio_iommu *iommu = iommu_data; > > + int offset = iova & ~PAGE_MASK; > > + int seg_len; > > + int ret = 0; > > + > > + iova = iova & PAGE_MASK; > > + > > + mutex_lock(&iommu->lock); > > + while ((seg_len = next_segment(len, offset)) > 0) { > > + ret = vfio_iommu_type1_rw_iova_seg(iommu, iova, data, > > + seg_len, offset, write); > > Why do we need to split operations at page boundaries? It seems really > inefficient that at each page crossing we need to lookup the vfio_dma > again (probably the same one), switch to the mm (probably the same one), > and perform another copy_{to,from}_user() when potentially have > everything we need to perform a larger copy. Thanks, > you are right. Maybe I can first search dma with size of 1 and then check the size of the found dma to perform a larger copy. Thanks Yan > > > + if (ret) > > + break; > > + > > + offset = 0; > > + len -= seg_len; > > + data += seg_len; > > + iova += PAGE_SIZE; > > + } > > + > > + mutex_unlock(&iommu->lock); > > + return ret; > > +} > > + > > static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = { > > .name = "vfio-iommu-type1", > > .owner = THIS_MODULE, > > @@ -2338,6 +2418,7 @@ static const struct vfio_iommu_driver_ops vfio_iommu_driver_ops_type1 = { > > .unpin_pages = vfio_iommu_type1_unpin_pages, > > .register_notifier = vfio_iommu_type1_register_notifier, > > .unregister_notifier = vfio_iommu_type1_unregister_notifier, > > + .iova_rw = vfio_iommu_type1_iova_rw, > > }; > > > > static int __init vfio_iommu_type1_init(void) > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h > > index e42a711a2800..7bf18a31bbcf 100644 > > --- a/include/linux/vfio.h > > +++ b/include/linux/vfio.h > > @@ -82,6 +82,8 @@ struct vfio_iommu_driver_ops { > > struct notifier_block *nb); > > int (*unregister_notifier)(void *iommu_data, > > struct notifier_block *nb); > > + int (*iova_rw)(void *iommu_data, unsigned long iova, > > + void *data, unsigned long len, bool write); > > }; > > > > extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops); > > @@ -107,6 +109,9 @@ extern int vfio_pin_pages(struct device *dev, unsigned long *user_pfn, > > extern int vfio_unpin_pages(struct device *dev, unsigned long *user_pfn, > > int npage); > > > > +extern int vfio_iova_rw(struct device *dev, unsigned long iova, void *data, > > + unsigned long len, bool write); > > + > > /* each type has independent events */ > > enum vfio_notify_type { > > VFIO_IOMMU_NOTIFY = 0, > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/i915/gvt: subsitute kvm_read/write_guest with vfio_iova_rw 2020-01-03 1:00 [PATCH 0/2] use vfio_iova_rw() to read/write IOVAs from CPU side Yan Zhao 2020-01-03 1:02 ` [PATCH 1/2] vfio: introduce vfio_iova_rw to read/write a range of IOVAs Yan Zhao @ 2020-01-03 1:03 ` Yan Zhao 2020-01-07 10:21 ` Paolo Bonzini 1 sibling, 1 reply; 6+ messages in thread From: Yan Zhao @ 2020-01-03 1:03 UTC (permalink / raw) To: zhenyuw Cc: alex.williamson, kvm, linux-kernel, intel-gvt, pbonzini, kevin.tian, Yan Zhao As a device model, it is better to read/write guest memory using vfio interface, so that vfio is able to maintain dirty info of device IOVAs. Compared to CPU side interfaces kvm_read/write_guest(), vfio_iova_rw() has ~600 cycles more overhead on average. ------------------------------------- | interface | avg cpu cycles | |-----------------------------------| | kvm_write_guest | 1546 | | ----------------------------------| | kvm_read_guest | 686 | |-----------------------------------| | vfio_iova_rw(w) | 2233 | |-----------------------------------| | vfio_iova_rw(r) | 1262 | ------------------------------------- Comparison of benchmarks scores are as blow: --------------------------------------------------------- | avg score | kvm_read/write_guest | vfio_iova_rw | --------------------------------------------------------- | Glmark2 | 1132 | 1138.2 | --------------------------------------------------------- | Lightsmark | 61.558 | 61.538 | |-------------------------------------------------------- | OpenArena | 142.77 | 136.6 | --------------------------------------------------------- | Heaven | 698 | 686.8 | -------------------------------------------------------- No obvious performance downgrade found. Cc: Kevin Tian <kevin.tian@intel.com> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- drivers/gpu/drm/i915/gvt/kvmgt.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index bd79a9718cc7..576b05db3998 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -1966,31 +1966,19 @@ static int kvmgt_rw_gpa(unsigned long handle, unsigned long gpa, void *buf, unsigned long len, bool write) { struct kvmgt_guest_info *info; - struct kvm *kvm; - int idx, ret; - bool kthread = current->mm == NULL; + int ret; + struct intel_vgpu *vgpu; + struct device *dev; if (!handle_valid(handle)) return -ESRCH; info = (struct kvmgt_guest_info *)handle; - kvm = info->kvm; - - if (kthread) { - if (!mmget_not_zero(kvm->mm)) - return -EFAULT; - use_mm(kvm->mm); - } - - idx = srcu_read_lock(&kvm->srcu); - ret = write ? kvm_write_guest(kvm, gpa, buf, len) : - kvm_read_guest(kvm, gpa, buf, len); - srcu_read_unlock(&kvm->srcu, idx); + vgpu = info->vgpu; + dev = mdev_dev(vgpu->vdev.mdev); - if (kthread) { - unuse_mm(kvm->mm); - mmput(kvm->mm); - } + ret = write ? vfio_iova_rw(dev, gpa, buf, len, true) : + vfio_iova_rw(dev, gpa, buf, len, false); return ret; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/i915/gvt: subsitute kvm_read/write_guest with vfio_iova_rw 2020-01-03 1:03 ` [PATCH 2/2] drm/i915/gvt: subsitute kvm_read/write_guest with vfio_iova_rw Yan Zhao @ 2020-01-07 10:21 ` Paolo Bonzini 0 siblings, 0 replies; 6+ messages in thread From: Paolo Bonzini @ 2020-01-07 10:21 UTC (permalink / raw) To: Yan Zhao, zhenyuw Cc: alex.williamson, kvm, linux-kernel, intel-gvt, kevin.tian On 03/01/20 02:03, Yan Zhao wrote: > + ret = write ? vfio_iova_rw(dev, gpa, buf, len, true) : > + vfio_iova_rw(dev, gpa, buf, len, false); > > return ret; "Write" can be just the final argument to vfio_iova_rw, that is return vfio_iova_rw(dev, gpa, buf, len, write): Thanks, Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-10 3:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-01-03 1:00 [PATCH 0/2] use vfio_iova_rw() to read/write IOVAs from CPU side Yan Zhao 2020-01-03 1:02 ` [PATCH 1/2] vfio: introduce vfio_iova_rw to read/write a range of IOVAs Yan Zhao 2020-01-09 18:16 ` Alex Williamson 2020-01-10 3:24 ` Yan Zhao 2020-01-03 1:03 ` [PATCH 2/2] drm/i915/gvt: subsitute kvm_read/write_guest with vfio_iova_rw Yan Zhao 2020-01-07 10:21 ` Paolo Bonzini
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).