[dpdk-dev] [PATCH v3 0/6] net/virtio-user: fix server mode

[dpdk-dev] [PATCH v3 0/6] net/virtio-user: fix server mode

[Adrian Moreno]

A number of issues have been detected that currently break virtio-user
server mode.
This series addresses such issues.
Note that virtio-user server mode is broken by design and many of the
problems that it currently has should be fixed by a bigger rework.

--
v2->v3:
Fix potential concurrency problem on get/set state
Handle STATUS protocol feature flag
Fix undefined behaviour if STATUS feature is not supported
Ensure packed virtqueues are reset on reconnection


v1->v2:
Added patch 2 and 3 addressing additional issues
Check errno to select vhost-user backend and log the detected backend
type

Adrian Moreno (6):
  net/virtio-user: fix backend selection if stat fails
  net/virtio-user: don't set/get_status until FEATURES_OK
  net/virtio-user: ignore result if STATUS is unsupported
  net/virtio-user: lock-protect status updates
  net/virtio-user: don't assume vhost status feature
  net/virtio-user: set status on socket reconnect

 drivers/net/virtio/virtio_user/vhost_user.c   | 14 ++---
 .../net/virtio/virtio_user/virtio_user_dev.c  | 53 +++++++++++++------
 .../net/virtio/virtio_user/virtio_user_dev.h  |  5 +-
 drivers/net/virtio/virtio_user_ethdev.c       | 40 ++++++++++----
 4 files changed, 78 insertions(+), 34 deletions(-)

-- 
2.26.2

[dpdk-dev] [PATCH v3 1/6] net/virtio-user: fix backend selection if stat fails

[Adrian Moreno]

If stat fails because the file does not exist, it means that
the backend must be vhost-user in server mode.

Also, log the detected backend type.

Bugzilla ID: 559
Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
Cc: stable@dpdk.org

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Acked-by: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 7 +++++++
 drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
 drivers/net/virtio/virtio_user_ethdev.c          | 6 +++++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 3681758ef..27814eadb 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -22,6 +22,13 @@
 
 #define VIRTIO_USER_MEM_EVENT_CLB_NAME "virtio_user_mem_event_clb"
 
+const char * const virtio_user_backend_strings[] = {
+	[VIRTIO_USER_BACKEND_UNKNOWN] = "VIRTIO_USER_BACKEND_UNKNOWN",
+	[VIRTIO_USER_BACKEND_VHOST_USER] = "VHOST_USER",
+	[VIRTIO_USER_BACKEND_VHOST_KERNEL] = "VHOST_NET",
+	[VIRTIO_USER_BACKEND_VHOST_VDPA] = "VHOST_VDPA",
+};
+
 static int
 virtio_user_create_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
 {
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 3e9d1a1eb..998986875 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -83,4 +83,5 @@ void virtio_user_handle_cq_packed(struct virtio_user_dev *dev,
 uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs);
 int virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status);
 int virtio_user_update_status(struct virtio_user_dev *dev);
+extern const char * const virtio_user_backend_strings[];
 #endif
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 042665bc0..e870fb2ff 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -560,6 +560,9 @@ virtio_user_backend_type(const char *path)
 	struct stat sb;
 
 	if (stat(path, &sb) == -1) {
+		if (errno == ENOENT)
+			return VIRTIO_USER_BACKEND_VHOST_USER;
+
 		PMD_INIT_LOG(ERR, "Stat fails: %s (%s)\n", path,
 			     strerror(errno));
 		return VIRTIO_USER_BACKEND_UNKNOWN;
@@ -697,7 +700,8 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
 			path);
 		goto end;
 	}
-
+	PMD_INIT_LOG(INFO, "Backend type detected: %s",
+		     virtio_user_backend_strings[backend_type]);
 
 	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_INTERFACE_NAME) == 1) {
 		if (backend_type != VIRTIO_USER_BACKEND_VHOST_KERNEL) {
-- 
2.26.2

[dpdk-dev] [PATCH v3 2/6] net/virtio-user: don't set/get_status until FEATURES_OK

[Adrian Moreno]

According to the virtio spec, ACK and DRIVER status bits should be set
before feature negotiation.

However, until the protocol features are negotiated, the driver does not
know if the device actually supports those vhost-user messages.
Therefore, until FEATURES_OK is set, the GET/SET_STATUS messages should
not be sent.

Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
Cc: maxime.coquelin@redhat.com
Cc: stable@dpdk.org

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user/vhost_user.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index ef290c357..450d77e92 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -278,8 +278,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
 
 	switch (req) {
 	case VHOST_USER_GET_STATUS:
-		if (!(dev->protocol_features &
-				(1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
+		if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
+		    (!(dev->protocol_features &
+				(1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
 			return 0;
 		/* Fallthrough */
 	case VHOST_USER_GET_FEATURES:
@@ -288,8 +289,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
 		break;
 
 	case VHOST_USER_SET_STATUS:
-		if (!(dev->protocol_features &
-				(1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
+		if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
+		    (!(dev->protocol_features &
+				(1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
 			return 0;
 
 		if (has_reply_ack)
-- 
2.26.2

[dpdk-dev] [PATCH v3 3/6] net/virtio-user: ignore result if STATUS is unsupported

[Adrian Moreno]

GET/SET STATUS is an optional feature, so it may not be negotiated. In
that case, the VIRTIO_GET_STATUS call will not update the status (given
as a pointer argument). Failing to identify this case would lead to
undefined behavior as the device status will be updated with the value
of a stack-allocated variable.

To fix this, return ENOTSUP if the feature is not supported and, in that
case, don't update device status.

Fixes: 44102e6298e7 ("net/virtio: check protocol feature in user backend")
Cc: maxime.coquelin@redhat.com
Cc stable@dpdk.org

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user/vhost_user.c   |  4 +--
 .../net/virtio/virtio_user/virtio_user_dev.c  | 28 +++++++++----------
 2 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index 450d77e92..b93e65c60 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -281,7 +281,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
 		if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
 		    (!(dev->protocol_features &
 				(1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
-			return 0;
+			return -ENOTSUP;
 		/* Fallthrough */
 	case VHOST_USER_GET_FEATURES:
 	case VHOST_USER_GET_PROTOCOL_FEATURES:
@@ -292,7 +292,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
 		if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
 		    (!(dev->protocol_features &
 				(1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
-			return 0;
+			return -ENOTSUP;
 
 		if (has_reply_ack)
 			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 27814eadb..5a1e76006 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -818,15 +818,13 @@ virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status)
 		ret = dev->ops->send_request(dev,
 				VHOST_USER_SET_STATUS, &status);
 	else
-		return 0;
+		ret = -ENOTSUP;
 
-	if (ret) {
+	if (ret && ret != -ENOTSUP) {
 		PMD_INIT_LOG(ERR, "VHOST_USER_SET_STATUS failed (%d): %s", ret,
 			     strerror(errno));
-		return -1;
 	}
-
-	return 0;
+	return ret;
 }
 
 int
@@ -849,17 +847,12 @@ virtio_user_update_status(struct virtio_user_dev *dev)
 		err = dev->ops->send_request(dev, VHOST_USER_GET_STATUS,
 				&status);
 	} else {
-		return 0;
-	}
-
-	if (err) {
-		PMD_INIT_LOG(ERR, "VHOST_USER_GET_STATUS failed (%d): %s", err,
-			     strerror(errno));
-		return -1;
+		err = -ENOTSUP;
 	}
 
-	dev->status = status;
-	PMD_INIT_LOG(DEBUG, "Updated Device Status(0x%08x):\n"
+	if (!err) {
+		dev->status = status;
+		PMD_INIT_LOG(DEBUG, "Updated Device Status(0x%08x):\n"
 			"\t-RESET: %u\n"
 			"\t-ACKNOWLEDGE: %u\n"
 			"\t-DRIVER: %u\n"
@@ -875,5 +868,10 @@ virtio_user_update_status(struct virtio_user_dev *dev)
 			!!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK),
 			!!(dev->status & VIRTIO_CONFIG_STATUS_DEV_NEED_RESET),
 			!!(dev->status & VIRTIO_CONFIG_STATUS_FAILED));
-	return 0;
+	} else if (err != -ENOTSUP) {
+		PMD_INIT_LOG(ERR, "VHOST_USER_GET_STATUS failed (%d): %s", err,
+			     strerror(errno));
+	}
+
+	return err;
 }
-- 
2.26.2

[dpdk-dev] [PATCH v3 4/6] net/virtio-user: lock-protect status updates

[Adrian Moreno]

In order to safely set and get the device status from different
threads (e.g: interrupt handlers).

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 14 +++++++++++---
 drivers/net/virtio/virtio_user/virtio_user_dev.h |  4 ++--
 drivers/net/virtio/virtio_user_ethdev.c          |  6 +++---
 3 files changed, 16 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 5a1e76006..36e5619df 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -806,11 +806,13 @@ virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx)
 }
 
 int
-virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status)
+virtio_user_dev_set_status(struct virtio_user_dev *dev, uint8_t status)
 {
 	int ret;
 	uint64_t arg = status;
 
+	pthread_mutex_lock(&dev->mutex);
+	dev->status = status;
 	if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER)
 		ret = dev->ops->send_request(dev,
 				VHOST_USER_SET_STATUS, &arg);
@@ -824,22 +826,26 @@ virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status)
 		PMD_INIT_LOG(ERR, "VHOST_USER_SET_STATUS failed (%d): %s", ret,
 			     strerror(errno));
 	}
+
+	pthread_mutex_unlock(&dev->mutex);
 	return ret;
 }
 
 int
-virtio_user_update_status(struct virtio_user_dev *dev)
+virtio_user_dev_update_status(struct virtio_user_dev *dev)
 {
 	uint64_t ret;
 	uint8_t status;
 	int err;
 
+	pthread_mutex_lock(&dev->mutex);
 	if (dev->backend_type == VIRTIO_USER_BACKEND_VHOST_USER) {
 		err = dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret);
 		if (!err && ret > UINT8_MAX) {
 			PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS "
 					"response 0x%" PRIx64 "\n", ret);
-			return -1;
+			err = -1;
+			goto error;
 		}
 
 		status = ret;
@@ -873,5 +879,7 @@ virtio_user_update_status(struct virtio_user_dev *dev)
 			     strerror(errno));
 	}
 
+error:
+	pthread_mutex_unlock(&dev->mutex);
 	return err;
 }
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 998986875..e053897d8 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -81,7 +81,7 @@ void virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx);
 void virtio_user_handle_cq_packed(struct virtio_user_dev *dev,
 				  uint16_t queue_idx);
 uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs);
-int virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status);
-int virtio_user_update_status(struct virtio_user_dev *dev);
+int virtio_user_dev_set_status(struct virtio_user_dev *dev, uint8_t status);
+int virtio_user_dev_update_status(struct virtio_user_dev *dev);
 extern const char * const virtio_user_backend_strings[];
 #endif
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index e870fb2ff..97ddc5651 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -278,8 +278,8 @@ virtio_user_set_status(struct virtio_hw *hw, uint8_t status)
 		virtio_user_start_device(dev);
 	else if (status == VIRTIO_CONFIG_STATUS_RESET)
 		virtio_user_reset(hw);
-	dev->status = status;
-	virtio_user_send_status_update(dev, status);
+
+	virtio_user_dev_set_status(dev, status);
 }
 
 static uint8_t
@@ -287,7 +287,7 @@ virtio_user_get_status(struct virtio_hw *hw)
 {
 	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
 
-	virtio_user_update_status(dev);
+	virtio_user_dev_update_status(dev);
 
 	return dev->status;
 }
-- 
2.26.2

[dpdk-dev] [PATCH v3 5/6] net/virtio-user: don't assume vhost status feature

[Adrian Moreno]

There are some status reads and updates that need to happen before the
protocol features are negotiated. Therefore, assuming the backend does
support this feature can lead to failures.

On server mode, do not assume the backend supports
VHOST_USER_PROTOCOL_F_STATUS. Activate it back on reconnection and
cleare it on disconnection.

Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
Cc: maxime.coquelin@redhat.com
Cc: stable@dpdk.org

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 ++++++
 drivers/net/virtio/virtio_user_ethdev.c          | 8 ++++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 36e5619df..053f0267c 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -523,6 +523,12 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		 * later.
 		 */
 		dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
+
+		/* We cannot assume VHOST_USER_PROTOCOL_F_STATUS is supported
+		 * until it's negotiated
+		 */
+		dev->protocol_features &=
+			~(1ULL << VHOST_USER_PROTOCOL_F_STATUS);
 	}
 
 
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 97ddc5651..142bdc0bd 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -92,6 +92,9 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 					&protocol_features))
 			return -1;
 
+		/* Offer VHOST_USER_PROTOCOL_F_STATUS */
+		dev->protocol_features |=
+			(1ULL << VHOST_USER_PROTOCOL_F_STATUS);
 		dev->protocol_features &= protocol_features;
 
 		if (dev->ops->send_request(dev,
@@ -168,6 +171,11 @@ virtio_user_delayed_handler(void *param)
 		if (dev->vhostfd >= 0) {
 			close(dev->vhostfd);
 			dev->vhostfd = -1;
+			/* Until the featuers are negotiated again, don't assume
+			 * the backend supports VHOST_USER_PROTOCOL_F_STATUS
+			 */
+			dev->protocol_features &=
+				~(1ULL << VHOST_USER_PROTOCOL_F_STATUS);
 		}
 		eth_dev->intr_handle->fd = dev->listenfd;
 		rte_intr_callback_register(eth_dev->intr_handle,
-- 
2.26.2

[dpdk-dev] [PATCH v3 6/6] net/virtio-user: set status on socket reconnect

[Adrian Moreno]

Newer vhost-user backends will rely on SET_STATUS to start the device
so this required to support them.

Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
Cc: maxime.coquelin@redhat.com
Cc: stable@dpdk.org

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 142bdc0bd..40345193e 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -67,8 +67,7 @@ virtio_user_reset_queues_packed(struct rte_eth_dev *dev)
 static int
 virtio_user_server_reconnect(struct virtio_user_dev *dev)
 {
-	int ret;
-	int connectfd;
+	int ret, connectfd, old_status;
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];
 	struct virtio_hw *hw = eth_dev->data->dev_private;
 	uint64_t protocol_features;
@@ -78,6 +77,14 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 		return -1;
 
 	dev->vhostfd = connectfd;
+	old_status = vtpci_get_status(hw);
+
+	vtpci_reset(hw);
+
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
+
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
+
 	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
 				   &dev->device_features) < 0) {
 		PMD_INIT_LOG(ERR, "get_features failed: %s",
@@ -116,14 +123,17 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 
 	/* For packed ring, resetting queues is required in reconnection. */
 	if (vtpci_packed_queue(hw) &&
-	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
+	   (old_status & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
 		PMD_INIT_LOG(NOTICE, "Packets on the fly will be dropped"
 				" when packed ring reconnecting.");
 		virtio_user_reset_queues_packed(eth_dev);
 	}
 
-	ret = virtio_user_start_device(dev);
-	if (ret < 0)
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
+
+	/* Start the device */
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
+	if (!dev->started)
 		return -1;
 
 	if (dev->queue_pairs > 1) {
-- 
2.26.2

Re: [dpdk-dev] [PATCH v3 0/6] net/virtio-user: fix server mode

[Jiang, YuX]

Tested-by: JiangYuX <yux.jiang@intel.com>

    Best Regards
    Jiang yu


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Adrian Moreno
> Sent: Tuesday, October 27, 2020 12:39 AM
> To: dev@dpdk.org
> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick
> <patrick.fu@intel.com>; Xia, Chenbo <chenbo.xia@intel.com>; Wang,
> Zhihong <zhihong.wang@intel.com>; maxime.coquelin@redhat.com; Adrian
> Moreno <amorenoz@redhat.com>
> Subject: [dpdk-dev] [PATCH v3 0/6] net/virtio-user: fix server mode
> 
> A number of issues have been detected that currently break virtio-user
> server mode.
> This series addresses such issues.
> Note that virtio-user server mode is broken by design and many of the
> problems that it currently has should be fixed by a bigger rework.
> 
> --
> v2->v3:
> Fix potential concurrency problem on get/set state Handle STATUS protocol
> feature flag Fix undefined behaviour if STATUS feature is not supported
> Ensure packed virtqueues are reset on reconnection
> 
> 
> v1->v2:
> Added patch 2 and 3 addressing additional issues Check errno to select vhost-
> user backend and log the detected backend type
> 
> Adrian Moreno (6):
>   net/virtio-user: fix backend selection if stat fails
>   net/virtio-user: don't set/get_status until FEATURES_OK
>   net/virtio-user: ignore result if STATUS is unsupported
>   net/virtio-user: lock-protect status updates
>   net/virtio-user: don't assume vhost status feature
>   net/virtio-user: set status on socket reconnect
> 
>  drivers/net/virtio/virtio_user/vhost_user.c   | 14 ++---
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 53 +++++++++++++------
>   .../net/virtio/virtio_user/virtio_user_dev.h  |  5 +-
>  drivers/net/virtio/virtio_user_ethdev.c       | 40 ++++++++++----
>  4 files changed, 78 insertions(+), 34 deletions(-)
> 
> --
> 2.26.2

Re: [dpdk-dev] [PATCH v3 3/6] net/virtio-user: ignore result if STATUS is unsupported

[Maxime Coquelin]

On 10/26/20 5:39 PM, Adrian Moreno wrote:
> GET/SET STATUS is an optional feature, so it may not be negotiated. In
> that case, the VIRTIO_GET_STATUS call will not update the status (given
> as a pointer argument). Failing to identify this case would lead to
> undefined behavior as the device status will be updated with the value
> of a stack-allocated variable.
> 
> To fix this, return ENOTSUP if the feature is not supported and, in that
> case, don't update device status.
> 
> Fixes: 44102e6298e7 ("net/virtio: check protocol feature in user backend")
> Cc: maxime.coquelin@redhat.com
> Cc stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/vhost_user.c   |  4 +--
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 28 +++++++++----------
>  2 files changed, 15 insertions(+), 17 deletions(-)
> 

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

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v3 4/6] net/virtio-user: lock-protect status updates

[Maxime Coquelin]

On 10/26/20 5:39 PM, Adrian Moreno wrote:
> In order to safely set and get the device status from different
> threads (e.g: interrupt handlers).
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 14 +++++++++++---
>  drivers/net/virtio/virtio_user/virtio_user_dev.h |  4 ++--
>  drivers/net/virtio/virtio_user_ethdev.c          |  6 +++---
>  3 files changed, 16 insertions(+), 8 deletions(-)

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

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v3 5/6] net/virtio-user: don't assume vhost status feature

[Maxime Coquelin]

On 10/26/20 5:39 PM, Adrian Moreno wrote:
> There are some status reads and updates that need to happen before the
> protocol features are negotiated. Therefore, assuming the backend does
> support this feature can lead to failures.
> 
> On server mode, do not assume the backend supports
> VHOST_USER_PROTOCOL_F_STATUS. Activate it back on reconnection and
> cleare it on disconnection.
> 
> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
> Cc: maxime.coquelin@redhat.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 ++++++
>  drivers/net/virtio/virtio_user_ethdev.c          | 8 ++++++++
>  2 files changed, 14 insertions(+)
> 


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

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v3 6/6] net/virtio-user: set status on socket reconnect

[Maxime Coquelin]

On 10/26/20 5:39 PM, Adrian Moreno wrote:
> Newer vhost-user backends will rely on SET_STATUS to start the device
> so this required to support them.
> 
> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
> Cc: maxime.coquelin@redhat.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user_ethdev.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 

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

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v3 0/6] net/virtio-user: fix server mode

[Maxime Coquelin]

On 10/26/20 5:39 PM, Adrian Moreno wrote:
> A number of issues have been detected that currently break virtio-user
> server mode.
> This series addresses such issues.
> Note that virtio-user server mode is broken by design and many of the
> problems that it currently has should be fixed by a bigger rework.
> 
> --
> v2->v3:
> Fix potential concurrency problem on get/set state
> Handle STATUS protocol feature flag
> Fix undefined behaviour if STATUS feature is not supported
> Ensure packed virtqueues are reset on reconnection
> 
> 
> v1->v2:
> Added patch 2 and 3 addressing additional issues
> Check errno to select vhost-user backend and log the detected backend
> type
> 
> Adrian Moreno (6):
>   net/virtio-user: fix backend selection if stat fails
>   net/virtio-user: don't set/get_status until FEATURES_OK
>   net/virtio-user: ignore result if STATUS is unsupported
>   net/virtio-user: lock-protect status updates
>   net/virtio-user: don't assume vhost status feature
>   net/virtio-user: set status on socket reconnect
> 
>  drivers/net/virtio/virtio_user/vhost_user.c   | 14 ++---
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 53 +++++++++++++------
>  .../net/virtio/virtio_user/virtio_user_dev.h  |  5 +-
>  drivers/net/virtio/virtio_user_ethdev.c       | 40 ++++++++++----
>  4 files changed, 78 insertions(+), 34 deletions(-)
> 

Applied to dpdk-next-virtio/main.

Thanks!
Maxime