[dpdk-dev,v2] vfio: fix pci_vfio_map_resource

Message ID 1468541710-47751-1-git-send-email-yongwang@vmware.com (mailing list archive)
State Accepted, archived
Headers

Commit Message

Yong Wang July 15, 2016, 12:15 a.m. UTC
  The offset of the 2nd mmap() when mapping the region after msix_bar
needs to take region address into consideration as mmap() takes
address that is resource-relative instead of bar-relative.  This is
exposed when binding vmxnet3 to vfio-pci.

Fixes: 90a1633b2347 ("eal/linux: allow to map BARs with MSI-X tables")

Signed-off-by: Yong Wang <yongwang@vmware.com>
Signed-off-by: Ronghua Zhang <rzhang@vmware.com>
---
v2:
* Addressed review comment from Dan

 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
  

Comments

Anatoly Burakov July 15, 2016, 1:05 p.m. UTC | #1
> The offset of the 2nd mmap() when mapping the region after msix_bar
> needs to take region address into consideration as mmap() takes address
> that is resource-relative instead of bar-relative.  This is exposed when
> binding vmxnet3 to vfio-pci.
> 
> Fixes: 90a1633b2347 ("eal/linux: allow to map BARs with MSI-X tables")
> 
> Signed-off-by: Yong Wang <yongwang@vmware.com>
> Signed-off-by: Ronghua Zhang <rzhang@vmware.com>
> ---
> v2:
> * Addressed review comment from Dan
> 
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> index 46cd683..bf7cf61 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -431,7 +431,7 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
>  			} else {
>  				memreg[0].offset = reg.offset;
>  				memreg[0].size = table_start;
> -				memreg[1].offset = table_end;
> +				memreg[1].offset = reg.offset + table_end;
>  				memreg[1].size = reg.size - table_end;
> 
>  				RTE_LOG(DEBUG, EAL,
> @@ -474,7 +474,8 @@ pci_vfio_map_resource(struct rte_pci_device *dev)
>  			/* if there's a second part, try to map it */
>  			if (map_addr != MAP_FAILED
>  			    && memreg[1].offset && memreg[1].size) {
> -				void *second_addr =
> RTE_PTR_ADD(bar_addr, memreg[1].offset);
> +				void *second_addr =
> RTE_PTR_ADD(bar_addr,
> +
> 	memreg[1].offset - reg.offset);
>  				map_addr =
> pci_map_resource(second_addr,
>  							    vfio_dev_fd,
> memreg[1].offset,
>  							    memreg[1].size,
> --
> 1.9.1

Thomas, I guess we can put it in? It doesn't affect anything outside the stated use case, and code wise there's nothing preventing this from being merged either. I've done cursory testing with an ixgbe device just in case, nothing exploded. So,

Acked-by: Anatoly  Burakov <anatoly.burakov@intel.com>
  
Thomas Monjalon July 15, 2016, 3:32 p.m. UTC | #2
2016-07-14 17:15, Yong Wang:
> -				void *second_addr = RTE_PTR_ADD(bar_addr, memreg[1].offset);
> +				void *second_addr = RTE_PTR_ADD(bar_addr,
> +								memreg[1].offset - reg.offset);

There is an error for 32-bit:
	error: cast to pointer from integer of different size
	note: in expansion of macro ‘RTE_PTR_ADD’
  
Thomas Monjalon July 15, 2016, 4:42 p.m. UTC | #3
2016-07-15 17:32, Thomas Monjalon:
> 2016-07-14 17:15, Yong Wang:
> > -				void *second_addr = RTE_PTR_ADD(bar_addr, memreg[1].offset);
> > +				void *second_addr = RTE_PTR_ADD(bar_addr,
> > +								memreg[1].offset - reg.offset);
> 
> There is an error for 32-bit:
> 	error: cast to pointer from integer of different size
> 	note: in expansion of macro ‘RTE_PTR_ADD’

It can fixed like this:
-                    memreg[1].offset - reg.offset);
+                    memreg[1].offset -
+                    (uintptr_t)reg.offset);
  
Thomas Monjalon July 15, 2016, 4:56 p.m. UTC | #4
2016-07-15 18:42, Thomas Monjalon:
> 2016-07-15 17:32, Thomas Monjalon:
> > 2016-07-14 17:15, Yong Wang:
> > > -				void *second_addr = RTE_PTR_ADD(bar_addr, memreg[1].offset);
> > > +				void *second_addr = RTE_PTR_ADD(bar_addr,
> > > +								memreg[1].offset - reg.offset);
> > 
> > There is an error for 32-bit:
> > 	error: cast to pointer from integer of different size
> > 	note: in expansion of macro ‘RTE_PTR_ADD’
> 
> It can fixed like this:
> -                    memreg[1].offset - reg.offset);
> +                    memreg[1].offset -
> +                    (uintptr_t)reg.offset);
> 

Applied with above change, thanks
  

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index 46cd683..bf7cf61 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -431,7 +431,7 @@  pci_vfio_map_resource(struct rte_pci_device *dev)
 			} else {
 				memreg[0].offset = reg.offset;
 				memreg[0].size = table_start;
-				memreg[1].offset = table_end;
+				memreg[1].offset = reg.offset + table_end;
 				memreg[1].size = reg.size - table_end;
 
 				RTE_LOG(DEBUG, EAL,
@@ -474,7 +474,8 @@  pci_vfio_map_resource(struct rte_pci_device *dev)
 			/* if there's a second part, try to map it */
 			if (map_addr != MAP_FAILED
 			    && memreg[1].offset && memreg[1].size) {
-				void *second_addr = RTE_PTR_ADD(bar_addr, memreg[1].offset);
+				void *second_addr = RTE_PTR_ADD(bar_addr,
+								memreg[1].offset - reg.offset);
 				map_addr = pci_map_resource(second_addr,
 							    vfio_dev_fd, memreg[1].offset,
 							    memreg[1].size,