vhost: fix physical address mapping

PATCH New Xuan Ding 2 participants 10 messages Nov 10, 2021 – Nov 17, 2021

Share & tools

Xuan Ding

author

2021-11-10 05:59 UTC #0 lore raw

PATCH 1/1 vhost: fix physical address mapping

When choosing IOVA as PA mode, IOVA is likely to be discontinuous, which requires page by page mapping for DMA devices. To be consistent, this patch implements page by page mapping instead of mapping at the region granularity for both IOVA as VA and PA mode.

Fixes 7c61fa08b716 ("vhost: enable IOMMU for async vhost")

Signed-off-by Xuan Ding <[email protected]>

---
 lib/vhost/vhost.h      |   1 +
 lib/vhost/vhost_user.c | 104 ++++++++++++++++++++---------------------
 2 files changed, 52 insertions(+), 53 deletions(-)

lib/vhost/vhost.h +1 -0

index 7085e0885c..d246538ca5 100644
@@ -355,6 +355,7 @@ struct vring_packed_desc_event {
 struct guest_page {
 	uint64_t guest_phys_addr;
 	uint64_t host_phys_addr;
+	uint64_t host_user_addr;
 	uint64_t size;
 };

lib/vhost/vhost_user.c +51 ~~-53~~

169 lines not on this page · raw message

Signature · 1 line

2.17.1

Xuan Ding

author

2021-11-10 06:10 UTC #1 lore raw

PATCH 1/1 vhost: fix physical address mapping

Fixes 7c61fa08b716 ("vhost: enable IOMMU for async vhost")

Signed-off-by Xuan Ding <[email protected]>

---

v2:
* Fix a format issue.

---
 lib/vhost/vhost.h      |   1 +
 lib/vhost/vhost_user.c | 104 ++++++++++++++++++++---------------------
 2 files changed, 52 insertions(+), 53 deletions(-)

lib/vhost/vhost.h +1 -0

index 7085e0885c..d246538ca5 100644
@@ -355,6 +355,7 @@ struct vring_packed_desc_event {
 struct guest_page {
 	uint64_t guest_phys_addr;
 	uint64_t host_phys_addr;
+	uint64_t host_user_addr;
 	uint64_t size;
 };

lib/vhost/vhost_user.c +51 ~~-53~~

169 lines not on this page · raw message

Signature · 1 line

2.17.1

Xuan Ding

author

2021-11-10 06:20 UTC #2 lore raw

PATCH v3 1/1 vhost: fix physical address mapping

In reply to Xuan Ding

Fixes 7c61fa08b716 ("vhost: enable IOMMU for async vhost")

Signed-off-by Xuan Ding <[email protected]>

---

v3:
* Fix commit title.

v2:
* Fix a format issue.

---
 lib/vhost/vhost.h      |   1 +
 lib/vhost/vhost_user.c | 105 ++++++++++++++++++++---------------------
 2 files changed, 53 insertions(+), 53 deletions(-)

lib/vhost/vhost.h +1 -0

index 7085e0885c..d246538ca5 100644
@@ -355,6 +355,7 @@ struct vring_packed_desc_event {
 struct guest_page {
 	uint64_t guest_phys_addr;
 	uint64_t host_phys_addr;
+	uint64_t host_user_addr;
 	uint64_t size;
 };

lib/vhost/vhost_user.c +52 ~~-53~~

170 lines not on this page · raw message

Signature · 1 line

2.17.1

Xia, Chenbo

2021-11-15 07:20 UTC #3 lore raw

Hi Xuan,

Quoted 102 lines

-----Original Message-----
From: Ding, Xuan <[email protected]>
Sent: Wednesday, November 10, 2021 2:07 PM
To: [email protected]; Xia, Chenbo <[email protected]>
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>; Ding, Xuan <[email protected]>
Subject: [PATCH v3] vhost: fix physical address mapping

Fixes 7c61fa08b716 ("vhost: enable IOMMU for async vhost")

Signed-off-by Xuan Ding <[email protected]>

---

v3:
* Fix commit title.

v2:
* Fix a format issue.

---
 lib/vhost/vhost.h      |   1 +
 lib/vhost/vhost_user.c | 105 ++++++++++++++++++++---------------------
 2 files changed, 53 insertions(+), 53 deletions(-)

lib/vhost/vhost.h +1 -0

index 7085e0885c..d246538ca5 100644
@@ -355,6 +355,7 @@ struct vring_packed_desc_event {
 struct guest_page {
 	uint64_t guest_phys_addr;
 	uint64_t host_phys_addr;
+	uint64_t host_user_addr;
 	uint64_t size;
 };

lib/vhost/vhost_user.c +13 ~~-23~~

index a781346c4d..37cdedda3c 100644
@@ -144,52 +144,55 @@ get_blk_size(int fd)
 }
 
 static int
-async_dma_map(struct rte_vhost_mem_region *region, bool do_map)
+async_dma_map(struct virtio_net *dev, bool do_map)
 {
-	uint64_t host_iova;
 	int ret = 0;
-
-	host_iova = rte_mem_virt2iova((void *)(uintptr_t)region->host_user_addr);
+	uint32_t i;
+	struct guest_page *page;
 	if (do_map) {
-		/* Add mapped region into the default container of DPDK. */
-		ret = rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
-						 region->host_user_addr,
-						 host_iova,
-						 region->size);
-		if (ret) {
-			/*
-			 * DMA device may bind with kernel driver, in this case,
-			 * we don't need to program IOMMU manually. However, if no
-			 * device is bound with vfio/uio in DPDK, and vfio kernel
-			 * module is loaded, the API will still be called and return
-			 * with ENODEV/ENOSUP.
-			 *
-			 * DPDK vfio only returns ENODEV/ENOSUP in very similar
-			 * situations(vfio either unsupported, or supported
-			 * but no devices found). Either way, no mappings could be
-			 * performed. We treat it as normal case in async path.
-			 */
-			if (rte_errno == ENODEV || rte_errno == ENOTSUP)
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
+			ret =
 rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
+							 page->host_user_addr,
+							 page->host_phys_addr,
+							 page->size);
+			if (ret) {
+				/*
+				 * DMA device may bind with kernel driver, in this
 case,
+				 * we don't need to program IOMMU manually. However,

if no

lib/vhost/vhost_user.c +1 -0

+				 * device is bound with vfio/uio in DPDK, and vfio

kernel

lib/vhost/vhost_user.c +1 -0

+				 * module is loaded, the API will still be called and

return

lib/vhost/vhost_user.c +3 -0

+				 * with ENODEV/ENOSUP.
+				 *
+				 * DPDK vfio only returns ENODEV/ENOSUP in very

similar

lib/vhost/vhost_user.c +2 -0

+				 * situations(vfio either unsupported, or supported
+				 * but no devices found). Either way, no mappings

could be

lib/vhost/vhost_user.c get_blk_size(int fd) · line 173

Quoted 4 lines

lib/vhost/vhost_user.c +4 -0

+				 * performed. We treat it as normal case in async path.
+				 */
+				if (rte_errno == ENODEV || rte_errno == ENOTSUP)
+					return 0;

I don't think this logic is good enough to only include the case of kernel driver + unneeded mapping. Could also be vfio driver + incorrect mapping. It's not good to assume ENODEV and ENOTSUP only comes from DPDK, it could be from kernel.

Quoted 5 lines

lib/vhost/vhost_user.c +3 -0
+
+				VHOST_LOG_CONFIG(ERR, "DMA engine map failed\n");
+				/* DMA mapping errors won't stop
VHST_USER_SET_MEM_TABLE. */
return 0;

I understand this function covers many cases and it's difficult to differentiate, So you don't check the return value but use the log here to inform users.

I suggest to use a WARNING log (since this could fail with kernel driver case but it's actually correct) and print the errno info for users.

Note: this is only a workaround, not a perfect solution. But since vhost with dmadev is in progress and most likely vhost lib will aware of dmadev id. The problem could be solved later (some dmadev api could be used to know VA/PA mode and kernel/user driver?)

Quoted 28 lines

lib/vhost/vhost_user.c +1 ~~-11~~

-
-			VHOST_LOG_CONFIG(ERR, "DMA engine map failed\n");
-			/* DMA mapping errors won't stop VHST_USER_SET_MEM_TABLE. */
-			return 0;
+			}
 		}
 
 	} else {
-		/* Remove mapped region from the default container of DPDK. */
-		ret = rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,
-						   region->host_user_addr,
-						   host_iova,
-						   region->size);
-		if (ret) {
-			/* like DMA map, ignore the kernel driver case when unmap.

lib/vhost/vhost_user.c +3 -2

-			if (rte_errno == EINVAL)
-				return 0;
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
+			ret =

rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,

lib/vhost/vhost_user.c +5 -0

+							   page->host_user_addr,
+							   page->host_phys_addr,
+							   page->size);
+			if (ret) {
+				/* like DMA map, ignore the kernel driver case when

unmap. */

lib/vhost/vhost_user.c get_blk_size(int fd) · line 194

Quoted 7 lines

lib/vhost/vhost_user.c +4 -2

+				if (rte_errno == EINVAL)
+					return 0;
 
-			VHOST_LOG_CONFIG(ERR, "DMA engine unmap failed\n");
-			return ret;
+				VHOST_LOG_CONFIG(ERR, "DMA engine unmap failed\n");
+				return ret;

Same here.

And since you don't check return value, you can just don't return anything and return type can be void

Thanks,
Chenbo

Quoted 97 lines

lib/vhost/vhost_user.c +6 -4

+			}
 		}
 	}

@@ -205,12 +208,12 @@ free_mem_region(struct virtio_net *dev)
 	if (!dev || !dev->mem)
 		return;
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, false);
+
 	for (i = 0; i < dev->mem->nregions; i++) {
 		reg = &dev->mem->regions[i];
 		if (reg->host_user_addr) {
-			if (dev->async_copy && rte_vfio_is_enabled("vfio"))
-				async_dma_map(reg, false);
-
 			munmap(reg->mmap_addr, reg->mmap_size);
 			close(reg->fd);
 		}

@@ -978,7 +981,7 @@ vhost_user_set_vring_base(struct virtio_net **pdev,
 
 static int
 add_one_guest_page(struct virtio_net *dev, uint64_t guest_phys_addr,
-		   uint64_t host_phys_addr, uint64_t size)
+		   uint64_t host_phys_addr, uint64_t host_user_addr, uint64_t size)
 {
 	struct guest_page *page, *last_page;
 	struct guest_page *old_pages;

@@ -1009,6 +1012,7 @@ add_one_guest_page(struct virtio_net *dev, uint64_t
 guest_phys_addr,
 	page = &dev->guest_pages[dev->nr_guest_pages++];
 	page->guest_phys_addr = guest_phys_addr;
 	page->host_phys_addr  = host_phys_addr;
+	page->host_user_addr = host_user_addr;
 	page->size = size;

lib/vhost/vhost_user.c +2 -1

 	return 0;

@@ -1028,7 +1032,8 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 	size = page_size - (guest_phys_addr & (page_size - 1));
 	size = RTE_MIN(size, reg_size);
 
-	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr, size) < 0)
+	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
+			       host_user_addr, size) < 0)
 		return -1;

lib/vhost/vhost_user.c +1 -1

 	host_user_addr  += size;

@@ -1040,7 +1045,7 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 		host_phys_addr = rte_mem_virt2iova((void *)(uintptr_t)
 						  host_user_addr);
 		if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
-				size) < 0)
+				       host_user_addr, size) < 0)
 			return -1;

lib/vhost/vhost_user.c +0 -9

 		host_user_addr  += size;

@@ -1215,7 +1220,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 	uint64_t mmap_size;
 	uint64_t alignment;
 	int populate;
-	int ret;
 
 	/* Check for memory_size + mmap_offset overflow */
 	if (mmap_offset >= -region->size) {

@@ -1274,14 +1278,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 			VHOST_LOG_CONFIG(ERR, "adding guest pages to region
 failed.\n");
 			return -1;
 		}
-
-		if (rte_vfio_is_enabled("vfio")) {
-			ret = async_dma_map(region, true);
-			if (ret) {
-				VHOST_LOG_CONFIG(ERR, "Configure IOMMU for DMA engine
 failed\n");
-				return -1;
-			}
-		}
 	}

lib/vhost/vhost_user.c +3 -1

 	VHOST_LOG_CONFIG(INFO,

@@ -1420,6 +1416,9 @@ vhost_user_set_mem_table(struct virtio_net **pdev,
 struct VhostUserMsg *msg,
 		dev->mem->nregions++;
 	}
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, true);
+
 	if (vhost_user_postcopy_register(dev, main_fd, msg) < 0)
 		goto free_mem_table;
 
--

2.17.1

Ding, Xuan

author

2021-11-15 08:13 UTC #4 lore raw

Hi Chenbo,

Quoted 132 lines

-----Original Message-----
From: Xia, Chenbo <[email protected]>
Sent: 2021年11月15日 15:21
To: Ding, Xuan <[email protected]>; [email protected]
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>
Subject: RE: [PATCH v3] vhost: fix physical address mapping
Hi Xuan,
Quoted 52 lines
-----Original Message-----
From: Ding, Xuan <[email protected]>
Sent: Wednesday, November 10, 2021 2:07 PM
To: [email protected]; Xia, Chenbo <[email protected]>
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>; Ding, Xuan <[email protected]>
Subject: [PATCH v3] vhost: fix physical address mapping
When choosing IOVA as PA mode, IOVA is likely to be discontinuous, which requires page by page mapping for DMA devices. To be consistent, this patch implements page by page mapping instead of mapping at the region granularity for both IOVA as VA and PA mode.
Fixes 7c61fa08b716 ("vhost: enable IOMMU for async vhost")
Signed-off-by Xuan Ding <[email protected]>
---
v3:
* Fix commit title.
v2:
* Fix a format issue.
---
 lib/vhost/vhost.h      |   1 +
 lib/vhost/vhost_user.c | 105
++++++++++++++++++++---------------------
 2 files changed, 53 insertions(+), 53 deletions(-)
lib/vhost/vhost.h index renamed from lib/vhost/vhost.h +0 -0
7085e0885c..d246538ca5 100644
lib/vhost/vhost.h +1 -0
@@ -355,6 +355,7 @@ struct vring_packed_desc_event {  struct
 guest_page {
 	uint64_t guest_phys_addr;
 	uint64_t host_phys_addr;
+	uint64_t host_user_addr;
 	uint64_t size;
 };
 
lib/vhost/vhost_user.c index renamed from lib/vhost/vhost_user.c +0 -0
a781346c4d..37cdedda3c 100644
lib/vhost/vhost_user.c +1 -1
@@ -144,52 +144,55 @@ get_blk_size(int fd)  }
 
 static int
-async_dma_map(struct rte_vhost_mem_region *region, bool do_map)
+async_dma_map(struct virtio_net *dev, bool do_map)
 {
lib/vhost/vhost_user.c get_blk_size(int fd) } · line 155
Quoted 10 lines
lib/vhost/vhost_user.c +2 -5
-	uint64_t host_iova;
 	int ret = 0;
-
-	host_iova = rte_mem_virt2iova((void *)(uintptr_t)region-
 host_user_addr);
+	uint32_t i;
+	struct guest_page *page;
 	if (do_map) {
-		/* Add mapped region into the default container of DPDK. */
-		ret =
rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 7 lines
lib/vhost/vhost_user.c +0 -7
-						 region->host_user_addr,
-						 host_iova,
-						 region->size);
-		if (ret) {
-			/*
-			 * DMA device may bind with kernel driver, in this case,
-			 * we don't need to program IOMMU manually.
However, if no
lib/vhost/vhost_user.c get_blk_size(int fd) } · line 164
Quoted 2 lines
lib/vhost/vhost_user.c +0 -2
-			 * device is bound with vfio/uio in DPDK, and vfio kernel
-			 * module is loaded, the API will still be called and
return
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 3 lines
lib/vhost/vhost_user.c +0 -3
-			 * with ENODEV/ENOSUP.
-			 *
-			 * DPDK vfio only returns ENODEV/ENOSUP in very
similar
lib/vhost/vhost_user.c get_blk_size(int fd) } · line 169
Quoted 2 lines
lib/vhost/vhost_user.c +0 -2
-			 * situations(vfio either unsupported, or supported
-			 * but no devices found). Either way, no mappings could
be
Quoted 5 lines
lib/vhost/vhost_user.c +2 -3
-			 * performed. We treat it as normal case in async path.
-			 */
-			if (rte_errno == ENODEV || rte_errno == ENOTSUP)
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 10 lines
lib/vhost/vhost_user.c +7 -0
+			ret =
 rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
+							 page-
 host_user_addr,
+							 page-
 host_phys_addr,
+							 page->size);
+			if (ret) {
+				/*
+				 * DMA device may bind with kernel driver, in
this
lib/vhost/vhost_user.c get_blk_size(int fd) } · line 166
Quoted 2 lines
lib/vhost/vhost_user.c +1 -0
 case,
+				 * we don't need to program IOMMU manually.
However,
lib/vhost/vhost_user.c
Quoted 2 lines
if no
lib/vhost/vhost_user.c +1 -0
+				 * device is bound with vfio/uio in DPDK, and
vfio
lib/vhost/vhost_user.c
Quoted 2 lines
kernel
lib/vhost/vhost_user.c +1 -0
+				 * module is loaded, the API will still be called
and
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 4 lines
return
lib/vhost/vhost_user.c +3 -0
+				 * with ENODEV/ENOSUP.
+				 *
+				 * DPDK vfio only returns ENODEV/ENOSUP in
very
lib/vhost/vhost_user.c
Quoted 2 lines
similar
lib/vhost/vhost_user.c +1 -0
+				 * situations(vfio either unsupported, or
supported
lib/vhost/vhost_user.c
Quoted 1 line
lib/vhost/vhost_user.c +1 -0
+				 * but no devices found). Either way, no
mappings
lib/vhost/vhost_user.c
Quoted 2 lines
could be
lib/vhost/vhost_user.c +1 -0
+				 * performed. We treat it as normal case in
async path.
lib/vhost/vhost_user.c
Quoted 2 lines
lib/vhost/vhost_user.c +2 -0
+				 */
+				if (rte_errno == ENODEV || rte_errno ==
ENOTSUP)

Quoted 6 lines

lib/vhost/vhost_user.c
Quoted 1 line
lib/vhost/vhost_user.c +1 -0
+					return 0;
I don't think this logic is good enough to only include the case of kernel driver + unneeded mapping. Could also be vfio driver + incorrect mapping. It's not good to assume ENODEV and ENOTSUP only comes from DPDK, it could be from kernel.

The code here is a workaround to be compatible with kernel driver, since we can't know the device driver in vhost for now. In fact, the appropriate approach for justifying whether perform mapping here should base on the device driver.

Quoted 11 lines

lib/vhost/vhost_user.c
Quoted 3 lines
lib/vhost/vhost_user.c +2 -0
+
+				VHOST_LOG_CONFIG(ERR, "DMA engine map
failed\n");
Quoted 3 lines
lib/vhost/vhost_user.c +1 -0
+				/* DMA mapping errors won't stop
VHST_USER_SET_MEM_TABLE. */
return 0;
I understand this function covers many cases and it's difficult to differentiate, So you don't check the return value but use the log here to inform users.

Quoted 2 lines

I suggest to use a WARNING log (since this could fail with kernel driver case but it's actually correct) and print the errno info for users.

Thanks for your suggestion. Accordingly, I will update the doc to inform user should take which action according to different warning message.

Quoted 5 lines

Note: this is only a workaround, not a perfect solution. But since vhost with dmadev is in progress and most likely vhost lib will aware of dmadev id. The problem could be solved later (some dmadev api could be used to know VA/PA mode and kernel/user driver?)

With rte_iova_mode(), we can know current IOVA mode in DPDK. It will not affect vfio mapping because bound a device to vfio always requires mapping. For dmadev, it is possible to know current driver, since the integration for async vhost with dmadev is in progress, so let's use this solution for now.

Quoted 44 lines

lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 4 lines
lib/vhost/vhost_user.c +0 -3
-
-			VHOST_LOG_CONFIG(ERR, "DMA engine map failed\n");
-			/* DMA mapping errors won't stop
VHST_USER_SET_MEM_TABLE. */
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 6 lines
lib/vhost/vhost_user.c +1 -2
-			return 0;
+			}
 		}
 
 	} else {
-		/* Remove mapped region from the default container of DPDK.
*/
lib/vhost/vhost_user.c
Quoted 1 line
lib/vhost/vhost_user.c +0 -1
-		ret =
rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 5 lines
lib/vhost/vhost_user.c +0 -5
-						   region->host_user_addr,
-						   host_iova,
-						   region->size);
-		if (ret) {
-			/* like DMA map, ignore the kernel driver case when
unmap.
Quoted 11 lines
*/
lib/vhost/vhost_user.c +3 -2
-			if (rte_errno == EINVAL)
-				return 0;
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
+			ret =
rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c +1 -0
+							   page-
host_user_addr,
lib/vhost/vhost_user.c +1 -0
+							   page-
host_phys_addr,
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 3 lines
lib/vhost/vhost_user.c +3 -0
+							   page->size);
+			if (ret) {
+				/* like DMA map, ignore the kernel driver case
when
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 5 lines
unmap. */
lib/vhost/vhost_user.c +2 -1
+				if (rte_errno == EINVAL)
+					return 0;
 
-			VHOST_LOG_CONFIG(ERR, "DMA engine unmap
failed\n");
lib/vhost/vhost_user.c
Quoted 2 lines
lib/vhost/vhost_user.c +1 -1
-			return ret;
+				VHOST_LOG_CONFIG(ERR, "DMA engine
unmap failed\n");

Quoted 6 lines

lib/vhost/vhost_user.c get_blk_size(int fd) · line 194
Quoted 1 line
lib/vhost/vhost_user.c +1 -0
+				return ret;
Same here.
And since you don't check return value, you can just don't return anything and return type can be void

Good catch, pls see next version.

Thanks,
Xuan

Quoted 106 lines

Thanks,
Chenbo

Quoted 21 lines

lib/vhost/vhost_user.c +4 -3

+			}
 		}
 	}

@@ -205,12 +208,12 @@ free_mem_region(struct virtio_net *dev)
 	if (!dev || !dev->mem)
 		return;
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, false);
+
 	for (i = 0; i < dev->mem->nregions; i++) {
 		reg = &dev->mem->regions[i];
 		if (reg->host_user_addr) {
-			if (dev->async_copy && rte_vfio_is_enabled("vfio"))
-				async_dma_map(reg, false);
-
 			munmap(reg->mmap_addr, reg->mmap_size);
 			close(reg->fd);
 		}

lib/vhost/vhost_user.c vhost_user_set_vring_base(struct virtio_net · line 985

Quoted 6 lines

lib/vhost/vhost_user.c +1 -1

@@ -978,7 +981,7 @@ vhost_user_set_vring_base(struct virtio_net
 **pdev,
 
 static int
 add_one_guest_page(struct virtio_net *dev, uint64_t guest_phys_addr,
-		   uint64_t host_phys_addr, uint64_t size)
+		   uint64_t host_phys_addr, uint64_t host_user_addr, uint64_t

size)

Quoted 13 lines

lib/vhost/vhost_user.c +0 -0

 {
 	struct guest_page *page, *last_page;

lib/vhost/vhost_user.c +1 -0

 	struct guest_page *old_pages;

@@ -1009,6 +1012,7 @@ add_one_guest_page(struct virtio_net *dev,
 uint64_t guest_phys_addr,
 	page = &dev->guest_pages[dev->nr_guest_pages++];
 	page->guest_phys_addr = guest_phys_addr;
 	page->host_phys_addr  = host_phys_addr;
+	page->host_user_addr = host_user_addr;
 	page->size = size;

lib/vhost/vhost_user.c +0 -0

 	return 0;

lib/vhost/vhost_user.c add_guest_pages(struct virtio_net *dev, struct · line 1032

Quoted 5 lines

lib/vhost/vhost_user.c +0 -1

@@ -1028,7 +1032,8 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 	size = page_size - (guest_phys_addr & (page_size - 1));
 	size = RTE_MIN(size, reg_size);
 
-	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr, size) <

Quoted 24 lines

lib/vhost/vhost_user.c +2 -0

+	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
+			       host_user_addr, size) < 0)
 		return -1;

lib/vhost/vhost_user.c +1 -1

 	host_user_addr  += size;

@@ -1040,7 +1045,7 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 		host_phys_addr = rte_mem_virt2iova((void *)(uintptr_t)
 						  host_user_addr);
 		if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
-				size) < 0)
+				       host_user_addr, size) < 0)
 			return -1;

lib/vhost/vhost_user.c +0 -1

 		host_user_addr  += size;

@@ -1215,7 +1220,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 	uint64_t mmap_size;
 	uint64_t alignment;
 	int populate;
-	int ret;
 
 	/* Check for memory_size + mmap_offset overflow */
 	if (mmap_offset >= -region->size) {

lib/vhost/vhost_user.c vhost_user_mmap_region(struct virtio_net *dev, · line 1278

Quoted 1 line

lib/vhost/vhost_user.c +0 -0

@@ -1274,14 +1278,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 			VHOST_LOG_CONFIG(ERR, "adding guest pages to

region failed.\n");

lib/vhost/vhost_user.c vhost_user_mmap_region(struct virtio_net *dev,

Quoted 7 lines

lib/vhost/vhost_user.c +0 -5

 			return -1;
 		}
-
-		if (rte_vfio_is_enabled("vfio")) {
-			ret = async_dma_map(region, true);
-			if (ret) {
-				VHOST_LOG_CONFIG(ERR, "Configure IOMMU

for DMA engine

Quoted 20 lines

lib/vhost/vhost_user.c +0 -3

 failed\n");
-				return -1;
-			}
-		}
 	}

lib/vhost/vhost_user.c +3 -1

 	VHOST_LOG_CONFIG(INFO,

@@ -1420,6 +1416,9 @@ vhost_user_set_mem_table(struct virtio_net
 **pdev, struct VhostUserMsg *msg,
 		dev->mem->nregions++;
 	}
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, true);
+
 	if (vhost_user_postcopy_register(dev, main_fd, msg) < 0)
 		goto free_mem_table;
 
--

2.17.1

Xia, Chenbo

2021-11-15 12:11 UTC #5 lore raw

Quoted 149 lines

-----Original Message-----
From: Ding, Xuan <[email protected]>
Sent: Monday, November 15, 2021 4:13 PM
To: Xia, Chenbo <[email protected]>; [email protected]
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>; Burakov, Anatoly <[email protected]>
Subject: RE: [PATCH v3] vhost: fix physical address mapping
Hi Chenbo,
Quoted 129 lines
-----Original Message-----
From: Xia, Chenbo <[email protected]>
Sent: 2021年11月15日 15:21
To: Ding, Xuan <[email protected]>; [email protected]
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>
Subject: RE: [PATCH v3] vhost: fix physical address mapping
Hi Xuan,
Quoted 52 lines
-----Original Message-----
From: Ding, Xuan <[email protected]>
Sent: Wednesday, November 10, 2021 2:07 PM
To: [email protected]; Xia, Chenbo <[email protected]>
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>; Ding, Xuan <[email protected]>
Subject: [PATCH v3] vhost: fix physical address mapping
When choosing IOVA as PA mode, IOVA is likely to be discontinuous, which requires page by page mapping for DMA devices. To be consistent, this patch implements page by page mapping instead of mapping at the region granularity for both IOVA as VA and PA mode.
Fixes 7c61fa08b716 ("vhost: enable IOMMU for async vhost")
Signed-off-by Xuan Ding <[email protected]>
---
v3:
* Fix commit title.
v2:
* Fix a format issue.
---
 lib/vhost/vhost.h      |   1 +
 lib/vhost/vhost_user.c | 105
++++++++++++++++++++---------------------
 2 files changed, 53 insertions(+), 53 deletions(-)
lib/vhost/vhost.h index renamed from lib/vhost/vhost.h +0 -0
7085e0885c..d246538ca5 100644
lib/vhost/vhost.h +1 -0
@@ -355,6 +355,7 @@ struct vring_packed_desc_event {  struct
 guest_page {
 	uint64_t guest_phys_addr;
 	uint64_t host_phys_addr;
+	uint64_t host_user_addr;
 	uint64_t size;
 };
 
lib/vhost/vhost_user.c index renamed from lib/vhost/vhost_user.c +0 -0
a781346c4d..37cdedda3c 100644
lib/vhost/vhost_user.c +1 -1
@@ -144,52 +144,55 @@ get_blk_size(int fd)  }
 
 static int
-async_dma_map(struct rte_vhost_mem_region *region, bool do_map)
+async_dma_map(struct virtio_net *dev, bool do_map)
 {
lib/vhost/vhost_user.c get_blk_size(int fd) } · line 155
Quoted 10 lines
lib/vhost/vhost_user.c +2 -5
-	uint64_t host_iova;
 	int ret = 0;
-
-	host_iova = rte_mem_virt2iova((void *)(uintptr_t)region-
 host_user_addr);
+	uint32_t i;
+	struct guest_page *page;
 	if (do_map) {
-		/* Add mapped region into the default container of DPDK. */
-		ret =
rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 7 lines
lib/vhost/vhost_user.c +0 -7
-						 region->host_user_addr,
-						 host_iova,
-						 region->size);
-		if (ret) {
-			/*
-			 * DMA device may bind with kernel driver, in this case,
-			 * we don't need to program IOMMU manually.
However, if no
lib/vhost/vhost_user.c get_blk_size(int fd) } · line 164
Quoted 2 lines
lib/vhost/vhost_user.c +0 -2
-			 * device is bound with vfio/uio in DPDK, and vfio kernel
-			 * module is loaded, the API will still be called and
return
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 3 lines
lib/vhost/vhost_user.c +0 -3
-			 * with ENODEV/ENOSUP.
-			 *
-			 * DPDK vfio only returns ENODEV/ENOSUP in very
similar
lib/vhost/vhost_user.c get_blk_size(int fd) } · line 169
Quoted 2 lines
lib/vhost/vhost_user.c +0 -2
-			 * situations(vfio either unsupported, or supported
-			 * but no devices found). Either way, no mappings could
be
Quoted 5 lines
lib/vhost/vhost_user.c +2 -3
-			 * performed. We treat it as normal case in async path.
-			 */
-			if (rte_errno == ENODEV || rte_errno == ENOTSUP)
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 10 lines
lib/vhost/vhost_user.c +7 -0
+			ret =
 rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
+							 page-
 host_user_addr,
+							 page-
 host_phys_addr,
+							 page->size);
+			if (ret) {
+				/*
+				 * DMA device may bind with kernel driver, in
this
lib/vhost/vhost_user.c get_blk_size(int fd) } · line 166
Quoted 2 lines
lib/vhost/vhost_user.c +1 -0
 case,
+				 * we don't need to program IOMMU manually.
However,
lib/vhost/vhost_user.c
Quoted 2 lines
if no
lib/vhost/vhost_user.c +1 -0
+				 * device is bound with vfio/uio in DPDK, and
vfio
lib/vhost/vhost_user.c
Quoted 2 lines
kernel
lib/vhost/vhost_user.c +1 -0
+				 * module is loaded, the API will still be called
and
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 4 lines
return
lib/vhost/vhost_user.c +3 -0
+				 * with ENODEV/ENOSUP.
+				 *
+				 * DPDK vfio only returns ENODEV/ENOSUP in
very
lib/vhost/vhost_user.c
Quoted 2 lines
similar
lib/vhost/vhost_user.c +1 -0
+				 * situations(vfio either unsupported, or
supported
lib/vhost/vhost_user.c
Quoted 1 line
lib/vhost/vhost_user.c +1 -0
+				 * but no devices found). Either way, no
mappings
lib/vhost/vhost_user.c
Quoted 2 lines
could be
lib/vhost/vhost_user.c +1 -0
+				 * performed. We treat it as normal case in
async path.
Quoted 6 lines
lib/vhost/vhost_user.c
Quoted 2 lines
lib/vhost/vhost_user.c +2 -0
+				 */
+				if (rte_errno == ENODEV || rte_errno ==
ENOTSUP)
lib/vhost/vhost_user.c
Quoted 1 line
lib/vhost/vhost_user.c +1 -0
+					return 0;
I don't think this logic is good enough to only include the case of kernel
driver +
Quoted 1 line
unneeded mapping. Could also be vfio driver + incorrect mapping. It's not
good

Quoted 8 lines

Quoted 2 lines
to assume ENODEV and ENOTSUP only comes from DPDK, it could be from kernel.
The code here is a workaround to be compatible with kernel driver, since we can't
know the device driver in vhost for now. In fact, the appropriate approach for justifying
whether perform mapping here should base on the device driver.

OK. But since this code does not consider the case of FreeBSD, checking ENODEV is enough.

Quoted 7 lines

Quoted 7 lines
lib/vhost/vhost_user.c
Quoted 4 lines
lib/vhost/vhost_user.c +2 -0
+
+				VHOST_LOG_CONFIG(ERR, "DMA engine map
failed\n");
Quoted 2 lines
lib/vhost/vhost_user.c +1 -0
+				/* DMA mapping errors won't stop
VHST_USER_SET_MEM_TABLE. */

VHST -> VHOST

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
return 0;
I understand this function covers many cases and it's difficult to
differentiate, So
Quoted 2 lines
you don't check the return value but use the log here to inform users.

Quoted 7 lines

Quoted 1 line
I suggest to use a WARNING log (since this could fail with kernel driver
case but
Quoted 1 line
it's actually correct) and print the errno info for users.
Thanks for your suggestion. Accordingly, I will update the doc to inform user should
take which action according to different warning message.

If ENODEV is used to filter the error but correct case (using kernel driver), you can just use ERR log here. But return type should be void.

Thanks,
Chenbo

Quoted 179 lines

Quoted 3 lines
Note: this is only a workaround, not a perfect solution. But since vhost
with
Quoted 3 lines
dmadev is in progress and most likely vhost lib will aware of dmadev id. The problem could be solved later (some dmadev api could be used to know VA/PA mode and kernel/user driver?)
With rte_iova_mode(), we can know current IOVA mode in DPDK. It will not affect
vfio mapping because bound a device to vfio always requires mapping. For dmadev,
it is possible to know current driver, since the integration for async vhost with dmadev
is in progress, so let's use this solution for now.
Quoted 41 lines
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 4 lines
lib/vhost/vhost_user.c +0 -3
-
-			VHOST_LOG_CONFIG(ERR, "DMA engine map failed\n");
-			/* DMA mapping errors won't stop
VHST_USER_SET_MEM_TABLE. */
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 6 lines
lib/vhost/vhost_user.c +1 -2
-			return 0;
+			}
 		}
 
 	} else {
-		/* Remove mapped region from the default container of DPDK.
*/
lib/vhost/vhost_user.c
Quoted 1 line
lib/vhost/vhost_user.c +0 -1
-		ret =
rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 5 lines
lib/vhost/vhost_user.c +0 -5
-						   region->host_user_addr,
-						   host_iova,
-						   region->size);
-		if (ret) {
-			/* like DMA map, ignore the kernel driver case when
unmap.
Quoted 11 lines
*/
lib/vhost/vhost_user.c +3 -2
-			if (rte_errno == EINVAL)
-				return 0;
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
+			ret =
rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c +1 -0
+							   page-
host_user_addr,
lib/vhost/vhost_user.c +1 -0
+							   page-
host_phys_addr,
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 3 lines
lib/vhost/vhost_user.c +3 -0
+							   page->size);
+			if (ret) {
+				/* like DMA map, ignore the kernel driver case
when
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 5 lines
unmap. */
lib/vhost/vhost_user.c +2 -1
+				if (rte_errno == EINVAL)
+					return 0;
 
-			VHOST_LOG_CONFIG(ERR, "DMA engine unmap
failed\n");
Quoted 8 lines
lib/vhost/vhost_user.c
Quoted 2 lines
lib/vhost/vhost_user.c +1 -1
-			return ret;
+				VHOST_LOG_CONFIG(ERR, "DMA engine
unmap failed\n");
lib/vhost/vhost_user.c get_blk_size(int fd) · line 194
Quoted 1 line
lib/vhost/vhost_user.c +1 -0
+				return ret;
Same here.
And since you don't check return value, you can just don't return anything
and
Quoted 1 line
return type can be void
Good catch, pls see next version.
Thanks,
Xuan
Quoted 106 lines
Thanks,
Chenbo
Quoted 21 lines
lib/vhost/vhost_user.c +4 -3
+			}
 		}
 	}
@@ -205,12 +208,12 @@ free_mem_region(struct virtio_net *dev)
 	if (!dev || !dev->mem)
 		return;
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, false);
+
 	for (i = 0; i < dev->mem->nregions; i++) {
 		reg = &dev->mem->regions[i];
 		if (reg->host_user_addr) {
-			if (dev->async_copy && rte_vfio_is_enabled("vfio"))
-				async_dma_map(reg, false);
-
 			munmap(reg->mmap_addr, reg->mmap_size);
 			close(reg->fd);
 		}
lib/vhost/vhost_user.c vhost_user_set_vring_base(struct virtio_net · line 985
Quoted 6 lines
lib/vhost/vhost_user.c +1 -1
@@ -978,7 +981,7 @@ vhost_user_set_vring_base(struct virtio_net
 **pdev,
 
 static int
 add_one_guest_page(struct virtio_net *dev, uint64_t guest_phys_addr,
-		   uint64_t host_phys_addr, uint64_t size)
+		   uint64_t host_phys_addr, uint64_t host_user_addr, uint64_t
size)
Quoted 13 lines
lib/vhost/vhost_user.c +0 -0
 {
 	struct guest_page *page, *last_page;
lib/vhost/vhost_user.c +1 -0
 	struct guest_page *old_pages;
@@ -1009,6 +1012,7 @@ add_one_guest_page(struct virtio_net *dev,
 uint64_t guest_phys_addr,
 	page = &dev->guest_pages[dev->nr_guest_pages++];
 	page->guest_phys_addr = guest_phys_addr;
 	page->host_phys_addr  = host_phys_addr;
+	page->host_user_addr = host_user_addr;
 	page->size = size;
 
lib/vhost/vhost_user.c +0 -0
 	return 0;
lib/vhost/vhost_user.c add_guest_pages(struct virtio_net *dev, struct · line 1032
Quoted 5 lines
lib/vhost/vhost_user.c +0 -1
@@ -1028,7 +1032,8 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 	size = page_size - (guest_phys_addr & (page_size - 1));
 	size = RTE_MIN(size, reg_size);
 
-	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr, size) <
0)
Quoted 24 lines
lib/vhost/vhost_user.c +2 -0
+	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
+			       host_user_addr, size) < 0)
 		return -1;
 
lib/vhost/vhost_user.c +1 -1
 	host_user_addr  += size;
@@ -1040,7 +1045,7 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 		host_phys_addr = rte_mem_virt2iova((void *)(uintptr_t)
 						  host_user_addr);
 		if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
-				size) < 0)
+				       host_user_addr, size) < 0)
 			return -1;
 
lib/vhost/vhost_user.c +0 -1
 		host_user_addr  += size;
@@ -1215,7 +1220,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 	uint64_t mmap_size;
 	uint64_t alignment;
 	int populate;
-	int ret;
 
 	/* Check for memory_size + mmap_offset overflow */
 	if (mmap_offset >= -region->size) {
lib/vhost/vhost_user.c vhost_user_mmap_region(struct virtio_net *dev, · line 1278
Quoted 1 line
lib/vhost/vhost_user.c +0 -0
@@ -1274,14 +1278,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 			VHOST_LOG_CONFIG(ERR, "adding guest pages to
region failed.\n");
lib/vhost/vhost_user.c vhost_user_mmap_region(struct virtio_net *dev,
Quoted 7 lines
lib/vhost/vhost_user.c +0 -5
 			return -1;
 		}
-
-		if (rte_vfio_is_enabled("vfio")) {
-			ret = async_dma_map(region, true);
-			if (ret) {
-				VHOST_LOG_CONFIG(ERR, "Configure IOMMU
for DMA engine
Quoted 20 lines
lib/vhost/vhost_user.c +0 -3
 failed\n");
-				return -1;
-			}
-		}
 	}
 
lib/vhost/vhost_user.c +3 -1
 	VHOST_LOG_CONFIG(INFO,
@@ -1420,6 +1416,9 @@ vhost_user_set_mem_table(struct virtio_net
 **pdev, struct VhostUserMsg *msg,
 		dev->mem->nregions++;
 	}
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, true);
+
 	if (vhost_user_postcopy_register(dev, main_fd, msg) < 0)
 		goto free_mem_table;
 
--
2.17.1

Xuan Ding

author

2021-11-15 12:46 UTC #6 lore raw

PATCH v4 1/1 vhost: fix physical address mapping

In reply to Xuan Ding

Fixes 7c61fa08b716 ("vhost: enable IOMMU for async vhost")

Signed-off-by Xuan Ding <[email protected]>

---
v4:
* Remove unnessary ENOSUP check.
* Adjust return type.

v3:
* Fix commit title.

v2:
* Fix a format issue.

---
 lib/vhost/vhost.h      |   1 +
 lib/vhost/vhost_user.c | 111 ++++++++++++++++++++---------------------
 2 files changed, 54 insertions(+), 58 deletions(-)

lib/vhost/vhost.h +1 -0

index 7085e0885c..d246538ca5 100644
@@ -355,6 +355,7 @@ struct vring_packed_desc_event {
 struct guest_page {
 	uint64_t guest_phys_addr;
 	uint64_t host_phys_addr;
+	uint64_t host_user_addr;
 	uint64_t size;
 };

lib/vhost/vhost_user.c +53 ~~-58~~

176 lines not on this page · raw message

Signature · 1 line

2.17.1

Xia, Chenbo

2021-11-16 07:48 UTC #7 lore raw

Quoted 37 lines

-----Original Message-----
From: Ding, Xuan <[email protected]>
Sent: Monday, November 15, 2021 8:32 PM
To: [email protected]; Xia, Chenbo <[email protected]>
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>; Ding, Xuan <[email protected]>
Subject: [PATCH v4] vhost: fix physical address mapping
When choosing IOVA as PA mode, IOVA is likely to be discontinuous, which requires page by page mapping for DMA devices. To be consistent, this patch implements page by page mapping instead of mapping at the region granularity for both IOVA as VA and PA mode.
Fixes 7c61fa08b716 ("vhost: enable IOMMU for async vhost")
Signed-off-by Xuan Ding <[email protected]>
---
v4:
* Remove unnessary ENOSUP check.
* Adjust return type.
v3:
* Fix commit title.
v2:
* Fix a format issue.
---
 lib/vhost/vhost.h      |   1 +
 lib/vhost/vhost_user.c | 111 ++++++++++++++++++++---------------------
 2 files changed, 54 insertions(+), 58 deletions(-)
lib/vhost/vhost.h +0 -0
index 7085e0885c..d246538ca5 100644

lib/vhost/vhost.h struct vring_packed_desc_event { · line 357

Quoted 3 lines

lib/vhost/vhost.h +0 -0

@@ -355,6 +355,7 @@ struct vring_packed_desc_event {
 struct guest_page {
 	uint64_t guest_phys_addr;
 	uint64_t host_phys_addr;

This name confused me when doing review: this should be host_iova.

Could you help rename it in the patch?

Quoted 79 lines

lib/vhost/vhost.h +1 -0

+	uint64_t host_user_addr;
 	uint64_t size;
 };

lib/vhost/vhost_user.c +14 ~~-29~~

index a781346c4d..0a9dc3350f 100644
@@ -143,57 +143,56 @@ get_blk_size(int fd)
 	return ret == -1 ? (uint64_t)-1 : (uint64_t)stat.st_blksize;
 }
 
-static int
-async_dma_map(struct rte_vhost_mem_region *region, bool do_map)
+static void
+async_dma_map(struct virtio_net *dev, bool do_map)
 {
-	uint64_t host_iova;
 	int ret = 0;
-
-	host_iova = rte_mem_virt2iova((void *)(uintptr_t)region->host_user_addr);
+	uint32_t i;
+	struct guest_page *page;
 	if (do_map) {
-		/* Add mapped region into the default container of DPDK. */
-		ret = rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
-						 region->host_user_addr,
-						 host_iova,
-						 region->size);
-		if (ret) {
-			/*
-			 * DMA device may bind with kernel driver, in this case,
-			 * we don't need to program IOMMU manually. However, if no
-			 * device is bound with vfio/uio in DPDK, and vfio kernel
-			 * module is loaded, the API will still be called and return
-			 * with ENODEV/ENOSUP.
-			 *
-			 * DPDK vfio only returns ENODEV/ENOSUP in very similar
-			 * situations(vfio either unsupported, or supported
-			 * but no devices found). Either way, no mappings could be
-			 * performed. We treat it as normal case in async path.
-			 */
-			if (rte_errno == ENODEV || rte_errno == ENOTSUP)
-				return 0;
-
-			VHOST_LOG_CONFIG(ERR, "DMA engine map failed\n");
-			/* DMA mapping errors won't stop VHST_USER_SET_MEM_TABLE. */
-			return 0;
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
+			ret =
 rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
+							 page->host_user_addr,
+							 page->host_phys_addr,
+							 page->size);
+			if (ret) {
+				/*
+				 * DMA device may bind with kernel driver, in this
 case,
+				 * we don't need to program IOMMU manually. However,

if no

lib/vhost/vhost_user.c +1 -0

+				 * device is bound with vfio/uio in DPDK, and vfio

kernel

lib/vhost/vhost_user.c +1 -0

+				 * module is loaded, the API will still be called and

return

lib/vhost/vhost_user.c +3 -0

+				 * with ENODEV.
+				 *
+				 * DPDK vfio only returns ENODEV in very similar

situations

lib/vhost/vhost_user.c +1 -0

+				 * (vfio either unsupported, or supported but no

devices found).

lib/vhost/vhost_user.c +1 -0

+				 * Either way, no mappings could be performed. We

treat it as

lib/vhost/vhost_user.c +6 -0

+				 * normal case in async path. This is a workaround.
+				 */
+				if (rte_errno == ENODEV)
+					return;
+
+				/* DMA mapping errors won't stop

Quoted 1 line

VHST_USER_SET_MEM_TABLE. */

A comment in v3 is missed:

VHST -> VHOST

Thanks,
Chenbo

Quoted 133 lines

lib/vhost/vhost_user.c +2 -7

+				VHOST_LOG_CONFIG(ERR, "DMA engine map failed\n");
+			}
 		}
 
 	} else {
-		/* Remove mapped region from the default container of DPDK. */
-		ret = rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,
-						   region->host_user_addr,
-						   host_iova,
-						   region->size);
-		if (ret) {
-			/* like DMA map, ignore the kernel driver case when unmap.

lib/vhost/vhost_user.c +3 -5

-			if (rte_errno == EINVAL)
-				return 0;
-
-			VHOST_LOG_CONFIG(ERR, "DMA engine unmap failed\n");
-			return ret;
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
+			ret =

rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,

lib/vhost/vhost_user.c +5 -0

+							   page->host_user_addr,
+							   page->host_phys_addr,
+							   page->size);
+			if (ret) {
+				/* like DMA map, ignore the kernel driver case when

unmap. */

lib/vhost/vhost_user.c +10 -6

+				if (rte_errno == EINVAL)
+					return;
+
+				VHOST_LOG_CONFIG(ERR, "DMA engine unmap failed\n");
+			}
 		}
 	}
-
-	return ret;
 }
 
 static void

@@ -205,12 +204,12 @@ free_mem_region(struct virtio_net *dev)
 	if (!dev || !dev->mem)
 		return;
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, false);
+
 	for (i = 0; i < dev->mem->nregions; i++) {
 		reg = &dev->mem->regions[i];
 		if (reg->host_user_addr) {
-			if (dev->async_copy && rte_vfio_is_enabled("vfio"))
-				async_dma_map(reg, false);
-
 			munmap(reg->mmap_addr, reg->mmap_size);
 			close(reg->fd);
 		}

@@ -978,7 +977,7 @@ vhost_user_set_vring_base(struct virtio_net **pdev,
 
 static int
 add_one_guest_page(struct virtio_net *dev, uint64_t guest_phys_addr,
-		   uint64_t host_phys_addr, uint64_t size)
+		   uint64_t host_phys_addr, uint64_t host_user_addr, uint64_t size)
 {
 	struct guest_page *page, *last_page;
 	struct guest_page *old_pages;

@@ -1009,6 +1008,7 @@ add_one_guest_page(struct virtio_net *dev, uint64_t
 guest_phys_addr,
 	page = &dev->guest_pages[dev->nr_guest_pages++];
 	page->guest_phys_addr = guest_phys_addr;
 	page->host_phys_addr  = host_phys_addr;
+	page->host_user_addr = host_user_addr;
 	page->size = size;

lib/vhost/vhost_user.c +2 -1

 	return 0;

@@ -1028,7 +1028,8 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 	size = page_size - (guest_phys_addr & (page_size - 1));
 	size = RTE_MIN(size, reg_size);
 
-	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr, size) < 0)
+	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
+			       host_user_addr, size) < 0)
 		return -1;

lib/vhost/vhost_user.c +1 -1

 	host_user_addr  += size;

@@ -1040,7 +1041,7 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 		host_phys_addr = rte_mem_virt2iova((void *)(uintptr_t)
 						  host_user_addr);
 		if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
-				size) < 0)
+				       host_user_addr, size) < 0)
 			return -1;

lib/vhost/vhost_user.c +0 -9

 		host_user_addr  += size;

@@ -1215,7 +1216,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 	uint64_t mmap_size;
 	uint64_t alignment;
 	int populate;
-	int ret;
 
 	/* Check for memory_size + mmap_offset overflow */
 	if (mmap_offset >= -region->size) {

@@ -1274,14 +1274,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 			VHOST_LOG_CONFIG(ERR, "adding guest pages to region
 failed.\n");
 			return -1;
 		}
-
-		if (rte_vfio_is_enabled("vfio")) {
-			ret = async_dma_map(region, true);
-			if (ret) {
-				VHOST_LOG_CONFIG(ERR, "Configure IOMMU for DMA engine
 failed\n");
-				return -1;
-			}
-		}
 	}

lib/vhost/vhost_user.c +3 -1

 	VHOST_LOG_CONFIG(INFO,

@@ -1420,6 +1412,9 @@ vhost_user_set_mem_table(struct virtio_net **pdev,
 struct VhostUserMsg *msg,
 		dev->mem->nregions++;
 	}
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, true);
+
 	if (vhost_user_postcopy_register(dev, main_fd, msg) < 0)
 		goto free_mem_table;
 
--

2.17.1

Ding, Xuan

author

2021-11-16 08:24 UTC #8 lore raw

Hi Chenbo,

Quoted 52 lines

-----Original Message-----
From: Xia, Chenbo <[email protected]>
Sent: 2021年11月16日 15:48
To: Ding, Xuan <[email protected]>; [email protected]
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>
Subject: RE: [PATCH v4] vhost: fix physical address mapping
Quoted 38 lines
-----Original Message-----
From: Ding, Xuan <[email protected]>
Sent: Monday, November 15, 2021 8:32 PM
To: [email protected]; Xia, Chenbo <[email protected]>
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>; Ding, Xuan <[email protected]>
Subject: [PATCH v4] vhost: fix physical address mapping
When choosing IOVA as PA mode, IOVA is likely to be discontinuous, which requires page by page mapping for DMA devices. To be consistent, this patch implements page by page mapping instead of mapping at the region granularity for both IOVA as VA and PA mode.
Fixes 7c61fa08b716 ("vhost: enable IOMMU for async vhost")
Signed-off-by Xuan Ding <[email protected]>
---
v4:
* Remove unnessary ENOSUP check.
* Adjust return type.
v3:
* Fix commit title.
v2:
* Fix a format issue.
---
 lib/vhost/vhost.h      |   1 +
 lib/vhost/vhost_user.c | 111
++++++++++++++++++++---------------------
 2 files changed, 54 insertions(+), 58 deletions(-)
lib/vhost/vhost.h index renamed from lib/vhost/vhost.h +0 -0
7085e0885c..d246538ca5 100644
lib/vhost/vhost.h struct vring_packed_desc_event { struct · line 357
Quoted 3 lines
lib/vhost/vhost.h +0 -0
@@ -355,6 +355,7 @@ struct vring_packed_desc_event {  struct
 guest_page {
 	uint64_t guest_phys_addr;
 	uint64_t host_phys_addr;
This name confused me when doing review: this should be host_iova.

Quoted 1 line

Could you help rename it in the patch?

Yes, the host_phys_addr is a iova, it depends on the DPDK IOVA mode.

Quoted 90 lines

Quoted 17 lines
lib/vhost/vhost.h +1 -0
+	uint64_t host_user_addr;
 	uint64_t size;
 };
 
lib/vhost/vhost_user.c index renamed from lib/vhost/vhost_user.c +0 -0
a781346c4d..0a9dc3350f 100644
lib/vhost/vhost_user.c +2 -2
@@ -143,57 +143,56 @@ get_blk_size(int fd)
 	return ret == -1 ? (uint64_t)-1 : (uint64_t)stat.st_blksize;  }
 
-static int
-async_dma_map(struct rte_vhost_mem_region *region, bool do_map)
+static void
+async_dma_map(struct virtio_net *dev, bool do_map)
 {
lib/vhost/vhost_user.c get_blk_size(int fd) · line 155
Quoted 10 lines
lib/vhost/vhost_user.c +2 -5
-	uint64_t host_iova;
 	int ret = 0;
-
-	host_iova = rte_mem_virt2iova((void *)(uintptr_t)region-
 host_user_addr);
+	uint32_t i;
+	struct guest_page *page;
 	if (do_map) {
-		/* Add mapped region into the default container of DPDK. */
-		ret =
rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 7 lines
lib/vhost/vhost_user.c +0 -7
-						 region->host_user_addr,
-						 host_iova,
-						 region->size);
-		if (ret) {
-			/*
-			 * DMA device may bind with kernel driver, in this case,
-			 * we don't need to program IOMMU manually.
However, if no
lib/vhost/vhost_user.c get_blk_size(int fd) · line 164
Quoted 2 lines
lib/vhost/vhost_user.c +0 -2
-			 * device is bound with vfio/uio in DPDK, and vfio kernel
-			 * module is loaded, the API will still be called and
return
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 3 lines
lib/vhost/vhost_user.c +0 -3
-			 * with ENODEV/ENOSUP.
-			 *
-			 * DPDK vfio only returns ENODEV/ENOSUP in very
similar
lib/vhost/vhost_user.c get_blk_size(int fd) · line 169
Quoted 2 lines
lib/vhost/vhost_user.c +0 -2
-			 * situations(vfio either unsupported, or supported
-			 * but no devices found). Either way, no mappings could
be
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 7 lines
lib/vhost/vhost_user.c +0 -7
-			 * performed. We treat it as normal case in async path.
-			 */
-			if (rte_errno == ENODEV || rte_errno == ENOTSUP)
-				return 0;
-
-			VHOST_LOG_CONFIG(ERR, "DMA engine map failed\n");
-			/* DMA mapping errors won't stop
VHST_USER_SET_MEM_TABLE. */
lib/vhost/vhost_user.c get_blk_size(int fd) · line 164
Quoted 13 lines
lib/vhost/vhost_user.c +9 -1
-			return 0;
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
+			ret =
 rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
+							 page-
 host_user_addr,
+							 page-
 host_phys_addr,
+							 page->size);
+			if (ret) {
+				/*
+				 * DMA device may bind with kernel driver, in
this
lib/vhost/vhost_user.c get_blk_size(int fd) · line 166
Quoted 2 lines
lib/vhost/vhost_user.c +1 -0
 case,
+				 * we don't need to program IOMMU manually.
However,
lib/vhost/vhost_user.c
Quoted 2 lines
if no
lib/vhost/vhost_user.c +1 -0
+				 * device is bound with vfio/uio in DPDK, and
vfio
lib/vhost/vhost_user.c
Quoted 2 lines
kernel
lib/vhost/vhost_user.c +1 -0
+				 * module is loaded, the API will still be called
and
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 4 lines
return
lib/vhost/vhost_user.c +3 -0
+				 * with ENODEV.
+				 *
+				 * DPDK vfio only returns ENODEV in very
similar
lib/vhost/vhost_user.c
Quoted 4 lines
situations
lib/vhost/vhost_user.c +1 -0
+				 * (vfio either unsupported, or supported but no
devices found).
lib/vhost/vhost_user.c +1 -0
+				 * Either way, no mappings could be performed.
We
lib/vhost/vhost_user.c
Quoted 2 lines
treat it as
lib/vhost/vhost_user.c +1 -0
+				 * normal case in async path. This is a
workaround.

Quoted 8 lines

Quoted 6 lines
lib/vhost/vhost_user.c +5 -0
+				 */
+				if (rte_errno == ENODEV)
+					return;
+
+				/* DMA mapping errors won't stop
VHST_USER_SET_MEM_TABLE. */
A comment in v3 is missed:

Thanks for the catch. Will fix it in v5.

Thanks,
Xuan

Quoted 152 lines

VHST -> VHOST

Thanks,
Chenbo

lib/vhost/vhost_user.c

Quoted 1 line

lib/vhost/vhost_user.c +1 -0
+				VHOST_LOG_CONFIG(ERR, "DMA engine map

failed\n");

lib/vhost/vhost_user.c get_blk_size(int fd)

Quoted 5 lines

lib/vhost/vhost_user.c +1 -1

+			}
 		}
 
 	} else {
-		/* Remove mapped region from the default container of DPDK.

lib/vhost/vhost_user.c

Quoted 1 line

lib/vhost/vhost_user.c +0 -1
-		ret =

rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,

lib/vhost/vhost_user.c get_blk_size(int fd)

Quoted 5 lines

lib/vhost/vhost_user.c +0 -5

-						   region->host_user_addr,
-						   host_iova,
-						   region->size);
-		if (ret) {
-			/* like DMA map, ignore the kernel driver case when

unmap.

lib/vhost/vhost_user.c get_blk_size(int fd)

Quoted 5 lines

lib/vhost/vhost_user.c +0 -4

-			if (rte_errno == EINVAL)
-				return 0;
-
-			VHOST_LOG_CONFIG(ERR, "DMA engine unmap

failed\n");

lib/vhost/vhost_user.c get_blk_size(int fd)

Quoted 12 lines

lib/vhost/vhost_user.c +3 -1
-			return ret;
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
+			ret =
rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c +1 -0
+							   page-
host_user_addr,
lib/vhost/vhost_user.c +1 -0
+							   page-
host_phys_addr,
lib/vhost/vhost_user.c +3 -0
+							   page->size);
+			if (ret) {
+				/* like DMA map, ignore the kernel driver case

when

lib/vhost/vhost_user.c get_blk_size(int fd)

Quoted 5 lines

unmap. */

lib/vhost/vhost_user.c +4 -0

+				if (rte_errno == EINVAL)
+					return;
+
+				VHOST_LOG_CONFIG(ERR, "DMA engine

unmap failed\n");

Quoted 25 lines

lib/vhost/vhost_user.c +4 -5

+			}
 		}
 	}
-
-	return ret;
 }
 
 static void

@@ -205,12 +204,12 @@ free_mem_region(struct virtio_net *dev)
 	if (!dev || !dev->mem)
 		return;
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, false);
+
 	for (i = 0; i < dev->mem->nregions; i++) {
 		reg = &dev->mem->regions[i];
 		if (reg->host_user_addr) {
-			if (dev->async_copy && rte_vfio_is_enabled("vfio"))
-				async_dma_map(reg, false);
-
 			munmap(reg->mmap_addr, reg->mmap_size);
 			close(reg->fd);
 		}

lib/vhost/vhost_user.c vhost_user_set_vring_base(struct virtio_net · line 981

Quoted 6 lines

lib/vhost/vhost_user.c +1 -1

@@ -978,7 +977,7 @@ vhost_user_set_vring_base(struct virtio_net
 **pdev,
 
 static int
 add_one_guest_page(struct virtio_net *dev, uint64_t guest_phys_addr,
-		   uint64_t host_phys_addr, uint64_t size)
+		   uint64_t host_phys_addr, uint64_t host_user_addr, uint64_t

size)

Quoted 13 lines

lib/vhost/vhost_user.c +0 -0

 {
 	struct guest_page *page, *last_page;

lib/vhost/vhost_user.c +1 -0

 	struct guest_page *old_pages;

@@ -1009,6 +1008,7 @@ add_one_guest_page(struct virtio_net *dev,
 uint64_t guest_phys_addr,
 	page = &dev->guest_pages[dev->nr_guest_pages++];
 	page->guest_phys_addr = guest_phys_addr;
 	page->host_phys_addr  = host_phys_addr;
+	page->host_user_addr = host_user_addr;
 	page->size = size;

lib/vhost/vhost_user.c +0 -0

 	return 0;

lib/vhost/vhost_user.c add_guest_pages(struct virtio_net *dev, struct · line 1032

Quoted 5 lines

lib/vhost/vhost_user.c +0 -1

@@ -1028,7 +1028,8 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 	size = page_size - (guest_phys_addr & (page_size - 1));
 	size = RTE_MIN(size, reg_size);
 
-	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr, size) <

Quoted 24 lines

lib/vhost/vhost_user.c +2 -0

+	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
+			       host_user_addr, size) < 0)
 		return -1;

lib/vhost/vhost_user.c +1 -1

 	host_user_addr  += size;

@@ -1040,7 +1041,7 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 		host_phys_addr = rte_mem_virt2iova((void *)(uintptr_t)
 						  host_user_addr);
 		if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
-				size) < 0)
+				       host_user_addr, size) < 0)
 			return -1;

lib/vhost/vhost_user.c +0 -1

 		host_user_addr  += size;

@@ -1215,7 +1216,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 	uint64_t mmap_size;
 	uint64_t alignment;
 	int populate;
-	int ret;
 
 	/* Check for memory_size + mmap_offset overflow */
 	if (mmap_offset >= -region->size) {

lib/vhost/vhost_user.c vhost_user_mmap_region(struct virtio_net *dev, · line 1274

Quoted 1 line

lib/vhost/vhost_user.c +0 -0

@@ -1274,14 +1274,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 			VHOST_LOG_CONFIG(ERR, "adding guest pages to

region failed.\n");

lib/vhost/vhost_user.c vhost_user_mmap_region(struct virtio_net *dev,

Quoted 7 lines

lib/vhost/vhost_user.c +0 -5

 			return -1;
 		}
-
-		if (rte_vfio_is_enabled("vfio")) {
-			ret = async_dma_map(region, true);
-			if (ret) {
-				VHOST_LOG_CONFIG(ERR, "Configure IOMMU

for DMA engine

Quoted 20 lines

lib/vhost/vhost_user.c +0 -3

 failed\n");
-				return -1;
-			}
-		}
 	}

lib/vhost/vhost_user.c +3 -1

 	VHOST_LOG_CONFIG(INFO,

@@ -1420,6 +1412,9 @@ vhost_user_set_mem_table(struct virtio_net
 **pdev, struct VhostUserMsg *msg,
 		dev->mem->nregions++;
 	}
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, true);
+
 	if (vhost_user_postcopy_register(dev, main_fd, msg) < 0)
 		goto free_mem_table;
 
--

2.17.1

Ding, Xuan

author

2021-11-17 14:39 UTC #9 lore raw

Hi Maxime,

Quoted 170 lines

-----Original Message-----
From: Ding, Xuan <[email protected]>
Sent: Tuesday, November 16, 2021 4:24 PM
To: Xia, Chenbo <[email protected]>; [email protected]
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>
Subject: RE: [PATCH v4] vhost: fix physical address mapping
Hi Chenbo,
Quoted 52 lines
-----Original Message-----
From: Xia, Chenbo <[email protected]>
Sent: 2021年11月16日 15:48
To: Ding, Xuan <[email protected]>; [email protected]
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>
Subject: RE: [PATCH v4] vhost: fix physical address mapping
Quoted 38 lines
-----Original Message-----
From: Ding, Xuan <[email protected]>
Sent: Monday, November 15, 2021 8:32 PM
To: [email protected]; Xia, Chenbo <[email protected]>
Cc: [email protected]; Hu, Jiayu <[email protected]>; Wang, YuanX <[email protected]>; He, Xingguang <[email protected]>; Ding, Xuan <[email protected]>
Subject: [PATCH v4] vhost: fix physical address mapping
When choosing IOVA as PA mode, IOVA is likely to be discontinuous, which requires page by page mapping for DMA devices. To be consistent, this patch implements page by page mapping instead of mapping at the region granularity for both IOVA as VA and PA mode.
Fixes 7c61fa08b716 ("vhost: enable IOMMU for async vhost")
Signed-off-by Xuan Ding <[email protected]>
---
v4:
* Remove unnessary ENOSUP check.
* Adjust return type.
v3:
* Fix commit title.
v2:
* Fix a format issue.
---
 lib/vhost/vhost.h      |   1 +
 lib/vhost/vhost_user.c | 111
++++++++++++++++++++---------------------
 2 files changed, 54 insertions(+), 58 deletions(-)
lib/vhost/vhost.h index renamed from lib/vhost/vhost.h +0 -0
7085e0885c..d246538ca5 100644
lib/vhost/vhost.h struct vring_packed_desc_event { struct · line 357
Quoted 3 lines
lib/vhost/vhost.h +0 -0
@@ -355,6 +355,7 @@ struct vring_packed_desc_event {  struct
 guest_page {
 	uint64_t guest_phys_addr;
 	uint64_t host_phys_addr;
This name confused me when doing review: this should be host_iova.
Quoted 1 line
Could you help rename it in the patch?
Yes, the host_phys_addr is a iova, it depends on the DPDK IOVA mode.
Quoted 36 lines
Quoted 17 lines
lib/vhost/vhost.h +1 -0
+	uint64_t host_user_addr;
 	uint64_t size;
 };
 
lib/vhost/vhost_user.c index renamed from lib/vhost/vhost_user.c +0 -0
a781346c4d..0a9dc3350f 100644
lib/vhost/vhost_user.c +2 -2
@@ -143,57 +143,56 @@ get_blk_size(int fd)
 	return ret == -1 ? (uint64_t)-1 : (uint64_t)stat.st_blksize;  }
 
-static int
-async_dma_map(struct rte_vhost_mem_region *region, bool do_map)
+static void
+async_dma_map(struct virtio_net *dev, bool do_map)
 {
lib/vhost/vhost_user.c get_blk_size(int fd) · line 155
Quoted 10 lines
lib/vhost/vhost_user.c +2 -5
-	uint64_t host_iova;
 	int ret = 0;
-
-	host_iova = rte_mem_virt2iova((void *)(uintptr_t)region-
 host_user_addr);
+	uint32_t i;
+	struct guest_page *page;
 	if (do_map) {
-		/* Add mapped region into the default container of DPDK. */
-		ret =
rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 7 lines
lib/vhost/vhost_user.c +0 -7
-						 region->host_user_addr,
-						 host_iova,
-						 region->size);
-		if (ret) {
-			/*
-			 * DMA device may bind with kernel driver, in this case,
-			 * we don't need to program IOMMU manually.
However, if no
Quoted 1 line
lib/vhost/vhost_user.c get_blk_size(int fd) · line 163
Quoted 1 line
lib/vhost/vhost_user.c +0 -1
-			 * device is bound with vfio/uio in DPDK, and vfio
kernel
Quoted 8 lines
lib/vhost/vhost_user.c get_blk_size(int fd) · line 164
Quoted 1 line
lib/vhost/vhost_user.c +0 -1
-			 * module is loaded, the API will still be called and
return
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 3 lines
lib/vhost/vhost_user.c +0 -3
-			 * with ENODEV/ENOSUP.
-			 *
-			 * DPDK vfio only returns ENODEV/ENOSUP in very
similar
lib/vhost/vhost_user.c get_blk_size(int fd) · line 169
Quoted 2 lines
lib/vhost/vhost_user.c +0 -2
-			 * situations(vfio either unsupported, or supported
-			 * but no devices found). Either way, no mappings
could
Quoted 7 lines
be
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 6 lines
lib/vhost/vhost_user.c +0 -6
-			 * performed. We treat it as normal case in async path.
-			 */
-			if (rte_errno == ENODEV || rte_errno == ENOTSUP)
-				return 0;
-
-			VHOST_LOG_CONFIG(ERR, "DMA engine map
failed\n");
Quoted 25 lines
lib/vhost/vhost_user.c get_blk_size(int fd) · line 176
Quoted 1 line
lib/vhost/vhost_user.c +0 -1
-			/* DMA mapping errors won't stop
VHST_USER_SET_MEM_TABLE. */
lib/vhost/vhost_user.c get_blk_size(int fd) · line 164
Quoted 13 lines
lib/vhost/vhost_user.c +9 -1
-			return 0;
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
+			ret =
 rte_vfio_container_dma_map(RTE_VFIO_DEFAULT_CONTAINER_FD,
+							 page-
 host_user_addr,
+							 page-
 host_phys_addr,
+							 page->size);
+			if (ret) {
+				/*
+				 * DMA device may bind with kernel driver, in
this
lib/vhost/vhost_user.c get_blk_size(int fd) · line 166
Quoted 2 lines
lib/vhost/vhost_user.c +1 -0
 case,
+				 * we don't need to program IOMMU manually.
However,
lib/vhost/vhost_user.c
Quoted 2 lines
if no
lib/vhost/vhost_user.c +1 -0
+				 * device is bound with vfio/uio in DPDK, and
vfio
lib/vhost/vhost_user.c
Quoted 2 lines
kernel
lib/vhost/vhost_user.c +1 -0
+				 * module is loaded, the API will still be called
and
Quoted 7 lines
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 4 lines
return
lib/vhost/vhost_user.c +3 -0
+				 * with ENODEV.
+				 *
+				 * DPDK vfio only returns ENODEV in very
similar
lib/vhost/vhost_user.c
Quoted 2 lines
situations
lib/vhost/vhost_user.c +1 -0
+				 * (vfio either unsupported, or supported but
no
Quoted 2 lines
lib/vhost/vhost_user.c
Quoted 2 lines
devices found).
lib/vhost/vhost_user.c +1 -0
+				 * Either way, no mappings could be
performed.
Quoted 4 lines
We
lib/vhost/vhost_user.c
Quoted 2 lines
treat it as
lib/vhost/vhost_user.c +1 -0
+				 * normal case in async path. This is a
workaround.
Quoted 8 lines
Quoted 6 lines
lib/vhost/vhost_user.c +5 -0
+				 */
+				if (rte_errno == ENODEV)
+					return;
+
+				/* DMA mapping errors won't stop
VHST_USER_SET_MEM_TABLE. */
A comment in v3 is missed:

Quoted 4 lines

Thanks for the catch. Will fix it in v5.
Thanks,
Xuan

After off list discussion with Yuan, this patch has dependency on Yuan's fix patch.
And it should also take the HVA continuity into consideration. Since the v21.11 merge window is almost closed, we plan to merge the two patches and send it in next release, does it make sense for you?

Sorry for missing some test cases under PA mode, which requires some changes for the current design.

Thanks,
Xuan

Quoted 155 lines

Quoted 91 lines

VHST -> VHOST
Thanks,
Chenbo
lib/vhost/vhost_user.c
Quoted 1 line
lib/vhost/vhost_user.c +1 -0
+				VHOST_LOG_CONFIG(ERR, "DMA engine map
failed\n");
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 5 lines
lib/vhost/vhost_user.c +1 -1
+			}
 		}
 
 	} else {
-		/* Remove mapped region from the default container of DPDK.
*/
lib/vhost/vhost_user.c
Quoted 1 line
lib/vhost/vhost_user.c +0 -1
-		ret =
rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 5 lines
lib/vhost/vhost_user.c +0 -5
-						   region->host_user_addr,
-						   host_iova,
-						   region->size);
-		if (ret) {
-			/* like DMA map, ignore the kernel driver case when
unmap.
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 5 lines
*/
lib/vhost/vhost_user.c +0 -4
-			if (rte_errno == EINVAL)
-				return 0;
-
-			VHOST_LOG_CONFIG(ERR, "DMA engine unmap
failed\n");
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 12 lines
lib/vhost/vhost_user.c +3 -1
-			return ret;
+		for (i = 0; i < dev->nr_guest_pages; i++) {
+			page = &dev->guest_pages[i];
+			ret =
rte_vfio_container_dma_unmap(RTE_VFIO_DEFAULT_CONTAINER_FD,
lib/vhost/vhost_user.c +1 -0
+							   page-
host_user_addr,
lib/vhost/vhost_user.c +1 -0
+							   page-
host_phys_addr,
lib/vhost/vhost_user.c +3 -0
+							   page->size);
+			if (ret) {
+				/* like DMA map, ignore the kernel driver case
when
lib/vhost/vhost_user.c get_blk_size(int fd)
Quoted 5 lines
unmap. */
lib/vhost/vhost_user.c +4 -0
+				if (rte_errno == EINVAL)
+					return;
+
+				VHOST_LOG_CONFIG(ERR, "DMA engine
unmap failed\n");
Quoted 25 lines
lib/vhost/vhost_user.c +4 -5
+			}
 		}
 	}
-
-	return ret;
 }
 
 static void
@@ -205,12 +204,12 @@ free_mem_region(struct virtio_net *dev)
 	if (!dev || !dev->mem)
 		return;
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, false);
+
 	for (i = 0; i < dev->mem->nregions; i++) {
 		reg = &dev->mem->regions[i];
 		if (reg->host_user_addr) {
-			if (dev->async_copy && rte_vfio_is_enabled("vfio"))
-				async_dma_map(reg, false);
-
 			munmap(reg->mmap_addr, reg->mmap_size);
 			close(reg->fd);
 		}
lib/vhost/vhost_user.c vhost_user_set_vring_base(struct virtio_net · line 981
Quoted 6 lines
lib/vhost/vhost_user.c +1 -1
@@ -978,7 +977,7 @@ vhost_user_set_vring_base(struct virtio_net
 **pdev,
 
 static int
 add_one_guest_page(struct virtio_net *dev, uint64_t guest_phys_addr,
-		   uint64_t host_phys_addr, uint64_t size)
+		   uint64_t host_phys_addr, uint64_t host_user_addr, uint64_t
size)
Quoted 11 lines
lib/vhost/vhost_user.c +0 -0
 {
 	struct guest_page *page, *last_page;
lib/vhost/vhost_user.c +1 -0
 	struct guest_page *old_pages;
@@ -1009,6 +1008,7 @@ add_one_guest_page(struct virtio_net *dev,
 uint64_t guest_phys_addr,
 	page = &dev->guest_pages[dev->nr_guest_pages++];
 	page->guest_phys_addr = guest_phys_addr;
 	page->host_phys_addr  = host_phys_addr;
+	page->host_user_addr = host_user_addr;
 	page->size = size;
 

Quoted 7 lines

lib/vhost/vhost_user.c add_guest_pages(struct virtio_net *dev, struct · line 1032

Quoted 7 lines

lib/vhost/vhost_user.c +0 -1

 	return 0;

@@ -1028,7 +1028,8 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 	size = page_size - (guest_phys_addr & (page_size - 1));
 	size = RTE_MIN(size, reg_size);
 
-	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr, size)

Quoted 11 lines

lib/vhost/vhost_user.c add_guest_pages(struct virtio_net *dev, struct · line 1044

Quoted 10 lines

lib/vhost/vhost_user.c +2 -0

+	if (add_one_guest_page(dev, guest_phys_addr, host_phys_addr,
+			       host_user_addr, size) < 0)
 		return -1;

lib/vhost/vhost_user.c +0 -0

 	host_user_addr  += size;

@@ -1040,7 +1041,7 @@ add_guest_pages(struct virtio_net *dev, struct
 rte_vhost_mem_region *reg,
 		host_phys_addr = rte_mem_virt2iova((void *)(uintptr_t)
 						  host_user_addr);
 		if (add_one_guest_page(dev, guest_phys_addr,

host_phys_addr,

Quoted 44 lines

Quoted 13 lines

lib/vhost/vhost_user.c +1 -1

-				size) < 0)
+				       host_user_addr, size) < 0)
 			return -1;

lib/vhost/vhost_user.c +0 -1

 		host_user_addr  += size;

@@ -1215,7 +1216,6 @@ vhost_user_mmap_region(struct virtio_net *dev,
 	uint64_t mmap_size;
 	uint64_t alignment;
 	int populate;
-	int ret;
 
 	/* Check for memory_size + mmap_offset overflow */
 	if (mmap_offset >= -region->size) { @@ -1274,14 +1274,6 @@

Quoted 2 lines

vhost_user_mmap_region(struct virtio_net *dev,
VHOST_LOG_CONFIG(ERR, "adding guest pages to

region failed.\n");

lib/vhost/vhost_user.c vhost_user_mmap_region(struct virtio_net *dev,

Quoted 7 lines

lib/vhost/vhost_user.c +0 -5

 			return -1;
 		}
-
-		if (rte_vfio_is_enabled("vfio")) {
-			ret = async_dma_map(region, true);
-			if (ret) {
-				VHOST_LOG_CONFIG(ERR, "Configure IOMMU

for DMA engine

Quoted 20 lines

failed\n");

lib/vhost/vhost_user.c +3 -4

-				return -1;
-			}
-		}
 	}
 
 	VHOST_LOG_CONFIG(INFO,

@@ -1420,6 +1412,9 @@ vhost_user_set_mem_table(struct virtio_net
 **pdev, struct VhostUserMsg *msg,
 		dev->mem->nregions++;
 	}
 
+	if (dev->async_copy && rte_vfio_is_enabled("vfio"))
+		async_dma_map(dev, true);
+
 	if (vhost_user_postcopy_register(dev, main_fd, msg) < 0)
 		goto free_mem_table;
 
--

2.17.1