[dpdk-stable] [PATCH 1/6] net/virtio-user: do not stop stopped device again

[dpdk-stable] [PATCH 1/6] net/virtio-user: do not stop stopped device again

[Tiwei Bie]

Without this change, virtio-user still works, but it will show
annoying error messages like this on shutdown:

vhost_kernel_set_backend(): VHOST_NET_SET_BACKEND fails, Operation not permitted
vhost_kernel_ioctl(): VHOST_RESET_OWNER failed: Operation not permitted

Fixes: e3b434818bbb ("net/virtio-user: support kernel vhost")
Fixes: 12ecb2f63b12 ("net/virtio-user: support memory hotplug")
Cc: stable@dpdk.org

Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index a185aed34..684702c56 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -184,6 +184,9 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
 	uint32_t i;
 
 	pthread_mutex_lock(&dev->mutex);
+	if (!dev->started)
+		goto out;
+
 	for (i = 0; i < dev->max_queue_pairs; ++i)
 		dev->ops->enable_qp(dev, i, 0);
 
@@ -193,6 +196,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
 		return -1;
 	}
 	dev->started = false;
+out:
 	pthread_mutex_unlock(&dev->mutex);
 
 	return 0;
-- 
2.19.1

Re: [dpdk-stable] [PATCH 1/6] net/virtio-user: do not stop stopped device again

[Maxime Coquelin]

On 10/29/18 6:28 AM, Tiwei Bie wrote:
> Without this change, virtio-user still works, but it will show
> annoying error messages like this on shutdown:
> 
> vhost_kernel_set_backend(): VHOST_NET_SET_BACKEND fails, Operation not permitted
> vhost_kernel_ioctl(): VHOST_RESET_OWNER failed: Operation not permitted
> 
> Fixes: e3b434818bbb ("net/virtio-user: support kernel vhost")
> Fixes: 12ecb2f63b12 ("net/virtio-user: support memory hotplug")
> Cc: stable@dpdk.org
> 
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_user/virtio_user_dev.c | 4 ++++
>   1 file changed, 4 insertions(+)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-stable] [PATCH 2/6] net/virtio-user: do not make vhost user channel nonblock

[Tiwei Bie]

There is no need to make the vhost user channel nonblock, and
making it nonblock will make vhost_user_read() fail with EAGAIN
when vhost messages need a reply.

Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index b51cbc85b..2100e43db 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -28,7 +28,6 @@ static int
 virtio_user_server_reconnect(struct virtio_user_dev *dev)
 {
 	int ret;
-	int flag;
 	int connectfd;
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
 
@@ -49,9 +48,6 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 
 	dev->features &= dev->device_features;
 
-	flag = fcntl(connectfd, F_GETFD);
-	fcntl(connectfd, F_SETFL, flag | O_NONBLOCK);
-
 	ret = virtio_user_start_device(dev);
 	if (ret < 0)
 		return -1;
-- 
2.19.1

Re: [dpdk-stable] [PATCH 2/6] net/virtio-user: do not make vhost user channel nonblock

[Maxime Coquelin]

On 10/29/18 6:28 AM, Tiwei Bie wrote:
> There is no need to make the vhost user channel nonblock, and
> making it nonblock will make vhost_user_read() fail with EAGAIN
> when vhost messages need a reply.
> 
> Fixes: bd8f50a45d0f ("net/virtio-user: support server mode")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_user_ethdev.c | 4 ----
>   1 file changed, 4 deletions(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-stable] [PATCH 3/6] net/virtio-user: do not reset owner when driver resets

[Tiwei Bie]

When driver resets the device, virtio-user just needs to send
GET_VRING_BASE messages to stop the vhost backend, and that's
what QEMU does. With this change, we won't need to set owner
when starting virtio-user device anymore. This will help us to
get rid of below error message on startup:

vhost_kernel_ioctl(): VHOST_SET_OWNER failed: Device or resource busy

Fixes: bce7e9050f9b ("net/virtio-user: fix start with kernel vhost")
Fixes: 0d6a8752ac9d ("net/virtio-user: fix crash as features change")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 .../net/virtio/virtio_user/virtio_user_dev.c  | 22 ++++++++++++-------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 684702c56..be70414a1 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -134,9 +134,6 @@ virtio_user_start_device(struct virtio_user_dev *dev)
 	if (is_vhost_user_by_type(dev->path) && dev->vhostfd < 0)
 		goto error;
 
-	/* Do not check return as already done in init, or reset in stop */
-	dev->ops->send_request(dev, VHOST_USER_SET_OWNER, NULL);
-
 	/* Step 0: tell vhost to create queues */
 	if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
 		goto error;
@@ -181,7 +178,9 @@ virtio_user_start_device(struct virtio_user_dev *dev)
 
 int virtio_user_stop_device(struct virtio_user_dev *dev)
 {
+	struct vhost_vring_state state;
 	uint32_t i;
+	int error = 0;
 
 	pthread_mutex_lock(&dev->mutex);
 	if (!dev->started)
@@ -190,16 +189,23 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
 	for (i = 0; i < dev->max_queue_pairs; ++i)
 		dev->ops->enable_qp(dev, i, 0);
 
-	if (dev->ops->send_request(dev, VHOST_USER_RESET_OWNER, NULL) < 0) {
-		PMD_DRV_LOG(INFO, "Failed to reset the device\n");
-		pthread_mutex_unlock(&dev->mutex);
-		return -1;
+	/* Stop the backend. */
+	for (i = 0; i < dev->max_queue_pairs * 2; ++i) {
+		state.index = i;
+		if (dev->ops->send_request(dev, VHOST_USER_GET_VRING_BASE,
+					   &state) < 0) {
+			PMD_DRV_LOG(ERR, "get_vring_base failed, index=%u\n",
+				    i);
+			error = -1;
+			goto out;
+		}
 	}
+
 	dev->started = false;
 out:
 	pthread_mutex_unlock(&dev->mutex);
 
-	return 0;
+	return error;
 }
 
 static inline void
-- 
2.19.1

Re: [dpdk-stable] [PATCH 3/6] net/virtio-user: do not reset owner when driver resets

[Maxime Coquelin]

On 10/29/18 6:28 AM, Tiwei Bie wrote:
> When driver resets the device, virtio-user just needs to send
> GET_VRING_BASE messages to stop the vhost backend, and that's
> what QEMU does. With this change, we won't need to set owner
> when starting virtio-user device anymore. This will help us to
> get rid of below error message on startup:
> 
> vhost_kernel_ioctl(): VHOST_SET_OWNER failed: Device or resource busy
> 
> Fixes: bce7e9050f9b ("net/virtio-user: fix start with kernel vhost")
> Fixes: 0d6a8752ac9d ("net/virtio-user: fix crash as features change")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   .../net/virtio/virtio_user/virtio_user_dev.c  | 22 ++++++++++++-------
>   1 file changed, 14 insertions(+), 8 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-stable] [PATCH 4/6] net/virtio-user: fix device features for server mode

[Tiwei Bie]

We need to save the supported frontend features (which won't be
announced by vhost backend), otherwise we will lost them when the
connection to vhost-user backend is established in server mode.

Fixes: 201a41651715 ("net/virtio-user: fix multiple queues fail in server mode")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 8 +++++---
 drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
 drivers/net/virtio/virtio_user_ethdev.c          | 2 ++
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index be70414a1..44e093eb7 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -421,6 +421,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 	dev->queue_pairs = 1; /* mq disabled by default */
 	dev->queue_size = queue_size;
 	dev->mac_specified = 0;
+	dev->frontend_features = 0;
 	dev->unsupported_features = 0;
 	parse_mac(dev, mac);
 
@@ -468,7 +469,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 	}
 
 	if (dev->mac_specified) {
-		dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
+		dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC);
 	} else {
 		dev->device_features &= ~(1ull << VIRTIO_NET_F_MAC);
 		dev->unsupported_features |= (1ull << VIRTIO_NET_F_MAC);
@@ -478,7 +479,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		/* device does not really need to know anything about CQ,
 		 * so if necessary, we just claim to support CQ
 		 */
-		dev->device_features |= (1ull << VIRTIO_NET_F_CTRL_VQ);
+		dev->frontend_features |= (1ull << VIRTIO_NET_F_CTRL_VQ);
 	} else {
 		dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
 		/* Also disable features depends on VIRTIO_NET_F_CTRL_VQ */
@@ -499,8 +500,9 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 
 	/* The backend will not report this feature, we add it explicitly */
 	if (is_vhost_user_by_type(dev->path))
-		dev->device_features |= (1ull << VIRTIO_NET_F_STATUS);
+		dev->frontend_features |= (1ull << VIRTIO_NET_F_STATUS);
 
+	dev->device_features |= dev->frontend_features;
 	dev->device_features &= VIRTIO_USER_SUPPORTED_FEATURES;
 	dev->unsupported_features |= ~VIRTIO_USER_SUPPORTED_FEATURES;
 
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index d6e0e137b..c42ce5d4b 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -33,6 +33,7 @@ struct virtio_user_dev {
 				   * and will be sync with device
 				   */
 	uint64_t	device_features; /* supported features by device */
+	uint64_t	frontend_features; /* enabled frontend features */
 	uint64_t	unsupported_features; /* unsupported features mask */
 	uint8_t		status;
 	uint16_t	port_id;
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 2100e43db..c419d5b9d 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -43,6 +43,8 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 		return -1;
 	}
 
+	dev->device_features |= dev->frontend_features;
+
 	/* umask vhost-user unsupported features */
 	dev->device_features &= ~(dev->unsupported_features);
 
-- 
2.19.1

Re: [dpdk-stable] [PATCH 4/6] net/virtio-user: fix device features for server mode

[Maxime Coquelin]

On 10/29/18 6:28 AM, Tiwei Bie wrote:
> We need to save the supported frontend features (which won't be
> announced by vhost backend), otherwise we will lost them when the
> connection to vhost-user backend is established in server mode.
> 
> Fixes: 201a41651715 ("net/virtio-user: fix multiple queues fail in server mode")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_user/virtio_user_dev.c | 8 +++++---
>   drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
>   drivers/net/virtio/virtio_user_ethdev.c          | 2 ++
>   3 files changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-stable] [PATCH 6/6] net/virtio: fix guest announce support

[Tiwei Bie]

We need to check the status field in virtio net config structure
instead of the bits read from ISR register to know whether we need
to do guest announce.

Fixes: 7365504f77e3 ("net/virtio: support guest announce")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 10a7e3fcc..27088e3a6 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1288,6 +1288,7 @@ virtio_interrupt_handler(void *param)
 	struct rte_eth_dev *dev = param;
 	struct virtio_hw *hw = dev->data->dev_private;
 	uint8_t isr;
+	uint16_t status;
 
 	/* Read interrupt status which clears interrupt */
 	isr = vtpci_isr(hw);
@@ -1301,12 +1302,17 @@ virtio_interrupt_handler(void *param)
 			_rte_eth_dev_callback_process(dev,
 						      RTE_ETH_EVENT_INTR_LSC,
 						      NULL);
-	}
 
-	if (isr & VIRTIO_NET_S_ANNOUNCE) {
-		virtio_notify_peers(dev);
-		if (hw->cvq)
-			virtio_ack_link_announce(dev);
+		if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
+			vtpci_read_dev_config(hw,
+				offsetof(struct virtio_net_config, status),
+				&status, sizeof(status));
+			if (status & VIRTIO_NET_S_ANNOUNCE) {
+				virtio_notify_peers(dev);
+				if (hw->cvq)
+					virtio_ack_link_announce(dev);
+			}
+		}
 	}
 }
 
-- 
2.19.1

Re: [dpdk-stable] [PATCH 6/6] net/virtio: fix guest announce support

[Maxime Coquelin]

On 10/29/18 6:28 AM, Tiwei Bie wrote:
> We need to check the status field in virtio net config structure
> instead of the bits read from ISR register to know whether we need
> to do guest announce.
> 
> Fixes: 7365504f77e3 ("net/virtio: support guest announce")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 16 +++++++++++-----
>   1 file changed, 11 insertions(+), 5 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime