DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/5] Add support for GET/SET_STATUS on virtio-user pmd
@ 2020-07-15 17:18 Adrian Moreno
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 1/5] net/virtio: split virtio-net and virtio status Adrian Moreno
                   ` (4 more replies)
  0 siblings, 5 replies; 18+ messages in thread
From: Adrian Moreno @ 2020-07-15 17:18 UTC (permalink / raw)
  To: dev; +Cc: maxime.coquelin, zhihong.wang, amorenoz, chenbo.xia

Recently, two new messages have been added to the vhost-user protocol that make
the device initialization more robust.

VHOST_VIRTIO_SET_STATUS allows the driver to set the device status
VHOST_VIRTIO_GET_STATUS allows the driver to read the status back from
the device

This series implements these features in the virtio-user pmd

Adrian Moreno (4):
  net/virtio: split virtio-net and virtio status
  net/virtio: add DEVICE_NEEDS_RESET status bit
  net/virtio: add GET_STATUS support to virtio-user
  net/virtio: enable feature checking on virtio-user

Maxime Coquelin (1):
  net/virtio: add VIRTIO_SET_STATUS support to Virtio-user

 drivers/net/virtio/virtio_ethdev.c            | 13 ++--
 drivers/net/virtio/virtio_pci.h               | 13 ++--
 drivers/net/virtio/virtio_user/vhost.h        |  6 ++
 drivers/net/virtio/virtio_user/vhost_user.c   | 12 ++++
 .../net/virtio/virtio_user/virtio_user_dev.c  | 62 ++++++++++++++++++-
 .../net/virtio/virtio_user/virtio_user_dev.h  |  7 ++-
 drivers/net/virtio/virtio_user_ethdev.c       | 14 +++--
 7 files changed, 108 insertions(+), 19 deletions(-)

-- 
2.26.2


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

* [dpdk-dev] [PATCH 1/5] net/virtio: split virtio-net and virtio status
  2020-07-15 17:18 [dpdk-dev] [PATCH 0/5] Add support for GET/SET_STATUS on virtio-user pmd Adrian Moreno
@ 2020-07-15 17:18 ` Adrian Moreno
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit Adrian Moreno
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 18+ messages in thread
From: Adrian Moreno @ 2020-07-15 17:18 UTC (permalink / raw)
  To: dev; +Cc: maxime.coquelin, zhihong.wang, amorenoz, chenbo.xia

Currently, the same field is being used to store both the generic virtio
device status and the virtio-net-specific status.

This can become problematic since they have different sets of status
bits that may collide (e.g: VIRTIO_CONFIG_STATUS_ACK and
VIRTIO_NET_S_LINK_UP).

Split them in different fields.

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

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 56e638f8a..10b274d19 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -43,7 +43,10 @@ struct virtio_user_dev {
 	uint64_t	protocol_features; /* negotiated protocol features
 					    * (Vhost-user only)
 					    */
-	uint8_t		status;
+
+	uint8_t		virtio_net_status; /* virtio-net device status */
+	uint8_t		status;		   /* virtio device status */
+
 	uint16_t	port_id;
 	uint8_t		mac_addr[RTE_ETHER_ADDR_LEN];
 	char		path[PATH_MAX];
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index e51425c4f..5b06d8e89 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -205,7 +205,8 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 			}
 			r = recv(dev->vhostfd, buf, 128, MSG_PEEK);
 			if (r == 0 || (r < 0 && errno != EAGAIN)) {
-				dev->status &= (~VIRTIO_NET_S_LINK_UP);
+				dev->virtio_net_status &=
+					(~VIRTIO_NET_S_LINK_UP);
 				PMD_DRV_LOG(ERR, "virtio-user port %u is down",
 					    hw->port_id);
 
@@ -217,7 +218,7 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 						  virtio_user_delayed_handler,
 						  (void *)hw);
 			} else {
-				dev->status |= VIRTIO_NET_S_LINK_UP;
+				dev->virtio_net_status |= VIRTIO_NET_S_LINK_UP;
 			}
 			if (fcntl(dev->vhostfd, F_SETFL,
 					flags & ~O_NONBLOCK) == -1) {
@@ -225,12 +226,12 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 				return;
 			}
 		} else if (dev->is_server) {
-			dev->status &= (~VIRTIO_NET_S_LINK_UP);
+			dev->virtio_net_status &= (~VIRTIO_NET_S_LINK_UP);
 			if (virtio_user_server_reconnect(dev) >= 0)
-				dev->status |= VIRTIO_NET_S_LINK_UP;
+				dev->virtio_net_status |= VIRTIO_NET_S_LINK_UP;
 		}
 
-		*(uint16_t *)dst = dev->status;
+		*(uint16_t *)dst = dev->virtio_net_status;
 	}
 
 	if (offset == offsetof(struct virtio_net_config, max_virtqueue_pairs))
-- 
2.26.2


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

* [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
  2020-07-15 17:18 [dpdk-dev] [PATCH 0/5] Add support for GET/SET_STATUS on virtio-user pmd Adrian Moreno
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 1/5] net/virtio: split virtio-net and virtio status Adrian Moreno
@ 2020-07-15 17:18 ` Adrian Moreno
  2020-07-16  2:26   ` Xia, Chenbo
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user Adrian Moreno
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 18+ messages in thread
From: Adrian Moreno @ 2020-07-15 17:18 UTC (permalink / raw)
  To: dev; +Cc: maxime.coquelin, zhihong.wang, amorenoz, chenbo.xia

For the sake of completeness, add the definition of the missing status
bit in accordance with the virtio spec

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_pci.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
index 74ed77e33..ab61e911b 100644
--- a/drivers/net/virtio/virtio_pci.h
+++ b/drivers/net/virtio/virtio_pci.h
@@ -57,12 +57,13 @@ struct virtnet_ctl;
 #define VIRTIO_ID_9P       0x09
 
 /* Status byte for guest to report progress. */
-#define VIRTIO_CONFIG_STATUS_RESET     0x00
-#define VIRTIO_CONFIG_STATUS_ACK       0x01
-#define VIRTIO_CONFIG_STATUS_DRIVER    0x02
-#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04
-#define VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08
-#define VIRTIO_CONFIG_STATUS_FAILED    0x80
+#define VIRTIO_CONFIG_STATUS_RESET		0x00
+#define VIRTIO_CONFIG_STATUS_ACK		0x01
+#define VIRTIO_CONFIG_STATUS_DRIVER		0x02
+#define VIRTIO_CONFIG_STATUS_DRIVER_OK		0x04
+#define VIRTIO_CONFIG_STATUS_FEATURES_OK	0x08
+#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET	0x40
+#define VIRTIO_CONFIG_STATUS_FAILED		0x80
 
 /*
  * Each virtqueue indirect descriptor list must be physically contiguous.
-- 
2.26.2


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

* [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user
  2020-07-15 17:18 [dpdk-dev] [PATCH 0/5] Add support for GET/SET_STATUS on virtio-user pmd Adrian Moreno
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 1/5] net/virtio: split virtio-net and virtio status Adrian Moreno
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit Adrian Moreno
@ 2020-07-15 17:18 ` Adrian Moreno
  2020-07-16  3:15   ` Xia, Chenbo
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user Adrian Moreno
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 5/5] net/virtio: enable feature checking on virtio-user Adrian Moreno
  4 siblings, 1 reply; 18+ messages in thread
From: Adrian Moreno @ 2020-07-15 17:18 UTC (permalink / raw)
  To: dev; +Cc: maxime.coquelin, zhihong.wang, amorenoz, chenbo.xia

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

This patch adds support for VHOST_USER_SET_STATUS
request. It is used to make the backend aware of
Virtio devices status update.

It is useful for the backend to know when the Virtio
driver is done with the Virtio device configuration.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user/vhost.h        |  6 +++++
 drivers/net/virtio/virtio_user/vhost_user.c   | 10 ++++++++
 .../net/virtio/virtio_user/virtio_user_dev.c  | 23 ++++++++++++++++++-
 .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
 drivers/net/virtio/virtio_user_ethdev.c       |  1 +
 5 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/vhost.h b/drivers/net/virtio/virtio_user/vhost.h
index 260e1c308..8f49ef457 100644
--- a/drivers/net/virtio/virtio_user/vhost.h
+++ b/drivers/net/virtio/virtio_user/vhost.h
@@ -57,6 +57,10 @@ struct vhost_vring_addr {
 #define VHOST_USER_PROTOCOL_F_REPLY_ACK 3
 #endif
 
+#ifndef VHOST_USER_PROTOCOL_F_STATUS
+#define VHOST_USER_PROTOCOL_F_STATUS 16
+#endif
+
 enum vhost_user_request {
 	VHOST_USER_NONE = 0,
 	VHOST_USER_GET_FEATURES = 1,
@@ -77,6 +81,8 @@ enum vhost_user_request {
 	VHOST_USER_SET_PROTOCOL_FEATURES = 16,
 	VHOST_USER_GET_QUEUE_NUM = 17,
 	VHOST_USER_SET_VRING_ENABLE = 18,
+	VHOST_USER_SET_STATUS = 39,
+	VHOST_USER_GET_STATUS = 40,
 	VHOST_USER_MAX
 };
 
diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index 631d0e353..2332e01d1 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -244,6 +244,8 @@ const char * const vhost_msg_strings[] = {
 	[VHOST_USER_SET_VRING_ENABLE] = "VHOST_SET_VRING_ENABLE",
 	[VHOST_USER_GET_PROTOCOL_FEATURES] = "VHOST_USER_GET_PROTOCOL_FEATURES",
 	[VHOST_USER_SET_PROTOCOL_FEATURES] = "VHOST_USER_SET_PROTOCOL_FEATURES",
+	[VHOST_USER_SET_STATUS] = "VHOST_SET_STATUS",
+	[VHOST_USER_GET_STATUS] = "VHOST_GET_STATUS",
 };
 
 static int
@@ -280,6 +282,14 @@ vhost_user_sock(struct virtio_user_dev *dev,
 		need_reply = 1;
 		break;
 
+	case VHOST_USER_SET_STATUS:
+		if (!(dev->protocol_features &
+				(1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
+			return 0;
+
+		if (has_reply_ack)
+			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
+		/* Fallthrough */
 	case VHOST_USER_SET_FEATURES:
 	case VHOST_USER_SET_PROTOCOL_FEATURES:
 	case VHOST_USER_SET_LOG_BASE:
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 0a6991bcc..2c400a448 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
 
 #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
 	(1ULL << VHOST_USER_PROTOCOL_F_MQ |		\
-	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
+	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |	\
+	 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
 
 int
 virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
@@ -783,3 +784,23 @@ virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx)
 		__atomic_add_fetch(&vring->used->idx, 1, __ATOMIC_RELAXED);
 	}
 }
+
+int
+virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status)
+{
+	int ret;
+	uint64_t arg = status;
+
+	/* Vhost-user only for now */
+	if (!is_vhost_user_by_type(dev->path))
+		return 0;
+
+	if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
+		return 0;
+
+	ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg);
+	if (ret)
+		return -1;
+
+	return 0;
+}
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 10b274d19..a76d7cfaf 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -74,4 +74,5 @@ 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);
 #endif
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 5b06d8e89..e52f11341 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -273,6 +273,7 @@ virtio_user_set_status(struct virtio_hw *hw, uint8_t status)
 	else if (status == VIRTIO_CONFIG_STATUS_RESET)
 		virtio_user_reset(hw);
 	dev->status = status;
+	virtio_user_send_status_update(dev, status);
 }
 
 static uint8_t
-- 
2.26.2


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

* [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user
  2020-07-15 17:18 [dpdk-dev] [PATCH 0/5] Add support for GET/SET_STATUS on virtio-user pmd Adrian Moreno
                   ` (2 preceding siblings ...)
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user Adrian Moreno
@ 2020-07-15 17:18 ` Adrian Moreno
  2020-07-16  3:31   ` Xia, Chenbo
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 5/5] net/virtio: enable feature checking on virtio-user Adrian Moreno
  4 siblings, 1 reply; 18+ messages in thread
From: Adrian Moreno @ 2020-07-15 17:18 UTC (permalink / raw)
  To: dev; +Cc: maxime.coquelin, zhihong.wang, amorenoz, chenbo.xia

This patch adds support for VHOST_USER_GET_STATUS request.

Only vhost-user backed is supported for now

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user/vhost_user.c   |  2 +
 .../net/virtio/virtio_user/virtio_user_dev.c  | 39 +++++++++++++++++++
 .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
 drivers/net/virtio/virtio_user_ethdev.c       |  2 +
 4 files changed, 44 insertions(+)

diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index 2332e01d1..43c630e47 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -279,6 +279,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
 	switch (req) {
 	case VHOST_USER_GET_FEATURES:
 	case VHOST_USER_GET_PROTOCOL_FEATURES:
+	case VHOST_USER_GET_STATUS:
 		need_reply = 1;
 		break;
 
@@ -373,6 +374,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
 
 		switch (req) {
 		case VHOST_USER_GET_FEATURES:
+		case VHOST_USER_GET_STATUS:
 		case VHOST_USER_GET_PROTOCOL_FEATURES:
 			if (msg.size != sizeof(m.payload.u64)) {
 				PMD_DRV_LOG(ERR, "Received bad msg size");
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 2c400a448..613caef56 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -804,3 +804,42 @@ virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status)
 
 	return 0;
 }
+
+int
+virtio_user_update_status(struct virtio_user_dev *dev)
+{
+	uint64_t ret;
+
+	/* Vhost-user only for now */
+	if (!is_vhost_user_by_type(dev->path))
+		return 0;
+
+	if (!(dev->protocol_features & (1UL << VHOST_USER_PROTOCOL_F_STATUS)))
+		return 0;
+
+	if (dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret) < 0) {
+		PMD_INIT_LOG(ERR, "GET_STATUS failed: %s", strerror(errno));
+		return -1;
+	}
+	if (ret > UINT8_MAX) {
+		PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS response 0x%" PRIx64 "\n", ret);
+		return -1;
+	}
+
+	dev->status = ret;
+	PMD_INIT_LOG(DEBUG, "Updated Device Status(0x%08x):\n"
+			"\t-ACKNOWLEDGE: %u\n"
+			"\t-DRIVER: %u\n"
+			"\t-DRIVER_OK: %u\n"
+			"\t-FEATURES_OK: %u\n"
+			"\t-DEVICE_NEED_RESET: %u\n"
+			"\t-FAILED: %u\n",
+			dev->status,
+			!!(dev->status & VIRTIO_CONFIG_STATUS_ACK),
+			!!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER),
+			!!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER_OK),
+			!!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK),
+			!!(dev->status & VIRTIO_CONFIG_STATUS_DEV_NEED_RESET),
+			!!(dev->status & VIRTIO_CONFIG_STATUS_FAILED));
+	return 0;
+}
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index a76d7cfaf..1a045af6f 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -75,4 +75,5 @@ 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);
 #endif
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index e52f11341..40330deed 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -281,6 +281,8 @@ virtio_user_get_status(struct virtio_hw *hw)
 {
 	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
 
+	virtio_user_update_status(dev);
+
 	return dev->status;
 }
 
-- 
2.26.2


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

* [dpdk-dev] [PATCH 5/5] net/virtio: enable feature checking on virtio-user
  2020-07-15 17:18 [dpdk-dev] [PATCH 0/5] Add support for GET/SET_STATUS on virtio-user pmd Adrian Moreno
                   ` (3 preceding siblings ...)
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user Adrian Moreno
@ 2020-07-15 17:18 ` Adrian Moreno
  4 siblings, 0 replies; 18+ messages in thread
From: Adrian Moreno @ 2020-07-15 17:18 UTC (permalink / raw)
  To: dev; +Cc: maxime.coquelin, zhihong.wang, amorenoz, chenbo.xia

virtio 1.0 introduced a mechanism for the driver to verify that the
feature bits it sets are accepted by the device. This mechanism consists
in setting the VIRTIO_STATUS_FEATURE_OK status bit and re-reading it,
whitch gives a chance for the device to clear it if the the features
were not accepted.

This is currently being done only in modern virtio-pci devices but since
the appropriate vhost-user messages have been added, it can also be done
in virtio-user (vhost-user only).

This patch activates this mechanism on virtio-user.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index dc0093bdf..9063bfeb2 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1355,12 +1355,13 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
 	PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64,
 		hw->guest_features);
 
-	if (hw->modern) {
-		if (!vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
-			PMD_INIT_LOG(ERR,
-				"VIRTIO_F_VERSION_1 features is not enabled.");
-			return -1;
-		}
+	if (hw->modern && !vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
+		PMD_INIT_LOG(ERR,
+			"VIRTIO_F_VERSION_1 features is not enabled.");
+		return -1;
+	}
+
+	if (hw->modern || hw->virtio_user_dev) {
 		vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
 		if (!(vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_FEATURES_OK)) {
 			PMD_INIT_LOG(ERR,
-- 
2.26.2


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

* Re: [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit Adrian Moreno
@ 2020-07-16  2:26   ` Xia, Chenbo
  2020-07-16  7:34     ` Adrian Moreno
  0 siblings, 1 reply; 18+ messages in thread
From: Xia, Chenbo @ 2020-07-16  2:26 UTC (permalink / raw)
  To: Adrian Moreno, dev; +Cc: maxime.coquelin, Wang, Zhihong, Li, Miao

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 1:18 AM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
> 
> For the sake of completeness, add the definition of the missing status bit in
> accordance with the virtio spec
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_pci.h | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h index
> 74ed77e33..ab61e911b 100644
> --- a/drivers/net/virtio/virtio_pci.h
> +++ b/drivers/net/virtio/virtio_pci.h
> @@ -57,12 +57,13 @@ struct virtnet_ctl;
>  #define VIRTIO_ID_9P       0x09
> 
>  /* Status byte for guest to report progress. */
> -#define VIRTIO_CONFIG_STATUS_RESET     0x00
> -#define VIRTIO_CONFIG_STATUS_ACK       0x01
> -#define VIRTIO_CONFIG_STATUS_DRIVER    0x02
> -#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 -#define
> VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08
> -#define VIRTIO_CONFIG_STATUS_FAILED    0x80
> +#define VIRTIO_CONFIG_STATUS_RESET		0x00
> +#define VIRTIO_CONFIG_STATUS_ACK		0x01
> +#define VIRTIO_CONFIG_STATUS_DRIVER		0x02
> +#define VIRTIO_CONFIG_STATUS_DRIVER_OK		0x04
> +#define VIRTIO_CONFIG_STATUS_FEATURES_OK	0x08
> +#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET	0x40
> +#define VIRTIO_CONFIG_STATUS_FAILED		0x80

Do we need to delete ' VIRTIO_CONFIG_STATUS_RESET'? I see vhost lib does not
have it now. And I read virtio 1.1 spec and find the description of writing 0 to
reset device is deleted. I think virtio 1.1 spec is not very clear about reset status.
Does 'DEV_NEED_RESET' equal old 'RESET'?

Thanks!
Chenbo 

> 
>  /*
>   * Each virtqueue indirect descriptor list must be physically contiguous.
> --
> 2.26.2


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

* Re: [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user Adrian Moreno
@ 2020-07-16  3:15   ` Xia, Chenbo
  2020-07-16  7:43     ` Adrian Moreno
  0 siblings, 1 reply; 18+ messages in thread
From: Xia, Chenbo @ 2020-07-16  3:15 UTC (permalink / raw)
  To: Adrian Moreno, dev; +Cc: maxime.coquelin, Wang, Zhihong

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 1:18 AM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user
> 
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> This patch adds support for VHOST_USER_SET_STATUS request. It is used to
> make the backend aware of Virtio devices status update.
> 
> It is useful for the backend to know when the Virtio driver is done with the Virtio
> device configuration.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/vhost.h        |  6 +++++
>  drivers/net/virtio/virtio_user/vhost_user.c   | 10 ++++++++
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 23 ++++++++++++++++++-
>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
>  5 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost.h
> b/drivers/net/virtio/virtio_user/vhost.h
> index 260e1c308..8f49ef457 100644
> --- a/drivers/net/virtio/virtio_user/vhost.h
> +++ b/drivers/net/virtio/virtio_user/vhost.h
> @@ -57,6 +57,10 @@ struct vhost_vring_addr {  #define
> VHOST_USER_PROTOCOL_F_REPLY_ACK 3  #endif
> 
> +#ifndef VHOST_USER_PROTOCOL_F_STATUS
> +#define VHOST_USER_PROTOCOL_F_STATUS 16 #endif
> +
>  enum vhost_user_request {
>  	VHOST_USER_NONE = 0,
>  	VHOST_USER_GET_FEATURES = 1,
> @@ -77,6 +81,8 @@ enum vhost_user_request {
>  	VHOST_USER_SET_PROTOCOL_FEATURES = 16,
>  	VHOST_USER_GET_QUEUE_NUM = 17,
>  	VHOST_USER_SET_VRING_ENABLE = 18,
> +	VHOST_USER_SET_STATUS = 39,
> +	VHOST_USER_GET_STATUS = 40,
>  	VHOST_USER_MAX
>  };
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
> b/drivers/net/virtio/virtio_user/vhost_user.c
> index 631d0e353..2332e01d1 100644
> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> @@ -244,6 +244,8 @@ const char * const vhost_msg_strings[] = {
>  	[VHOST_USER_SET_VRING_ENABLE] = "VHOST_SET_VRING_ENABLE",
>  	[VHOST_USER_GET_PROTOCOL_FEATURES] =
> "VHOST_USER_GET_PROTOCOL_FEATURES",
>  	[VHOST_USER_SET_PROTOCOL_FEATURES] =
> "VHOST_USER_SET_PROTOCOL_FEATURES",
> +	[VHOST_USER_SET_STATUS] = "VHOST_SET_STATUS",
> +	[VHOST_USER_GET_STATUS] = "VHOST_GET_STATUS",
>  };
> 
>  static int
> @@ -280,6 +282,14 @@ vhost_user_sock(struct virtio_user_dev *dev,
>  		need_reply = 1;
>  		break;
> 
> +	case VHOST_USER_SET_STATUS:
> +		if (!(dev->protocol_features &
> +				(1ULL <<
> VHOST_USER_PROTOCOL_F_STATUS)))
> +			return 0;
> +
> +		if (has_reply_ack)
> +			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> +		/* Fallthrough */

There's a coding style issue here:
WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough comment.
Could you fix this?

>  	case VHOST_USER_SET_FEATURES:
>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
>  	case VHOST_USER_SET_LOG_BASE:
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 0a6991bcc..2c400a448 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
> 
>  #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
>  	(1ULL << VHOST_USER_PROTOCOL_F_MQ |		\
> -	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
> +	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |	\
> +	 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
> 
>  int
>  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, @@ -
> 783,3 +784,23 @@ virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t
> queue_idx)
>  		__atomic_add_fetch(&vring->used->idx, 1,
> __ATOMIC_RELAXED);
>  	}
>  }
> +
> +int
> +virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t
> +status) {
> +	int ret;
> +	uint64_t arg = status;
> +
> +	/* Vhost-user only for now */
> +	if (!is_vhost_user_by_type(dev->path))
> +		return 0;
> +
> +	if (!(dev->protocol_features & (1ULL <<
> VHOST_USER_PROTOCOL_F_STATUS)))
> +		return 0;
> +
> +	ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg);
> +	if (ret)
> +		return -1;

Do you think we should add a log here to show if SET_STAUTS failed?

Thanks!
Chenbo

> +
> +	return 0;
> +}
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index 10b274d19..a76d7cfaf 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -74,4 +74,5 @@ 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);
>  #endif
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index 5b06d8e89..e52f11341 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -273,6 +273,7 @@ virtio_user_set_status(struct virtio_hw *hw, uint8_t
> status)
>  	else if (status == VIRTIO_CONFIG_STATUS_RESET)
>  		virtio_user_reset(hw);
>  	dev->status = status;
> +	virtio_user_send_status_update(dev, status);
>  }
> 
>  static uint8_t
> --
> 2.26.2


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

* Re: [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user
  2020-07-15 17:18 ` [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user Adrian Moreno
@ 2020-07-16  3:31   ` Xia, Chenbo
  2020-07-16  7:53     ` Adrian Moreno
  0 siblings, 1 reply; 18+ messages in thread
From: Xia, Chenbo @ 2020-07-16  3:31 UTC (permalink / raw)
  To: Adrian Moreno, dev; +Cc: maxime.coquelin, Wang, Zhihong

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 1:18 AM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user
> 
> This patch adds support for VHOST_USER_GET_STATUS request.
> 
> Only vhost-user backed is supported for now
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/vhost_user.c   |  2 +
>  .../net/virtio/virtio_user/virtio_user_dev.c  | 39
> +++++++++++++++++++  .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>  drivers/net/virtio/virtio_user_ethdev.c       |  2 +
>  4 files changed, 44 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
> b/drivers/net/virtio/virtio_user/vhost_user.c
> index 2332e01d1..43c630e47 100644
> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> @@ -279,6 +279,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
>  	switch (req) {
>  	case VHOST_USER_GET_FEATURES:
>  	case VHOST_USER_GET_PROTOCOL_FEATURES:
> +	case VHOST_USER_GET_STATUS:
>  		need_reply = 1;
>  		break;
> 
> @@ -373,6 +374,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
> 
>  		switch (req) {
>  		case VHOST_USER_GET_FEATURES:
> +		case VHOST_USER_GET_STATUS:
>  		case VHOST_USER_GET_PROTOCOL_FEATURES:
>  			if (msg.size != sizeof(m.payload.u64)) {
>  				PMD_DRV_LOG(ERR, "Received bad msg size");
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 2c400a448..613caef56 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -804,3 +804,42 @@ virtio_user_send_status_update(struct virtio_user_dev
> *dev, uint8_t status)
> 
>  	return 0;
>  }
> +
> +int
> +virtio_user_update_status(struct virtio_user_dev *dev) {
> +	uint64_t ret;
> +
> +	/* Vhost-user only for now */
> +	if (!is_vhost_user_by_type(dev->path))
> +		return 0;
> +
> +	if (!(dev->protocol_features & (1UL <<
> VHOST_USER_PROTOCOL_F_STATUS)))
> +		return 0;
> +
> +	if (dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret) < 0) {
> +		PMD_INIT_LOG(ERR, "GET_STATUS failed: %s", strerror(errno));
> +		return -1;
> +	}
> +	if (ret > UINT8_MAX) {
> +		PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS
> response 0x%" PRIx64 "\n", ret);

There's a coding style issue here:
CHECK:CAMELCASE: Avoid CamelCase: <PRIx64>
Could you fix this?

Btw, in case you don't know, there's a script in dpdk for check these issues:
devtools/checkpatches.sh. 😊

Thanks!
Chenbo

> +		return -1;
> +	}
> +
> +	dev->status = ret;
> +	PMD_INIT_LOG(DEBUG, "Updated Device Status(0x%08x):\n"
> +			"\t-ACKNOWLEDGE: %u\n"
> +			"\t-DRIVER: %u\n"
> +			"\t-DRIVER_OK: %u\n"
> +			"\t-FEATURES_OK: %u\n"
> +			"\t-DEVICE_NEED_RESET: %u\n"
> +			"\t-FAILED: %u\n",
> +			dev->status,
> +			!!(dev->status & VIRTIO_CONFIG_STATUS_ACK),
> +			!!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER),
> +			!!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER_OK),
> +			!!(dev->status &
> VIRTIO_CONFIG_STATUS_FEATURES_OK),
> +			!!(dev->status &
> VIRTIO_CONFIG_STATUS_DEV_NEED_RESET),
> +			!!(dev->status & VIRTIO_CONFIG_STATUS_FAILED));
> +	return 0;
> +}
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index a76d7cfaf..1a045af6f 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -75,4 +75,5 @@ 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);
>  #endif
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index e52f11341..40330deed 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -281,6 +281,8 @@ virtio_user_get_status(struct virtio_hw *hw)  {
>  	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
> 
> +	virtio_user_update_status(dev);
> +
>  	return dev->status;
>  }
> 
> --
> 2.26.2


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

* Re: [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
  2020-07-16  2:26   ` Xia, Chenbo
@ 2020-07-16  7:34     ` Adrian Moreno
  2020-07-16  8:14       ` Xia, Chenbo
  0 siblings, 1 reply; 18+ messages in thread
From: Adrian Moreno @ 2020-07-16  7:34 UTC (permalink / raw)
  To: Xia, Chenbo, dev; +Cc: maxime.coquelin, Wang, Zhihong, Li, Miao

Hi,

On 7/16/20 4:26 AM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Thursday, July 16, 2020 1:18 AM
>> To: dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
>> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
>>
>> For the sake of completeness, add the definition of the missing status bit in
>> accordance with the virtio spec
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_pci.h | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h index
>> 74ed77e33..ab61e911b 100644
>> --- a/drivers/net/virtio/virtio_pci.h
>> +++ b/drivers/net/virtio/virtio_pci.h
>> @@ -57,12 +57,13 @@ struct virtnet_ctl;
>>  #define VIRTIO_ID_9P       0x09
>>
>>  /* Status byte for guest to report progress. */
>> -#define VIRTIO_CONFIG_STATUS_RESET     0x00
>> -#define VIRTIO_CONFIG_STATUS_ACK       0x01
>> -#define VIRTIO_CONFIG_STATUS_DRIVER    0x02
>> -#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 -#define
>> VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08
>> -#define VIRTIO_CONFIG_STATUS_FAILED    0x80
>> +#define VIRTIO_CONFIG_STATUS_RESET		0x00
>> +#define VIRTIO_CONFIG_STATUS_ACK		0x01
>> +#define VIRTIO_CONFIG_STATUS_DRIVER		0x02
>> +#define VIRTIO_CONFIG_STATUS_DRIVER_OK		0x04
>> +#define VIRTIO_CONFIG_STATUS_FEATURES_OK	0x08
>> +#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET	0x40
>> +#define VIRTIO_CONFIG_STATUS_FAILED		0x80
> 
> Do we need to delete ' VIRTIO_CONFIG_STATUS_RESET'? I see vhost lib does not
> have it now. And I read virtio 1.1 spec and find the description of writing 0 to
> reset device is deleted. I think virtio 1.1 spec is not very clear about reset status.
> Does 'DEV_NEED_RESET' equal old 'RESET'?
> 
In virtio 1.1
"2.1.2 Device Requirements: Device Status Field

The device MUST initialize device status to 0 upon reset.
...
"
So I think having "#define VIRTIO_CONFIG_STATUS_RESET 0x00" is still useful to
understand what is going on in:

void
vtpci_reset(struct virtio_hw *hw)
{
	VTPCI_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
	/* flush status write */
	VTPCI_OPS(hw)->get_status(hw);
}

DEV_NEED_RESET is used for the device to notify that it has encountered an
unrecoverable error. Therefore, the driver would never
"set_status(VIRTIO_CONFIG_STATUS_DEV_NEED_RESET)"; instead, it should read the
status and if this bit is set, reset the device.


> Thanks!
> Chenbo 
> 
>>
>>  /*
>>   * Each virtqueue indirect descriptor list must be physically contiguous.
>> --
>> 2.26.2
> 

-- 
Adrián Moreno


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

* Re: [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user
  2020-07-16  3:15   ` Xia, Chenbo
@ 2020-07-16  7:43     ` Adrian Moreno
  2020-07-16  8:58       ` Xia, Chenbo
  0 siblings, 1 reply; 18+ messages in thread
From: Adrian Moreno @ 2020-07-16  7:43 UTC (permalink / raw)
  To: Xia, Chenbo, dev; +Cc: maxime.coquelin, Wang, Zhihong



On 7/16/20 5:15 AM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Thursday, July 16, 2020 1:18 AM
>> To: dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
>> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user
>>
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> This patch adds support for VHOST_USER_SET_STATUS request. It is used to
>> make the backend aware of Virtio devices status update.
>>
>> It is useful for the backend to know when the Virtio driver is done with the Virtio
>> device configuration.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_user/vhost.h        |  6 +++++
>>  drivers/net/virtio/virtio_user/vhost_user.c   | 10 ++++++++
>>  .../net/virtio/virtio_user/virtio_user_dev.c  | 23 ++++++++++++++++++-
>>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
>>  5 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
>> b/drivers/net/virtio/virtio_user/vhost.h
>> index 260e1c308..8f49ef457 100644
>> --- a/drivers/net/virtio/virtio_user/vhost.h
>> +++ b/drivers/net/virtio/virtio_user/vhost.h
>> @@ -57,6 +57,10 @@ struct vhost_vring_addr {  #define
>> VHOST_USER_PROTOCOL_F_REPLY_ACK 3  #endif
>>
>> +#ifndef VHOST_USER_PROTOCOL_F_STATUS
>> +#define VHOST_USER_PROTOCOL_F_STATUS 16 #endif
>> +
>>  enum vhost_user_request {
>>  	VHOST_USER_NONE = 0,
>>  	VHOST_USER_GET_FEATURES = 1,
>> @@ -77,6 +81,8 @@ enum vhost_user_request {
>>  	VHOST_USER_SET_PROTOCOL_FEATURES = 16,
>>  	VHOST_USER_GET_QUEUE_NUM = 17,
>>  	VHOST_USER_SET_VRING_ENABLE = 18,
>> +	VHOST_USER_SET_STATUS = 39,
>> +	VHOST_USER_GET_STATUS = 40,
>>  	VHOST_USER_MAX
>>  };
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
>> b/drivers/net/virtio/virtio_user/vhost_user.c
>> index 631d0e353..2332e01d1 100644
>> --- a/drivers/net/virtio/virtio_user/vhost_user.c
>> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
>> @@ -244,6 +244,8 @@ const char * const vhost_msg_strings[] = {
>>  	[VHOST_USER_SET_VRING_ENABLE] = "VHOST_SET_VRING_ENABLE",
>>  	[VHOST_USER_GET_PROTOCOL_FEATURES] =
>> "VHOST_USER_GET_PROTOCOL_FEATURES",
>>  	[VHOST_USER_SET_PROTOCOL_FEATURES] =
>> "VHOST_USER_SET_PROTOCOL_FEATURES",
>> +	[VHOST_USER_SET_STATUS] = "VHOST_SET_STATUS",
>> +	[VHOST_USER_GET_STATUS] = "VHOST_GET_STATUS",
>>  };
>>
>>  static int
>> @@ -280,6 +282,14 @@ vhost_user_sock(struct virtio_user_dev *dev,
>>  		need_reply = 1;
>>  		break;
>>
>> +	case VHOST_USER_SET_STATUS:
>> +		if (!(dev->protocol_features &
>> +				(1ULL <<
>> VHOST_USER_PROTOCOL_F_STATUS)))
>> +			return 0;
>> +
>> +		if (has_reply_ack)
>> +			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
>> +		/* Fallthrough */
> 
> There's a coding style issue here:
> WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough comment.
> Could you fix this?
> 
"fallthrough" is not defined. I think this macro is defined in the linux kernel
(where checkpatches.pl comes from?). We could define the same macro that would
depend in compiler support for __falthrough__ attribute.

In any case, this is not the only case:

$ find -name *.c | xargs grep -i "/\* fallthrough \*/" | wc -l
62

So maybe this warning is new?
Should I change all of them together in another patch?

>>  	case VHOST_USER_SET_FEATURES:
>>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
>>  	case VHOST_USER_SET_LOG_BASE:
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> index 0a6991bcc..2c400a448 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>>
>>  #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
>>  	(1ULL << VHOST_USER_PROTOCOL_F_MQ |		\
>> -	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
>> +	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |	\
>> +	 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
>>
>>  int
>>  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, @@ -
>> 783,3 +784,23 @@ virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t
>> queue_idx)
>>  		__atomic_add_fetch(&vring->used->idx, 1,
>> __ATOMIC_RELAXED);
>>  	}
>>  }
>> +
>> +int
>> +virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t
>> +status) {
>> +	int ret;
>> +	uint64_t arg = status;
>> +
>> +	/* Vhost-user only for now */
>> +	if (!is_vhost_user_by_type(dev->path))
>> +		return 0;
>> +
>> +	if (!(dev->protocol_features & (1ULL <<
>> VHOST_USER_PROTOCOL_F_STATUS)))
>> +		return 0;
>> +
>> +	ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg);
>> +	if (ret)
>> +		return -1;
> 
> Do you think we should add a log here to show if SET_STAUTS failed?
> 
Good idea! Will do in the next version. Thanks

> Thanks!
> Chenbo
> 
>> +
>> +	return 0;
>> +}
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> index 10b274d19..a76d7cfaf 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> @@ -74,4 +74,5 @@ 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);
>>  #endif
>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>> b/drivers/net/virtio/virtio_user_ethdev.c
>> index 5b06d8e89..e52f11341 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -273,6 +273,7 @@ virtio_user_set_status(struct virtio_hw *hw, uint8_t
>> status)
>>  	else if (status == VIRTIO_CONFIG_STATUS_RESET)
>>  		virtio_user_reset(hw);
>>  	dev->status = status;
>> +	virtio_user_send_status_update(dev, status);
>>  }
>>
>>  static uint8_t
>> --
>> 2.26.2
> 

-- 
Adrián Moreno


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

* Re: [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user
  2020-07-16  3:31   ` Xia, Chenbo
@ 2020-07-16  7:53     ` Adrian Moreno
  2020-07-16  8:18       ` David Marchand
  0 siblings, 1 reply; 18+ messages in thread
From: Adrian Moreno @ 2020-07-16  7:53 UTC (permalink / raw)
  To: Xia, Chenbo, dev; +Cc: maxime.coquelin, Wang, Zhihong, David Marchand



On 7/16/20 5:31 AM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Thursday, July 16, 2020 1:18 AM
>> To: dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
>> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user
>>
>> This patch adds support for VHOST_USER_GET_STATUS request.
>>
>> Only vhost-user backed is supported for now
>>
>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_user/vhost_user.c   |  2 +
>>  .../net/virtio/virtio_user/virtio_user_dev.c  | 39
>> +++++++++++++++++++  .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>>  drivers/net/virtio/virtio_user_ethdev.c       |  2 +
>>  4 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
>> b/drivers/net/virtio/virtio_user/vhost_user.c
>> index 2332e01d1..43c630e47 100644
>> --- a/drivers/net/virtio/virtio_user/vhost_user.c
>> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
>> @@ -279,6 +279,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
>>  	switch (req) {
>>  	case VHOST_USER_GET_FEATURES:
>>  	case VHOST_USER_GET_PROTOCOL_FEATURES:
>> +	case VHOST_USER_GET_STATUS:
>>  		need_reply = 1;
>>  		break;
>>
>> @@ -373,6 +374,7 @@ vhost_user_sock(struct virtio_user_dev *dev,
>>
>>  		switch (req) {
>>  		case VHOST_USER_GET_FEATURES:
>> +		case VHOST_USER_GET_STATUS:
>>  		case VHOST_USER_GET_PROTOCOL_FEATURES:
>>  			if (msg.size != sizeof(m.payload.u64)) {
>>  				PMD_DRV_LOG(ERR, "Received bad msg size");
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> index 2c400a448..613caef56 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -804,3 +804,42 @@ virtio_user_send_status_update(struct virtio_user_dev
>> *dev, uint8_t status)
>>
>>  	return 0;
>>  }
>> +
>> +int
>> +virtio_user_update_status(struct virtio_user_dev *dev) {
>> +	uint64_t ret;
>> +
>> +	/* Vhost-user only for now */
>> +	if (!is_vhost_user_by_type(dev->path))
>> +		return 0;
>> +
>> +	if (!(dev->protocol_features & (1UL <<
>> VHOST_USER_PROTOCOL_F_STATUS)))
>> +		return 0;
>> +
>> +	if (dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret) < 0) {
>> +		PMD_INIT_LOG(ERR, "GET_STATUS failed: %s", strerror(errno));
>> +		return -1;
>> +	}
>> +	if (ret > UINT8_MAX) {
>> +		PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS
>> response 0x%" PRIx64 "\n", ret);
> 
> There's a coding style issue here:
> CHECK:CAMELCASE: Avoid CamelCase: <PRIx64>
> Could you fix this?

I did see this one as well. It also seems new because PRIx64 is widely used in DPDK.

find -iname *.c | xargs grep -i "PRIx64" | wc -l
520

So, ether we have to modify the script to ignore this one or change all 520
cases, right?

+ David, what do you think?

> 
> Btw, in case you don't know, there's a script in dpdk for check these issues:
> devtools/checkpatches.sh. 😊
> 
> Thanks!
> Chenbo
> 
>> +		return -1;
>> +	}
>> +
>> +	dev->status = ret;
>> +	PMD_INIT_LOG(DEBUG, "Updated Device Status(0x%08x):\n"
>> +			"\t-ACKNOWLEDGE: %u\n"
>> +			"\t-DRIVER: %u\n"
>> +			"\t-DRIVER_OK: %u\n"
>> +			"\t-FEATURES_OK: %u\n"
>> +			"\t-DEVICE_NEED_RESET: %u\n"
>> +			"\t-FAILED: %u\n",
>> +			dev->status,
>> +			!!(dev->status & VIRTIO_CONFIG_STATUS_ACK),
>> +			!!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER),
>> +			!!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER_OK),
>> +			!!(dev->status &
>> VIRTIO_CONFIG_STATUS_FEATURES_OK),
>> +			!!(dev->status &
>> VIRTIO_CONFIG_STATUS_DEV_NEED_RESET),
>> +			!!(dev->status & VIRTIO_CONFIG_STATUS_FAILED));
>> +	return 0;
>> +}
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> index a76d7cfaf..1a045af6f 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> @@ -75,4 +75,5 @@ 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);
>>  #endif
>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>> b/drivers/net/virtio/virtio_user_ethdev.c
>> index e52f11341..40330deed 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -281,6 +281,8 @@ virtio_user_get_status(struct virtio_hw *hw)  {
>>  	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
>>
>> +	virtio_user_update_status(dev);
>> +
>>  	return dev->status;
>>  }
>>
>> --
>> 2.26.2
> 

-- 
Adrián Moreno


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

* Re: [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
  2020-07-16  7:34     ` Adrian Moreno
@ 2020-07-16  8:14       ` Xia, Chenbo
  0 siblings, 0 replies; 18+ messages in thread
From: Xia, Chenbo @ 2020-07-16  8:14 UTC (permalink / raw)
  To: Adrian Moreno, dev; +Cc: maxime.coquelin, Wang, Zhihong, Li, Miao

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 3:34 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> Li, Miao <miao.li@intel.com>
> Subject: Re: [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
> 
> Hi,
> 
> On 7/16/20 4:26 AM, Xia, Chenbo wrote:
> > Hi Adrian,
> >
> >> -----Original Message-----
> >> From: Adrian Moreno <amorenoz@redhat.com>
> >> Sent: Thursday, July 16, 2020 1:18 AM
> >> To: dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> >> <zhihong.wang@intel.com>; amorenoz@redhat.com; Xia, Chenbo
> >> <chenbo.xia@intel.com>
> >> Subject: [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
> >>
> >> For the sake of completeness, add the definition of the missing
> >> status bit in accordance with the virtio spec
> >>
> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> >> ---
> >>  drivers/net/virtio/virtio_pci.h | 13 +++++++------
> >>  1 file changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/net/virtio/virtio_pci.h
> >> b/drivers/net/virtio/virtio_pci.h index 74ed77e33..ab61e911b 100644
> >> --- a/drivers/net/virtio/virtio_pci.h
> >> +++ b/drivers/net/virtio/virtio_pci.h
> >> @@ -57,12 +57,13 @@ struct virtnet_ctl;
> >>  #define VIRTIO_ID_9P       0x09
> >>
> >>  /* Status byte for guest to report progress. */
> >> -#define VIRTIO_CONFIG_STATUS_RESET     0x00
> >> -#define VIRTIO_CONFIG_STATUS_ACK       0x01
> >> -#define VIRTIO_CONFIG_STATUS_DRIVER    0x02
> >> -#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 -#define
> >> VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08
> >> -#define VIRTIO_CONFIG_STATUS_FAILED    0x80
> >> +#define VIRTIO_CONFIG_STATUS_RESET		0x00
> >> +#define VIRTIO_CONFIG_STATUS_ACK		0x01
> >> +#define VIRTIO_CONFIG_STATUS_DRIVER		0x02
> >> +#define VIRTIO_CONFIG_STATUS_DRIVER_OK		0x04
> >> +#define VIRTIO_CONFIG_STATUS_FEATURES_OK	0x08
> >> +#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET	0x40
> >> +#define VIRTIO_CONFIG_STATUS_FAILED		0x80
> >
> > Do we need to delete ' VIRTIO_CONFIG_STATUS_RESET'? I see vhost lib
> > does not have it now. And I read virtio 1.1 spec and find the
> > description of writing 0 to reset device is deleted. I think virtio 1.1 spec is not
> very clear about reset status.
> > Does 'DEV_NEED_RESET' equal old 'RESET'?
> >
> In virtio 1.1
> "2.1.2 Device Requirements: Device Status Field
> 
> The device MUST initialize device status to 0 upon reset.
> ...
> "
> So I think having "#define VIRTIO_CONFIG_STATUS_RESET 0x00" is still useful to
> understand what is going on in:
> 
> void
> vtpci_reset(struct virtio_hw *hw)
> {
> 	VTPCI_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
> 	/* flush status write */
> 	VTPCI_OPS(hw)->get_status(hw);
> }
> 
> DEV_NEED_RESET is used for the device to notify that it has encountered an
> unrecoverable error. Therefore, the driver would never
> "set_status(VIRTIO_CONFIG_STATUS_DEV_NEED_RESET)"; instead, it should
> read the status and if this bit is set, reset the device.

Yes, you are correct! I missed that part. Btw, should we add STATUS_RESET to
Vhost lib? I see there's no reset status now.

Thanks!
Chenbo

> 
> 
> > Thanks!
> > Chenbo
> >
> >>
> >>  /*
> >>   * Each virtqueue indirect descriptor list must be physically contiguous.
> >> --
> >> 2.26.2
> >
> 
> --
> Adrián Moreno


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

* Re: [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user
  2020-07-16  7:53     ` Adrian Moreno
@ 2020-07-16  8:18       ` David Marchand
  2020-07-16  8:28         ` Adrian Moreno
  0 siblings, 1 reply; 18+ messages in thread
From: David Marchand @ 2020-07-16  8:18 UTC (permalink / raw)
  To: Adrian Moreno, Xia, Chenbo
  Cc: dev, maxime.coquelin, Wang, Zhihong, Thomas Monjalon

On Thu, Jul 16, 2020 at 9:53 AM Adrian Moreno <amorenoz@redhat.com> wrote:
> >> +    if (ret > UINT8_MAX) {
> >> +            PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS
> >> response 0x%" PRIx64 "\n", ret);
> >
> > There's a coding style issue here:
> > CHECK:CAMELCASE: Avoid CamelCase: <PRIx64>
> > Could you fix this?
>
> I did see this one as well. It also seems new because PRIx64 is widely used in DPDK.
>
> find -iname *.c | xargs grep -i "PRIx64" | wc -l
> 520
>
> So, ether we have to modify the script to ignore this one or change all 520
> cases, right?

This is a false positive.
We won't drop use of this standard macro because checkpatch is not
happy with it.

I don't see how to silence this warning but I can see that the kernel
uses this macro: so I guess people are simply ignoring this one.


-- 
David Marchand


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

* Re: [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user
  2020-07-16  8:18       ` David Marchand
@ 2020-07-16  8:28         ` Adrian Moreno
  0 siblings, 0 replies; 18+ messages in thread
From: Adrian Moreno @ 2020-07-16  8:28 UTC (permalink / raw)
  To: David Marchand, Xia, Chenbo
  Cc: dev, maxime.coquelin, Wang, Zhihong, Thomas Monjalon



On 7/16/20 10:18 AM, David Marchand wrote:
> On Thu, Jul 16, 2020 at 9:53 AM Adrian Moreno <amorenoz@redhat.com> wrote:
>>>> +    if (ret > UINT8_MAX) {
>>>> +            PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS
>>>> response 0x%" PRIx64 "\n", ret);
>>>
>>> There's a coding style issue here:
>>> CHECK:CAMELCASE: Avoid CamelCase: <PRIx64>
>>> Could you fix this?
>>
>> I did see this one as well. It also seems new because PRIx64 is widely used in DPDK.
>>
>> find -iname *.c | xargs grep -i "PRIx64" | wc -l
>> 520
>>
>> So, ether we have to modify the script to ignore this one or change all 520
>> cases, right?
> 
> This is a false positive.
> We won't drop use of this standard macro because checkpatch is not
> happy with it.
> 
> I don't see how to silence this warning but I can see that the kernel
> uses this macro: so I guess people are simply ignoring this one.
> 
> 

Thanks David!

-- 
Adrián Moreno


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

* Re: [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user
  2020-07-16  7:43     ` Adrian Moreno
@ 2020-07-16  8:58       ` Xia, Chenbo
  2020-07-16  9:51         ` Adrian Moreno
  0 siblings, 1 reply; 18+ messages in thread
From: Xia, Chenbo @ 2020-07-16  8:58 UTC (permalink / raw)
  To: Adrian Moreno, dev; +Cc: maxime.coquelin, Wang, Zhihong

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 3:43 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>
> Subject: Re: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-
> user
> 
> 
> 
> On 7/16/20 5:15 AM, Xia, Chenbo wrote:
> > Hi Adrian,
> >
> >> -----Original Message-----
> >> From: Adrian Moreno <amorenoz@redhat.com>
> >> Sent: Thursday, July 16, 2020 1:18 AM
> >> To: dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> >> <zhihong.wang@intel.com>; amorenoz@redhat.com; Xia, Chenbo
> >> <chenbo.xia@intel.com>
> >> Subject: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to
> >> Virtio-user
> >>
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>
> >> This patch adds support for VHOST_USER_SET_STATUS request. It is used
> >> to make the backend aware of Virtio devices status update.
> >>
> >> It is useful for the backend to know when the Virtio driver is done
> >> with the Virtio device configuration.
> >>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> >> ---
> >>  drivers/net/virtio/virtio_user/vhost.h        |  6 +++++
> >>  drivers/net/virtio/virtio_user/vhost_user.c   | 10 ++++++++
> >>  .../net/virtio/virtio_user/virtio_user_dev.c  | 23 ++++++++++++++++++-
> >>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
> >>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
> >>  5 files changed, 40 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/virtio/virtio_user/vhost.h
> >> b/drivers/net/virtio/virtio_user/vhost.h
> >> index 260e1c308..8f49ef457 100644
> >> --- a/drivers/net/virtio/virtio_user/vhost.h
> >> +++ b/drivers/net/virtio/virtio_user/vhost.h
> >> @@ -57,6 +57,10 @@ struct vhost_vring_addr {  #define
> >> VHOST_USER_PROTOCOL_F_REPLY_ACK 3  #endif
> >>
> >> +#ifndef VHOST_USER_PROTOCOL_F_STATUS #define
> >> +VHOST_USER_PROTOCOL_F_STATUS 16 #endif
> >> +
> >>  enum vhost_user_request {
> >>  	VHOST_USER_NONE = 0,
> >>  	VHOST_USER_GET_FEATURES = 1,
> >> @@ -77,6 +81,8 @@ enum vhost_user_request {
> >>  	VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> >>  	VHOST_USER_GET_QUEUE_NUM = 17,
> >>  	VHOST_USER_SET_VRING_ENABLE = 18,
> >> +	VHOST_USER_SET_STATUS = 39,
> >> +	VHOST_USER_GET_STATUS = 40,
> >>  	VHOST_USER_MAX
> >>  };
> >>
> >> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
> >> b/drivers/net/virtio/virtio_user/vhost_user.c
> >> index 631d0e353..2332e01d1 100644
> >> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> >> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> >> @@ -244,6 +244,8 @@ const char * const vhost_msg_strings[] = {
> >>  	[VHOST_USER_SET_VRING_ENABLE] = "VHOST_SET_VRING_ENABLE",
> >>  	[VHOST_USER_GET_PROTOCOL_FEATURES] =
> >> "VHOST_USER_GET_PROTOCOL_FEATURES",
> >>  	[VHOST_USER_SET_PROTOCOL_FEATURES] =
> >> "VHOST_USER_SET_PROTOCOL_FEATURES",
> >> +	[VHOST_USER_SET_STATUS] = "VHOST_SET_STATUS",
> >> +	[VHOST_USER_GET_STATUS] = "VHOST_GET_STATUS",
> >>  };
> >>
> >>  static int
> >> @@ -280,6 +282,14 @@ vhost_user_sock(struct virtio_user_dev *dev,
> >>  		need_reply = 1;
> >>  		break;
> >>
> >> +	case VHOST_USER_SET_STATUS:
> >> +		if (!(dev->protocol_features &
> >> +				(1ULL <<
> >> VHOST_USER_PROTOCOL_F_STATUS)))
> >> +			return 0;
> >> +
> >> +		if (has_reply_ack)
> >> +			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> >> +		/* Fallthrough */
> >
> > There's a coding style issue here:
> > WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough
> comment.
> > Could you fix this?
> >
> "fallthrough" is not defined. I think this macro is defined in the linux kernel
> (where checkpatches.pl comes from?). We could define the same macro that
> would depend in compiler support for __falthrough__ attribute.
> 
> In any case, this is not the only case:
> 
> $ find -name *.c | xargs grep -i "/\* fallthrough \*/" | wc -l
> 62
> 
> So maybe this warning is new?
> Should I change all of them together in another patch?

checkpatches.pl is in linux kernel. I think it prefers 'fallthrough' rather than
'Fallthrough'. I think it's a linux style, right?

Thanks!
Chenbo

> 
> >>  	case VHOST_USER_SET_FEATURES:
> >>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
> >>  	case VHOST_USER_SET_LOG_BASE:
> >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >> index 0a6991bcc..2c400a448 100644
> >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >> @@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev
> >> *dev)
> >>
> >>  #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
> >>  	(1ULL << VHOST_USER_PROTOCOL_F_MQ |		\
> >> -	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
> >> +	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |	\
> >> +	 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
> >>
> >>  int
> >>  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int
> >> queues, @@ -
> >> 783,3 +784,23 @@ virtio_user_handle_cq(struct virtio_user_dev *dev,
> >> uint16_t
> >> queue_idx)
> >>  		__atomic_add_fetch(&vring->used->idx, 1,
> __ATOMIC_RELAXED);
> >>  	}
> >>  }
> >> +
> >> +int
> >> +virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t
> >> +status) {
> >> +	int ret;
> >> +	uint64_t arg = status;
> >> +
> >> +	/* Vhost-user only for now */
> >> +	if (!is_vhost_user_by_type(dev->path))
> >> +		return 0;
> >> +
> >> +	if (!(dev->protocol_features & (1ULL <<
> >> VHOST_USER_PROTOCOL_F_STATUS)))
> >> +		return 0;
> >> +
> >> +	ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg);
> >> +	if (ret)
> >> +		return -1;
> >
> > Do you think we should add a log here to show if SET_STAUTS failed?
> >
> Good idea! Will do in the next version. Thanks
> 
> > Thanks!
> > Chenbo
> >
> >> +
> >> +	return 0;
> >> +}
> >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >> index 10b274d19..a76d7cfaf 100644
> >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >> @@ -74,4 +74,5 @@ 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);
> >>  #endif
> >> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> >> b/drivers/net/virtio/virtio_user_ethdev.c
> >> index 5b06d8e89..e52f11341 100644
> >> --- a/drivers/net/virtio/virtio_user_ethdev.c
> >> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> >> @@ -273,6 +273,7 @@ virtio_user_set_status(struct virtio_hw *hw,
> >> uint8_t
> >> status)
> >>  	else if (status == VIRTIO_CONFIG_STATUS_RESET)
> >>  		virtio_user_reset(hw);
> >>  	dev->status = status;
> >> +	virtio_user_send_status_update(dev, status);
> >>  }
> >>
> >>  static uint8_t
> >> --
> >> 2.26.2
> >
> 
> --
> Adrián Moreno


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

* Re: [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user
  2020-07-16  8:58       ` Xia, Chenbo
@ 2020-07-16  9:51         ` Adrian Moreno
  2020-07-16 11:18           ` Xia, Chenbo
  0 siblings, 1 reply; 18+ messages in thread
From: Adrian Moreno @ 2020-07-16  9:51 UTC (permalink / raw)
  To: Xia, Chenbo, dev; +Cc: maxime.coquelin, Wang, Zhihong



On 7/16/20 10:58 AM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Thursday, July 16, 2020 3:43 PM
>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>
>> Subject: Re: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-
>> user
>>
>>
>>
>> On 7/16/20 5:15 AM, Xia, Chenbo wrote:
>>> Hi Adrian,
>>>
>>>> -----Original Message-----
>>>> From: Adrian Moreno <amorenoz@redhat.com>
>>>> Sent: Thursday, July 16, 2020 1:18 AM
>>>> To: dev@dpdk.org
>>>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
>>>> <zhihong.wang@intel.com>; amorenoz@redhat.com; Xia, Chenbo
>>>> <chenbo.xia@intel.com>
>>>> Subject: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to
>>>> Virtio-user
>>>>
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>
>>>> This patch adds support for VHOST_USER_SET_STATUS request. It is used
>>>> to make the backend aware of Virtio devices status update.
>>>>
>>>> It is useful for the backend to know when the Virtio driver is done
>>>> with the Virtio device configuration.
>>>>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>>> ---
>>>>  drivers/net/virtio/virtio_user/vhost.h        |  6 +++++
>>>>  drivers/net/virtio/virtio_user/vhost_user.c   | 10 ++++++++
>>>>  .../net/virtio/virtio_user/virtio_user_dev.c  | 23 ++++++++++++++++++-
>>>>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>>>>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
>>>>  5 files changed, 40 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
>>>> b/drivers/net/virtio/virtio_user/vhost.h
>>>> index 260e1c308..8f49ef457 100644
>>>> --- a/drivers/net/virtio/virtio_user/vhost.h
>>>> +++ b/drivers/net/virtio/virtio_user/vhost.h
>>>> @@ -57,6 +57,10 @@ struct vhost_vring_addr {  #define
>>>> VHOST_USER_PROTOCOL_F_REPLY_ACK 3  #endif
>>>>
>>>> +#ifndef VHOST_USER_PROTOCOL_F_STATUS #define
>>>> +VHOST_USER_PROTOCOL_F_STATUS 16 #endif
>>>> +
>>>>  enum vhost_user_request {
>>>>  	VHOST_USER_NONE = 0,
>>>>  	VHOST_USER_GET_FEATURES = 1,
>>>> @@ -77,6 +81,8 @@ enum vhost_user_request {
>>>>  	VHOST_USER_SET_PROTOCOL_FEATURES = 16,
>>>>  	VHOST_USER_GET_QUEUE_NUM = 17,
>>>>  	VHOST_USER_SET_VRING_ENABLE = 18,
>>>> +	VHOST_USER_SET_STATUS = 39,
>>>> +	VHOST_USER_GET_STATUS = 40,
>>>>  	VHOST_USER_MAX
>>>>  };
>>>>
>>>> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
>>>> b/drivers/net/virtio/virtio_user/vhost_user.c
>>>> index 631d0e353..2332e01d1 100644
>>>> --- a/drivers/net/virtio/virtio_user/vhost_user.c
>>>> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
>>>> @@ -244,6 +244,8 @@ const char * const vhost_msg_strings[] = {
>>>>  	[VHOST_USER_SET_VRING_ENABLE] = "VHOST_SET_VRING_ENABLE",
>>>>  	[VHOST_USER_GET_PROTOCOL_FEATURES] =
>>>> "VHOST_USER_GET_PROTOCOL_FEATURES",
>>>>  	[VHOST_USER_SET_PROTOCOL_FEATURES] =
>>>> "VHOST_USER_SET_PROTOCOL_FEATURES",
>>>> +	[VHOST_USER_SET_STATUS] = "VHOST_SET_STATUS",
>>>> +	[VHOST_USER_GET_STATUS] = "VHOST_GET_STATUS",
>>>>  };
>>>>
>>>>  static int
>>>> @@ -280,6 +282,14 @@ vhost_user_sock(struct virtio_user_dev *dev,
>>>>  		need_reply = 1;
>>>>  		break;
>>>>
>>>> +	case VHOST_USER_SET_STATUS:
>>>> +		if (!(dev->protocol_features &
>>>> +				(1ULL <<
>>>> VHOST_USER_PROTOCOL_F_STATUS)))
>>>> +			return 0;
>>>> +
>>>> +		if (has_reply_ack)
>>>> +			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
>>>> +		/* Fallthrough */
>>>
>>> There's a coding style issue here:
>>> WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough
>> comment.
>>> Could you fix this?
>>>
>> "fallthrough" is not defined. I think this macro is defined in the linux kernel
>> (where checkpatches.pl comes from?). We could define the same macro that
>> would depend in compiler support for __falthrough__ attribute.
>>
>> In any case, this is not the only case:
>>
>> $ find -name *.c | xargs grep -i "/\* fallthrough \*/" | wc -l
>> 62
>>
>> So maybe this warning is new?
>> Should I change all of them together in another patch?
> 
> checkpatches.pl is in linux kernel. I think it prefers 'fallthrough' rather than
> 'Fallthrough'. I think it's a linux style, right?
> 
It refers to this:
https://github.com/torvalds/linux/blob/91a9a90d040e8b9ff63d48ea71468e0f4db764ff/include/linux/compiler_attributes.h#L201

Since we don't have that pseudo-keyword, we should see if we can disable this in
checkpatches.


> Thanks!
> Chenbo
> 
>>
>>>>  	case VHOST_USER_SET_FEATURES:
>>>>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
>>>>  	case VHOST_USER_SET_LOG_BASE:
>>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>> index 0a6991bcc..2c400a448 100644
>>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>>> @@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev
>>>> *dev)
>>>>
>>>>  #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
>>>>  	(1ULL << VHOST_USER_PROTOCOL_F_MQ |		\
>>>> -	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
>>>> +	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |	\
>>>> +	 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
>>>>
>>>>  int
>>>>  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int
>>>> queues, @@ -
>>>> 783,3 +784,23 @@ virtio_user_handle_cq(struct virtio_user_dev *dev,
>>>> uint16_t
>>>> queue_idx)
>>>>  		__atomic_add_fetch(&vring->used->idx, 1,
>> __ATOMIC_RELAXED);
>>>>  	}
>>>>  }
>>>> +
>>>> +int
>>>> +virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t
>>>> +status) {
>>>> +	int ret;
>>>> +	uint64_t arg = status;
>>>> +
>>>> +	/* Vhost-user only for now */
>>>> +	if (!is_vhost_user_by_type(dev->path))
>>>> +		return 0;
>>>> +
>>>> +	if (!(dev->protocol_features & (1ULL <<
>>>> VHOST_USER_PROTOCOL_F_STATUS)))
>>>> +		return 0;
>>>> +
>>>> +	ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg);
>>>> +	if (ret)
>>>> +		return -1;
>>>
>>> Do you think we should add a log here to show if SET_STAUTS failed?
>>>
>> Good idea! Will do in the next version. Thanks
>>
>>> Thanks!
>>> Chenbo
>>>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>>> index 10b274d19..a76d7cfaf 100644
>>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>>> @@ -74,4 +74,5 @@ 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);
>>>>  #endif
>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>>> index 5b06d8e89..e52f11341 100644
>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>> @@ -273,6 +273,7 @@ virtio_user_set_status(struct virtio_hw *hw,
>>>> uint8_t
>>>> status)
>>>>  	else if (status == VIRTIO_CONFIG_STATUS_RESET)
>>>>  		virtio_user_reset(hw);
>>>>  	dev->status = status;
>>>> +	virtio_user_send_status_update(dev, status);
>>>>  }
>>>>
>>>>  static uint8_t
>>>> --
>>>> 2.26.2
>>>
>>
>> --
>> Adrián Moreno
> 

-- 
Adrián Moreno


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

* Re: [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user
  2020-07-16  9:51         ` Adrian Moreno
@ 2020-07-16 11:18           ` Xia, Chenbo
  0 siblings, 0 replies; 18+ messages in thread
From: Xia, Chenbo @ 2020-07-16 11:18 UTC (permalink / raw)
  To: Adrian Moreno, dev; +Cc: maxime.coquelin, Wang, Zhihong

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 5:51 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>
> Subject: Re: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-
> user
> 
> 
> 
> On 7/16/20 10:58 AM, Xia, Chenbo wrote:
> > Hi Adrian,
> >
> >> -----Original Message-----
> >> From: Adrian Moreno <amorenoz@redhat.com>
> >> Sent: Thursday, July 16, 2020 3:43 PM
> >> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> >> <zhihong.wang@intel.com>
> >> Subject: Re: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to
> >> Virtio- user
> >>
> >>
> >>
> >> On 7/16/20 5:15 AM, Xia, Chenbo wrote:
> >>> Hi Adrian,
> >>>
> >>>> -----Original Message-----
> >>>> From: Adrian Moreno <amorenoz@redhat.com>
> >>>> Sent: Thursday, July 16, 2020 1:18 AM
> >>>> To: dev@dpdk.org
> >>>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> >>>> <zhihong.wang@intel.com>; amorenoz@redhat.com; Xia, Chenbo
> >>>> <chenbo.xia@intel.com>
> >>>> Subject: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to
> >>>> Virtio-user
> >>>>
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>
> >>>> This patch adds support for VHOST_USER_SET_STATUS request. It is
> >>>> used to make the backend aware of Virtio devices status update.
> >>>>
> >>>> It is useful for the backend to know when the Virtio driver is done
> >>>> with the Virtio device configuration.
> >>>>
> >>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> >>>> ---
> >>>>  drivers/net/virtio/virtio_user/vhost.h        |  6 +++++
> >>>>  drivers/net/virtio/virtio_user/vhost_user.c   | 10 ++++++++
> >>>>  .../net/virtio/virtio_user/virtio_user_dev.c  | 23 ++++++++++++++++++-
> >>>>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
> >>>>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
> >>>>  5 files changed, 40 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/net/virtio/virtio_user/vhost.h
> >>>> b/drivers/net/virtio/virtio_user/vhost.h
> >>>> index 260e1c308..8f49ef457 100644
> >>>> --- a/drivers/net/virtio/virtio_user/vhost.h
> >>>> +++ b/drivers/net/virtio/virtio_user/vhost.h
> >>>> @@ -57,6 +57,10 @@ struct vhost_vring_addr {  #define
> >>>> VHOST_USER_PROTOCOL_F_REPLY_ACK 3  #endif
> >>>>
> >>>> +#ifndef VHOST_USER_PROTOCOL_F_STATUS #define
> >>>> +VHOST_USER_PROTOCOL_F_STATUS 16 #endif
> >>>> +
> >>>>  enum vhost_user_request {
> >>>>  	VHOST_USER_NONE = 0,
> >>>>  	VHOST_USER_GET_FEATURES = 1,
> >>>> @@ -77,6 +81,8 @@ enum vhost_user_request {
> >>>>  	VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> >>>>  	VHOST_USER_GET_QUEUE_NUM = 17,
> >>>>  	VHOST_USER_SET_VRING_ENABLE = 18,
> >>>> +	VHOST_USER_SET_STATUS = 39,
> >>>> +	VHOST_USER_GET_STATUS = 40,
> >>>>  	VHOST_USER_MAX
> >>>>  };
> >>>>
> >>>> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
> >>>> b/drivers/net/virtio/virtio_user/vhost_user.c
> >>>> index 631d0e353..2332e01d1 100644
> >>>> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> >>>> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> >>>> @@ -244,6 +244,8 @@ const char * const vhost_msg_strings[] = {
> >>>>  	[VHOST_USER_SET_VRING_ENABLE] = "VHOST_SET_VRING_ENABLE",
> >>>>  	[VHOST_USER_GET_PROTOCOL_FEATURES] =
> >>>> "VHOST_USER_GET_PROTOCOL_FEATURES",
> >>>>  	[VHOST_USER_SET_PROTOCOL_FEATURES] =
> >>>> "VHOST_USER_SET_PROTOCOL_FEATURES",
> >>>> +	[VHOST_USER_SET_STATUS] = "VHOST_SET_STATUS",
> >>>> +	[VHOST_USER_GET_STATUS] = "VHOST_GET_STATUS",
> >>>>  };
> >>>>
> >>>>  static int
> >>>> @@ -280,6 +282,14 @@ vhost_user_sock(struct virtio_user_dev *dev,
> >>>>  		need_reply = 1;
> >>>>  		break;
> >>>>
> >>>> +	case VHOST_USER_SET_STATUS:
> >>>> +		if (!(dev->protocol_features &
> >>>> +				(1ULL <<
> >>>> VHOST_USER_PROTOCOL_F_STATUS)))
> >>>> +			return 0;
> >>>> +
> >>>> +		if (has_reply_ack)
> >>>> +			msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> >>>> +		/* Fallthrough */
> >>>
> >>> There's a coding style issue here:
> >>> WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough
> >> comment.
> >>> Could you fix this?
> >>>
> >> "fallthrough" is not defined. I think this macro is defined in the
> >> linux kernel (where checkpatches.pl comes from?). We could define the
> >> same macro that would depend in compiler support for __falthrough__
> attribute.
> >>
> >> In any case, this is not the only case:
> >>
> >> $ find -name *.c | xargs grep -i "/\* fallthrough \*/" | wc -l
> >> 62
> >>
> >> So maybe this warning is new?
> >> Should I change all of them together in another patch?
> >
> > checkpatches.pl is in linux kernel. I think it prefers 'fallthrough'
> > rather than 'Fallthrough'. I think it's a linux style, right?
> >
> It refers to this:
> https://github.com/torvalds/linux/blob/91a9a90d040e8b9ff63d48ea71468e0f4
> db764ff/include/linux/compiler_attributes.h#L201
> 
> Since we don't have that pseudo-keyword, we should see if we can disable this
> in checkpatches.

Yeah, I confirmed this could be ignored. So you could change it or keep it.
Both fine.

Cheers,
Chenbo

> 
> 
> > Thanks!
> > Chenbo
> >
> >>
> >>>>  	case VHOST_USER_SET_FEATURES:
> >>>>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
> >>>>  	case VHOST_USER_SET_LOG_BASE:
> >>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>> index 0a6991bcc..2c400a448 100644
> >>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> >>>> @@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev
> >>>> *dev)
> >>>>
> >>>>  #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES		\
> >>>>  	(1ULL << VHOST_USER_PROTOCOL_F_MQ |		\
> >>>> -	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
> >>>> +	 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |	\
> >>>> +	 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
> >>>>
> >>>>  int
> >>>>  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int
> >>>> queues, @@ -
> >>>> 783,3 +784,23 @@ virtio_user_handle_cq(struct virtio_user_dev *dev,
> >>>> uint16_t
> >>>> queue_idx)
> >>>>  		__atomic_add_fetch(&vring->used->idx, 1,
> >> __ATOMIC_RELAXED);
> >>>>  	}
> >>>>  }
> >>>> +
> >>>> +int
> >>>> +virtio_user_send_status_update(struct virtio_user_dev *dev,
> >>>> +uint8_t
> >>>> +status) {
> >>>> +	int ret;
> >>>> +	uint64_t arg = status;
> >>>> +
> >>>> +	/* Vhost-user only for now */
> >>>> +	if (!is_vhost_user_by_type(dev->path))
> >>>> +		return 0;
> >>>> +
> >>>> +	if (!(dev->protocol_features & (1ULL <<
> >>>> VHOST_USER_PROTOCOL_F_STATUS)))
> >>>> +		return 0;
> >>>> +
> >>>> +	ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg);
> >>>> +	if (ret)
> >>>> +		return -1;
> >>>
> >>> Do you think we should add a log here to show if SET_STAUTS failed?
> >>>
> >> Good idea! Will do in the next version. Thanks
> >>
> >>> Thanks!
> >>> Chenbo
> >>>
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >>>> index 10b274d19..a76d7cfaf 100644
> >>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >>>> @@ -74,4 +74,5 @@ 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);
> >>>>  #endif
> >>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> >>>> b/drivers/net/virtio/virtio_user_ethdev.c
> >>>> index 5b06d8e89..e52f11341 100644
> >>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
> >>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> >>>> @@ -273,6 +273,7 @@ virtio_user_set_status(struct virtio_hw *hw,
> >>>> uint8_t
> >>>> status)
> >>>>  	else if (status == VIRTIO_CONFIG_STATUS_RESET)
> >>>>  		virtio_user_reset(hw);
> >>>>  	dev->status = status;
> >>>> +	virtio_user_send_status_update(dev, status);
> >>>>  }
> >>>>
> >>>>  static uint8_t
> >>>> --
> >>>> 2.26.2
> >>>
> >>
> >> --
> >> Adrián Moreno
> >
> 
> --
> Adrián Moreno


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

end of thread, other threads:[~2020-07-16 11:19 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-15 17:18 [dpdk-dev] [PATCH 0/5] Add support for GET/SET_STATUS on virtio-user pmd Adrian Moreno
2020-07-15 17:18 ` [dpdk-dev] [PATCH 1/5] net/virtio: split virtio-net and virtio status Adrian Moreno
2020-07-15 17:18 ` [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit Adrian Moreno
2020-07-16  2:26   ` Xia, Chenbo
2020-07-16  7:34     ` Adrian Moreno
2020-07-16  8:14       ` Xia, Chenbo
2020-07-15 17:18 ` [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user Adrian Moreno
2020-07-16  3:15   ` Xia, Chenbo
2020-07-16  7:43     ` Adrian Moreno
2020-07-16  8:58       ` Xia, Chenbo
2020-07-16  9:51         ` Adrian Moreno
2020-07-16 11:18           ` Xia, Chenbo
2020-07-15 17:18 ` [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user Adrian Moreno
2020-07-16  3:31   ` Xia, Chenbo
2020-07-16  7:53     ` Adrian Moreno
2020-07-16  8:18       ` David Marchand
2020-07-16  8:28         ` Adrian Moreno
2020-07-15 17:18 ` [dpdk-dev] [PATCH 5/5] net/virtio: enable feature checking on virtio-user Adrian Moreno

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git