Fixes for virtio/virtio-user

PATCH Applied Tiwei Bie 2 participants 14 messages Oct 29, 2018 – Oct 30, 2018 Network drivers

Share & tools

Tiwei Bie

author

2018-10-29 05:29 UTC #0 lore raw

cover Fixes for virtio/virtio-user

Tiwei Bie (6):
net/virtio-user: do not stop stopped device again net/virtio-user: do not make vhost user channel nonblock net/virtio-user: do not reset owner when driver resets net/virtio-user: fix device features for server mode net/virtio-user: simplify device features preparation net/virtio: fix guest announce support

 drivers/net/virtio/virtio_ethdev.c            | 16 +++--
 .../net/virtio/virtio_user/virtio_user_dev.c  | 65 ++++++++++---------
 .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
 drivers/net/virtio/virtio_user_ethdev.c       |  6 +-
 4 files changed, 48 insertions(+), 40 deletions(-)

Signature · 1 line

2.19.1

Tiwei Bie

author

2018-10-29 05:28 UTC #1 lore raw

PATCH 1/6 net/virtio-user: do not stop stopped device again

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

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

Fixes e3b434818bbb ("net/virtio-user: support kernel vhost")

Fixes 12ecb2f63b12 ("net/virtio-user: support memory hotplug")

Cc [email protected]

Reported-by Stephen Hemminger <[email protected]>

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/net/virtio/virtio_user/virtio_user_dev.c +4 -0

index a185aed34..684702c56 100644
@@ -184,6 +184,9 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
 	uint32_t i;
 
 	pthread_mutex_lock(&dev->mutex);
+	if (!dev->started)
+		goto out;
+
 	for (i = 0; i < dev->max_queue_pairs; ++i)
 		dev->ops->enable_qp(dev, i, 0);

@@ -193,6 +196,7 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
 		return -1;
 	}
 	dev->started = false;
+out:
 	pthread_mutex_unlock(&dev->mutex);
 
 	return 0;

Signature · 1 line

2.19.1

Maxime Coquelin

Reviewed-by on v1 1/6

2018-10-30 10:19 UTC #2 lore raw

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

Quoted 10 lines · Tiwei Bie

Without this change, virtio-user still works, but it will show annoying error messages like this on shutdown:
vhost_kernel_set_backend(): VHOST_NET_SET_BACKEND fails, Operation not permitted vhost_kernel_ioctl(): VHOST_RESET_OWNER failed: Operation not permitted
Fixes e3b434818bbb ("net/virtio-user: support kernel vhost")
Fixes 12ecb2f63b12 ("net/virtio-user: support memory hotplug")
Cc [email protected]

Quoted 5 lines

Reported-by Stephen Hemminger <[email protected]>
Signed-off-by Tiwei Bie <[email protected]>
---
  drivers/net/virtio/virtio_user/virtio_user_dev.c | 4 ++++
  1 file changed, 4 insertions(+)

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Tiwei Bie

author

2018-10-29 05:28 UTC #3 lore raw

PATCH 2/6 net/virtio-user: do not make vhost user channel nonblock

In reply to Tiwei Bie

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

Fixes bd8f50a45d0f ("net/virtio-user: support server mode")

Cc [email protected]

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/net/virtio/virtio_user_ethdev.c | 4 ----
 1 file changed, 4 deletions(-)

drivers/net/virtio/virtio_user_ethdev.c +0 -4

index b51cbc85b..2100e43db 100644
@@ -28,7 +28,6 @@ static int
 virtio_user_server_reconnect(struct virtio_user_dev *dev)
 {
 	int ret;
-	int flag;
 	int connectfd;
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[dev->port_id];

@@ -49,9 +48,6 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 
 	dev->features &= dev->device_features;
 
-	flag = fcntl(connectfd, F_GETFD);
-	fcntl(connectfd, F_SETFL, flag | O_NONBLOCK);
-
 	ret = virtio_user_start_device(dev);
 	if (ret < 0)
 		return -1;

Signature · 1 line

2.19.1

Maxime Coquelin

Reviewed-by on v1 2/6

2018-10-30 10:20 UTC #4 lore raw

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

Quoted 4 lines · Tiwei Bie

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

Quoted 8 lines

Fixes bd8f50a45d0f ("net/virtio-user: support server mode")
Cc [email protected]
Signed-off-by Tiwei Bie <[email protected]>
---
  drivers/net/virtio/virtio_user_ethdev.c | 4 ----
  1 file changed, 4 deletions(-)

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Tiwei Bie

author

2018-10-29 05:28 UTC #5 lore raw

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

In reply to Tiwei Bie

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

vhost_kernel_ioctl(): VHOST_SET_OWNER failed: Device or resource busy

Fixes bce7e9050f9b ("net/virtio-user: fix start with kernel vhost")

Fixes 0d6a8752ac9d ("net/virtio-user: fix crash as features change")

Cc [email protected]

Signed-off-by Tiwei Bie <[email protected]>

---
 .../net/virtio/virtio_user/virtio_user_dev.c  | 22 ++++++++++++-------
 1 file changed, 14 insertions(+), 8 deletions(-)

drivers/net/virtio/virtio_user/virtio_user_dev.c +14 -8

index 684702c56..be70414a1 100644
@@ -134,9 +134,6 @@ virtio_user_start_device(struct virtio_user_dev *dev)
 	if (is_vhost_user_by_type(dev->path) && dev->vhostfd < 0)
 		goto error;
 
-	/* Do not check return as already done in init, or reset in stop */
-	dev->ops->send_request(dev, VHOST_USER_SET_OWNER, NULL);
-
 	/* Step 0: tell vhost to create queues */
 	if (virtio_user_queue_setup(dev, virtio_user_create_queue) < 0)
 		goto error;

@@ -181,7 +178,9 @@ virtio_user_start_device(struct virtio_user_dev *dev)
 
 int virtio_user_stop_device(struct virtio_user_dev *dev)
 {
+	struct vhost_vring_state state;
 	uint32_t i;
+	int error = 0;
 
 	pthread_mutex_lock(&dev->mutex);
 	if (!dev->started)

@@ -190,16 +189,23 @@ int virtio_user_stop_device(struct virtio_user_dev *dev)
 	for (i = 0; i < dev->max_queue_pairs; ++i)
 		dev->ops->enable_qp(dev, i, 0);
 
-	if (dev->ops->send_request(dev, VHOST_USER_RESET_OWNER, NULL) < 0) {
-		PMD_DRV_LOG(INFO, "Failed to reset the device\n");
-		pthread_mutex_unlock(&dev->mutex);
-		return -1;
+	/* Stop the backend. */
+	for (i = 0; i < dev->max_queue_pairs * 2; ++i) {
+		state.index = i;
+		if (dev->ops->send_request(dev, VHOST_USER_GET_VRING_BASE,
+					   &state) < 0) {
+			PMD_DRV_LOG(ERR, "get_vring_base failed, index=%u\n",
+				    i);
+			error = -1;
+			goto out;
+		}
 	}
+
 	dev->started = false;
 out:
 	pthread_mutex_unlock(&dev->mutex);
 
-	return 0;
+	return error;
 }
 
 static inline void

Signature · 1 line

2.19.1

Maxime Coquelin

Reviewed-by on v1 3/6

2018-10-30 10:21 UTC #6 lore raw

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

Quoted 8 lines · Tiwei Bie

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

Quoted 8 lines

Fixes bce7e9050f9b ("net/virtio-user: fix start with kernel vhost")
Fixes 0d6a8752ac9d ("net/virtio-user: fix crash as features change")
Cc [email protected]
Signed-off-by Tiwei Bie <[email protected]>
---
  .../net/virtio/virtio_user/virtio_user_dev.c  | 22 ++++++++++++-------
  1 file changed, 14 insertions(+), 8 deletions(-)

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Tiwei Bie

author

2018-10-29 05:28 UTC #7 lore raw

PATCH 4/6 net/virtio-user: fix device features for server mode

In reply to Tiwei Bie

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

Fixes 201a41651715 ("net/virtio-user: fix multiple queues fail in server mode")

Cc [email protected]

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 8 +++++---
 drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
 drivers/net/virtio/virtio_user_ethdev.c          | 2 ++
 3 files changed, 8 insertions(+), 3 deletions(-)

drivers/net/virtio/virtio_user/virtio_user_dev.c +5 -3

index be70414a1..44e093eb7 100644
@@ -421,6 +421,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 	dev->queue_pairs = 1; /* mq disabled by default */
 	dev->queue_size = queue_size;
 	dev->mac_specified = 0;
+	dev->frontend_features = 0;
 	dev->unsupported_features = 0;
 	parse_mac(dev, mac);

@@ -468,7 +469,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 	}
 
 	if (dev->mac_specified) {
-		dev->device_features |= (1ull << VIRTIO_NET_F_MAC);
+		dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC);
 	} else {
 		dev->device_features &= ~(1ull << VIRTIO_NET_F_MAC);
 		dev->unsupported_features |= (1ull << VIRTIO_NET_F_MAC);

@@ -478,7 +479,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		/* device does not really need to know anything about CQ,
 		 * so if necessary, we just claim to support CQ
 		 */
-		dev->device_features |= (1ull << VIRTIO_NET_F_CTRL_VQ);
+		dev->frontend_features |= (1ull << VIRTIO_NET_F_CTRL_VQ);
 	} else {
 		dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
 		/* Also disable features depends on VIRTIO_NET_F_CTRL_VQ */

@@ -499,8 +500,9 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 
 	/* The backend will not report this feature, we add it explicitly */
 	if (is_vhost_user_by_type(dev->path))
-		dev->device_features |= (1ull << VIRTIO_NET_F_STATUS);
+		dev->frontend_features |= (1ull << VIRTIO_NET_F_STATUS);
 
+	dev->device_features |= dev->frontend_features;
 	dev->device_features &= VIRTIO_USER_SUPPORTED_FEATURES;
 	dev->unsupported_features |= ~VIRTIO_USER_SUPPORTED_FEATURES;

drivers/net/virtio/virtio_user/virtio_user_dev.h +1 -0

index d6e0e137b..c42ce5d4b 100644
@@ -33,6 +33,7 @@ struct virtio_user_dev {
 				   * and will be sync with device
 				   */
 	uint64_t	device_features; /* supported features by device */
+	uint64_t	frontend_features; /* enabled frontend features */
 	uint64_t	unsupported_features; /* unsupported features mask */
 	uint8_t		status;
 	uint16_t	port_id;

drivers/net/virtio/virtio_user_ethdev.c +2 -0

index 2100e43db..c419d5b9d 100644
@@ -43,6 +43,8 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 		return -1;
 	}
 
+	dev->device_features |= dev->frontend_features;
+
 	/* umask vhost-user unsupported features */
 	dev->device_features &= ~(dev->unsupported_features);

Signature · 1 line

2.19.1

Maxime Coquelin

Reviewed-by on v1 4/6

2018-10-30 10:25 UTC #8 lore raw

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

Quoted 7 lines · Tiwei Bie

We need to save the supported frontend features (which won't be announced by vhost backend), otherwise we will lost them when the connection to vhost-user backend is established in server mode.
Fixes 201a41651715 ("net/virtio-user: fix multiple queues fail in server mode")
Cc [email protected]

Quoted 6 lines

Signed-off-by Tiwei Bie <[email protected]>

---
  drivers/net/virtio/virtio_user/virtio_user_dev.c | 8 +++++---
  drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
  drivers/net/virtio/virtio_user_ethdev.c          | 2 ++
  3 files changed, 8 insertions(+), 3 deletions(-)

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Tiwei Bie

author

2018-10-29 05:29 UTC #9 lore raw

PATCH 5/6 net/virtio-user: simplify device features preparation

In reply to Tiwei Bie

Get rid of the duplicated code in device features preparation which looks awful.

Signed-off-by Tiwei Bie <[email protected]>

---
 .../net/virtio/virtio_user/virtio_user_dev.c  | 31 +++++++------------
 1 file changed, 11 insertions(+), 20 deletions(-)

drivers/net/virtio/virtio_user/virtio_user_dev.c +11 ~~-20~~

index 44e093eb7..20abfcd19 100644
@@ -422,7 +422,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 	dev->queue_size = queue_size;
 	dev->mac_specified = 0;
 	dev->frontend_features = 0;
-	dev->unsupported_features = 0;
+	dev->unsupported_features = ~VIRTIO_USER_SUPPORTED_FEATURES;
 	parse_mac(dev, mac);
 
 	if (*ifname) {

@@ -458,22 +458,16 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES;
 	}
 
-	if (!mrg_rxbuf) {
-		dev->device_features &= ~(1ull << VIRTIO_NET_F_MRG_RXBUF);
+	if (!mrg_rxbuf)
 		dev->unsupported_features |= (1ull << VIRTIO_NET_F_MRG_RXBUF);
-	}
 
-	if (!in_order) {
-		dev->device_features &= ~(1ull << VIRTIO_F_IN_ORDER);
+	if (!in_order)
 		dev->unsupported_features |= (1ull << VIRTIO_F_IN_ORDER);
-	}
 
-	if (dev->mac_specified) {
+	if (dev->mac_specified)
 		dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC);
-	} else {
-		dev->device_features &= ~(1ull << VIRTIO_NET_F_MAC);
+	else
 		dev->unsupported_features |= (1ull << VIRTIO_NET_F_MAC);
-	}
 
 	if (cq) {
 		/* device does not really need to know anything about CQ,

@@ -481,14 +475,8 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		 */
 		dev->frontend_features |= (1ull << VIRTIO_NET_F_CTRL_VQ);
 	} else {
-		dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_VQ);
-		/* Also disable features depends on VIRTIO_NET_F_CTRL_VQ */
-		dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_RX);
-		dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_VLAN);
-		dev->device_features &= ~(1ull << VIRTIO_NET_F_GUEST_ANNOUNCE);
-		dev->device_features &= ~(1ull << VIRTIO_NET_F_MQ);
-		dev->device_features &= ~(1ull << VIRTIO_NET_F_CTRL_MAC_ADDR);
 		dev->unsupported_features |= (1ull << VIRTIO_NET_F_CTRL_VQ);
+		/* Also disable features that depend on VIRTIO_NET_F_CTRL_VQ */
 		dev->unsupported_features |= (1ull << VIRTIO_NET_F_CTRL_RX);
 		dev->unsupported_features |= (1ull << VIRTIO_NET_F_CTRL_VLAN);
 		dev->unsupported_features |=

@@ -502,9 +490,12 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 	if (is_vhost_user_by_type(dev->path))
 		dev->frontend_features |= (1ull << VIRTIO_NET_F_STATUS);
 
+	/*
+	 * Device features =
+	 *     (frontend_features | backend_features) & ~unsupported_features;
+	 */
 	dev->device_features |= dev->frontend_features;
-	dev->device_features &= VIRTIO_USER_SUPPORTED_FEATURES;
-	dev->unsupported_features |= ~VIRTIO_USER_SUPPORTED_FEATURES;
+	dev->device_features &= ~dev->unsupported_features;
 
 	if (rte_mem_event_callback_register(VIRTIO_USER_MEM_EVENT_CLB_NAME,
 				virtio_user_mem_event_cb, dev)) {

Signature · 1 line

2.19.1

Maxime Coquelin

Reviewed-by on v1 5/6

2018-10-30 10:26 UTC #10 lore raw

Re: [PATCH 5/6] net/virtio-user: simplify device features preparation

Quoted 7 lines · Tiwei Bie

Get rid of the duplicated code in device features preparation which looks awful.
Signed-off-by Tiwei Bie <[email protected]>
---
  .../net/virtio/virtio_user/virtio_user_dev.c  | 31 +++++++------------
  1 file changed, 11 insertions(+), 20 deletions(-)

Welcomed cleanup!

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Tiwei Bie

author

2018-10-29 05:28 UTC #11 lore raw

PATCH 6/6 net/virtio: fix guest announce support

In reply to Tiwei Bie

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

Fixes 7365504f77e3 ("net/virtio: support guest announce")

Cc [email protected]

Signed-off-by Tiwei Bie <[email protected]>

---
 drivers/net/virtio/virtio_ethdev.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

drivers/net/virtio/virtio_ethdev.c +11 -5

index 10a7e3fcc..27088e3a6 100644
@@ -1288,6 +1288,7 @@ virtio_interrupt_handler(void *param)
 	struct rte_eth_dev *dev = param;
 	struct virtio_hw *hw = dev->data->dev_private;
 	uint8_t isr;
+	uint16_t status;
 
 	/* Read interrupt status which clears interrupt */
 	isr = vtpci_isr(hw);

@@ -1301,12 +1302,17 @@ virtio_interrupt_handler(void *param)
 			_rte_eth_dev_callback_process(dev,
 						      RTE_ETH_EVENT_INTR_LSC,
 						      NULL);
-	}
 
-	if (isr & VIRTIO_NET_S_ANNOUNCE) {
-		virtio_notify_peers(dev);
-		if (hw->cvq)
-			virtio_ack_link_announce(dev);
+		if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
+			vtpci_read_dev_config(hw,
+				offsetof(struct virtio_net_config, status),
+				&status, sizeof(status));
+			if (status & VIRTIO_NET_S_ANNOUNCE) {
+				virtio_notify_peers(dev);
+				if (hw->cvq)
+					virtio_ack_link_announce(dev);
+			}
+		}
 	}
 }

Signature · 1 line

2.19.1

Maxime Coquelin

Reviewed-by on v1 6/6

2018-10-30 10:30 UTC #12 lore raw

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

Quoted 11 lines · Tiwei Bie

We need to check the status field in virtio net config structure instead of the bits read from ISR register to know whether we need to do guest announce.
Fixes 7365504f77e3 ("net/virtio: support guest announce")
Cc [email protected]
Signed-off-by Tiwei Bie <[email protected]>
---
  drivers/net/virtio/virtio_ethdev.c | 16 +++++++++++-----
  1 file changed, 11 insertions(+), 5 deletions(-)

Reviewed-by Maxime Coquelin <[email protected]>

Thanks,
Maxime

Maxime Coquelin

Applied

2018-10-30 10:56 UTC #13 lore raw

In reply to Tiwei Bie

Quoted 8 lines · Tiwei Bie

Tiwei Bie (6):
net/virtio-user: do not stop stopped device again net/virtio-user: do not make vhost user channel nonblock net/virtio-user: do not reset owner when driver resets net/virtio-user: fix device features for server mode net/virtio-user: simplify device features preparation net/virtio: fix guest announce support

Quoted 6 lines

  drivers/net/virtio/virtio_ethdev.c            | 16 +++--
  .../net/virtio/virtio_user/virtio_user_dev.c  | 65 ++++++++++---------
  .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
  drivers/net/virtio/virtio_user_ethdev.c       |  6 +-
  4 files changed, 48 insertions(+), 40 deletions(-)

Applied to dpdk-next-virtio/master

Thanks,
Maxime