DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init
@ 2020-07-02  8:32 Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 1/8] vhost: fix virtio ready flag check Adrian Moreno
                   ` (7 more replies)
  0 siblings, 8 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-02  8:32 UTC (permalink / raw)
  To: dev, xiaolong.ye, shahafs, matan, maxime.coquelin, xiao.w.wang,
	viacheslavo
  Cc: jasowang, lulu, Adrian Moreno

The goal of this series is to make the Vhost/vDPA device init more
robust by adding support to a new protocol feature and two new messages.

VHOST_USER_SET_STATUS is received by the backend when the driver updates
the virtio device status register.

For now this series only deals with the DRIVER_OK bit, which indicates
that the driver is done with the device initialization. So, if the
feature is negotiated, make it a requirement to consider the device
ready.

For example, such information helps the Vhost backend to know when it
can call the dev_conf vDPA callback.

VIRTIO_USER_GET_STATUS is received by the backend to report the status
of the virtio device as per the virtio specification.

One use of this message is for the frontend to read the status after
the feature negotiation. If the device has cleared the FEATURE_OK bit,
the driver should interpret that the device has rejected the features
and should fail accordingly.

Before adding support for these new requests, some clean-ups and
refactoring are done as preliminary steps to make the code more easily
readable. Some vDPA APIs are also made mandatory.

Note that the VHOST_USER_PROTOCOL_F_STATUS protocol feature requires
frontend support. For Qemu it has been posted [1] , reviewed and acked.
However, it's still waiting for the nex merge windows, so this series
should not be merged until the former is.

Posting it earler so it can be reviewed, specially after the recent
series developed by Matan which also affects the vhost ready status.

[1] https://patchwork.ozlabs.org/project/qemu-devel/patch/20200514073332.1434576-1-maxime.coquelin@redhat.com/

---
Changes from v1:
- Rebased on top of virtio-next:
 - Dropped [PATCH 9/9] vhost: only use vDPA config workaround if needed
 since the latest changes have made the workaround uncessary.
 - Dropped [PATCH 3/9] vdpa/ifc: add support to vDPA queue enable
 Callbacks are now expected to reconfigure the file descriptors.
- Updated the message ids as per latest version of qemu spec:
- Added VHOST_USER_GET_STATUS support
- [Chenbo Xia] Add validate_msg_fds to set_staus message handler


Adrian Moreno (1):
  vhost: add support for virtio get status message

Maxime Coquelin (7):
  vhost: fix virtio ready flag check
  vhost: refactor Virtio ready check
  vhost: make some vDPA callbacks mandatory
  vhost: check vDPA configuration succeed
  vhost: add support for virtio status
  vdpa/ifc: enable status protocol feature
  vdpa/mlx5: enable status protocol feature

 drivers/vdpa/ifc/ifcvf_vdpa.c   |   3 +-
 drivers/vdpa/mlx5/mlx5_vdpa.c   |   3 +-
 lib/librte_vhost/rte_vdpa_dev.h |  14 ++--
 lib/librte_vhost/rte_vhost.h    |   4 ++
 lib/librte_vhost/socket.c       |   6 +-
 lib/librte_vhost/vdpa.c         |  10 +++
 lib/librte_vhost/vhost.c        |   3 +-
 lib/librte_vhost/vhost.h        |  11 +++
 lib/librte_vhost/vhost_user.c   | 117 +++++++++++++++++++++++++++-----
 lib/librte_vhost/vhost_user.h   |   7 +-
 10 files changed, 148 insertions(+), 30 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2 1/8] vhost: fix virtio ready flag check
  2020-07-02  8:32 [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init Adrian Moreno
@ 2020-07-02  8:32 ` Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 2/8] vhost: refactor Virtio ready check Adrian Moreno
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-02  8:32 UTC (permalink / raw)
  To: dev, xiaolong.ye, shahafs, matan, maxime.coquelin, xiao.w.wang,
	viacheslavo
  Cc: jasowang, lulu, stable, Adrian Moreno

From: Maxime Coquelin <maxime.coquelin@redhat.com>

Before checking whether the device is ready is done
a check on whether the RUNNING flag is set. Then the
READY flag is set if virtio_is_ready() returns true.

While it seems to not cause any issue, it makes more
sense to check whether the READY flag is set and not
the RUNNING one.

Fixes: c0674b1bc898 ("vhost: move the device ready check at proper place")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 6039a8fdb..5750dde6d 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -2825,7 +2825,7 @@ vhost_user_msg_handler(int vid, int fd)
 	}
 
 
-	if (!(dev->flags & VIRTIO_DEV_RUNNING) && virtio_is_ready(dev)) {
+	if (!(dev->flags & VIRTIO_DEV_READY) && virtio_is_ready(dev)) {
 		dev->flags |= VIRTIO_DEV_READY;
 
 		if (!(dev->flags & VIRTIO_DEV_RUNNING)) {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2 2/8] vhost: refactor Virtio ready check
  2020-07-02  8:32 [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 1/8] vhost: fix virtio ready flag check Adrian Moreno
@ 2020-07-02  8:32 ` Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 3/8] vhost: make some vDPA callbacks mandatory Adrian Moreno
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-02  8:32 UTC (permalink / raw)
  To: dev, xiaolong.ye, shahafs, matan, maxime.coquelin, xiao.w.wang,
	viacheslavo
  Cc: jasowang, lulu, Adrian Moreno

From: Maxime Coquelin <maxime.coquelin@redhat.com>

This patch is a small refactoring, as preliminary work
for adding support to Virtio status support.

No functionnal change here.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 lib/librte_vhost/vhost.c      |  1 +
 lib/librte_vhost/vhost_user.c | 33 +++++++++++++++++++++------------
 2 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 0d822d6a3..aa1424261 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -710,6 +710,7 @@ vhost_enable_dequeue_zero_copy(int vid)
 		return;
 
 	dev->dequeue_zero_copy = 1;
+	VHOST_LOG_CONFIG(INFO, "dequeue zero copy is enabled\n");
 }
 
 void
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 5750dde6d..ff8b1752b 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1337,6 +1337,9 @@ virtio_is_ready(struct virtio_net *dev)
 	struct vhost_virtqueue *vq;
 	uint32_t i;
 
+	if (dev->flags & VIRTIO_DEV_READY)
+		return 1;
+
 	if (dev->nr_vring < VIRTIO_DEV_NUM_VQS_TO_BE_READY)
 		return 0;
 
@@ -1347,6 +1350,8 @@ virtio_is_ready(struct virtio_net *dev)
 			return 0;
 	}
 
+	dev->flags |= VIRTIO_DEV_READY;
+
 	if (!(dev->flags & VIRTIO_DEV_RUNNING))
 		VHOST_LOG_CONFIG(INFO,
 			"virtio is now ready for processing.\n");
@@ -2825,28 +2830,32 @@ vhost_user_msg_handler(int vid, int fd)
 	}
 
 
-	if (!(dev->flags & VIRTIO_DEV_READY) && virtio_is_ready(dev)) {
-		dev->flags |= VIRTIO_DEV_READY;
+	if (!virtio_is_ready(dev))
+		goto out;
 
-		if (!(dev->flags & VIRTIO_DEV_RUNNING)) {
-			if (dev->dequeue_zero_copy) {
-				VHOST_LOG_CONFIG(INFO,
-						"dequeue zero copy is enabled\n");
-			}
+	/*
+	 * Virtio is now ready. If not done already, it is time
+	 * to notify the application it can process the rings and
+	 * configure the vDPA device if present.
+	 */
 
-			if (dev->notify_ops->new_device(dev->vid) == 0)
-				dev->flags |= VIRTIO_DEV_RUNNING;
-		}
+	if (!(dev->flags & VIRTIO_DEV_RUNNING)) {
+		if (dev->notify_ops->new_device(dev->vid) == 0)
+			dev->flags |= VIRTIO_DEV_RUNNING;
 	}
 
 	vdpa_dev = dev->vdpa_dev;
-	if (vdpa_dev && virtio_is_ready(dev) &&
-	    !(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
+	if (!vdpa_dev)
+		goto out;
+
+	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
 		if (vdpa_dev->ops->dev_conf)
 			vdpa_dev->ops->dev_conf(dev->vid);
+
 		dev->flags |= VIRTIO_DEV_VDPA_CONFIGURED;
 	}
 
+out:
 	return 0;
 }
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2 3/8] vhost: make some vDPA callbacks mandatory
  2020-07-02  8:32 [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 1/8] vhost: fix virtio ready flag check Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 2/8] vhost: refactor Virtio ready check Adrian Moreno
@ 2020-07-02  8:32 ` Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 4/8] vhost: check vDPA configuration succeed Adrian Moreno
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-02  8:32 UTC (permalink / raw)
  To: dev, xiaolong.ye, shahafs, matan, maxime.coquelin, xiao.w.wang,
	viacheslavo
  Cc: jasowang, lulu, Adrian Moreno

From: Maxime Coquelin <maxime.coquelin@redhat.com>

Some of the vDPA callbacks have to be implemented
for vDPA to work properly.

This patch marks them as mandatory in the API doc and
simplify code calling these ops with removing
unnecessary checks that are now done at registration
time.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 lib/librte_vhost/rte_vdpa_dev.h | 14 ++++++++------
 lib/librte_vhost/socket.c       |  6 +++---
 lib/librte_vhost/vdpa.c         | 10 ++++++++++
 lib/librte_vhost/vhost.c        |  2 +-
 lib/librte_vhost/vhost_user.c   |  6 ++----
 5 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/lib/librte_vhost/rte_vdpa_dev.h b/lib/librte_vhost/rte_vdpa_dev.h
index 65557cb05..89444c2ea 100644
--- a/lib/librte_vhost/rte_vdpa_dev.h
+++ b/lib/librte_vhost/rte_vdpa_dev.h
@@ -15,24 +15,26 @@
  * vdpa device operations
  */
 struct rte_vdpa_dev_ops {
-	/** Get capabilities of this device */
+	/** Get capabilities of this device (Mandatory) */
 	int (*get_queue_num)(struct rte_vdpa_device *dev, uint32_t *queue_num);
 
-	/** Get supported features of this device */
+	/** Get supported features of this device (Mandatory) */
 	int (*get_features)(struct rte_vdpa_device *dev, uint64_t *features);
 
-	/** Get supported protocol features of this device */
+	/** Get supported protocol features of this device (Mandatory) */
 	int (*get_protocol_features)(struct rte_vdpa_device *dev,
 			uint64_t *protocol_features);
 
-	/** Driver configure/close the device */
+	/** Driver configure the device (Mandatory) */
 	int (*dev_conf)(int vid);
+
+	/** Driver close the device (Mandatory) */
 	int (*dev_close)(int vid);
 
-	/** Enable/disable this vring */
+	/** Enable/disable this vring (Mandatory) */
 	int (*set_vring_state)(int vid, int vring, int state);
 
-	/** Set features when changed */
+	/** Set features when changed (Mandatory) */
 	int (*set_features)(int vid);
 
 	/** Destination operations when migration done */
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 49267cebf..047d9395d 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -701,7 +701,7 @@ rte_vhost_driver_get_features(const char *path, uint64_t *features)
 	}
 
 	vdpa_dev = vsocket->vdpa_dev;
-	if (!vdpa_dev || !vdpa_dev->ops->get_features) {
+	if (!vdpa_dev) {
 		*features = vsocket->features;
 		goto unlock_exit;
 	}
@@ -754,7 +754,7 @@ rte_vhost_driver_get_protocol_features(const char *path,
 	}
 
 	vdpa_dev = vsocket->vdpa_dev;
-	if (!vdpa_dev || !vdpa_dev->ops->get_protocol_features) {
+	if (!vdpa_dev) {
 		*protocol_features = vsocket->protocol_features;
 		goto unlock_exit;
 	}
@@ -794,7 +794,7 @@ rte_vhost_driver_get_queue_num(const char *path, uint32_t *queue_num)
 	}
 
 	vdpa_dev = vsocket->vdpa_dev;
-	if (!vdpa_dev || !vdpa_dev->ops->get_queue_num) {
+	if (!vdpa_dev) {
 		*queue_num = VHOST_MAX_QUEUE_PAIRS;
 		goto unlock_exit;
 	}
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index ef21ca2a1..ae6fdd24e 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -77,6 +77,16 @@ rte_vdpa_register_device(struct rte_device *rte_dev,
 	if (ops == NULL)
 		return NULL;
 
+	/* Check mandatory ops are implemented */
+	if (!ops->get_queue_num || !ops->get_features ||
+			!ops->get_protocol_features || !ops->dev_conf ||
+			!ops->dev_close || !ops->set_vring_state ||
+			!ops->set_features) {
+		VHOST_LOG_CONFIG(ERR,
+				"Some mandatory vDPA ops aren't implemented\n");
+		return NULL;
+	}
+
 	rte_spinlock_lock(&vdpa_device_list_lock);
 	/* Check the device hasn't been register already */
 	dev = __vdpa_find_device_by_name(rte_dev->name);
diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index aa1424261..da461e843 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -646,7 +646,7 @@ vhost_destroy_device_notify(struct virtio_net *dev)
 
 	if (dev->flags & VIRTIO_DEV_RUNNING) {
 		vdpa_dev = dev->vdpa_dev;
-		if (vdpa_dev && vdpa_dev->ops->dev_close)
+		if (vdpa_dev)
 			vdpa_dev->ops->dev_close(dev->vid);
 		dev->flags &= ~VIRTIO_DEV_RUNNING;
 		dev->notify_ops->destroy_device(dev->vid);
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index ff8b1752b..6be5c771b 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -398,7 +398,7 @@ vhost_user_set_features(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	}
 
 	vdpa_dev = dev->vdpa_dev;
-	if (vdpa_dev && vdpa_dev->ops->set_features)
+	if (vdpa_dev)
 		vdpa_dev->ops->set_features(dev->vid);
 
 	return RTE_VHOST_MSG_RESULT_OK;
@@ -2849,9 +2849,7 @@ vhost_user_msg_handler(int vid, int fd)
 		goto out;
 
 	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
-		if (vdpa_dev->ops->dev_conf)
-			vdpa_dev->ops->dev_conf(dev->vid);
-
+		vdpa_dev->ops->dev_conf(dev->vid);
 		dev->flags |= VIRTIO_DEV_VDPA_CONFIGURED;
 	}
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2 4/8] vhost: check vDPA configuration succeed
  2020-07-02  8:32 [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init Adrian Moreno
                   ` (2 preceding siblings ...)
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 3/8] vhost: make some vDPA callbacks mandatory Adrian Moreno
@ 2020-07-02  8:32 ` Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 5/8] vhost: add support for virtio status Adrian Moreno
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-02  8:32 UTC (permalink / raw)
  To: dev, xiaolong.ye, shahafs, matan, maxime.coquelin, xiao.w.wang,
	viacheslavo
  Cc: jasowang, lulu, Adrian Moreno

From: Maxime Coquelin <maxime.coquelin@redhat.com>

This patch checks whether vDPA device configuration
succeed and does not set the CONFIGURED flag if it
didn't.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 6be5c771b..bf079c914 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -2849,8 +2849,11 @@ vhost_user_msg_handler(int vid, int fd)
 		goto out;
 
 	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
-		vdpa_dev->ops->dev_conf(dev->vid);
-		dev->flags |= VIRTIO_DEV_VDPA_CONFIGURED;
+		if (vdpa_dev->ops->dev_conf(dev->vid))
+			VHOST_LOG_CONFIG(ERR,
+					 "Failed to configure vDPA device\n")
+		else
+			dev->flags |= VIRTIO_DEV_VDPA_CONFIGURED;
 	}
 
 out:
-- 
2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2 5/8] vhost: add support for virtio status
  2020-07-02  8:32 [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init Adrian Moreno
                   ` (3 preceding siblings ...)
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 4/8] vhost: check vDPA configuration succeed Adrian Moreno
@ 2020-07-02  8:32 ` Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message Adrian Moreno
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-02  8:32 UTC (permalink / raw)
  To: dev, xiaolong.ye, shahafs, matan, maxime.coquelin, xiao.w.wang,
	viacheslavo
  Cc: jasowang, lulu, Adrian Moreno

From: Maxime Coquelin <maxime.coquelin@redhat.com>

This patch adds support to the new Virtio device status
Vhost-user protocol feature.

Getting such information in the backend helps to know
when the driver is done with the device configuration
and so makes the initialization phase more robust.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 lib/librte_vhost/rte_vhost.h  |  4 ++++
 lib/librte_vhost/vhost.h      |  9 +++++++
 lib/librte_vhost/vhost_user.c | 45 ++++++++++++++++++++++++++++++++++-
 lib/librte_vhost/vhost_user.h |  6 +++--
 4 files changed, 61 insertions(+), 3 deletions(-)

diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index 8a5c332c8..104e2e869 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -102,6 +102,10 @@ extern "C" {
 #define VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD 12
 #endif
 
+#ifndef VHOST_USER_PROTOCOL_F_STATUS
+#define VHOST_USER_PROTOCOL_F_STATUS 16
+#endif
+
 /** Indicate whether protocol features negotiation is supported. */
 #ifndef VHOST_USER_F_PROTOCOL_FEATURES
 #define VHOST_USER_F_PROTOCOL_FEATURES	30
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 034463699..25d31c71b 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -204,6 +204,14 @@ struct vhost_virtqueue {
 	TAILQ_HEAD(, vhost_iotlb_entry) iotlb_pending_list;
 } __rte_cache_aligned;
 
+/* Virtio device status as per Virtio specification */
+#define VIRTIO_DEVICE_STATUS_ACK		0x01
+#define VIRTIO_DEVICE_STATUS_DRIVER		0x02
+#define VIRTIO_DEVICE_STATUS_DRIVER_OK		0x04
+#define VIRTIO_DEVICE_STATUS_FEATURES_OK	0x08
+#define VIRTIO_DEVICE_STATUS_DEV_NEED_RESET	0x40
+#define VIRTIO_DEVICE_STATUS_FAILED		0x80
+
 #define VHOST_MAX_VRING			0x100
 #define VHOST_MAX_QUEUE_PAIRS		0x80
 
@@ -349,6 +357,7 @@ struct virtio_net {
 	uint64_t		log_addr;
 	struct rte_ether_addr	mac;
 	uint16_t		mtu;
+	uint8_t			status;
 
 	struct vhost_device_ops const *notify_ops;
 
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index bf079c914..8d3d13913 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -87,6 +87,7 @@ static const char *vhost_message_str[VHOST_USER_MAX] = {
 	[VHOST_USER_POSTCOPY_END]  = "VHOST_USER_POSTCOPY_END",
 	[VHOST_USER_GET_INFLIGHT_FD] = "VHOST_USER_GET_INFLIGHT_FD",
 	[VHOST_USER_SET_INFLIGHT_FD] = "VHOST_USER_SET_INFLIGHT_FD",
+	[VHOST_USER_SET_STATUS] = "VHOST_USER_SET_STATUS",
 };
 
 static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg);
@@ -1350,6 +1351,11 @@ virtio_is_ready(struct virtio_net *dev)
 			return 0;
 	}
 
+	/* If supported, ensure the frontend is really done with config */
+	if (dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_STATUS))
+		if (!(dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK))
+			return 0;
+
 	dev->flags |= VIRTIO_DEV_READY;
 
 	if (!(dev->flags & VIRTIO_DEV_RUNNING))
@@ -2452,6 +2458,42 @@ vhost_user_postcopy_end(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	return RTE_VHOST_MSG_RESULT_REPLY;
 }
 
+static int
+vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
+			int main_fd __rte_unused)
+{
+	struct virtio_net *dev = *pdev;
+
+	if (validate_msg_fds(msg, 0) != 0)
+		return RTE_VHOST_MSG_RESULT_ERR;
+
+	/* As per Virtio specification, the device status is 8bits long */
+	if (msg->payload.u64 > UINT8_MAX) {
+		VHOST_LOG_CONFIG(ERR, "Invalid VHOST_USER_SET_STATUS payload 0x%" PRIx64 "\n",
+				msg->payload.u64);
+		return RTE_VHOST_MSG_RESULT_ERR;
+	}
+
+	dev->status = msg->payload.u64;
+
+	VHOST_LOG_CONFIG(INFO, "New device status(0x%08x):\n"
+			"\t-ACKNOWLEDGE: %u\n"
+			"\t-DRIVER: %u\n"
+			"\t-FEATURES_OK: %u\n"
+			"\t-DRIVER_OK: %u\n"
+			"\t-DEVICE_NEED_RESET: %u\n"
+			"\t-FAILED: %u\n",
+			dev->status,
+			!!(dev->status & VIRTIO_DEVICE_STATUS_ACK),
+			!!(dev->status & VIRTIO_DEVICE_STATUS_DRIVER),
+			!!(dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK),
+			!!(dev->status & VIRTIO_DEVICE_STATUS_DRIVER_OK),
+			!!(dev->status & VIRTIO_DEVICE_STATUS_DEV_NEED_RESET),
+			!!(dev->status & VIRTIO_DEVICE_STATUS_FAILED));
+
+	return RTE_VHOST_MSG_RESULT_OK;
+}
+
 typedef int (*vhost_message_handler_t)(struct virtio_net **pdev,
 					struct VhostUserMsg *msg,
 					int main_fd);
@@ -2484,6 +2526,7 @@ static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
 	[VHOST_USER_POSTCOPY_END] = vhost_user_postcopy_end,
 	[VHOST_USER_GET_INFLIGHT_FD] = vhost_user_get_inflight_fd,
 	[VHOST_USER_SET_INFLIGHT_FD] = vhost_user_set_inflight_fd,
+	[VHOST_USER_SET_STATUS] = vhost_user_set_status,
 };
 
 /* return bytes# of read on success or negative val on failure. */
@@ -2851,7 +2894,7 @@ vhost_user_msg_handler(int vid, int fd)
 	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
 		if (vdpa_dev->ops->dev_conf(dev->vid))
 			VHOST_LOG_CONFIG(ERR,
-					 "Failed to configure vDPA device\n")
+					 "Failed to configure vDPA device\n");
 		else
 			dev->flags |= VIRTIO_DEV_VDPA_CONFIGURED;
 	}
diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
index 1f65efa4a..82885ab5e 100644
--- a/lib/librte_vhost/vhost_user.h
+++ b/lib/librte_vhost/vhost_user.h
@@ -23,7 +23,8 @@
 					 (1ULL << VHOST_USER_PROTOCOL_F_CRYPTO_SESSION) | \
 					 (1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD) | \
 					 (1ULL << VHOST_USER_PROTOCOL_F_HOST_NOTIFIER) | \
-					 (1ULL << VHOST_USER_PROTOCOL_F_PAGEFAULT))
+					 (1ULL << VHOST_USER_PROTOCOL_F_PAGEFAULT) | \
+					 (1ULL << VHOST_USER_PROTOCOL_F_STATUS))
 
 typedef enum VhostUserRequest {
 	VHOST_USER_NONE = 0,
@@ -56,7 +57,8 @@ typedef enum VhostUserRequest {
 	VHOST_USER_POSTCOPY_END = 30,
 	VHOST_USER_GET_INFLIGHT_FD = 31,
 	VHOST_USER_SET_INFLIGHT_FD = 32,
-	VHOST_USER_MAX = 33
+	VHOST_USER_SET_STATUS = 39,
+	VHOST_USER_MAX = 41
 } VhostUserRequest;
 
 typedef enum VhostUserSlaveRequest {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message
  2020-07-02  8:32 [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init Adrian Moreno
                   ` (4 preceding siblings ...)
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 5/8] vhost: add support for virtio status Adrian Moreno
@ 2020-07-02  8:32 ` Adrian Moreno
  2020-07-05 13:15   ` Xia, Chenbo
  2020-07-06  3:22   ` Xia, Chenbo
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 7/8] vdpa/ifc: enable status protocol feature Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 8/8] vdpa/mlx5: " Adrian Moreno
  7 siblings, 2 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-02  8:32 UTC (permalink / raw)
  To: dev, xiaolong.ye, shahafs, matan, maxime.coquelin, xiao.w.wang,
	viacheslavo
  Cc: jasowang, lulu, Adrian Moreno

This patch adds support to the new Virtio device get status
Vhost-user message.

The driver can send this new message to read the device status.

One of the uses of this message is to ensure the feature negotiation has
succeded.  According to the virtio spec, after completing the feature
negotiation, the driver sets the FEATURE_OK status bit and re-reads it
to ensure the device has accepted the features.

This patch also clears the FEATURE_OK status bit if the feature
negotiation has failed to let the driver know about his failure.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 lib/librte_vhost/vhost.h      |  2 ++
 lib/librte_vhost/vhost_user.c | 32 ++++++++++++++++++++++++++++++++
 lib/librte_vhost/vhost_user.h |  1 +
 3 files changed, 35 insertions(+)

diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index 25d31c71b..e743821cc 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -32,6 +32,8 @@
 #define VIRTIO_DEV_BUILTIN_VIRTIO_NET 4
 /* Used to indicate that the device has its own data path and configured */
 #define VIRTIO_DEV_VDPA_CONFIGURED 8
+/* Used to indicate that the feature negotiation failed */
+#define VIRTIO_DEV_FEATURES_FAILED 16
 
 /* Backend value set by guest. */
 #define VIRTIO_DEV_STOPPED -1
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 8d3d13913..00da7bf18 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -88,6 +88,7 @@ static const char *vhost_message_str[VHOST_USER_MAX] = {
 	[VHOST_USER_GET_INFLIGHT_FD] = "VHOST_USER_GET_INFLIGHT_FD",
 	[VHOST_USER_SET_INFLIGHT_FD] = "VHOST_USER_SET_INFLIGHT_FD",
 	[VHOST_USER_SET_STATUS] = "VHOST_USER_SET_STATUS",
+	[VHOST_USER_GET_STATUS] = "VHOST_USER_GET_STATUS",
 };
 
 static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg);
@@ -339,6 +340,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct VhostUserMsg *msg,
 		VHOST_LOG_CONFIG(ERR,
 			"(%d) received invalid negotiated features.\n",
 			dev->vid);
+		dev->flags |= VIRTIO_DEV_FEATURES_FAILED;
+		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
+
 		return RTE_VHOST_MSG_RESULT_ERR;
 	}
 
@@ -402,6 +406,7 @@ vhost_user_set_features(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	if (vdpa_dev)
 		vdpa_dev->ops->set_features(dev->vid);
 
+	dev->flags &= ~VIRTIO_DEV_FEATURES_FAILED;
 	return RTE_VHOST_MSG_RESULT_OK;
 }
 
@@ -2458,6 +2463,22 @@ vhost_user_postcopy_end(struct virtio_net **pdev, struct VhostUserMsg *msg,
 	return RTE_VHOST_MSG_RESULT_REPLY;
 }
 
+static int
+vhost_user_get_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
+		      int main_fd __rte_unused)
+{
+	struct virtio_net *dev = *pdev;
+
+	if (validate_msg_fds(msg, 0) != 0)
+		return RTE_VHOST_MSG_RESULT_ERR;
+
+	msg->payload.u64 = dev->status;
+	msg->size = sizeof(msg->payload.u64);
+	msg->fd_num = 0;
+
+	return RTE_VHOST_MSG_RESULT_OK;
+}
+
 static int
 vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
 			int main_fd __rte_unused)
@@ -2476,6 +2497,16 @@ vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
 
 	dev->status = msg->payload.u64;
 
+	if ((dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK) &&
+	    (dev->flags & VIRTIO_DEV_FEATURES_FAILED)) {
+		VHOST_LOG_CONFIG(ERR, "FEATURES_OK bit is set but feature negotiation failed\n");
+		/*
+		 * Clear the bit to let the driver know about the feature
+		 * negotiation failure
+		 */
+		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
+	    }
+
 	VHOST_LOG_CONFIG(INFO, "New device status(0x%08x):\n"
 			"\t-ACKNOWLEDGE: %u\n"
 			"\t-DRIVER: %u\n"
@@ -2527,6 +2558,7 @@ static vhost_message_handler_t vhost_message_handlers[VHOST_USER_MAX] = {
 	[VHOST_USER_GET_INFLIGHT_FD] = vhost_user_get_inflight_fd,
 	[VHOST_USER_SET_INFLIGHT_FD] = vhost_user_set_inflight_fd,
 	[VHOST_USER_SET_STATUS] = vhost_user_set_status,
+	[VHOST_USER_GET_STATUS] = vhost_user_get_status,
 };
 
 /* return bytes# of read on success or negative val on failure. */
diff --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
index 82885ab5e..16fe03f88 100644
--- a/lib/librte_vhost/vhost_user.h
+++ b/lib/librte_vhost/vhost_user.h
@@ -58,6 +58,7 @@ typedef enum VhostUserRequest {
 	VHOST_USER_GET_INFLIGHT_FD = 31,
 	VHOST_USER_SET_INFLIGHT_FD = 32,
 	VHOST_USER_SET_STATUS = 39,
+	VHOST_USER_GET_STATUS = 40,
 	VHOST_USER_MAX = 41
 } VhostUserRequest;
 
-- 
2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2 7/8] vdpa/ifc: enable status protocol feature
  2020-07-02  8:32 [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init Adrian Moreno
                   ` (5 preceding siblings ...)
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message Adrian Moreno
@ 2020-07-02  8:32 ` Adrian Moreno
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 8/8] vdpa/mlx5: " Adrian Moreno
  7 siblings, 0 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-02  8:32 UTC (permalink / raw)
  To: dev, xiaolong.ye, shahafs, matan, maxime.coquelin, xiao.w.wang,
	viacheslavo
  Cc: jasowang, lulu, Adrian Moreno

From: Maxime Coquelin <maxime.coquelin@redhat.com>

This patch advertises VHOST_USER_PROTOCOL_F_STATUS
support in the IFC driver so that that the protocol
feature is negotiated.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/vdpa/ifc/ifcvf_vdpa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c b/drivers/vdpa/ifc/ifcvf_vdpa.c
index de54dc8aa..8bcc24e45 100644
--- a/drivers/vdpa/ifc/ifcvf_vdpa.c
+++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
@@ -1074,7 +1074,8 @@ ifcvf_get_vdpa_features(struct rte_vdpa_device *vdev, uint64_t *features)
 		 1ULL << VHOST_USER_PROTOCOL_F_SLAVE_REQ | \
 		 1ULL << VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD | \
 		 1ULL << VHOST_USER_PROTOCOL_F_HOST_NOTIFIER | \
-		 1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD)
+		 1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD | \
+		 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
 static int
 ifcvf_get_protocol_features(struct rte_vdpa_device *vdev, uint64_t *features)
 {
-- 
2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2 8/8] vdpa/mlx5: enable status protocol feature
  2020-07-02  8:32 [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init Adrian Moreno
                   ` (6 preceding siblings ...)
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 7/8] vdpa/ifc: enable status protocol feature Adrian Moreno
@ 2020-07-02  8:32 ` Adrian Moreno
  7 siblings, 0 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-02  8:32 UTC (permalink / raw)
  To: dev, xiaolong.ye, shahafs, matan, maxime.coquelin, xiao.w.wang,
	viacheslavo
  Cc: jasowang, lulu, Adrian Moreno

From: Maxime Coquelin <maxime.coquelin@redhat.com>

This patch advertises VHOST_USER_PROTOCOL_F_STATUS
support in the MLX5 driver so that that the protocol
feature is negotiated.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/vdpa/mlx5/mlx5_vdpa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
index dbd36ab0c..83901e37f 100644
--- a/drivers/vdpa/mlx5/mlx5_vdpa.c
+++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
@@ -39,7 +39,8 @@
 			     (1ULL << VHOST_USER_PROTOCOL_F_HOST_NOTIFIER) | \
 			     (1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD) | \
 			     (1ULL << VHOST_USER_PROTOCOL_F_MQ) | \
-			     (1ULL << VHOST_USER_PROTOCOL_F_NET_MTU))
+			     (1ULL << VHOST_USER_PROTOCOL_F_NET_MTU) | \
+			     (1ULL << VHOST_USER_PROTOCOL_F_STATUS))
 
 #define MLX5_VDPA_MAX_RETRIES 20
 #define MLX5_VDPA_USEC 1000
-- 
2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message Adrian Moreno
@ 2020-07-05 13:15   ` Xia, Chenbo
  2020-07-06  8:15     ` Adrian Moreno
  2020-07-06  3:22   ` Xia, Chenbo
  1 sibling, 1 reply; 15+ messages in thread
From: Xia, Chenbo @ 2020-07-05 13:15 UTC (permalink / raw)
  To: Adrian Moreno, dev, Ye, Xiaolong, shahafs, matan,
	maxime.coquelin, Wang, Xiao W, viacheslavo
  Cc: jasowang, lulu

Hi Adrian,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Adrian Moreno
> Sent: Thursday, July 2, 2020 4:33 PM
> To: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>;
> shahafs@mellanox.com; matan@mellanox.com; maxime.coquelin@redhat.com;
> Wang, Xiao W <xiao.w.wang@intel.com>; viacheslavo@mellanox.com
> Cc: jasowang@redhat.com; lulu@redhat.com; Adrian Moreno
> <amorenoz@redhat.com>
> Subject: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status
> message
> 
> This patch adds support to the new Virtio device get status Vhost-user message.
> 
> The driver can send this new message to read the device status.
> 
> One of the uses of this message is to ensure the feature negotiation has
> succeded.  According to the virtio spec, after completing the feature negotiation,
> the driver sets the FEATURE_OK status bit and re-reads it to ensure the device
> has accepted the features.
> 
> This patch also clears the FEATURE_OK status bit if the feature negotiation has
> failed to let the driver know about his failure.
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  lib/librte_vhost/vhost.h      |  2 ++
>  lib/librte_vhost/vhost_user.c | 32 ++++++++++++++++++++++++++++++++
> lib/librte_vhost/vhost_user.h |  1 +
>  3 files changed, 35 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h index
> 25d31c71b..e743821cc 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -32,6 +32,8 @@
>  #define VIRTIO_DEV_BUILTIN_VIRTIO_NET 4
>  /* Used to indicate that the device has its own data path and configured */
> #define VIRTIO_DEV_VDPA_CONFIGURED 8
> +/* Used to indicate that the feature negotiation failed */ #define
> +VIRTIO_DEV_FEATURES_FAILED 16
> 
>  /* Backend value set by guest. */
>  #define VIRTIO_DEV_STOPPED -1
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index
> 8d3d13913..00da7bf18 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -88,6 +88,7 @@ static const char *vhost_message_str[VHOST_USER_MAX]
> = {
>  	[VHOST_USER_GET_INFLIGHT_FD] = "VHOST_USER_GET_INFLIGHT_FD",
>  	[VHOST_USER_SET_INFLIGHT_FD] = "VHOST_USER_SET_INFLIGHT_FD",
>  	[VHOST_USER_SET_STATUS] = "VHOST_USER_SET_STATUS",
> +	[VHOST_USER_GET_STATUS] = "VHOST_USER_GET_STATUS",
>  };
> 
>  static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg); @@ -339,6
> +340,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct
> VhostUserMsg *msg,
>  		VHOST_LOG_CONFIG(ERR,
>  			"(%d) received invalid negotiated features.\n",
>  			dev->vid);
> +		dev->flags |= VIRTIO_DEV_FEATURES_FAILED;
> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
> +
>  		return RTE_VHOST_MSG_RESULT_ERR;
>  	}
> 
> @@ -402,6 +406,7 @@ vhost_user_set_features(struct virtio_net **pdev, struct
> VhostUserMsg *msg,
>  	if (vdpa_dev)
>  		vdpa_dev->ops->set_features(dev->vid);
> 
> +	dev->flags &= ~VIRTIO_DEV_FEATURES_FAILED;
>  	return RTE_VHOST_MSG_RESULT_OK;
>  }
> 
> @@ -2458,6 +2463,22 @@ vhost_user_postcopy_end(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
>  	return RTE_VHOST_MSG_RESULT_REPLY;
>  }
> 
> +static int
> +vhost_user_get_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
> +		      int main_fd __rte_unused)
> +{
> +	struct virtio_net *dev = *pdev;
> +
> +	if (validate_msg_fds(msg, 0) != 0)
> +		return RTE_VHOST_MSG_RESULT_ERR;
> +
> +	msg->payload.u64 = dev->status;
> +	msg->size = sizeof(msg->payload.u64);
> +	msg->fd_num = 0;
> +
> +	return RTE_VHOST_MSG_RESULT_OK;

Should this 'RESULT_OK' be 'RESULT_REPLY' since get_status msg needs a reply?

Thanks!
Chenbo

> +}
> +
>  static int
>  vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
>  			int main_fd __rte_unused)
> @@ -2476,6 +2497,16 @@ vhost_user_set_status(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
> 
>  	dev->status = msg->payload.u64;
> 
> +	if ((dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK) &&
> +	    (dev->flags & VIRTIO_DEV_FEATURES_FAILED)) {
> +		VHOST_LOG_CONFIG(ERR, "FEATURES_OK bit is set but feature
> negotiation failed\n");
> +		/*
> +		 * Clear the bit to let the driver know about the feature
> +		 * negotiation failure
> +		 */
> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
> +	    }
> +
>  	VHOST_LOG_CONFIG(INFO, "New device status(0x%08x):\n"
>  			"\t-ACKNOWLEDGE: %u\n"
>  			"\t-DRIVER: %u\n"
> @@ -2527,6 +2558,7 @@ static vhost_message_handler_t
> vhost_message_handlers[VHOST_USER_MAX] = {
>  	[VHOST_USER_GET_INFLIGHT_FD] = vhost_user_get_inflight_fd,
>  	[VHOST_USER_SET_INFLIGHT_FD] = vhost_user_set_inflight_fd,
>  	[VHOST_USER_SET_STATUS] = vhost_user_set_status,
> +	[VHOST_USER_GET_STATUS] = vhost_user_get_status,
>  };
> 
>  /* return bytes# of read on success or negative val on failure. */ diff --git
> a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h index
> 82885ab5e..16fe03f88 100644
> --- a/lib/librte_vhost/vhost_user.h
> +++ b/lib/librte_vhost/vhost_user.h
> @@ -58,6 +58,7 @@ typedef enum VhostUserRequest {
>  	VHOST_USER_GET_INFLIGHT_FD = 31,
>  	VHOST_USER_SET_INFLIGHT_FD = 32,
>  	VHOST_USER_SET_STATUS = 39,
> +	VHOST_USER_GET_STATUS = 40,
>  	VHOST_USER_MAX = 41
>  } VhostUserRequest;
> 
> --
> 2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message
  2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message Adrian Moreno
  2020-07-05 13:15   ` Xia, Chenbo
@ 2020-07-06  3:22   ` Xia, Chenbo
  2020-07-06  8:48     ` Adrian Moreno
  1 sibling, 1 reply; 15+ messages in thread
From: Xia, Chenbo @ 2020-07-06  3:22 UTC (permalink / raw)
  To: Adrian Moreno, dev, shahafs, matan, maxime.coquelin, Wang,
	Xiao W, viacheslavo
  Cc: jasowang, lulu

Hi Adrian,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Adrian Moreno
> Sent: Thursday, July 2, 2020 4:33 PM
> To: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>;
> shahafs@mellanox.com; matan@mellanox.com; maxime.coquelin@redhat.com;
> Wang, Xiao W <xiao.w.wang@intel.com>; viacheslavo@mellanox.com
> Cc: jasowang@redhat.com; lulu@redhat.com; Adrian Moreno
> <amorenoz@redhat.com>
> Subject: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status
> message
> 
> This patch adds support to the new Virtio device get status Vhost-user message.
> 
> The driver can send this new message to read the device status.
> 
> One of the uses of this message is to ensure the feature negotiation has
> succeded.  According to the virtio spec, after completing the feature negotiation,
> the driver sets the FEATURE_OK status bit and re-reads it to ensure the device
> has accepted the features.
> 
> This patch also clears the FEATURE_OK status bit if the feature negotiation has
> failed to let the driver know about his failure.
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  lib/librte_vhost/vhost.h      |  2 ++
>  lib/librte_vhost/vhost_user.c | 32 ++++++++++++++++++++++++++++++++
> lib/librte_vhost/vhost_user.h |  1 +
>  3 files changed, 35 insertions(+)
> 
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h index
> 25d31c71b..e743821cc 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -32,6 +32,8 @@
>  #define VIRTIO_DEV_BUILTIN_VIRTIO_NET 4
>  /* Used to indicate that the device has its own data path and configured */
> #define VIRTIO_DEV_VDPA_CONFIGURED 8
> +/* Used to indicate that the feature negotiation failed */ #define
> +VIRTIO_DEV_FEATURES_FAILED 16
> 
>  /* Backend value set by guest. */
>  #define VIRTIO_DEV_STOPPED -1
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index
> 8d3d13913..00da7bf18 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -88,6 +88,7 @@ static const char *vhost_message_str[VHOST_USER_MAX]
> = {
>  	[VHOST_USER_GET_INFLIGHT_FD] = "VHOST_USER_GET_INFLIGHT_FD",
>  	[VHOST_USER_SET_INFLIGHT_FD] = "VHOST_USER_SET_INFLIGHT_FD",
>  	[VHOST_USER_SET_STATUS] = "VHOST_USER_SET_STATUS",
> +	[VHOST_USER_GET_STATUS] = "VHOST_USER_GET_STATUS",
>  };
> 
>  static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg); @@ -339,6
> +340,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct
> VhostUserMsg *msg,
>  		VHOST_LOG_CONFIG(ERR,
>  			"(%d) received invalid negotiated features.\n",
>  			dev->vid);
> +		dev->flags |= VIRTIO_DEV_FEATURES_FAILED;
> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
> +
>  		return RTE_VHOST_MSG_RESULT_ERR;
>  	}
> 
> @@ -402,6 +406,7 @@ vhost_user_set_features(struct virtio_net **pdev, struct
> VhostUserMsg *msg,
>  	if (vdpa_dev)
>  		vdpa_dev->ops->set_features(dev->vid);
> 
> +	dev->flags &= ~VIRTIO_DEV_FEATURES_FAILED;
>  	return RTE_VHOST_MSG_RESULT_OK;
>  }
> 
> @@ -2458,6 +2463,22 @@ vhost_user_postcopy_end(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
>  	return RTE_VHOST_MSG_RESULT_REPLY;
>  }
> 
> +static int
> +vhost_user_get_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
> +		      int main_fd __rte_unused)
> +{
> +	struct virtio_net *dev = *pdev;
> +
> +	if (validate_msg_fds(msg, 0) != 0)
> +		return RTE_VHOST_MSG_RESULT_ERR;
> +
> +	msg->payload.u64 = dev->status;
> +	msg->size = sizeof(msg->payload.u64);
> +	msg->fd_num = 0;
> +
> +	return RTE_VHOST_MSG_RESULT_OK;
> +}
> +
>  static int
>  vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
>  			int main_fd __rte_unused)
> @@ -2476,6 +2497,16 @@ vhost_user_set_status(struct virtio_net **pdev,
> struct VhostUserMsg *msg,
> 
>  	dev->status = msg->payload.u64;
> 
> +	if ((dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK) &&
> +	    (dev->flags & VIRTIO_DEV_FEATURES_FAILED)) {
> +		VHOST_LOG_CONFIG(ERR, "FEATURES_OK bit is set but feature
> negotiation failed\n");
> +		/*
> +		 * Clear the bit to let the driver know about the feature
> +		 * negotiation failure
> +		 */
> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
> +	    }
> +

There's a coding style issue because of above '}' alignment. Could you fix this?

Another thing I'm not sure: if above condition happens, should it be treated
as err? If set status is with replay-ack (this will happen, right?), would QEMU
like to know this status is not set? As QEMU should know it during SET_FEATURES,
I'm not sure whether this will also need NACK when reply-ack enabled. What's
your opinion?

Thanks!
Chenbo

>  	VHOST_LOG_CONFIG(INFO, "New device status(0x%08x):\n"
>  			"\t-ACKNOWLEDGE: %u\n"
>  			"\t-DRIVER: %u\n"
> @@ -2527,6 +2558,7 @@ static vhost_message_handler_t
> vhost_message_handlers[VHOST_USER_MAX] = {
>  	[VHOST_USER_GET_INFLIGHT_FD] = vhost_user_get_inflight_fd,
>  	[VHOST_USER_SET_INFLIGHT_FD] = vhost_user_set_inflight_fd,
>  	[VHOST_USER_SET_STATUS] = vhost_user_set_status,
> +	[VHOST_USER_GET_STATUS] = vhost_user_get_status,
>  };
> 
>  /* return bytes# of read on success or negative val on failure. */ diff --git
> a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h index
> 82885ab5e..16fe03f88 100644
> --- a/lib/librte_vhost/vhost_user.h
> +++ b/lib/librte_vhost/vhost_user.h
> @@ -58,6 +58,7 @@ typedef enum VhostUserRequest {
>  	VHOST_USER_GET_INFLIGHT_FD = 31,
>  	VHOST_USER_SET_INFLIGHT_FD = 32,
>  	VHOST_USER_SET_STATUS = 39,
> +	VHOST_USER_GET_STATUS = 40,
>  	VHOST_USER_MAX = 41
>  } VhostUserRequest;
> 
> --
> 2.26.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message
  2020-07-05 13:15   ` Xia, Chenbo
@ 2020-07-06  8:15     ` Adrian Moreno
  0 siblings, 0 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-06  8:15 UTC (permalink / raw)
  To: Xia, Chenbo, dev, Ye, Xiaolong, shahafs, matan, maxime.coquelin,
	Wang, Xiao W, viacheslavo
  Cc: jasowang, lulu



On 7/5/20 3:15 PM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Adrian Moreno
>> Sent: Thursday, July 2, 2020 4:33 PM
>> To: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>;
>> shahafs@mellanox.com; matan@mellanox.com; maxime.coquelin@redhat.com;
>> Wang, Xiao W <xiao.w.wang@intel.com>; viacheslavo@mellanox.com
>> Cc: jasowang@redhat.com; lulu@redhat.com; Adrian Moreno
>> <amorenoz@redhat.com>
>> Subject: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status
>> message
>>
>> This patch adds support to the new Virtio device get status Vhost-user message.
>>
>> The driver can send this new message to read the device status.
>>
>> One of the uses of this message is to ensure the feature negotiation has
>> succeded.  According to the virtio spec, after completing the feature negotiation,
>> the driver sets the FEATURE_OK status bit and re-reads it to ensure the device
>> has accepted the features.
>>
>> This patch also clears the FEATURE_OK status bit if the feature negotiation has
>> failed to let the driver know about his failure.
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>  lib/librte_vhost/vhost.h      |  2 ++
>>  lib/librte_vhost/vhost_user.c | 32 ++++++++++++++++++++++++++++++++
>> lib/librte_vhost/vhost_user.h |  1 +
>>  3 files changed, 35 insertions(+)
>>
>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h index
>> 25d31c71b..e743821cc 100644
>> --- a/lib/librte_vhost/vhost.h
>> +++ b/lib/librte_vhost/vhost.h
>> @@ -32,6 +32,8 @@
>>  #define VIRTIO_DEV_BUILTIN_VIRTIO_NET 4
>>  /* Used to indicate that the device has its own data path and configured */
>> #define VIRTIO_DEV_VDPA_CONFIGURED 8
>> +/* Used to indicate that the feature negotiation failed */ #define
>> +VIRTIO_DEV_FEATURES_FAILED 16
>>
>>  /* Backend value set by guest. */
>>  #define VIRTIO_DEV_STOPPED -1
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index
>> 8d3d13913..00da7bf18 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -88,6 +88,7 @@ static const char *vhost_message_str[VHOST_USER_MAX]
>> = {
>>  	[VHOST_USER_GET_INFLIGHT_FD] = "VHOST_USER_GET_INFLIGHT_FD",
>>  	[VHOST_USER_SET_INFLIGHT_FD] = "VHOST_USER_SET_INFLIGHT_FD",
>>  	[VHOST_USER_SET_STATUS] = "VHOST_USER_SET_STATUS",
>> +	[VHOST_USER_GET_STATUS] = "VHOST_USER_GET_STATUS",
>>  };
>>
>>  static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg); @@ -339,6
>> +340,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct
>> VhostUserMsg *msg,
>>  		VHOST_LOG_CONFIG(ERR,
>>  			"(%d) received invalid negotiated features.\n",
>>  			dev->vid);
>> +		dev->flags |= VIRTIO_DEV_FEATURES_FAILED;
>> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
>> +
>>  		return RTE_VHOST_MSG_RESULT_ERR;
>>  	}
>>
>> @@ -402,6 +406,7 @@ vhost_user_set_features(struct virtio_net **pdev, struct
>> VhostUserMsg *msg,
>>  	if (vdpa_dev)
>>  		vdpa_dev->ops->set_features(dev->vid);
>>
>> +	dev->flags &= ~VIRTIO_DEV_FEATURES_FAILED;
>>  	return RTE_VHOST_MSG_RESULT_OK;
>>  }
>>
>> @@ -2458,6 +2463,22 @@ vhost_user_postcopy_end(struct virtio_net **pdev,
>> struct VhostUserMsg *msg,
>>  	return RTE_VHOST_MSG_RESULT_REPLY;
>>  }
>>
>> +static int
>> +vhost_user_get_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
>> +		      int main_fd __rte_unused)
>> +{
>> +	struct virtio_net *dev = *pdev;
>> +
>> +	if (validate_msg_fds(msg, 0) != 0)
>> +		return RTE_VHOST_MSG_RESULT_ERR;
>> +
>> +	msg->payload.u64 = dev->status;
>> +	msg->size = sizeof(msg->payload.u64);
>> +	msg->fd_num = 0;
>> +
>> +	return RTE_VHOST_MSG_RESULT_OK;
> 
> Should this 'RESULT_OK' be 'RESULT_REPLY' since get_status msg needs a reply?
> 
Right! Thanks for catching it.
I'll update.

> Thanks!
> Chenbo
> 
>> +}
>> +
>>  static int
>>  vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>  			int main_fd __rte_unused)
>> @@ -2476,6 +2497,16 @@ vhost_user_set_status(struct virtio_net **pdev,
>> struct VhostUserMsg *msg,
>>
>>  	dev->status = msg->payload.u64;
>>
>> +	if ((dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK) &&
>> +	    (dev->flags & VIRTIO_DEV_FEATURES_FAILED)) {
>> +		VHOST_LOG_CONFIG(ERR, "FEATURES_OK bit is set but feature
>> negotiation failed\n");
>> +		/*
>> +		 * Clear the bit to let the driver know about the feature
>> +		 * negotiation failure
>> +		 */
>> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
>> +	    }
>> +
>>  	VHOST_LOG_CONFIG(INFO, "New device status(0x%08x):\n"
>>  			"\t-ACKNOWLEDGE: %u\n"
>>  			"\t-DRIVER: %u\n"
>> @@ -2527,6 +2558,7 @@ static vhost_message_handler_t
>> vhost_message_handlers[VHOST_USER_MAX] = {
>>  	[VHOST_USER_GET_INFLIGHT_FD] = vhost_user_get_inflight_fd,
>>  	[VHOST_USER_SET_INFLIGHT_FD] = vhost_user_set_inflight_fd,
>>  	[VHOST_USER_SET_STATUS] = vhost_user_set_status,
>> +	[VHOST_USER_GET_STATUS] = vhost_user_get_status,
>>  };
>>
>>  /* return bytes# of read on success or negative val on failure. */ diff --git
>> a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h index
>> 82885ab5e..16fe03f88 100644
>> --- a/lib/librte_vhost/vhost_user.h
>> +++ b/lib/librte_vhost/vhost_user.h
>> @@ -58,6 +58,7 @@ typedef enum VhostUserRequest {
>>  	VHOST_USER_GET_INFLIGHT_FD = 31,
>>  	VHOST_USER_SET_INFLIGHT_FD = 32,
>>  	VHOST_USER_SET_STATUS = 39,
>> +	VHOST_USER_GET_STATUS = 40,
>>  	VHOST_USER_MAX = 41
>>  } VhostUserRequest;
>>
>> --
>> 2.26.2
> 

-- 
Adrián Moreno


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message
  2020-07-06  3:22   ` Xia, Chenbo
@ 2020-07-06  8:48     ` Adrian Moreno
  2020-07-06 10:29       ` Xia, Chenbo
  0 siblings, 1 reply; 15+ messages in thread
From: Adrian Moreno @ 2020-07-06  8:48 UTC (permalink / raw)
  To: Xia, Chenbo, dev, shahafs, matan, maxime.coquelin, Wang, Xiao W,
	viacheslavo
  Cc: jasowang, lulu



On 7/6/20 5:22 AM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Adrian Moreno
>> Sent: Thursday, July 2, 2020 4:33 PM
>> To: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>;
>> shahafs@mellanox.com; matan@mellanox.com; maxime.coquelin@redhat.com;
>> Wang, Xiao W <xiao.w.wang@intel.com>; viacheslavo@mellanox.com
>> Cc: jasowang@redhat.com; lulu@redhat.com; Adrian Moreno
>> <amorenoz@redhat.com>
>> Subject: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status
>> message
>>
>> This patch adds support to the new Virtio device get status Vhost-user message.
>>
>> The driver can send this new message to read the device status.
>>
>> One of the uses of this message is to ensure the feature negotiation has
>> succeded.  According to the virtio spec, after completing the feature negotiation,
>> the driver sets the FEATURE_OK status bit and re-reads it to ensure the device
>> has accepted the features.
>>
>> This patch also clears the FEATURE_OK status bit if the feature negotiation has
>> failed to let the driver know about his failure.
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>  lib/librte_vhost/vhost.h      |  2 ++
>>  lib/librte_vhost/vhost_user.c | 32 ++++++++++++++++++++++++++++++++
>> lib/librte_vhost/vhost_user.h |  1 +
>>  3 files changed, 35 insertions(+)
>>
>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h index
>> 25d31c71b..e743821cc 100644
>> --- a/lib/librte_vhost/vhost.h
>> +++ b/lib/librte_vhost/vhost.h
>> @@ -32,6 +32,8 @@
>>  #define VIRTIO_DEV_BUILTIN_VIRTIO_NET 4
>>  /* Used to indicate that the device has its own data path and configured */
>> #define VIRTIO_DEV_VDPA_CONFIGURED 8
>> +/* Used to indicate that the feature negotiation failed */ #define
>> +VIRTIO_DEV_FEATURES_FAILED 16
>>
>>  /* Backend value set by guest. */
>>  #define VIRTIO_DEV_STOPPED -1
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index
>> 8d3d13913..00da7bf18 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -88,6 +88,7 @@ static const char *vhost_message_str[VHOST_USER_MAX]
>> = {
>>  	[VHOST_USER_GET_INFLIGHT_FD] = "VHOST_USER_GET_INFLIGHT_FD",
>>  	[VHOST_USER_SET_INFLIGHT_FD] = "VHOST_USER_SET_INFLIGHT_FD",
>>  	[VHOST_USER_SET_STATUS] = "VHOST_USER_SET_STATUS",
>> +	[VHOST_USER_GET_STATUS] = "VHOST_USER_GET_STATUS",
>>  };
>>
>>  static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg); @@ -339,6
>> +340,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct
>> VhostUserMsg *msg,
>>  		VHOST_LOG_CONFIG(ERR,
>>  			"(%d) received invalid negotiated features.\n",
>>  			dev->vid);
>> +		dev->flags |= VIRTIO_DEV_FEATURES_FAILED;
>> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
>> +
>>  		return RTE_VHOST_MSG_RESULT_ERR;
>>  	}
>>
>> @@ -402,6 +406,7 @@ vhost_user_set_features(struct virtio_net **pdev, struct
>> VhostUserMsg *msg,
>>  	if (vdpa_dev)
>>  		vdpa_dev->ops->set_features(dev->vid);
>>
>> +	dev->flags &= ~VIRTIO_DEV_FEATURES_FAILED;
>>  	return RTE_VHOST_MSG_RESULT_OK;
>>  }
>>
>> @@ -2458,6 +2463,22 @@ vhost_user_postcopy_end(struct virtio_net **pdev,
>> struct VhostUserMsg *msg,
>>  	return RTE_VHOST_MSG_RESULT_REPLY;
>>  }
>>
>> +static int
>> +vhost_user_get_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
>> +		      int main_fd __rte_unused)
>> +{
>> +	struct virtio_net *dev = *pdev;
>> +
>> +	if (validate_msg_fds(msg, 0) != 0)
>> +		return RTE_VHOST_MSG_RESULT_ERR;
>> +
>> +	msg->payload.u64 = dev->status;
>> +	msg->size = sizeof(msg->payload.u64);
>> +	msg->fd_num = 0;
>> +
>> +	return RTE_VHOST_MSG_RESULT_OK;
>> +}
>> +
>>  static int
>>  vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>  			int main_fd __rte_unused)
>> @@ -2476,6 +2497,16 @@ vhost_user_set_status(struct virtio_net **pdev,
>> struct VhostUserMsg *msg,
>>
>>  	dev->status = msg->payload.u64;
>>
>> +	if ((dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK) &&
>> +	    (dev->flags & VIRTIO_DEV_FEATURES_FAILED)) {
>> +		VHOST_LOG_CONFIG(ERR, "FEATURES_OK bit is set but feature
>> negotiation failed\n");
>> +		/*
>> +		 * Clear the bit to let the driver know about the feature
>> +		 * negotiation failure
>> +		 */
>> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
>> +	    }
>> +
> 
> There's a coding style issue because of above '}' alignment. Could you fix this?
> 
> Another thing I'm not sure: if above condition happens, should it be treated
> as err? If set status is with replay-ack (this will happen, right?), would QEMU
> like to know this status is not set? As QEMU should know it during SET_FEATURES,
> I'm not sure whether this will also need NACK when reply-ack enabled. What's
> your opinion?
> 

My interpretation was that, since we have already NACKed SET_FEATURES,
SET_STATUS should only NACK if we were unable to set the status (device is not
present, invalid message, etc), and according to the virtio standard the driver
must read again and verify FEATURES_OK is still set, therefore NACKing the
SET_STATUS would only hide the real problem.

Besides, for a driver (e.g: qemu) that implements the virtio/vhost logic
agnostic of the underlying vhost type (vhost-net or vhost-user) a spec-oriented
way of expressing errors is preferred. See as an example a (still unmerged) use
of this feature in function "static int vhost_vdpa_set_features()" in:

https://patchwork.ozlabs.org/project/qemu-devel/patch/20200701145538.22333-14-lulu@redhat.com/

Having said all this, I realize this should be a rare case. This mechanism is in
place to prevent the driver from configuring an incompatible combination of
features. However, the vhost backend only checks that qemu has honored it's
original feature set which the driver must do according to the spec. So I'm
happy to change it if you have a strong opinion on this.


> Thanks!
> Chenbo
> 
>>  	VHOST_LOG_CONFIG(INFO, "New device status(0x%08x):\n"
>>  			"\t-ACKNOWLEDGE: %u\n"
>>  			"\t-DRIVER: %u\n"
>> @@ -2527,6 +2558,7 @@ static vhost_message_handler_t
>> vhost_message_handlers[VHOST_USER_MAX] = {
>>  	[VHOST_USER_GET_INFLIGHT_FD] = vhost_user_get_inflight_fd,
>>  	[VHOST_USER_SET_INFLIGHT_FD] = vhost_user_set_inflight_fd,
>>  	[VHOST_USER_SET_STATUS] = vhost_user_set_status,
>> +	[VHOST_USER_GET_STATUS] = vhost_user_get_status,
>>  };
>>
>>  /* return bytes# of read on success or negative val on failure. */ diff --git
>> a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h index
>> 82885ab5e..16fe03f88 100644
>> --- a/lib/librte_vhost/vhost_user.h
>> +++ b/lib/librte_vhost/vhost_user.h
>> @@ -58,6 +58,7 @@ typedef enum VhostUserRequest {
>>  	VHOST_USER_GET_INFLIGHT_FD = 31,
>>  	VHOST_USER_SET_INFLIGHT_FD = 32,
>>  	VHOST_USER_SET_STATUS = 39,
>> +	VHOST_USER_GET_STATUS = 40,
>>  	VHOST_USER_MAX = 41
>>  } VhostUserRequest;
>>
>> --
>> 2.26.2
> 

-- 
Adrián Moreno


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message
  2020-07-06  8:48     ` Adrian Moreno
@ 2020-07-06 10:29       ` Xia, Chenbo
  2020-07-06 10:45         ` Adrian Moreno
  0 siblings, 1 reply; 15+ messages in thread
From: Xia, Chenbo @ 2020-07-06 10:29 UTC (permalink / raw)
  To: Adrian Moreno, dev, shahafs, matan, maxime.coquelin, Wang,
	Xiao W, viacheslavo
  Cc: jasowang, lulu

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Monday, July 6, 2020 4:49 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org;
> shahafs@mellanox.com; matan@mellanox.com; maxime.coquelin@redhat.com;
> Wang, Xiao W <xiao.w.wang@intel.com>; viacheslavo@mellanox.com
> Cc: jasowang@redhat.com; lulu@redhat.com
> Subject: Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status
> message
> 
> 
> 
> On 7/6/20 5:22 AM, Xia, Chenbo wrote:
> > Hi Adrian,
> >
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Adrian Moreno
> >> Sent: Thursday, July 2, 2020 4:33 PM
> >> To: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>;
> >> shahafs@mellanox.com; matan@mellanox.com;
> maxime.coquelin@redhat.com;
> >> Wang, Xiao W <xiao.w.wang@intel.com>; viacheslavo@mellanox.com
> >> Cc: jasowang@redhat.com; lulu@redhat.com; Adrian Moreno
> >> <amorenoz@redhat.com>
> >> Subject: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get
> >> status message
> >>
> >> This patch adds support to the new Virtio device get status Vhost-user
> message.
> >>
> >> The driver can send this new message to read the device status.
> >>
> >> One of the uses of this message is to ensure the feature negotiation
> >> has succeded.  According to the virtio spec, after completing the
> >> feature negotiation, the driver sets the FEATURE_OK status bit and
> >> re-reads it to ensure the device has accepted the features.
> >>
> >> This patch also clears the FEATURE_OK status bit if the feature
> >> negotiation has failed to let the driver know about his failure.
> >>
> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> >> ---
> >>  lib/librte_vhost/vhost.h      |  2 ++
> >>  lib/librte_vhost/vhost_user.c | 32 ++++++++++++++++++++++++++++++++
> >> lib/librte_vhost/vhost_user.h |  1 +
> >>  3 files changed, 35 insertions(+)
> >>
> >> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> >> index 25d31c71b..e743821cc 100644
> >> --- a/lib/librte_vhost/vhost.h
> >> +++ b/lib/librte_vhost/vhost.h
> >> @@ -32,6 +32,8 @@
> >>  #define VIRTIO_DEV_BUILTIN_VIRTIO_NET 4
> >>  /* Used to indicate that the device has its own data path and
> >> configured */ #define VIRTIO_DEV_VDPA_CONFIGURED 8
> >> +/* Used to indicate that the feature negotiation failed */ #define
> >> +VIRTIO_DEV_FEATURES_FAILED 16
> >>
> >>  /* Backend value set by guest. */
> >>  #define VIRTIO_DEV_STOPPED -1
> >> diff --git a/lib/librte_vhost/vhost_user.c
> >> b/lib/librte_vhost/vhost_user.c index
> >> 8d3d13913..00da7bf18 100644
> >> --- a/lib/librte_vhost/vhost_user.c
> >> +++ b/lib/librte_vhost/vhost_user.c
> >> @@ -88,6 +88,7 @@ static const char
> >> *vhost_message_str[VHOST_USER_MAX]
> >> = {
> >>  	[VHOST_USER_GET_INFLIGHT_FD] = "VHOST_USER_GET_INFLIGHT_FD",
> >>  	[VHOST_USER_SET_INFLIGHT_FD] = "VHOST_USER_SET_INFLIGHT_FD",
> >>  	[VHOST_USER_SET_STATUS] = "VHOST_USER_SET_STATUS",
> >> +	[VHOST_USER_GET_STATUS] = "VHOST_USER_GET_STATUS",
> >>  };
> >>
> >>  static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg);
> >> @@ -339,6
> >> +340,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct
> >> VhostUserMsg *msg,
> >>  		VHOST_LOG_CONFIG(ERR,
> >>  			"(%d) received invalid negotiated features.\n",
> >>  			dev->vid);
> >> +		dev->flags |= VIRTIO_DEV_FEATURES_FAILED;
> >> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
> >> +
> >>  		return RTE_VHOST_MSG_RESULT_ERR;
> >>  	}
> >>
> >> @@ -402,6 +406,7 @@ vhost_user_set_features(struct virtio_net **pdev,
> >> struct VhostUserMsg *msg,
> >>  	if (vdpa_dev)
> >>  		vdpa_dev->ops->set_features(dev->vid);
> >>
> >> +	dev->flags &= ~VIRTIO_DEV_FEATURES_FAILED;
> >>  	return RTE_VHOST_MSG_RESULT_OK;
> >>  }
> >>
> >> @@ -2458,6 +2463,22 @@ vhost_user_postcopy_end(struct virtio_net
> >> **pdev, struct VhostUserMsg *msg,
> >>  	return RTE_VHOST_MSG_RESULT_REPLY;
> >>  }
> >>
> >> +static int
> >> +vhost_user_get_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
> >> +		      int main_fd __rte_unused)
> >> +{
> >> +	struct virtio_net *dev = *pdev;
> >> +
> >> +	if (validate_msg_fds(msg, 0) != 0)
> >> +		return RTE_VHOST_MSG_RESULT_ERR;
> >> +
> >> +	msg->payload.u64 = dev->status;
> >> +	msg->size = sizeof(msg->payload.u64);
> >> +	msg->fd_num = 0;
> >> +
> >> +	return RTE_VHOST_MSG_RESULT_OK;
> >> +}
> >> +
> >>  static int
> >>  vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
> >>  			int main_fd __rte_unused)
> >> @@ -2476,6 +2497,16 @@ vhost_user_set_status(struct virtio_net
> >> **pdev, struct VhostUserMsg *msg,
> >>
> >>  	dev->status = msg->payload.u64;
> >>
> >> +	if ((dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK) &&
> >> +	    (dev->flags & VIRTIO_DEV_FEATURES_FAILED)) {
> >> +		VHOST_LOG_CONFIG(ERR, "FEATURES_OK bit is set but feature
> >> negotiation failed\n");
> >> +		/*
> >> +		 * Clear the bit to let the driver know about the feature
> >> +		 * negotiation failure
> >> +		 */
> >> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
> >> +	    }
> >> +
> >
> > There's a coding style issue because of above '}' alignment. Could you fix this?
> >
> > Another thing I'm not sure: if above condition happens, should it be
> > treated as err? If set status is with replay-ack (this will happen,
> > right?), would QEMU like to know this status is not set? As QEMU
> > should know it during SET_FEATURES, I'm not sure whether this will
> > also need NACK when reply-ack enabled. What's your opinion?
> >
> 
> My interpretation was that, since we have already NACKed SET_FEATURES,
> SET_STATUS should only NACK if we were unable to set the status (device is not
> present, invalid message, etc), and according to the virtio standard the driver
> must read again and verify FEATURES_OK is still set, therefore NACKing the
> SET_STATUS would only hide the real problem.
> 
> Besides, for a driver (e.g: qemu) that implements the virtio/vhost logic agnostic
> of the underlying vhost type (vhost-net or vhost-user) a spec-oriented way of
> expressing errors is preferred. See as an example a (still unmerged) use of this
> feature in function "static int vhost_vdpa_set_features()" in:
> 
> https://patchwork.ozlabs.org/project/qemu-
> devel/patch/20200701145538.22333-14-lulu@redhat.com/
> 
> Having said all this, I realize this should be a rare case. This mechanism is in place
> to prevent the driver from configuring an incompatible combination of features.
> However, the vhost backend only checks that qemu has honored it's original
> feature set which the driver must do according to the spec. So I'm happy to
> change it if you have a strong opinion on this.

Yeah, it makes sense that we should NACK SET_FEATURES for this. So I'm fine with
current implementation. BTW, about REPLY_ACK, does spec say something about
which messages should set NEED_REPLY if REPLY_ACK is supported? I only see some
msg like SET_SLAVE_REQ_FD has description about REPLY_ACK.

Thanks,
Chenbo

> 
> 
> > Thanks!
> > Chenbo
> >
> >>  	VHOST_LOG_CONFIG(INFO, "New device status(0x%08x):\n"
> >>  			"\t-ACKNOWLEDGE: %u\n"
> >>  			"\t-DRIVER: %u\n"
> >> @@ -2527,6 +2558,7 @@ static vhost_message_handler_t
> >> vhost_message_handlers[VHOST_USER_MAX] = {
> >>  	[VHOST_USER_GET_INFLIGHT_FD] = vhost_user_get_inflight_fd,
> >>  	[VHOST_USER_SET_INFLIGHT_FD] = vhost_user_set_inflight_fd,
> >>  	[VHOST_USER_SET_STATUS] = vhost_user_set_status,
> >> +	[VHOST_USER_GET_STATUS] = vhost_user_get_status,
> >>  };
> >>
> >>  /* return bytes# of read on success or negative val on failure. */
> >> diff --git a/lib/librte_vhost/vhost_user.h
> >> b/lib/librte_vhost/vhost_user.h index
> >> 82885ab5e..16fe03f88 100644
> >> --- a/lib/librte_vhost/vhost_user.h
> >> +++ b/lib/librte_vhost/vhost_user.h
> >> @@ -58,6 +58,7 @@ typedef enum VhostUserRequest {
> >>  	VHOST_USER_GET_INFLIGHT_FD = 31,
> >>  	VHOST_USER_SET_INFLIGHT_FD = 32,
> >>  	VHOST_USER_SET_STATUS = 39,
> >> +	VHOST_USER_GET_STATUS = 40,
> >>  	VHOST_USER_MAX = 41
> >>  } VhostUserRequest;
> >>
> >> --
> >> 2.26.2
> >
> 
> --
> Adrián Moreno


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message
  2020-07-06 10:29       ` Xia, Chenbo
@ 2020-07-06 10:45         ` Adrian Moreno
  0 siblings, 0 replies; 15+ messages in thread
From: Adrian Moreno @ 2020-07-06 10:45 UTC (permalink / raw)
  To: Xia, Chenbo, dev, shahafs, matan, maxime.coquelin, Wang, Xiao W,
	viacheslavo
  Cc: jasowang, lulu



On 7/6/20 12:29 PM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Monday, July 6, 2020 4:49 PM
>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org;
>> shahafs@mellanox.com; matan@mellanox.com; maxime.coquelin@redhat.com;
>> Wang, Xiao W <xiao.w.wang@intel.com>; viacheslavo@mellanox.com
>> Cc: jasowang@redhat.com; lulu@redhat.com
>> Subject: Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status
>> message
>>
>>
>>
>> On 7/6/20 5:22 AM, Xia, Chenbo wrote:
>>> Hi Adrian,
>>>
>>>> -----Original Message-----
>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Adrian Moreno
>>>> Sent: Thursday, July 2, 2020 4:33 PM
>>>> To: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>;
>>>> shahafs@mellanox.com; matan@mellanox.com;
>> maxime.coquelin@redhat.com;
>>>> Wang, Xiao W <xiao.w.wang@intel.com>; viacheslavo@mellanox.com
>>>> Cc: jasowang@redhat.com; lulu@redhat.com; Adrian Moreno
>>>> <amorenoz@redhat.com>
>>>> Subject: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get
>>>> status message
>>>>
>>>> This patch adds support to the new Virtio device get status Vhost-user
>> message.
>>>>
>>>> The driver can send this new message to read the device status.
>>>>
>>>> One of the uses of this message is to ensure the feature negotiation
>>>> has succeded.  According to the virtio spec, after completing the
>>>> feature negotiation, the driver sets the FEATURE_OK status bit and
>>>> re-reads it to ensure the device has accepted the features.
>>>>
>>>> This patch also clears the FEATURE_OK status bit if the feature
>>>> negotiation has failed to let the driver know about his failure.
>>>>
>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>>> ---
>>>>  lib/librte_vhost/vhost.h      |  2 ++
>>>>  lib/librte_vhost/vhost_user.c | 32 ++++++++++++++++++++++++++++++++
>>>> lib/librte_vhost/vhost_user.h |  1 +
>>>>  3 files changed, 35 insertions(+)
>>>>
>>>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
>>>> index 25d31c71b..e743821cc 100644
>>>> --- a/lib/librte_vhost/vhost.h
>>>> +++ b/lib/librte_vhost/vhost.h
>>>> @@ -32,6 +32,8 @@
>>>>  #define VIRTIO_DEV_BUILTIN_VIRTIO_NET 4
>>>>  /* Used to indicate that the device has its own data path and
>>>> configured */ #define VIRTIO_DEV_VDPA_CONFIGURED 8
>>>> +/* Used to indicate that the feature negotiation failed */ #define
>>>> +VIRTIO_DEV_FEATURES_FAILED 16
>>>>
>>>>  /* Backend value set by guest. */
>>>>  #define VIRTIO_DEV_STOPPED -1
>>>> diff --git a/lib/librte_vhost/vhost_user.c
>>>> b/lib/librte_vhost/vhost_user.c index
>>>> 8d3d13913..00da7bf18 100644
>>>> --- a/lib/librte_vhost/vhost_user.c
>>>> +++ b/lib/librte_vhost/vhost_user.c
>>>> @@ -88,6 +88,7 @@ static const char
>>>> *vhost_message_str[VHOST_USER_MAX]
>>>> = {
>>>>  	[VHOST_USER_GET_INFLIGHT_FD] = "VHOST_USER_GET_INFLIGHT_FD",
>>>>  	[VHOST_USER_SET_INFLIGHT_FD] = "VHOST_USER_SET_INFLIGHT_FD",
>>>>  	[VHOST_USER_SET_STATUS] = "VHOST_USER_SET_STATUS",
>>>> +	[VHOST_USER_GET_STATUS] = "VHOST_USER_GET_STATUS",
>>>>  };
>>>>
>>>>  static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg);
>>>> @@ -339,6
>>>> +340,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct
>>>> VhostUserMsg *msg,
>>>>  		VHOST_LOG_CONFIG(ERR,
>>>>  			"(%d) received invalid negotiated features.\n",
>>>>  			dev->vid);
>>>> +		dev->flags |= VIRTIO_DEV_FEATURES_FAILED;
>>>> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
>>>> +
>>>>  		return RTE_VHOST_MSG_RESULT_ERR;
>>>>  	}
>>>>
>>>> @@ -402,6 +406,7 @@ vhost_user_set_features(struct virtio_net **pdev,
>>>> struct VhostUserMsg *msg,
>>>>  	if (vdpa_dev)
>>>>  		vdpa_dev->ops->set_features(dev->vid);
>>>>
>>>> +	dev->flags &= ~VIRTIO_DEV_FEATURES_FAILED;
>>>>  	return RTE_VHOST_MSG_RESULT_OK;
>>>>  }
>>>>
>>>> @@ -2458,6 +2463,22 @@ vhost_user_postcopy_end(struct virtio_net
>>>> **pdev, struct VhostUserMsg *msg,
>>>>  	return RTE_VHOST_MSG_RESULT_REPLY;
>>>>  }
>>>>
>>>> +static int
>>>> +vhost_user_get_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>>> +		      int main_fd __rte_unused)
>>>> +{
>>>> +	struct virtio_net *dev = *pdev;
>>>> +
>>>> +	if (validate_msg_fds(msg, 0) != 0)
>>>> +		return RTE_VHOST_MSG_RESULT_ERR;
>>>> +
>>>> +	msg->payload.u64 = dev->status;
>>>> +	msg->size = sizeof(msg->payload.u64);
>>>> +	msg->fd_num = 0;
>>>> +
>>>> +	return RTE_VHOST_MSG_RESULT_OK;
>>>> +}
>>>> +
>>>>  static int
>>>>  vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>>>  			int main_fd __rte_unused)
>>>> @@ -2476,6 +2497,16 @@ vhost_user_set_status(struct virtio_net
>>>> **pdev, struct VhostUserMsg *msg,
>>>>
>>>>  	dev->status = msg->payload.u64;
>>>>
>>>> +	if ((dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK) &&
>>>> +	    (dev->flags & VIRTIO_DEV_FEATURES_FAILED)) {
>>>> +		VHOST_LOG_CONFIG(ERR, "FEATURES_OK bit is set but feature
>>>> negotiation failed\n");
>>>> +		/*
>>>> +		 * Clear the bit to let the driver know about the feature
>>>> +		 * negotiation failure
>>>> +		 */
>>>> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
>>>> +	    }
>>>> +
>>>
>>> There's a coding style issue because of above '}' alignment. Could you fix this?
>>>
>>> Another thing I'm not sure: if above condition happens, should it be
>>> treated as err? If set status is with replay-ack (this will happen,
>>> right?), would QEMU like to know this status is not set? As QEMU
>>> should know it during SET_FEATURES, I'm not sure whether this will
>>> also need NACK when reply-ack enabled. What's your opinion?
>>>
>>
>> My interpretation was that, since we have already NACKed SET_FEATURES,
>> SET_STATUS should only NACK if we were unable to set the status (device is not
>> present, invalid message, etc), and according to the virtio standard the driver
>> must read again and verify FEATURES_OK is still set, therefore NACKing the
>> SET_STATUS would only hide the real problem.
>>
>> Besides, for a driver (e.g: qemu) that implements the virtio/vhost logic agnostic
>> of the underlying vhost type (vhost-net or vhost-user) a spec-oriented way of
>> expressing errors is preferred. See as an example a (still unmerged) use of this
>> feature in function "static int vhost_vdpa_set_features()" in:
>>
>> https://patchwork.ozlabs.org/project/qemu-
>> devel/patch/20200701145538.22333-14-lulu@redhat.com/
>>
>> Having said all this, I realize this should be a rare case. This mechanism is in place
>> to prevent the driver from configuring an incompatible combination of features.
>> However, the vhost backend only checks that qemu has honored it's original
>> feature set which the driver must do according to the spec. So I'm happy to
>> change it if you have a strong opinion on this.
> 
> Yeah, it makes sense that we should NACK SET_FEATURES for this. So I'm fine with
> current implementation. BTW, about REPLY_ACK, does spec say something about
> which messages should set NEED_REPLY if REPLY_ACK is supported? I only see some
> msg like SET_SLAVE_REQ_FD has description about REPLY_ACK.
> 

OK. I'll resend the series addressing the other comments.

With regards to the VHOST_USER_PROTOCOL_F_REPLY_ACK feature bit, the spec says:

"With this protocol extension negotiated, the sender (QEMU) can set the
need_reply [Bit 3] flag to any command."

So, in general, the vhost backend which usually acts as "slave" (until we find a
better word), only needs to send the response if qemu has requested it.

Now, I said in general because there are some messages that are originated by
the backend if VHOST_USER_PROTOCOL_F_SLAVE_REQ is negotiated [1]. Those messages
start with "VHOST_USER_SLAVE_*". That's why you will find code that sets the
need_reply bit on those messages.

Thanks,
Adrián

[1]
https://github.com/qemu/qemu/blob/master/docs/interop/vhost-user.rst#slave-communication





>>
>>
>>> Thanks!
>>> Chenbo
>>>
>>>>  	VHOST_LOG_CONFIG(INFO, "New device status(0x%08x):\n"
>>>>  			"\t-ACKNOWLEDGE: %u\n"
>>>>  			"\t-DRIVER: %u\n"
>>>> @@ -2527,6 +2558,7 @@ static vhost_message_handler_t
>>>> vhost_message_handlers[VHOST_USER_MAX] = {
>>>>  	[VHOST_USER_GET_INFLIGHT_FD] = vhost_user_get_inflight_fd,
>>>>  	[VHOST_USER_SET_INFLIGHT_FD] = vhost_user_set_inflight_fd,
>>>>  	[VHOST_USER_SET_STATUS] = vhost_user_set_status,
>>>> +	[VHOST_USER_GET_STATUS] = vhost_user_get_status,
>>>>  };
>>>>
>>>>  /* return bytes# of read on success or negative val on failure. */
>>>> diff --git a/lib/librte_vhost/vhost_user.h
>>>> b/lib/librte_vhost/vhost_user.h index
>>>> 82885ab5e..16fe03f88 100644
>>>> --- a/lib/librte_vhost/vhost_user.h
>>>> +++ b/lib/librte_vhost/vhost_user.h
>>>> @@ -58,6 +58,7 @@ typedef enum VhostUserRequest {
>>>>  	VHOST_USER_GET_INFLIGHT_FD = 31,
>>>>  	VHOST_USER_SET_INFLIGHT_FD = 32,
>>>>  	VHOST_USER_SET_STATUS = 39,
>>>> +	VHOST_USER_GET_STATUS = 40,
>>>>  	VHOST_USER_MAX = 41
>>>>  } VhostUserRequest;
>>>>
>>>> --
>>>> 2.26.2
>>>
>>
>> --
>> Adrián Moreno
> 

-- 
Adrián Moreno


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-07-06 10:45 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-02  8:32 [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 1/8] vhost: fix virtio ready flag check Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 2/8] vhost: refactor Virtio ready check Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 3/8] vhost: make some vDPA callbacks mandatory Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 4/8] vhost: check vDPA configuration succeed Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 5/8] vhost: add support for virtio status Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message Adrian Moreno
2020-07-05 13:15   ` Xia, Chenbo
2020-07-06  8:15     ` Adrian Moreno
2020-07-06  3:22   ` Xia, Chenbo
2020-07-06  8:48     ` Adrian Moreno
2020-07-06 10:29       ` Xia, Chenbo
2020-07-06 10:45         ` Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 7/8] vdpa/ifc: enable status protocol feature Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 8/8] vdpa/mlx5: " Adrian Moreno

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).