Otherwise the virtqueue object to instate could point to invalid address
that was unmapped from the MTT:
mlx5_core 0000:41:04.2: mlx5_cmd_out_err:782:(pid 8321):
CREATE_GENERAL_OBJECT(0xa00) op_mod(0xd) failed, status
bad parameter(0x3), syndrome (0x5fa1c), err(-22)
While at it, add warning message to tell apart which object is
responsible for the CREATE_GENERAL_OBJECT command failure.
Fixes: cae15c2ed8e6 ("vdpa/mlx5: Implement susupend virtqueue callback")
Cc: Eli Cohen <[email protected]>
Signed-off-by: Si-Wei Liu <[email protected]>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 3a6dbbc6..c05c7f6 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -165,6 +165,7 @@ struct mlx5_vdpa_net {
u32 cur_num_vqs;
u32 rqt_size;
bool nb_registered;
+ bool suspended;
struct notifier_block nb;
struct vdpa_callback config_cb;
struct mlx5_vdpa_wq_ent cvq_ent;
@@ -1245,12 +1246,18 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
goto err_connect;
err = counter_set_alloc(ndev, mvq);
- if (err)
+ if (err) {
+ mlx5_vdpa_warn(&ndev->mvdev, "failed to alloc counter on vq idx %d(%d)\n",
+ idx, err);
goto err_counter;
+ }
err = create_virtqueue(ndev, mvq);
- if (err)
+ if (err) {
+ mlx5_vdpa_warn(&ndev->mvdev, "failed to create virtqueue idx %d(%d)\n",
+ idx, err);
goto err_connect;
+ }
if (mvq->ready) {
err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
@@ -2411,7 +2418,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev,
if (err)
goto err_mr;
- if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
+ if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK) || ndev->suspended)
goto err_mr;
restore_channels_info(ndev);
@@ -2580,6 +2587,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
mlx5_vdpa_destroy_mr(&ndev->mvdev);
ndev->mvdev.status = 0;
ndev->cur_num_vqs = 0;
+ ndev->suspended = false;
ndev->mvdev.cvq.received_desc = 0;
ndev->mvdev.cvq.completed_desc = 0;
memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
@@ -2815,6 +2823,8 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
struct mlx5_vdpa_virtqueue *mvq;
int i;
+ mlx5_vdpa_info(mvdev, "suspending device\n");
+
down_write(&ndev->reslock);
ndev->nb_registered = false;
mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
@@ -2824,6 +2834,7 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
suspend_vq(ndev, mvq);
}
mlx5_vdpa_cvq_suspend(mvdev);
+ ndev->suspended = true;
up_write(&ndev->reslock);
return 0;
}
--
1.8.3.1
On 11/02/2023 8:08, Si-Wei Liu wrote:
> Otherwise the virtqueue object to instate could point to invalid address
> that was unmapped from the MTT:
>
> mlx5_core 0000:41:04.2: mlx5_cmd_out_err:782:(pid 8321):
> CREATE_GENERAL_OBJECT(0xa00) op_mod(0xd) failed, status
> bad parameter(0x3), syndrome (0x5fa1c), err(-22)
I am familiar with this issue but I think it should be handled at the
qemu level. This logic just hides the real problem. qemu provides the
iova and the VQs' addresses so it should not shrink the map while such
that VQ addresses are out of the iova range.
>
> While at it, add warning message to tell apart which object is
> responsible for the CREATE_GENERAL_OBJECT command failure.
>
> Fixes: cae15c2ed8e6 ("vdpa/mlx5: Implement susupend virtqueue callback")
> Cc: Eli Cohen <[email protected]>
> Signed-off-by: Si-Wei Liu <[email protected]>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 3a6dbbc6..c05c7f6 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -165,6 +165,7 @@ struct mlx5_vdpa_net {
> u32 cur_num_vqs;
> u32 rqt_size;
> bool nb_registered;
> + bool suspended;
> struct notifier_block nb;
> struct vdpa_callback config_cb;
> struct mlx5_vdpa_wq_ent cvq_ent;
> @@ -1245,12 +1246,18 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> goto err_connect;
>
> err = counter_set_alloc(ndev, mvq);
> - if (err)
> + if (err) {
> + mlx5_vdpa_warn(&ndev->mvdev, "failed to alloc counter on vq idx %d(%d)\n",
> + idx, err);
Although you mention in the commit log, maybe it's better to have a
separate patch for improving warning messages.
> goto err_counter;
> + }
>
> err = create_virtqueue(ndev, mvq);
> - if (err)
> + if (err) {
> + mlx5_vdpa_warn(&ndev->mvdev, "failed to create virtqueue idx %d(%d)\n",
> + idx, err);
> goto err_connect;
> + }
>
> if (mvq->ready) {
> err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> @@ -2411,7 +2418,7 @@ static int mlx5_vdpa_change_map(struct mlx5_vdpa_dev *mvdev,
> if (err)
> goto err_mr;
>
> - if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
> + if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK) || ndev->suspended)
> goto err_mr;
>
> restore_channels_info(ndev);
> @@ -2580,6 +2587,7 @@ static int mlx5_vdpa_reset(struct vdpa_device *vdev)
> mlx5_vdpa_destroy_mr(&ndev->mvdev);
> ndev->mvdev.status = 0;
> ndev->cur_num_vqs = 0;
> + ndev->suspended = false;
> ndev->mvdev.cvq.received_desc = 0;
> ndev->mvdev.cvq.completed_desc = 0;
> memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) * (mvdev->max_vqs + 1));
> @@ -2815,6 +2823,8 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
> struct mlx5_vdpa_virtqueue *mvq;
> int i;
>
> + mlx5_vdpa_info(mvdev, "suspending device\n");
> +
> down_write(&ndev->reslock);
> ndev->nb_registered = false;
> mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
> @@ -2824,6 +2834,7 @@ static int mlx5_vdpa_suspend(struct vdpa_device *vdev)
> suspend_vq(ndev, mvq);
> }
> mlx5_vdpa_cvq_suspend(mvdev);
> + ndev->suspended = true;
> up_write(&ndev->reslock);
> return 0;
> }
+ Eugenio
On 2/11/2023 10:55 PM, Eli Cohen wrote:
>
> On 11/02/2023 8:08, Si-Wei Liu wrote:
>> Otherwise the virtqueue object to instate could point to invalid address
>> that was unmapped from the MTT:
>>
>> mlx5_core 0000:41:04.2: mlx5_cmd_out_err:782:(pid 8321):
>> CREATE_GENERAL_OBJECT(0xa00) op_mod(0xd) failed, status
>> bad parameter(0x3), syndrome (0x5fa1c), err(-22)
> I am familiar with this issue but I think it should be handled at the
> qemu level. This logic just hides the real problem. qemu provides the
> iova and the VQs' addresses so it should not shrink the map while such
> that VQ addresses are out of the iova range.
Here the whole device is already suspended, which should guarantee no
further DMA memory access will be initiated by the device. What is the
problem the mapping can't shrink in this case? Noted the code already
allows shrinking for the reset case (~VIRTIO_CONFIG_S_DRIVER_OK status
check), I see no essential difference the same couldn't be applied to
the suspend case. On the other hand, map shrinking works well with
platform IOMMU iommu_map/unmap() calls while vdpa device is suspended.
If your concern is that the device won't work with the shrunk map once
the device is to be RESUME'd, I think I agree that's what QEMU has to
guarantee, by either adding back the required mapping entries as needed,
or change VQ addresses during suspend that points to mapped IOVA
addresses. Or both can be done at the same time during suspend, with
that we can build a fast path to switch svq mode with on-chip IOMMU e.g.
mlx5_vdpa, rather than go through full device reset cycle.
>>
>> While at it, add warning message to tell apart which object is
>> responsible for the CREATE_GENERAL_OBJECT command failure.
>>
>> Fixes: cae15c2ed8e6 ("vdpa/mlx5: Implement susupend virtqueue callback")
>> Cc: Eli Cohen <[email protected]>
>> Signed-off-by: Si-Wei Liu <[email protected]>
>> ---
>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 ++++++++++++++---
>> 1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index 3a6dbbc6..c05c7f6 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -165,6 +165,7 @@ struct mlx5_vdpa_net {
>> u32 cur_num_vqs;
>> u32 rqt_size;
>> bool nb_registered;
>> + bool suspended;
>> struct notifier_block nb;
>> struct vdpa_callback config_cb;
>> struct mlx5_vdpa_wq_ent cvq_ent;
>> @@ -1245,12 +1246,18 @@ static int setup_vq(struct mlx5_vdpa_net
>> *ndev, struct mlx5_vdpa_virtqueue *mvq)
>> goto err_connect;
>> err = counter_set_alloc(ndev, mvq);
>> - if (err)
>> + if (err) {
>> + mlx5_vdpa_warn(&ndev->mvdev, "failed to alloc counter on vq
>> idx %d(%d)\n",
>> + idx, err);
> Although you mention in the commit log, maybe it's better to have a
> separate patch for improving warning messages.
Alright, I will move the improved warning message out of this patch.
This issue effectively blocks live migrating mlx5_vdpa device that has
to get merged asap. While others can post patches to improve warnings on
top.
Thanks,
-Siwei
>> goto err_counter;
>> + }
>> err = create_virtqueue(ndev, mvq);
>> - if (err)
>> + if (err) {
>> + mlx5_vdpa_warn(&ndev->mvdev, "failed to create virtqueue idx
>> %d(%d)\n",
>> + idx, err);
>> goto err_connect;
>> + }
>> if (mvq->ready) {
>> err = modify_virtqueue(ndev, mvq,
>> MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
>> @@ -2411,7 +2418,7 @@ static int mlx5_vdpa_change_map(struct
>> mlx5_vdpa_dev *mvdev,
>> if (err)
>> goto err_mr;
>> - if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK))
>> + if (!(mvdev->status & VIRTIO_CONFIG_S_DRIVER_OK) ||
>> ndev->suspended)
>> goto err_mr;
>> restore_channels_info(ndev);
>> @@ -2580,6 +2587,7 @@ static int mlx5_vdpa_reset(struct vdpa_device
>> *vdev)
>> mlx5_vdpa_destroy_mr(&ndev->mvdev);
>> ndev->mvdev.status = 0;
>> ndev->cur_num_vqs = 0;
>> + ndev->suspended = false;
>> ndev->mvdev.cvq.received_desc = 0;
>> ndev->mvdev.cvq.completed_desc = 0;
>> memset(ndev->event_cbs, 0, sizeof(*ndev->event_cbs) *
>> (mvdev->max_vqs + 1));
>> @@ -2815,6 +2823,8 @@ static int mlx5_vdpa_suspend(struct vdpa_device
>> *vdev)
>> struct mlx5_vdpa_virtqueue *mvq;
>> int i;
>> + mlx5_vdpa_info(mvdev, "suspending device\n");
>> +
>> down_write(&ndev->reslock);
>> ndev->nb_registered = false;
>> mlx5_notifier_unregister(mvdev->mdev, &ndev->nb);
>> @@ -2824,6 +2834,7 @@ static int mlx5_vdpa_suspend(struct vdpa_device
>> *vdev)
>> suspend_vq(ndev, mvq);
>> }
>> mlx5_vdpa_cvq_suspend(mvdev);
>> + ndev->suspended = true;
>> up_write(&ndev->reslock);
>> return 0;
>> }