[PATCH] vhost: fix vring enable with VDPA

[PATCH] vhost: fix vring enable with VDPA

[David Marchand]

For VDPA devices, vq are not locked once the device has been configured.
But we need to hold the vq lock to check if vhost async is enabled.

Combining VDPA and vhost async does not seem to make much sense, so
prevent this combination, and only assert the lock is taken when VDPA is
not configured on this vq.

Bugzilla ID: 1169
Fixes: 4b02c2673757 ("vhost: annotate async accesses")

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/vhost/vhost.c      | 4 ++--
 lib/vhost/vhost_user.c | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index c36dc06a0a..6b73c03224 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1754,7 +1754,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id)
 
 	vq = dev->virtqueue[queue_id];
 
-	if (unlikely(vq == NULL || !dev->async_copy))
+	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
 		return -1;
 
 	rte_spinlock_lock(&vq->access_lock);
@@ -1778,7 +1778,7 @@ rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id)
 
 	vq = dev->virtqueue[queue_id];
 
-	if (unlikely(vq == NULL || !dev->async_copy))
+	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
 		return -1;
 
 	vq_assert_lock(dev, vq);
diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index d702d082dd..daf3458985 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2177,10 +2177,10 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
 		enable, index);
 
 	vq = dev->virtqueue[index];
-	/* vhost_user_lock_all_queue_pairs locked all qps */
-	vq_assert_lock(dev, vq);
-	if (enable && vq->async) {
-		if (vq->async->pkts_inflight_n) {
+	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
+		/* vhost_user_lock_all_queue_pairs locked all qps */
+		vq_assert_lock(dev, vq);
+		if (enable && vq->async && vq->async->pkts_inflight_n) {
 			VHOST_LOG_CONFIG(dev->ifname, ERR,
 				"failed to enable vring. Inflight packets must be completed first\n");
 			return RTE_VHOST_MSG_RESULT_ERR;
-- 
2.39.2

Re: [PATCH] vhost: fix vring enable with VDPA

[Maxime Coquelin]

On 3/2/23 10:48, David Marchand wrote:
> For VDPA devices, vq are not locked once the device has been configured.
> But we need to hold the vq lock to check if vhost async is enabled.
> 
> Combining VDPA and vhost async does not seem to make much sense, so
> prevent this combination, and only assert the lock is taken when VDPA is
> not configured on this vq.
> 
> Bugzilla ID: 1169
> Fixes: 4b02c2673757 ("vhost: annotate async accesses")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/vhost/vhost.c      | 4 ++--
>   lib/vhost/vhost_user.c | 8 ++++----
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index c36dc06a0a..6b73c03224 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1754,7 +1754,7 @@ rte_vhost_async_channel_register(int vid, uint16_t queue_id)
>   
>   	vq = dev->virtqueue[queue_id];
>   
> -	if (unlikely(vq == NULL || !dev->async_copy))
> +	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
>   		return -1;
>   
>   	rte_spinlock_lock(&vq->access_lock);
> @@ -1778,7 +1778,7 @@ rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id)
>   
>   	vq = dev->virtqueue[queue_id];
>   
> -	if (unlikely(vq == NULL || !dev->async_copy))
> +	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
>   		return -1;
>   
>   	vq_assert_lock(dev, vq);
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index d702d082dd..daf3458985 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -2177,10 +2177,10 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
>   		enable, index);
>   
>   	vq = dev->virtqueue[index];
> -	/* vhost_user_lock_all_queue_pairs locked all qps */
> -	vq_assert_lock(dev, vq);
> -	if (enable && vq->async) {
> -		if (vq->async->pkts_inflight_n) {
> +	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
> +		/* vhost_user_lock_all_queue_pairs locked all qps */
> +		vq_assert_lock(dev, vq);
> +		if (enable && vq->async && vq->async->pkts_inflight_n) {
>   			VHOST_LOG_CONFIG(dev->ifname, ERR,
>   				"failed to enable vring. Inflight packets must be completed first\n");
>   			return RTE_VHOST_MSG_RESULT_ERR;

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

RE: [PATCH] vhost: fix vring enable with VDPA

[Xia, Chenbo]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, March 2, 2023 5:48 PM
> To: dev@dpdk.org
> Cc: ssimma@nvidia.com; yajunw@nvidia.com; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Xia, Chenbo <chenbo.xia@intel.com>; Morten
> Brørup <mb@smartsharesystems.com>
> Subject: [PATCH] vhost: fix vring enable with VDPA
> 
> For VDPA devices, vq are not locked once the device has been configured.
> But we need to hold the vq lock to check if vhost async is enabled.
> 
> Combining VDPA and vhost async does not seem to make much sense, so
> prevent this combination, and only assert the lock is taken when VDPA is
> not configured on this vq.
> 
> Bugzilla ID: 1169
> Fixes: 4b02c2673757 ("vhost: annotate async accesses")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/vhost/vhost.c      | 4 ++--
>  lib/vhost/vhost_user.c | 8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index c36dc06a0a..6b73c03224 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1754,7 +1754,7 @@ rte_vhost_async_channel_register(int vid, uint16_t
> queue_id)
> 
>  	vq = dev->virtqueue[queue_id];
> 
> -	if (unlikely(vq == NULL || !dev->async_copy))
> +	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
>  		return -1;
> 
>  	rte_spinlock_lock(&vq->access_lock);
> @@ -1778,7 +1778,7 @@ rte_vhost_async_channel_register_thread_unsafe(int
> vid, uint16_t queue_id)
> 
>  	vq = dev->virtqueue[queue_id];
> 
> -	if (unlikely(vq == NULL || !dev->async_copy))
> +	if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
>  		return -1;
> 
>  	vq_assert_lock(dev, vq);
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index d702d082dd..daf3458985 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -2177,10 +2177,10 @@ vhost_user_set_vring_enable(struct virtio_net
> **pdev,
>  		enable, index);
> 
>  	vq = dev->virtqueue[index];
> -	/* vhost_user_lock_all_queue_pairs locked all qps */
> -	vq_assert_lock(dev, vq);
> -	if (enable && vq->async) {
> -		if (vq->async->pkts_inflight_n) {
> +	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
> +		/* vhost_user_lock_all_queue_pairs locked all qps */
> +		vq_assert_lock(dev, vq);
> +		if (enable && vq->async && vq->async->pkts_inflight_n) {
>  			VHOST_LOG_CONFIG(dev->ifname, ERR,
>  				"failed to enable vring. Inflight packets must be
> completed first\n");
>  			return RTE_VHOST_MSG_RESULT_ERR;
> --
> 2.39.2

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

RE: [PATCH] vhost: fix vring enable with VDPA

[Ali Alnubani]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, March 2, 2023 11:48 AM
> To: dev@dpdk.org
> Cc: Semion Simma <ssimma@nvidia.com>; Yajun Wu <yajunw@nvidia.com>;
> Maxime Coquelin <maxime.coquelin@redhat.com>; Chenbo Xia
> <chenbo.xia@intel.com>; Morten Brørup <mb@smartsharesystems.com>
> Subject: [PATCH] vhost: fix vring enable with VDPA
> 
> For VDPA devices, vq are not locked once the device has been configured.
> But we need to hold the vq lock to check if vhost async is enabled.
> 
> Combining VDPA and vhost async does not seem to make much sense, so
> prevent this combination, and only assert the lock is taken when VDPA is
> not configured on this vq.
> 
> Bugzilla ID: 1169
> Fixes: 4b02c2673757 ("vhost: annotate async accesses")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Verified it resolves the issue, thanks David.

Tested-by: Ali Alnubani <alialnu@nvidia.com>

Re: [PATCH] vhost: fix vring enable with VDPA

[Maxime Coquelin]

On 3/2/23 10:48, David Marchand wrote:
> For VDPA devices, vq are not locked once the device has been configured.
> But we need to hold the vq lock to check if vhost async is enabled.
> 
> Combining VDPA and vhost async does not seem to make much sense, so
> prevent this combination, and only assert the lock is taken when VDPA is
> not configured on this vq.
> 
> Bugzilla ID: 1169
> Fixes: 4b02c2673757 ("vhost: annotate async accesses")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/vhost/vhost.c      | 4 ++--
>   lib/vhost/vhost_user.c | 8 ++++----
>   2 files changed, 6 insertions(+), 6 deletions(-)
> 

Applied to dpdk-next-virtio/main.

Thanks,
Maxime