[dpdk-dev,v3,2/7] net/virtio_user: fix not properly reset device

Message ID 1483502366-140154-3-git-send-email-jianfeng.tan@intel.com
State Superseded, archived
Delegated to: Yuanhan Liu
Headers show

Checks

Context Check Description
ci/Intel compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Jianfeng Tan Jan. 4, 2017, 3:59 a.m.
virtio_user is not properly reset when users call vtpci_reset(),
as it ignores VIRTIO_CONFIG_STATUS_RESET status in
virtio_user_set_status().

This might lead to initialization failure as it starts to re-init
the device before sending RESET messege to backend. Besides, previous
callfds and kickfds are not closed.

To fix it, we add support to disable virtqueues when it's set to
DRIVER OK status, and re-init fields in struct virtio_user_dev.

Fixes: e9efa4d93821 ("net/virtio-user: add new virtual PCI driver")
Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer")

CC: stable@dpdk.org

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 26 ++++++++++++++++--------
 drivers/net/virtio/virtio_user_ethdev.c          | 15 ++++++++------
 2 files changed, 27 insertions(+), 14 deletions(-)

Comments

Yuanhan Liu Jan. 4, 2017, 5:46 a.m. | #1
On Wed, Jan 04, 2017 at 03:59:21AM +0000, Jianfeng Tan wrote:
> virtio_user is not properly reset when users call vtpci_reset(),
> as it ignores VIRTIO_CONFIG_STATUS_RESET status in
> virtio_user_set_status().
> 
> This might lead to initialization failure as it starts to re-init
> the device before sending RESET messege to backend. Besides, previous
> callfds and kickfds are not closed.
> 
> To fix it, we add support to disable virtqueues when it's set to
> DRIVER OK status, and re-init fields in struct virtio_user_dev.
> 
> Fixes: e9efa4d93821 ("net/virtio-user: add new virtual PCI driver")
> Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer")
> 
> CC: stable@dpdk.org
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Note that, typically, there should be no empty line between 'Cc' and SoB.

> ---
>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 26 ++++++++++++++++--------
>  drivers/net/virtio/virtio_user_ethdev.c          | 15 ++++++++------
>  2 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 0d7e17b..a38398b 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -182,7 +182,17 @@ virtio_user_start_device(struct virtio_user_dev *dev)
>  
>  int virtio_user_stop_device(struct virtio_user_dev *dev)

The name doesn't seem to be well named: "dev_stop" comes to my firstly
when I saw that :/

Rename it to "xxx_reset_device"?

	--yliu

Patch

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 0d7e17b..a38398b 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -182,7 +182,17 @@  virtio_user_start_device(struct virtio_user_dev *dev)
 
 int virtio_user_stop_device(struct virtio_user_dev *dev)
 {
-	return vhost_user_sock(dev->vhostfd, VHOST_USER_RESET_OWNER, NULL);
+	uint32_t i;
+
+	for (i = 0; i < dev->max_queue_pairs * 2; ++i) {
+		close(dev->callfds[i]);
+		close(dev->kickfds[i]);
+	}
+
+	for (i = 0; i < dev->max_queue_pairs; ++i)
+		vhost_user_enable_queue_pair(dev->vhostfd, i, 0);
+
+	return 0;
 }
 
 static inline void
@@ -210,6 +220,8 @@  int
 virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		     int cq, int queue_size, const char *mac)
 {
+	uint32_t i;
+
 	snprintf(dev->path, PATH_MAX, "%s", path);
 	dev->max_queue_pairs = queues;
 	dev->queue_pairs = 1; /* mq disabled by default */
@@ -218,6 +230,11 @@  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 	parse_mac(dev, mac);
 	dev->vhostfd = -1;
 
+	for (i = 0; i < VIRTIO_MAX_VIRTQUEUES * 2 + 1; ++i) {
+		dev->kickfds[i] = -1;
+		dev->callfds[i] = -1;
+	}
+
 	dev->vhostfd = vhost_user_setup(dev->path);
 	if (dev->vhostfd < 0) {
 		PMD_INIT_LOG(ERR, "backend set up fails");
@@ -264,13 +281,6 @@  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 void
 virtio_user_dev_uninit(struct virtio_user_dev *dev)
 {
-	uint32_t i;
-
-	for (i = 0; i < dev->max_queue_pairs * 2; ++i) {
-		close(dev->callfds[i]);
-		close(dev->kickfds[i]);
-	}
-
 	close(dev->vhostfd);
 }
 
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 4a5a227..93f5b01 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -87,21 +87,24 @@  virtio_user_write_dev_config(struct virtio_hw *hw, size_t offset,
 }
 
 static void
-virtio_user_set_status(struct virtio_hw *hw, uint8_t status)
+virtio_user_reset(struct virtio_hw *hw)
 {
 	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
 
-	if (status & VIRTIO_CONFIG_STATUS_DRIVER_OK)
-		virtio_user_start_device(dev);
-	dev->status = status;
+	if (dev->status & VIRTIO_CONFIG_STATUS_DRIVER_OK)
+		virtio_user_stop_device(dev);
 }
 
 static void
-virtio_user_reset(struct virtio_hw *hw)
+virtio_user_set_status(struct virtio_hw *hw, uint8_t status)
 {
 	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
 
-	virtio_user_stop_device(dev);
+	if (status & VIRTIO_CONFIG_STATUS_DRIVER_OK)
+		virtio_user_start_device(dev);
+	else if (status == VIRTIO_CONFIG_STATUS_RESET)
+		virtio_user_reset(hw);
+	dev->status = status;
 }
 
 static uint8_t