[PATCH 1/2] vfio: fix pci_vfio_map_resource

[PATCH 1/2] vfio: fix pci_vfio_map_resource

[Yong Wang]

The offset of the 2nd mmap when mapping the region after msix_bar
needs to take region address into consideration.  This is exposed
when using vfio-pci to manage vmxnet3 pmd.

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>
---
 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 f91b924..3729c35 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -896,7 +896,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,
@@ -939,7 +939,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 - memreg[0].offset);
 				map_addr = pci_map_resource(second_addr,
 							    vfio_dev_fd, memreg[1].offset,
 							    memreg[1].size,
-- 
1.9.1

[PATCH 2/2] vfio: fix coding style

[Yong Wang]

Signed-off-by: Yong Wang <yongwang@vmware.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c | 6 ++++--
 1 file changed, 4 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 3729c35..6492216 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -274,7 +274,8 @@ pci_vfio_set_bus_master(int dev_fd)
 
 /* pick IOMMU type. returns a pointer to vfio_iommu_type or NULL for error */
 static const struct vfio_iommu_type *
-pci_vfio_set_iommu_type(int vfio_container_fd) {
+pci_vfio_set_iommu_type(int vfio_container_fd)
+{
 	unsigned idx;
 	for (idx = 0; idx < RTE_DIM(iommu_types); idx++) {
 		const struct vfio_iommu_type *t = &iommu_types[idx];
@@ -297,7 +298,8 @@ pci_vfio_set_iommu_type(int vfio_container_fd) {
 
 /* check if we have any supported extensions */
 static int
-pci_vfio_has_supported_extensions(int vfio_container_fd) {
+pci_vfio_has_supported_extensions(int vfio_container_fd)
+{
 	int ret;
 	unsigned idx, n_extensions = 0;
 	for (idx = 0; idx < RTE_DIM(iommu_types); idx++) {
-- 
1.9.1

Re: [PATCH 1/2] vfio: fix pci_vfio_map_resource

[Thomas Monjalon]

Someone to review this patch please?
It can be integrated in RC3 if we are sure it doesn't break anything.

2016-07-07 15:26, Yong Wang:
> The offset of the 2nd mmap when mapping the region after msix_bar
> needs to take region address into consideration.  This is exposed
> when using vfio-pci to manage vmxnet3 pmd.
> 
> 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>
> ---
>  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 f91b924..3729c35 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
> @@ -896,7 +896,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,
> @@ -939,7 +939,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 - memreg[0].offset);
>  				map_addr = pci_map_resource(second_addr,
>  							    vfio_dev_fd, memreg[1].offset,
>  							    memreg[1].size,
> 

Re: [PATCH 1/2] vfio: fix pci_vfio_map_resource

[Burakov, Anatoly]

> Someone to review this patch please?
> It can be integrated in RC3 if we are sure it doesn't break anything.
> 
> 2016-07-07 15:26, Yong Wang:
> > The offset of the 2nd mmap when mapping the region after msix_bar
> > needs to take region address into consideration.  This is exposed when
> > using vfio-pci to manage vmxnet3 pmd.
> >
> > 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>

Hi Thomas,

The patch makes sense to me, but I don't have any devices that trigger that particular codepath in my immediate vicinity. The original patch was mentioning an NVMe adapter, so that probably should be tested with this change. I'm CC'ing the original author of that patch just in case. Do we know of any other NICs/devices that might be affected by this? Aside from the obvious example of vmxnet3... 

Thanks,
Anatoly

Re: [PATCH 1/2] vfio: fix pci_vfio_map_resource

[Dan Aloni]

On Thu, Jul 14, 2016 at 02:50:51PM +0000, Burakov, Anatoly wrote:
> > Someone to review this patch please?
> > It can be integrated in RC3 if we are sure it doesn't break anything.
> > 
> > 2016-07-07 15:26, Yong Wang:
> > > The offset of the 2nd mmap when mapping the region after msix_bar
> > > needs to take region address into consideration.  This is exposed when
> > > using vfio-pci to manage vmxnet3 pmd.
> > >
> > > 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>
> 
> Hi Thomas,
> 
> The patch makes sense to me, but I don't have any devices that trigger that particular codepath in my immediate vicinity. The original patch was mentioning an NVMe adapter, so that probably should be tested with this change. I'm CC'ing the original author of that patch just in case. Do we know of any other NICs/devices that might be affected by this? Aside from the obvious example of vmxnet3... 

The patch makes sense. To explain further, the reg.offset is
resource-relative and not bar-relative like table_start and table_end.
The mmap() needs to be resource-relative.

I think it would be cleaner to use 'reg.offset' to substruct,
conveying the fact that bar_addr already accounts for it.

-                               void *second_addr = RTE_PTR_ADD(bar_addr, memreg[1].offset);
+                               void *second_addr = RTE_PTR_ADD(bar_addr,
+                                                               memreg[1].offset - reg.offset);

The work I've done with the Intel NVMe adapter was cut short due to
other tasks I've had to shift to. But it was left in a state where it
almost worked under VFIO, in a sense that the NVME card entered a
fault state and I had not invested enough time to figure out why.

If it's due to this bug, then I would be able to finally close this
one. But that's a bit hopeful, though. Happy to find actual users of
the changes.

-- 
Dan Aloni

[PATCH v2] vfio: fix pci_vfio_map_resource

[Yong Wang]

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

Re: [PATCH v2] vfio: fix pci_vfio_map_resource

[Burakov, Anatoly]

> 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>

Re: [PATCH v2] vfio: fix pci_vfio_map_resource

[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’

Re: [PATCH v2] vfio: fix pci_vfio_map_resource

[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);

Re: [PATCH v2] vfio: fix pci_vfio_map_resource

[Thomas Monjalon]

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