Re: [PATCH] vdpa/mlx5: should not activate virtq object when suspended

[Si-Wei Liu]

+ 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 <elic@xxxxxxxxxx>
> > Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> > ---
> >   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;
> >   }