[PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[Eli Cohen]

When we suspend the VM, the VDPA interface will be reset. When the VM is
resumed again, clear_virtqueues() will clear the available and used
indices resulting in hardware virqtqueue objects becoming out of sync.
We can avoid this function alltogether since qemu will clear them if
required, e.g. when the VM went through a reboot.

Moreover, since the hw available and used indices should always be
identical on query and should be restored to the same value same value
for virtqueues that complete in order, we set the single value provided
by set_vq_state(). In get_vq_state() we return the value of hardware
used index.

Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
Signed-off-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b8e9d525d66c..a51b0f86afe2 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
 		return;
 	}
 	mvq->avail_idx = attr.available_index;
+	mvq->used_idx = attr.used_index;
 }
 
 static void suspend_vqs(struct mlx5_vdpa_net *ndev)
@@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
 		return -EINVAL;
 	}
 
+	mvq->used_idx = state->avail_index;
 	mvq->avail_idx = state->avail_index;
 	return 0;
 }
@@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
 	 * that cares about emulating the index after vq is stopped.
 	 */
 	if (!mvq->initialized) {
-		state->avail_index = mvq->avail_idx;
+		state->avail_index = mvq->used_idx;
 		return 0;
 	}
 
@@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
 		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
 		return err;
 	}
-	state->avail_index = attr.available_index;
+	state->avail_index = attr.used_index;
 	return 0;
 }
 
@@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
 	}
 }
 
-static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
-{
-	int i;
-
-	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
-		ndev->vqs[i].avail_idx = 0;
-		ndev->vqs[i].used_idx = 0;
-	}
-}
-
 /* TODO: cross-endian support */
 static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
 {
@@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
 	if (!status) {
 		mlx5_vdpa_info(mvdev, "performing device reset\n");
 		teardown_driver(ndev);
-		clear_virtqueues(ndev);
 		mlx5_vdpa_destroy_mr(&ndev->mvdev);
 		ndev->mvdev.status = 0;
 		++mvdev->generation;
-- 
2.29.2

Re: [PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[Si-Wei Liu]

On 2/16/2021 8:20 AM, Eli Cohen wrote:
> When we suspend the VM, the VDPA interface will be reset. When the VM is
> resumed again, clear_virtqueues() will clear the available and used
> indices resulting in hardware virqtqueue objects becoming out of sync.
> We can avoid this function alltogether since qemu will clear them if
> required, e.g. when the VM went through a reboot.
>
> Moreover, since the hw available and used indices should always be
> identical on query and should be restored to the same value same value
> for virtqueues that complete in order, we set the single value provided
> by set_vq_state(). In get_vq_state() we return the value of hardware
> used index.
>
> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>

> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
>   1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index b8e9d525d66c..a51b0f86afe2 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
>   		return;
>   	}
>   	mvq->avail_idx = attr.available_index;
> +	mvq->used_idx = attr.used_index;
>   }
>   
>   static void suspend_vqs(struct mlx5_vdpa_net *ndev)
> @@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
>   		return -EINVAL;
>   	}
>   
> +	mvq->used_idx = state->avail_index;
>   	mvq->avail_idx = state->avail_index;
>   	return 0;
>   }
> @@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
>   	 * that cares about emulating the index after vq is stopped.
>   	 */
>   	if (!mvq->initialized) {
> -		state->avail_index = mvq->avail_idx;
> +		state->avail_index = mvq->used_idx;
>   		return 0;
>   	}
>   
> @@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
>   		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
>   		return err;
>   	}
> -	state->avail_index = attr.available_index;
> +	state->avail_index = attr.used_index;
>   	return 0;
>   }
>   
> @@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
>   	}
>   }
>   
> -static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
> -{
> -	int i;
> -
> -	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
> -		ndev->vqs[i].avail_idx = 0;
> -		ndev->vqs[i].used_idx = 0;
> -	}
> -}
> -
>   /* TODO: cross-endian support */
>   static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
>   {
> @@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>   	if (!status) {
>   		mlx5_vdpa_info(mvdev, "performing device reset\n");
>   		teardown_driver(ndev);
> -		clear_virtqueues(ndev);
>   		mlx5_vdpa_destroy_mr(&ndev->mvdev);
>   		ndev->mvdev.status = 0;
>   		++mvdev->generation;

Re: [PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[Si-Wei Liu]

On 2/16/2021 8:20 AM, Eli Cohen wrote:
> When we suspend the VM, the VDPA interface will be reset. When the VM is
> resumed again, clear_virtqueues() will clear the available and used
> indices resulting in hardware virqtqueue objects becoming out of sync.
> We can avoid this function alltogether since qemu will clear them if
> required, e.g. when the VM went through a reboot.
>
> Moreover, since the hw available and used indices should always be
> identical on query and should be restored to the same value same value
> for virtqueues that complete in order, we set the single value provided
> by set_vq_state(). In get_vq_state() we return the value of hardware
> used index.
>
> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>

> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
>   1 file changed, 4 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index b8e9d525d66c..a51b0f86afe2 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
>   		return;
>   	}
>   	mvq->avail_idx = attr.available_index;
> +	mvq->used_idx = attr.used_index;
>   }
>   
>   static void suspend_vqs(struct mlx5_vdpa_net *ndev)
> @@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
>   		return -EINVAL;
>   	}
>   
> +	mvq->used_idx = state->avail_index;
>   	mvq->avail_idx = state->avail_index;
>   	return 0;
>   }
> @@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
>   	 * that cares about emulating the index after vq is stopped.
>   	 */
>   	if (!mvq->initialized) {
> -		state->avail_index = mvq->avail_idx;
> +		state->avail_index = mvq->used_idx;
>   		return 0;
>   	}
>   
> @@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
>   		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
>   		return err;
>   	}
> -	state->avail_index = attr.available_index;
> +	state->avail_index = attr.used_index;
>   	return 0;
>   }
>   
> @@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
>   	}
>   }
>   
> -static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
> -{
> -	int i;
> -
> -	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
> -		ndev->vqs[i].avail_idx = 0;
> -		ndev->vqs[i].used_idx = 0;
> -	}
> -}
> -
>   /* TODO: cross-endian support */
>   static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
>   {
> @@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>   	if (!status) {
>   		mlx5_vdpa_info(mvdev, "performing device reset\n");
>   		teardown_driver(ndev);
> -		clear_virtqueues(ndev);
>   		mlx5_vdpa_destroy_mr(&ndev->mvdev);
>   		ndev->mvdev.status = 0;
>   		++mvdev->generation;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[Michael S. Tsirkin]

On Wed, Feb 17, 2021 at 11:42:48AM -0800, Si-Wei Liu wrote:
> 
> 
> On 2/16/2021 8:20 AM, Eli Cohen wrote:
> > When we suspend the VM, the VDPA interface will be reset. When the VM is
> > resumed again, clear_virtqueues() will clear the available and used
> > indices resulting in hardware virqtqueue objects becoming out of sync.
> > We can avoid this function alltogether since qemu will clear them if
> > required, e.g. when the VM went through a reboot.
> > 
> > Moreover, since the hw available and used indices should always be
> > identical on query and should be restored to the same value same value
> > for virtqueues that complete in order, we set the single value provided
> > by set_vq_state(). In get_vq_state() we return the value of hardware
> > used index.
> > 
> > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>


Seems to also fix b35ccebe3ef76168aa2edaa35809c0232cb3578e, right?


> > ---
> >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
> >   1 file changed, 4 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index b8e9d525d66c..a51b0f86afe2 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
> >   		return;
> >   	}
> >   	mvq->avail_idx = attr.available_index;
> > +	mvq->used_idx = attr.used_index;
> >   }
> >   static void suspend_vqs(struct mlx5_vdpa_net *ndev)
> > @@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
> >   		return -EINVAL;
> >   	}
> > +	mvq->used_idx = state->avail_index;
> >   	mvq->avail_idx = state->avail_index;
> >   	return 0;
> >   }
> > @@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
> >   	 * that cares about emulating the index after vq is stopped.
> >   	 */
> >   	if (!mvq->initialized) {
> > -		state->avail_index = mvq->avail_idx;
> > +		state->avail_index = mvq->used_idx;
> >   		return 0;
> >   	}
> > @@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
> >   		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
> >   		return err;
> >   	}
> > -	state->avail_index = attr.available_index;
> > +	state->avail_index = attr.used_index;
> >   	return 0;
> >   }
> > @@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
> >   	}
> >   }
> > -static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
> > -{
> > -	int i;
> > -
> > -	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
> > -		ndev->vqs[i].avail_idx = 0;
> > -		ndev->vqs[i].used_idx = 0;
> > -	}
> > -}
> > -
> >   /* TODO: cross-endian support */
> >   static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
> >   {
> > @@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> >   	if (!status) {
> >   		mlx5_vdpa_info(mvdev, "performing device reset\n");
> >   		teardown_driver(ndev);
> > -		clear_virtqueues(ndev);
> >   		mlx5_vdpa_destroy_mr(&ndev->mvdev);
> >   		ndev->mvdev.status = 0;
> >   		++mvdev->generation;

Re: [PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[Michael S. Tsirkin]

On Wed, Feb 17, 2021 at 11:42:48AM -0800, Si-Wei Liu wrote:
> 
> 
> On 2/16/2021 8:20 AM, Eli Cohen wrote:
> > When we suspend the VM, the VDPA interface will be reset. When the VM is
> > resumed again, clear_virtqueues() will clear the available and used
> > indices resulting in hardware virqtqueue objects becoming out of sync.
> > We can avoid this function alltogether since qemu will clear them if
> > required, e.g. when the VM went through a reboot.
> > 
> > Moreover, since the hw available and used indices should always be
> > identical on query and should be restored to the same value same value
> > for virtqueues that complete in order, we set the single value provided
> > by set_vq_state(). In get_vq_state() we return the value of hardware
> > used index.
> > 
> > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>


Seems to also fix b35ccebe3ef76168aa2edaa35809c0232cb3578e, right?


> > ---
> >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
> >   1 file changed, 4 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index b8e9d525d66c..a51b0f86afe2 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
> >   		return;
> >   	}
> >   	mvq->avail_idx = attr.available_index;
> > +	mvq->used_idx = attr.used_index;
> >   }
> >   static void suspend_vqs(struct mlx5_vdpa_net *ndev)
> > @@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
> >   		return -EINVAL;
> >   	}
> > +	mvq->used_idx = state->avail_index;
> >   	mvq->avail_idx = state->avail_index;
> >   	return 0;
> >   }
> > @@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
> >   	 * that cares about emulating the index after vq is stopped.
> >   	 */
> >   	if (!mvq->initialized) {
> > -		state->avail_index = mvq->avail_idx;
> > +		state->avail_index = mvq->used_idx;
> >   		return 0;
> >   	}
> > @@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
> >   		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
> >   		return err;
> >   	}
> > -	state->avail_index = attr.available_index;
> > +	state->avail_index = attr.used_index;
> >   	return 0;
> >   }
> > @@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
> >   	}
> >   }
> > -static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
> > -{
> > -	int i;
> > -
> > -	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
> > -		ndev->vqs[i].avail_idx = 0;
> > -		ndev->vqs[i].used_idx = 0;
> > -	}
> > -}
> > -
> >   /* TODO: cross-endian support */
> >   static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
> >   {
> > @@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> >   	if (!status) {
> >   		mlx5_vdpa_info(mvdev, "performing device reset\n");
> >   		teardown_driver(ndev);
> > -		clear_virtqueues(ndev);
> >   		mlx5_vdpa_destroy_mr(&ndev->mvdev);
> >   		ndev->mvdev.status = 0;
> >   		++mvdev->generation;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[Si-Wei Liu]

On 2/17/2021 1:20 PM, Michael S. Tsirkin wrote:
> On Wed, Feb 17, 2021 at 11:42:48AM -0800, Si-Wei Liu wrote:
>>
>> On 2/16/2021 8:20 AM, Eli Cohen wrote:
>>> When we suspend the VM, the VDPA interface will be reset. When the VM is
>>> resumed again, clear_virtqueues() will clear the available and used
>>> indices resulting in hardware virqtqueue objects becoming out of sync.
>>> We can avoid this function alltogether since qemu will clear them if
>>> required, e.g. when the VM went through a reboot.
>>>
>>> Moreover, since the hw available and used indices should always be
>>> identical on query and should be restored to the same value same value
>>> for virtqueues that complete in order, we set the single value provided
>>> by set_vq_state(). In get_vq_state() we return the value of hardware
>>> used index.
>>>
>>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>
> Seems to also fix b35ccebe3ef76168aa2edaa35809c0232cb3578e, right?
I think so. It should have both "Fixes" tags.

-Siwei
>
>>> ---
>>>    drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
>>>    1 file changed, 4 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index b8e9d525d66c..a51b0f86afe2 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
>>>    		return;
>>>    	}
>>>    	mvq->avail_idx = attr.available_index;
>>> +	mvq->used_idx = attr.used_index;
>>>    }
>>>    static void suspend_vqs(struct mlx5_vdpa_net *ndev)
>>> @@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
>>>    		return -EINVAL;
>>>    	}
>>> +	mvq->used_idx = state->avail_index;
>>>    	mvq->avail_idx = state->avail_index;
>>>    	return 0;
>>>    }
>>> @@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
>>>    	 * that cares about emulating the index after vq is stopped.
>>>    	 */
>>>    	if (!mvq->initialized) {
>>> -		state->avail_index = mvq->avail_idx;
>>> +		state->avail_index = mvq->used_idx;
>>>    		return 0;
>>>    	}
>>> @@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
>>>    		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
>>>    		return err;
>>>    	}
>>> -	state->avail_index = attr.available_index;
>>> +	state->avail_index = attr.used_index;
>>>    	return 0;
>>>    }
>>> @@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
>>>    	}
>>>    }
>>> -static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
>>> -{
>>> -	int i;
>>> -
>>> -	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
>>> -		ndev->vqs[i].avail_idx = 0;
>>> -		ndev->vqs[i].used_idx = 0;
>>> -	}
>>> -}
>>> -
>>>    /* TODO: cross-endian support */
>>>    static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
>>>    {
>>> @@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>>>    	if (!status) {
>>>    		mlx5_vdpa_info(mvdev, "performing device reset\n");
>>>    		teardown_driver(ndev);
>>> -		clear_virtqueues(ndev);
>>>    		mlx5_vdpa_destroy_mr(&ndev->mvdev);
>>>    		ndev->mvdev.status = 0;
>>>    		++mvdev->generation;

Re: [PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[Si-Wei Liu]

On 2/17/2021 1:20 PM, Michael S. Tsirkin wrote:
> On Wed, Feb 17, 2021 at 11:42:48AM -0800, Si-Wei Liu wrote:
>>
>> On 2/16/2021 8:20 AM, Eli Cohen wrote:
>>> When we suspend the VM, the VDPA interface will be reset. When the VM is
>>> resumed again, clear_virtqueues() will clear the available and used
>>> indices resulting in hardware virqtqueue objects becoming out of sync.
>>> We can avoid this function alltogether since qemu will clear them if
>>> required, e.g. when the VM went through a reboot.
>>>
>>> Moreover, since the hw available and used indices should always be
>>> identical on query and should be restored to the same value same value
>>> for virtqueues that complete in order, we set the single value provided
>>> by set_vq_state(). In get_vq_state() we return the value of hardware
>>> used index.
>>>
>>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>
> Seems to also fix b35ccebe3ef76168aa2edaa35809c0232cb3578e, right?
I think so. It should have both "Fixes" tags.

-Siwei
>
>>> ---
>>>    drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
>>>    1 file changed, 4 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index b8e9d525d66c..a51b0f86afe2 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
>>>    		return;
>>>    	}
>>>    	mvq->avail_idx = attr.available_index;
>>> +	mvq->used_idx = attr.used_index;
>>>    }
>>>    static void suspend_vqs(struct mlx5_vdpa_net *ndev)
>>> @@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
>>>    		return -EINVAL;
>>>    	}
>>> +	mvq->used_idx = state->avail_index;
>>>    	mvq->avail_idx = state->avail_index;
>>>    	return 0;
>>>    }
>>> @@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
>>>    	 * that cares about emulating the index after vq is stopped.
>>>    	 */
>>>    	if (!mvq->initialized) {
>>> -		state->avail_index = mvq->avail_idx;
>>> +		state->avail_index = mvq->used_idx;
>>>    		return 0;
>>>    	}
>>> @@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
>>>    		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
>>>    		return err;
>>>    	}
>>> -	state->avail_index = attr.available_index;
>>> +	state->avail_index = attr.used_index;
>>>    	return 0;
>>>    }
>>> @@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
>>>    	}
>>>    }
>>> -static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
>>> -{
>>> -	int i;
>>> -
>>> -	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
>>> -		ndev->vqs[i].avail_idx = 0;
>>> -		ndev->vqs[i].used_idx = 0;
>>> -	}
>>> -}
>>> -
>>>    /* TODO: cross-endian support */
>>>    static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
>>>    {
>>> @@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
>>>    	if (!status) {
>>>    		mlx5_vdpa_info(mvdev, "performing device reset\n");
>>>    		teardown_driver(ndev);
>>> -		clear_virtqueues(ndev);
>>>    		mlx5_vdpa_destroy_mr(&ndev->mvdev);
>>>    		ndev->mvdev.status = 0;
>>>    		++mvdev->generation;

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[Eli Cohen]

On Wed, Feb 17, 2021 at 04:20:14PM -0500, Michael S. Tsirkin wrote:
> On Wed, Feb 17, 2021 at 11:42:48AM -0800, Si-Wei Liu wrote:
> > 
> > 
> > On 2/16/2021 8:20 AM, Eli Cohen wrote:
> > > When we suspend the VM, the VDPA interface will be reset. When the VM is
> > > resumed again, clear_virtqueues() will clear the available and used
> > > indices resulting in hardware virqtqueue objects becoming out of sync.
> > > We can avoid this function alltogether since qemu will clear them if
> > > required, e.g. when the VM went through a reboot.
> > > 
> > > Moreover, since the hw available and used indices should always be
> > > identical on query and should be restored to the same value same value
> > > for virtqueues that complete in order, we set the single value provided
> > > by set_vq_state(). In get_vq_state() we return the value of hardware
> > > used index.
> > > 
> > > Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices")
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
> 
> 
> Seems to also fix b35ccebe3ef76168aa2edaa35809c0232cb3578e, right?
> 

Right.

> 
> > > ---
> > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
> > >   1 file changed, 4 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index b8e9d525d66c..a51b0f86afe2 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
> > >   		return;
> > >   	}
> > >   	mvq->avail_idx = attr.available_index;
> > > +	mvq->used_idx = attr.used_index;
> > >   }
> > >   static void suspend_vqs(struct mlx5_vdpa_net *ndev)
> > > @@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct vdpa_device *vdev, u16 idx,
> > >   		return -EINVAL;
> > >   	}
> > > +	mvq->used_idx = state->avail_index;
> > >   	mvq->avail_idx = state->avail_index;
> > >   	return 0;
> > >   }
> > > @@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
> > >   	 * that cares about emulating the index after vq is stopped.
> > >   	 */
> > >   	if (!mvq->initialized) {
> > > -		state->avail_index = mvq->avail_idx;
> > > +		state->avail_index = mvq->used_idx;
> > >   		return 0;
> > >   	}
> > > @@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct vdpa_device *vdev, u16 idx, struct vdpa
> > >   		mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
> > >   		return err;
> > >   	}
> > > -	state->avail_index = attr.available_index;
> > > +	state->avail_index = attr.used_index;
> > >   	return 0;
> > >   }
> > > @@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct mlx5_vdpa_net *ndev)
> > >   	}
> > >   }
> > > -static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
> > > -{
> > > -	int i;
> > > -
> > > -	for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
> > > -		ndev->vqs[i].avail_idx = 0;
> > > -		ndev->vqs[i].used_idx = 0;
> > > -	}
> > > -}
> > > -
> > >   /* TODO: cross-endian support */
> > >   static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
> > >   {
> > > @@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
> > >   	if (!status) {
> > >   		mlx5_vdpa_info(mvdev, "performing device reset\n");
> > >   		teardown_driver(ndev);
> > > -		clear_virtqueues(ndev);
> > >   		mlx5_vdpa_destroy_mr(&ndev->mvdev);
> > >   		ndev->mvdev.status = 0;
> > >   		++mvdev->generation;
> 

Re: [PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[Si-Wei Liu]

On 2/17/2021 11:42 AM, Si-Wei Liu wrote:
>
>
> On 2/16/2021 8:20 AM, Eli Cohen wrote:
>> When we suspend the VM, the VDPA interface will be reset. When the VM is
>> resumed again, clear_virtqueues() will clear the available and used
>> indices resulting in hardware virqtqueue objects becoming out of sync.
>> We can avoid this function alltogether since qemu will clear them if
>> required, e.g. when the VM went through a reboot.
>>
>> Moreover, since the hw available and used indices should always be
>> identical on query and should be restored to the same value same value
>> for virtqueues that complete in order, we set the single value provided
>> by set_vq_state(). In get_vq_state() we return the value of hardware
>> used index.
>>
>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 
>> devices")
>> Signed-off-by: Eli Cohen <elic@nvidia.com>
> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>

I'd take it back for the moment, according to Jason's latest comment. 
Discussion still going.

>
>> ---
>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
>>   1 file changed, 4 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c 
>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index b8e9d525d66c..a51b0f86afe2 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net 
>> *ndev, struct mlx5_vdpa_virtqueue *m
>>           return;
>>       }
>>       mvq->avail_idx = attr.available_index;
>> +    mvq->used_idx = attr.used_index;
>>   }
>>     static void suspend_vqs(struct mlx5_vdpa_net *ndev)
>> @@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct 
>> vdpa_device *vdev, u16 idx,
>>           return -EINVAL;
>>       }
>>   +    mvq->used_idx = state->avail_index;
>>       mvq->avail_idx = state->avail_index;
This is where things starts getting interesting. According to Jason, the 
original expectation of this API would be to restore the internal 
last_avail_index in the hardware. With Mellanox network vDPA hardware 
implementation, there's no such last_avail_index thing in the hardware 
but only a single last_used_index representing both indices, which 
should always be the same and in sync with each other. So from migration 
point of view, it appears logical to restore the saved last_avail_index 
to the last_used_index in the hardware, right? But what is the point to 
restore mvq->avail_idx?

Actually, this code path is being repurposed to address the index reset 
problem in the device reset scenario. Where the mvq->avail_idx and 
mvq->used_idx are both reset to 0 once device is reset. This is a bit 
crossing the boundary to me.


>>       return 0;
>>   }
>> @@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct 
>> vdpa_device *vdev, u16 idx, struct vdpa
>>        * that cares about emulating the index after vq is stopped.
>>        */
>>       if (!mvq->initialized) {
>> -        state->avail_index = mvq->avail_idx;
>> +        state->avail_index = mvq->used_idx;
This is where the last_used_index gets loaded from the hardware (when 
device is stopped).

>>           return 0;
>>       }
>>   @@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct 
>> vdpa_device *vdev, u16 idx, struct vdpa
>>           mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
>>           return err;
>>       }
>> -    state->avail_index = attr.available_index;
>> +    state->avail_index = attr.used_index;
This code path never gets called from userspace (when device is running).

-Siwei

>>       return 0;
>>   }
>>   @@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct 
>> mlx5_vdpa_net *ndev)
>>       }
>>   }
>>   -static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
>> -{
>> -    int i;
>> -
>> -    for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
>> -        ndev->vqs[i].avail_idx = 0;
>> -        ndev->vqs[i].used_idx = 0;
>> -    }
>> -}
>> -
>>   /* TODO: cross-endian support */
>>   static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev 
>> *mvdev)
>>   {
>> @@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct 
>> vdpa_device *vdev, u8 status)
>>       if (!status) {
>>           mlx5_vdpa_info(mvdev, "performing device reset\n");
>>           teardown_driver(ndev);
>> -        clear_virtqueues(ndev);
>>           mlx5_vdpa_destroy_mr(&ndev->mvdev);
>>           ndev->mvdev.status = 0;
>>           ++mvdev->generation;
>

Re: [PATCH 1/2] vdpa/mlx5: Fix suspend/resume index restoration

[Si-Wei Liu]

On 2/17/2021 11:42 AM, Si-Wei Liu wrote:
>
>
> On 2/16/2021 8:20 AM, Eli Cohen wrote:
>> When we suspend the VM, the VDPA interface will be reset. When the VM is
>> resumed again, clear_virtqueues() will clear the available and used
>> indices resulting in hardware virqtqueue objects becoming out of sync.
>> We can avoid this function alltogether since qemu will clear them if
>> required, e.g. when the VM went through a reboot.
>>
>> Moreover, since the hw available and used indices should always be
>> identical on query and should be restored to the same value same value
>> for virtqueues that complete in order, we set the single value provided
>> by set_vq_state(). In get_vq_state() we return the value of hardware
>> used index.
>>
>> Fixes: 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 
>> devices")
>> Signed-off-by: Eli Cohen <elic@nvidia.com>
> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>

I'd take it back for the moment, according to Jason's latest comment. 
Discussion still going.

>
>> ---
>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++-------------
>>   1 file changed, 4 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c 
>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index b8e9d525d66c..a51b0f86afe2 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -1169,6 +1169,7 @@ static void suspend_vq(struct mlx5_vdpa_net 
>> *ndev, struct mlx5_vdpa_virtqueue *m
>>           return;
>>       }
>>       mvq->avail_idx = attr.available_index;
>> +    mvq->used_idx = attr.used_index;
>>   }
>>     static void suspend_vqs(struct mlx5_vdpa_net *ndev)
>> @@ -1426,6 +1427,7 @@ static int mlx5_vdpa_set_vq_state(struct 
>> vdpa_device *vdev, u16 idx,
>>           return -EINVAL;
>>       }
>>   +    mvq->used_idx = state->avail_index;
>>       mvq->avail_idx = state->avail_index;
This is where things starts getting interesting. According to Jason, the 
original expectation of this API would be to restore the internal 
last_avail_index in the hardware. With Mellanox network vDPA hardware 
implementation, there's no such last_avail_index thing in the hardware 
but only a single last_used_index representing both indices, which 
should always be the same and in sync with each other. So from migration 
point of view, it appears logical to restore the saved last_avail_index 
to the last_used_index in the hardware, right? But what is the point to 
restore mvq->avail_idx?

Actually, this code path is being repurposed to address the index reset 
problem in the device reset scenario. Where the mvq->avail_idx and 
mvq->used_idx are both reset to 0 once device is reset. This is a bit 
crossing the boundary to me.


>>       return 0;
>>   }
>> @@ -1443,7 +1445,7 @@ static int mlx5_vdpa_get_vq_state(struct 
>> vdpa_device *vdev, u16 idx, struct vdpa
>>        * that cares about emulating the index after vq is stopped.
>>        */
>>       if (!mvq->initialized) {
>> -        state->avail_index = mvq->avail_idx;
>> +        state->avail_index = mvq->used_idx;
This is where the last_used_index gets loaded from the hardware (when 
device is stopped).

>>           return 0;
>>       }
>>   @@ -1452,7 +1454,7 @@ static int mlx5_vdpa_get_vq_state(struct 
>> vdpa_device *vdev, u16 idx, struct vdpa
>>           mlx5_vdpa_warn(mvdev, "failed to query virtqueue\n");
>>           return err;
>>       }
>> -    state->avail_index = attr.available_index;
>> +    state->avail_index = attr.used_index;
This code path never gets called from userspace (when device is running).

-Siwei

>>       return 0;
>>   }
>>   @@ -1532,16 +1534,6 @@ static void teardown_virtqueues(struct 
>> mlx5_vdpa_net *ndev)
>>       }
>>   }
>>   -static void clear_virtqueues(struct mlx5_vdpa_net *ndev)
>> -{
>> -    int i;
>> -
>> -    for (i = ndev->mvdev.max_vqs - 1; i >= 0; i--) {
>> -        ndev->vqs[i].avail_idx = 0;
>> -        ndev->vqs[i].used_idx = 0;
>> -    }
>> -}
>> -
>>   /* TODO: cross-endian support */
>>   static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev 
>> *mvdev)
>>   {
>> @@ -1777,7 +1769,6 @@ static void mlx5_vdpa_set_status(struct 
>> vdpa_device *vdev, u8 status)
>>       if (!status) {
>>           mlx5_vdpa_info(mvdev, "performing device reset\n");
>>           teardown_driver(ndev);
>> -        clear_virtqueues(ndev);
>>           mlx5_vdpa_destroy_mr(&ndev->mvdev);
>>           ndev->mvdev.status = 0;
>>           ++mvdev->generation;
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization