[dpdk-dev] [PATCH] vhost: maintain separate virtio features field

[dpdk-dev] [PATCH] vhost: maintain separate virtio features field

[Tomasz Kulasek]

There are two separate abstraction layers:
* vsocket - which represents a unix domain socket
* virtio_net - which represents a vsocket connection

There can be many connections on the same socket. vsocket provides an
API to enable/disable particular virtio features on the fly, but it's
the virtio_net that uses these features.

virtio_net used to rely on vsocket->features during feature negotiation,
breaking the layer encapsulation (and yet causing a deadlock - two locks
were being locked in a separate order). Now each virtio_net device has
it's own copy of vsocket features, created at the time of virtio_net
creation.

vsocket->features have to be still present, as features can be
enabled/disabled while no virtio_net device has been created yet.

Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
 lib/librte_vhost/socket.c     |  2 +-
 lib/librte_vhost/vhost.c      |  9 +++++----
 lib/librte_vhost/vhost.h      |  8 +++++---
 lib/librte_vhost/vhost_user.c | 33 +++++++++++++++++----------------
 4 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 83befdced..260e38dbe 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -188,7 +188,7 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
 		return;
 	}
 
-	vid = vhost_new_device();
+	vid = vhost_new_device(vsocket->features);
 	if (vid == -1) {
 		goto err;
 	}
diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index a407067e2..a307a19ed 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -256,7 +256,7 @@ reset_device(struct virtio_net *dev)
 {
 	uint32_t i;
 
-	dev->features = 0;
+	dev->negotiated_features = 0;
 	dev->protocol_features = 0;
 	dev->flags &= VIRTIO_DEV_BUILTIN_VIRTIO_NET;
 
@@ -269,7 +269,7 @@ reset_device(struct virtio_net *dev)
  * there is a new virtio device being attached).
  */
 int
-vhost_new_device(void)
+vhost_new_device(uint64_t features)
 {
 	struct virtio_net *dev;
 	int i;
@@ -296,6 +296,7 @@ vhost_new_device(void)
 	dev->vid = i;
 	dev->flags = VIRTIO_DEV_BUILTIN_VIRTIO_NET;
 	dev->slave_req_fd = -1;
+	dev->features = features;
 
 	return i;
 }
@@ -376,7 +377,7 @@ rte_vhost_get_mtu(int vid, uint16_t *mtu)
 	if (!(dev->flags & VIRTIO_DEV_READY))
 		return -EAGAIN;
 
-	if (!(dev->features & (1ULL << VIRTIO_NET_F_MTU)))
+	if (!(dev->negotiated_features & (1ULL << VIRTIO_NET_F_MTU)))
 		return -ENOTSUP;
 
 	*mtu = dev->mtu;
@@ -458,7 +459,7 @@ rte_vhost_get_negotiated_features(int vid, uint64_t *features)
 	if (!dev)
 		return -1;
 
-	*features = dev->features;
+	*features = dev->negotiated_features;
 	return 0;
 }
 
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index d947bc9e3..efbc89857 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -217,6 +217,7 @@ struct virtio_net {
 	/* Frontend (QEMU) memory and memory region information */
 	struct rte_vhost_memory	*mem;
 	uint64_t		features;
+	uint64_t		negotiated_features;
 	uint64_t		protocol_features;
 	int			vid;
 	uint32_t		flags;
@@ -266,8 +267,9 @@ vhost_log_write(struct virtio_net *dev, uint64_t addr, uint64_t len)
 {
 	uint64_t page;
 
-	if (likely(((dev->features & (1ULL << VHOST_F_LOG_ALL)) == 0) ||
-		   !dev->log_base || !len))
+	if (likely(((dev->negotiated_features &
+			(1ULL << VHOST_F_LOG_ALL)) == 0) || !dev->log_base ||
+			!len))
 		return;
 
 	if (unlikely(dev->log_size <= ((addr + len - 1) / VHOST_LOG_PAGE / 8)))
@@ -347,7 +349,7 @@ gpa_to_hpa(struct virtio_net *dev, uint64_t gpa, uint64_t size)
 
 struct virtio_net *get_device(int vid);
 
-int vhost_new_device(void);
+int vhost_new_device(uint64_t features);
 void cleanup_device(struct virtio_net *dev, int destroy);
 void reset_device(struct virtio_net *dev);
 void vhost_destroy_device(int);
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 65ee33919..818fc4263 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -132,10 +132,7 @@ vhost_user_reset_owner(struct virtio_net *dev)
 static uint64_t
 vhost_user_get_features(struct virtio_net *dev)
 {
-	uint64_t features = 0;
-
-	rte_vhost_driver_get_features(dev->ifname, &features);
-	return features;
+	return dev->features;
 }
 
 /*
@@ -146,7 +143,7 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 {
 	uint64_t vhost_features = 0;
 
-	rte_vhost_driver_get_features(dev->ifname, &vhost_features);
+	vhost_features = vhost_user_get_features(dev);
 	if (features & ~vhost_features) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%d) received invalid negotiated features.\n",
@@ -155,7 +152,7 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 	}
 
 	if (dev->flags & VIRTIO_DEV_RUNNING) {
-		if (dev->features == features)
+		if (dev->negotiated_features == features)
 			return 0;
 
 		/*
@@ -163,7 +160,8 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 		 * in running state. The exception being VHOST_F_LOG_ALL, which
 		 * is enabled when the live-migration starts.
 		 */
-		if ((dev->features ^ features) & ~(1ULL << VHOST_F_LOG_ALL)) {
+		if ((dev->negotiated_features ^ features) &
+				~(1ULL << VHOST_F_LOG_ALL)) {
 			RTE_LOG(ERR, VHOST_CONFIG,
 				"(%d) features changed while device is running.\n",
 				dev->vid);
@@ -174,9 +172,9 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 			dev->notify_ops->features_changed(dev->vid, features);
 	}
 
-	dev->features = features;
-	if (dev->features &
-		((1 << VIRTIO_NET_F_MRG_RXBUF) | (1ULL << VIRTIO_F_VERSION_1))) {
+	dev->negotiated_features = features;
+	if (dev->negotiated_features & ((1 << VIRTIO_NET_F_MRG_RXBUF) |
+			(1ULL << VIRTIO_F_VERSION_1))) {
 		dev->vhost_hlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
 	} else {
 		dev->vhost_hlen = sizeof(struct virtio_net_hdr);
@@ -184,11 +182,13 @@ vhost_user_set_features(struct virtio_net *dev, uint64_t features)
 	LOG_DEBUG(VHOST_CONFIG,
 		"(%d) mergeable RX buffers %s, virtio 1 %s\n",
 		dev->vid,
-		(dev->features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ? "on" : "off",
-		(dev->features & (1ULL << VIRTIO_F_VERSION_1)) ? "on" : "off");
+		(dev->negotiated_features & (1 << VIRTIO_NET_F_MRG_RXBUF)) ?
+				"on" : "off",
+		(dev->negotiated_features & (1ULL << VIRTIO_F_VERSION_1)) ?
+				"on" : "off");
 
 	if ((dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET) &&
-	    !(dev->features & (1ULL << VIRTIO_NET_F_MQ))) {
+	    !(dev->negotiated_features & (1ULL << VIRTIO_NET_F_MQ))) {
 		/*
 		 * Remove all but first queue pair if MQ hasn't been
 		 * negotiated. This is safe because the device is not
@@ -395,7 +395,7 @@ static uint64_t
 ring_addr_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		uint64_t ra, uint64_t size)
 {
-	if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
+	if (dev->negotiated_features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
 		uint64_t vva;
 
 		vva = vhost_user_iotlb_cache_find(vq, ra,
@@ -498,7 +498,7 @@ vhost_user_set_vring_addr(struct virtio_net **pdev, VhostUserMsg *msg)
 
 	vring_invalidate(dev, vq);
 
-	if (vq->enabled && (dev->features &
+	if (vq->enabled && (dev->negotiated_features &
 				(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))) {
 		dev = translate_ring_addresses(dev, msg->payload.state.index);
 		if (!dev)
@@ -840,7 +840,8 @@ vhost_user_set_vring_kick(struct virtio_net **pdev, struct VhostUserMsg *pmsg)
 	 * the ring starts already enabled. Otherwise, it is enabled via
 	 * the SET_VRING_ENABLE message.
 	 */
-	if (!(dev->features & (1ULL << VHOST_USER_F_PROTOCOL_FEATURES)))
+	if (!(dev->negotiated_features &
+			(1ULL << VHOST_USER_F_PROTOCOL_FEATURES)))
 		vq->enabled = 1;
 
 	if (vq->kickfd >= 0)
-- 
2.14.1

Re: [dpdk-dev] [PATCH] vhost: maintain separate virtio features field

[Maxime Coquelin]

Hi Tomasz,

On 03/05/2018 04:49 PM, Tomasz Kulasek wrote:
> There are two separate abstraction layers:
> * vsocket - which represents a unix domain socket
> * virtio_net - which represents a vsocket connection
> 
> There can be many connections on the same socket. vsocket provides an
> API to enable/disable particular virtio features on the fly, but it's
> the virtio_net that uses these features.
> 
> virtio_net used to rely on vsocket->features during feature negotiation,
> breaking the layer encapsulation (and yet causing a deadlock - two locks
> were being locked in a separate order). Now each virtio_net device has
> it's own copy of vsocket features, created at the time of virtio_net
> creation.
> 
> vsocket->features have to be still present, as features can be
> enabled/disabled while no virtio_net device has been created yet.
> 
> Signed-off-by: Dariusz Stojaczyk <dariuszx.stojaczyk@intel.com>
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> ---
>   lib/librte_vhost/socket.c     |  2 +-
>   lib/librte_vhost/vhost.c      |  9 +++++----
>   lib/librte_vhost/vhost.h      |  8 +++++---
>   lib/librte_vhost/vhost_user.c | 33 +++++++++++++++++----------------
>   4 files changed, 28 insertions(+), 24 deletions(-)

Thanks for the patch.
I think it is valid, but I would prefer we create a .backend_features
field to represent the features that the backend supports, and keep
.features for the negotiated features.

Do you agree to re-post with this change?

Thanks,
Maxime