[v3] bus/pci: don't open uio device in secondary process

Message ID 20241011111533.20746-1-konrad.sztyber@intel.com (mailing list archive)
State New
Delegated to: David Marchand
Headers
Series [v3] bus/pci: don't open uio device in secondary process |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/github-robot: build success github build: passed
ci/iol-unit-amd64-testing fail Testing issues
ci/Intel-compilation success Compilation OK
ci/iol-compile-arm64-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS RETEST #1
ci/iol-intel-Functional success Functional Testing PASS RETEST #1
ci/iol-marvell-Functional success Functional Testing PASS RETEST #1
ci/iol-broadcom-Performance success Performance Testing PASS RETEST #1
ci/iol-sample-apps-testing success Testing PASS RETEST #1

Commit Message

Konrad Sztyber Oct. 11, 2024, 11:15 a.m. UTC
The uio_pci_generic driver clears the bus master bit when the device
file is closed. So, when the secondary process terminates after probing
a device, that device becomes unusable in the primary process.

To avoid that, the device file is now opened only in the primary process
and the secondary gets it over UNIX domain socket via SCM_RIGHTS.

Fixes: 847d78fb9530 ("bus/pci: fix FD in secondary process")
Cc: stable@dpdk.org

Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
---
v3:
  Use the rte_mp_* infrastructure to pass the uio fd from the primary
  process to the secondary.
v2:
  Fixed coding style issues.
---
 drivers/bus/pci/linux/pci_uio.c | 140 ++++++++++++++++++++++++++++----
 1 file changed, 126 insertions(+), 14 deletions(-)
  

Comments

David Marchand Oct. 24, 2024, 9:05 a.m. UTC | #1
On Fri, Oct 11, 2024 at 1:17 PM Konrad Sztyber <konrad.sztyber@intel.com> wrote:
>
> The uio_pci_generic driver clears the bus master bit when the device
> file is closed. So, when the secondary process terminates after probing
> a device, that device becomes unusable in the primary process.
>
> To avoid that, the device file is now opened only in the primary process
> and the secondary gets it over UNIX domain socket via SCM_RIGHTS.
>
> Fixes: 847d78fb9530 ("bus/pci: fix FD in secondary process")
> Cc: stable@dpdk.org
>
> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>

Recheck-request: rebase=main,iol-compile-amd64-testing
  
Konrad Sztyber Nov. 19, 2024, 10:39 a.m. UTC | #2
On 10/24/24 11:05, David Marchand wrote:
> On Fri, Oct 11, 2024 at 1:17 PM Konrad Sztyber <konrad.sztyber@intel.com> wrote:
>>
>> The uio_pci_generic driver clears the bus master bit when the device
>> file is closed. So, when the secondary process terminates after probing
>> a device, that device becomes unusable in the primary process.
>>
>> To avoid that, the device file is now opened only in the primary process
>> and the secondary gets it over UNIX domain socket via SCM_RIGHTS.
>>
>> Fixes: 847d78fb9530 ("bus/pci: fix FD in secondary process")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Konrad Sztyber <konrad.sztyber@intel.com>
> 
> Recheck-request: rebase=main,iol-compile-amd64-testing

Is there anything that's required of me to get this patch merged?
  

Patch

diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c
index 4c1d3327a9..220390d921 100644
--- a/drivers/bus/pci/linux/pci_uio.c
+++ b/drivers/bus/pci/linux/pci_uio.c
@@ -21,14 +21,22 @@ 
 #include <rte_bus_pci.h>
 #include <rte_common.h>
 #include <rte_malloc.h>
+#include <rte_eal.h>
+#include <rte_errno.h>
 
 #include "eal_filesystem.h"
 #include "pci_init.h"
 #include "private.h"
 
 void *pci_map_addr = NULL;
+static int pci_uio_dev_count;
 
 #define OFF_MAX              ((uint64_t)(off_t)-1)
+#define SEND_FD_MP_KEY       "pci_uio_send_fd"
+
+struct pci_uio_send_fd_param {
+	struct rte_pci_addr addr;
+};
 
 int
 pci_uio_read_config(const struct rte_intr_handle *intr_handle,
@@ -211,6 +219,93 @@  pci_uio_free_resource(struct rte_pci_device *dev,
 		rte_intr_fd_set(dev->intr_handle, -1);
 		rte_intr_type_set(dev->intr_handle, RTE_INTR_HANDLE_UNKNOWN);
 	}
+
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		assert(pci_uio_dev_count > 0);
+		pci_uio_dev_count--;
+		if (pci_uio_dev_count == 0)
+			rte_mp_action_unregister(SEND_FD_MP_KEY);
+	}
+}
+
+static int
+pci_uio_send_fd(const struct rte_mp_msg *request, const void *peer)
+{
+	struct rte_pci_device *dev;
+	const struct pci_uio_send_fd_param *param =
+		(const struct pci_uio_send_fd_param *)request->param;
+	struct rte_mp_msg reply = {};
+	int fd;
+
+	strlcpy(reply.name, request->name, sizeof(reply.name));
+	TAILQ_FOREACH(dev, &rte_pci_bus.device_list, next) {
+		if (!rte_pci_addr_cmp(&dev->addr, &param->addr))
+			break;
+	}
+
+	if (dev == NULL) {
+		PCI_LOG(ERR, "Could not find PCI device (" PCI_PRI_FMT ")",
+			param->addr.domain, param->addr.bus,
+			param->addr.devid, param->addr.function);
+		goto reply;
+	}
+
+	fd = rte_intr_fd_get(dev->intr_handle);
+	if (fd < 0) {
+		PCI_LOG(ERR, "Could not get fd (" PCI_PRI_FMT ")",
+			param->addr.domain, param->addr.bus,
+			param->addr.devid, param->addr.function);
+		goto reply;
+	}
+
+	reply.num_fds = 1;
+	reply.fds[0] = fd;
+reply:
+	if (rte_mp_reply(&reply, peer) != 0) {
+		PCI_LOG(ERR, "Failed to send reply: %d (" PCI_PRI_FMT ")",
+			rte_errno, param->addr.domain, param->addr.bus,
+			param->addr.devid, param->addr.function);
+		return -1;
+	}
+
+	return 0;
+}
+
+static int
+pci_uio_request_fd(struct rte_pci_device *dev)
+{
+	struct rte_mp_msg request = {}, *reply;
+	struct timespec timeout = {.tv_sec = 1, .tv_nsec = 0};
+	struct pci_uio_send_fd_param *param =
+		(struct pci_uio_send_fd_param *)request.param;
+	struct rte_mp_reply replies;
+	int rc;
+
+	strlcpy(request.name, SEND_FD_MP_KEY, sizeof(request.name));
+	memcpy(&param->addr, &dev->addr, sizeof(param->addr));
+	request.len_param = sizeof(*param);
+
+	rc = rte_mp_request_sync(&request, &replies, &timeout);
+	if (rc != 0 || replies.nb_received != 1) {
+		PCI_LOG(ERR, "Failed to request fd from primary: %d (" PCI_PRI_FMT ")",
+			rte_errno, dev->addr.domain, dev->addr.bus,
+			dev->addr.devid, dev->addr.function);
+		return -1;
+	}
+
+	reply = replies.msgs;
+	if (reply->num_fds != 1) {
+		PCI_LOG(ERR, "Received unexpected number of fds: %d (" PCI_PRI_FMT ")",
+			reply->num_fds, dev->addr.domain, dev->addr.bus,
+			dev->addr.devid, dev->addr.function);
+		free(reply);
+		return -1;
+	}
+
+	rte_intr_fd_set(dev->intr_handle, reply->fds[0]);
+	free(reply);
+
+	return 0;
 }
 
 int
@@ -220,7 +315,7 @@  pci_uio_alloc_resource(struct rte_pci_device *dev,
 	char dirname[PATH_MAX];
 	char cfgname[PATH_MAX];
 	char devname[PATH_MAX]; /* contains the /dev/uioX */
-	int uio_num, fd, uio_cfg_fd;
+	int rc, uio_num, fd, uio_cfg_fd;
 	struct rte_pci_addr *loc;
 
 	loc = &dev->addr;
@@ -232,18 +327,6 @@  pci_uio_alloc_resource(struct rte_pci_device *dev,
 			loc->domain, loc->bus, loc->devid, loc->function);
 		return 1;
 	}
-	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
-
-	/* save fd */
-	fd = open(devname, O_RDWR);
-	if (fd < 0) {
-		PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
-		goto error;
-	}
-
-	if (rte_intr_fd_set(dev->intr_handle, fd))
-		goto error;
-
 	snprintf(cfgname, sizeof(cfgname),
 			"/sys/class/uio/uio%u/device/config", uio_num);
 
@@ -270,8 +353,27 @@  pci_uio_alloc_resource(struct rte_pci_device *dev,
 		}
 	}
 
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+		if (pci_uio_request_fd(dev) != 0)
+			goto error;
 		return 0;
+	}
+
+	/*
+	 * The uio_pci_generic driver clears the bus master enable bit when the
+	 * device file is closed, so open it only in the primary process. The
+	 * secondary will get it via SCM_RIGHTS.
+	 */
+	snprintf(devname, sizeof(devname), "/dev/uio%u", uio_num);
+	/* save fd */
+	fd = open(devname, O_RDWR);
+	if (fd < 0) {
+		PCI_LOG(ERR, "Cannot open %s: %s", devname, strerror(errno));
+		goto error;
+	}
+
+	if (rte_intr_fd_set(dev->intr_handle, fd))
+		goto error;
 
 	/* allocate the mapping details for secondary processes*/
 	*uio_res = rte_zmalloc("UIO_RES", sizeof(**uio_res), 0);
@@ -280,6 +382,16 @@  pci_uio_alloc_resource(struct rte_pci_device *dev,
 		goto error;
 	}
 
+	if (pci_uio_dev_count == 0) {
+		rc = rte_mp_action_register(SEND_FD_MP_KEY, pci_uio_send_fd);
+		if (rc != 0 && rte_errno != ENOTSUP) {
+			PCI_LOG(ERR, "Failed to register multi-process callback: %d",
+				rte_errno);
+			goto error;
+		}
+	}
+
+	pci_uio_dev_count++;
 	strlcpy((*uio_res)->path, devname, sizeof((*uio_res)->path));
 	memcpy(&(*uio_res)->pci_addr, &dev->addr, sizeof((*uio_res)->pci_addr));