[v2,1/2] bus/pci: fix secondary process PCI uio resource map problem

Message ID 20240129092231.3531217-2-chaoyong.he@corigine.com (mailing list archive)
State Superseded
Delegated to: Thomas Monjalon
Headers
Series fix secondary process PCI UIO resource problem |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chaoyong He Jan. 29, 2024, 9:22 a.m. UTC
  From: Zerun Fu <zerun.fu@corigine.com>

For the primary process, the logic loops all BARs and will skip
the map of BAR with an invalid physical address (0), also will
assign 'uio_res->nb_maps' with the real mapped BARs number. But
for the secondary process, instead of loops all BARs, the logic
using the 'uio_res->nb_map' as index. If the device uses continuous
BARs there will be no problem, whereas if it uses discrete BARs,
it will lead to mapping errors.

Fix this problem by also loops all BARs and skip the map of BAR
with an invalid physical address in secondary process.

Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
Cc: mukawa@igel.co.jp
Cc: stable@dpdk.org

Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
Reviewed-by: Long Wu <long.wu@corigine.com>
Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
---
 drivers/bus/pci/pci_common_uio.c | 94 ++++++++++++++++++++------------
 1 file changed, 60 insertions(+), 34 deletions(-)
  

Comments

fengchengwen Jan. 30, 2024, 4 a.m. UTC | #1
Hi,

Nice to see this fix.
ps: our team also founds this problem, and the bugfix is still being reviewed internally.

As for this patch, I prefer not extract subfunction, because the father is simple and just
need do some minor adjustment.

		map_idx = 0;
		for (i = 0; i != PCI_MAX_RESOURCE; i++) {
			/* skip empty BAR */
			if (dev->mem_resource[i].phys_addr == 0)
				continue;
			... // use map_idx instead i
			map_idx++;
			dev->mem_resource[i].addr = mapaddr;
		}

Also suggest use pci_uio_find_resource() to refactor pci_uio_map_secondary() function.

Thanks

On 2024/1/29 17:22, Chaoyong He wrote:
> From: Zerun Fu <zerun.fu@corigine.com>
> 
> For the primary process, the logic loops all BARs and will skip
> the map of BAR with an invalid physical address (0), also will
> assign 'uio_res->nb_maps' with the real mapped BARs number. But
> for the secondary process, instead of loops all BARs, the logic
> using the 'uio_res->nb_map' as index. If the device uses continuous
> BARs there will be no problem, whereas if it uses discrete BARs,
> it will lead to mapping errors.
> 
> Fix this problem by also loops all BARs and skip the map of BAR
> with an invalid physical address in secondary process.
> 
> Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
> Cc: mukawa@igel.co.jp
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> ---
>  drivers/bus/pci/pci_common_uio.c | 94 ++++++++++++++++++++------------
>  1 file changed, 60 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
> index 76c661f054..fcd8a49daf 100644
> --- a/drivers/bus/pci/pci_common_uio.c
> +++ b/drivers/bus/pci/pci_common_uio.c
> @@ -23,10 +23,57 @@ static struct rte_tailq_elem rte_uio_tailq = {
>  };
>  EAL_REGISTER_TAILQ(rte_uio_tailq)
>  
> +static int
> +pci_uio_map_secondary_resource_by_index(struct rte_pci_device *dev,
> +		int res_idx, struct mapped_pci_resource *uio_res, int map_idx)
> +{
> +	int fd, i;
> +
> +	if (map_idx >= uio_res->nb_maps)
> +		return -1;
> +
> +	/*
> +	 * open devname, to mmap it
> +	 */
> +	fd = open(uio_res->maps[map_idx].path, O_RDWR);
> +	if (fd < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> +			uio_res->maps[map_idx].path, strerror(errno));
> +		return -1;
> +	}
> +
> +	void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr,
> +			fd, (off_t)uio_res->maps[map_idx].offset,
> +			(size_t)uio_res->maps[map_idx].size, 0);
> +
> +	/* fd is not needed in secondary process, close it */
> +	close(fd);
> +	if (mapaddr != uio_res->maps[map_idx].addr) {
> +		RTE_LOG(ERR, EAL,
> +			"Cannot mmap device resource file %s to address: %p\n",
> +			uio_res->maps[map_idx].path,
> +			uio_res->maps[map_idx].addr);
> +		if (mapaddr != NULL) {
> +			/* unmap addrs correctly mapped */
> +			for (i = 0; i < map_idx; i++)
> +				pci_unmap_resource(
> +					uio_res->maps[i].addr,
> +					(size_t)uio_res->maps[i].size);
> +			/* unmap addr wrongly mapped */
> +			pci_unmap_resource(mapaddr,
> +				(size_t)uio_res->maps[map_idx].size);
> +		}
> +		return -1;
> +	}
> +	dev->mem_resource[res_idx].addr = mapaddr;
> +
> +	return 0;
> +}
> +
>  static int
>  pci_uio_map_secondary(struct rte_pci_device *dev)
>  {
> -	int fd, i, j;
> +	int map_idx = 0, res_idx, ret;
>  	struct mapped_pci_resource *uio_res;
>  	struct mapped_pci_res_list *uio_res_list =
>  			RTE_TAILQ_CAST(rte_uio_tailq.head, mapped_pci_res_list);
> @@ -37,41 +84,20 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
>  		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
>  			continue;
>  
> -		for (i = 0; i != uio_res->nb_maps; i++) {
> -			/*
> -			 * open devname, to mmap it
> -			 */
> -			fd = open(uio_res->maps[i].path, O_RDWR);
> -			if (fd < 0) {
> -				RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> -					uio_res->maps[i].path, strerror(errno));
> -				return -1;
> +		/* Map all BARs */
> +		for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) {
> +			 /* skip empty BAR */
> +			if (dev->mem_resource[res_idx].phys_addr == 0)
> +				continue;
> +
> +			ret = pci_uio_map_secondary_resource_by_index(dev, res_idx,
> +					uio_res, map_idx);
> +			if (ret < 0) {
> +				RTE_LOG(ERR, EAL, "Failed to map resources\n");
> +				return ret;
>  			}
>  
> -			void *mapaddr = pci_map_resource(uio_res->maps[i].addr,
> -					fd, (off_t)uio_res->maps[i].offset,
> -					(size_t)uio_res->maps[i].size, 0);
> -
> -			/* fd is not needed in secondary process, close it */
> -			close(fd);
> -			if (mapaddr != uio_res->maps[i].addr) {
> -				RTE_LOG(ERR, EAL,
> -					"Cannot mmap device resource file %s to address: %p\n",
> -					uio_res->maps[i].path,
> -					uio_res->maps[i].addr);
> -				if (mapaddr != NULL) {
> -					/* unmap addrs correctly mapped */
> -					for (j = 0; j < i; j++)
> -						pci_unmap_resource(
> -							uio_res->maps[j].addr,
> -							(size_t)uio_res->maps[j].size);
> -					/* unmap addr wrongly mapped */
> -					pci_unmap_resource(mapaddr,
> -						(size_t)uio_res->maps[i].size);
> -				}
> -				return -1;
> -			}
> -			dev->mem_resource[i].addr = mapaddr;
> +			map_idx++;
>  		}
>  		return 0;
>  	}
>
  
Burakov, Anatoly March 14, 2024, 10:55 a.m. UTC | #2
On 1/29/2024 10:22 AM, Chaoyong He wrote:
> From: Zerun Fu <zerun.fu@corigine.com>
> 
> For the primary process, the logic loops all BARs and will skip
> the map of BAR with an invalid physical address (0), also will
> assign 'uio_res->nb_maps' with the real mapped BARs number. But
> for the secondary process, instead of loops all BARs, the logic
> using the 'uio_res->nb_map' as index. If the device uses continuous
> BARs there will be no problem, whereas if it uses discrete BARs,
> it will lead to mapping errors.
> 
> Fix this problem by also loops all BARs and skip the map of BAR
> with an invalid physical address in secondary process.
> 
> Fixes: 9b957f378abf ("pci: merge uio functions for linux and bsd")
> Cc: mukawa@igel.co.jp
> Cc: stable@dpdk.org
> 
> Signed-off-by: Zerun Fu <zerun.fu@corigine.com>
> Reviewed-by: Chaoyong He <chaoyong.he@corigine.com>
> Reviewed-by: Long Wu <long.wu@corigine.com>
> Reviewed-by: Peng Zhang <peng.zhang@corigine.com>
> ---
>   drivers/bus/pci/pci_common_uio.c | 94 ++++++++++++++++++++------------
>   1 file changed, 60 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
> index 76c661f054..fcd8a49daf 100644
> --- a/drivers/bus/pci/pci_common_uio.c
> +++ b/drivers/bus/pci/pci_common_uio.c
> @@ -23,10 +23,57 @@ static struct rte_tailq_elem rte_uio_tailq = {
>   };
>   EAL_REGISTER_TAILQ(rte_uio_tailq)
>   
> +static int
> +pci_uio_map_secondary_resource_by_index(struct rte_pci_device *dev,
> +		int res_idx, struct mapped_pci_resource *uio_res, int map_idx)
> +{
> +	int fd, i;
> +
> +	if (map_idx >= uio_res->nb_maps)
> +		return -1;
> +
> +	/*
> +	 * open devname, to mmap it
> +	 */
> +	fd = open(uio_res->maps[map_idx].path, O_RDWR);
> +	if (fd < 0) {
> +		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> +			uio_res->maps[map_idx].path, strerror(errno));
> +		return -1;
> +	}
> +
> +	void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr,
> +			fd, (off_t)uio_res->maps[map_idx].offset,
> +			(size_t)uio_res->maps[map_idx].size, 0);
> +
> +	/* fd is not needed in secondary process, close it */
> +	close(fd);
> +	if (mapaddr != uio_res->maps[map_idx].addr) {
> +		RTE_LOG(ERR, EAL,
> +			"Cannot mmap device resource file %s to address: %p\n",
> +			uio_res->maps[map_idx].path,
> +			uio_res->maps[map_idx].addr);
> +		if (mapaddr != NULL) {
> +			/* unmap addrs correctly mapped */
> +			for (i = 0; i < map_idx; i++)
> +				pci_unmap_resource(
> +					uio_res->maps[i].addr,
> +					(size_t)uio_res->maps[i].size);
> +			/* unmap addr wrongly mapped */
> +			pci_unmap_resource(mapaddr,
> +				(size_t)uio_res->maps[map_idx].size);
> +		}
> +		return -1;
> +	}
> +	dev->mem_resource[res_idx].addr = mapaddr;
> +
> +	return 0;
> +}
> +
>   static int
>   pci_uio_map_secondary(struct rte_pci_device *dev)
>   {
> -	int fd, i, j;
> +	int map_idx = 0, res_idx, ret;
>   	struct mapped_pci_resource *uio_res;
>   	struct mapped_pci_res_list *uio_res_list =
>   			RTE_TAILQ_CAST(rte_uio_tailq.head, mapped_pci_res_list);
> @@ -37,41 +84,20 @@ pci_uio_map_secondary(struct rte_pci_device *dev)
>   		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
>   			continue;
>   
> -		for (i = 0; i != uio_res->nb_maps; i++) {
> -			/*
> -			 * open devname, to mmap it
> -			 */
> -			fd = open(uio_res->maps[i].path, O_RDWR);
> -			if (fd < 0) {
> -				RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
> -					uio_res->maps[i].path, strerror(errno));
> -				return -1;
> +		/* Map all BARs */
> +		for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) {
> +			 /* skip empty BAR */
> +			if (dev->mem_resource[res_idx].phys_addr == 0)
> +				continue;
> +
> +			ret = pci_uio_map_secondary_resource_by_index(dev, res_idx,
> +					uio_res, map_idx);
> +			if (ret < 0) {
> +				RTE_LOG(ERR, EAL, "Failed to map resources\n");
> +				return ret;
>   			}
>   
> -			void *mapaddr = pci_map_resource(uio_res->maps[i].addr,
> -					fd, (off_t)uio_res->maps[i].offset,
> -					(size_t)uio_res->maps[i].size, 0);
> -
> -			/* fd is not needed in secondary process, close it */
> -			close(fd);
> -			if (mapaddr != uio_res->maps[i].addr) {
> -				RTE_LOG(ERR, EAL,
> -					"Cannot mmap device resource file %s to address: %p\n",
> -					uio_res->maps[i].path,
> -					uio_res->maps[i].addr);
> -				if (mapaddr != NULL) {
> -					/* unmap addrs correctly mapped */
> -					for (j = 0; j < i; j++)
> -						pci_unmap_resource(
> -							uio_res->maps[j].addr,
> -							(size_t)uio_res->maps[j].size);
> -					/* unmap addr wrongly mapped */
> -					pci_unmap_resource(mapaddr,
> -						(size_t)uio_res->maps[i].size);

Nitpicking, but I feel like this would've been better done from the 
caller, not here. This is how it's done in primary process.

> -				}
> -				return -1;
> -			}
> -			dev->mem_resource[i].addr = mapaddr;
> +			map_idx++;
>   		}
>   		return 0;
>   	}

With fix above,

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
  

Patch

diff --git a/drivers/bus/pci/pci_common_uio.c b/drivers/bus/pci/pci_common_uio.c
index 76c661f054..fcd8a49daf 100644
--- a/drivers/bus/pci/pci_common_uio.c
+++ b/drivers/bus/pci/pci_common_uio.c
@@ -23,10 +23,57 @@  static struct rte_tailq_elem rte_uio_tailq = {
 };
 EAL_REGISTER_TAILQ(rte_uio_tailq)
 
+static int
+pci_uio_map_secondary_resource_by_index(struct rte_pci_device *dev,
+		int res_idx, struct mapped_pci_resource *uio_res, int map_idx)
+{
+	int fd, i;
+
+	if (map_idx >= uio_res->nb_maps)
+		return -1;
+
+	/*
+	 * open devname, to mmap it
+	 */
+	fd = open(uio_res->maps[map_idx].path, O_RDWR);
+	if (fd < 0) {
+		RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
+			uio_res->maps[map_idx].path, strerror(errno));
+		return -1;
+	}
+
+	void *mapaddr = pci_map_resource(uio_res->maps[map_idx].addr,
+			fd, (off_t)uio_res->maps[map_idx].offset,
+			(size_t)uio_res->maps[map_idx].size, 0);
+
+	/* fd is not needed in secondary process, close it */
+	close(fd);
+	if (mapaddr != uio_res->maps[map_idx].addr) {
+		RTE_LOG(ERR, EAL,
+			"Cannot mmap device resource file %s to address: %p\n",
+			uio_res->maps[map_idx].path,
+			uio_res->maps[map_idx].addr);
+		if (mapaddr != NULL) {
+			/* unmap addrs correctly mapped */
+			for (i = 0; i < map_idx; i++)
+				pci_unmap_resource(
+					uio_res->maps[i].addr,
+					(size_t)uio_res->maps[i].size);
+			/* unmap addr wrongly mapped */
+			pci_unmap_resource(mapaddr,
+				(size_t)uio_res->maps[map_idx].size);
+		}
+		return -1;
+	}
+	dev->mem_resource[res_idx].addr = mapaddr;
+
+	return 0;
+}
+
 static int
 pci_uio_map_secondary(struct rte_pci_device *dev)
 {
-	int fd, i, j;
+	int map_idx = 0, res_idx, ret;
 	struct mapped_pci_resource *uio_res;
 	struct mapped_pci_res_list *uio_res_list =
 			RTE_TAILQ_CAST(rte_uio_tailq.head, mapped_pci_res_list);
@@ -37,41 +84,20 @@  pci_uio_map_secondary(struct rte_pci_device *dev)
 		if (rte_pci_addr_cmp(&uio_res->pci_addr, &dev->addr))
 			continue;
 
-		for (i = 0; i != uio_res->nb_maps; i++) {
-			/*
-			 * open devname, to mmap it
-			 */
-			fd = open(uio_res->maps[i].path, O_RDWR);
-			if (fd < 0) {
-				RTE_LOG(ERR, EAL, "Cannot open %s: %s\n",
-					uio_res->maps[i].path, strerror(errno));
-				return -1;
+		/* Map all BARs */
+		for (res_idx = 0; res_idx != PCI_MAX_RESOURCE; res_idx++) {
+			 /* skip empty BAR */
+			if (dev->mem_resource[res_idx].phys_addr == 0)
+				continue;
+
+			ret = pci_uio_map_secondary_resource_by_index(dev, res_idx,
+					uio_res, map_idx);
+			if (ret < 0) {
+				RTE_LOG(ERR, EAL, "Failed to map resources\n");
+				return ret;
 			}
 
-			void *mapaddr = pci_map_resource(uio_res->maps[i].addr,
-					fd, (off_t)uio_res->maps[i].offset,
-					(size_t)uio_res->maps[i].size, 0);
-
-			/* fd is not needed in secondary process, close it */
-			close(fd);
-			if (mapaddr != uio_res->maps[i].addr) {
-				RTE_LOG(ERR, EAL,
-					"Cannot mmap device resource file %s to address: %p\n",
-					uio_res->maps[i].path,
-					uio_res->maps[i].addr);
-				if (mapaddr != NULL) {
-					/* unmap addrs correctly mapped */
-					for (j = 0; j < i; j++)
-						pci_unmap_resource(
-							uio_res->maps[j].addr,
-							(size_t)uio_res->maps[j].size);
-					/* unmap addr wrongly mapped */
-					pci_unmap_resource(mapaddr,
-						(size_t)uio_res->maps[i].size);
-				}
-				return -1;
-			}
-			dev->mem_resource[i].addr = mapaddr;
+			map_idx++;
 		}
 		return 0;
 	}