diff mbox

[dpdk-dev] eal: mmap uio resources using resourceX files

Message ID 1424703444-30761-1-git-send-email-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Bruce Richardson Feb. 23, 2015, 2:57 p.m. UTC 
  Instead of distinguishing the BAR mappings via offset within a single
file, originally /dev/uioX, switch to mapping each individual bar via
the appropriately numbered resourceX file.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/include/rte_pci.h    |  2 +-
 lib/librte_eal/linuxapp/eal/eal_pci_init.h |  1 +
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c  | 34 ++++++++++++++++--------------
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |  1 +
 4 files changed, 21 insertions(+), 17 deletions(-)

Comments

Bruce Richardson Feb. 23, 2015, 3 p.m. UTC | #1 
On Mon, Feb 23, 2015 at 02:57:24PM +0000, Bruce Richardson wrote:
> Instead of distinguishing the BAR mappings via offset within a single
> file, originally /dev/uioX, switch to mapping each individual bar via
> the appropriately numbered resourceX file.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
Hi Tetsuya,

in our tests here, this patch seems to fix the immediate problem you were
experiencing on your system. Can you perhaps verify?

Thanks,
/Bruce
Iremonger, Bernard Feb. 23, 2015, 3:30 p.m. UTC | #2 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> Sent: Monday, February 23, 2015 3:00 PM
> To: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] eal: mmap uio resources using resourceX files
> 
> On Mon, Feb 23, 2015 at 02:57:24PM +0000, Bruce Richardson wrote:
> > Instead of distinguishing the BAR mappings via offset within a single
> > file, originally /dev/uioX, switch to mapping each individual bar via
> > the appropriately numbered resourceX file.
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> Hi Tetsuya,
> 
> in our tests here, this patch seems to fix the immediate problem you were experiencing on your
> system. Can you perhaps verify?
> 
> Thanks,
> /Bruce

Hi Bruce,

I was seeing a similar problem on my system when attaching a virtual function port.

testpmd> port attach 0000:06:10.0 
Attaching a new port...
EAL: PCI device 0000:06:10.0 on NUMA socket -1
EAL:   probe driver: 8086:10ed rte_ixgbevf_pmd
EAL:   PCI memory mapped at 0x10000
EAL: pci_map_resource(): cannot mmap(27, 0x14000, 0x4000, 0x1000): Invalid argument (0xffffffffffffffff)
EAL: Requested device 0000:06:10.0 cannot be used
EAL: Driver, cannot attach the device

This patch seems to solve the problem.

Regards,

Bernard.
Bruce Richardson Feb. 23, 2015, 5:02 p.m. UTC | #3 
This patch does some cleanup of the uio mapping code to 
a) fix issue with mmap of PCI bars reported by Tetsuya and confirmed
by others.
b) eliminate redundant code and reduce scans of /sys


Bruce Richardson (2):
  eal: mmap uio resources using resourceX files
  eal: populate uio_maps from pci mem_resources array

 lib/librte_eal/common/include/rte_pci.h    |   2 +-
 lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 +
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c  | 173 +++++++++++------------------
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   1 +
 4 files changed, 66 insertions(+), 111 deletions(-)
Tetsuya Mukawa Feb. 24, 2015, 9:21 a.m. UTC | #4 
On 2015/02/24 0:00, Bruce Richardson wrote:
> On Mon, Feb 23, 2015 at 02:57:24PM +0000, Bruce Richardson wrote:
>> Instead of distinguishing the BAR mappings via offset within a single
>> file, originally /dev/uioX, switch to mapping each individual bar via
>> the appropriately numbered resourceX file.
>>
>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>> ---
> Hi Tetsuya,
>
> in our tests here, this patch seems to fix the immediate problem you were
> experiencing on your system. Can you perhaps verify?
>
> Thanks,
> /Bruce
>
Hi Bruce,

I've checked it, and the patch solves my issue.
Thank you so much.

Thanks,
Tetsuya
Bruce Richardson Feb. 24, 2015, 10:53 a.m. UTC | #5 
On Mon, Feb 23, 2015 at 05:02:33PM +0000, Bruce Richardson wrote:
> This patch does some cleanup of the uio mapping code to 
> a) fix issue with mmap of PCI bars reported by Tetsuya and confirmed
> by others.
> b) eliminate redundant code and reduce scans of /sys
> 
> 
> Bruce Richardson (2):
>   eal: mmap uio resources using resourceX files
>   eal: populate uio_maps from pci mem_resources array
> 
>  lib/librte_eal/common/include/rte_pci.h    |   2 +-
>  lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 +
>  lib/librte_eal/linuxapp/eal/eal_pci_uio.c  | 173 +++++++++++------------------
>  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   1 +
>  4 files changed, 66 insertions(+), 111 deletions(-)
> 
> -- 
> 2.1.0
> 
Hi David,

Given your previous suggestions on the uio_pic_generic code, I'd appreciate
any feedback you could provide on this patchset.

Regards,
/Bruce
David Marchand Feb. 24, 2015, 11:23 a.m. UTC | #6 
Hello Bruce,

On Tue, Feb 24, 2015 at 11:53 AM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> On Mon, Feb 23, 2015 at 05:02:33PM +0000, Bruce Richardson wrote:
> > This patch does some cleanup of the uio mapping code to
> > a) fix issue with mmap of PCI bars reported by Tetsuya and confirmed
> > by others.
> > b) eliminate redundant code and reduce scans of /sys
> >
> >
> > Bruce Richardson (2):
> >   eal: mmap uio resources using resourceX files
> >   eal: populate uio_maps from pci mem_resources array
> >
> >  lib/librte_eal/common/include/rte_pci.h    |   2 +-
> >  lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 +
> >  lib/librte_eal/linuxapp/eal/eal_pci_uio.c  | 173
> +++++++++++------------------
> >  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   1 +
> >  4 files changed, 66 insertions(+), 111 deletions(-)
> >
> > --
> > 2.1.0
> >
> Given your previous suggestions on the uio_pic_generic code, I'd appreciate
> any feedback you could provide on this patchset.


Well, I only have one pending question on the use of resourceX files
instead of /dev/uioX.
You rely on sysfs mmap code for pci resources.
Is this really equivalent to uio mmap operations ?
If you can ensure me this won't break igb_uio setups, then these patches
are ok for me.

Thanks for the cleanup in eal_pci_uio.c.
Bruce Richardson Feb. 24, 2015, 11:32 a.m. UTC | #7 
On Tue, Feb 24, 2015 at 12:23:15PM +0100, David Marchand wrote:
> Hello Bruce,
> 
> On Tue, Feb 24, 2015 at 11:53 AM, Bruce Richardson <
> bruce.richardson@intel.com> wrote:
> 
> > On Mon, Feb 23, 2015 at 05:02:33PM +0000, Bruce Richardson wrote:
> > > This patch does some cleanup of the uio mapping code to
> > > a) fix issue with mmap of PCI bars reported by Tetsuya and confirmed
> > > by others.
> > > b) eliminate redundant code and reduce scans of /sys
> > >
> > >
> > > Bruce Richardson (2):
> > >   eal: mmap uio resources using resourceX files
> > >   eal: populate uio_maps from pci mem_resources array
> > >
> > >  lib/librte_eal/common/include/rte_pci.h    |   2 +-
> > >  lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 +
> > >  lib/librte_eal/linuxapp/eal/eal_pci_uio.c  | 173
> > +++++++++++------------------
> > >  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   1 +
> > >  4 files changed, 66 insertions(+), 111 deletions(-)
> > >
> > > --
> > > 2.1.0
> > >
> > Given your previous suggestions on the uio_pic_generic code, I'd appreciate
> > any feedback you could provide on this patchset.
> 
> 
> Well, I only have one pending question on the use of resourceX files
> instead of /dev/uioX.
> You rely on sysfs mmap code for pci resources.
> Is this really equivalent to uio mmap operations ?

uio_pci_generic provides no mappings via /dev/uioX, so the may to mmap the 
bars using uio_pci_generic is via the sysfs. [1]

> If you can ensure me this won't break igb_uio setups, then these patches
> are ok for me.

Since igb_uio is based on the same uio_pci_generic framework, what works for
uio_pci_generic should work for igb_uio also - and testing indicates that this
works. Danny could perhaps provide better insights than I can into any guarantees
as to not breaking things, but I've tested this on a couple of platforms with
1G and 10G NICs using both igb_uio and uio_pci_generic, both individually and
in combination. I've also verified multiprocess support and done a quick sanity
test with 32-bit. Everything seems ok in testing thus far.

> 
> Thanks for the cleanup in eal_pci_uio.c.
> 
No problem.

> 
> -- 
> David Marchand

/Bruce

[1]: https://lkml.org/lkml/2012/3/20/375
Bruce Richardson Feb. 24, 2015, 12:20 p.m. UTC | #8 
V3 changes: Rebase to take account of commit 90a1633b 
	"eal/linux: allow to map BARs with MSI-X tables"

This patch does some cleanup of the uio mapping code to 
a) fix issue with mmap of PCI bars reported by Tetsuya and confirmed
by others.
b) eliminate redundant code and reduce scans of /sys

Bruce Richardson (2):
  eal: mmap uio resources using resourceX files
  eal: populate uio_maps from pci mem_resources array

 lib/librte_eal/common/include/rte_pci.h    |   2 +-
 lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 +
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c  | 173 +++++++++++------------------
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   1 +
 4 files changed, 66 insertions(+), 111 deletions(-)
David Marchand Feb. 24, 2015, 12:35 p.m. UTC | #9 
On Tue, Feb 24, 2015 at 12:32 PM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> On Tue, Feb 24, 2015 at 12:23:15PM +0100, David Marchand wrote:
> > Hello Bruce,
> >
> > On Tue, Feb 24, 2015 at 11:53 AM, Bruce Richardson <
> > bruce.richardson@intel.com> wrote:
> >
> > > On Mon, Feb 23, 2015 at 05:02:33PM +0000, Bruce Richardson wrote:
> > > > This patch does some cleanup of the uio mapping code to
> > > > a) fix issue with mmap of PCI bars reported by Tetsuya and confirmed
> > > > by others.
> > > > b) eliminate redundant code and reduce scans of /sys
> > > >
> > > >
> > > > Bruce Richardson (2):
> > > >   eal: mmap uio resources using resourceX files
> > > >   eal: populate uio_maps from pci mem_resources array
> > > >
> > > >  lib/librte_eal/common/include/rte_pci.h    |   2 +-
> > > >  lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 +
> > > >  lib/librte_eal/linuxapp/eal/eal_pci_uio.c  | 173
> > > +++++++++++------------------
> > > >  lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   1 +
> > > >  4 files changed, 66 insertions(+), 111 deletions(-)
> > > >
> > > > --
> > > > 2.1.0
> > > >
> > > Given your previous suggestions on the uio_pic_generic code, I'd
> appreciate
> > > any feedback you could provide on this patchset.
> >
> >
> > Well, I only have one pending question on the use of resourceX files
> > instead of /dev/uioX.
> > You rely on sysfs mmap code for pci resources.
> > Is this really equivalent to uio mmap operations ?
>
> uio_pci_generic provides no mappings via /dev/uioX, so the may to mmap the
> bars using uio_pci_generic is via the sysfs. [1]
>
> > If you can ensure me this won't break igb_uio setups, then these patches
> > are ok for me.
>
> Since igb_uio is based on the same uio_pci_generic framework, what works
> for
> uio_pci_generic should work for igb_uio also - and testing indicates that
> this
> works. Danny could perhaps provide better insights than I can into any
> guarantees
> as to not breaking things, but I've tested this on a couple of platforms
> with
> 1G and 10G NICs using both igb_uio and uio_pci_generic, both individually
> and
> in combination. I've also verified multiprocess support and done a quick
> sanity
> test with 32-bit. Everything seems ok in testing thus far.
>

Ok, noted.
I am looking at your v3.
Bruce Richardson Feb. 24, 2015, 1:30 p.m. UTC | #10 
V4 changes: Split second patch into two for more readable history

V3 changes: Rebase to take account of commit 90a1633b 
	"eal/linux: allow to map BARs with MSI-X tables"

This patch does some cleanup of the uio mapping code to 
a) fix issue with mmap of PCI bars reported by Tetsuya and confirmed
by others.
b) eliminate redundant code and reduce scans of /sys

Bruce Richardson (3):
  eal: mmap uio resources using resourceX files
  eal: populate uio_maps from pci mem_resources array
  eal: remove unnecessary check for primary instance

 lib/librte_eal/common/include/rte_pci.h    |   2 +-
 lib/librte_eal/linuxapp/eal/eal_pci_init.h |   1 +
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c  | 173 +++++++++++------------------
 lib/librte_eal/linuxapp/eal/eal_pci_vfio.c |   1 +
 4 files changed, 66 insertions(+), 111 deletions(-)
Thomas Monjalon Feb. 24, 2015, 9:33 p.m. UTC | #11 
> V4 changes: Split second patch into two for more readable history
> 
> V3 changes: Rebase to take account of commit 90a1633b 
> 	"eal/linux: allow to map BARs with MSI-X tables"
> 
> This patch does some cleanup of the uio mapping code to 
> a) fix issue with mmap of PCI bars reported by Tetsuya and confirmed
> by others.
> b) eliminate redundant code and reduce scans of /sys
> 
> Bruce Richardson (3):
>   eal: mmap uio resources using resourceX files
>   eal: populate uio_maps from pci mem_resources array
>   eal: remove unnecessary check for primary instance

Applied, thanks
diff mbox

Patch

diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
index 4301c16..e34b139 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -117,7 +117,7 @@  struct rte_pci_resource {
 };
 
 /** Maximum number of PCI resources. */
-#define PCI_MAX_RESOURCE 7
+#define PCI_MAX_RESOURCE 6
 
 /**
  * A structure describing an ID for a PCI driver. Each driver provides a
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_init.h b/lib/librte_eal/linuxapp/eal/eal_pci_init.h
index 1070eb8..2125d7b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_init.h
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_init.h
@@ -38,6 +38,7 @@ 
 
 struct pci_map {
 	void *addr;
+	char *path;
 	uint64_t offset;
 	uint64_t size;
 	uint64_t phaddr;
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
index 2b16fcb..ecf385a 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
@@ -137,10 +137,10 @@  pci_uio_map_secondary(struct rte_pci_device *dev)
 			/*
 			 * open devname, to mmap it
 			 */
-			fd = open(uio_res->path, O_RDWR);
+			fd = open(uio_res->maps[i].path, O_RDWR);
 			if (fd < 0) {
 				RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
-					uio_res->path, strerror(errno));
+					uio_res->maps[i].path, strerror(errno));
 				return -1;
 			}
 
@@ -149,7 +149,8 @@  pci_uio_map_secondary(struct rte_pci_device *dev)
 					     (size_t)uio_res->maps[i].size)
 			    != uio_res->maps[i].addr) {
 				RTE_LOG(ERR, EAL,
-					"Cannot mmap device resource\n");
+					"Cannot mmap device resource file: %s\n",
+					uio_res->maps[i].path);
 				close(fd);
 				return -1;
 			}
@@ -294,8 +295,6 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 	void *mapaddr;
 	int uio_num;
 	uint64_t phaddr;
-	uint64_t offset;
-	uint64_t pagesz;
 	int nb_maps;
 	struct rte_pci_addr *loc = &dev->addr;
 	struct mapped_pci_resource *uio_res;
@@ -336,11 +335,6 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 		return -1;
 	}
 
-	/* update devname for mmap  */
-	snprintf(devname, sizeof(devname),
-		SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/resource%d",
-		loc->domain, loc->bus, loc->devid, loc->function, 0);
-
 	/* set bus master that is not done by uio_pci_generic */
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		if (pci_uio_set_bus_master(dev->intr_handle.uio_cfg_fd)) {
@@ -370,8 +364,6 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 	uio_res->nb_maps = nb_maps;
 
 	/* Map all BARs */
-	pagesz = sysconf(_SC_PAGESIZE);
-
 	maps = uio_res->maps;
 	for (i = 0; i != PCI_MAX_RESOURCE; i++) {
 		int fd;
@@ -389,10 +381,15 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 		/* if matching map is found, then use it */
 		if (j != nb_maps) {
 			int fail = 0;
-			offset = j * pagesz;
+
+			/* update devname for mmap  */
+			snprintf(devname, sizeof(devname),
+				SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/resource%d",
+				loc->domain, loc->bus, loc->devid, loc->function,
+				i);
 
 			/*
-			 * open devname, to mmap it
+			 * open resource file, to mmap it
 			 */
 			fd = open(devname, O_RDWR);
 			if (fd < 0) {
@@ -408,7 +405,7 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 				if (pci_map_addr == NULL)
 					pci_map_addr = pci_find_max_end_va();
 
-				mapaddr = pci_map_resource(pci_map_addr, fd, (off_t)offset,
+				mapaddr = pci_map_resource(pci_map_addr, fd, 0,
 						(size_t)maps[j].size);
 				if (mapaddr == MAP_FAILED)
 					fail = 1;
@@ -416,6 +413,10 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 				pci_map_addr = RTE_PTR_ADD(mapaddr, (size_t) maps[j].size);
 			}
 
+			maps[j].path = rte_malloc(NULL, strlen(devname) + 1, 0);
+			if (maps[j].path == NULL)
+				fail = 1;
+
 			if (fail) {
 				rte_free(uio_res);
 				close(fd);
@@ -424,7 +425,8 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 			close(fd);
 
 			maps[j].addr = mapaddr;
-			maps[j].offset = offset;
+			maps[j].offset = 0;
+			strcpy(maps[j].path, devname);
 			dev->mem_resource[i].addr = mapaddr;
 		}
 	}
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
index 20e0977..7a57d0f 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_vfio.c
@@ -751,6 +751,7 @@  pci_vfio_map_resource(struct rte_pci_device *dev)
 		maps[i].addr = bar_addr;
 		maps[i].offset = reg.offset;
 		maps[i].size = reg.size;
+		maps[i].path = NULL; /* vfio doesn't have per-resource paths */
 		dev->mem_resource[i].addr = bar_addr;
 	}