DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [Patch v2] bus/vmbus: map to the correct ring buffer addresses for the secondary process
@ 2021-09-29 20:46 longli
  2021-10-13 11:55 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
  0 siblings, 1 reply; 3+ messages in thread
From: longli @ 2021-09-29 20:46 UTC (permalink / raw)
  To: dev, Stephen Hemminger, Jonathan Erb; +Cc: Long Li, stable

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.

Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")

Reported-by: Jonathan Erb <jonathan.erb@banduracyber.com>
Cc: stable@dpdk.org
Signed-off-by: Long Li <longli@microsoft.com>
---

Changes in v2:
Fixed compiling errors on ARM64.
Made log messages single lines to make it easier to locate failures.

 drivers/bus/vmbus/linux/vmbus_uio.c  | 99 +++++++++++++++-------------
 drivers/bus/vmbus/private.h          | 15 ++++-
 drivers/bus/vmbus/vmbus_channel.c    |  4 +-
 drivers/bus/vmbus/vmbus_common_uio.c | 52 +++++++++++----
 4 files changed, 107 insertions(+), 63 deletions(-)

diff --git a/drivers/bus/vmbus/linux/vmbus_uio.c b/drivers/bus/vmbus/linux/vmbus_uio.c
index b52ca5bf1..70b0d098e 100644
--- a/drivers/bus/vmbus/linux/vmbus_uio.c
+++ b/drivers/bus/vmbus/linux/vmbus_uio.c
@@ -11,6 +11,7 @@
 #include <sys/stat.h>
 #include <sys/mman.h>
 
+#include <rte_eal.h>
 #include <rte_log.h>
 #include <rte_bus.h>
 #include <rte_memory.h>
@@ -203,6 +204,37 @@ 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)",
+				UIO_MAX_SUBCHANNEL);
+			VMBUS_LOG(ERR, "Change UIO_MAX_SUBCHANNEL and recompile");
+			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 +271,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..041712fe7 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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [Patch v2] bus/vmbus: map to the correct ring buffer addresses for the secondary process
  2021-09-29 20:46 [dpdk-dev] [Patch v2] bus/vmbus: map to the correct ring buffer addresses for the secondary process longli
@ 2021-10-13 11:55 ` Thomas Monjalon
  2021-10-13 17:42   ` Long Li
  0 siblings, 1 reply; 3+ messages in thread
From: Thomas Monjalon @ 2021-10-13 11:55 UTC (permalink / raw)
  To: Long Li; +Cc: dev, Stephen Hemminger, Jonathan Erb, stable

29/09/2021 22:46, longli@linuxonhyperv.com:
> 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.
> 
> Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> 
> Reported-by: Jonathan Erb <jonathan.erb@banduracyber.com>
> Cc: stable@dpdk.org
> Signed-off-by: Long Li <longli@microsoft.com>

You forgot this ack from v1:
Acked-by: Stephen Hemminger <sthemmin@microsoft.com>

Applied, thanks



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [Patch v2] bus/vmbus: map to the correct ring buffer addresses for the secondary process
  2021-10-13 11:55 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
@ 2021-10-13 17:42   ` Long Li
  0 siblings, 0 replies; 3+ messages in thread
From: Long Li @ 2021-10-13 17:42 UTC (permalink / raw)
  To: thomas; +Cc: dev, Stephen Hemminger, Jonathan Erb, stable

> Subject: Re: [dpdk-stable] [Patch v2] bus/vmbus: map to the correct ring buffer
> addresses for the secondary process
> 
> 29/09/2021 22:46, longli@linuxonhyperv.com:
> > 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.
> >
> > Fixes: 831dba47bd36 ("bus/vmbus: add Hyper-V virtual bus support")
> >
> > Reported-by: Jonathan Erb <jonathan.erb@banduracyber.com>
> > Cc: stable@dpdk.org
> > Signed-off-by: Long Li <longli@microsoft.com>
> 
> You forgot this ack from v1:
> Acked-by: Stephen Hemminger <sthemmin@microsoft.com>

Sorry, my bad.

Thank you!

> 
> Applied, thanks
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-10-13 17:42 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-29 20:46 [dpdk-dev] [Patch v2] bus/vmbus: map to the correct ring buffer addresses for the secondary process longli
2021-10-13 11:55 ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2021-10-13 17:42   ` Long Li

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).