[dpdk-dev,v1,1/3] eal: enable uio_pci_generic support

Message ID 1422523699-17181-2-git-send-email-danny.zhou@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Zhou, Danny Jan. 29, 2015, 9:28 a.m. UTC
  1) Unify procedure to retrieve BAR resource mapping information.  
2) Setup bus master bit in NIC's PCIe configuration space for uio_pci_generic.

Signed-off-by: Danny Zhou <danny.zhou@intel.com>
Tested-by: Qun Wan <qun.wan@intel.com>
---
 lib/librte_eal/common/include/rte_pci.h            |   1 +
 lib/librte_eal/linuxapp/eal/eal_pci.c              |   2 +-
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c          | 202 +++++++++++----------
 .../linuxapp/eal/include/exec-env/rte_interrupts.h |  10 +-
 4 files changed, 119 insertions(+), 96 deletions(-)
  

Comments

Thomas Monjalon Feb. 18, 2015, 1:39 p.m. UTC | #1
Hi Danny,

I wanted to apply this patchset which was reviewed. But when having a quick
overview, I've seen some strange additions.

2015-01-29 17:28, Danny Zhou:
> 1) Unify procedure to retrieve BAR resource mapping information.  
> 2) Setup bus master bit in NIC's PCIe configuration space for uio_pci_generic.
> 
> Signed-off-by: Danny Zhou <danny.zhou@intel.com>
> Tested-by: Qun Wan <qun.wan@intel.com>
[...]
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -148,6 +148,7 @@ struct rte_pci_device {
>  	struct rte_pci_id id;                   /**< PCI ID. */
>  	struct rte_pci_resource mem_resource[PCI_MAX_RESOURCE];   /**< PCI Memory Resource */
>  	struct rte_intr_handle intr_handle;     /**< Interrupt handle */
> +	char driver_name[BUFSIZ];               /**< driver name */

Why not embedding this field in driver struct?
The name and comment should be more precise.
There is also driver->name and hotplug patchset is adding a kernel driver name.
Please bring the light in all these driver names :)

>  	const struct rte_pci_driver *driver;    /**< Associated driver */
[...]
> --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> +#define IORESOURCE_MEM        0x00000200

Please comment this value.

> --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> @@ -50,8 +50,14 @@ enum rte_intr_handle_type {
>  
>  /** Handle for interrupts. */
>  struct rte_intr_handle {
> -	int vfio_dev_fd;                 /**< VFIO device file descriptor */
> -	int fd;                          /**< file descriptor */
> +	union {
> +		int vfio_dev_fd;  /**< VFIO device file descriptor */
> +	};
> +        union {
> +		int uio_cfg_fd;  /**< UIO config file descriptor
> +					for uio_pci_generic */
> +	};

Apart the indent, it seems there is a mistake here.
Why 2 unions with 1 field each?

> +	int fd;	 /**< interrupt event file descriptor */
>  	enum rte_intr_handle_type type;  /**< handle type */
>  };
  
Zhou, Danny Feb. 19, 2015, 3:48 p.m. UTC | #2
Thomas, thanks for review and I added comments inline.

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, February 18, 2015 9:40 PM
> To: Zhou, Danny
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v1 1/3] eal: enable uio_pci_generic support
> 
> Hi Danny,
> 
> I wanted to apply this patchset which was reviewed. But when having a quick
> overview, I've seen some strange additions.
> 
> 2015-01-29 17:28, Danny Zhou:
> > 1) Unify procedure to retrieve BAR resource mapping information.
> > 2) Setup bus master bit in NIC's PCIe configuration space for uio_pci_generic.
> >
> > Signed-off-by: Danny Zhou <danny.zhou@intel.com>
> > Tested-by: Qun Wan <qun.wan@intel.com>
> [...]
> > --- a/lib/librte_eal/common/include/rte_pci.h
> > +++ b/lib/librte_eal/common/include/rte_pci.h
> > @@ -148,6 +148,7 @@ struct rte_pci_device {
> >  	struct rte_pci_id id;                   /**< PCI ID. */
> >  	struct rte_pci_resource mem_resource[PCI_MAX_RESOURCE];   /**< PCI Memory Resource */
> >  	struct rte_intr_handle intr_handle;     /**< Interrupt handle */
> > +	char driver_name[BUFSIZ];               /**< driver name */
> 
> Why not embedding this field in driver struct?
> The name and comment should be more precise.
> There is also driver->name and hotplug patchset is adding a kernel driver name.
> Please bring the light in all these driver names :)
> 

This driver_name is the name of kernel driver(e.g. vfio_pci, igb_uio, uio_pci_generic) while the driver->name is
a user-defined name for user space driver. I am going to change it to kernel_driver_name with precise comment in V2
patch, and when the V2 patch is applied, I think the function pci_get_kernel_driver_by_path() in hotplug patchset is not 
necessary then as it could directly retrieve the kernel driver name from this variable.

> >  	const struct rte_pci_driver *driver;    /**< Associated driver */
> [...]
> > --- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
> > +#define IORESOURCE_MEM        0x00000200
> 
> Please comment this value.

Will do.

> 
> > --- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > +++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
> > @@ -50,8 +50,14 @@ enum rte_intr_handle_type {
> >
> >  /** Handle for interrupts. */
> >  struct rte_intr_handle {
> > -	int vfio_dev_fd;                 /**< VFIO device file descriptor */
> > -	int fd;                          /**< file descriptor */
> > +	union {
> > +		int vfio_dev_fd;  /**< VFIO device file descriptor */
> > +	};
> > +        union {
> > +		int uio_cfg_fd;  /**< UIO config file descriptor
> > +					for uio_pci_generic */
> > +	};
> 
> Apart the indent, it seems there is a mistake here.
> Why 2 unions with 1 field each?

It is a mistake I made during code merge, will fix it in V2.

> 
> > +	int fd;	 /**< interrupt event file descriptor */
> >  	enum rte_intr_handle_type type;  /**< handle type */
> >  };
  

Patch

diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
index 66ed793..71ca882 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -148,6 +148,7 @@  struct rte_pci_device {
 	struct rte_pci_id id;                   /**< PCI ID. */
 	struct rte_pci_resource mem_resource[PCI_MAX_RESOURCE];   /**< PCI Memory Resource */
 	struct rte_intr_handle intr_handle;     /**< Interrupt handle */
+	char driver_name[BUFSIZ];               /**< driver name */
 	const struct rte_pci_driver *driver;    /**< Associated driver */
 	uint16_t max_vfs;                       /**< sriov enable if not zero */
 	int numa_node;                          /**< NUMA node connection */
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index b5f5410..8c307e9 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -512,7 +512,7 @@  pci_map_device(struct rte_pci_device *dev)
 			return ret;
 	}
 #endif
-	/* map resources for devices that use igb_uio */
+	/* map resources for devices that use uio_pci_generic or igb_uio */
 	if (!mapped) {
 		ret = pci_uio_map_resource(dev);
 		if (ret != 0)
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
index e53f06b..0f0d4e0 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
@@ -36,6 +36,7 @@ 
 #include <dirent.h>
 #include <sys/stat.h>
 #include <sys/mman.h>
+#include <linux/pci_regs.h>
 
 #include <rte_log.h>
 #include <rte_pci.h>
@@ -47,71 +48,71 @@ 
 #include "eal_filesystem.h"
 #include "eal_pci_init.h"
 
-static int pci_parse_sysfs_value(const char *filename, uint64_t *val);
+#define IORESOURCE_MEM        0x00000200
+#define UIO_PCI_GENERIC_NAME  "uio_pci_generic"
 
 void *pci_map_addr = NULL;
 
 
 #define OFF_MAX              ((uint64_t)(off_t)-1)
 static int
-pci_uio_get_mappings(const char *devname, struct pci_map maps[], int nb_maps)
+pci_uio_get_mappings(struct rte_pci_device *dev, struct pci_map maps[], int nb_maps)
 {
-	int i;
-	char dirname[PATH_MAX];
+	struct rte_pci_addr *loc = &dev->addr;
+        int i = 0;
 	char filename[PATH_MAX];
-	uint64_t offset, size;
-
-	for (i = 0; i != nb_maps; i++) {
+	unsigned long long start_addr, end_addr, flags;
+	FILE *f;
 
-		/* check if map directory exists */
-		snprintf(dirname, sizeof(dirname),
-			"%s/maps/map%u", devname, i);
+	snprintf(filename, sizeof(filename),
+		SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/resource",
+		loc->domain, loc->bus, loc->devid, loc->function);
 
-		if (access(dirname, F_OK) != 0)
-			break;
+	f = fopen(filename, "r");
+	if (f == NULL) {
+		RTE_LOG(ERR, EAL,
+		"%s(): cannot open sysfs %s\n",
+		__func__, filename);
+		return -1;
+	}
 
-		/* get mapping offset */
-		snprintf(filename, sizeof(filename),
-			"%s/offset", dirname);
-		if (pci_parse_sysfs_value(filename, &offset) < 0) {
-			RTE_LOG(ERR, EAL,
-				"%s(): cannot parse offset of %s\n",
-				__func__, dirname);
-			return -1;
+	while (fscanf(f, "%llx %llx %llx", &start_addr,
+			&end_addr, &flags) == 3 && i < nb_maps) {
+		if (flags & IORESOURCE_MEM) {
+			maps[i].offset = 0x0;
+			maps[i].size = end_addr - start_addr + 1;
+			maps[i].phaddr = start_addr;
+			i++;
 		}
+	}
+	fclose(f);
 
-		/* get mapping size */
-		snprintf(filename, sizeof(filename),
-			"%s/size", dirname);
-		if (pci_parse_sysfs_value(filename, &size) < 0) {
-			RTE_LOG(ERR, EAL,
-				"%s(): cannot parse size of %s\n",
-				__func__, dirname);
-			return -1;
-		}
+	return i;
+}
 
-		/* get mapping physical address */
-		snprintf(filename, sizeof(filename),
-			"%s/addr", dirname);
-		if (pci_parse_sysfs_value(filename, &maps[i].phaddr) < 0) {
-			RTE_LOG(ERR, EAL,
-				"%s(): cannot parse addr of %s\n",
-				__func__, dirname);
-			return -1;
-		}
+static int
+pci_uio_set_bus_master(int dev_fd)
+{
+	uint16_t reg;
+	int ret;
 
-		if ((offset > OFF_MAX) || (size > SIZE_MAX)) {
-			RTE_LOG(ERR, EAL,
-				"%s(): offset/size exceed system max value\n",
-				__func__);
-			return -1;
-		}
+	ret = pread(dev_fd, &reg, sizeof(reg), PCI_COMMAND);
+	if (ret != sizeof(reg)) {
+		RTE_LOG(ERR, EAL,
+			"Cannot read command from PCI config space!\n");
+		return -1;
+	}
+
+	reg |= PCI_COMMAND_MASTER;
 
-		maps[i].offset = offset;
-		maps[i].size = size;
+	ret = pwrite(dev_fd, &reg, sizeof(reg), PCI_COMMAND);
+	if (ret != sizeof(reg)) {
+		RTE_LOG(ERR, EAL,
+			"Cannot write command to PCI config space!\n");
+		return -1;
 	}
 
-	return i;
+	return 0;
 }
 
 static int
@@ -213,6 +214,10 @@  pci_get_uio_dev(struct rte_pci_device *dev, char *dstbuf,
 	struct dirent *e;
 	DIR *dir;
 	char dirname[PATH_MAX];
+	FILE *f;
+	char fullpath[PATH_MAX];
+	char buf[BUFSIZ];
+	char *s;
 
 	/* depending on kernel version, uio can be located in uio/uioX
 	 * or uio:uioX */
@@ -268,6 +273,30 @@  pci_get_uio_dev(struct rte_pci_device *dev, char *dstbuf,
 	if (e == NULL)
 		return -1;
 
+	/* remember driver name of uio device */
+	snprintf(fullpath, sizeof(fullpath),
+			SYSFS_PCI_DEVICES "/" PCI_PRI_FMT "/uio/%s/name" ,
+			loc->domain, loc->bus, loc->devid,
+			loc->function, e->d_name);
+
+	f = fopen(fullpath, "r");
+	if (f == NULL) {
+		RTE_LOG(ERR, EAL,
+			"%s(): cannot open sysfs to get driver name\n",
+			__func__);
+		return -1;
+	}
+	s = fgets(buf, sizeof(buf), f);
+	if (s == NULL) {
+		RTE_LOG(ERR, EAL,
+			"%s(): cannot read sysfs to get driver name\n",
+			__func__);
+		fclose(f);
+		return -1;
+	}
+	strncpy(dev->driver_name, buf, sizeof(buf));
+	fclose(f);
+
 	/* create uio device if we've been asked to */
 	if (internal_config.create_uio_dev &&
 			pci_mknod_uio_dev(dstbuf, uio_num) < 0)
@@ -282,6 +311,7 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 {
 	int i, j;
 	char dirname[PATH_MAX];
+	char cfgname[PATH_MAX];
 	char devname[PATH_MAX]; /* contains the /dev/uioX */
 	void *mapaddr;
 	int uio_num;
@@ -294,6 +324,7 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 	struct pci_map *maps;
 
 	dev->intr_handle.fd = -1;
+	dev->intr_handle.uio_cfg_fd = -1;
 	dev->intr_handle.type = RTE_INTR_HANDLE_UNKNOWN;
 
 	/* secondary processes - use already recorded details */
@@ -318,6 +349,33 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 	}
 	dev->intr_handle.type = RTE_INTR_HANDLE_UIO;
 
+	snprintf(cfgname, sizeof(cfgname),
+			"/sys/class/uio/uio%u/device/config", uio_num);
+	dev->intr_handle.uio_cfg_fd = open(cfgname, O_RDWR);
+	if (dev->intr_handle.uio_cfg_fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+			cfgname, strerror(errno));
+		return -1;
+	}
+
+	/* handle Misc. stuff for uio_pci_generic  */
+	if (strncmp(dev->driver_name, UIO_PCI_GENERIC_NAME,
+			strlen(UIO_PCI_GENERIC_NAME)) == 0) {
+		/* update devname for uio_pci_generic  */
+		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)) {
+				RTE_LOG(ERR, EAL,
+					"Cannot set up bus mastering!\n");
+				return -1;
+			}
+		}
+	}
+
 	/* allocate the mapping details for secondary processes*/
 	uio_res = rte_zmalloc("UIO_RES", sizeof(*uio_res), 0);
 	if (uio_res == NULL) {
@@ -330,13 +388,12 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 	memcpy(&uio_res->pci_addr, &dev->addr, sizeof(uio_res->pci_addr));
 
 	/* collect info about device mappings */
-	nb_maps = pci_uio_get_mappings(dirname, uio_res->maps,
-				       RTE_DIM(uio_res->maps));
+	nb_maps = pci_uio_get_mappings(dev, uio_res->maps,
+					RTE_DIM(uio_res->maps));
 	if (nb_maps < 0) {
 		rte_free(uio_res);
 		return nb_maps;
 	}
-
 	uio_res->nb_maps = nb_maps;
 
 	/* Map all BARs */
@@ -374,16 +431,10 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 			if (maps[j].addr != NULL)
 				fail = 1;
 			else {
-				/* try mapping somewhere close to the end of hugepages */
-				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(NULL, fd, (off_t)offset,
 						(size_t)maps[j].size);
-				if (mapaddr == MAP_FAILED)
+				if (mapaddr == NULL)
 					fail = 1;
-
-				pci_map_addr = RTE_PTR_ADD(mapaddr, (size_t) maps[j].size);
 			}
 
 			if (fail) {
@@ -403,38 +454,3 @@  pci_uio_map_resource(struct rte_pci_device *dev)
 
 	return 0;
 }
-
-/*
- * parse a sysfs file containing one integer value
- * different to the eal version, as it needs to work with 64-bit values
- */
-static int
-pci_parse_sysfs_value(const char *filename, uint64_t *val)
-{
-	FILE *f;
-	char buf[BUFSIZ];
-	char *end = NULL;
-
-	f = fopen(filename, "r");
-	if (f == NULL) {
-		RTE_LOG(ERR, EAL, "%s(): cannot open sysfs value %s\n",
-				__func__, filename);
-		return -1;
-	}
-
-	if (fgets(buf, sizeof(buf), f) == NULL) {
-		RTE_LOG(ERR, EAL, "%s(): cannot read sysfs value %s\n",
-				__func__, filename);
-		fclose(f);
-		return -1;
-	}
-	*val = strtoull(buf, &end, 0);
-	if ((buf[0] == '\0') || (end == NULL) || (*end != '\n')) {
-		RTE_LOG(ERR, EAL, "%s(): cannot parse sysfs value %s\n",
-				__func__, filename);
-		fclose(f);
-		return -1;
-	}
-	fclose(f);
-	return 0;
-}
diff --git a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
index 23eafd9..adc1c8d 100644
--- a/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
+++ b/lib/librte_eal/linuxapp/eal/include/exec-env/rte_interrupts.h
@@ -50,8 +50,14 @@  enum rte_intr_handle_type {
 
 /** Handle for interrupts. */
 struct rte_intr_handle {
-	int vfio_dev_fd;                 /**< VFIO device file descriptor */
-	int fd;                          /**< file descriptor */
+	union {
+		int vfio_dev_fd;  /**< VFIO device file descriptor */
+	};
+        union {
+		int uio_cfg_fd;  /**< UIO config file descriptor
+					for uio_pci_generic */
+	};
+	int fd;	 /**< interrupt event file descriptor */
 	enum rte_intr_handle_type type;  /**< handle type */
 };