[RFC 01/27] vhost: fix missing guest notif stat increment

[RFC 01/27] vhost: fix missing guest notif stat increment

[Maxime Coquelin]

Guest notification counter was only incremented for split
ring, this patch adds it also for packed ring.

Fixes: 1ea74efd7fa4 ("vhost: add statistics for guest notification")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
index 8fdab13c70..8554ab4002 100644
--- a/lib/vhost/vhost.h
+++ b/lib/vhost/vhost.h
@@ -973,6 +973,8 @@ vhost_vring_call_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
 kick:
 	if (kick) {
 		eventfd_write(vq->callfd, (eventfd_t)1);
+		if (dev->flags & VIRTIO_DEV_STATS_ENABLED)
+			vq->stats.guest_notifications++;
 		if (dev->notify_ops->guest_notified)
 			dev->notify_ops->guest_notified(dev->vid);
 	}
-- 
2.39.2

RE: [RFC 01/27] vhost: fix missing guest notif stat increment

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, March 31, 2023 11:43 PM
> To: dev@dpdk.org; david.marchand@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>; mkp@redhat.com; fbl@redhat.com;
> jasowang@redhat.com; Liang, Cunming <cunming.liang@intel.com>; Xie, Yongji
> <xieyongji@bytedance.com>; echaudro@redhat.com; eperezma@redhat.com;
> amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [RFC 01/27] vhost: fix missing guest notif stat increment
> 
> Guest notification counter was only incremented for split
> ring, this patch adds it also for packed ring.
> 
> Fixes: 1ea74efd7fa4 ("vhost: add statistics for guest notification")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/vhost.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
> index 8fdab13c70..8554ab4002 100644
> --- a/lib/vhost/vhost.h
> +++ b/lib/vhost/vhost.h
> @@ -973,6 +973,8 @@ vhost_vring_call_packed(struct virtio_net *dev, struct
> vhost_virtqueue *vq)
>  kick:
>  	if (kick) {
>  		eventfd_write(vq->callfd, (eventfd_t)1);
> +		if (dev->flags & VIRTIO_DEV_STATS_ENABLED)
> +			vq->stats.guest_notifications++;
>  		if (dev->notify_ops->guest_notified)
>  			dev->notify_ops->guest_notified(dev->vid);
>  	}
> --
> 2.39.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

[RFC 02/27] vhost: fix invalid call FD handling

[Maxime Coquelin]

This patch fixes cases where IRQ injection is tried while
the call FD is not valid, which should not happen.

Fixes: b1cce26af1dc ("vhost: add notification for packed ring")
Fixes: e37ff954405a ("vhost: support virtqueue interrupt/notification suppression")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
index 8554ab4002..40863f7bfd 100644
--- a/lib/vhost/vhost.h
+++ b/lib/vhost/vhost.h
@@ -902,9 +902,9 @@ vhost_vring_call_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
 			"%s: used_event_idx=%d, old=%d, new=%d\n",
 			__func__, vhost_used_event(vq), old, new);
 
-		if ((vhost_need_event(vhost_used_event(vq), new, old) &&
-					(vq->callfd >= 0)) ||
-				unlikely(!signalled_used_valid)) {
+		if ((vhost_need_event(vhost_used_event(vq), new, old) ||
+					unlikely(!signalled_used_valid)) &&
+				vq->callfd >= 0) {
 			eventfd_write(vq->callfd, (eventfd_t) 1);
 			if (dev->flags & VIRTIO_DEV_STATS_ENABLED)
 				vq->stats.guest_notifications++;
@@ -971,7 +971,7 @@ vhost_vring_call_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
 	if (vhost_need_event(off, new, old))
 		kick = true;
 kick:
-	if (kick) {
+	if (kick && vq->callfd >= 0) {
 		eventfd_write(vq->callfd, (eventfd_t)1);
 		if (dev->flags & VIRTIO_DEV_STATS_ENABLED)
 			vq->stats.guest_notifications++;
-- 
2.39.2

RE: [RFC 02/27] vhost: fix invalid call FD handling

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, March 31, 2023 11:43 PM
> To: dev@dpdk.org; david.marchand@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>; mkp@redhat.com; fbl@redhat.com;
> jasowang@redhat.com; Liang, Cunming <cunming.liang@intel.com>; Xie, Yongji
> <xieyongji@bytedance.com>; echaudro@redhat.com; eperezma@redhat.com;
> amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [RFC 02/27] vhost: fix invalid call FD handling
> 
> This patch fixes cases where IRQ injection is tried while
> the call FD is not valid, which should not happen.
> 
> Fixes: b1cce26af1dc ("vhost: add notification for packed ring")
> Fixes: e37ff954405a ("vhost: support virtqueue interrupt/notification
> suppression")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/vhost.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
> index 8554ab4002..40863f7bfd 100644
> --- a/lib/vhost/vhost.h
> +++ b/lib/vhost/vhost.h
> @@ -902,9 +902,9 @@ vhost_vring_call_split(struct virtio_net *dev, struct
> vhost_virtqueue *vq)
>  			"%s: used_event_idx=%d, old=%d, new=%d\n",
>  			__func__, vhost_used_event(vq), old, new);
> 
> -		if ((vhost_need_event(vhost_used_event(vq), new, old) &&
> -					(vq->callfd >= 0)) ||
> -				unlikely(!signalled_used_valid)) {
> +		if ((vhost_need_event(vhost_used_event(vq), new, old) ||
> +					unlikely(!signalled_used_valid)) &&
> +				vq->callfd >= 0) {
>  			eventfd_write(vq->callfd, (eventfd_t) 1);
>  			if (dev->flags & VIRTIO_DEV_STATS_ENABLED)
>  				vq->stats.guest_notifications++;
> @@ -971,7 +971,7 @@ vhost_vring_call_packed(struct virtio_net *dev, struct
> vhost_virtqueue *vq)
>  	if (vhost_need_event(off, new, old))
>  		kick = true;
>  kick:
> -	if (kick) {
> +	if (kick && vq->callfd >= 0) {
>  		eventfd_write(vq->callfd, (eventfd_t)1);
>  		if (dev->flags & VIRTIO_DEV_STATS_ENABLED)
>  			vq->stats.guest_notifications++;
> --
> 2.39.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

[RFC 03/27] vhost: fix IOTLB entries overlap check with previous entry

[Maxime Coquelin]

Commit 22b6d0ac691a ("vhost: fix madvise IOTLB entries pages overlap check")
fixed the check to ensure the entry to be removed does not
overlap with the next one in the IOTLB cache before marking
it as DONTDUMP with madvise(). This is not enough, because
the same issue is present when comparing with the previous
entry in the cache, where the end address of the previous
entry should be used, not the start one.

Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/iotlb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/vhost/iotlb.c b/lib/vhost/iotlb.c
index 3f45bc6061..870c8acb88 100644
--- a/lib/vhost/iotlb.c
+++ b/lib/vhost/iotlb.c
@@ -178,8 +178,8 @@ vhost_user_iotlb_cache_random_evict(struct virtio_net *dev, struct vhost_virtque
 			mask = ~(alignment - 1);
 
 			/* Don't disable coredump if the previous node is in the same page */
-			if (prev_node == NULL ||
-					(node->uaddr & mask) != (prev_node->uaddr & mask)) {
+			if (prev_node == NULL || (node->uaddr & mask) !=
+					((prev_node->uaddr + prev_node->size - 1) & mask)) {
 				next_node = RTE_TAILQ_NEXT(node, next);
 				/* Don't disable coredump if the next node is in the same page */
 				if (next_node == NULL || ((node->uaddr + node->size - 1) & mask) !=
@@ -283,8 +283,8 @@ vhost_user_iotlb_cache_remove(struct virtio_net *dev, struct vhost_virtqueue *vq
 			mask = ~(alignment-1);
 
 			/* Don't disable coredump if the previous node is in the same page */
-			if (prev_node == NULL ||
-					(node->uaddr & mask) != (prev_node->uaddr & mask)) {
+			if (prev_node == NULL || (node->uaddr & mask) !=
+					((prev_node->uaddr + prev_node->size - 1) & mask)) {
 				next_node = RTE_TAILQ_NEXT(node, next);
 				/* Don't disable coredump if the next node is in the same page */
 				if (next_node == NULL || ((node->uaddr + node->size - 1) & mask) !=
-- 
2.39.2

Re: [RFC 03/27] vhost: fix IOTLB entries overlap check with previous entry

[Mike Pattrick]

On Fri, Mar 31, 2023 at 11:43 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> Commit 22b6d0ac691a ("vhost: fix madvise IOTLB entries pages overlap check")
> fixed the check to ensure the entry to be removed does not
> overlap with the next one in the IOTLB cache before marking
> it as DONTDUMP with madvise(). This is not enough, because
> the same issue is present when comparing with the previous
> entry in the cache, where the end address of the previous
> entry should be used, not the start one.
>
> Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Hi Maxime,

This makes sense.

Acked-by: Mike Pattrick <mkp@redhat.com>

> ---
>  lib/vhost/iotlb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/vhost/iotlb.c b/lib/vhost/iotlb.c
> index 3f45bc6061..870c8acb88 100644
> --- a/lib/vhost/iotlb.c
> +++ b/lib/vhost/iotlb.c
> @@ -178,8 +178,8 @@ vhost_user_iotlb_cache_random_evict(struct virtio_net *dev, struct vhost_virtque
>                         mask = ~(alignment - 1);
>
>                         /* Don't disable coredump if the previous node is in the same page */
> -                       if (prev_node == NULL ||
> -                                       (node->uaddr & mask) != (prev_node->uaddr & mask)) {
> +                       if (prev_node == NULL || (node->uaddr & mask) !=
> +                                       ((prev_node->uaddr + prev_node->size - 1) & mask)) {
>                                 next_node = RTE_TAILQ_NEXT(node, next);
>                                 /* Don't disable coredump if the next node is in the same page */
>                                 if (next_node == NULL || ((node->uaddr + node->size - 1) & mask) !=
> @@ -283,8 +283,8 @@ vhost_user_iotlb_cache_remove(struct virtio_net *dev, struct vhost_virtqueue *vq
>                         mask = ~(alignment-1);
>
>                         /* Don't disable coredump if the previous node is in the same page */
> -                       if (prev_node == NULL ||
> -                                       (node->uaddr & mask) != (prev_node->uaddr & mask)) {
> +                       if (prev_node == NULL || (node->uaddr & mask) !=
> +                                       ((prev_node->uaddr + prev_node->size - 1) & mask)) {
>                                 next_node = RTE_TAILQ_NEXT(node, next);
>                                 /* Don't disable coredump if the next node is in the same page */
>                                 if (next_node == NULL || ((node->uaddr + node->size - 1) & mask) !=
> --
> 2.39.2
>

RE: [RFC 03/27] vhost: fix IOTLB entries overlap check with previous entry

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, March 31, 2023 11:43 PM
> To: dev@dpdk.org; david.marchand@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>; mkp@redhat.com; fbl@redhat.com;
> jasowang@redhat.com; Liang, Cunming <cunming.liang@intel.com>; Xie, Yongji
> <xieyongji@bytedance.com>; echaudro@redhat.com; eperezma@redhat.com;
> amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [RFC 03/27] vhost: fix IOTLB entries overlap check with previous
> entry
> 
> Commit 22b6d0ac691a ("vhost: fix madvise IOTLB entries pages overlap
> check")
> fixed the check to ensure the entry to be removed does not
> overlap with the next one in the IOTLB cache before marking
> it as DONTDUMP with madvise(). This is not enough, because
> the same issue is present when comparing with the previous
> entry in the cache, where the end address of the previous
> entry should be used, not the start one.
> 
> Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/iotlb.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/vhost/iotlb.c b/lib/vhost/iotlb.c
> index 3f45bc6061..870c8acb88 100644
> --- a/lib/vhost/iotlb.c
> +++ b/lib/vhost/iotlb.c
> @@ -178,8 +178,8 @@ vhost_user_iotlb_cache_random_evict(struct virtio_net
> *dev, struct vhost_virtque
>  			mask = ~(alignment - 1);
> 
>  			/* Don't disable coredump if the previous node is in the
> same page */
> -			if (prev_node == NULL ||
> -					(node->uaddr & mask) != (prev_node->uaddr &
> mask)) {
> +			if (prev_node == NULL || (node->uaddr & mask) !=
> +					((prev_node->uaddr + prev_node->size - 1) &
> mask)) {
>  				next_node = RTE_TAILQ_NEXT(node, next);
>  				/* Don't disable coredump if the next node is in
> the same page */
>  				if (next_node == NULL || ((node->uaddr + node-
> >size - 1) & mask) !=
> @@ -283,8 +283,8 @@ vhost_user_iotlb_cache_remove(struct virtio_net *dev,
> struct vhost_virtqueue *vq
>  			mask = ~(alignment-1);
> 
>  			/* Don't disable coredump if the previous node is in the
> same page */
> -			if (prev_node == NULL ||
> -					(node->uaddr & mask) != (prev_node->uaddr &
> mask)) {
> +			if (prev_node == NULL || (node->uaddr & mask) !=
> +					((prev_node->uaddr + prev_node->size - 1) &
> mask)) {
>  				next_node = RTE_TAILQ_NEXT(node, next);
>  				/* Don't disable coredump if the next node is in
> the same page */
>  				if (next_node == NULL || ((node->uaddr + node-
> >size - 1) & mask) !=
> --
> 2.39.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

[RFC 06/27] vhost: don't dump unneeded pages with IOTLB

[Maxime Coquelin]

On IOTLB entry removal, previous fixes took care of not
marking pages shared with other IOTLB entries as DONTDUMP.

However, if an IOTLB entry is spanned on multiple pages,
the other pages were kept as DODUMP while they might not
have been shared with other entries, increasing needlessly
the coredump size.

This patch addresses this issue by excluding only the
shared pages from madvise's DONTDUMP.

Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/iotlb.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/lib/vhost/iotlb.c b/lib/vhost/iotlb.c
index d919f74704..f598c0a8c4 100644
--- a/lib/vhost/iotlb.c
+++ b/lib/vhost/iotlb.c
@@ -54,16 +54,23 @@ static void
 vhost_user_iotlb_clear_dump(struct virtio_net *dev, struct vhost_iotlb_entry *node,
 		struct vhost_iotlb_entry *prev, struct vhost_iotlb_entry *next)
 {
-	uint64_t align;
+	uint64_t align, start, end;
+
+	start = node->uaddr;
+	end = node->uaddr + node->size;
 
 	align = hua_to_alignment(dev->mem, (void *)(uintptr_t)node->uaddr);
 
-	/* Don't disable coredump if the previous node is in the same page */
-	if (!vhost_user_iotlb_share_page(prev, node, align)) {
-		/* Don't disable coredump if the next node is in the same page */
-		if (!vhost_user_iotlb_share_page(node, next, align))
-			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size, false, align);
-	}
+	/* Skip first page if shared with previous entry. */
+	if (vhost_user_iotlb_share_page(prev, node, align))
+		start = RTE_ALIGN_CEIL(start, align);
+
+	/* Skip last page if shared with next entry. */
+	if (vhost_user_iotlb_share_page(node, next, align))
+		end = RTE_ALIGN_FLOOR(end, align);
+
+	if (end > start)
+		mem_set_dump((void *)(uintptr_t)start, end - start, false, align);
 }
 
 static struct vhost_iotlb_entry *
-- 
2.39.2

Re: [RFC 06/27] vhost: don't dump unneeded pages with IOTLB

[Mike Pattrick]

On Fri, Mar 31, 2023 at 11:43 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> On IOTLB entry removal, previous fixes took care of not
> marking pages shared with other IOTLB entries as DONTDUMP.
>
> However, if an IOTLB entry is spanned on multiple pages,
> the other pages were kept as DODUMP while they might not
> have been shared with other entries, increasing needlessly
> the coredump size.
>
> This patch addresses this issue by excluding only the
> shared pages from madvise's DONTDUMP.
>
> Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Looks good to me.

Acked-by: Mike Pattrick <mkp@redhat.com>

RE: [RFC 06/27] vhost: don't dump unneeded pages with IOTLB

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, March 31, 2023 11:43 PM
> To: dev@dpdk.org; david.marchand@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>; mkp@redhat.com; fbl@redhat.com;
> jasowang@redhat.com; Liang, Cunming <cunming.liang@intel.com>; Xie, Yongji
> <xieyongji@bytedance.com>; echaudro@redhat.com; eperezma@redhat.com;
> amorenoz@redhat.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [RFC 06/27] vhost: don't dump unneeded pages with IOTLB
> 
> On IOTLB entry removal, previous fixes took care of not
> marking pages shared with other IOTLB entries as DONTDUMP.
> 
> However, if an IOTLB entry is spanned on multiple pages,
> the other pages were kept as DODUMP while they might not
> have been shared with other entries, increasing needlessly
> the coredump size.
> 
> This patch addresses this issue by excluding only the
> shared pages from madvise's DONTDUMP.
> 
> Fixes: dea092d0addb ("vhost: fix madvise arguments alignment")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/iotlb.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/vhost/iotlb.c b/lib/vhost/iotlb.c
> index d919f74704..f598c0a8c4 100644
> --- a/lib/vhost/iotlb.c
> +++ b/lib/vhost/iotlb.c
> @@ -54,16 +54,23 @@ static void
>  vhost_user_iotlb_clear_dump(struct virtio_net *dev, struct
> vhost_iotlb_entry *node,
>  		struct vhost_iotlb_entry *prev, struct vhost_iotlb_entry *next)
>  {
> -	uint64_t align;
> +	uint64_t align, start, end;
> +
> +	start = node->uaddr;
> +	end = node->uaddr + node->size;
> 
>  	align = hua_to_alignment(dev->mem, (void *)(uintptr_t)node->uaddr);
> 
> -	/* Don't disable coredump if the previous node is in the same page
> */
> -	if (!vhost_user_iotlb_share_page(prev, node, align)) {
> -		/* Don't disable coredump if the next node is in the same page
> */
> -		if (!vhost_user_iotlb_share_page(node, next, align))
> -			mem_set_dump((void *)(uintptr_t)node->uaddr, node->size,
> false, align);
> -	}
> +	/* Skip first page if shared with previous entry. */
> +	if (vhost_user_iotlb_share_page(prev, node, align))
> +		start = RTE_ALIGN_CEIL(start, align);
> +
> +	/* Skip last page if shared with next entry. */
> +	if (vhost_user_iotlb_share_page(node, next, align))
> +		end = RTE_ALIGN_FLOOR(end, align);
> +
> +	if (end > start)
> +		mem_set_dump((void *)(uintptr_t)start, end - start, false,
> align);
>  }
> 
>  static struct vhost_iotlb_entry *
> --
> 2.39.2

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>