DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource
@ 2016-07-07 22:26 Yong Wang
  2016-07-07 22:26 ` [dpdk-dev] [PATCH 2/2] vfio: fix coding style Yong Wang
  2016-07-14 13:25 ` [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource Thomas Monjalon
  0 siblings, 2 replies; 10+ messages in thread
From: Yong Wang @ 2016-07-07 22:26 UTC (permalink / raw)
  To: anatoly.burakov; +Cc: dev, Yong Wang, Ronghua Zhang

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-dev] [PATCH 2/2] vfio: fix coding style
  2016-07-07 22:26 [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource Yong Wang
@ 2016-07-07 22:26 ` Yong Wang
  2016-07-14 13:25 ` [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource Thomas Monjalon
  1 sibling, 0 replies; 10+ messages in thread
From: Yong Wang @ 2016-07-07 22:26 UTC (permalink / raw)
  To: anatoly.burakov; +Cc: dev, 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource
  2016-07-07 22:26 [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource Yong Wang
  2016-07-07 22:26 ` [dpdk-dev] [PATCH 2/2] vfio: fix coding style Yong Wang
@ 2016-07-14 13:25 ` Thomas Monjalon
  2016-07-14 14:50   ` Burakov, Anatoly
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2016-07-14 13:25 UTC (permalink / raw)
  To: dev; +Cc: Yong Wang, anatoly.burakov, Ronghua Zhang

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource
  2016-07-14 13:25 ` [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource Thomas Monjalon
@ 2016-07-14 14:50   ` Burakov, Anatoly
  2016-07-14 15:34     ` Dan Aloni
  0 siblings, 1 reply; 10+ messages in thread
From: Burakov, Anatoly @ 2016-07-14 14:50 UTC (permalink / raw)
  To: Thomas Monjalon, dev; +Cc: Yong Wang, Ronghua Zhang, dan

> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource
  2016-07-14 14:50   ` Burakov, Anatoly
@ 2016-07-14 15:34     ` Dan Aloni
  2016-07-15  0:15       ` [dpdk-dev] [PATCH v2] " Yong Wang
  0 siblings, 1 reply; 10+ messages in thread
From: Dan Aloni @ 2016-07-14 15:34 UTC (permalink / raw)
  To: Burakov, Anatoly, r; +Cc: Thomas Monjalon, dev, Yong Wang, Ronghua Zhang

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-dev] [PATCH v2] vfio: fix pci_vfio_map_resource
  2016-07-14 15:34     ` Dan Aloni
@ 2016-07-15  0:15       ` Yong Wang
  2016-07-15 13:05         ` Burakov, Anatoly
  2016-07-15 15:32         ` Thomas Monjalon
  0 siblings, 2 replies; 10+ messages in thread
From: Yong Wang @ 2016-07-15  0:15 UTC (permalink / raw)
  To: dev; +Cc: anatoly.burakov, dan, thomas.monjalon, Yong Wang, Ronghua Zhang

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vfio: fix pci_vfio_map_resource
  2016-07-15  0:15       ` [dpdk-dev] [PATCH v2] " Yong Wang
@ 2016-07-15 13:05         ` Burakov, Anatoly
  2016-07-15 15:32         ` Thomas Monjalon
  1 sibling, 0 replies; 10+ messages in thread
From: Burakov, Anatoly @ 2016-07-15 13:05 UTC (permalink / raw)
  To: Yong Wang, dev; +Cc: dan, thomas.monjalon, Ronghua Zhang

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vfio: fix pci_vfio_map_resource
  2016-07-15  0:15       ` [dpdk-dev] [PATCH v2] " Yong Wang
  2016-07-15 13:05         ` Burakov, Anatoly
@ 2016-07-15 15:32         ` Thomas Monjalon
  2016-07-15 16:42           ` Thomas Monjalon
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2016-07-15 15:32 UTC (permalink / raw)
  To: Yong Wang; +Cc: dev, anatoly.burakov, dan, Ronghua Zhang

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’

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vfio: fix pci_vfio_map_resource
  2016-07-15 15:32         ` Thomas Monjalon
@ 2016-07-15 16:42           ` Thomas Monjalon
  2016-07-15 16:56             ` Thomas Monjalon
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Monjalon @ 2016-07-15 16:42 UTC (permalink / raw)
  To: Yong Wang; +Cc: dev, anatoly.burakov, dan, Ronghua Zhang

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v2] vfio: fix pci_vfio_map_resource
  2016-07-15 16:42           ` Thomas Monjalon
@ 2016-07-15 16:56             ` Thomas Monjalon
  0 siblings, 0 replies; 10+ messages in thread
From: Thomas Monjalon @ 2016-07-15 16:56 UTC (permalink / raw)
  To: Yong Wang; +Cc: dev, anatoly.burakov, dan, Ronghua Zhang

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-07-15 16:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-07 22:26 [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource Yong Wang
2016-07-07 22:26 ` [dpdk-dev] [PATCH 2/2] vfio: fix coding style Yong Wang
2016-07-14 13:25 ` [dpdk-dev] [PATCH 1/2] vfio: fix pci_vfio_map_resource Thomas Monjalon
2016-07-14 14:50   ` Burakov, Anatoly
2016-07-14 15:34     ` Dan Aloni
2016-07-15  0:15       ` [dpdk-dev] [PATCH v2] " Yong Wang
2016-07-15 13:05         ` Burakov, Anatoly
2016-07-15 15:32         ` Thomas Monjalon
2016-07-15 16:42           ` Thomas Monjalon
2016-07-15 16:56             ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).