[dpdk-dev] [PATCH 1/3] vhost: Fix return value of GET_VRING_BASE message

[dpdk-dev] [PATCH 1/3] vhost: Fix return value of GET_VRING_BASE message

[Tetsuya Mukawa]

When vhost-user frontend sends GET_VRING_BASE, last used index of vring
should be returned. In DPDK vhost library, 'last_used_idx' represents it.
But the value can be over max index value. To return correct value to
vhost frontend, it's needed to be masked.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_vhost/virtio-net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index b520ec5..144f301 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -661,7 +661,8 @@ get_vring_base(struct vhost_device_ctx ctx, uint32_t index,
 
 	state->index = index;
 	/* State->index refers to the queue index. The txq is 1, rxq is 0. */
-	state->num = dev->virtqueue[state->index]->last_used_idx;
+	state->num = dev->virtqueue[state->index]->last_used_idx
+			& (dev->virtqueue[state->index]->size - 1);
 
 	return 0;
 }
-- 
2.1.4

[dpdk-dev] [PATCH 2/3] vhost: Fix RESET_OWNER handling not to close callfd

[Tetsuya Mukawa]

When RESET_OWNER message is issued, vhost backend shouldn't close
'callfd', because it will be valid while vhost-user connection
is established. It should be closed when connection is closed.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_vhost/virtio-net.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 144f301..7794e8b 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -189,13 +189,9 @@ cleanup_device(struct virtio_net *dev)
 		free(dev->mem);
 	}
 
-	/* Close any event notifiers opened by device. */
-	if ((int)dev->virtqueue[VIRTIO_RXQ]->callfd >= 0)
-		close((int)dev->virtqueue[VIRTIO_RXQ]->callfd);
+	/* Close kickfd event notifiers opened by device. */
 	if ((int)dev->virtqueue[VIRTIO_RXQ]->kickfd >= 0)
 		close((int)dev->virtqueue[VIRTIO_RXQ]->kickfd);
-	if ((int)dev->virtqueue[VIRTIO_TXQ]->callfd >= 0)
-		close((int)dev->virtqueue[VIRTIO_TXQ]->callfd);
 	if ((int)dev->virtqueue[VIRTIO_TXQ]->kickfd >= 0)
 		close((int)dev->virtqueue[VIRTIO_TXQ]->kickfd);
 }
@@ -206,6 +202,12 @@ cleanup_device(struct virtio_net *dev)
 static void
 free_device(struct virtio_net_config_ll *ll_dev)
 {
+	/* close callfd when connection is closed */
+	if ((int)ll_dev->dev.virtqueue[VIRTIO_RXQ]->callfd >= 0)
+		close((int)ll_dev->dev.virtqueue[VIRTIO_RXQ]->callfd);
+	if ((int)ll_dev->dev.virtqueue[VIRTIO_TXQ]->callfd >= 0)
+		close((int)ll_dev->dev.virtqueue[VIRTIO_TXQ]->callfd);
+
 	/* Free any malloc'd memory */
 	rte_free(ll_dev->dev.virtqueue[VIRTIO_RXQ]);
 	rte_free(ll_dev->dev.virtqueue[VIRTIO_TXQ]);
@@ -241,6 +243,21 @@ rm_config_ll_entry(struct virtio_net_config_ll *ll_dev,
 	}
 }
 
+
+/*
+ *  Rset variables in device structure.
+ */
+static void
+reset_device(struct virtio_net *dev)
+{
+	dev->virtqueue[VIRTIO_RXQ]->kickfd = (eventfd_t)-1;
+	dev->virtqueue[VIRTIO_TXQ]->kickfd = (eventfd_t)-1;
+
+	/* Backends are set to -1 indicating an inactive device. */
+	dev->virtqueue[VIRTIO_RXQ]->backend = VIRTIO_DEV_STOPPED;
+	dev->virtqueue[VIRTIO_TXQ]->backend = VIRTIO_DEV_STOPPED;
+}
+
 /*
  *  Initialise all variables in device structure.
  */
@@ -261,14 +278,10 @@ init_device(struct virtio_net *dev)
 	memset(dev->virtqueue[VIRTIO_RXQ], 0, sizeof(struct vhost_virtqueue));
 	memset(dev->virtqueue[VIRTIO_TXQ], 0, sizeof(struct vhost_virtqueue));
 
-	dev->virtqueue[VIRTIO_RXQ]->kickfd = (eventfd_t)-1;
 	dev->virtqueue[VIRTIO_RXQ]->callfd = (eventfd_t)-1;
-	dev->virtqueue[VIRTIO_TXQ]->kickfd = (eventfd_t)-1;
 	dev->virtqueue[VIRTIO_TXQ]->callfd = (eventfd_t)-1;
 
-	/* Backends are set to -1 indicating an inactive device. */
-	dev->virtqueue[VIRTIO_RXQ]->backend = VIRTIO_DEV_STOPPED;
-	dev->virtqueue[VIRTIO_TXQ]->backend = VIRTIO_DEV_STOPPED;
+	reset_device(dev);
 }
 
 /*
@@ -403,7 +416,7 @@ reset_owner(struct vhost_device_ctx ctx)
 	ll_dev = get_config_ll_entry(ctx);
 
 	cleanup_device(&ll_dev->dev);
-	init_device(&ll_dev->dev);
+	reset_device(&ll_dev->dev);
 
 	return 0;
 }
-- 
2.1.4

[dpdk-dev] [PATCH 3/3] vhost: Fix RESET_OWNER handling not to free virtqueue

[Tetsuya Mukawa]

When RESET_OWNER message is issued, vhost backend should not clear and
free virtqueue memories. This is because vhost backend should handle
GET_VRING_BASE message followed by the RESET_OWNER, so still vhost
backend need to access virtqueue memories.
These memories should be freed when SET_MEM_TABLE message is issued,
and vhost library has already had such a functionality.

Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
---
 lib/librte_vhost/virtio-net.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 7794e8b..9475d00 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -182,13 +182,6 @@ add_config_ll_entry(struct virtio_net_config_ll *new_ll_dev)
 static void
 cleanup_device(struct virtio_net *dev)
 {
-	/* Unmap QEMU memory file if mapped. */
-	if (dev->mem) {
-		munmap((void *)(uintptr_t)dev->mem->mapped_address,
-			(size_t)dev->mem->mapped_size);
-		free(dev->mem);
-	}
-
 	/* Close kickfd event notifiers opened by device. */
 	if ((int)dev->virtqueue[VIRTIO_RXQ]->kickfd >= 0)
 		close((int)dev->virtqueue[VIRTIO_RXQ]->kickfd);
-- 
2.1.4

Re: [dpdk-dev] [PATCH 1/3] vhost: Fix return value of GET_VRING_BASE message

[Thomas Monjalon]

Huawei, Changchun,
Please could you review this series?
Thanks

Re: [dpdk-dev] [PATCH 1/3] vhost: Fix return value of GET_VRING_BASE message

[Xie, Huawei]

On 9/25/2015 9:15 PM, Thomas Monjalon wrote:
> Huawei, Changchun,
> Please could you review this series?
> Thanks
>
>
Sorry, recently quite busy. Would check this and Pavel's patch after
Chinese Holiday(Oct.1-Oct.7).
Btw, Changchun left Intel and could be no longer reached by this mail
address. I think he will show up with another address soon.

Re: [dpdk-dev] [PATCH 2/3] vhost: Fix RESET_OWNER handling not to close callfd

[Yuanhan Liu]

On Wed, Aug 19, 2015 at 06:51:08PM +0900, Tetsuya Mukawa wrote:
> When RESET_OWNER message is issued, vhost backend shouldn't close
> 'callfd', because it will be valid while vhost-user connection
> is established. It should be closed when connection is closed.

Doesn't it mean the end of connection when RESET_OWNER is received?

If you check my latest patch set for enabling vhost mq, you will
find free_device() will be invoked immediately once RESET_OWNER
signal is received.

    http://dpdk.org/ml/archives/dev/2015-September/023752.html

So, I doubt this patch is necessary.

BTW, does these 3 patches fix a real issue, or just some potential
issue in your mind? I saw that you mentioned your vhost pmd won't
work without those 3 patches, but I never saw you mentioned what
issue you fixed exactly. And it'd be good if you can post them in
commit log.

And, if we encounter same issue, will my above patch fix yours?

	--yliu
> 
> Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
> ---
>  lib/librte_vhost/virtio-net.c | 35 ++++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index 144f301..7794e8b 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -189,13 +189,9 @@ cleanup_device(struct virtio_net *dev)
>  		free(dev->mem);
>  	}
>  
> -	/* Close any event notifiers opened by device. */
> -	if ((int)dev->virtqueue[VIRTIO_RXQ]->callfd >= 0)
> -		close((int)dev->virtqueue[VIRTIO_RXQ]->callfd);
> +	/* Close kickfd event notifiers opened by device. */
>  	if ((int)dev->virtqueue[VIRTIO_RXQ]->kickfd >= 0)
>  		close((int)dev->virtqueue[VIRTIO_RXQ]->kickfd);
> -	if ((int)dev->virtqueue[VIRTIO_TXQ]->callfd >= 0)
> -		close((int)dev->virtqueue[VIRTIO_TXQ]->callfd);
>  	if ((int)dev->virtqueue[VIRTIO_TXQ]->kickfd >= 0)
>  		close((int)dev->virtqueue[VIRTIO_TXQ]->kickfd);
>  }
> @@ -206,6 +202,12 @@ cleanup_device(struct virtio_net *dev)
>  static void
>  free_device(struct virtio_net_config_ll *ll_dev)
>  {
> +	/* close callfd when connection is closed */
> +	if ((int)ll_dev->dev.virtqueue[VIRTIO_RXQ]->callfd >= 0)
> +		close((int)ll_dev->dev.virtqueue[VIRTIO_RXQ]->callfd);
> +	if ((int)ll_dev->dev.virtqueue[VIRTIO_TXQ]->callfd >= 0)
> +		close((int)ll_dev->dev.virtqueue[VIRTIO_TXQ]->callfd);
> +
>  	/* Free any malloc'd memory */
>  	rte_free(ll_dev->dev.virtqueue[VIRTIO_RXQ]);
>  	rte_free(ll_dev->dev.virtqueue[VIRTIO_TXQ]);
> @@ -241,6 +243,21 @@ rm_config_ll_entry(struct virtio_net_config_ll *ll_dev,
>  	}
>  }
>  
> +
> +/*
> + *  Rset variables in device structure.
> + */
> +static void
> +reset_device(struct virtio_net *dev)
> +{
> +	dev->virtqueue[VIRTIO_RXQ]->kickfd = (eventfd_t)-1;
> +	dev->virtqueue[VIRTIO_TXQ]->kickfd = (eventfd_t)-1;
> +
> +	/* Backends are set to -1 indicating an inactive device. */
> +	dev->virtqueue[VIRTIO_RXQ]->backend = VIRTIO_DEV_STOPPED;
> +	dev->virtqueue[VIRTIO_TXQ]->backend = VIRTIO_DEV_STOPPED;
> +}
> +
>  /*
>   *  Initialise all variables in device structure.
>   */
> @@ -261,14 +278,10 @@ init_device(struct virtio_net *dev)
>  	memset(dev->virtqueue[VIRTIO_RXQ], 0, sizeof(struct vhost_virtqueue));
>  	memset(dev->virtqueue[VIRTIO_TXQ], 0, sizeof(struct vhost_virtqueue));
>  
> -	dev->virtqueue[VIRTIO_RXQ]->kickfd = (eventfd_t)-1;
>  	dev->virtqueue[VIRTIO_RXQ]->callfd = (eventfd_t)-1;
> -	dev->virtqueue[VIRTIO_TXQ]->kickfd = (eventfd_t)-1;
>  	dev->virtqueue[VIRTIO_TXQ]->callfd = (eventfd_t)-1;
>  
> -	/* Backends are set to -1 indicating an inactive device. */
> -	dev->virtqueue[VIRTIO_RXQ]->backend = VIRTIO_DEV_STOPPED;
> -	dev->virtqueue[VIRTIO_TXQ]->backend = VIRTIO_DEV_STOPPED;
> +	reset_device(dev);
>  }
>  
>  /*
> @@ -403,7 +416,7 @@ reset_owner(struct vhost_device_ctx ctx)
>  	ll_dev = get_config_ll_entry(ctx);
>  
>  	cleanup_device(&ll_dev->dev);
> -	init_device(&ll_dev->dev);
> +	reset_device(&ll_dev->dev);
>  
>  	return 0;
>  }
> -- 
> 2.1.4