* [dpdk-dev] [PATCH v2 1/6] ethdev: fix port data mismatched in multiple process model
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 0/6] net/virtio: fix several " Yuanhan Liu
@ 2016-12-28 11:02 ` Yuanhan Liu
2017-01-04 17:34 ` Thomas Monjalon
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 2/6] net/virtio: fix wrong Rx/Tx method for secondary process Yuanhan Liu
` (5 subsequent siblings)
6 siblings, 1 reply; 37+ messages in thread
From: Yuanhan Liu @ 2016-12-28 11:02 UTC (permalink / raw)
To: dev; +Cc: Yuanhan Liu, stable, Thomas Monjalon, Bruce Richardson, Ferruh Yigit
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [dpdk-dev] [PATCH v2 1/6] ethdev: fix port data mismatched in multiple process model
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 1/6] ethdev: fix port data mismatched in multiple process model Yuanhan Liu
@ 2017-01-04 17:34 ` Thomas Monjalon
2017-01-05 6:25 ` Yuanhan Liu
0 siblings, 1 reply; 37+ messages in thread
From: Thomas Monjalon @ 2017-01-04 17:34 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev, Bruce Richardson, Ferruh Yigit, Gonzalez Monroy, Sergio
+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;
> + }
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [dpdk-dev] [PATCH v2 1/6] ethdev: fix port data mismatched in multiple process model
2017-01-04 17:34 ` Thomas Monjalon
@ 2017-01-05 6:25 ` Yuanhan Liu
0 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-05 6:25 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, Bruce Richardson, Ferruh Yigit, Gonzalez Monroy, Sergio
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [dpdk-dev] [PATCH v2 2/6] net/virtio: fix wrong Rx/Tx method for secondary process
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 0/6] net/virtio: fix several " Yuanhan Liu
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 1/6] ethdev: fix port data mismatched in multiple process model Yuanhan Liu
@ 2016-12-28 11:02 ` Yuanhan Liu
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 3/6] net/virtio: store PCI operators pointer locally Yuanhan Liu
` (4 subsequent siblings)
6 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2016-12-28 11:02 UTC (permalink / raw)
To: dev; +Cc: Yuanhan Liu, stable
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
^ permalink raw reply [flat|nested] 37+ messages in thread* [dpdk-dev] [PATCH v2 3/6] net/virtio: store PCI operators pointer locally
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 0/6] net/virtio: fix several " Yuanhan Liu
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 1/6] ethdev: fix port data mismatched in multiple process model Yuanhan Liu
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 2/6] net/virtio: fix wrong Rx/Tx method for secondary process Yuanhan Liu
@ 2016-12-28 11:02 ` Yuanhan Liu
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 4/6] net/virtio: store IO port info locally Yuanhan Liu
` (3 subsequent siblings)
6 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2016-12-28 11:02 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 37+ messages in thread* [dpdk-dev] [PATCH v2 4/6] net/virtio: store IO port info locally
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 0/6] net/virtio: fix several " Yuanhan Liu
` (2 preceding siblings ...)
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 3/6] net/virtio: store PCI operators pointer locally Yuanhan Liu
@ 2016-12-28 11:02 ` Yuanhan Liu
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 5/6] net/virtio: fix multiple process support Yuanhan Liu
` (2 subsequent siblings)
6 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2016-12-28 11:02 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 37+ messages in thread* [dpdk-dev] [PATCH v2 5/6] net/virtio: fix multiple process support
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 0/6] net/virtio: fix several " Yuanhan Liu
` (3 preceding siblings ...)
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 4/6] net/virtio: store IO port info locally Yuanhan Liu
@ 2016-12-28 11:02 ` Yuanhan Liu
2016-12-28 11:14 ` Yuanhan Liu
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 6/6] net/virtio: remove dead structure field Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 0/6] net/virtio: fix several multiple process issues Yuanhan Liu
6 siblings, 1 reply; 37+ messages in thread
From: Yuanhan Liu @ 2016-12-28 11:02 UTC (permalink / raw)
To: dev; +Cc: Yuanhan Liu, stable, Juho Snellman, Yaron Illouz
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
^ permalink raw reply [flat|nested] 37+ messages in thread* [dpdk-dev] [PATCH v2 6/6] net/virtio: remove dead structure field
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 0/6] net/virtio: fix several " Yuanhan Liu
` (4 preceding siblings ...)
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 5/6] net/virtio: fix multiple process support Yuanhan Liu
@ 2016-12-28 11:02 ` Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 0/6] net/virtio: fix several multiple process issues Yuanhan Liu
6 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2016-12-28 11:02 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 37+ messages in thread* [dpdk-dev] [PATCH v3 0/6] net/virtio: fix several multiple process issues
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 0/6] net/virtio: fix several " Yuanhan Liu
` (5 preceding siblings ...)
2016-12-28 11:02 ` [dpdk-dev] [PATCH v2 6/6] net/virtio: remove dead structure field Yuanhan Liu
@ 2017-01-06 10:16 ` Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model Yuanhan Liu
` (5 more replies)
6 siblings, 6 replies; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-06 10:16 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 37+ messages in thread* [dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 0/6] net/virtio: fix several multiple process issues Yuanhan Liu
@ 2017-01-06 10:16 ` Yuanhan Liu
2017-01-06 13:12 ` Thomas Monjalon
2017-01-09 7:50 ` [dpdk-dev] [PATCH v4] " Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process Yuanhan Liu
` (4 subsequent siblings)
5 siblings, 2 replies; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-06 10:16 UTC (permalink / raw)
To: dev; +Cc: Yuanhan Liu, stable, Thomas Monjalon, Bruce Richardson, Ferruh Yigit
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model Yuanhan Liu
@ 2017-01-06 13:12 ` Thomas Monjalon
2017-01-06 14:19 ` Yuanhan Liu
2017-01-09 7:50 ` [dpdk-dev] [PATCH v4] " Yuanhan Liu
1 sibling, 1 reply; 37+ messages in thread
From: Thomas Monjalon @ 2017-01-06 13:12 UTC (permalink / raw)
To: Yuanhan Liu, Bruce Richardson; +Cc: dev, Ferruh Yigit
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",
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model
2017-01-06 13:12 ` Thomas Monjalon
@ 2017-01-06 14:19 ` Yuanhan Liu
0 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-06 14:19 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: Bruce Richardson, dev, Ferruh Yigit
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model Yuanhan Liu
2017-01-06 13:12 ` Thomas Monjalon
@ 2017-01-09 7:50 ` Yuanhan Liu
2017-01-09 17:08 ` Thomas Monjalon
2017-01-19 18:39 ` Ferruh Yigit
1 sibling, 2 replies; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-09 7:50 UTC (permalink / raw)
To: dev; +Cc: Yuanhan Liu, stable, Thomas Monjalon, Bruce Richardson, Ferruh Yigit
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model
2017-01-09 7:50 ` [dpdk-dev] [PATCH v4] " Yuanhan Liu
@ 2017-01-09 17:08 ` Thomas Monjalon
2017-01-10 14:33 ` Yuanhan Liu
2017-01-19 18:39 ` Ferruh Yigit
1 sibling, 1 reply; 37+ messages in thread
From: Thomas Monjalon @ 2017-01-09 17:08 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev, stable, Bruce Richardson, Ferruh Yigit
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>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model
2017-01-09 17:08 ` Thomas Monjalon
@ 2017-01-10 14:33 ` Yuanhan Liu
2017-01-11 13:32 ` Thomas Monjalon
0 siblings, 1 reply; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-10 14:33 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, stable, Bruce Richardson, Ferruh Yigit
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model
2017-01-10 14:33 ` Yuanhan Liu
@ 2017-01-11 13:32 ` Thomas Monjalon
2017-01-12 3:10 ` Yuanhan Liu
0 siblings, 1 reply; 37+ messages in thread
From: Thomas Monjalon @ 2017-01-11 13:32 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev, stable, Bruce Richardson, Ferruh Yigit
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.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model
2017-01-11 13:32 ` Thomas Monjalon
@ 2017-01-12 3:10 ` Yuanhan Liu
0 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-12 3:10 UTC (permalink / raw)
To: Thomas Monjalon; +Cc: dev, stable, Bruce Richardson, Ferruh Yigit
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model
2017-01-09 7:50 ` [dpdk-dev] [PATCH v4] " Yuanhan Liu
2017-01-09 17:08 ` Thomas Monjalon
@ 2017-01-19 18:39 ` Ferruh Yigit
2017-01-20 7:58 ` Yuanhan Liu
1 sibling, 1 reply; 37+ messages in thread
From: Ferruh Yigit @ 2017-01-19 18:39 UTC (permalink / raw)
To: Yuanhan Liu, dev; +Cc: stable, Thomas Monjalon, Bruce Richardson, Jan Blunck
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;
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [dpdk-dev] [PATCH v4] ethdev: fix port data mismatched in multiple process model
2017-01-19 18:39 ` Ferruh Yigit
@ 2017-01-20 7:58 ` Yuanhan Liu
0 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-20 7:58 UTC (permalink / raw)
To: Ferruh Yigit; +Cc: dev, stable, Thomas Monjalon, Bruce Richardson, Jan Blunck
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.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 0/6] net/virtio: fix several multiple process issues Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model Yuanhan Liu
@ 2017-01-06 10:16 ` Yuanhan Liu
2017-01-08 23:15 ` Stephen Hemminger
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 3/6] net/virtio: store PCI operators pointer locally Yuanhan Liu
` (3 subsequent siblings)
5 siblings, 1 reply; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-06 10:16 UTC (permalink / raw)
To: dev; +Cc: Yuanhan Liu, stable
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process Yuanhan Liu
@ 2017-01-08 23:15 ` Stephen Hemminger
2017-01-09 5:19 ` Yuanhan Liu
0 siblings, 1 reply; 37+ messages in thread
From: Stephen Hemminger @ 2017-01-08 23:15 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev, stable
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process
2017-01-08 23:15 ` Stephen Hemminger
@ 2017-01-09 5:19 ` Yuanhan Liu
2017-01-09 8:02 ` Xu, Qian Q
0 siblings, 1 reply; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-09 5:19 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, stable, Xu, Qian Q, Liu, Yong
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process
2017-01-09 5:19 ` Yuanhan Liu
@ 2017-01-09 8:02 ` Xu, Qian Q
2017-01-09 8:05 ` Wei, FangfangX
0 siblings, 1 reply; 37+ messages in thread
From: Xu, Qian Q @ 2017-01-09 8:02 UTC (permalink / raw)
To: Yuanhan Liu, Stephen Hemminger, Wei, FangfangX; +Cc: dev, stable, Liu, Yong
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process
2017-01-09 8:02 ` Xu, Qian Q
@ 2017-01-09 8:05 ` Wei, FangfangX
0 siblings, 0 replies; 37+ messages in thread
From: Wei, FangfangX @ 2017-01-09 8:05 UTC (permalink / raw)
To: Xu, Qian Q, Yuanhan Liu, Stephen Hemminger; +Cc: dev, stable, Liu, Yong
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [dpdk-dev] [PATCH v3 3/6] net/virtio: store PCI operators pointer locally
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 0/6] net/virtio: fix several multiple process issues Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 1/6] ethdev: fix port data mismatched in multiple process model Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 2/6] net/virtio: fix wrong Rx/Tx method for secondary process Yuanhan Liu
@ 2017-01-06 10:16 ` Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 4/6] net/virtio: store IO port info locally Yuanhan Liu
` (2 subsequent siblings)
5 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-06 10:16 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 37+ messages in thread* [dpdk-dev] [PATCH v3 4/6] net/virtio: store IO port info locally
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 0/6] net/virtio: fix several multiple process issues Yuanhan Liu
` (2 preceding siblings ...)
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 3/6] net/virtio: store PCI operators pointer locally Yuanhan Liu
@ 2017-01-06 10:16 ` Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 5/6] net/virtio: fix multiple process support Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 6/6] net/virtio: remove dead structure field Yuanhan Liu
5 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-06 10:16 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 37+ messages in thread* [dpdk-dev] [PATCH v3 5/6] net/virtio: fix multiple process support
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 0/6] net/virtio: fix several multiple process issues Yuanhan Liu
` (3 preceding siblings ...)
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 4/6] net/virtio: store IO port info locally Yuanhan Liu
@ 2017-01-06 10:16 ` Yuanhan Liu
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 6/6] net/virtio: remove dead structure field Yuanhan Liu
5 siblings, 0 replies; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-06 10:16 UTC (permalink / raw)
To: dev; +Cc: Yuanhan Liu, stable, Juho Snellman, Yaron Illouz
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
^ permalink raw reply [flat|nested] 37+ messages in thread* [dpdk-dev] [PATCH v3 6/6] net/virtio: remove dead structure field
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 0/6] net/virtio: fix several multiple process issues Yuanhan Liu
` (4 preceding siblings ...)
2017-01-06 10:16 ` [dpdk-dev] [PATCH v3 5/6] net/virtio: fix multiple process support Yuanhan Liu
@ 2017-01-06 10:16 ` Yuanhan Liu
2017-01-12 6:02 ` Yuanhan Liu
5 siblings, 1 reply; 37+ messages in thread
From: Yuanhan Liu @ 2017-01-06 10:16 UTC (permalink / raw)
To: dev; +Cc: 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
^ permalink raw reply [flat|nested] 37+ messages in thread