[dpdk-dev] [PATCH 0/3] net/virtio: a temp to fix few multiple process issues

[dpdk-dev] [PATCH 0/3] net/virtio: a temp to fix few multiple process issues

[Yuanhan Liu]

Here is a temp to fix few multiple process issues: only did some basic
test, not positive sure (though it's likely) it will fix the issue
reported by Yaron.

Yaron, please help testing. Thanks!

---
Yuanhan Liu (3):
  net/virtio: fix crash for secondary process
  net/virtio: fix multiple process support
  net/virtio: fix wrong Rx/Tx method for secondary process

 drivers/net/virtio/virtio_ethdev.c      | 27 ++++++++++++++++++++++++++-
 drivers/net/virtio/virtio_pci.c         |  4 ++--
 drivers/net/virtio/virtio_pci.h         |  4 ++++
 drivers/net/virtio/virtio_user_ethdev.c |  2 +-
 4 files changed, 33 insertions(+), 4 deletions(-)

-- 
1.9.0

[dpdk-dev] [PATCH 1/3] net/virtio: fix crash for secondary process

[Yuanhan Liu]

If a virtio net device is managed by kernel driver, the primary
process would be aware of it and skip it.

For secondary process, however, it isn't aware of that. Instead,
the 'hw' would be NULL. If we keep going on with the rx_func_get(),
a crash would happen. For such case, we simply return 1 to let
EAL skip this device. Thus, the crashed could be fixed.

Fixes: c1f86306a026 ("virtio: add new driver")

Cc: stable@dpdk.org
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 079fd6c..f2a803b 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1304,6 +1304,15 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		/*
+		 * if a virtio net device is managed by kernel driver,
+		 * the secondary process won't be aware of it. Instead,
+		 * it sees hw being set with NULL. For such case, here
+		 * we returns 1 to let EAL skip this device.
+		 */
+		if (!hw)
+			return 1;
+
 		rx_func_get(eth_dev);
 		return 0;
 	}
-- 
1.9.0

[dpdk-dev] [PATCH 2/3] net/virtio: fix multiple process support

[Yuanhan Liu]

The introduce of virtio 1.0 support brings yet another set of ops, badly,
it's not handled correctly, that it breaks the multiple process support.

The issue is the function pointer may vary from different processes, and
the current code just does one time set (for primary process only). That
said, the function pointer the secondary process saw is actually from the
primary process space. Accessing it could likely result to a crash.

The fix is somehow straightforward (at least, way more simpler than what
I have firstly thought): just reset the function pointers for the secondary
process on init stage.

Fixes: d5bbeefca826 ("virtio: introduce PCI implementation structure")

Cc: stable@dpdk.org
Reported-by: Yaron Illouz <yaroni@radcom.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_ethdev.c      | 12 ++++++++++++
 drivers/net/virtio/virtio_pci.c         |  4 ++--
 drivers/net/virtio/virtio_pci.h         |  4 ++++
 drivers/net/virtio/virtio_user_ethdev.c |  2 +-
 4 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index f2a803b..a9f7ae4 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1174,6 +1174,17 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 }
 
 static void
+set_vtpci_ops(struct rte_eth_dev *eth_dev, struct virtio_hw *hw)
+{
+	if (eth_dev->pci_dev == NULL)
+		hw->vtpci_ops = &virtio_user_ops;
+	else if (hw->modern)
+		hw->vtpci_ops = &modern_ops;
+	else
+		hw->vtpci_ops = &legacy_ops;
+}
+
+static void
 rx_func_get(struct rte_eth_dev *eth_dev)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
@@ -1313,6 +1324,7 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 		if (!hw)
 			return 1;
 
+		set_vtpci_ops(eth_dev, hw);
 		rx_func_get(eth_dev);
 		return 0;
 	}
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 9b47165..e9e53a5 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -300,7 +300,7 @@
 	return 0;
 }
 
-static const struct virtio_pci_ops legacy_ops = {
+const struct virtio_pci_ops legacy_ops = {
 	.read_dev_cfg	= legacy_read_dev_config,
 	.write_dev_cfg	= legacy_write_dev_config,
 	.reset		= legacy_reset,
@@ -516,7 +516,7 @@
 	io_write16(1, vq->notify_addr);
 }
 
-static const struct virtio_pci_ops modern_ops = {
+const struct virtio_pci_ops modern_ops = {
 	.read_dev_cfg	= modern_read_dev_config,
 	.write_dev_cfg	= modern_write_dev_config,
 	.reset		= modern_reset,
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index de271bf..3e1e911 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -317,4 +317,8 @@ int vtpci_init(struct rte_pci_device *, struct virtio_hw *,
 
 uint16_t vtpci_irq_config(struct virtio_hw *, uint16_t);
 
+extern const struct virtio_pci_ops legacy_ops;
+extern const struct virtio_pci_ops modern_ops;
+extern const struct virtio_pci_ops virtio_user_ops;
+
 #endif /* _VIRTIO_PCI_H_ */
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 406beea..26eca37 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -212,7 +212,7 @@
 			    strerror(errno));
 }
 
-static const struct virtio_pci_ops virtio_user_ops = {
+const struct virtio_pci_ops virtio_user_ops = {
 	.read_dev_cfg	= virtio_user_read_dev_config,
 	.write_dev_cfg	= virtio_user_write_dev_config,
 	.reset		= virtio_user_reset,
-- 
1.9.0

[dpdk-dev] [PATCH 3/3] net/virtio: fix wrong Rx/Tx method for secondary process

[Yuanhan Liu]

If the primary enables the vector Rx/Tx path, the current code would
let the secondary always choose the non vector Rx/Tx path. This results
to a Rx/Tx method mismatch between primary and secondary process. Werid
errors then may happen, something like:

    PMD: virtio_xmit_pkts() tx: virtqueue_enqueue error: -14

Fix it by choosing the correct Rx/Tx callbacks for the secondary process.
That is, use vector path if it's given.

Fixes: 8d8393fb1861 ("virtio: pick simple Rx/Tx")

Cc: stable@dpdk.org
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index a9f7ae4..04d93de 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1325,7 +1325,11 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 			return 1;
 
 		set_vtpci_ops(eth_dev, hw);
-		rx_func_get(eth_dev);
+		if (hw->use_simple_rxtx) {
+			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
+			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
+		} else
+			rx_func_get(eth_dev);
 		return 0;
 	}
 
-- 
1.9.0

Re: [dpdk-dev] [PATCH 0/3] net/virtio: a temp to fix few multiple process issues

[Yuanhan Liu]

On Thu, Dec 22, 2016 at 03:18:40PM +0800, Yuanhan Liu wrote:
> Here is a temp to fix few multiple process issues: only did some basic
            ^^^^

Sign.., I meant "attempt" :-/

	--yliu

Re: [dpdk-dev] [PATCH 0/3] net/virtio: a temp to fix few multiple process issues

[Xu, Qian Q]

Yaron
Could you also share your test case step by step here? I want to learn from u on the cases. 

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Yuanhan Liu
> Sent: Thursday, December 22, 2016 3:19 PM
> To: dev@dpdk.org
> Cc: Yaron Illouz <yaroni@radcom.com>; Yuanhan Liu
> <yuanhan.liu@linux.intel.com>
> Subject: [dpdk-dev] [PATCH 0/3] net/virtio: a temp to fix few multiple process
> issues
> 
> Here is a temp to fix few multiple process issues: only did some basic test, not
> positive sure (though it's likely) it will fix the issue reported by Yaron.
> 
> Yaron, please help testing. Thanks!
> 
> ---
> Yuanhan Liu (3):
>   net/virtio: fix crash for secondary process
>   net/virtio: fix multiple process support
>   net/virtio: fix wrong Rx/Tx method for secondary process
> 
>  drivers/net/virtio/virtio_ethdev.c      | 27 ++++++++++++++++++++++++++-
>  drivers/net/virtio/virtio_pci.c         |  4 ++--
>  drivers/net/virtio/virtio_pci.h         |  4 ++++
>  drivers/net/virtio/virtio_user_ethdev.c |  2 +-
>  4 files changed, 33 insertions(+), 4 deletions(-)
> 
> --
> 1.9.0

[dpdk-dev] [PATCH v2 0/6] net/virtio: fix several multiple process issues

[Yuanhan Liu]

This patch series fixes few crash issues regarding to multiple process
model. In my limited fuzzy test, now it works for both virtio 0.95 and
1.0, as well as for the case some virtio-net devices are managed by
kernel device while some others are managed by DPDK.

---
Maintaining the multiple process support is not an easy task -- you
have to be very mindful while coding -- what kind of stuff should
and should not be in shared memory. Otherwise, it's very likely the
multiple process model will be broken.

A typical example is the ops pointer, a pointer to a set of function
pointers.  Normally, it's a pointer stored in a read-only data section
of the application:

    static const struct virtio_pci_ops legacy_ops = {
            ...,
    }

The pointer, of course, may vary in different process space. If,
however, we store the pointer into shared memory, we could only
have one value for it.  Setting it from process A and accessing
it from process B would likely lead to an illegal memory access.
As a result, crash happens.

The fix is to keep those addresses locally, in a new struct,
virtio_hw_internal. By that, each process maintains it's own
version of the pointer (from its own process space). Thus,
everything would work as expected.

---
Yuanhan Liu (6):
  ethdev: fix port data mismatched in multiple process model
  net/virtio: fix wrong Rx/Tx method for secondary process
  net/virtio: store PCI operators pointer locally
  net/virtio: store IO port info locally
  net/virtio: fix multiple process support
  net/virtio: remove dead structure field

 drivers/net/virtio/virtio_ethdev.c      | 69 +++++++++++++++++++++++++---
 drivers/net/virtio/virtio_pci.c         | 81 +++++++++++++++++----------------
 drivers/net/virtio/virtio_pci.h         | 25 ++++++++--
 drivers/net/virtio/virtio_user_ethdev.c |  5 +-
 drivers/net/virtio/virtqueue.h          |  2 +-
 lib/librte_ether/rte_ethdev.c           | 58 ++++++++++++++++++++---
 6 files changed, 182 insertions(+), 58 deletions(-)

-- 
2.8.1

[dpdk-dev] [PATCH v2 1/6] ethdev: fix port data mismatched in multiple process model

[Yuanhan Liu]

Assume we have two virtio ports, 00:03.0 and 00:04.0. The first one is
managed by the kernel driver, while the later one is managed by DPDK.

Now we start the primary process. 00:03.0 will be skipped by DPDK virtio
PMD driver (since it's being used by the kernel). 00:04.0 would be
successfully initiated by DPDK virtio PMD (if nothing abnormal happens).
After that, we would get a port id 0, and all the related info needed
by virtio (virtio_hw) is stored at rte_eth_dev_data[0].

Then we start the secondary process. As usual, 00:03.0 will be firstly
probed. It firstly tries to get a local eth_dev structure for it (by
rte_eth_dev_allocate):

        port_id = rte_eth_dev_find_free_port();
        ...

        eth_dev = &rte_eth_devices[port_id];
        eth_dev->data = &rte_eth_dev_data[port_id];
        ...

        return eth_dev;

Since it's a first PCI device, port_id will be 0. eth_dev->data would
then point to rte_eth_dev_data[0]. And here things start going wrong,
as rte_eth_dev_data[0] actually stores the virtio_hw for 00:04.0.

That said, in the secondary process, DPDK will continue to drive PCI
device 00.03.0 (despite the fact it's been managed by kernel), with
the info from PCI device 00:04.0. Which is wrong.

The fix is to attach the port already registered by the primary process:
iterate the rte_eth_dev_data[], and get the port id who's PCI ID matches
the current PCI device.

This would let us maintain same port ID for the same PCI device, keeping
the chance of referencing to wrong data minimal.

Fixes: af75078fece3 ("first public release")

Cc: stable@dpdk.org
Cc: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: Bruce Richardson <bruce.richardson@intel.com>
Cc: Ferruh Yigit <ferruh.yigit@intel.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_ether/rte_ethdev.c | 58 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fde8112..c10eb9c 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -201,9 +201,6 @@ rte_eth_dev_allocate(const char *name)
 		return NULL;
 	}
 
-	if (rte_eth_dev_data == NULL)
-		rte_eth_dev_data_alloc();
-
 	if (rte_eth_dev_allocated(name) != NULL) {
 		RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s already allocated!\n",
 				name);
@@ -231,6 +228,38 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 	return 0;
 }
 
+/*
+ * Attach to a port already registered by the primary process, which
+ * makes sure that the same device would both have the same port id
+ * in the primary and secondary process.
+ */
+static struct rte_eth_dev *
+eth_dev_attach(const char *name)
+{
+	uint8_t i;
+	struct rte_eth_dev *eth_dev;
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
+			break;
+	}
+	if (i == RTE_MAX_ETHPORTS) {
+		RTE_PMD_DEBUG_TRACE(
+			"device %s is not driven by the primary process\n",
+			name);
+		return NULL;
+	}
+
+	RTE_ASSERT(eth_dev->data->port_id == i);
+
+	eth_dev = &rte_eth_devices[i];
+	eth_dev->data = &rte_eth_dev_data[i];
+	eth_dev->attached = DEV_ATTACHED;
+	nb_ports++;
+
+	return eth_dev;
+}
+
 int
 rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
 		      struct rte_pci_device *pci_dev)
@@ -246,9 +275,26 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
 	rte_eal_pci_device_name(&pci_dev->addr, ethdev_name,
 			sizeof(ethdev_name));
 
-	eth_dev = rte_eth_dev_allocate(ethdev_name);
-	if (eth_dev == NULL)
-		return -ENOMEM;
+	if (rte_eth_dev_data == NULL)
+		rte_eth_dev_data_alloc();
+
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		eth_dev = rte_eth_dev_allocate(ethdev_name);
+		if (eth_dev == NULL)
+			return -ENOMEM;
+	} else {
+		/*
+		 * if we failed to attach a device, it means that
+		 * device is skipped, due to some errors. Take
+		 * virtio-net device as example, it could be the
+		 * device is managed by virtio-net kernel driver.
+		 * For such case, we return a positive value, to
+		 * let EAL skip it as well.
+		 */
+		eth_dev = eth_dev_attach(ethdev_name);
+		if (eth_dev == NULL)
+			return 1;
+	}
 
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		eth_dev->data->dev_private = rte_zmalloc("ethdev private structure",
-- 
2.8.1

[dpdk-dev] [PATCH v2 2/6] net/virtio: fix wrong Rx/Tx method for secondary process

[Yuanhan Liu]

If the primary enables the vector Rx/Tx path, the current code would
let the secondary always choose the non vector Rx/Tx path. This results
to a Rx/Tx method mismatch between primary and secondary process. Werid
errors then may happen, something like:

    PMD: virtio_xmit_pkts() tx: virtqueue_enqueue error: -14

Fix it by choosing the correct Rx/Tx callbacks for the secondary process.
That is, use vector path if it's given.

Fixes: 8d8393fb1861 ("virtio: pick simple Rx/Tx")

Cc: stable@dpdk.org
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---

v2: fix a checkpatch warning: use {} consistently
---
 drivers/net/virtio/virtio_ethdev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 079fd6c..ef37ad1 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1304,7 +1304,12 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
-		rx_func_get(eth_dev);
+		if (hw->use_simple_rxtx) {
+			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
+			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
+		} else {
+			rx_func_get(eth_dev);
+		}
 		return 0;
 	}
 
-- 
2.8.1

[dpdk-dev] [PATCH v2 3/6] net/virtio: store PCI operators pointer locally

[Yuanhan Liu]

We used to store the vtpci_ops at virtio_hw structure. The struct,
however, is stored in shared memory. That means only one value is
allowed. For the multiple process model, however, the address of
vtpci_ops should be different among different processes.

Take virtio PMD as example, the vtpci_ops is set by the primary
process, based on its own process space. If we access that address
from the secondary process, that would be an illegal memory access,
A crash then might happen.

To make the multiple process model work, we need store the vtpci_ops
in local memory but not in a shared memory. This is what the patch
does: a local virtio_hw_internal array of size RTE_MAX_ETHPORTS is
allocated. This new structure is used to store all these kind of
info in a non-shared memory. Current, we have:

- vtpci_ops

- rte_pci_ioport

- virtio pci mapped memory, such as common_cfg.

The later two will be done in coming patches. Later patches would also
set them correctly for secondary process, so that the multiple process
model could work.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_ethdev.c      |  9 ++++++---
 drivers/net/virtio/virtio_pci.c         | 26 +++++++++++++-------------
 drivers/net/virtio/virtio_pci.h         | 17 ++++++++++++++++-
 drivers/net/virtio/virtio_user_ethdev.c |  3 ++-
 drivers/net/virtio/virtqueue.h          |  2 +-
 5 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index ef37ad1..5567aa2 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -152,6 +152,8 @@ static const struct rte_virtio_xstats_name_off rte_virtio_txq_stat_strings[] = {
 #define VIRTIO_NB_TXQ_XSTATS (sizeof(rte_virtio_txq_stat_strings) / \
 			    sizeof(rte_virtio_txq_stat_strings[0]))
 
+struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS];
+
 static int
 virtio_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
 		int *dlen, int pkt_num)
@@ -360,7 +362,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
 	 * Read the virtqueue size from the Queue Size field
 	 * Always power of 2 and if 0 virtqueue does not exist
 	 */
-	vq_size = hw->vtpci_ops->get_queue_num(hw, vtpci_queue_idx);
+	vq_size = VTPCI_OPS(hw)->get_queue_num(hw, vtpci_queue_idx);
 	PMD_INIT_LOG(DEBUG, "vq_size: %u", vq_size);
 	if (vq_size == 0) {
 		PMD_INIT_LOG(ERR, "virtqueue does not exist");
@@ -519,7 +521,7 @@ virtio_init_queue(struct rte_eth_dev *dev, uint16_t vtpci_queue_idx)
 		}
 	}
 
-	if (hw->vtpci_ops->setup_queue(hw, vq) < 0) {
+	if (VTPCI_OPS(hw)->setup_queue(hw, vq) < 0) {
 		PMD_INIT_LOG(ERR, "setup_queue failed");
 		return -EINVAL;
 	}
@@ -1114,7 +1116,7 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
 		req_features);
 
 	/* Read device(host) feature bits */
-	host_features = hw->vtpci_ops->get_features(hw);
+	host_features = VTPCI_OPS(hw)->get_features(hw);
 	PMD_INIT_LOG(DEBUG, "host_features before negotiate = %" PRIx64,
 		host_features);
 
@@ -1322,6 +1324,7 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 		return -ENOMEM;
 	}
 
+	hw->port_id = eth_dev->data->port_id;
 	pci_dev = eth_dev->pci_dev;
 
 	if (pci_dev) {
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 9b47165..b1f2e18 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -537,14 +537,14 @@ void
 vtpci_read_dev_config(struct virtio_hw *hw, size_t offset,
 		      void *dst, int length)
 {
-	hw->vtpci_ops->read_dev_cfg(hw, offset, dst, length);
+	VTPCI_OPS(hw)->read_dev_cfg(hw, offset, dst, length);
 }
 
 void
 vtpci_write_dev_config(struct virtio_hw *hw, size_t offset,
 		       const void *src, int length)
 {
-	hw->vtpci_ops->write_dev_cfg(hw, offset, src, length);
+	VTPCI_OPS(hw)->write_dev_cfg(hw, offset, src, length);
 }
 
 uint64_t
@@ -557,7 +557,7 @@ vtpci_negotiate_features(struct virtio_hw *hw, uint64_t host_features)
 	 * host all support.
 	 */
 	features = host_features & hw->guest_features;
-	hw->vtpci_ops->set_features(hw, features);
+	VTPCI_OPS(hw)->set_features(hw, features);
 
 	return features;
 }
@@ -565,9 +565,9 @@ vtpci_negotiate_features(struct virtio_hw *hw, uint64_t host_features)
 void
 vtpci_reset(struct virtio_hw *hw)
 {
-	hw->vtpci_ops->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
+	VTPCI_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
 	/* flush status write */
-	hw->vtpci_ops->get_status(hw);
+	VTPCI_OPS(hw)->get_status(hw);
 }
 
 void
@@ -580,21 +580,21 @@ void
 vtpci_set_status(struct virtio_hw *hw, uint8_t status)
 {
 	if (status != VIRTIO_CONFIG_STATUS_RESET)
-		status |= hw->vtpci_ops->get_status(hw);
+		status |= VTPCI_OPS(hw)->get_status(hw);
 
-	hw->vtpci_ops->set_status(hw, status);
+	VTPCI_OPS(hw)->set_status(hw, status);
 }
 
 uint8_t
 vtpci_get_status(struct virtio_hw *hw)
 {
-	return hw->vtpci_ops->get_status(hw);
+	return VTPCI_OPS(hw)->get_status(hw);
 }
 
 uint8_t
 vtpci_isr(struct virtio_hw *hw)
 {
-	return hw->vtpci_ops->get_isr(hw);
+	return VTPCI_OPS(hw)->get_isr(hw);
 }
 
 
@@ -602,7 +602,7 @@ vtpci_isr(struct virtio_hw *hw)
 uint16_t
 vtpci_irq_config(struct virtio_hw *hw, uint16_t vec)
 {
-	return hw->vtpci_ops->set_config_irq(hw, vec);
+	return VTPCI_OPS(hw)->set_config_irq(hw, vec);
 }
 
 static void *
@@ -736,8 +736,8 @@ vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw,
 	 */
 	if (virtio_read_caps(dev, hw) == 0) {
 		PMD_INIT_LOG(INFO, "modern virtio pci detected.");
-		hw->vtpci_ops = &modern_ops;
-		hw->modern    = 1;
+		virtio_hw_internal[hw->port_id].vtpci_ops = &modern_ops;
+		hw->modern = 1;
 		*dev_flags |= RTE_ETH_DEV_INTR_LSC;
 		return 0;
 	}
@@ -755,7 +755,7 @@ vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw,
 		return -1;
 	}
 
-	hw->vtpci_ops = &legacy_ops;
+	virtio_hw_internal[hw->port_id].vtpci_ops = &legacy_ops;
 	hw->use_msix = legacy_virtio_has_msix(&dev->addr);
 	hw->modern   = 0;
 
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index de271bf..268bb82 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -254,6 +254,7 @@ struct virtio_hw {
 	uint8_t	    use_msix;
 	uint8_t     modern;
 	uint8_t     use_simple_rxtx;
+	uint8_t     port_id;
 	uint8_t     mac_addr[ETHER_ADDR_LEN];
 	uint32_t    notify_off_multiplier;
 	uint8_t     *isr;
@@ -261,12 +262,26 @@ struct virtio_hw {
 	struct rte_pci_device *dev;
 	struct virtio_pci_common_cfg *common_cfg;
 	struct virtio_net_config *dev_cfg;
-	const struct virtio_pci_ops *vtpci_ops;
 	void	    *virtio_user_dev;
 
 	struct virtqueue **vqs;
 };
 
+
+/*
+ * While virtio_hw is stored in shared memory, this structure stores
+ * some infos that may vary in the multiple process model locally.
+ * For example, the vtpci_ops pointer.
+ */
+struct virtio_hw_internal {
+	const struct virtio_pci_ops *vtpci_ops;
+};
+
+#define VTPCI_OPS(hw)	(virtio_hw_internal[(hw)->port_id].vtpci_ops)
+
+extern struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS];
+
+
 /*
  * This structure is just a reference to read
  * net device specific config space; it just a chodu structure
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 406beea..7d2a9d9 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -301,7 +301,8 @@ virtio_user_eth_dev_alloc(const char *name)
 		return NULL;
 	}
 
-	hw->vtpci_ops = &virtio_user_ops;
+	hw->port_id = data->port_id;
+	virtio_hw_internal[hw->port_id].vtpci_ops = &virtio_user_ops;
 	hw->use_msix = 0;
 	hw->modern   = 0;
 	hw->use_simple_rxtx = 0;
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index f0bb089..b1070e0 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -330,7 +330,7 @@ virtqueue_notify(struct virtqueue *vq)
 	 * For virtio on IA, the notificaiton is through io port operation
 	 * which is a serialization instruction itself.
 	 */
-	vq->hw->vtpci_ops->notify_queue(vq->hw, vq);
+	VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
 }
 
 #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DUMP
-- 
2.8.1

[dpdk-dev] [PATCH v2 4/6] net/virtio: store IO port info locally

[Yuanhan Liu]

Like vtpci_ops, the rte_pci_ioport has to store in local memory. This
is basically for the rte_pci_device field is allocated from process
local memory, but not from shared memory.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_pci.c | 49 ++++++++++++++++++++++-------------------
 drivers/net/virtio/virtio_pci.h |  3 ++-
 2 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index b1f2e18..d1e9c05 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -92,17 +92,17 @@ legacy_read_dev_config(struct virtio_hw *hw, size_t offset,
 	while (length > 0) {
 		if (length >= 4) {
 			size = 4;
-			rte_eal_pci_ioport_read(&hw->io, dst, size,
+			rte_eal_pci_ioport_read(VTPCI_IO(hw), dst, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 			*(uint32_t *)dst = rte_be_to_cpu_32(*(uint32_t *)dst);
 		} else if (length >= 2) {
 			size = 2;
-			rte_eal_pci_ioport_read(&hw->io, dst, size,
+			rte_eal_pci_ioport_read(VTPCI_IO(hw), dst, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 			*(uint16_t *)dst = rte_be_to_cpu_16(*(uint16_t *)dst);
 		} else {
 			size = 1;
-			rte_eal_pci_ioport_read(&hw->io, dst, size,
+			rte_eal_pci_ioport_read(VTPCI_IO(hw), dst, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 		}
 
@@ -111,7 +111,7 @@ legacy_read_dev_config(struct virtio_hw *hw, size_t offset,
 		length -= size;
 	}
 #else
-	rte_eal_pci_ioport_read(&hw->io, dst, length,
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), dst, length,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 #endif
 }
@@ -131,16 +131,16 @@ legacy_write_dev_config(struct virtio_hw *hw, size_t offset,
 		if (length >= 4) {
 			size = 4;
 			tmp.u32 = rte_cpu_to_be_32(*(const uint32_t *)src);
-			rte_eal_pci_ioport_write(&hw->io, &tmp.u32, size,
+			rte_eal_pci_ioport_write(VTPCI_IO(hw), &tmp.u32, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 		} else if (length >= 2) {
 			size = 2;
 			tmp.u16 = rte_cpu_to_be_16(*(const uint16_t *)src);
-			rte_eal_pci_ioport_write(&hw->io, &tmp.u16, size,
+			rte_eal_pci_ioport_write(VTPCI_IO(hw), &tmp.u16, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 		} else {
 			size = 1;
-			rte_eal_pci_ioport_write(&hw->io, src, size,
+			rte_eal_pci_ioport_write(VTPCI_IO(hw), src, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 		}
 
@@ -149,7 +149,7 @@ legacy_write_dev_config(struct virtio_hw *hw, size_t offset,
 		length -= size;
 	}
 #else
-	rte_eal_pci_ioport_write(&hw->io, src, length,
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), src, length,
 				 VIRTIO_PCI_CONFIG(hw) + offset);
 #endif
 }
@@ -159,7 +159,7 @@ legacy_get_features(struct virtio_hw *hw)
 {
 	uint32_t dst;
 
-	rte_eal_pci_ioport_read(&hw->io, &dst, 4, VIRTIO_PCI_HOST_FEATURES);
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), &dst, 4, VIRTIO_PCI_HOST_FEATURES);
 	return dst;
 }
 
@@ -171,7 +171,7 @@ legacy_set_features(struct virtio_hw *hw, uint64_t features)
 			"only 32 bit features are allowed for legacy virtio!");
 		return;
 	}
-	rte_eal_pci_ioport_write(&hw->io, &features, 4,
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &features, 4,
 				 VIRTIO_PCI_GUEST_FEATURES);
 }
 
@@ -180,14 +180,14 @@ legacy_get_status(struct virtio_hw *hw)
 {
 	uint8_t dst;
 
-	rte_eal_pci_ioport_read(&hw->io, &dst, 1, VIRTIO_PCI_STATUS);
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), &dst, 1, VIRTIO_PCI_STATUS);
 	return dst;
 }
 
 static void
 legacy_set_status(struct virtio_hw *hw, uint8_t status)
 {
-	rte_eal_pci_ioport_write(&hw->io, &status, 1, VIRTIO_PCI_STATUS);
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &status, 1, VIRTIO_PCI_STATUS);
 }
 
 static void
@@ -201,7 +201,7 @@ legacy_get_isr(struct virtio_hw *hw)
 {
 	uint8_t dst;
 
-	rte_eal_pci_ioport_read(&hw->io, &dst, 1, VIRTIO_PCI_ISR);
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), &dst, 1, VIRTIO_PCI_ISR);
 	return dst;
 }
 
@@ -211,8 +211,10 @@ legacy_set_config_irq(struct virtio_hw *hw, uint16_t vec)
 {
 	uint16_t dst;
 
-	rte_eal_pci_ioport_write(&hw->io, &vec, 2, VIRTIO_MSI_CONFIG_VECTOR);
-	rte_eal_pci_ioport_read(&hw->io, &dst, 2, VIRTIO_MSI_CONFIG_VECTOR);
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &vec, 2,
+				 VIRTIO_MSI_CONFIG_VECTOR);
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), &dst, 2,
+				VIRTIO_MSI_CONFIG_VECTOR);
 	return dst;
 }
 
@@ -221,8 +223,9 @@ legacy_get_queue_num(struct virtio_hw *hw, uint16_t queue_id)
 {
 	uint16_t dst;
 
-	rte_eal_pci_ioport_write(&hw->io, &queue_id, 2, VIRTIO_PCI_QUEUE_SEL);
-	rte_eal_pci_ioport_read(&hw->io, &dst, 2, VIRTIO_PCI_QUEUE_NUM);
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &queue_id, 2,
+				 VIRTIO_PCI_QUEUE_SEL);
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), &dst, 2, VIRTIO_PCI_QUEUE_NUM);
 	return dst;
 }
 
@@ -234,10 +237,10 @@ legacy_setup_queue(struct virtio_hw *hw, struct virtqueue *vq)
 	if (!check_vq_phys_addr_ok(vq))
 		return -1;
 
-	rte_eal_pci_ioport_write(&hw->io, &vq->vq_queue_index, 2,
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &vq->vq_queue_index, 2,
 			 VIRTIO_PCI_QUEUE_SEL);
 	src = vq->vq_ring_mem >> VIRTIO_PCI_QUEUE_ADDR_SHIFT;
-	rte_eal_pci_ioport_write(&hw->io, &src, 4, VIRTIO_PCI_QUEUE_PFN);
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &src, 4, VIRTIO_PCI_QUEUE_PFN);
 
 	return 0;
 }
@@ -247,15 +250,15 @@ legacy_del_queue(struct virtio_hw *hw, struct virtqueue *vq)
 {
 	uint32_t src = 0;
 
-	rte_eal_pci_ioport_write(&hw->io, &vq->vq_queue_index, 2,
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &vq->vq_queue_index, 2,
 			 VIRTIO_PCI_QUEUE_SEL);
-	rte_eal_pci_ioport_write(&hw->io, &src, 4, VIRTIO_PCI_QUEUE_PFN);
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &src, 4, VIRTIO_PCI_QUEUE_PFN);
 }
 
 static void
 legacy_notify_queue(struct virtio_hw *hw, struct virtqueue *vq)
 {
-	rte_eal_pci_ioport_write(&hw->io, &vq->vq_queue_index, 2,
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &vq->vq_queue_index, 2,
 			 VIRTIO_PCI_QUEUE_NOTIFY);
 }
 
@@ -289,7 +292,7 @@ static int
 legacy_virtio_resource_init(struct rte_pci_device *pci_dev,
 			    struct virtio_hw *hw, uint32_t *dev_flags)
 {
-	if (rte_eal_pci_ioport_map(pci_dev, 0, &hw->io) < 0)
+	if (rte_eal_pci_ioport_map(pci_dev, 0, VTPCI_IO(hw)) < 0)
 		return -1;
 
 	if (pci_dev->intr_handle.type != RTE_INTR_HANDLE_UNKNOWN)
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 268bb82..6b9aecf 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -245,7 +245,6 @@ struct virtio_net_config;
 
 struct virtio_hw {
 	struct virtnet_ctl *cvq;
-	struct rte_pci_ioport io;
 	uint64_t    req_guest_features;
 	uint64_t    guest_features;
 	uint32_t    max_queue_pairs;
@@ -275,9 +274,11 @@ struct virtio_hw {
  */
 struct virtio_hw_internal {
 	const struct virtio_pci_ops *vtpci_ops;
+	struct rte_pci_ioport io;
 };
 
 #define VTPCI_OPS(hw)	(virtio_hw_internal[(hw)->port_id].vtpci_ops)
+#define VTPCI_IO(hw)	(&virtio_hw_internal[(hw)->port_id].io)
 
 extern struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS];
 
-- 
2.8.1

[dpdk-dev] [PATCH v2 5/6] net/virtio: fix multiple process support

[Yuanhan Liu]

The introduce of virtio 1.0 support brings yet another set of ops, badly,
it's not handled correctly, that it breaks the multiple process support.

The issue is the data/function pointer may vary from different processes,
and the old used to do one time set (for primary process only). That
said, the function pointer the secondary process saw is actually from the
primary process space. Accessing it could likely result to a crash.

Kudos to the last patches, we now be able to maintain those info that may
vary among different process locally, meaning every process could have its
own copy for each of them, with the correct value set. And this is what
this patch does:

- remap the PCI (IO port for legacy device and memory map for modern
  device)

- set vtpci_ops correctly

After that, multiple process would work like a charm. (At least, it
passed my fuzzy test)

Fixes: b8f04520ad71 ("virtio: use PCI ioport API")
Fixes: d5bbeefca826 ("virtio: introduce PCI implementation structure")
Fixes: 6ba1f63b5ab0 ("virtio: support specification 1.0")

Cc: stable@kernel.org
Cc: Juho Snellman <jsnell@iki.fi>
Cc: Yaron Illouz <yaroni@radcom.com>
Reported-by: Juho Snellman <jsnell@iki.fi>
Reported-by: Yaron Illouz <yaroni@radcom.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_ethdev.c      | 53 +++++++++++++++++++++++++++++++--
 drivers/net/virtio/virtio_pci.c         |  4 +--
 drivers/net/virtio/virtio_pci.h         |  4 +++
 drivers/net/virtio/virtio_user_ethdev.c |  2 +-
 4 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 5567aa2..19d4348 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1289,6 +1289,49 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 }
 
 /*
+ * Remap the PCI device again (IO port map for legacy device and
+ * memory map for modern device), so that the secondary process
+ * could have the PCI initiated correctly.
+ */
+static int
+virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
+{
+	if (hw->modern) {
+		/*
+		 * We don't have to re-parse the PCI config space, since
+		 * rte_eal_pci_map_device() makes sure the mapped address
+		 * in secondary process would equal to the one mapped in
+		 * the primary process: error will be returned if that
+		 * requirement is not met.
+		 *
+		 * That said, we could simply reuse all cap pointers
+		 * (such as dev_cfg, common_cfg, etc.) parsed from the
+		 * primary process, which is stored in shared memory.
+		 */
+		if (rte_eal_pci_map_device(pci_dev)) {
+			PMD_INIT_LOG(DEBUG, "failed to map pci device!");
+			return -1;
+		}
+	} else {
+		if (rte_eal_pci_ioport_map(pci_dev, 0, VTPCI_IO(hw)) < 0)
+			return -1;
+	}
+
+	return 0;
+}
+
+static void
+virtio_set_vtpci_ops(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
+{
+	if (pci_dev == NULL)
+		VTPCI_OPS(hw) = &virtio_user_ops;
+	else if (hw->modern)
+		VTPCI_OPS(hw) = &modern_ops;
+	else
+		VTPCI_OPS(hw) = &legacy_ops;
+}
+
+/*
  * This function is based on probe() function in virtio_pci.c
  * It returns 0 on success.
  */
@@ -1296,7 +1339,7 @@ int
 eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
-	struct rte_pci_device *pci_dev;
+	struct rte_pci_device *pci_dev = eth_dev->pci_dev;
 	uint32_t dev_flags = RTE_ETH_DEV_DETACHABLE;
 	int ret;
 
@@ -1306,6 +1349,13 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		if (pci_dev) {
+			ret = virtio_remap_pci(pci_dev, hw);
+			if (ret)
+				return ret;
+		}
+
+		virtio_set_vtpci_ops(pci_dev, hw);
 		if (hw->use_simple_rxtx) {
 			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
 			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
@@ -1325,7 +1375,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	}
 
 	hw->port_id = eth_dev->data->port_id;
-	pci_dev = eth_dev->pci_dev;
 
 	if (pci_dev) {
 		ret = vtpci_init(pci_dev, hw, &dev_flags);
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index d1e9c05..f5754e5 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -303,7 +303,7 @@ legacy_virtio_resource_init(struct rte_pci_device *pci_dev,
 	return 0;
 }
 
-static const struct virtio_pci_ops legacy_ops = {
+const struct virtio_pci_ops legacy_ops = {
 	.read_dev_cfg	= legacy_read_dev_config,
 	.write_dev_cfg	= legacy_write_dev_config,
 	.reset		= legacy_reset,
@@ -519,7 +519,7 @@ modern_notify_queue(struct virtio_hw *hw __rte_unused, struct virtqueue *vq)
 	io_write16(1, vq->notify_addr);
 }
 
-static const struct virtio_pci_ops modern_ops = {
+const struct virtio_pci_ops modern_ops = {
 	.read_dev_cfg	= modern_read_dev_config,
 	.write_dev_cfg	= modern_write_dev_config,
 	.reset		= modern_reset,
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 6b9aecf..511a1c8 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -333,4 +333,8 @@ uint8_t vtpci_isr(struct virtio_hw *);
 
 uint16_t vtpci_irq_config(struct virtio_hw *, uint16_t);
 
+extern const struct virtio_pci_ops legacy_ops;
+extern const struct virtio_pci_ops modern_ops;
+extern const struct virtio_pci_ops virtio_user_ops;
+
 #endif /* _VIRTIO_PCI_H_ */
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 7d2a9d9..3563952 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -212,7 +212,7 @@ virtio_user_notify_queue(struct virtio_hw *hw, struct virtqueue *vq)
 			    strerror(errno));
 }
 
-static const struct virtio_pci_ops virtio_user_ops = {
+const struct virtio_pci_ops virtio_user_ops = {
 	.read_dev_cfg	= virtio_user_read_dev_config,
 	.write_dev_cfg	= virtio_user_write_dev_config,
 	.reset		= virtio_user_reset,
-- 
2.8.1

[dpdk-dev] [PATCH v2 6/6] net/virtio: remove dead structure field

[Yuanhan Liu]

Actually, virtio_hw->dev is not used since the beginning when it's
introduced. Remove it.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_pci.c | 2 --
 drivers/net/virtio/virtio_pci.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index f5754e5..fbdb5b7 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -730,8 +730,6 @@ int
 vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw,
 	   uint32_t *dev_flags)
 {
-	hw->dev = dev;
-
 	/*
 	 * Try if we can succeed reading virtio pci caps, which exists
 	 * only on modern pci device. If failed, we fallback to legacy
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 511a1c8..4235bef 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -258,7 +258,6 @@ struct virtio_hw {
 	uint32_t    notify_off_multiplier;
 	uint8_t     *isr;
 	uint16_t    *notify_base;
-	struct rte_pci_device *dev;
 	struct virtio_pci_common_cfg *common_cfg;
 	struct virtio_net_config *dev_cfg;
 	void	    *virtio_user_dev;
-- 
2.8.1

Re: [dpdk-dev] [PATCH v2 5/6] net/virtio: fix multiple process support

[Yuanhan Liu]

On Wed, Dec 28, 2016 at 07:02:41PM +0800, Yuanhan Liu wrote:
...
> Cc: stable@kernel.org

I knew I would make a mistake like this some day :/

Not my first time typing wrong, but it's the first time sending it out.
Sorry for that. Fixed in this reply.

	--yliu

Re: [dpdk-dev] [PATCH v2 1/6] ethdev: fix port data mismatched in multiple process model

[Thomas Monjalon]

+Cc Sergio (maintainer of the secondary process thing)

2016-12-28 19:02, Yuanhan Liu:
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -201,9 +201,6 @@ rte_eth_dev_allocate(const char *name)
>  		return NULL;
>  	}
>  
> -	if (rte_eth_dev_data == NULL)
> -		rte_eth_dev_data_alloc();
> -

It is dangerous to move this to rte_eth_dev_pci_probe.
Please keep it here and duplicate it in eth_dev_attach.

[...]
> +/*
> + * Attach to a port already registered by the primary process, which
> + * makes sure that the same device would both have the same port id
> + * in the primary and secondary process.
> + */
> +static struct rte_eth_dev *
> +eth_dev_attach(const char *name)

Maybe that the word "secondary" could help to differentiate of
the function rte_eth_dev_attach().

> +{
> +	uint8_t i;
> +	struct rte_eth_dev *eth_dev;
> +
> +	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> +		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
> +			break;
> +	}
> +	if (i == RTE_MAX_ETHPORTS) {
> +		RTE_PMD_DEBUG_TRACE(
> +			"device %s is not driven by the primary process\n",
> +			name);
> +		return NULL;
> +	}
> +
> +	RTE_ASSERT(eth_dev->data->port_id == i);
> +
> +	eth_dev = &rte_eth_devices[i];
> +	eth_dev->data = &rte_eth_dev_data[i];
> +	eth_dev->attached = DEV_ATTACHED;
> +	nb_ports++;

I am a bit nervous when I see these lines duplicated from rte_eth_dev_allocate.
Not sure whether it deserves a common function or not.

[...]
> @@ -246,9 +275,26 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
> -	eth_dev = rte_eth_dev_allocate(ethdev_name);
> -	if (eth_dev == NULL)
> -		return -ENOMEM;
> +	if (rte_eth_dev_data == NULL)
> +		rte_eth_dev_data_alloc();
> +
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		eth_dev = rte_eth_dev_allocate(ethdev_name);
> +		if (eth_dev == NULL)
> +			return -ENOMEM;
> +	} else {
> +		/*
> +		 * if we failed to attach a device, it means that
> +		 * device is skipped, due to some errors. Take
> +		 * virtio-net device as example, it could be the
> +		 * device is managed by virtio-net kernel driver.
> +		 * For such case, we return a positive value, to
> +		 * let EAL skip it as well.
> +		 */

This comment (a bit too long) should be placed between "if" and "return".

> +		eth_dev = eth_dev_attach(ethdev_name);
> +		if (eth_dev == NULL)
> +			return 1;
> +	}

Re: [dpdk-dev] [PATCH v2 1/6] ethdev: fix port data mismatched in multiple process model

[Yuanhan Liu]

On Wed, Jan 04, 2017 at 06:34:40PM +0100, Thomas Monjalon wrote:
> +Cc Sergio (maintainer of the secondary process thing)
> 
> 2016-12-28 19:02, Yuanhan Liu:
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -201,9 +201,6 @@ rte_eth_dev_allocate(const char *name)
> >  		return NULL;
> >  	}
> >  
> > -	if (rte_eth_dev_data == NULL)
> > -		rte_eth_dev_data_alloc();
> > -
> 
> It is dangerous to move this to rte_eth_dev_pci_probe.
> Please keep it here and duplicate it in eth_dev_attach.

Oh, right, I missed the fact that it could be invoked from other places.

> [...]
> > +/*
> > + * Attach to a port already registered by the primary process, which
> > + * makes sure that the same device would both have the same port id
> > + * in the primary and secondary process.
> > + */
> > +static struct rte_eth_dev *
> > +eth_dev_attach(const char *name)
> 
> Maybe that the word "secondary" could help to differentiate of
> the function rte_eth_dev_attach().

Yes, it's better. How about "_attach_secondary", or "_attach_to_primary"?

> > +{
> > +	uint8_t i;
> > +	struct rte_eth_dev *eth_dev;
> > +
> > +	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> > +		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
> > +			break;
> > +	}
> > +	if (i == RTE_MAX_ETHPORTS) {
> > +		RTE_PMD_DEBUG_TRACE(
> > +			"device %s is not driven by the primary process\n",
> > +			name);
> > +		return NULL;
> > +	}
> > +
> > +	RTE_ASSERT(eth_dev->data->port_id == i);
> > +
> > +	eth_dev = &rte_eth_devices[i];
> > +	eth_dev->data = &rte_eth_dev_data[i];
> > +	eth_dev->attached = DEV_ATTACHED;
> > +	nb_ports++;
> 
> I am a bit nervous when I see these lines duplicated from rte_eth_dev_allocate.
> Not sure whether it deserves a common function or not.

I don't think so, they do share some common assignments, but the assignments
are actually not the same. The primary one has few more: notably, they are:

- eth_dev->data
- eth_dev->data->name
- eth_dev->data->port_id

> 
> [...]
> > @@ -246,9 +275,26 @@ rte_eth_dev_pci_probe(struct rte_pci_driver *pci_drv,
> > -	eth_dev = rte_eth_dev_allocate(ethdev_name);
> > -	if (eth_dev == NULL)
> > -		return -ENOMEM;
> > +	if (rte_eth_dev_data == NULL)
> > +		rte_eth_dev_data_alloc();
> > +
> > +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > +		eth_dev = rte_eth_dev_allocate(ethdev_name);
> > +		if (eth_dev == NULL)
> > +			return -ENOMEM;
> > +	} else {
> > +		/*
> > +		 * if we failed to attach a device, it means that
> > +		 * device is skipped, due to some errors. Take
> > +		 * virtio-net device as example, it could be the
> > +		 * device is managed by virtio-net kernel driver.
> > +		 * For such case, we return a positive value, to
> > +		 * let EAL skip it as well.
> > +		 */
> 
> This comment (a bit too long) should be placed between "if" and "return".

Okay.

	--yliu

[dpdk-dev] [PATCH v3 0/6] net/virtio: fix several multiple process issues

[Yuanhan Liu]

v3: - fixed several comments from Thomas regarding to eth_dev
    - updated the release note and nic features matrix

v2: - fixed virtio 1.0 multiple process support
    - fixed the case when few virtio net devices are managed by DPDK
      while few others are handled by Linux kernel.

This patch series fixes few crash issues regarding to multiple process
model. In my limited fuzzy test, now it works for both virtio 0.95 and
1.0, as well as for the case some virtio-net devices are managed by
kernel device while some others are managed by DPDK.

---
Maintaining the multiple process support is not an easy task -- you
have to be very mindful while coding -- what kind of stuff should
and should not be in shared memory. Otherwise, it's very likely the
multiple process model will be broken.

A typical example is the ops pointer, a pointer to a set of function
pointers.  Normally, it's a pointer stored in a read-only data section
of the application:

    static const struct virtio_pci_ops legacy_ops = {
            ...,
    }

The pointer, of course, may vary in different process space. If,
however, we store the pointer into shared memory, we could only
have one value for it.  Setting it from process A and accessing
it from process B would likely lead to an illegal memory access.
As a result, crash happens.

The fix is to keep those addresses locally, in a new struct,
virtio_hw_internal. By that, each process maintains it's own
version of the pointer (from its own process space). Thus,
everything would work as expected.


---
Yuanhan Liu (6):
  ethdev: fix port data mismatched in multiple process model
  net/virtio: fix wrong Rx/Tx method for secondary process
  net/virtio: store PCI operators pointer locally
  net/virtio: store IO port info locally
  net/virtio: fix multiple process support
  net/virtio: remove dead structure field

 doc/guides/nics/features/virtio.ini     |  1 +
 doc/guides/rel_notes/release_17_02.rst  |  5 ++
 drivers/net/virtio/virtio_ethdev.c      | 69 +++++++++++++++++++++++++---
 drivers/net/virtio/virtio_pci.c         | 81 +++++++++++++++++----------------
 drivers/net/virtio/virtio_pci.h         | 25 ++++++++--
 drivers/net/virtio/virtio_user_ethdev.c |  5 +-
 drivers/net/virtio/virtqueue.h          |  2 +-
 lib/librte_ether/rte_ethdev.c           | 77 +++++++++++++++++++++++++++----
 8 files changed, 204 insertions(+), 61 deletions(-)

-- 
1.9.0

[dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model

[Yuanhan Liu]

Assume we have two virtio ports, 00:03.0 and 00:04.0. The first one is
managed by the kernel driver, while the later one is managed by DPDK.

Now we start the primary process. 00:03.0 will be skipped by DPDK virtio
PMD driver (since it's being used by the kernel). 00:04.0 would be
successfully initiated by DPDK virtio PMD (if nothing abnormal happens).
After that, we would get a port id 0, and all the related info needed
by virtio (virtio_hw) is stored at rte_eth_dev_data[0].

Then we start the secondary process. As usual, 00:03.0 will be firstly
probed. It firstly tries to get a local eth_dev structure for it (by
rte_eth_dev_allocate):

        port_id = rte_eth_dev_find_free_port();
        ...

        eth_dev = &rte_eth_devices[port_id];
        eth_dev->data = &rte_eth_dev_data[port_id];
        ...

        return eth_dev;

Since it's a first PCI device, port_id will be 0. eth_dev->data would
then point to rte_eth_dev_data[0]. And here things start going wrong,
as rte_eth_dev_data[0] actually stores the virtio_hw for 00:04.0.

That said, in the secondary process, DPDK will continue to drive PCI
device 00.03.0 (despite the fact it's been managed by kernel), with
the info from PCI device 00:04.0. Which is wrong.

The fix is to attach the port already registered by the primary process:
iterate the rte_eth_dev_data[], and get the port id who's PCI ID matches
the current PCI device.

This would let us maintain same port ID for the same PCI device, keeping
the chance of referencing to wrong data minimal.

Fixes: af75078fece3 ("first public release")

Cc: stable@dpdk.org
Cc: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: Bruce Richardson <bruce.richardson@intel.com>
Cc: Ferruh Yigit <ferruh.yigit@intel.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---

v3: - do not move rte_eth_dev_data_alloc to pci_probe
    - rename eth_dev_attach to eth_dev_attach_secondary
    - introduce eth_dev_init() for common eth_dev struct initiation
    - move comment block inside the "if" block
---
 lib/librte_ether/rte_ethdev.c | 77 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 68 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fde8112..c3e65f1 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -189,6 +189,21 @@ struct rte_eth_dev *
 	return RTE_MAX_ETHPORTS;
 }
 
+static void
+eth_dev_init(struct rte_eth_dev *eth_dev, uint8_t port_id, const char *name)
+{
+	eth_dev->data = &rte_eth_dev_data[port_id];
+	eth_dev->attached = DEV_ATTACHED;
+	eth_dev_last_created_port = port_id;
+	nb_ports++;
+
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		snprintf(eth_dev->data->name, sizeof(eth_dev->data->name),
+			 "%s", name);
+		eth_dev->data->port_id = port_id;
+	}
+}
+
 struct rte_eth_dev *
 rte_eth_dev_allocate(const char *name)
 {
@@ -211,12 +226,41 @@ struct rte_eth_dev *
 	}
 
 	eth_dev = &rte_eth_devices[port_id];
-	eth_dev->data = &rte_eth_dev_data[port_id];
-	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
-	eth_dev->data->port_id = port_id;
-	eth_dev->attached = DEV_ATTACHED;
-	eth_dev_last_created_port = port_id;
-	nb_ports++;
+	eth_dev_init(eth_dev, port_id, name);
+
+	return eth_dev;
+}
+
+/*
+ * Attach to a port already registered by the primary process, which
+ * makes sure that the same device would have the same port id both
+ * in the primary and secondary process.
+ */
+static struct rte_eth_dev *
+eth_dev_attach_secondary(const char *name)
+{
+	uint8_t i;
+	struct rte_eth_dev *eth_dev;
+
+	if (rte_eth_dev_data == NULL)
+		rte_eth_dev_data_alloc();
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
+			break;
+	}
+	if (i == RTE_MAX_ETHPORTS) {
+		RTE_PMD_DEBUG_TRACE(
+			"device %s is not driven by the primary process\n",
+			name);
+		return NULL;
+	}
+
+	RTE_ASSERT(eth_dev->data->port_id == i);
+
+	eth_dev = &rte_eth_devices[i];
+	eth_dev_init(eth_dev, i, NULL);
+
 	return eth_dev;
 }
 
@@ -246,9 +290,24 @@ struct rte_eth_dev *
 	rte_eal_pci_device_name(&pci_dev->addr, ethdev_name,
 			sizeof(ethdev_name));
 
-	eth_dev = rte_eth_dev_allocate(ethdev_name);
-	if (eth_dev == NULL)
-		return -ENOMEM;
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		eth_dev = rte_eth_dev_allocate(ethdev_name);
+		if (eth_dev == NULL)
+			return -ENOMEM;
+	} else {
+		eth_dev = eth_dev_attach_secondary(ethdev_name);
+		if (eth_dev == NULL) {
+			/*
+			 * if we failed to attach a device, it means
+			 * the device is skipped, due to some errors.
+			 * Take virtio-net device as example, it could
+			 * due to the device is managed by virtio-net
+			 * kernel driver.  For such case, we return a
+			 * positive value, to let EAL skip it as well.
+			 */
+			return 1;
+		}
+	}
 
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		eth_dev->data->dev_private = rte_zmalloc("ethdev private structure",
-- 
1.9.0

[dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process

[Yuanhan Liu]

If the primary enables the vector Rx/Tx path, the current code would
let the secondary always choose the non vector Rx/Tx path. This results
to a Rx/Tx method mismatch between primary and secondary process. Werid
errors then may happen, something like:

    PMD: virtio_xmit_pkts() tx: virtqueue_enqueue error: -14

Fix it by choosing the correct Rx/Tx callbacks for the secondary process.
That is, use vector path if it's given.

Fixes: 8d8393fb1861 ("virtio: pick simple Rx/Tx")

Cc: stable@dpdk.org
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---

v2: fix a checkpatch warning: use {} consistently
---
 drivers/net/virtio/virtio_ethdev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 079fd6c..ef37ad1 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1304,7 +1304,12 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
-		rx_func_get(eth_dev);
+		if (hw->use_simple_rxtx) {
+			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
+			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
+		} else {
+			rx_func_get(eth_dev);
+		}
 		return 0;
 	}
 
-- 
1.9.0

[dpdk-dev] [PATCH v3 3/6] net/virtio: store PCI operators pointer locally

[Yuanhan Liu]

We used to store the vtpci_ops at virtio_hw structure. The struct,
however, is stored in shared memory. That means only one value is
allowed. For the multiple process model, however, the address of
vtpci_ops should be different among different processes.

Take virtio PMD as example, the vtpci_ops is set by the primary
process, based on its own process space. If we access that address
from the secondary process, that would be an illegal memory access,
A crash then might happen.

To make the multiple process model work, we need store the vtpci_ops
in local memory but not in a shared memory. This is what the patch
does: a local virtio_hw_internal array of size RTE_MAX_ETHPORTS is
allocated. This new structure is used to store all these kind of
info in a non-shared memory. Current, we have:

- vtpci_ops

- rte_pci_ioport

- virtio pci mapped memory, such as common_cfg.

The later two will be done in coming patches. Later patches would also
set them correctly for secondary process, so that the multiple process
model could work.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_ethdev.c      |  9 ++++++---
 drivers/net/virtio/virtio_pci.c         | 26 +++++++++++++-------------
 drivers/net/virtio/virtio_pci.h         | 17 ++++++++++++++++-
 drivers/net/virtio/virtio_user_ethdev.c |  3 ++-
 drivers/net/virtio/virtqueue.h          |  2 +-
 5 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index ef37ad1..5567aa2 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -152,6 +152,8 @@ struct rte_virtio_xstats_name_off {
 #define VIRTIO_NB_TXQ_XSTATS (sizeof(rte_virtio_txq_stat_strings) / \
 			    sizeof(rte_virtio_txq_stat_strings[0]))
 
+struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS];
+
 static int
 virtio_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
 		int *dlen, int pkt_num)
@@ -360,7 +362,7 @@ struct rte_virtio_xstats_name_off {
 	 * Read the virtqueue size from the Queue Size field
 	 * Always power of 2 and if 0 virtqueue does not exist
 	 */
-	vq_size = hw->vtpci_ops->get_queue_num(hw, vtpci_queue_idx);
+	vq_size = VTPCI_OPS(hw)->get_queue_num(hw, vtpci_queue_idx);
 	PMD_INIT_LOG(DEBUG, "vq_size: %u", vq_size);
 	if (vq_size == 0) {
 		PMD_INIT_LOG(ERR, "virtqueue does not exist");
@@ -519,7 +521,7 @@ struct rte_virtio_xstats_name_off {
 		}
 	}
 
-	if (hw->vtpci_ops->setup_queue(hw, vq) < 0) {
+	if (VTPCI_OPS(hw)->setup_queue(hw, vq) < 0) {
 		PMD_INIT_LOG(ERR, "setup_queue failed");
 		return -EINVAL;
 	}
@@ -1114,7 +1116,7 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 		req_features);
 
 	/* Read device(host) feature bits */
-	host_features = hw->vtpci_ops->get_features(hw);
+	host_features = VTPCI_OPS(hw)->get_features(hw);
 	PMD_INIT_LOG(DEBUG, "host_features before negotiate = %" PRIx64,
 		host_features);
 
@@ -1322,6 +1324,7 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 		return -ENOMEM;
 	}
 
+	hw->port_id = eth_dev->data->port_id;
 	pci_dev = eth_dev->pci_dev;
 
 	if (pci_dev) {
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index 9b47165..b1f2e18 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -537,14 +537,14 @@
 vtpci_read_dev_config(struct virtio_hw *hw, size_t offset,
 		      void *dst, int length)
 {
-	hw->vtpci_ops->read_dev_cfg(hw, offset, dst, length);
+	VTPCI_OPS(hw)->read_dev_cfg(hw, offset, dst, length);
 }
 
 void
 vtpci_write_dev_config(struct virtio_hw *hw, size_t offset,
 		       const void *src, int length)
 {
-	hw->vtpci_ops->write_dev_cfg(hw, offset, src, length);
+	VTPCI_OPS(hw)->write_dev_cfg(hw, offset, src, length);
 }
 
 uint64_t
@@ -557,7 +557,7 @@
 	 * host all support.
 	 */
 	features = host_features & hw->guest_features;
-	hw->vtpci_ops->set_features(hw, features);
+	VTPCI_OPS(hw)->set_features(hw, features);
 
 	return features;
 }
@@ -565,9 +565,9 @@
 void
 vtpci_reset(struct virtio_hw *hw)
 {
-	hw->vtpci_ops->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
+	VTPCI_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
 	/* flush status write */
-	hw->vtpci_ops->get_status(hw);
+	VTPCI_OPS(hw)->get_status(hw);
 }
 
 void
@@ -580,21 +580,21 @@
 vtpci_set_status(struct virtio_hw *hw, uint8_t status)
 {
 	if (status != VIRTIO_CONFIG_STATUS_RESET)
-		status |= hw->vtpci_ops->get_status(hw);
+		status |= VTPCI_OPS(hw)->get_status(hw);
 
-	hw->vtpci_ops->set_status(hw, status);
+	VTPCI_OPS(hw)->set_status(hw, status);
 }
 
 uint8_t
 vtpci_get_status(struct virtio_hw *hw)
 {
-	return hw->vtpci_ops->get_status(hw);
+	return VTPCI_OPS(hw)->get_status(hw);
 }
 
 uint8_t
 vtpci_isr(struct virtio_hw *hw)
 {
-	return hw->vtpci_ops->get_isr(hw);
+	return VTPCI_OPS(hw)->get_isr(hw);
 }
 
 
@@ -602,7 +602,7 @@
 uint16_t
 vtpci_irq_config(struct virtio_hw *hw, uint16_t vec)
 {
-	return hw->vtpci_ops->set_config_irq(hw, vec);
+	return VTPCI_OPS(hw)->set_config_irq(hw, vec);
 }
 
 static void *
@@ -736,8 +736,8 @@
 	 */
 	if (virtio_read_caps(dev, hw) == 0) {
 		PMD_INIT_LOG(INFO, "modern virtio pci detected.");
-		hw->vtpci_ops = &modern_ops;
-		hw->modern    = 1;
+		virtio_hw_internal[hw->port_id].vtpci_ops = &modern_ops;
+		hw->modern = 1;
 		*dev_flags |= RTE_ETH_DEV_INTR_LSC;
 		return 0;
 	}
@@ -755,7 +755,7 @@
 		return -1;
 	}
 
-	hw->vtpci_ops = &legacy_ops;
+	virtio_hw_internal[hw->port_id].vtpci_ops = &legacy_ops;
 	hw->use_msix = legacy_virtio_has_msix(&dev->addr);
 	hw->modern   = 0;
 
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index de271bf..268bb82 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -254,6 +254,7 @@ struct virtio_hw {
 	uint8_t	    use_msix;
 	uint8_t     modern;
 	uint8_t     use_simple_rxtx;
+	uint8_t     port_id;
 	uint8_t     mac_addr[ETHER_ADDR_LEN];
 	uint32_t    notify_off_multiplier;
 	uint8_t     *isr;
@@ -261,12 +262,26 @@ struct virtio_hw {
 	struct rte_pci_device *dev;
 	struct virtio_pci_common_cfg *common_cfg;
 	struct virtio_net_config *dev_cfg;
-	const struct virtio_pci_ops *vtpci_ops;
 	void	    *virtio_user_dev;
 
 	struct virtqueue **vqs;
 };
 
+
+/*
+ * While virtio_hw is stored in shared memory, this structure stores
+ * some infos that may vary in the multiple process model locally.
+ * For example, the vtpci_ops pointer.
+ */
+struct virtio_hw_internal {
+	const struct virtio_pci_ops *vtpci_ops;
+};
+
+#define VTPCI_OPS(hw)	(virtio_hw_internal[(hw)->port_id].vtpci_ops)
+
+extern struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS];
+
+
 /*
  * This structure is just a reference to read
  * net device specific config space; it just a chodu structure
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 406beea..7d2a9d9 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -301,7 +301,8 @@
 		return NULL;
 	}
 
-	hw->vtpci_ops = &virtio_user_ops;
+	hw->port_id = data->port_id;
+	virtio_hw_internal[hw->port_id].vtpci_ops = &virtio_user_ops;
 	hw->use_msix = 0;
 	hw->modern   = 0;
 	hw->use_simple_rxtx = 0;
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index f0bb089..b1070e0 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -330,7 +330,7 @@ struct virtio_tx_region {
 	 * For virtio on IA, the notificaiton is through io port operation
 	 * which is a serialization instruction itself.
 	 */
-	vq->hw->vtpci_ops->notify_queue(vq->hw, vq);
+	VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
 }
 
 #ifdef RTE_LIBRTE_VIRTIO_DEBUG_DUMP
-- 
1.9.0

[dpdk-dev] [PATCH v3 4/6] net/virtio: store IO port info locally

[Yuanhan Liu]

Like vtpci_ops, the rte_pci_ioport has to store in local memory. This
is basically for the rte_pci_device field is allocated from process
local memory, but not from shared memory.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_pci.c | 49 ++++++++++++++++++++++-------------------
 drivers/net/virtio/virtio_pci.h |  3 ++-
 2 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index b1f2e18..d1e9c05 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -92,17 +92,17 @@
 	while (length > 0) {
 		if (length >= 4) {
 			size = 4;
-			rte_eal_pci_ioport_read(&hw->io, dst, size,
+			rte_eal_pci_ioport_read(VTPCI_IO(hw), dst, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 			*(uint32_t *)dst = rte_be_to_cpu_32(*(uint32_t *)dst);
 		} else if (length >= 2) {
 			size = 2;
-			rte_eal_pci_ioport_read(&hw->io, dst, size,
+			rte_eal_pci_ioport_read(VTPCI_IO(hw), dst, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 			*(uint16_t *)dst = rte_be_to_cpu_16(*(uint16_t *)dst);
 		} else {
 			size = 1;
-			rte_eal_pci_ioport_read(&hw->io, dst, size,
+			rte_eal_pci_ioport_read(VTPCI_IO(hw), dst, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 		}
 
@@ -111,7 +111,7 @@
 		length -= size;
 	}
 #else
-	rte_eal_pci_ioport_read(&hw->io, dst, length,
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), dst, length,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 #endif
 }
@@ -131,16 +131,16 @@
 		if (length >= 4) {
 			size = 4;
 			tmp.u32 = rte_cpu_to_be_32(*(const uint32_t *)src);
-			rte_eal_pci_ioport_write(&hw->io, &tmp.u32, size,
+			rte_eal_pci_ioport_write(VTPCI_IO(hw), &tmp.u32, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 		} else if (length >= 2) {
 			size = 2;
 			tmp.u16 = rte_cpu_to_be_16(*(const uint16_t *)src);
-			rte_eal_pci_ioport_write(&hw->io, &tmp.u16, size,
+			rte_eal_pci_ioport_write(VTPCI_IO(hw), &tmp.u16, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 		} else {
 			size = 1;
-			rte_eal_pci_ioport_write(&hw->io, src, size,
+			rte_eal_pci_ioport_write(VTPCI_IO(hw), src, size,
 				VIRTIO_PCI_CONFIG(hw) + offset);
 		}
 
@@ -149,7 +149,7 @@
 		length -= size;
 	}
 #else
-	rte_eal_pci_ioport_write(&hw->io, src, length,
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), src, length,
 				 VIRTIO_PCI_CONFIG(hw) + offset);
 #endif
 }
@@ -159,7 +159,7 @@
 {
 	uint32_t dst;
 
-	rte_eal_pci_ioport_read(&hw->io, &dst, 4, VIRTIO_PCI_HOST_FEATURES);
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), &dst, 4, VIRTIO_PCI_HOST_FEATURES);
 	return dst;
 }
 
@@ -171,7 +171,7 @@
 			"only 32 bit features are allowed for legacy virtio!");
 		return;
 	}
-	rte_eal_pci_ioport_write(&hw->io, &features, 4,
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &features, 4,
 				 VIRTIO_PCI_GUEST_FEATURES);
 }
 
@@ -180,14 +180,14 @@
 {
 	uint8_t dst;
 
-	rte_eal_pci_ioport_read(&hw->io, &dst, 1, VIRTIO_PCI_STATUS);
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), &dst, 1, VIRTIO_PCI_STATUS);
 	return dst;
 }
 
 static void
 legacy_set_status(struct virtio_hw *hw, uint8_t status)
 {
-	rte_eal_pci_ioport_write(&hw->io, &status, 1, VIRTIO_PCI_STATUS);
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &status, 1, VIRTIO_PCI_STATUS);
 }
 
 static void
@@ -201,7 +201,7 @@
 {
 	uint8_t dst;
 
-	rte_eal_pci_ioport_read(&hw->io, &dst, 1, VIRTIO_PCI_ISR);
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), &dst, 1, VIRTIO_PCI_ISR);
 	return dst;
 }
 
@@ -211,8 +211,10 @@
 {
 	uint16_t dst;
 
-	rte_eal_pci_ioport_write(&hw->io, &vec, 2, VIRTIO_MSI_CONFIG_VECTOR);
-	rte_eal_pci_ioport_read(&hw->io, &dst, 2, VIRTIO_MSI_CONFIG_VECTOR);
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &vec, 2,
+				 VIRTIO_MSI_CONFIG_VECTOR);
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), &dst, 2,
+				VIRTIO_MSI_CONFIG_VECTOR);
 	return dst;
 }
 
@@ -221,8 +223,9 @@
 {
 	uint16_t dst;
 
-	rte_eal_pci_ioport_write(&hw->io, &queue_id, 2, VIRTIO_PCI_QUEUE_SEL);
-	rte_eal_pci_ioport_read(&hw->io, &dst, 2, VIRTIO_PCI_QUEUE_NUM);
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &queue_id, 2,
+				 VIRTIO_PCI_QUEUE_SEL);
+	rte_eal_pci_ioport_read(VTPCI_IO(hw), &dst, 2, VIRTIO_PCI_QUEUE_NUM);
 	return dst;
 }
 
@@ -234,10 +237,10 @@
 	if (!check_vq_phys_addr_ok(vq))
 		return -1;
 
-	rte_eal_pci_ioport_write(&hw->io, &vq->vq_queue_index, 2,
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &vq->vq_queue_index, 2,
 			 VIRTIO_PCI_QUEUE_SEL);
 	src = vq->vq_ring_mem >> VIRTIO_PCI_QUEUE_ADDR_SHIFT;
-	rte_eal_pci_ioport_write(&hw->io, &src, 4, VIRTIO_PCI_QUEUE_PFN);
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &src, 4, VIRTIO_PCI_QUEUE_PFN);
 
 	return 0;
 }
@@ -247,15 +250,15 @@
 {
 	uint32_t src = 0;
 
-	rte_eal_pci_ioport_write(&hw->io, &vq->vq_queue_index, 2,
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &vq->vq_queue_index, 2,
 			 VIRTIO_PCI_QUEUE_SEL);
-	rte_eal_pci_ioport_write(&hw->io, &src, 4, VIRTIO_PCI_QUEUE_PFN);
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &src, 4, VIRTIO_PCI_QUEUE_PFN);
 }
 
 static void
 legacy_notify_queue(struct virtio_hw *hw, struct virtqueue *vq)
 {
-	rte_eal_pci_ioport_write(&hw->io, &vq->vq_queue_index, 2,
+	rte_eal_pci_ioport_write(VTPCI_IO(hw), &vq->vq_queue_index, 2,
 			 VIRTIO_PCI_QUEUE_NOTIFY);
 }
 
@@ -289,7 +292,7 @@
 legacy_virtio_resource_init(struct rte_pci_device *pci_dev,
 			    struct virtio_hw *hw, uint32_t *dev_flags)
 {
-	if (rte_eal_pci_ioport_map(pci_dev, 0, &hw->io) < 0)
+	if (rte_eal_pci_ioport_map(pci_dev, 0, VTPCI_IO(hw)) < 0)
 		return -1;
 
 	if (pci_dev->intr_handle.type != RTE_INTR_HANDLE_UNKNOWN)
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 268bb82..6b9aecf 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -245,7 +245,6 @@ struct virtio_pci_ops {
 
 struct virtio_hw {
 	struct virtnet_ctl *cvq;
-	struct rte_pci_ioport io;
 	uint64_t    req_guest_features;
 	uint64_t    guest_features;
 	uint32_t    max_queue_pairs;
@@ -275,9 +274,11 @@ struct virtio_hw {
  */
 struct virtio_hw_internal {
 	const struct virtio_pci_ops *vtpci_ops;
+	struct rte_pci_ioport io;
 };
 
 #define VTPCI_OPS(hw)	(virtio_hw_internal[(hw)->port_id].vtpci_ops)
+#define VTPCI_IO(hw)	(&virtio_hw_internal[(hw)->port_id].io)
 
 extern struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS];
 
-- 
1.9.0

[dpdk-dev] [PATCH v3 5/6] net/virtio: fix multiple process support

[Yuanhan Liu]

The introduce of virtio 1.0 support brings yet another set of ops, badly,
it's not handled correctly, that it breaks the multiple process support.

The issue is the data/function pointer may vary from different processes,
and the old used to do one time set (for primary process only). That
said, the function pointer the secondary process saw is actually from the
primary process space. Accessing it could likely result to a crash.

Kudos to the last patches, we now be able to maintain those info that may
vary among different process locally, meaning every process could have its
own copy for each of them, with the correct value set. And this is what
this patch does:

- remap the PCI (IO port for legacy device and memory map for modern
  device)

- set vtpci_ops correctly

After that, multiple process would work like a charm. (At least, it
passed my fuzzy test)

Fixes: b8f04520ad71 ("virtio: use PCI ioport API")
Fixes: d5bbeefca826 ("virtio: introduce PCI implementation structure")
Fixes: 6ba1f63b5ab0 ("virtio: support specification 1.0")

Cc: stable@dpdk.org
Cc: Juho Snellman <jsnell@iki.fi>
Cc: Yaron Illouz <yaroni@radcom.com>
Reported-by: Juho Snellman <jsnell@iki.fi>
Reported-by: Yaron Illouz <yaroni@radcom.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---

v3: - update release note and nic matrix

v2: - fixed PCI remap, so that virtio 1.0 also works
---
 doc/guides/nics/features/virtio.ini     |  1 +
 doc/guides/rel_notes/release_17_02.rst  |  5 ++++
 drivers/net/virtio/virtio_ethdev.c      | 53 +++++++++++++++++++++++++++++++--
 drivers/net/virtio/virtio_pci.c         |  4 +--
 drivers/net/virtio/virtio_pci.h         |  4 +++
 drivers/net/virtio/virtio_user_ethdev.c |  2 +-
 6 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/doc/guides/nics/features/virtio.ini b/doc/guides/nics/features/virtio.ini
index 41830c1..f5de291 100644
--- a/doc/guides/nics/features/virtio.ini
+++ b/doc/guides/nics/features/virtio.ini
@@ -22,3 +22,4 @@ ARMv8                = Y
 x86-32               = Y
 x86-64               = Y
 Usage doc            = Y
+Multiprocess aware   = Y
diff --git a/doc/guides/rel_notes/release_17_02.rst b/doc/guides/rel_notes/release_17_02.rst
index 3b65038..a4260de 100644
--- a/doc/guides/rel_notes/release_17_02.rst
+++ b/doc/guides/rel_notes/release_17_02.rst
@@ -54,6 +54,11 @@ Resolved Issues
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+   * **fixed virtio multiple process support.**
+
+     Fixed few regressions introduced in recent releases that break the virtio
+     multiple process support.
+
 
 EAL
 ~~~
diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 5567aa2..19d4348 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1289,6 +1289,49 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 }
 
 /*
+ * Remap the PCI device again (IO port map for legacy device and
+ * memory map for modern device), so that the secondary process
+ * could have the PCI initiated correctly.
+ */
+static int
+virtio_remap_pci(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
+{
+	if (hw->modern) {
+		/*
+		 * We don't have to re-parse the PCI config space, since
+		 * rte_eal_pci_map_device() makes sure the mapped address
+		 * in secondary process would equal to the one mapped in
+		 * the primary process: error will be returned if that
+		 * requirement is not met.
+		 *
+		 * That said, we could simply reuse all cap pointers
+		 * (such as dev_cfg, common_cfg, etc.) parsed from the
+		 * primary process, which is stored in shared memory.
+		 */
+		if (rte_eal_pci_map_device(pci_dev)) {
+			PMD_INIT_LOG(DEBUG, "failed to map pci device!");
+			return -1;
+		}
+	} else {
+		if (rte_eal_pci_ioport_map(pci_dev, 0, VTPCI_IO(hw)) < 0)
+			return -1;
+	}
+
+	return 0;
+}
+
+static void
+virtio_set_vtpci_ops(struct rte_pci_device *pci_dev, struct virtio_hw *hw)
+{
+	if (pci_dev == NULL)
+		VTPCI_OPS(hw) = &virtio_user_ops;
+	else if (hw->modern)
+		VTPCI_OPS(hw) = &modern_ops;
+	else
+		VTPCI_OPS(hw) = &legacy_ops;
+}
+
+/*
  * This function is based on probe() function in virtio_pci.c
  * It returns 0 on success.
  */
@@ -1296,7 +1339,7 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
-	struct rte_pci_device *pci_dev;
+	struct rte_pci_device *pci_dev = eth_dev->pci_dev;
 	uint32_t dev_flags = RTE_ETH_DEV_DETACHABLE;
 	int ret;
 
@@ -1306,6 +1349,13 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 	eth_dev->tx_pkt_burst = &virtio_xmit_pkts;
 
 	if (rte_eal_process_type() == RTE_PROC_SECONDARY) {
+		if (pci_dev) {
+			ret = virtio_remap_pci(pci_dev, hw);
+			if (ret)
+				return ret;
+		}
+
+		virtio_set_vtpci_ops(pci_dev, hw);
 		if (hw->use_simple_rxtx) {
 			eth_dev->tx_pkt_burst = virtio_xmit_pkts_simple;
 			eth_dev->rx_pkt_burst = virtio_recv_pkts_vec;
@@ -1325,7 +1375,6 @@ static int virtio_dev_xstats_get_names(struct rte_eth_dev *dev,
 	}
 
 	hw->port_id = eth_dev->data->port_id;
-	pci_dev = eth_dev->pci_dev;
 
 	if (pci_dev) {
 		ret = vtpci_init(pci_dev, hw, &dev_flags);
diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index d1e9c05..f5754e5 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -303,7 +303,7 @@
 	return 0;
 }
 
-static const struct virtio_pci_ops legacy_ops = {
+const struct virtio_pci_ops legacy_ops = {
 	.read_dev_cfg	= legacy_read_dev_config,
 	.write_dev_cfg	= legacy_write_dev_config,
 	.reset		= legacy_reset,
@@ -519,7 +519,7 @@
 	io_write16(1, vq->notify_addr);
 }
 
-static const struct virtio_pci_ops modern_ops = {
+const struct virtio_pci_ops modern_ops = {
 	.read_dev_cfg	= modern_read_dev_config,
 	.write_dev_cfg	= modern_write_dev_config,
 	.reset		= modern_reset,
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 6b9aecf..511a1c8 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -333,4 +333,8 @@ int vtpci_init(struct rte_pci_device *, struct virtio_hw *,
 
 uint16_t vtpci_irq_config(struct virtio_hw *, uint16_t);
 
+extern const struct virtio_pci_ops legacy_ops;
+extern const struct virtio_pci_ops modern_ops;
+extern const struct virtio_pci_ops virtio_user_ops;
+
 #endif /* _VIRTIO_PCI_H_ */
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 7d2a9d9..3563952 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -212,7 +212,7 @@
 			    strerror(errno));
 }
 
-static const struct virtio_pci_ops virtio_user_ops = {
+const struct virtio_pci_ops virtio_user_ops = {
 	.read_dev_cfg	= virtio_user_read_dev_config,
 	.write_dev_cfg	= virtio_user_write_dev_config,
 	.reset		= virtio_user_reset,
-- 
1.9.0

[dpdk-dev] [PATCH v3 6/6] net/virtio: remove dead structure field

[Yuanhan Liu]

Actually, virtio_hw->dev is not used since the beginning when it's
introduced. Remove it.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 drivers/net/virtio/virtio_pci.c | 2 --
 drivers/net/virtio/virtio_pci.h | 1 -
 2 files changed, 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.c b/drivers/net/virtio/virtio_pci.c
index f5754e5..fbdb5b7 100644
--- a/drivers/net/virtio/virtio_pci.c
+++ b/drivers/net/virtio/virtio_pci.c
@@ -730,8 +730,6 @@
 vtpci_init(struct rte_pci_device *dev, struct virtio_hw *hw,
 	   uint32_t *dev_flags)
 {
-	hw->dev = dev;
-
 	/*
 	 * Try if we can succeed reading virtio pci caps, which exists
 	 * only on modern pci device. If failed, we fallback to legacy
diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 511a1c8..4235bef 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -258,7 +258,6 @@ struct virtio_hw {
 	uint32_t    notify_off_multiplier;
 	uint8_t     *isr;
 	uint16_t    *notify_base;
-	struct rte_pci_device *dev;
 	struct virtio_pci_common_cfg *common_cfg;
 	struct virtio_net_config *dev_cfg;
 	void	    *virtio_user_dev;
-- 
1.9.0

Re: [dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model

[Thomas Monjalon]

2017-01-06 18:16, Yuanhan Liu:
> +static void
> +eth_dev_init(struct rte_eth_dev *eth_dev, uint8_t port_id, const char *name)
> +{
> +	eth_dev->data = &rte_eth_dev_data[port_id];
> +	eth_dev->attached = DEV_ATTACHED;
> +	eth_dev_last_created_port = port_id;
> +	nb_ports++;
> +
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		snprintf(eth_dev->data->name, sizeof(eth_dev->data->name),
> +			 "%s", name);
> +		eth_dev->data->port_id = port_id;
> +	}

Why not keeping eth_dev->data filling in rte_eth_dev_allocate?

> +}
> +
>  struct rte_eth_dev *
>  rte_eth_dev_allocate(const char *name)
>  {
> @@ -211,12 +226,41 @@ struct rte_eth_dev *
>  	}
>  
>  	eth_dev = &rte_eth_devices[port_id];

Why not moving this line in eth_dev_init?

> -	eth_dev->data = &rte_eth_dev_data[port_id];
> -	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
> -	eth_dev->data->port_id = port_id;
> -	eth_dev->attached = DEV_ATTACHED;
> -	eth_dev_last_created_port = port_id;
> -	nb_ports++;
> +	eth_dev_init(eth_dev, port_id, name);
> +
> +	return eth_dev;
> +}

[...]
> +/*
> + * Attach to a port already registered by the primary process, which
> + * makes sure that the same device would have the same port id both
> + * in the primary and secondary process.
> + */
> +static struct rte_eth_dev *
> +eth_dev_attach_secondary(const char *name)

OK, good description

[...]
> -	eth_dev = rte_eth_dev_allocate(ethdev_name);
> -	if (eth_dev == NULL)
> -		return -ENOMEM;
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		eth_dev = rte_eth_dev_allocate(ethdev_name);
> +		if (eth_dev == NULL)
> +			return -ENOMEM;

You could merge here the rest of primary init below.

> +	} else {
> +		eth_dev = eth_dev_attach_secondary(ethdev_name);
> +		if (eth_dev == NULL) {
> +			/*
> +			 * if we failed to attach a device, it means
> +			 * the device is skipped, due to some errors.
> +			 * Take virtio-net device as example, it could
> +			 * due to the device is managed by virtio-net
> +			 * kernel driver.  For such case, we return a
> +			 * positive value, to let EAL skip it as well.
> +			 */

I'm not sure we need an example here.
Is the virtio case special?

nit: "it could due" looks to be a typo

> +			return 1;
> +		}
> +	}
>  
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>  		eth_dev->data->dev_private = rte_zmalloc("ethdev private structure",

Re: [dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model

[Yuanhan Liu]

On Fri, Jan 06, 2017 at 02:12:48PM +0100, Thomas Monjalon wrote:
> 2017-01-06 18:16, Yuanhan Liu:
> > +static void
> > +eth_dev_init(struct rte_eth_dev *eth_dev, uint8_t port_id, const char *name)
> > +{
> > +	eth_dev->data = &rte_eth_dev_data[port_id];
> > +	eth_dev->attached = DEV_ATTACHED;
> > +	eth_dev_last_created_port = port_id;
> > +	nb_ports++;
> > +
> > +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > +		snprintf(eth_dev->data->name, sizeof(eth_dev->data->name),
> > +			 "%s", name);
> > +		eth_dev->data->port_id = port_id;
> > +	}
> 
> Why not keeping eth_dev->data filling in rte_eth_dev_allocate?

I could do that.

> 
> > +}
> > +
> >  struct rte_eth_dev *
> >  rte_eth_dev_allocate(const char *name)
> >  {
> > @@ -211,12 +226,41 @@ struct rte_eth_dev *
> >  	}
> >  
> >  	eth_dev = &rte_eth_devices[port_id];
> 
> Why not moving this line in eth_dev_init?

Ditto.

> 
> > -	eth_dev->data = &rte_eth_dev_data[port_id];
> > -	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
> > -	eth_dev->data->port_id = port_id;
> > -	eth_dev->attached = DEV_ATTACHED;
> > -	eth_dev_last_created_port = port_id;
> > -	nb_ports++;
> > +	eth_dev_init(eth_dev, port_id, name);
> > +
> > +	return eth_dev;
> > +}
> 
> [...]
> > +/*
> > + * Attach to a port already registered by the primary process, which
> > + * makes sure that the same device would have the same port id both
> > + * in the primary and secondary process.
> > + */
> > +static struct rte_eth_dev *
> > +eth_dev_attach_secondary(const char *name)
> 
> OK, good description
> 
> [...]
> > -	eth_dev = rte_eth_dev_allocate(ethdev_name);
> > -	if (eth_dev == NULL)
> > -		return -ENOMEM;
> > +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> > +		eth_dev = rte_eth_dev_allocate(ethdev_name);
> > +		if (eth_dev == NULL)
> > +			return -ENOMEM;
> 
> You could merge here the rest of primary init below.

Oh, right.

> 
> > +	} else {
> > +		eth_dev = eth_dev_attach_secondary(ethdev_name);
> > +		if (eth_dev == NULL) {
> > +			/*
> > +			 * if we failed to attach a device, it means
> > +			 * the device is skipped, due to some errors.
> > +			 * Take virtio-net device as example, it could
> > +			 * due to the device is managed by virtio-net
> > +			 * kernel driver.  For such case, we return a
> > +			 * positive value, to let EAL skip it as well.
> > +			 */
> 
> I'm not sure we need an example here.
> Is the virtio case special?

Not quite sure, likely not. I was thinking a detailed example helps
people to understand better. I could remove it if you think it's
not necessary.

> nit: "it could due" looks to be a typo

Oops.

	--yliu

Re: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process

[Stephen Hemminger]

On Fri,  6 Jan 2017 18:16:16 +0800
Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:

> If the primary enables the vector Rx/Tx path, the current code would
> let the secondary always choose the non vector Rx/Tx path. This results
> to a Rx/Tx method mismatch between primary and secondary process. Werid
> errors then may happen, something like:
> 
>     PMD: virtio_xmit_pkts() tx: virtqueue_enqueue error: -14
> 
> Fix it by choosing the correct Rx/Tx callbacks for the secondary process.
> That is, use vector path if it's given.
> 
> Fixes: 8d8393fb1861 ("virtio: pick simple Rx/Tx")
> 
> Cc: stable@dpdk.org
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

This is failing on intel compile tests.


http://dpdk.org/patch/18975

_Compilation issues_

Submitter: Yuanhan Liu <yuanhan.liu at linux.intel.com>
Date: Fri,  6 Jan 2017 18:16:18 +0800
DPDK git baseline: Repo:dpdk-next-virtio, Branch:master, CommitID:2b2669fc4c792f9a3ab73490bb93f7810a71c089

Patch18974-18975 --> compile error
Build Summary: 18 Builds Done, 0 Successful, 18 Failures

Test environment and configuration as below:
OS: RHEL7.2_64
    Kernel Version:3.10.0-327.el7.x86_64
    CPU info:Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz
    GCC Version:gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-4)
    Clang Version:3.4.2
    i686-native-linuxapp-gcc
    x86_64-native-linuxapp-gcc
    x86_64-native-linuxapp-gcc-shared
OS: FreeBSD10.3_64
    Kernel Version:10.3-RELEASE
    CPU info: Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz (2194.97-MHz K8-class CPU)
    GCC Version:gcc (FreeBSD Ports Collection) 4.8.5
    Clang Version:3.4.1
    x86_64-native-bsdapp-clang
    x86_64-native-bsdapp-gcc
OS: FC24_64
    Kernel Version:4.8.6-201.fc24.x86_64
    CPU info:Intel(R) Xeon(R) CPU E5-2699 v3 @ 2.30GHz
    GCC Version:gcc (GCC) 6.2.1 20160916 (Red Hat 6.2.1-2)
    Clang Version:3.8.0
    x86_64-native-linuxapp-gcc-debug
    i686-native-linuxapp-gcc
    x86_64-native-linuxapp-gcc
    x86_64-native-linuxapp-gcc-shared
    x86_64-native-linuxapp-clang
OS: UB1604_64
    Kernel Version:4.4.0-53-generic
    CPU info:Intel(R) Xeon(R) CPU E5-2699 v3 @ 2.30GHz
    GCC Version:gcc (Ubuntu 5.4.0-6ubuntu1~16.04.4) 5.4.0 20160609
    Clang Version:3.8.0
    i686-native-linuxapp-gcc
    x86_64-native-linuxapp-gcc
    x86_64-native-linuxapp-gcc-shared
    x86_64-native-linuxapp-clang
OS: CentOS7_64
    Kernel Version:3.10.0-327.el7.x86_64
    CPU info:Intel(R) Xeon(R) CPU E5-2699 v3 @ 2.30GHz
    GCC Version:gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-4)
    Clang Version:3.4.2
    i686-native-linuxapp-gcc
    x86_64-native-linuxapp-clang
    x86_64-native-linuxapp-gcc-shared
    x86_64-native-linuxapp-gcc

Failed Build #1:
OS: RHEL7.2_64
Target: i686-native-linuxapp-gcc
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/i686-native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x930): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status


Failed Build #2:
OS: RHEL7.2_64
Target: x86_64-native-linuxapp-gcc
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x9c4): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status


Failed Build #3:
OS: RHEL7.2_64
Target: x86_64-native-linuxapp-gcc-shared

/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c: In function ‘rte_eth_dev_pci_probe’:
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:3: warning: implicit declaration of function ‘eth_dev_attach_secondary’ [-Wimplicit-function-declaration]
   eth_dev = eth_dev_attach_secondary(ethdev_name);
   ^
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:3: warning: nested extern declaration of ‘eth_dev_attach_secondary’ [-Wnested-externs]
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:11: warning: assignment makes pointer from integer without a cast [enabled by default]
   eth_dev = eth_dev_attach_secondary(ethdev_name);
           ^
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c: At top level:
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:193:1: warning: ‘eth_dev_init’ defined but not used [-Wunused-function]
 eth_dev_init(struct rte_eth_dev *eth_dev, uint8_t port_id, const char *name)
 ^  LD librte_ethdev.so.5.1
rte_ethdev.o: In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0xa37): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status


Failed Build #4:
OS: FreeBSD10.3_64
Target: x86_64-native-bsdapp-clang
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-bsdapp-clang/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:(.text+0x295): undefined reference to `eth_dev_attach_secondary'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
/home/patchWorkOrg/compilation/mk/rte.app.mk:231: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'app' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #5:
OS: FreeBSD10.3_64
Target: x86_64-native-bsdapp-gcc
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-bsdapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x9c4): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status
/home/patchWorkOrg/compilation/mk/rte.app.mk:231: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'app' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #6:
OS: FC24_64
Target: x86_64-native-linuxapp-gcc-debug

MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0xba6): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status
/home/patchWorkOrg/compilation/mk/rte.app.mk:231: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'app' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #7:
OS: FC24_64
Target: i686-native-linuxapp-gcc
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/i686-native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x89d): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status
/home/patchWorkOrg/compilation/mk/rte.app.mk:231: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'app' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #8:
OS: FC24_64
Target: x86_64-native-linuxapp-gcc
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x916): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status
/home/patchWorkOrg/compilation/mk/rte.app.mk:231: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'app' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #9:
OS: FC24_64
Target: x86_64-native-linuxapp-gcc-shared

/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c: In function ‘rte_eth_dev_pci_probe’:
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:13: warning: implicit declaration of function ‘eth_dev_attach_secondary’ [-Wimplicit-function-declaration]
   eth_dev = eth_dev_attach_secondary(ethdev_name);
             ^~~~~~~~~~~~~~~~~~~~~~~~
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:3: warning: nested extern declaration of ‘eth_dev_attach_secondary’ [-Wnested-externs]
   eth_dev = eth_dev_attach_secondary(ethdev_name);
   ^~~~~~~
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:11: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
   eth_dev = eth_dev_attach_secondary(ethdev_name);
           ^
At top level:
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:193:1: warning: ‘eth_dev_init’ defined but not used [-Wunused-function]
 eth_dev_init(struct rte_eth_dev *eth_dev, uint8_t port_id, const char *name)
 ^~~~~~~~~~~~  LD librte_ethdev.so.5.1
rte_ethdev.o: In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x98d): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status
/home/patchWorkOrg/compilation/mk/rte.lib.mk:120: recipe for target 'librte_ethdev.so.5.1' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'librte_ether' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'lib' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #10:
OS: FC24_64
Target: x86_64-native-linuxapp-clang
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-linuxapp-clang/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:(.text+0x346): undefined reference to `eth_dev_attach_secondary'
clang-3.8: error: linker command failed with exit code 1 (use -v to see invocation)
/home/patchWorkOrg/compilation/mk/rte.app.mk:231: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'app' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #11:
OS: UB1604_64
Target: i686-native-linuxapp-gcc
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/i686-native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x901): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status
/home/patchWorkOrg/compilation/mk/rte.app.mk:231: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'app' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #12:
OS: UB1604_64
Target: x86_64-native-linuxapp-gcc
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x974): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status
/home/patchWorkOrg/compilation/mk/rte.app.mk:231: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'app' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #13:
OS: UB1604_64
Target: x86_64-native-linuxapp-gcc-shared

/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c: In function ‘rte_eth_dev_pci_probe’:
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:13: warning: implicit declaration of function ‘eth_dev_attach_secondary’ [-Wimplicit-function-declaration]
   eth_dev = eth_dev_attach_secondary(ethdev_name);
             ^
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:3: warning: nested extern declaration of ‘eth_dev_attach_secondary’ [-Wnested-externs]
   eth_dev = eth_dev_attach_secondary(ethdev_name);
   ^
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:11: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
   eth_dev = eth_dev_attach_secondary(ethdev_name);
           ^
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c: At top level:
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:193:1: warning: ‘eth_dev_init’ defined but not used [-Wunused-function]
 eth_dev_init(struct rte_eth_dev *eth_dev, uint8_t port_id, const char *name)
 ^  LD librte_ethdev.so.5.1
rte_ethdev.o: In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x9ba): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status
/home/patchWorkOrg/compilation/mk/rte.lib.mk:120: recipe for target 'librte_ethdev.so.5.1' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'librte_ether' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'lib' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #14:
OS: UB1604_64
Target: x86_64-native-linuxapp-clang
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-linuxapp-clang/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:(.text+0x346): undefined reference to `eth_dev_attach_secondary'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
/home/patchWorkOrg/compilation/mk/rte.app.mk:231: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.subdir.mk:61: recipe for target 'test' failed
/home/patchWorkOrg/compilation/mk/rte.sdkbuild.mk:78: recipe for target 'app' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:126: recipe for target 'all' failed
/home/patchWorkOrg/compilation/mk/rte.sdkinstall.mk:85: recipe for target 'pre_install' failed
/home/patchWorkOrg/compilation/mk/rte.sdkroot.mk:101: recipe for target 'install' failed


Failed Build #15:
OS: CentOS7_64
Target: i686-native-linuxapp-gcc
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/i686-native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x930): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status


Failed Build #16:
OS: CentOS7_64
Target: x86_64-native-linuxapp-clang
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-linuxapp-clang/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:(.text+0x273): undefined reference to `eth_dev_attach_secondary'
clang: error: linker command failed with exit code 1 (use -v to see invocation)


Failed Build #17:
OS: CentOS7_64
Target: x86_64-native-linuxapp-gcc-shared

/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c: In function ‘rte_eth_dev_pci_probe’:
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:3: warning: implicit declaration of function ‘eth_dev_attach_secondary’ [-Wimplicit-function-declaration]
   eth_dev = eth_dev_attach_secondary(ethdev_name);
   ^
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:3: warning: nested extern declaration of ‘eth_dev_attach_secondary’ [-Wnested-externs]
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:273:11: warning: assignment makes pointer from integer without a cast [enabled by default]
   eth_dev = eth_dev_attach_secondary(ethdev_name);
           ^
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c: At top level:
/home/patchWorkOrg/compilation/lib/librte_ether/rte_ethdev.c:193:1: warning: ‘eth_dev_init’ defined but not used [-Wunused-function]
 eth_dev_init(struct rte_eth_dev *eth_dev, uint8_t port_id, const char *name)
 ^  LD librte_ethdev.so.5.1
rte_ethdev.o: In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0xa37): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status


Failed Build #18:
OS: CentOS7_64
Target: x86_64-native-linuxapp-gcc
MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
rte_ethdev.c:(.text+0x9c4): undefined reference to `eth_dev_attach_secondary'
collect2: error: ld returned 1 exit status

Re: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process

[Yuanhan Liu]

On Sun, Jan 08, 2017 at 03:15:00PM -0800, Stephen Hemminger wrote:
> On Fri,  6 Jan 2017 18:16:16 +0800
> Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> 
> > If the primary enables the vector Rx/Tx path, the current code would
> > let the secondary always choose the non vector Rx/Tx path. This results
> > to a Rx/Tx method mismatch between primary and secondary process. Werid
> > errors then may happen, something like:
> > 
> >     PMD: virtio_xmit_pkts() tx: virtqueue_enqueue error: -14
> > 
> > Fix it by choosing the correct Rx/Tx callbacks for the secondary process.
> > That is, use vector path if it's given.
> > 
> > Fixes: 8d8393fb1861 ("virtio: pick simple Rx/Tx")
> > 
> > Cc: stable@dpdk.org
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> 
> This is failing on intel compile tests.
> 
> 
> http://dpdk.org/patch/18975

Thanks, but it looks like a false alarm to me, for reasons below.

> Failed Build #2:
> OS: RHEL7.2_64
> Target: x86_64-native-linuxapp-gcc
> MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function `rte_eth_dev_pci_probe':
> rte_ethdev.c:(.text+0x9c4): undefined reference to `eth_dev_attach_secondary'
> collect2: error: ld returned 1 exit status

- eth_dev_attach_secondary is not defined in this patch, it's defined
  (and used) in the first patch.

- eth_dev_attach_secondary is actually defined; The report even shows
  it fails to build with gcc: the gcc build passes on my dev box.

Honestly, I seldom trusted the build reports from STV.

	--yliu

[dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model

[Yuanhan Liu]

Assume we have two virtio ports, 00:03.0 and 00:04.0. The first one is
managed by the kernel driver, while the later one is managed by DPDK.

Now we start the primary process. 00:03.0 will be skipped by DPDK virtio
PMD driver (since it's being used by the kernel). 00:04.0 would be
successfully initiated by DPDK virtio PMD (if nothing abnormal happens).
After that, we would get a port id 0, and all the related info needed
by virtio (virtio_hw) is stored at rte_eth_dev_data[0].

Then we start the secondary process. As usual, 00:03.0 will be firstly
probed. It firstly tries to get a local eth_dev structure for it (by
rte_eth_dev_allocate):

        port_id = rte_eth_dev_find_free_port();
        ...

        eth_dev = &rte_eth_devices[port_id];
        eth_dev->data = &rte_eth_dev_data[port_id];
        ...

        return eth_dev;

Since it's a first PCI device, port_id will be 0. eth_dev->data would
then point to rte_eth_dev_data[0]. And here things start going wrong,
as rte_eth_dev_data[0] actually stores the virtio_hw for 00:04.0.

That said, in the secondary process, DPDK will continue to drive PCI
device 00.03.0 (despite the fact it's been managed by kernel), with
the info from PCI device 00:04.0. Which is wrong.

The fix is to attach the port already registered by the primary process:
iterate the rte_eth_dev_data[], and get the port id who's PCI ID matches
the current PCI device.

This would let us maintain same port ID for the same PCI device, keeping
the chance of referencing to wrong data minimal.

Fixes: af75078fece3 ("first public release")

Cc: stable@dpdk.org
Cc: Thomas Monjalon <thomas.monjalon@6wind.com>
Cc: Bruce Richardson <bruce.richardson@intel.com>
Cc: Ferruh Yigit <ferruh.yigit@intel.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---

v4: - assign eth_dev in the common eth_dev init help function
      it also renamed to eth_dev_get, to not confuse with the
      eth_dev_init callback.
    - move primoary process specific assignments to rte_eth_dev_allocate
    - drop the virtio example in comment
    - combine two code block for primary into one

v3: - do not move rte_eth_dev_data_alloc to pci_probe
    - rename eth_dev_attach to eth_dev_attach_secondary
    - introduce eth_dev_init() for common eth_dev struct initiation
    - move comment block inside the "if" block
---
 lib/librte_ether/rte_ethdev.c | 71 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 62 insertions(+), 9 deletions(-)

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fde8112..1453df1 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -189,6 +189,19 @@ struct rte_eth_dev *
 	return RTE_MAX_ETHPORTS;
 }
 
+static struct rte_eth_dev *
+eth_dev_get(uint8_t port_id)
+{
+	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
+
+	eth_dev->data = &rte_eth_dev_data[port_id];
+	eth_dev->attached = DEV_ATTACHED;
+	eth_dev_last_created_port = port_id;
+	nb_ports++;
+
+	return eth_dev;
+}
+
 struct rte_eth_dev *
 rte_eth_dev_allocate(const char *name)
 {
@@ -210,13 +223,41 @@ struct rte_eth_dev *
 		return NULL;
 	}
 
-	eth_dev = &rte_eth_devices[port_id];
-	eth_dev->data = &rte_eth_dev_data[port_id];
+	eth_dev = eth_dev_get(port_id);
 	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
 	eth_dev->data->port_id = port_id;
-	eth_dev->attached = DEV_ATTACHED;
-	eth_dev_last_created_port = port_id;
-	nb_ports++;
+
+	return eth_dev;
+}
+
+/*
+ * Attach to a port already registered by the primary process, which
+ * makes sure that the same device would have the same port id both
+ * in the primary and secondary process.
+ */
+static struct rte_eth_dev *
+eth_dev_attach_secondary(const char *name)
+{
+	uint8_t i;
+	struct rte_eth_dev *eth_dev;
+
+	if (rte_eth_dev_data == NULL)
+		rte_eth_dev_data_alloc();
+
+	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
+		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
+			break;
+	}
+	if (i == RTE_MAX_ETHPORTS) {
+		RTE_PMD_DEBUG_TRACE(
+			"device %s is not driven by the primary process\n",
+			name);
+		return NULL;
+	}
+
+	eth_dev = eth_dev_get(i);
+	RTE_ASSERT(eth_dev->data->port_id == i);
+
 	return eth_dev;
 }
 
@@ -246,16 +287,28 @@ struct rte_eth_dev *
 	rte_eal_pci_device_name(&pci_dev->addr, ethdev_name,
 			sizeof(ethdev_name));
 
-	eth_dev = rte_eth_dev_allocate(ethdev_name);
-	if (eth_dev == NULL)
-		return -ENOMEM;
-
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		eth_dev = rte_eth_dev_allocate(ethdev_name);
+		if (eth_dev == NULL)
+			return -ENOMEM;
+
 		eth_dev->data->dev_private = rte_zmalloc("ethdev private structure",
 				  eth_drv->dev_private_size,
 				  RTE_CACHE_LINE_SIZE);
 		if (eth_dev->data->dev_private == NULL)
 			rte_panic("Cannot allocate memzone for private port data\n");
+	} else {
+		eth_dev = eth_dev_attach_secondary(ethdev_name);
+		if (eth_dev == NULL) {
+			/*
+			 * if we failed to attach a device, it means the
+			 * device is skipped in primary process, due to
+			 * some errors. If so, we return a positive value,
+			 * to let EAL skip it for the secondary process
+			 * as well.
+			 */
+			return 1;
+		}
 	}
 	eth_dev->pci_dev = pci_dev;
 	eth_dev->driver = eth_drv;
-- 
1.9.0

Re: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process

[Xu, Qian Q]

Fangfang, 
Could you help double confirm below error and the patchset ? Thanks. 
http://dpdk.org/patch/18975

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, January 9, 2017 1:19 PM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: dev@dpdk.org; stable@dpdk.org; Xu, Qian Q <qian.q.xu@intel.com>; Liu,
> Yong <yong.liu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for
> secondary process
> 
> On Sun, Jan 08, 2017 at 03:15:00PM -0800, Stephen Hemminger wrote:
> > On Fri,  6 Jan 2017 18:16:16 +0800
> > Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> >
> > > If the primary enables the vector Rx/Tx path, the current code would
> > > let the secondary always choose the non vector Rx/Tx path. This
> > > results to a Rx/Tx method mismatch between primary and secondary
> > > process. Werid errors then may happen, something like:
> > >
> > >     PMD: virtio_xmit_pkts() tx: virtqueue_enqueue error: -14
> > >
> > > Fix it by choosing the correct Rx/Tx callbacks for the secondary process.
> > > That is, use vector path if it's given.
> > >
> > > Fixes: 8d8393fb1861 ("virtio: pick simple Rx/Tx")
> > >
> > > Cc: stable@dpdk.org
> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >
> > This is failing on intel compile tests.
> >
> >
> > http://dpdk.org/patch/18975
> 
> Thanks, but it looks like a false alarm to me, for reasons below.
> 
> > Failed Build #2:
> > OS: RHEL7.2_64
> > Target: x86_64-native-linuxapp-gcc
> > MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-
> native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function
> `rte_eth_dev_pci_probe':
> > rte_ethdev.c:(.text+0x9c4): undefined reference to
> `eth_dev_attach_secondary'
> > collect2: error: ld returned 1 exit status
> 
> - eth_dev_attach_secondary is not defined in this patch, it's defined
>   (and used) in the first patch.
> 
> - eth_dev_attach_secondary is actually defined; The report even shows
>   it fails to build with gcc: the gcc build passes on my dev box.
> 
> Honestly, I seldom trusted the build reports from STV.
> 
> 	--yliu

Re: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process

[Wei, FangfangX]

I've re-build it manually, it passed with this patch.


-----Original Message-----
From: Xu, Qian Q 
Sent: Monday, January 9, 2017 4:02 PM
To: Yuanhan Liu <yuanhan.liu@linux.intel.com>; Stephen Hemminger <stephen@networkplumber.org>; Wei, FangfangX <fangfangx.wei@intel.com>
Cc: dev@dpdk.org; stable@dpdk.org; Liu, Yong <yong.liu@intel.com>
Subject: RE: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process

Fangfang,
Could you help double confirm below error and the patchset ? Thanks. 
http://dpdk.org/patch/18975

> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Monday, January 9, 2017 1:19 PM
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: dev@dpdk.org; stable@dpdk.org; Xu, Qian Q <qian.q.xu@intel.com>; 
> Liu, Yong <yong.liu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx 
> method for secondary process
> 
> On Sun, Jan 08, 2017 at 03:15:00PM -0800, Stephen Hemminger wrote:
> > On Fri,  6 Jan 2017 18:16:16 +0800
> > Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:
> >
> > > If the primary enables the vector Rx/Tx path, the current code 
> > > would let the secondary always choose the non vector Rx/Tx path. 
> > > This results to a Rx/Tx method mismatch between primary and 
> > > secondary process. Werid errors then may happen, something like:
> > >
> > >     PMD: virtio_xmit_pkts() tx: virtqueue_enqueue error: -14
> > >
> > > Fix it by choosing the correct Rx/Tx callbacks for the secondary process.
> > > That is, use vector path if it's given.
> > >
> > > Fixes: 8d8393fb1861 ("virtio: pick simple Rx/Tx")
> > >
> > > Cc: stable@dpdk.org
> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >
> > This is failing on intel compile tests.
> >
> >
> > http://dpdk.org/patch/18975
> 
> Thanks, but it looks like a false alarm to me, for reasons below.
> 
> > Failed Build #2:
> > OS: RHEL7.2_64
> > Target: x86_64-native-linuxapp-gcc
> > MKRES test_resource_c.res.o  /home/patchWorkOrg/compilation/x86_64-
> native-linuxapp-gcc/lib/librte_ethdev.a(rte_ethdev.o): In function
> `rte_eth_dev_pci_probe':
> > rte_ethdev.c:(.text+0x9c4): undefined reference to
> `eth_dev_attach_secondary'
> > collect2: error: ld returned 1 exit status
> 
> - eth_dev_attach_secondary is not defined in this patch, it's defined
>   (and used) in the first patch.
> 
> - eth_dev_attach_secondary is actually defined; The report even shows
>   it fails to build with gcc: the gcc build passes on my dev box.
> 
> Honestly, I seldom trusted the build reports from STV.
> 
> 	--yliu

Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model

[Thomas Monjalon]

Hi Yuanhan,

Nit: the title should be "v4 1/6"
Except that, good patch :)

> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model

[Yuanhan Liu]

On Mon, Jan 09, 2017 at 06:08:04PM +0100, Thomas Monjalon wrote:
> Hi Yuanhan,
> 
> Nit: the title should be "v4 1/6"

Oops, my bad.

> Except that, good patch :)
> 
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> 
> Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>

Thanks for review! Mind if I apply it to the next-virtio tree?

	--yliu

Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model

[Thomas Monjalon]

2017-01-10 22:33, Yuanhan Liu:
> On Mon, Jan 09, 2017 at 06:08:04PM +0100, Thomas Monjalon wrote:
> > Hi Yuanhan,
> > 
> > Nit: the title should be "v4 1/6"
> 
> Oops, my bad.
> 
> > Except that, good patch :)
> > 
> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > 
> > Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> 
> Thanks for review! Mind if I apply it to the next-virtio tree?

Yes, please do.

Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model

[Yuanhan Liu]

On Wed, Jan 11, 2017 at 02:32:03PM +0100, Thomas Monjalon wrote:
> 2017-01-10 22:33, Yuanhan Liu:
> > On Mon, Jan 09, 2017 at 06:08:04PM +0100, Thomas Monjalon wrote:
> > > Hi Yuanhan,
> > > 
> > > Nit: the title should be "v4 1/6"
> > 
> > Oops, my bad.
> > 
> > > Except that, good patch :)
> > > 
> > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > > 
> > > Acked-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> > 
> > Thanks for review! Mind if I apply it to the next-virtio tree?
> 
> Yes, please do.

Thanks!

Series applied to dpdk-next-virtio.

	--yliu

Re: [dpdk-dev] [PATCH v3 6/6] net/virtio: remove dead structure field

[Yuanhan Liu]

On Fri, Jan 06, 2017 at 06:16:20PM +0800, Yuanhan Liu wrote:
> Actually, virtio_hw->dev is not used since the beginning when it's
> introduced. Remove it.

It's not true after the refactoring of decoupling from PCI device.
This patch is dropped. Instead, two more patches will be sent soon.

	--yliu

Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model

[Ferruh Yigit]

On 1/9/2017 7:50 AM, Yuanhan Liu wrote:
> Assume we have two virtio ports, 00:03.0 and 00:04.0. The first one is
> managed by the kernel driver, while the later one is managed by DPDK.
> 
> Now we start the primary process. 00:03.0 will be skipped by DPDK virtio
> PMD driver (since it's being used by the kernel). 00:04.0 would be
> successfully initiated by DPDK virtio PMD (if nothing abnormal happens).
> After that, we would get a port id 0, and all the related info needed
> by virtio (virtio_hw) is stored at rte_eth_dev_data[0].
> 
> Then we start the secondary process. As usual, 00:03.0 will be firstly
> probed. It firstly tries to get a local eth_dev structure for it (by
> rte_eth_dev_allocate):
> 
>         port_id = rte_eth_dev_find_free_port();
>         ...
> 
>         eth_dev = &rte_eth_devices[port_id];
>         eth_dev->data = &rte_eth_dev_data[port_id];
>         ...
> 
>         return eth_dev;
> 
> Since it's a first PCI device, port_id will be 0. eth_dev->data would
> then point to rte_eth_dev_data[0]. And here things start going wrong,
> as rte_eth_dev_data[0] actually stores the virtio_hw for 00:04.0.
> 
> That said, in the secondary process, DPDK will continue to drive PCI
> device 00.03.0 (despite the fact it's been managed by kernel), with
> the info from PCI device 00:04.0. Which is wrong.
> 
> The fix is to attach the port already registered by the primary process:
> iterate the rte_eth_dev_data[], and get the port id who's PCI ID matches
> the current PCI device.
> 
> This would let us maintain same port ID for the same PCI device, keeping
> the chance of referencing to wrong data minimal.
> 
> Fixes: af75078fece3 ("first public release")
> 
> Cc: stable@dpdk.org
> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>
> Cc: Bruce Richardson <bruce.richardson@intel.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> 
> v4: - assign eth_dev in the common eth_dev init help function
>       it also renamed to eth_dev_get, to not confuse with the
>       eth_dev_init callback.
>     - move primoary process specific assignments to rte_eth_dev_allocate
>     - drop the virtio example in comment
>     - combine two code block for primary into one
> 
> v3: - do not move rte_eth_dev_data_alloc to pci_probe
>     - rename eth_dev_attach to eth_dev_attach_secondary
>     - introduce eth_dev_init() for common eth_dev struct initiation
>     - move comment block inside the "if" block
> ---
>  lib/librte_ether/rte_ethdev.c | 71 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 62 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index fde8112..1453df1 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -189,6 +189,19 @@ struct rte_eth_dev *
>  	return RTE_MAX_ETHPORTS;
>  }
>  
> +static struct rte_eth_dev *
> +eth_dev_get(uint8_t port_id)
> +{
> +	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
> +
> +	eth_dev->data = &rte_eth_dev_data[port_id];
> +	eth_dev->attached = DEV_ATTACHED;
> +	eth_dev_last_created_port = port_id;
> +	nb_ports++;
> +
> +	return eth_dev;
> +}
> +
>  struct rte_eth_dev *
>  rte_eth_dev_allocate(const char *name)
>  {
> @@ -210,13 +223,41 @@ struct rte_eth_dev *
>  		return NULL;
>  	}
>  
> -	eth_dev = &rte_eth_devices[port_id];
> -	eth_dev->data = &rte_eth_dev_data[port_id];
> +	eth_dev = eth_dev_get(port_id);

There can be a merge issue here, please help me understand.

In repo, different from seen here, this patch does this here:
  -       eth_dev = &rte_eth_devices[port_id];
  -       eth_dev->data = &rte_eth_dev_data[port_id];
  -       memset(eth_dev->data, 0, sizeof(*eth_dev->data));
  +       memset(&rte_eth_devices[port_id], 0, sizeof(*eth_dev->data));
  +       eth_dev = eth_dev_get(port_id);

Which no more resets the eth_dev->data, but rte_eth_devices[port_id]
(with sizeof(*eth_dev->data))

memset(eth_dev->data) added by Jan Blunck on comment:
7f95f78a8aea ("ethdev: clear data when allocating device")

most probably it should stay as "memset(eth_dev->data)", but if not,
please aware that commit 7f95f78a8aea removed some assignment from
drivers relying this memset, they needs to be added back.

>  	snprintf(eth_dev->data->name, sizeof(eth_dev->data->name), "%s", name);
>  	eth_dev->data->port_id = port_id;
> -	eth_dev->attached = DEV_ATTACHED;
> -	eth_dev_last_created_port = port_id;
> -	nb_ports++;
> +
> +	return eth_dev;
> +}
> +
> +/*
> + * Attach to a port already registered by the primary process, which
> + * makes sure that the same device would have the same port id both
> + * in the primary and secondary process.
> + */
> +static struct rte_eth_dev *
> +eth_dev_attach_secondary(const char *name)
> +{
> +	uint8_t i;
> +	struct rte_eth_dev *eth_dev;
> +
> +	if (rte_eth_dev_data == NULL)
> +		rte_eth_dev_data_alloc();
> +
> +	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> +		if (strcmp(rte_eth_dev_data[i].name, name) == 0)
> +			break;
> +	}
> +	if (i == RTE_MAX_ETHPORTS) {
> +		RTE_PMD_DEBUG_TRACE(
> +			"device %s is not driven by the primary process\n",
> +			name);
> +		return NULL;
> +	}
> +
> +	eth_dev = eth_dev_get(i);
> +	RTE_ASSERT(eth_dev->data->port_id == i);
> +
>  	return eth_dev;
>  }
>  
> @@ -246,16 +287,28 @@ struct rte_eth_dev *
>  	rte_eal_pci_device_name(&pci_dev->addr, ethdev_name,
>  			sizeof(ethdev_name));
>  
> -	eth_dev = rte_eth_dev_allocate(ethdev_name);
> -	if (eth_dev == NULL)
> -		return -ENOMEM;
> -
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		eth_dev = rte_eth_dev_allocate(ethdev_name);
> +		if (eth_dev == NULL)
> +			return -ENOMEM;
> +
>  		eth_dev->data->dev_private = rte_zmalloc("ethdev private structure",
>  				  eth_drv->dev_private_size,
>  				  RTE_CACHE_LINE_SIZE);
>  		if (eth_dev->data->dev_private == NULL)
>  			rte_panic("Cannot allocate memzone for private port data\n");
> +	} else {
> +		eth_dev = eth_dev_attach_secondary(ethdev_name);
> +		if (eth_dev == NULL) {
> +			/*
> +			 * if we failed to attach a device, it means the
> +			 * device is skipped in primary process, due to
> +			 * some errors. If so, we return a positive value,
> +			 * to let EAL skip it for the secondary process
> +			 * as well.
> +			 */
> +			return 1;
> +		}
>  	}
>  	eth_dev->pci_dev = pci_dev;
>  	eth_dev->driver = eth_drv;
> 

Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model

[Yuanhan Liu]

On Thu, Jan 19, 2017 at 06:39:13PM +0000, Ferruh Yigit wrote:
> >  struct rte_eth_dev *
> >  rte_eth_dev_allocate(const char *name)
> >  {
> > @@ -210,13 +223,41 @@ struct rte_eth_dev *
> >  		return NULL;
> >  	}
> >  
> > -	eth_dev = &rte_eth_devices[port_id];
> > -	eth_dev->data = &rte_eth_dev_data[port_id];
> > +	eth_dev = eth_dev_get(port_id);
> 
> There can be a merge issue here, please help me understand.
> 
> In repo, different from seen here, this patch does this here:
>   -       eth_dev = &rte_eth_devices[port_id];
>   -       eth_dev->data = &rte_eth_dev_data[port_id];
>   -       memset(eth_dev->data, 0, sizeof(*eth_dev->data));
>   +       memset(&rte_eth_devices[port_id], 0, sizeof(*eth_dev->data));
>   +       eth_dev = eth_dev_get(port_id);
> 
> Which no more resets the eth_dev->data, but rte_eth_devices[port_id]
> (with sizeof(*eth_dev->data))

Nice catch! Sorry, it's a silly error by auto-complete :/
As you said, it should be:

    memset(&rte_eth_dev_data[port_id], 0, sizeof(struct rte_eth_dev_data));

Will make a patch soon. Thanks.

	--yliu

> 
> memset(eth_dev->data) added by Jan Blunck on comment:
> 7f95f78a8aea ("ethdev: clear data when allocating device")
> 
> most probably it should stay as "memset(eth_dev->data)", but if not,
> please aware that commit 7f95f78a8aea removed some assignment from
> drivers relying this memset, they needs to be added back.