[PATCH v4 0/5] VDUSE: Support registering userspace memory as bounce buffer

[PATCH v4 0/5] VDUSE: Support registering userspace memory as bounce buffer

[Xie Yongji]

Hi all,

This series introduces some new ioctls: VDUSE_IOTLB_GET_INFO,
VDUSE_IOTLB_REG_UMEM and VDUSE_IOTLB_DEREG_UMEM to support
registering and de-registering userspace memory for IOTLB
as bounce buffer in virtio-vdpa case.

The VDUSE_IOTLB_GET_INFO ioctl can help user to query whether
one IOVA region support userspace memory registration. Then
user can use VDUSE_IOTLB_REG_UMEM and VDUSE_IOTLB_DEREG_UMEM
ioctls to register and de-register userspace memory for this
region.

During registering and de-registering, the DMA data in use
would be copied from kernel bounce pages to userspace bounce
pages and back.

With this feature, some existing application such as SPDK
and DPDK can leverage the datapath of VDUSE directly and
efficiently as discussed before [1][2]. They can register
some preallocated hugepages to VDUSE to avoid an extra
memcpy from bounce-buffer to hugepages.

The kernel and userspace codes could be found in github:

https://github.com/bytedance/linux/tree/vduse-umem
https://github.com/bytedance/qemu/tree/vduse-umem

To test it with qemu-storage-daemon:

$ qemu-storage-daemon \
    --chardev socket,id=charmonitor,path=/tmp/qmp.sock,server=on,wait=off \
    --monitor chardev=charmonitor \
    --blockdev driver=host_device,cache.direct=on,aio=native,filename=/dev/nullb0,node-name=disk0 \
    --export type=vduse-blk,id=vduse-test,name=vduse-test,node-name=disk0,writable=on

[1] https://lkml.org/lkml/2021/6/27/318
[2] https://lkml.org/lkml/2022/7/4/246

Please review, thanks!

V3 to V4:
- Fix the ioctl definition [Jason]
- Rename VDUSE_IOVA_CAP_UMEM_SUPPORT to VDUSE_IOVA_CAP_UMEM [Jason]

V2 to V3:
- Check if the reserved fields is zero [MST]
- Fix some compile errors
- Rework the VDUSE_IOTLB_GET_INFO ioctl to hide some
  internal implementation (bounce buffer) [Jason]
- Add more commit logs for patch 1 [MST]

V1 to V2:
- Drop the patch that updating API version [MST]
- Replace unpin_user_pages() with unpin_user_pages_dirty_lock() [MST]
- Use __vmalloc(__GFP_ACCOUNT) for memory accounting [MST]

Xie Yongji (5):
  vduse: Remove unnecessary spin lock protection
  vduse: Use memcpy_{to,from}_page() in do_bounce()
  vduse: Support using userspace pages as bounce buffer
  vduse: Support registering userspace memory for IOVA regions
  vduse: Support querying information of IOVA regions

 drivers/vdpa/vdpa_user/iova_domain.c | 102 +++++++++++++--
 drivers/vdpa/vdpa_user/iova_domain.h |   8 ++
 drivers/vdpa/vdpa_user/vduse_dev.c   | 180 +++++++++++++++++++++++++++
 include/uapi/linux/vduse.h           |  47 +++++++
 4 files changed, 324 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v4 1/5] vduse: Remove unnecessary spin lock protection

[Xie Yongji]

Now we use domain->iotlb_lock to protect two different
variables: domain->bounce_maps->bounce_page and
domain->iotlb. But for domain->bounce_maps->bounce_page,
we actually don't need any synchronization between
vduse_domain_get_bounce_page() and vduse_domain_free_bounce_pages()
since vduse_domain_get_bounce_page() will only be called in
page fault handler and vduse_domain_free_bounce_pages() will
be called during file release.

So let's remove the unnecessary spin lock protection in
vduse_domain_get_bounce_page(). Then the usage of
domain->iotlb_lock could be more clear: the lock will be
only used to protect the domain->iotlb.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/vdpa_user/iova_domain.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/iova_domain.c b/drivers/vdpa/vdpa_user/iova_domain.c
index 6daa3978d290..bca1f0b8850c 100644
--- a/drivers/vdpa/vdpa_user/iova_domain.c
+++ b/drivers/vdpa/vdpa_user/iova_domain.c
@@ -211,17 +211,14 @@ static struct page *
 vduse_domain_get_bounce_page(struct vduse_iova_domain *domain, u64 iova)
 {
 	struct vduse_bounce_map *map;
-	struct page *page = NULL;
+	struct page *page;
 
-	spin_lock(&domain->iotlb_lock);
 	map = &domain->bounce_maps[iova >> PAGE_SHIFT];
 	if (!map->bounce_page)
-		goto out;
+		return NULL;
 
 	page = map->bounce_page;
 	get_page(page);
-out:
-	spin_unlock(&domain->iotlb_lock);
 
 	return page;
 }
-- 
2.20.1

[PATCH v4 2/5] vduse: Use memcpy_{to,from}_page() in do_bounce()

[Xie Yongji]

kmap_atomic() is being deprecated in favor of kmap_local_page().

The use of kmap_atomic() in do_bounce() is all thread local therefore
kmap_local_page() is a sufficient replacement.

Convert to kmap_local_page() but, instead of open coding it,
use the helpers memcpy_to_page() and memcpy_from_page().

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/vdpa_user/iova_domain.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/iova_domain.c b/drivers/vdpa/vdpa_user/iova_domain.c
index bca1f0b8850c..50d7c08d5450 100644
--- a/drivers/vdpa/vdpa_user/iova_domain.c
+++ b/drivers/vdpa/vdpa_user/iova_domain.c
@@ -138,18 +138,17 @@ static void do_bounce(phys_addr_t orig, void *addr, size_t size,
 {
 	unsigned long pfn = PFN_DOWN(orig);
 	unsigned int offset = offset_in_page(orig);
-	char *buffer;
+	struct page *page;
 	unsigned int sz = 0;
 
 	while (size) {
 		sz = min_t(size_t, PAGE_SIZE - offset, size);
 
-		buffer = kmap_atomic(pfn_to_page(pfn));
+		page = pfn_to_page(pfn);
 		if (dir == DMA_TO_DEVICE)
-			memcpy(addr, buffer + offset, sz);
+			memcpy_from_page(addr, page, offset, sz);
 		else
-			memcpy(buffer + offset, addr, sz);
-		kunmap_atomic(buffer);
+			memcpy_to_page(page, offset, addr, sz);
 
 		size -= sz;
 		pfn++;
-- 
2.20.1

Re: [PATCH v4 2/5] vduse: Use memcpy_{to,from}_page() in do_bounce()

[Muchun Song]

On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
>
> kmap_atomic() is being deprecated in favor of kmap_local_page().
>
> The use of kmap_atomic() in do_bounce() is all thread local therefore
> kmap_local_page() is a sufficient replacement.
>
> Convert to kmap_local_page() but, instead of open coding it,
> use the helpers memcpy_to_page() and memcpy_from_page().
>
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> Acked-by: Jason Wang <jasowang@redhat.com>

Reviewed-by: Muchun Song <songmuchun@bytedance.com>

[PATCH v4 3/5] vduse: Support using userspace pages as bounce buffer

[Xie Yongji]

Introduce two APIs: vduse_domain_add_user_bounce_pages()
and vduse_domain_remove_user_bounce_pages() to support
adding and removing userspace pages for bounce buffers.
During adding and removing, the DMA data would be copied
from the kernel bounce pages to the userspace bounce pages
and back.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/vdpa_user/iova_domain.c | 96 +++++++++++++++++++++++++---
 drivers/vdpa/vdpa_user/iova_domain.h |  8 +++
 2 files changed, 96 insertions(+), 8 deletions(-)

diff --git a/drivers/vdpa/vdpa_user/iova_domain.c b/drivers/vdpa/vdpa_user/iova_domain.c
index 50d7c08d5450..e682bc7ee6c9 100644
--- a/drivers/vdpa/vdpa_user/iova_domain.c
+++ b/drivers/vdpa/vdpa_user/iova_domain.c
@@ -178,8 +178,9 @@ static void vduse_domain_bounce(struct vduse_iova_domain *domain,
 			    map->orig_phys == INVALID_PHYS_ADDR))
 			return;
 
-		addr = page_address(map->bounce_page) + offset;
-		do_bounce(map->orig_phys + offset, addr, sz, dir);
+		addr = kmap_local_page(map->bounce_page);
+		do_bounce(map->orig_phys + offset, addr + offset, sz, dir);
+		kunmap_local(addr);
 		size -= sz;
 		iova += sz;
 	}
@@ -210,20 +211,23 @@ static struct page *
 vduse_domain_get_bounce_page(struct vduse_iova_domain *domain, u64 iova)
 {
 	struct vduse_bounce_map *map;
-	struct page *page;
+	struct page *page = NULL;
 
+	read_lock(&domain->bounce_lock);
 	map = &domain->bounce_maps[iova >> PAGE_SHIFT];
-	if (!map->bounce_page)
-		return NULL;
+	if (domain->user_bounce_pages || !map->bounce_page)
+		goto out;
 
 	page = map->bounce_page;
 	get_page(page);
+out:
+	read_unlock(&domain->bounce_lock);
 
 	return page;
 }
 
 static void
-vduse_domain_free_bounce_pages(struct vduse_iova_domain *domain)
+vduse_domain_free_kernel_bounce_pages(struct vduse_iova_domain *domain)
 {
 	struct vduse_bounce_map *map;
 	unsigned long pfn, bounce_pfns;
@@ -243,6 +247,73 @@ vduse_domain_free_bounce_pages(struct vduse_iova_domain *domain)
 	}
 }
 
+int vduse_domain_add_user_bounce_pages(struct vduse_iova_domain *domain,
+				       struct page **pages, int count)
+{
+	struct vduse_bounce_map *map;
+	int i, ret;
+
+	/* Now we don't support partial mapping */
+	if (count != (domain->bounce_size >> PAGE_SHIFT))
+		return -EINVAL;
+
+	write_lock(&domain->bounce_lock);
+	ret = -EEXIST;
+	if (domain->user_bounce_pages)
+		goto out;
+
+	for (i = 0; i < count; i++) {
+		map = &domain->bounce_maps[i];
+		if (map->bounce_page) {
+			/* Copy kernel page to user page if it's in use */
+			if (map->orig_phys != INVALID_PHYS_ADDR)
+				memcpy_to_page(pages[i], 0,
+					       page_address(map->bounce_page),
+					       PAGE_SIZE);
+			__free_page(map->bounce_page);
+		}
+		map->bounce_page = pages[i];
+		get_page(pages[i]);
+	}
+	domain->user_bounce_pages = true;
+	ret = 0;
+out:
+	write_unlock(&domain->bounce_lock);
+
+	return ret;
+}
+
+void vduse_domain_remove_user_bounce_pages(struct vduse_iova_domain *domain)
+{
+	struct vduse_bounce_map *map;
+	unsigned long i, count;
+
+	write_lock(&domain->bounce_lock);
+	if (!domain->user_bounce_pages)
+		goto out;
+
+	count = domain->bounce_size >> PAGE_SHIFT;
+	for (i = 0; i < count; i++) {
+		struct page *page = NULL;
+
+		map = &domain->bounce_maps[i];
+		if (WARN_ON(!map->bounce_page))
+			continue;
+
+		/* Copy user page to kernel page if it's in use */
+		if (map->orig_phys != INVALID_PHYS_ADDR) {
+			page = alloc_page(GFP_ATOMIC | __GFP_NOFAIL);
+			memcpy_from_page(page_address(page),
+					 map->bounce_page, 0, PAGE_SIZE);
+		}
+		put_page(map->bounce_page);
+		map->bounce_page = page;
+	}
+	domain->user_bounce_pages = false;
+out:
+	write_unlock(&domain->bounce_lock);
+}
+
 void vduse_domain_reset_bounce_map(struct vduse_iova_domain *domain)
 {
 	if (!domain->bounce_map)
@@ -318,13 +389,18 @@ dma_addr_t vduse_domain_map_page(struct vduse_iova_domain *domain,
 	if (vduse_domain_init_bounce_map(domain))
 		goto err;
 
+	read_lock(&domain->bounce_lock);
 	if (vduse_domain_map_bounce_page(domain, (u64)iova, (u64)size, pa))
-		goto err;
+		goto err_unlock;
 
 	if (dir == DMA_TO_DEVICE || dir == DMA_BIDIRECTIONAL)
 		vduse_domain_bounce(domain, iova, size, DMA_TO_DEVICE);
 
+	read_unlock(&domain->bounce_lock);
+
 	return iova;
+err_unlock:
+	read_unlock(&domain->bounce_lock);
 err:
 	vduse_domain_free_iova(iovad, iova, size);
 	return DMA_MAPPING_ERROR;
@@ -336,10 +412,12 @@ void vduse_domain_unmap_page(struct vduse_iova_domain *domain,
 {
 	struct iova_domain *iovad = &domain->stream_iovad;
 
+	read_lock(&domain->bounce_lock);
 	if (dir == DMA_FROM_DEVICE || dir == DMA_BIDIRECTIONAL)
 		vduse_domain_bounce(domain, dma_addr, size, DMA_FROM_DEVICE);
 
 	vduse_domain_unmap_bounce_page(domain, (u64)dma_addr, (u64)size);
+	read_unlock(&domain->bounce_lock);
 	vduse_domain_free_iova(iovad, dma_addr, size);
 }
 
@@ -447,7 +525,8 @@ static int vduse_domain_release(struct inode *inode, struct file *file)
 
 	spin_lock(&domain->iotlb_lock);
 	vduse_iotlb_del_range(domain, 0, ULLONG_MAX);
-	vduse_domain_free_bounce_pages(domain);
+	vduse_domain_remove_user_bounce_pages(domain);
+	vduse_domain_free_kernel_bounce_pages(domain);
 	spin_unlock(&domain->iotlb_lock);
 	put_iova_domain(&domain->stream_iovad);
 	put_iova_domain(&domain->consistent_iovad);
@@ -507,6 +586,7 @@ vduse_domain_create(unsigned long iova_limit, size_t bounce_size)
 		goto err_file;
 
 	domain->file = file;
+	rwlock_init(&domain->bounce_lock);
 	spin_lock_init(&domain->iotlb_lock);
 	init_iova_domain(&domain->stream_iovad,
 			PAGE_SIZE, IOVA_START_PFN);
diff --git a/drivers/vdpa/vdpa_user/iova_domain.h b/drivers/vdpa/vdpa_user/iova_domain.h
index 2722d9b8e21a..4e0e50e7ac15 100644
--- a/drivers/vdpa/vdpa_user/iova_domain.h
+++ b/drivers/vdpa/vdpa_user/iova_domain.h
@@ -14,6 +14,7 @@
 #include <linux/iova.h>
 #include <linux/dma-mapping.h>
 #include <linux/vhost_iotlb.h>
+#include <linux/rwlock.h>
 
 #define IOVA_START_PFN 1
 
@@ -34,6 +35,8 @@ struct vduse_iova_domain {
 	struct vhost_iotlb *iotlb;
 	spinlock_t iotlb_lock;
 	struct file *file;
+	bool user_bounce_pages;
+	rwlock_t bounce_lock;
 };
 
 int vduse_domain_set_map(struct vduse_iova_domain *domain,
@@ -61,6 +64,11 @@ void vduse_domain_free_coherent(struct vduse_iova_domain *domain, size_t size,
 
 void vduse_domain_reset_bounce_map(struct vduse_iova_domain *domain);
 
+int vduse_domain_add_user_bounce_pages(struct vduse_iova_domain *domain,
+				       struct page **pages, int count);
+
+void vduse_domain_remove_user_bounce_pages(struct vduse_iova_domain *domain);
+
 void vduse_domain_destroy(struct vduse_iova_domain *domain);
 
 struct vduse_iova_domain *vduse_domain_create(unsigned long iova_limit,
-- 
2.20.1

[PATCH v4 4/5] vduse: Support registering userspace memory for IOVA regions

[Xie Yongji]

Introduce two ioctls: VDUSE_IOTLB_REG_UMEM and
VDUSE_IOTLB_DEREG_UMEM to support registering
and de-registering userspace memory for IOVA
regions.

Now it only supports registering userspace memory
for bounce buffer region in virtio-vdpa case.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 141 +++++++++++++++++++++++++++++
 include/uapi/linux/vduse.h         |  23 +++++
 2 files changed, 164 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 3bc27de58f46..eedff0a3885a 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -21,6 +21,8 @@
 #include <linux/uio.h>
 #include <linux/vdpa.h>
 #include <linux/nospec.h>
+#include <linux/vmalloc.h>
+#include <linux/sched/mm.h>
 #include <uapi/linux/vduse.h>
 #include <uapi/linux/vdpa.h>
 #include <uapi/linux/virtio_config.h>
@@ -64,6 +66,13 @@ struct vduse_vdpa {
 	struct vduse_dev *dev;
 };
 
+struct vduse_umem {
+	unsigned long iova;
+	unsigned long npages;
+	struct page **pages;
+	struct mm_struct *mm;
+};
+
 struct vduse_dev {
 	struct vduse_vdpa *vdev;
 	struct device *dev;
@@ -95,6 +104,8 @@ struct vduse_dev {
 	u8 status;
 	u32 vq_num;
 	u32 vq_align;
+	struct vduse_umem *umem;
+	struct mutex mem_lock;
 };
 
 struct vduse_dev_msg {
@@ -917,6 +928,102 @@ static int vduse_dev_queue_irq_work(struct vduse_dev *dev,
 	return ret;
 }
 
+static int vduse_dev_dereg_umem(struct vduse_dev *dev,
+				u64 iova, u64 size)
+{
+	int ret;
+
+	mutex_lock(&dev->mem_lock);
+	ret = -ENOENT;
+	if (!dev->umem)
+		goto unlock;
+
+	ret = -EINVAL;
+	if (dev->umem->iova != iova || size != dev->domain->bounce_size)
+		goto unlock;
+
+	vduse_domain_remove_user_bounce_pages(dev->domain);
+	unpin_user_pages_dirty_lock(dev->umem->pages,
+				    dev->umem->npages, true);
+	atomic64_sub(dev->umem->npages, &dev->umem->mm->pinned_vm);
+	mmdrop(dev->umem->mm);
+	vfree(dev->umem->pages);
+	kfree(dev->umem);
+	dev->umem = NULL;
+	ret = 0;
+unlock:
+	mutex_unlock(&dev->mem_lock);
+	return ret;
+}
+
+static int vduse_dev_reg_umem(struct vduse_dev *dev,
+			      u64 iova, u64 uaddr, u64 size)
+{
+	struct page **page_list = NULL;
+	struct vduse_umem *umem = NULL;
+	long pinned = 0;
+	unsigned long npages, lock_limit;
+	int ret;
+
+	if (!dev->domain->bounce_map ||
+	    size != dev->domain->bounce_size ||
+	    iova != 0 || uaddr & ~PAGE_MASK)
+		return -EINVAL;
+
+	mutex_lock(&dev->mem_lock);
+	ret = -EEXIST;
+	if (dev->umem)
+		goto unlock;
+
+	ret = -ENOMEM;
+	npages = size >> PAGE_SHIFT;
+	page_list = __vmalloc(array_size(npages, sizeof(struct page *)),
+			      GFP_KERNEL_ACCOUNT);
+	umem = kzalloc(sizeof(*umem), GFP_KERNEL);
+	if (!page_list || !umem)
+		goto unlock;
+
+	mmap_read_lock(current->mm);
+
+	lock_limit = PFN_DOWN(rlimit(RLIMIT_MEMLOCK));
+	if (npages + atomic64_read(&current->mm->pinned_vm) > lock_limit)
+		goto out;
+
+	pinned = pin_user_pages(uaddr, npages, FOLL_LONGTERM | FOLL_WRITE,
+				page_list, NULL);
+	if (pinned != npages) {
+		ret = pinned < 0 ? pinned : -ENOMEM;
+		goto out;
+	}
+
+	ret = vduse_domain_add_user_bounce_pages(dev->domain,
+						 page_list, pinned);
+	if (ret)
+		goto out;
+
+	atomic64_add(npages, &current->mm->pinned_vm);
+
+	umem->pages = page_list;
+	umem->npages = pinned;
+	umem->iova = iova;
+	umem->mm = current->mm;
+	mmgrab(current->mm);
+
+	dev->umem = umem;
+out:
+	if (ret && pinned > 0)
+		unpin_user_pages(page_list, pinned);
+
+	mmap_read_unlock(current->mm);
+unlock:
+	if (ret) {
+		vfree(page_list);
+		kfree(umem);
+	}
+	mutex_unlock(&dev->mem_lock);
+	return ret;
+}
+
 static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg)
 {
@@ -1089,6 +1196,38 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 		ret = vduse_dev_queue_irq_work(dev, &dev->vqs[index].inject);
 		break;
 	}
+	case VDUSE_IOTLB_REG_UMEM: {
+		struct vduse_iova_umem umem;
+
+		ret = -EFAULT;
+		if (copy_from_user(&umem, argp, sizeof(umem)))
+			break;
+
+		ret = -EINVAL;
+		if (!is_mem_zero((const char *)umem.reserved,
+				 sizeof(umem.reserved)))
+			break;
+
+		ret = vduse_dev_reg_umem(dev, umem.iova,
+					 umem.uaddr, umem.size);
+		break;
+	}
+	case VDUSE_IOTLB_DEREG_UMEM: {
+		struct vduse_iova_umem umem;
+
+		ret = -EFAULT;
+		if (copy_from_user(&umem, argp, sizeof(umem)))
+			break;
+
+		ret = -EINVAL;
+		if (!is_mem_zero((const char *)umem.reserved,
+				 sizeof(umem.reserved)))
+			break;
+
+		ret = vduse_dev_dereg_umem(dev, umem.iova,
+					   umem.size);
+		break;
+	}
 	default:
 		ret = -ENOIOCTLCMD;
 		break;
@@ -1101,6 +1240,7 @@ static int vduse_dev_release(struct inode *inode, struct file *file)
 {
 	struct vduse_dev *dev = file->private_data;
 
+	vduse_dev_dereg_umem(dev, 0, dev->domain->bounce_size);
 	spin_lock(&dev->msg_lock);
 	/* Make sure the inflight messages can processed after reconncection */
 	list_splice_init(&dev->recv_list, &dev->send_list);
@@ -1163,6 +1303,7 @@ static struct vduse_dev *vduse_dev_create(void)
 		return NULL;
 
 	mutex_init(&dev->lock);
+	mutex_init(&dev->mem_lock);
 	spin_lock_init(&dev->msg_lock);
 	INIT_LIST_HEAD(&dev->send_list);
 	INIT_LIST_HEAD(&dev->recv_list);
diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
index 7cfe1c1280c0..9885e0571f09 100644
--- a/include/uapi/linux/vduse.h
+++ b/include/uapi/linux/vduse.h
@@ -210,6 +210,29 @@ struct vduse_vq_eventfd {
  */
 #define VDUSE_VQ_INJECT_IRQ	_IOW(VDUSE_BASE, 0x17, __u32)
 
+/**
+ * struct vduse_iova_umem - userspace memory configuration for one IOVA region
+ * @uaddr: start address of userspace memory, it must be aligned to page size
+ * @iova: start of the IOVA region
+ * @size: size of the IOVA region
+ * @reserved: for future use, needs to be initialized to zero
+ *
+ * Structure used by VDUSE_IOTLB_REG_UMEM and VDUSE_IOTLB_DEREG_UMEM
+ * ioctls to register/de-register userspace memory for IOVA regions
+ */
+struct vduse_iova_umem {
+	__u64 uaddr;
+	__u64 iova;
+	__u64 size;
+	__u64 reserved[3];
+};
+
+/* Register userspace memory for IOVA regions */
+#define VDUSE_IOTLB_REG_UMEM	_IOW(VDUSE_BASE, 0x18, struct vduse_iova_umem)
+
+/* De-register the userspace memory. Caller should set iova and size field. */
+#define VDUSE_IOTLB_DEREG_UMEM	_IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
+
 /* The control messages definition for read(2)/write(2) on /dev/vduse/$NAME */
 
 /**
-- 
2.20.1

[PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Xie Yongji]

This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
support querying some information of IOVA regions.

Now it can be used to query whether the IOVA region
supports userspace memory registration.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
 include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index eedff0a3885a..e820c37dcba8 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 					   umem.size);
 		break;
 	}
+	case VDUSE_IOTLB_GET_INFO: {
+		struct vduse_iova_info info;
+		struct vhost_iotlb_map *map;
+		struct vduse_iova_domain *domain = dev->domain;
+
+		ret = -EFAULT;
+		if (copy_from_user(&info, argp, sizeof(info)))
+			break;
+
+		ret = -EINVAL;
+		if (info.start > info.last)
+			break;
+
+		if (!is_mem_zero((const char *)info.reserved,
+				 sizeof(info.reserved)))
+			break;
+
+		spin_lock(&domain->iotlb_lock);
+		map = vhost_iotlb_itree_first(domain->iotlb,
+					      info.start, info.last);
+		if (map) {
+			info.start = map->start;
+			info.last = map->last;
+			info.capability = 0;
+			if (domain->bounce_map && map->start >= 0 &&
+			    map->last < domain->bounce_size)
+				info.capability |= VDUSE_IOVA_CAP_UMEM;
+		}
+		spin_unlock(&domain->iotlb_lock);
+		if (!map)
+			break;
+
+		ret = -EFAULT;
+		if (copy_to_user(argp, &info, sizeof(info)))
+			break;
+
+		ret = 0;
+		break;
+	}
 	default:
 		ret = -ENOIOCTLCMD;
 		break;
diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
index 9885e0571f09..11bd48c72c6c 100644
--- a/include/uapi/linux/vduse.h
+++ b/include/uapi/linux/vduse.h
@@ -233,6 +233,30 @@ struct vduse_iova_umem {
 /* De-register the userspace memory. Caller should set iova and size field. */
 #define VDUSE_IOTLB_DEREG_UMEM	_IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
 
+/**
+ * struct vduse_iova_info - information of one IOVA region
+ * @start: start of the IOVA region
+ * @last: last of the IOVA region
+ * @capability: capability of the IOVA regsion
+ * @reserved: for future use, needs to be initialized to zero
+ *
+ * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
+ * one IOVA region.
+ */
+struct vduse_iova_info {
+	__u64 start;
+	__u64 last;
+#define VDUSE_IOVA_CAP_UMEM (1 << 0)
+	__u64 capability;
+	__u64 reserved[3];
+};
+
+/*
+ * Find the first IOVA region that overlaps with the range [start, last]
+ * and return some information on it. Caller should set start and last fields.
+ */
+#define VDUSE_IOTLB_GET_INFO	_IOWR(VDUSE_BASE, 0x1a, struct vduse_iova_info)
+
 /* The control messages definition for read(2)/write(2) on /dev/vduse/$NAME */
 
 /**
-- 
2.20.1

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Jason Wang]

On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
>
> This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> support querying some information of IOVA regions.
>
> Now it can be used to query whether the IOVA region
> supports userspace memory registration.
>
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
>  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index eedff0a3885a..e820c37dcba8 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                                            umem.size);
>                 break;
>         }
> +       case VDUSE_IOTLB_GET_INFO: {
> +               struct vduse_iova_info info;
> +               struct vhost_iotlb_map *map;
> +               struct vduse_iova_domain *domain = dev->domain;
> +
> +               ret = -EFAULT;
> +               if (copy_from_user(&info, argp, sizeof(info)))
> +                       break;
> +
> +               ret = -EINVAL;
> +               if (info.start > info.last)
> +                       break;
> +
> +               if (!is_mem_zero((const char *)info.reserved,
> +                                sizeof(info.reserved)))
> +                       break;
> +
> +               spin_lock(&domain->iotlb_lock);
> +               map = vhost_iotlb_itree_first(domain->iotlb,
> +                                             info.start, info.last);
> +               if (map) {
> +                       info.start = map->start;
> +                       info.last = map->last;
> +                       info.capability = 0;
> +                       if (domain->bounce_map && map->start >= 0 &&
> +                           map->last < domain->bounce_size)
> +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> +               }
> +               spin_unlock(&domain->iotlb_lock);
> +               if (!map)
> +                       break;
> +
> +               ret = -EFAULT;
> +               if (copy_to_user(argp, &info, sizeof(info)))
> +                       break;
> +
> +               ret = 0;
> +               break;
> +       }
>         default:
>                 ret = -ENOIOCTLCMD;
>                 break;
> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> index 9885e0571f09..11bd48c72c6c 100644
> --- a/include/uapi/linux/vduse.h
> +++ b/include/uapi/linux/vduse.h
> @@ -233,6 +233,30 @@ struct vduse_iova_umem {
>  /* De-register the userspace memory. Caller should set iova and size field. */
>  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
>
> +/**
> + * struct vduse_iova_info - information of one IOVA region
> + * @start: start of the IOVA region
> + * @last: last of the IOVA region
> + * @capability: capability of the IOVA regsion
> + * @reserved: for future use, needs to be initialized to zero
> + *
> + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> + * one IOVA region.
> + */
> +struct vduse_iova_info {
> +       __u64 start;
> +       __u64 last;
> +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> +       __u64 capability;
> +       __u64 reserved[3];
> +};
> +
> +/*
> + * Find the first IOVA region that overlaps with the range [start, last]

So the code is actually find the IOVA region that is the super range
of [start, last] instead of overlap:


> +                       if (domain->bounce_map && map->start >= 0 &&
> +                           map->last < domain->bounce_size)
> +                               info.capability |= VDUSE_IOVA_CAP_UMEM;

Which part is wrong?

Thanks

> + * and return some information on it. Caller should set start and last fields.
> + */
> +#define VDUSE_IOTLB_GET_INFO   _IOWR(VDUSE_BASE, 0x1a, struct vduse_iova_info)
> +
>  /* The control messages definition for read(2)/write(2) on /dev/vduse/$NAME */
>
>  /**
> --
> 2.20.1
>

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Jason Wang]

On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
>
> This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> support querying some information of IOVA regions.
>
> Now it can be used to query whether the IOVA region
> supports userspace memory registration.
>
> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> ---
>  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
>  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> index eedff0a3885a..e820c37dcba8 100644
> --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
>                                            umem.size);
>                 break;
>         }
> +       case VDUSE_IOTLB_GET_INFO: {
> +               struct vduse_iova_info info;
> +               struct vhost_iotlb_map *map;
> +               struct vduse_iova_domain *domain = dev->domain;
> +
> +               ret = -EFAULT;
> +               if (copy_from_user(&info, argp, sizeof(info)))
> +                       break;
> +
> +               ret = -EINVAL;
> +               if (info.start > info.last)
> +                       break;
> +
> +               if (!is_mem_zero((const char *)info.reserved,
> +                                sizeof(info.reserved)))
> +                       break;
> +
> +               spin_lock(&domain->iotlb_lock);
> +               map = vhost_iotlb_itree_first(domain->iotlb,
> +                                             info.start, info.last);
> +               if (map) {
> +                       info.start = map->start;
> +                       info.last = map->last;
> +                       info.capability = 0;
> +                       if (domain->bounce_map && map->start >= 0 &&
> +                           map->last < domain->bounce_size)
> +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> +               }
> +               spin_unlock(&domain->iotlb_lock);
> +               if (!map)
> +                       break;
> +
> +               ret = -EFAULT;
> +               if (copy_to_user(argp, &info, sizeof(info)))
> +                       break;
> +
> +               ret = 0;
> +               break;
> +       }
>         default:
>                 ret = -ENOIOCTLCMD;
>                 break;
> diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> index 9885e0571f09..11bd48c72c6c 100644
> --- a/include/uapi/linux/vduse.h
> +++ b/include/uapi/linux/vduse.h
> @@ -233,6 +233,30 @@ struct vduse_iova_umem {
>  /* De-register the userspace memory. Caller should set iova and size field. */
>  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
>
> +/**
> + * struct vduse_iova_info - information of one IOVA region
> + * @start: start of the IOVA region
> + * @last: last of the IOVA region
> + * @capability: capability of the IOVA regsion
> + * @reserved: for future use, needs to be initialized to zero
> + *
> + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> + * one IOVA region.
> + */
> +struct vduse_iova_info {
> +       __u64 start;
> +       __u64 last;
> +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> +       __u64 capability;
> +       __u64 reserved[3];
> +};
> +
> +/*
> + * Find the first IOVA region that overlaps with the range [start, last]

So the code is actually find the IOVA region that is the super range
of [start, last] instead of overlap:


> +                       if (domain->bounce_map && map->start >= 0 &&
> +                           map->last < domain->bounce_size)
> +                               info.capability |= VDUSE_IOVA_CAP_UMEM;

Which part is wrong?

Thanks

> + * and return some information on it. Caller should set start and last fields.
> + */
> +#define VDUSE_IOTLB_GET_INFO   _IOWR(VDUSE_BASE, 0x1a, struct vduse_iova_info)
> +
>  /* The control messages definition for read(2)/write(2) on /dev/vduse/$NAME */
>
>  /**
> --
> 2.20.1
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Yongji Xie]

On Thu, Jul 28, 2022 at 1:58 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
> >
> > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > support querying some information of IOVA regions.
> >
> > Now it can be used to query whether the IOVA region
> > supports userspace memory registration.
> >
> > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > ---
> >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
> >  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
> >  2 files changed, 63 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > index eedff0a3885a..e820c37dcba8 100644
> > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> >                                            umem.size);
> >                 break;
> >         }
> > +       case VDUSE_IOTLB_GET_INFO: {
> > +               struct vduse_iova_info info;
> > +               struct vhost_iotlb_map *map;
> > +               struct vduse_iova_domain *domain = dev->domain;
> > +
> > +               ret = -EFAULT;
> > +               if (copy_from_user(&info, argp, sizeof(info)))
> > +                       break;
> > +
> > +               ret = -EINVAL;
> > +               if (info.start > info.last)
> > +                       break;
> > +
> > +               if (!is_mem_zero((const char *)info.reserved,
> > +                                sizeof(info.reserved)))
> > +                       break;
> > +
> > +               spin_lock(&domain->iotlb_lock);
> > +               map = vhost_iotlb_itree_first(domain->iotlb,
> > +                                             info.start, info.last);
> > +               if (map) {
> > +                       info.start = map->start;
> > +                       info.last = map->last;
> > +                       info.capability = 0;
> > +                       if (domain->bounce_map && map->start >= 0 &&
> > +                           map->last < domain->bounce_size)
> > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> > +               }
> > +               spin_unlock(&domain->iotlb_lock);
> > +               if (!map)
> > +                       break;
> > +
> > +               ret = -EFAULT;
> > +               if (copy_to_user(argp, &info, sizeof(info)))
> > +                       break;
> > +
> > +               ret = 0;
> > +               break;
> > +       }
> >         default:
> >                 ret = -ENOIOCTLCMD;
> >                 break;
> > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > index 9885e0571f09..11bd48c72c6c 100644
> > --- a/include/uapi/linux/vduse.h
> > +++ b/include/uapi/linux/vduse.h
> > @@ -233,6 +233,30 @@ struct vduse_iova_umem {
> >  /* De-register the userspace memory. Caller should set iova and size field. */
> >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
> >
> > +/**
> > + * struct vduse_iova_info - information of one IOVA region
> > + * @start: start of the IOVA region
> > + * @last: last of the IOVA region
> > + * @capability: capability of the IOVA regsion
> > + * @reserved: for future use, needs to be initialized to zero
> > + *
> > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > + * one IOVA region.
> > + */
> > +struct vduse_iova_info {
> > +       __u64 start;
> > +       __u64 last;
> > +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> > +       __u64 capability;
> > +       __u64 reserved[3];
> > +};
> > +
> > +/*
> > + * Find the first IOVA region that overlaps with the range [start, last]
>
> So the code is actually find the IOVA region that is the super range
> of [start, last] instead of overlap:
>

This is achieved by vhost_iotlb_itree_first(). And can't the super
range of [start,last] be considered overlapping?

>
> > +                       if (domain->bounce_map && map->start >= 0 &&
> > +                           map->last < domain->bounce_size)
> > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
>
> Which part is wrong?
>

We will first call vhost_iotlb_itree_first() which will find the first
IOVA region that overlaps with the range [start, last]. Then the flag
will only be set if the IOVA region is within the bounce range.

Thanks,
Yongji

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Jason Wang]

On Thu, Jul 28, 2022 at 2:36 PM Yongji Xie <xieyongji@bytedance.com> wrote:
>
> On Thu, Jul 28, 2022 at 1:58 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
> > >
> > > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > > support querying some information of IOVA regions.
> > >
> > > Now it can be used to query whether the IOVA region
> > > supports userspace memory registration.
> > >
> > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > ---
> > >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
> > >  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
> > >  2 files changed, 63 insertions(+)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index eedff0a3885a..e820c37dcba8 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > >                                            umem.size);
> > >                 break;
> > >         }
> > > +       case VDUSE_IOTLB_GET_INFO: {
> > > +               struct vduse_iova_info info;
> > > +               struct vhost_iotlb_map *map;
> > > +               struct vduse_iova_domain *domain = dev->domain;
> > > +
> > > +               ret = -EFAULT;
> > > +               if (copy_from_user(&info, argp, sizeof(info)))
> > > +                       break;
> > > +
> > > +               ret = -EINVAL;
> > > +               if (info.start > info.last)
> > > +                       break;
> > > +
> > > +               if (!is_mem_zero((const char *)info.reserved,
> > > +                                sizeof(info.reserved)))
> > > +                       break;
> > > +
> > > +               spin_lock(&domain->iotlb_lock);
> > > +               map = vhost_iotlb_itree_first(domain->iotlb,
> > > +                                             info.start, info.last);
> > > +               if (map) {
> > > +                       info.start = map->start;
> > > +                       info.last = map->last;
> > > +                       info.capability = 0;
> > > +                       if (domain->bounce_map && map->start >= 0 &&
> > > +                           map->last < domain->bounce_size)
> > > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> > > +               }
> > > +               spin_unlock(&domain->iotlb_lock);
> > > +               if (!map)
> > > +                       break;
> > > +
> > > +               ret = -EFAULT;
> > > +               if (copy_to_user(argp, &info, sizeof(info)))
> > > +                       break;
> > > +
> > > +               ret = 0;
> > > +               break;
> > > +       }
> > >         default:
> > >                 ret = -ENOIOCTLCMD;
> > >                 break;
> > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > index 9885e0571f09..11bd48c72c6c 100644
> > > --- a/include/uapi/linux/vduse.h
> > > +++ b/include/uapi/linux/vduse.h
> > > @@ -233,6 +233,30 @@ struct vduse_iova_umem {
> > >  /* De-register the userspace memory. Caller should set iova and size field. */
> > >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
> > >
> > > +/**
> > > + * struct vduse_iova_info - information of one IOVA region
> > > + * @start: start of the IOVA region
> > > + * @last: last of the IOVA region
> > > + * @capability: capability of the IOVA regsion
> > > + * @reserved: for future use, needs to be initialized to zero
> > > + *
> > > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > > + * one IOVA region.
> > > + */
> > > +struct vduse_iova_info {
> > > +       __u64 start;
> > > +       __u64 last;
> > > +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> > > +       __u64 capability;
> > > +       __u64 reserved[3];
> > > +};
> > > +
> > > +/*
> > > + * Find the first IOVA region that overlaps with the range [start, last]
> >
> > So the code is actually find the IOVA region that is the super range
> > of [start, last] instead of overlap:
> >
>
> This is achieved by vhost_iotlb_itree_first(). And can't the super
> range of [start,last] be considered overlapping?

Ok, but what I want to ask is, under which condition can we hit the
following case

map->last >= domain->bounce_size ?

Thanks

>
> >
> > > +                       if (domain->bounce_map && map->start >= 0 &&
> > > +                           map->last < domain->bounce_size)
> > > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> >
> > Which part is wrong?
> >
>
> We will first call vhost_iotlb_itree_first() which will find the first
> IOVA region that overlaps with the range [start, last]. Then the flag
> will only be set if the IOVA region is within the bounce range.
>
> Thanks,
> Yongji
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Jason Wang]

On Thu, Jul 28, 2022 at 2:36 PM Yongji Xie <xieyongji@bytedance.com> wrote:
>
> On Thu, Jul 28, 2022 at 1:58 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
> > >
> > > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > > support querying some information of IOVA regions.
> > >
> > > Now it can be used to query whether the IOVA region
> > > supports userspace memory registration.
> > >
> > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > ---
> > >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
> > >  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
> > >  2 files changed, 63 insertions(+)
> > >
> > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > index eedff0a3885a..e820c37dcba8 100644
> > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > >                                            umem.size);
> > >                 break;
> > >         }
> > > +       case VDUSE_IOTLB_GET_INFO: {
> > > +               struct vduse_iova_info info;
> > > +               struct vhost_iotlb_map *map;
> > > +               struct vduse_iova_domain *domain = dev->domain;
> > > +
> > > +               ret = -EFAULT;
> > > +               if (copy_from_user(&info, argp, sizeof(info)))
> > > +                       break;
> > > +
> > > +               ret = -EINVAL;
> > > +               if (info.start > info.last)
> > > +                       break;
> > > +
> > > +               if (!is_mem_zero((const char *)info.reserved,
> > > +                                sizeof(info.reserved)))
> > > +                       break;
> > > +
> > > +               spin_lock(&domain->iotlb_lock);
> > > +               map = vhost_iotlb_itree_first(domain->iotlb,
> > > +                                             info.start, info.last);
> > > +               if (map) {
> > > +                       info.start = map->start;
> > > +                       info.last = map->last;
> > > +                       info.capability = 0;
> > > +                       if (domain->bounce_map && map->start >= 0 &&
> > > +                           map->last < domain->bounce_size)
> > > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> > > +               }
> > > +               spin_unlock(&domain->iotlb_lock);
> > > +               if (!map)
> > > +                       break;
> > > +
> > > +               ret = -EFAULT;
> > > +               if (copy_to_user(argp, &info, sizeof(info)))
> > > +                       break;
> > > +
> > > +               ret = 0;
> > > +               break;
> > > +       }
> > >         default:
> > >                 ret = -ENOIOCTLCMD;
> > >                 break;
> > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > index 9885e0571f09..11bd48c72c6c 100644
> > > --- a/include/uapi/linux/vduse.h
> > > +++ b/include/uapi/linux/vduse.h
> > > @@ -233,6 +233,30 @@ struct vduse_iova_umem {
> > >  /* De-register the userspace memory. Caller should set iova and size field. */
> > >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
> > >
> > > +/**
> > > + * struct vduse_iova_info - information of one IOVA region
> > > + * @start: start of the IOVA region
> > > + * @last: last of the IOVA region
> > > + * @capability: capability of the IOVA regsion
> > > + * @reserved: for future use, needs to be initialized to zero
> > > + *
> > > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > > + * one IOVA region.
> > > + */
> > > +struct vduse_iova_info {
> > > +       __u64 start;
> > > +       __u64 last;
> > > +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> > > +       __u64 capability;
> > > +       __u64 reserved[3];
> > > +};
> > > +
> > > +/*
> > > + * Find the first IOVA region that overlaps with the range [start, last]
> >
> > So the code is actually find the IOVA region that is the super range
> > of [start, last] instead of overlap:
> >
>
> This is achieved by vhost_iotlb_itree_first(). And can't the super
> range of [start,last] be considered overlapping?

Ok, but what I want to ask is, under which condition can we hit the
following case

map->last >= domain->bounce_size ?

Thanks

>
> >
> > > +                       if (domain->bounce_map && map->start >= 0 &&
> > > +                           map->last < domain->bounce_size)
> > > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> >
> > Which part is wrong?
> >
>
> We will first call vhost_iotlb_itree_first() which will find the first
> IOVA region that overlaps with the range [start, last]. Then the flag
> will only be set if the IOVA region is within the bounce range.
>
> Thanks,
> Yongji
>

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Yongji Xie]

On Thu, Jul 28, 2022 at 2:45 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jul 28, 2022 at 2:36 PM Yongji Xie <xieyongji@bytedance.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 1:58 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
> > > >
> > > > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > > > support querying some information of IOVA regions.
> > > >
> > > > Now it can be used to query whether the IOVA region
> > > > supports userspace memory registration.
> > > >
> > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > ---
> > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
> > > >  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
> > > >  2 files changed, 63 insertions(+)
> > > >
> > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > index eedff0a3885a..e820c37dcba8 100644
> > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > >                                            umem.size);
> > > >                 break;
> > > >         }
> > > > +       case VDUSE_IOTLB_GET_INFO: {
> > > > +               struct vduse_iova_info info;
> > > > +               struct vhost_iotlb_map *map;
> > > > +               struct vduse_iova_domain *domain = dev->domain;
> > > > +
> > > > +               ret = -EFAULT;
> > > > +               if (copy_from_user(&info, argp, sizeof(info)))
> > > > +                       break;
> > > > +
> > > > +               ret = -EINVAL;
> > > > +               if (info.start > info.last)
> > > > +                       break;
> > > > +
> > > > +               if (!is_mem_zero((const char *)info.reserved,
> > > > +                                sizeof(info.reserved)))
> > > > +                       break;
> > > > +
> > > > +               spin_lock(&domain->iotlb_lock);
> > > > +               map = vhost_iotlb_itree_first(domain->iotlb,
> > > > +                                             info.start, info.last);
> > > > +               if (map) {
> > > > +                       info.start = map->start;
> > > > +                       info.last = map->last;
> > > > +                       info.capability = 0;
> > > > +                       if (domain->bounce_map && map->start >= 0 &&
> > > > +                           map->last < domain->bounce_size)
> > > > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> > > > +               }
> > > > +               spin_unlock(&domain->iotlb_lock);
> > > > +               if (!map)
> > > > +                       break;
> > > > +
> > > > +               ret = -EFAULT;
> > > > +               if (copy_to_user(argp, &info, sizeof(info)))
> > > > +                       break;
> > > > +
> > > > +               ret = 0;
> > > > +               break;
> > > > +       }
> > > >         default:
> > > >                 ret = -ENOIOCTLCMD;
> > > >                 break;
> > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > index 9885e0571f09..11bd48c72c6c 100644
> > > > --- a/include/uapi/linux/vduse.h
> > > > +++ b/include/uapi/linux/vduse.h
> > > > @@ -233,6 +233,30 @@ struct vduse_iova_umem {
> > > >  /* De-register the userspace memory. Caller should set iova and size field. */
> > > >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
> > > >
> > > > +/**
> > > > + * struct vduse_iova_info - information of one IOVA region
> > > > + * @start: start of the IOVA region
> > > > + * @last: last of the IOVA region
> > > > + * @capability: capability of the IOVA regsion
> > > > + * @reserved: for future use, needs to be initialized to zero
> > > > + *
> > > > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > > > + * one IOVA region.
> > > > + */
> > > > +struct vduse_iova_info {
> > > > +       __u64 start;
> > > > +       __u64 last;
> > > > +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> > > > +       __u64 capability;
> > > > +       __u64 reserved[3];
> > > > +};
> > > > +
> > > > +/*
> > > > + * Find the first IOVA region that overlaps with the range [start, last]
> > >
> > > So the code is actually find the IOVA region that is the super range
> > > of [start, last] instead of overlap:
> > >
> >
> > This is achieved by vhost_iotlb_itree_first(). And can't the super
> > range of [start,last] be considered overlapping?
>
> Ok, but what I want to ask is, under which condition can we hit the
> following case
>
> map->last >= domain->bounce_size ?
>

I think we would not hit this case currently.

Thanks,
Yongji

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Jason Wang]

On Thu, Jul 28, 2022 at 4:27 PM Yongji Xie <xieyongji@bytedance.com> wrote:
>
> On Thu, Jul 28, 2022 at 2:45 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 2:36 PM Yongji Xie <xieyongji@bytedance.com> wrote:
> > >
> > > On Thu, Jul 28, 2022 at 1:58 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
> > > > >
> > > > > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > > > > support querying some information of IOVA regions.
> > > > >
> > > > > Now it can be used to query whether the IOVA region
> > > > > supports userspace memory registration.
> > > > >
> > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > ---
> > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
> > > > >  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
> > > > >  2 files changed, 63 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > index eedff0a3885a..e820c37dcba8 100644
> > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > > >                                            umem.size);
> > > > >                 break;
> > > > >         }
> > > > > +       case VDUSE_IOTLB_GET_INFO: {
> > > > > +               struct vduse_iova_info info;
> > > > > +               struct vhost_iotlb_map *map;
> > > > > +               struct vduse_iova_domain *domain = dev->domain;
> > > > > +
> > > > > +               ret = -EFAULT;
> > > > > +               if (copy_from_user(&info, argp, sizeof(info)))
> > > > > +                       break;
> > > > > +
> > > > > +               ret = -EINVAL;
> > > > > +               if (info.start > info.last)
> > > > > +                       break;
> > > > > +
> > > > > +               if (!is_mem_zero((const char *)info.reserved,
> > > > > +                                sizeof(info.reserved)))
> > > > > +                       break;
> > > > > +
> > > > > +               spin_lock(&domain->iotlb_lock);
> > > > > +               map = vhost_iotlb_itree_first(domain->iotlb,
> > > > > +                                             info.start, info.last);
> > > > > +               if (map) {
> > > > > +                       info.start = map->start;
> > > > > +                       info.last = map->last;
> > > > > +                       info.capability = 0;
> > > > > +                       if (domain->bounce_map && map->start >= 0 &&
> > > > > +                           map->last < domain->bounce_size)
> > > > > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> > > > > +               }
> > > > > +               spin_unlock(&domain->iotlb_lock);
> > > > > +               if (!map)
> > > > > +                       break;
> > > > > +
> > > > > +               ret = -EFAULT;
> > > > > +               if (copy_to_user(argp, &info, sizeof(info)))
> > > > > +                       break;
> > > > > +
> > > > > +               ret = 0;
> > > > > +               break;
> > > > > +       }
> > > > >         default:
> > > > >                 ret = -ENOIOCTLCMD;
> > > > >                 break;
> > > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > > index 9885e0571f09..11bd48c72c6c 100644
> > > > > --- a/include/uapi/linux/vduse.h
> > > > > +++ b/include/uapi/linux/vduse.h
> > > > > @@ -233,6 +233,30 @@ struct vduse_iova_umem {
> > > > >  /* De-register the userspace memory. Caller should set iova and size field. */
> > > > >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
> > > > >
> > > > > +/**
> > > > > + * struct vduse_iova_info - information of one IOVA region
> > > > > + * @start: start of the IOVA region
> > > > > + * @last: last of the IOVA region
> > > > > + * @capability: capability of the IOVA regsion
> > > > > + * @reserved: for future use, needs to be initialized to zero
> > > > > + *
> > > > > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > > > > + * one IOVA region.
> > > > > + */
> > > > > +struct vduse_iova_info {
> > > > > +       __u64 start;
> > > > > +       __u64 last;
> > > > > +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> > > > > +       __u64 capability;
> > > > > +       __u64 reserved[3];
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Find the first IOVA region that overlaps with the range [start, last]
> > > >
> > > > So the code is actually find the IOVA region that is the super range
> > > > of [start, last] instead of overlap:
> > > >
> > >
> > > This is achieved by vhost_iotlb_itree_first(). And can't the super
> > > range of [start,last] be considered overlapping?
> >
> > Ok, but what I want to ask is, under which condition can we hit the
> > following case
> >
> > map->last >= domain->bounce_size ?
> >
>
> I think we would not hit this case currently.

I wonder if it's worthwhile to have a WARN or just remove this check.

Thanks

>
> Thanks,
> Yongji
>

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Jason Wang]

On Thu, Jul 28, 2022 at 4:27 PM Yongji Xie <xieyongji@bytedance.com> wrote:
>
> On Thu, Jul 28, 2022 at 2:45 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 2:36 PM Yongji Xie <xieyongji@bytedance.com> wrote:
> > >
> > > On Thu, Jul 28, 2022 at 1:58 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
> > > > >
> > > > > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > > > > support querying some information of IOVA regions.
> > > > >
> > > > > Now it can be used to query whether the IOVA region
> > > > > supports userspace memory registration.
> > > > >
> > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > ---
> > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
> > > > >  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
> > > > >  2 files changed, 63 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > index eedff0a3885a..e820c37dcba8 100644
> > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > > >                                            umem.size);
> > > > >                 break;
> > > > >         }
> > > > > +       case VDUSE_IOTLB_GET_INFO: {
> > > > > +               struct vduse_iova_info info;
> > > > > +               struct vhost_iotlb_map *map;
> > > > > +               struct vduse_iova_domain *domain = dev->domain;
> > > > > +
> > > > > +               ret = -EFAULT;
> > > > > +               if (copy_from_user(&info, argp, sizeof(info)))
> > > > > +                       break;
> > > > > +
> > > > > +               ret = -EINVAL;
> > > > > +               if (info.start > info.last)
> > > > > +                       break;
> > > > > +
> > > > > +               if (!is_mem_zero((const char *)info.reserved,
> > > > > +                                sizeof(info.reserved)))
> > > > > +                       break;
> > > > > +
> > > > > +               spin_lock(&domain->iotlb_lock);
> > > > > +               map = vhost_iotlb_itree_first(domain->iotlb,
> > > > > +                                             info.start, info.last);
> > > > > +               if (map) {
> > > > > +                       info.start = map->start;
> > > > > +                       info.last = map->last;
> > > > > +                       info.capability = 0;
> > > > > +                       if (domain->bounce_map && map->start >= 0 &&
> > > > > +                           map->last < domain->bounce_size)
> > > > > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> > > > > +               }
> > > > > +               spin_unlock(&domain->iotlb_lock);
> > > > > +               if (!map)
> > > > > +                       break;
> > > > > +
> > > > > +               ret = -EFAULT;
> > > > > +               if (copy_to_user(argp, &info, sizeof(info)))
> > > > > +                       break;
> > > > > +
> > > > > +               ret = 0;
> > > > > +               break;
> > > > > +       }
> > > > >         default:
> > > > >                 ret = -ENOIOCTLCMD;
> > > > >                 break;
> > > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > > index 9885e0571f09..11bd48c72c6c 100644
> > > > > --- a/include/uapi/linux/vduse.h
> > > > > +++ b/include/uapi/linux/vduse.h
> > > > > @@ -233,6 +233,30 @@ struct vduse_iova_umem {
> > > > >  /* De-register the userspace memory. Caller should set iova and size field. */
> > > > >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
> > > > >
> > > > > +/**
> > > > > + * struct vduse_iova_info - information of one IOVA region
> > > > > + * @start: start of the IOVA region
> > > > > + * @last: last of the IOVA region
> > > > > + * @capability: capability of the IOVA regsion
> > > > > + * @reserved: for future use, needs to be initialized to zero
> > > > > + *
> > > > > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > > > > + * one IOVA region.
> > > > > + */
> > > > > +struct vduse_iova_info {
> > > > > +       __u64 start;
> > > > > +       __u64 last;
> > > > > +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> > > > > +       __u64 capability;
> > > > > +       __u64 reserved[3];
> > > > > +};
> > > > > +
> > > > > +/*
> > > > > + * Find the first IOVA region that overlaps with the range [start, last]
> > > >
> > > > So the code is actually find the IOVA region that is the super range
> > > > of [start, last] instead of overlap:
> > > >
> > >
> > > This is achieved by vhost_iotlb_itree_first(). And can't the super
> > > range of [start,last] be considered overlapping?
> >
> > Ok, but what I want to ask is, under which condition can we hit the
> > following case
> >
> > map->last >= domain->bounce_size ?
> >
>
> I think we would not hit this case currently.

I wonder if it's worthwhile to have a WARN or just remove this check.

Thanks

>
> Thanks,
> Yongji
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Yongji Xie]

On Thu, Jul 28, 2022 at 5:02 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jul 28, 2022 at 4:27 PM Yongji Xie <xieyongji@bytedance.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 2:45 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Jul 28, 2022 at 2:36 PM Yongji Xie <xieyongji@bytedance.com> wrote:
> > > >
> > > > On Thu, Jul 28, 2022 at 1:58 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
> > > > > >
> > > > > > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > > > > > support querying some information of IOVA regions.
> > > > > >
> > > > > > Now it can be used to query whether the IOVA region
> > > > > > supports userspace memory registration.
> > > > > >
> > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > ---
> > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
> > > > > >  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
> > > > > >  2 files changed, 63 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > index eedff0a3885a..e820c37dcba8 100644
> > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > > > >                                            umem.size);
> > > > > >                 break;
> > > > > >         }
> > > > > > +       case VDUSE_IOTLB_GET_INFO: {
> > > > > > +               struct vduse_iova_info info;
> > > > > > +               struct vhost_iotlb_map *map;
> > > > > > +               struct vduse_iova_domain *domain = dev->domain;
> > > > > > +
> > > > > > +               ret = -EFAULT;
> > > > > > +               if (copy_from_user(&info, argp, sizeof(info)))
> > > > > > +                       break;
> > > > > > +
> > > > > > +               ret = -EINVAL;
> > > > > > +               if (info.start > info.last)
> > > > > > +                       break;
> > > > > > +
> > > > > > +               if (!is_mem_zero((const char *)info.reserved,
> > > > > > +                                sizeof(info.reserved)))
> > > > > > +                       break;
> > > > > > +
> > > > > > +               spin_lock(&domain->iotlb_lock);
> > > > > > +               map = vhost_iotlb_itree_first(domain->iotlb,
> > > > > > +                                             info.start, info.last);
> > > > > > +               if (map) {
> > > > > > +                       info.start = map->start;
> > > > > > +                       info.last = map->last;
> > > > > > +                       info.capability = 0;
> > > > > > +                       if (domain->bounce_map && map->start >= 0 &&
> > > > > > +                           map->last < domain->bounce_size)
> > > > > > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> > > > > > +               }
> > > > > > +               spin_unlock(&domain->iotlb_lock);
> > > > > > +               if (!map)
> > > > > > +                       break;
> > > > > > +
> > > > > > +               ret = -EFAULT;
> > > > > > +               if (copy_to_user(argp, &info, sizeof(info)))
> > > > > > +                       break;
> > > > > > +
> > > > > > +               ret = 0;
> > > > > > +               break;
> > > > > > +       }
> > > > > >         default:
> > > > > >                 ret = -ENOIOCTLCMD;
> > > > > >                 break;
> > > > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > > > index 9885e0571f09..11bd48c72c6c 100644
> > > > > > --- a/include/uapi/linux/vduse.h
> > > > > > +++ b/include/uapi/linux/vduse.h
> > > > > > @@ -233,6 +233,30 @@ struct vduse_iova_umem {
> > > > > >  /* De-register the userspace memory. Caller should set iova and size field. */
> > > > > >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
> > > > > >
> > > > > > +/**
> > > > > > + * struct vduse_iova_info - information of one IOVA region
> > > > > > + * @start: start of the IOVA region
> > > > > > + * @last: last of the IOVA region
> > > > > > + * @capability: capability of the IOVA regsion
> > > > > > + * @reserved: for future use, needs to be initialized to zero
> > > > > > + *
> > > > > > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > > > > > + * one IOVA region.
> > > > > > + */
> > > > > > +struct vduse_iova_info {
> > > > > > +       __u64 start;
> > > > > > +       __u64 last;
> > > > > > +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> > > > > > +       __u64 capability;
> > > > > > +       __u64 reserved[3];
> > > > > > +};
> > > > > > +
> > > > > > +/*
> > > > > > + * Find the first IOVA region that overlaps with the range [start, last]
> > > > >
> > > > > So the code is actually find the IOVA region that is the super range
> > > > > of [start, last] instead of overlap:
> > > > >
> > > >
> > > > This is achieved by vhost_iotlb_itree_first(). And can't the super
> > > > range of [start,last] be considered overlapping?
> > >
> > > Ok, but what I want to ask is, under which condition can we hit the
> > > following case
> > >
> > > map->last >= domain->bounce_size ?
> > >
> >
> > I think we would not hit this case currently.
>
> I wonder if it's worthwhile to have a WARN or just remove this check.
>

I think we can't remove the check since not only the bounce region
will match the condition: map->start >= 0.

Or changing to map->start == 0 && map->last == domain->bounce_size - 1 ?

Thanks,
Yongji

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Jason Wang]

On Fri, Jul 29, 2022 at 11:17 AM Yongji Xie <xieyongji@bytedance.com> wrote:
>
> On Thu, Jul 28, 2022 at 5:02 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 4:27 PM Yongji Xie <xieyongji@bytedance.com> wrote:
> > >
> > > On Thu, Jul 28, 2022 at 2:45 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Jul 28, 2022 at 2:36 PM Yongji Xie <xieyongji@bytedance.com> wrote:
> > > > >
> > > > > On Thu, Jul 28, 2022 at 1:58 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
> > > > > > >
> > > > > > > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > > > > > > support querying some information of IOVA regions.
> > > > > > >
> > > > > > > Now it can be used to query whether the IOVA region
> > > > > > > supports userspace memory registration.
> > > > > > >
> > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > > ---
> > > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
> > > > > > >  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
> > > > > > >  2 files changed, 63 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > index eedff0a3885a..e820c37dcba8 100644
> > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > > > > >                                            umem.size);
> > > > > > >                 break;
> > > > > > >         }
> > > > > > > +       case VDUSE_IOTLB_GET_INFO: {
> > > > > > > +               struct vduse_iova_info info;
> > > > > > > +               struct vhost_iotlb_map *map;
> > > > > > > +               struct vduse_iova_domain *domain = dev->domain;
> > > > > > > +
> > > > > > > +               ret = -EFAULT;
> > > > > > > +               if (copy_from_user(&info, argp, sizeof(info)))
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               ret = -EINVAL;
> > > > > > > +               if (info.start > info.last)
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               if (!is_mem_zero((const char *)info.reserved,
> > > > > > > +                                sizeof(info.reserved)))
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               spin_lock(&domain->iotlb_lock);
> > > > > > > +               map = vhost_iotlb_itree_first(domain->iotlb,
> > > > > > > +                                             info.start, info.last);
> > > > > > > +               if (map) {
> > > > > > > +                       info.start = map->start;
> > > > > > > +                       info.last = map->last;
> > > > > > > +                       info.capability = 0;
> > > > > > > +                       if (domain->bounce_map && map->start >= 0 &&
> > > > > > > +                           map->last < domain->bounce_size)
> > > > > > > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> > > > > > > +               }
> > > > > > > +               spin_unlock(&domain->iotlb_lock);
> > > > > > > +               if (!map)
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               ret = -EFAULT;
> > > > > > > +               if (copy_to_user(argp, &info, sizeof(info)))
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               ret = 0;
> > > > > > > +               break;
> > > > > > > +       }
> > > > > > >         default:
> > > > > > >                 ret = -ENOIOCTLCMD;
> > > > > > >                 break;
> > > > > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > > > > index 9885e0571f09..11bd48c72c6c 100644
> > > > > > > --- a/include/uapi/linux/vduse.h
> > > > > > > +++ b/include/uapi/linux/vduse.h
> > > > > > > @@ -233,6 +233,30 @@ struct vduse_iova_umem {
> > > > > > >  /* De-register the userspace memory. Caller should set iova and size field. */
> > > > > > >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
> > > > > > >
> > > > > > > +/**
> > > > > > > + * struct vduse_iova_info - information of one IOVA region
> > > > > > > + * @start: start of the IOVA region
> > > > > > > + * @last: last of the IOVA region
> > > > > > > + * @capability: capability of the IOVA regsion
> > > > > > > + * @reserved: for future use, needs to be initialized to zero
> > > > > > > + *
> > > > > > > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > > > > > > + * one IOVA region.
> > > > > > > + */
> > > > > > > +struct vduse_iova_info {
> > > > > > > +       __u64 start;
> > > > > > > +       __u64 last;
> > > > > > > +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> > > > > > > +       __u64 capability;
> > > > > > > +       __u64 reserved[3];
> > > > > > > +};
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Find the first IOVA region that overlaps with the range [start, last]
> > > > > >
> > > > > > So the code is actually find the IOVA region that is the super range
> > > > > > of [start, last] instead of overlap:
> > > > > >
> > > > >
> > > > > This is achieved by vhost_iotlb_itree_first(). And can't the super
> > > > > range of [start,last] be considered overlapping?
> > > >
> > > > Ok, but what I want to ask is, under which condition can we hit the
> > > > following case
> > > >
> > > > map->last >= domain->bounce_size ?
> > > >
> > >
> > > I think we would not hit this case currently.
> >
> > I wonder if it's worthwhile to have a WARN or just remove this check.
> >
>
> I think we can't remove the check since not only the bounce region
> will match the condition: map->start >= 0.
>
> Or changing to map->start == 0 && map->last == domain->bounce_size - 1 ?

This looks like an assumption that there should be only one single map
for bound buffer.

It should be fine.

Thanks

>
> Thanks,
> Yongji
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH v4 5/5] vduse: Support querying information of IOVA regions

[Jason Wang]

On Fri, Jul 29, 2022 at 11:17 AM Yongji Xie <xieyongji@bytedance.com> wrote:
>
> On Thu, Jul 28, 2022 at 5:02 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Jul 28, 2022 at 4:27 PM Yongji Xie <xieyongji@bytedance.com> wrote:
> > >
> > > On Thu, Jul 28, 2022 at 2:45 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Jul 28, 2022 at 2:36 PM Yongji Xie <xieyongji@bytedance.com> wrote:
> > > > >
> > > > > On Thu, Jul 28, 2022 at 1:58 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Jul 28, 2022 at 11:20 AM Xie Yongji <xieyongji@bytedance.com> wrote:
> > > > > > >
> > > > > > > This introduces a new ioctl: VDUSE_IOTLB_GET_INFO to
> > > > > > > support querying some information of IOVA regions.
> > > > > > >
> > > > > > > Now it can be used to query whether the IOVA region
> > > > > > > supports userspace memory registration.
> > > > > > >
> > > > > > > Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
> > > > > > > ---
> > > > > > >  drivers/vdpa/vdpa_user/vduse_dev.c | 39 ++++++++++++++++++++++++++++++
> > > > > > >  include/uapi/linux/vduse.h         | 24 ++++++++++++++++++
> > > > > > >  2 files changed, 63 insertions(+)
> > > > > > >
> > > > > > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > index eedff0a3885a..e820c37dcba8 100644
> > > > > > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c
> > > > > > > @@ -1228,6 +1228,45 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
> > > > > > >                                            umem.size);
> > > > > > >                 break;
> > > > > > >         }
> > > > > > > +       case VDUSE_IOTLB_GET_INFO: {
> > > > > > > +               struct vduse_iova_info info;
> > > > > > > +               struct vhost_iotlb_map *map;
> > > > > > > +               struct vduse_iova_domain *domain = dev->domain;
> > > > > > > +
> > > > > > > +               ret = -EFAULT;
> > > > > > > +               if (copy_from_user(&info, argp, sizeof(info)))
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               ret = -EINVAL;
> > > > > > > +               if (info.start > info.last)
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               if (!is_mem_zero((const char *)info.reserved,
> > > > > > > +                                sizeof(info.reserved)))
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               spin_lock(&domain->iotlb_lock);
> > > > > > > +               map = vhost_iotlb_itree_first(domain->iotlb,
> > > > > > > +                                             info.start, info.last);
> > > > > > > +               if (map) {
> > > > > > > +                       info.start = map->start;
> > > > > > > +                       info.last = map->last;
> > > > > > > +                       info.capability = 0;
> > > > > > > +                       if (domain->bounce_map && map->start >= 0 &&
> > > > > > > +                           map->last < domain->bounce_size)
> > > > > > > +                               info.capability |= VDUSE_IOVA_CAP_UMEM;
> > > > > > > +               }
> > > > > > > +               spin_unlock(&domain->iotlb_lock);
> > > > > > > +               if (!map)
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               ret = -EFAULT;
> > > > > > > +               if (copy_to_user(argp, &info, sizeof(info)))
> > > > > > > +                       break;
> > > > > > > +
> > > > > > > +               ret = 0;
> > > > > > > +               break;
> > > > > > > +       }
> > > > > > >         default:
> > > > > > >                 ret = -ENOIOCTLCMD;
> > > > > > >                 break;
> > > > > > > diff --git a/include/uapi/linux/vduse.h b/include/uapi/linux/vduse.h
> > > > > > > index 9885e0571f09..11bd48c72c6c 100644
> > > > > > > --- a/include/uapi/linux/vduse.h
> > > > > > > +++ b/include/uapi/linux/vduse.h
> > > > > > > @@ -233,6 +233,30 @@ struct vduse_iova_umem {
> > > > > > >  /* De-register the userspace memory. Caller should set iova and size field. */
> > > > > > >  #define VDUSE_IOTLB_DEREG_UMEM _IOW(VDUSE_BASE, 0x19, struct vduse_iova_umem)
> > > > > > >
> > > > > > > +/**
> > > > > > > + * struct vduse_iova_info - information of one IOVA region
> > > > > > > + * @start: start of the IOVA region
> > > > > > > + * @last: last of the IOVA region
> > > > > > > + * @capability: capability of the IOVA regsion
> > > > > > > + * @reserved: for future use, needs to be initialized to zero
> > > > > > > + *
> > > > > > > + * Structure used by VDUSE_IOTLB_GET_INFO ioctl to get information of
> > > > > > > + * one IOVA region.
> > > > > > > + */
> > > > > > > +struct vduse_iova_info {
> > > > > > > +       __u64 start;
> > > > > > > +       __u64 last;
> > > > > > > +#define VDUSE_IOVA_CAP_UMEM (1 << 0)
> > > > > > > +       __u64 capability;
> > > > > > > +       __u64 reserved[3];
> > > > > > > +};
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * Find the first IOVA region that overlaps with the range [start, last]
> > > > > >
> > > > > > So the code is actually find the IOVA region that is the super range
> > > > > > of [start, last] instead of overlap:
> > > > > >
> > > > >
> > > > > This is achieved by vhost_iotlb_itree_first(). And can't the super
> > > > > range of [start,last] be considered overlapping?
> > > >
> > > > Ok, but what I want to ask is, under which condition can we hit the
> > > > following case
> > > >
> > > > map->last >= domain->bounce_size ?
> > > >
> > >
> > > I think we would not hit this case currently.
> >
> > I wonder if it's worthwhile to have a WARN or just remove this check.
> >
>
> I think we can't remove the check since not only the bounce region
> will match the condition: map->start >= 0.
>
> Or changing to map->start == 0 && map->last == domain->bounce_size - 1 ?

This looks like an assumption that there should be only one single map
for bound buffer.

It should be fine.

Thanks

>
> Thanks,
> Yongji
>