DPDK patches and discussions
 help / color / mirror / Atom feed
From: longli@linuxonhyperv.com
To: dev@dpdk.org, Stephen Hemminger <sthemmin@microsoft.com>,
	Jonathan Erb <jonathan.erb@banduracyber.com>
Cc: Long Li <longli@microsoft.com>, stable@dpdk.org
Subject: [dpdk-dev] [PATCH] bus/vmbus: map to the correct ring buffer addresses for the secondary process
Date: Mon, 30 Aug 2021 22:56:44 -0700	[thread overview]
Message-ID: <1630389404-13468-1-git-send-email-longli@linuxonhyperv.com> (raw)

From: Long Li <longli@microsoft.com>

The driver code had wrong assumption that all the addresses to ring buffers
in the secondary process are the same as those in the primary process. This
is not always correct as the channels could be mapped to different
addresses in the secondary process.

Fix this by keeping track of all the mapped addresses from the primary
process in the shared uio_res, and have second process map to the same
addresses.

Reported-by: Jonathan Erb <jonathan.erb@banduracyber.com>
Cc: stable@dpdk.org
Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/bus/vmbus/linux/vmbus_uio.c  | 97 +++++++++++++++-------------
 drivers/bus/vmbus/private.h          | 15 ++++-
 drivers/bus/vmbus/vmbus_channel.c    |  4 +-
 drivers/bus/vmbus/vmbus_common_uio.c | 52 +++++++++++----
 4 files changed, 105 insertions(+), 63 deletions(-)

diff --git a/drivers/bus/vmbus/linux/vmbus_uio.c b/drivers/bus/vmbus/linux/vmbus_uio.c
index b52ca5bf1..bc3436228 100644
--- a/drivers/bus/vmbus/linux/vmbus_uio.c
+++ b/drivers/bus/vmbus/linux/vmbus_uio.c
@@ -203,6 +203,36 @@ static int vmbus_uio_map_subchan(const struct rte_vmbus_device *dev,
 	struct stat sb;
 	void *mapaddr;
 	int fd;
+	struct mapped_vmbus_resource *uio_res;
+	int channel_idx;
+
+	uio_res = vmbus_uio_find_resource(dev);
+	if (!uio_res) {
+		VMBUS_LOG(ERR, "can not find resources for mapping subchan");
+		return -ENOMEM;
+	}
+
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		if (uio_res->nb_subchannels >= UIO_MAX_SUBCHANNEL) {
+			VMBUS_LOG(ERR, "exceeding max subchannels"
+				" UIO_MAX_SUBCHANNEL(%d). Increase this value"
+				" and recompile",
+				UIO_MAX_SUBCHANNEL);
+			return -ENOMEM;
+		}
+	} else {
+		for (channel_idx = 0; channel_idx < uio_res->nb_subchannels;
+		     channel_idx++)
+			if (uio_res->subchannel_maps[channel_idx].relid ==
+					chan->relid)
+				break;
+		if (channel_idx == uio_res->nb_subchannels) {
+			VMBUS_LOG(ERR, "couldn't find sub channel %d from"
+				" shared mapping in primary", chan->relid);
+			return -ENOMEM;
+		}
+		vmbus_map_addr = uio_res->subchannel_maps[channel_idx].addr;
+	}
 
 	snprintf(ring_path, sizeof(ring_path),
 		 "%s/%s/channels/%u/ring",
@@ -239,58 +269,33 @@ static int vmbus_uio_map_subchan(const struct rte_vmbus_device *dev,
 	if (mapaddr == MAP_FAILED)
 		return -EIO;
 
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+
+		/* Add this mapping to uio_res for use by secondary */
+		uio_res->subchannel_maps[uio_res->nb_subchannels].relid =
+			chan->relid;
+		uio_res->subchannel_maps[uio_res->nb_subchannels].addr =
+			mapaddr;
+		uio_res->subchannel_maps[uio_res->nb_subchannels].size =
+			file_size;
+		uio_res->nb_subchannels++;
+
+		vmbus_map_addr = RTE_PTR_ADD(mapaddr, file_size);
+	} else {
+		if (mapaddr != vmbus_map_addr) {
+			VMBUS_LOG(ERR, "failed to map channel %d to addr %p",
+					chan->relid, mapaddr);
+			vmbus_unmap_resource(mapaddr, file_size);
+			return -EIO;
+		}
+	}
+
 	*ring_size = file_size / 2;
 	*ring_buf = mapaddr;
 
-	vmbus_map_addr = RTE_PTR_ADD(mapaddr, file_size);
 	return 0;
 }
 
-int
-vmbus_uio_map_secondary_subchan(const struct rte_vmbus_device *dev,
-				const struct vmbus_channel *chan)
-{
-	const struct vmbus_br *br = &chan->txbr;
-	char ring_path[PATH_MAX];
-	void *mapaddr, *ring_buf;
-	uint32_t ring_size;
-	int fd;
-
-	snprintf(ring_path, sizeof(ring_path),
-		 "%s/%s/channels/%u/ring",
-		 SYSFS_VMBUS_DEVICES, dev->device.name,
-		 chan->relid);
-
-	ring_buf = br->vbr;
-	ring_size = br->dsize + sizeof(struct vmbus_bufring);
-	VMBUS_LOG(INFO, "secondary ring_buf %p size %u",
-		  ring_buf, ring_size);
-
-	fd = open(ring_path, O_RDWR);
-	if (fd < 0) {
-		VMBUS_LOG(ERR, "Cannot open %s: %s",
-			  ring_path, strerror(errno));
-		return -errno;
-	}
-
-	mapaddr = vmbus_map_resource(ring_buf, fd, 0, 2 * ring_size, 0);
-	close(fd);
-
-	if (mapaddr == ring_buf)
-		return 0;
-
-	if (mapaddr == MAP_FAILED)
-		VMBUS_LOG(ERR,
-			  "mmap subchan %u in secondary failed", chan->relid);
-	else {
-		VMBUS_LOG(ERR,
-			  "mmap subchan %u in secondary address mismatch",
-			  chan->relid);
-		vmbus_unmap_resource(mapaddr, 2 * ring_size);
-	}
-	return -1;
-}
-
 int vmbus_uio_map_rings(struct vmbus_channel *chan)
 {
 	const struct rte_vmbus_device *dev = chan->device;
diff --git a/drivers/bus/vmbus/private.h b/drivers/bus/vmbus/private.h
index 528d60a42..1bca147e1 100644
--- a/drivers/bus/vmbus/private.h
+++ b/drivers/bus/vmbus/private.h
@@ -33,6 +33,13 @@ struct vmbus_map {
 	uint64_t size;	/* length */
 };
 
+#define UIO_MAX_SUBCHANNEL 128
+struct subchannel_map {
+	uint16_t relid;
+	void *addr;
+	uint64_t size;
+};
+
 /*
  * For multi-process we need to reproduce all vmbus mappings in secondary
  * processes, so save them in a tailq.
@@ -41,10 +48,14 @@ struct mapped_vmbus_resource {
 	TAILQ_ENTRY(mapped_vmbus_resource) next;
 
 	rte_uuid_t id;
+
 	int nb_maps;
-	struct vmbus_channel *primary;
 	struct vmbus_map maps[VMBUS_MAX_RESOURCE];
+
 	char path[PATH_MAX];
+
+	int nb_subchannels;
+	struct subchannel_map subchannel_maps[UIO_MAX_SUBCHANNEL];
 };
 
 TAILQ_HEAD(mapped_vmbus_res_list, mapped_vmbus_resource);
@@ -105,8 +116,6 @@ bool vmbus_uio_subchannels_supported(const struct rte_vmbus_device *dev,
 int vmbus_uio_get_subchan(struct vmbus_channel *primary,
 			  struct vmbus_channel **subchan);
 int vmbus_uio_map_rings(struct vmbus_channel *chan);
-int vmbus_uio_map_secondary_subchan(const struct rte_vmbus_device *dev,
-				    const struct vmbus_channel *chan);
 
 void vmbus_br_setup(struct vmbus_br *br, void *buf, unsigned int blen);
 
diff --git a/drivers/bus/vmbus/vmbus_channel.c b/drivers/bus/vmbus/vmbus_channel.c
index f67f1c438..119b9b367 100644
--- a/drivers/bus/vmbus/vmbus_channel.c
+++ b/drivers/bus/vmbus/vmbus_channel.c
@@ -351,10 +351,8 @@ int rte_vmbus_chan_open(struct rte_vmbus_device *device,
 
 	err = vmbus_chan_create(device, device->relid, 0,
 				device->monitor_id, new_chan);
-	if (!err) {
+	if (!err)
 		device->primary = *new_chan;
-		uio_res->primary = *new_chan;
-	}
 
 	return err;
 }
diff --git a/drivers/bus/vmbus/vmbus_common_uio.c b/drivers/bus/vmbus/vmbus_common_uio.c
index 8582e32c1..9851c6e56 100644
--- a/drivers/bus/vmbus/vmbus_common_uio.c
+++ b/drivers/bus/vmbus/vmbus_common_uio.c
@@ -69,8 +69,10 @@ vmbus_uio_map_secondary(struct rte_vmbus_device *dev)
 					     fd, offset,
 					     uio_res->maps[i].size, 0);
 
-		if (mapaddr == uio_res->maps[i].addr)
+		if (mapaddr == uio_res->maps[i].addr) {
+			dev->resource[i].addr = mapaddr;
 			continue;	/* successful map */
+		}
 
 		if (mapaddr == MAP_FAILED)
 			VMBUS_LOG(ERR,
@@ -88,19 +90,39 @@ vmbus_uio_map_secondary(struct rte_vmbus_device *dev)
 	/* fd is not needed in secondary process, close it */
 	close(fd);
 
-	dev->primary = uio_res->primary;
-	if (!dev->primary) {
-		VMBUS_LOG(ERR, "missing primary channel");
-		return -1;
+	/* Create and map primary channel */
+	if (vmbus_chan_create(dev, dev->relid, 0,
+					dev->monitor_id, &dev->primary)) {
+		VMBUS_LOG(ERR, "cannot create primary channel");
+		goto failed_primary;
 	}
 
-	STAILQ_FOREACH(chan, &dev->primary->subchannel_list, next) {
-		if (vmbus_uio_map_secondary_subchan(dev, chan) != 0) {
-			VMBUS_LOG(ERR, "cannot map secondary subchan");
-			return -1;
+	/* Create and map sub channels */
+	for (i = 0; i < uio_res->nb_subchannels; i++) {
+		if (rte_vmbus_subchan_open(dev->primary, &chan)) {
+			VMBUS_LOG(ERR, "failed to create subchannel at "
+					"index %d", i);
+			goto failed_secondary;
 		}
 	}
+
 	return 0;
+
+failed_secondary:
+	while (!STAILQ_EMPTY(&dev->primary->subchannel_list)) {
+		chan = STAILQ_FIRST(&dev->primary->subchannel_list);
+		vmbus_unmap_resource(chan->txbr.vbr, chan->txbr.dsize * 2);
+		rte_vmbus_chan_close(chan);
+	}
+	rte_vmbus_chan_close(dev->primary);
+
+failed_primary:
+	for (i = 0; i != uio_res->nb_maps; i++) {
+		vmbus_unmap_resource(
+				uio_res->maps[i].addr, uio_res->maps[i].size);
+	}
+
+	return -1;
 }
 
 static int
@@ -188,6 +210,11 @@ vmbus_uio_unmap(struct mapped_vmbus_resource *uio_res)
 	if (uio_res == NULL)
 		return;
 
+	for (i = 0; i < uio_res->nb_subchannels; i++) {
+		vmbus_unmap_resource(uio_res->subchannel_maps[i].addr,
+				uio_res->subchannel_maps[i].size);
+	}
+
 	for (i = 0; i != uio_res->nb_maps; i++) {
 		vmbus_unmap_resource(uio_res->maps[i].addr,
 				     (size_t)uio_res->maps[i].size);
@@ -211,8 +238,11 @@ vmbus_uio_unmap_resource(struct rte_vmbus_device *dev)
 		return;
 
 	/* secondary processes - just free maps */
-	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
-		return vmbus_uio_unmap(uio_res);
+	if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
+		vmbus_uio_unmap(uio_res);
+		rte_free(dev->primary);
+		return;
+	}
 
 	TAILQ_REMOVE(uio_res_list, uio_res, next);
 
-- 
2.25.1


             reply	other threads:[~2021-08-31  5:58 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-31  5:56 longli [this message]
2021-08-31 17:15 ` Stephen Hemminger

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1630389404-13468-1-git-send-email-longli@linuxonhyperv.com \
    --to=longli@linuxonhyperv.com \
    --cc=dev@dpdk.org \
    --cc=jonathan.erb@banduracyber.com \
    --cc=longli@microsoft.com \
    --cc=stable@dpdk.org \
    --cc=sthemmin@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).