* [dpdk-stable] [PATCH v2 2/3] net/virtio_user: fix wrong sequence of messages
2016-09-27 19:11 ` [dpdk-stable] [PATCH v2 " Jianfeng Tan
2016-09-27 19:11 ` [dpdk-stable] [PATCH v2 1/3] net/virtio_user: fix queue pair not enabled Jianfeng Tan
@ 2016-09-27 19:11 ` Jianfeng Tan
2016-09-27 19:11 ` [dpdk-stable] [PATCH v2 3/3] net/virtio_user: fix dev not freed after init error Jianfeng Tan
2 siblings, 0 replies; 6+ messages in thread
From: Jianfeng Tan @ 2016-09-27 19:11 UTC (permalink / raw)
To: dev; +Cc: stable, yuanhan.liu, Jianfeng Tan
When virtio_user is used with VPP's native vhost user, it cannot
send/receive any packets.
The root cause is that vpp-vhost-user translates the message
VHOST_USER_SET_FEATURES as puting this device into init state,
aka, zero all related structures. However, previous code
puts this message at last in the whole initialization process,
which leads to all previous information are zeroed.
To fix this issue, we rearrange the sequence of those messages.
- step 0, send VHOST_USER_SET_VRING_CALL so that vhost allocates
virtqueue structures;
- step 1, send VHOST_USER_SET_FEATURES to confirm the features;
- step 2, send VHOST_USER_SET_MEM_TABLE to share mem regions;
- step 3, send VHOST_USER_SET_VRING_NUM, VHOST_USER_SET_VRING_BASE,
VHOST_USER_SET_VRING_ADDR, VHOST_USER_SET_VRING_KICK for each
queue;
- ...
Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer")
Reported-by: Zhihong Wang <zhihong.wang@intel.com>
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
drivers/net/virtio/virtio_user/virtio_user_dev.c | 113 ++++++++++++++---------
1 file changed, 69 insertions(+), 44 deletions(-)
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index bf1155c..e239e0e 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -45,20 +45,14 @@
#include "../virtio_ethdev.h"
static int
-virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
+virtio_user_create_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
{
- int callfd, kickfd;
+ /* Of all per virtqueue MSGs, make sure VHOST_SET_VRING_CALL come
+ * firstly because vhost depends on this msg to allocate virtqueue
+ * pair.
+ */
+ int callfd;
struct vhost_vring_file file;
- struct vhost_vring_state state;
- struct vring *vring = &dev->vrings[queue_sel];
- struct vhost_vring_addr addr = {
- .index = queue_sel,
- .desc_user_addr = (uint64_t)(uintptr_t)vring->desc,
- .avail_user_addr = (uint64_t)(uintptr_t)vring->avail,
- .used_user_addr = (uint64_t)(uintptr_t)vring->used,
- .log_guest_addr = 0,
- .flags = 0, /* disable log */
- };
/* May use invalid flag, but some backend leverages kickfd and callfd as
* criteria to judge if dev is alive. so finally we use real event_fd.
@@ -68,22 +62,30 @@ virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
PMD_DRV_LOG(ERR, "callfd error, %s\n", strerror(errno));
return -1;
}
- kickfd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
- if (kickfd < 0) {
- close(callfd);
- PMD_DRV_LOG(ERR, "kickfd error, %s\n", strerror(errno));
- return -1;
- }
-
- /* Of all per virtqueue MSGs, make sure VHOST_SET_VRING_CALL come
- * firstly because vhost depends on this msg to allocate virtqueue
- * pair.
- */
file.index = queue_sel;
file.fd = callfd;
vhost_user_sock(dev->vhostfd, VHOST_USER_SET_VRING_CALL, &file);
dev->callfds[queue_sel] = callfd;
+ return 0;
+}
+
+static int
+virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
+{
+ int kickfd;
+ struct vhost_vring_file file;
+ struct vhost_vring_state state;
+ struct vring *vring = &dev->vrings[queue_sel];
+ struct vhost_vring_addr addr = {
+ .index = queue_sel,
+ .desc_user_addr = (uint64_t)(uintptr_t)vring->desc,
+ .avail_user_addr = (uint64_t)(uintptr_t)vring->avail,
+ .used_user_addr = (uint64_t)(uintptr_t)vring->used,
+ .log_guest_addr = 0,
+ .flags = 0, /* disable log */
+ };
+
state.index = queue_sel;
state.num = vring->num;
vhost_user_sock(dev->vhostfd, VHOST_USER_SET_VRING_NUM, &state);
@@ -97,6 +99,12 @@ virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
* lastly because vhost depends on this msg to judge if
* virtio is ready.
*/
+ kickfd = eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK);
+ if (kickfd < 0) {
+ PMD_DRV_LOG(ERR, "kickfd error, %s\n", strerror(errno));
+ return -1;
+ }
+ file.index = queue_sel;
file.fd = kickfd;
vhost_user_sock(dev->vhostfd, VHOST_USER_SET_VRING_KICK, &file);
dev->kickfds[queue_sel] = kickfd;
@@ -104,40 +112,43 @@ virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
return 0;
}
-int
-virtio_user_start_device(struct virtio_user_dev *dev)
+static int
+virtio_user_queue_setup(struct virtio_user_dev *dev,
+ int (*fn)(struct virtio_user_dev *, uint32_t))
{
- uint64_t features;
uint32_t i, queue_sel;
- int ret;
-
- /* construct memory region inside each implementation */
- ret = vhost_user_sock(dev->vhostfd, VHOST_USER_SET_MEM_TABLE, NULL);
- if (ret < 0)
- goto error;
for (i = 0; i < dev->max_queue_pairs; ++i) {
queue_sel = 2 * i + VTNET_SQ_RQ_QUEUE_IDX;
- if (virtio_user_kick_queue(dev, queue_sel) < 0) {
- PMD_DRV_LOG(INFO, "kick rx vq fails: %u", i);
- goto error;
+ if (fn(dev, queue_sel) < 0) {
+ PMD_DRV_LOG(INFO, "setup rx vq fails: %u", i);
+ return -1;
}
}
for (i = 0; i < dev->max_queue_pairs; ++i) {
queue_sel = 2 * i + VTNET_SQ_TQ_QUEUE_IDX;
- if (virtio_user_kick_queue(dev, queue_sel) < 0) {
- PMD_DRV_LOG(INFO, "kick tx vq fails: %u", i);
- goto error;
+ if (fn(dev, queue_sel) < 0) {
+ PMD_DRV_LOG(INFO, "setup tx vq fails: %u", i);
+ return -1;
}
}
- /* we enable the 1st queue pair by default. */
- vhost_user_enable_queue_pair(dev->vhostfd, 0, 1);
+ return 0;
+}
+
+int
+virtio_user_start_device(struct virtio_user_dev *dev)
+{
+ uint64_t features;
+ int ret;
+
+ /* Step 0: tell vhost to create queues */
+ if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
+ goto error;
- /* After setup all virtqueues, we need to set_features so that these
- * features can be set into each virtqueue in vhost side. And before
- * that, make sure VHOST_USER_F_PROTOCOL_FEATURES is added if mq is
- * enabled, and VIRTIO_NET_F_MAC is stripped.
+ /* Step 1: set features
+ * Make sure VHOST_USER_F_PROTOCOL_FEATURES is added if mq is enabled,
+ * and VIRTIO_NET_F_MAC is stripped.
*/
features = dev->features;
if (dev->max_queue_pairs > 1)
@@ -148,6 +159,20 @@ virtio_user_start_device(struct virtio_user_dev *dev)
goto error;
PMD_DRV_LOG(INFO, "set features: %" PRIx64, features);
+ /* Step 2: share memory regions */
+ ret = vhost_user_sock(dev->vhostfd, VHOST_USER_SET_MEM_TABLE, NULL);
+ if (ret < 0)
+ goto error;
+
+ /* Step 3: kick queues */
+ if (virtio_user_queue_setup(dev, virtio_user_kick_queue) < 0)
+ goto error;
+
+ /* Step 4: enable queues
+ * we enable the 1st queue pair by default.
+ */
+ vhost_user_enable_queue_pair(dev->vhostfd, 0, 1);
+
return 0;
error:
/* TODO: free resource here or caller to check */
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread* [dpdk-stable] [PATCH v2 3/3] net/virtio_user: fix dev not freed after init error
2016-09-27 19:11 ` [dpdk-stable] [PATCH v2 " Jianfeng Tan
2016-09-27 19:11 ` [dpdk-stable] [PATCH v2 1/3] net/virtio_user: fix queue pair not enabled Jianfeng Tan
2016-09-27 19:11 ` [dpdk-stable] [PATCH v2 2/3] net/virtio_user: fix wrong sequence of messages Jianfeng Tan
@ 2016-09-27 19:11 ` Jianfeng Tan
2 siblings, 0 replies; 6+ messages in thread
From: Jianfeng Tan @ 2016-09-27 19:11 UTC (permalink / raw)
To: dev; +Cc: stable, yuanhan.liu, Jianfeng Tan
Currently, when virtio_user device fails to be started (e.g., vhost
unix socket does not exit), the init function does not return struct
rte_eth_dev (and some other structs) back to ether layer. And what's
more, it does not report the error to upper layer.
The fix is to free those structs and report error when failing to
start virtio_user devices.
Fixes: ce2eabdd43ec ("net/virtio-user: add virtual device")
Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
drivers/net/virtio/virtio_user_ethdev.c | 42 ++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index daef09b..bba7402 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -313,6 +313,17 @@ virtio_user_eth_dev_alloc(const char *name)
return eth_dev;
}
+static void
+virtio_user_eth_dev_free(struct rte_eth_dev *eth_dev)
+{
+ struct rte_eth_dev_data *data = eth_dev->data;
+ struct virtio_hw *hw = data->dev_private;
+
+ rte_free(hw->virtio_user_dev);
+ rte_free(hw);
+ rte_eth_dev_release_port(eth_dev);
+}
+
/* Dev initialization routine. Invoked once for each virtio vdev at
* EAL init time, see rte_eal_dev_init().
* Returns 0 on success.
@@ -343,9 +354,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
}
if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_PATH) == 1) {
- ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_PATH,
- &get_string_arg, &path);
- if (ret < 0) {
+ if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_PATH,
+ &get_string_arg, &path) < 0) {
PMD_INIT_LOG(ERR, "error to parse %s",
VIRTIO_USER_ARG_PATH);
goto end;
@@ -357,9 +367,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
}
if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_MAC) == 1) {
- ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_MAC,
- &get_string_arg, &mac_addr);
- if (ret < 0) {
+ if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_MAC,
+ &get_string_arg, &mac_addr) < 0) {
PMD_INIT_LOG(ERR, "error to parse %s",
VIRTIO_USER_ARG_MAC);
goto end;
@@ -367,9 +376,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
}
if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_QUEUE_SIZE) == 1) {
- ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUE_SIZE,
- &get_integer_arg, &queue_size);
- if (ret < 0) {
+ if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUE_SIZE,
+ &get_integer_arg, &queue_size) < 0) {
PMD_INIT_LOG(ERR, "error to parse %s",
VIRTIO_USER_ARG_QUEUE_SIZE);
goto end;
@@ -377,9 +385,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
}
if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_QUEUES_NUM) == 1) {
- ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUES_NUM,
- &get_integer_arg, &queues);
- if (ret < 0) {
+ if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_QUEUES_NUM,
+ &get_integer_arg, &queues) < 0) {
PMD_INIT_LOG(ERR, "error to parse %s",
VIRTIO_USER_ARG_QUEUES_NUM);
goto end;
@@ -387,9 +394,8 @@ virtio_user_pmd_devinit(const char *name, const char *params)
}
if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_CQ_NUM) == 1) {
- ret = rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
- &get_integer_arg, &cq);
- if (ret < 0) {
+ if (rte_kvargs_process(kvlist, VIRTIO_USER_ARG_CQ_NUM,
+ &get_integer_arg, &cq) < 0) {
PMD_INIT_LOG(ERR, "error to parse %s",
VIRTIO_USER_ARG_CQ_NUM);
goto end;
@@ -411,12 +417,16 @@ virtio_user_pmd_devinit(const char *name, const char *params)
hw = eth_dev->data->dev_private;
if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
- queue_size, mac_addr) < 0)
+ queue_size, mac_addr) < 0) {
+ PMD_INIT_LOG(ERR, "virtio_user_dev_init fails");
+ virtio_user_eth_dev_free(eth_dev);
goto end;
+ }
/* previously called by rte_eal_pci_probe() for physical dev */
if (eth_virtio_dev_init(eth_dev) < 0) {
PMD_INIT_LOG(ERR, "eth_virtio_dev_init fails");
+ virtio_user_eth_dev_free(eth_dev);
goto end;
}
ret = 0;
--
2.7.4
^ permalink raw reply [flat|nested] 6+ messages in thread