On Wed, Jul 13, 2022 at 1:18 PM Eli Cohen <[email protected]> wrote: > > > From: Jason Wang <[email protected]> > > Sent: Wednesday, July 13, 2022 6:29 AM > > To: Eugenio Perez Martin <[email protected]> > > Cc: Eli Cohen <[email protected]>; mst <[email protected]>; virtualization > > <[email protected]>; linux-kernel > > <[email protected]>; Si-Wei Liu <[email protected]>; Parav > > Pandit <[email protected]> > > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend virtqueue > > callback > > > > On Tue, Jul 12, 2022 at 5:16 PM Eugenio Perez Martin > > <[email protected]> wrote: > > > > > > On Tue, Jul 12, 2022 at 10:14 AM Jason Wang <[email protected]> wrote: > > > > > > > > On Mon, Jul 11, 2022 at 2:14 PM Eli Cohen <[email protected]> wrote: > > > > > > > > > > > From: Jason Wang <[email protected]> > > > > > > Sent: Tuesday, June 21, 2022 6:05 AM > > > > > > To: Eugenio Perez Martin <[email protected]> > > > > > > Cc: Eli Cohen <[email protected]>; mst <[email protected]>; > > > > > > virtualization <[email protected]>; linux- > > kernel > > > > > > <[email protected]>; Si-Wei Liu <[email protected]>; > > > > > > Parav Pandit <[email protected]> > > > > > > Subject: Re: [PATCH RFC 1/3] vdpa/mlx5: Implement susupend > > > > > > virtqueue callback > > > > > > > > > > > > On Mon, Jun 20, 2022 at 5:59 PM Eugenio Perez Martin > > > > > > <[email protected]> wrote: > > > > > > > > > > > > > > On Mon, Jun 20, 2022 at 10:56 AM Jason Wang <[email protected]> > > > > > > > wrote: > > > > > > > > > > > > > > > > On Thu, Jun 16, 2022 at 9:27 PM Eli Cohen <[email protected]> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Implement the suspend callback allowing to suspend the > > > > > > > > > virtqueues so > > > > > > > > > they stop processing descriptors. This is required to allow > > > > > > > > > the shadow > > > > > > > > > virtqueue to kick in. > > > > > > > > > > > > > > > > > > Signed-off-by: Eli Cohen <[email protected]> > > > > > > > > > --- > > > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 68 > > > > > > > > > +++++++++++++++++++++++++++++- > > > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 8 ++++ > > > > > > > > > 2 files changed, 75 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > index fb0b23e71383..ea4bc8a0cd25 100644 > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > > > > > > > > @@ -895,6 +895,7 @@ static int create_virtqueue(struct > > > > > > > > > mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque > > > > > > > > > if (err) > > > > > > > > > goto err_cmd; > > > > > > > > > > > > > > > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT; > > > > > > > > > kfree(in); > > > > > > > > > mvq->virtq_id = MLX5_GET(general_obj_out_cmd_hdr, > > > > > > > > > out, obj_id); > > > > > > > > > > > > > > > > > > @@ -922,6 +923,7 @@ static void destroy_virtqueue(struct > > > > > > > > > mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtq > > > > > > > > > mlx5_vdpa_warn(&ndev->mvdev, "destroy > > > > > > > > > virtqueue 0x%x\n", mvq->virtq_id); > > > > > > > > > return; > > > > > > > > > } > > > > > > > > > + mvq->fw_state = MLX5_VIRTIO_NET_Q_OBJECT_NONE; > > > > > > > > > umems_destroy(ndev, mvq); > > > > > > > > > } > > > > > > > > > > > > > > > > > > @@ -1121,6 +1123,20 @@ static int query_virtqueue(struct > > > > > > > > > mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueu > > > > > > > > > return err; > > > > > > > > > } > > > > > > > > > > > > > > > > > > +static bool is_valid_state_change(int oldstate, int newstate) > > > > > > > > > +{ > > > > > > > > > + switch (oldstate) { > > > > > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT: > > > > > > > > > + return newstate == > > > > > > > > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY; > > > > > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY: > > > > > > > > > + return newstate == > > > > > > > > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND; > > > > > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND: > > > > > > > > > + case MLX5_VIRTIO_NET_Q_OBJECT_STATE_ERR: > > > > > > > > > + default: > > > > > > > > > + return false; > > > > > > > > > + } > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > static int modify_virtqueue(struct mlx5_vdpa_net *ndev, > > > > > > > > > struct mlx5_vdpa_virtqueue *mvq, int state) > > > > > > > > > { > > > > > > > > > int inlen = MLX5_ST_SZ_BYTES(modify_virtio_net_q_in); > > > > > > > > > @@ -1130,6 +1146,12 @@ static int modify_virtqueue(struct > > > > > > > > > mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque > > > > > > > > > void *in; > > > > > > > > > int err; > > > > > > > > > > > > > > > > > > + if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE) > > > > > > > > > + return 0; > > > > > > > > > + > > > > > > > > > + if (!is_valid_state_change(mvq->fw_state, state)) > > > > > > > > > + return -EINVAL; > > > > > > > > > + > > > > > > > > > in = kzalloc(inlen, GFP_KERNEL); > > > > > > > > > if (!in) > > > > > > > > > return -ENOMEM; > > > > > > > > > @@ -1991,6 +2013,7 @@ static void > > > > > > > > > mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, > > > > > > > > > bool ready > > > > > > > > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev); > > > > > > > > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); > > > > > > > > > struct mlx5_vdpa_virtqueue *mvq; > > > > > > > > > + int err; > > > > > > > > > > > > > > > > > > if (!mvdev->actual_features) > > > > > > > > > return; > > > > > > > > > @@ -2004,8 +2027,16 @@ static void > > > > > > > > > mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, > > > > > > > > > bool ready > > > > > > > > > } > > > > > > > > > > > > > > > > > > mvq = &ndev->vqs[idx]; > > > > > > > > > - if (!ready) > > > > > > > > > + if (!ready) { > > > > > > > > > suspend_vq(ndev, mvq); > > > > > > > > > + } else { > > > > > > > > > + err = modify_virtqueue(ndev, mvq, > > > > > > > > > MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY); > > > > > > > > > + if (err) { > > > > > > > > > + mlx5_vdpa_warn(mvdev, "modify VQ %d > > > > > > > > > to ready failed (%d)\n", idx, err); > > > > > > > > > + ready = false; > > > > > > > > > + } > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > > > > > > > > > > mvq->ready = ready; > > > > > > > > > } > > > > > > > > > @@ -2732,6 +2763,39 @@ static int > > > > > > > > > mlx5_vdpa_get_vendor_vq_stats(struct vdpa_device *vdev, u16 > > > > > > > > > idx, > > > > > > > > > return err; > > > > > > > > > } > > > > > > > > > > > > > > > > > > +static void mlx5_vdpa_cvq_suspend(struct mlx5_vdpa_dev > > > > > > > > > *mvdev, bool suspend) > > > > > > > > > +{ > > > > > > > > > + struct mlx5_control_vq *cvq; > > > > > > > > > + > > > > > > > > > + if (!(mvdev->actual_features & > > > > > > > > > BIT_ULL(VIRTIO_NET_F_CTRL_VQ))) > > > > > > > > > + return; > > > > > > > > > + > > > > > > > > > + cvq = &mvdev->cvq; > > > > > > > > > + cvq->ready = !suspend; > > > > > > > > > +} > > > > > > > > > > > > > > > > It looks to me we need to synchronize this with reslock. And > > > > > > > > this > > > > > > > > probably deserve a dedicated fix. > > > > > > > > > > > > > > > > > + > > > > > > > > > +static int mlx5_vdpa_suspend(struct vdpa_device *vdev, bool > > > > > > > > > suspend) > > > > > > > > > +{ > > > > > > > > > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev); > > > > > > > > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev); > > > > > > > > > + struct mlx5_vdpa_virtqueue *mvq; > > > > > > > > > + int i; > > > > > > > > > + > > > > > > > > > + if (!suspend) { > > > > > > > > > + mlx5_vdpa_warn(mvdev, "Resume of virtqueues > > > > > > > > > is not supported\n"); > > > > > > > > > + return -EOPNOTSUPP; > > > > > > > > > + } > > > > > > > > > + > > > > > > > > > + down_write(&ndev->reslock); > > > > > > > > > + for (i = 0; i < ndev->cur_num_vqs; i++) { > > > > > > > > > + mvq = &ndev->vqs[i]; > > > > > > > > > + suspend_vq(ndev, mvq); > > > > > > > > > + } > > > > > > > > > + mlx5_vdpa_cvq_suspend(mvdev, suspend); > > > > > > > > > > > > > > > > Do we need to synchronize with the carrier work here? Otherwise > > > > > > > > we may > > > > > > > > get config notification after suspending. > > > > > > > > > > > > > > > > > + up_write(&ndev->reslock); > > > > > > > > > + return 0; > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > static const struct vdpa_config_ops mlx5_vdpa_ops = { > > > > > > > > > .set_vq_address = mlx5_vdpa_set_vq_address, > > > > > > > > > .set_vq_num = mlx5_vdpa_set_vq_num, > > > > > > > > > @@ -2762,6 +2826,7 @@ static const struct vdpa_config_ops > > > > > > > > > mlx5_vdpa_ops = { > > > > > > > > > .get_generation = mlx5_vdpa_get_generation, > > > > > > > > > .set_map = mlx5_vdpa_set_map, > > > > > > > > > .free = mlx5_vdpa_free, > > > > > > > > > + .suspend = mlx5_vdpa_suspend, > > > > > > > > > > > > > > > > I don't see the vDPA bus patch to enable this method. Or > > > > > > > > anything I missed here? > > > > > > > > > > > > > > > > > > > > > > Should we add > > > > > > > Based-on: <[email protected]> > > > > > > > > > > > > > > To this series? > > > > > > > > > > > > Probably, but that series seems to support resume while this series > > > > > > doesn't. > > > > > > > > > > > > Any reason for this? > > > > > > > > > > I think Eugenio agreed that resume is not really required since we're > > > > > going stop using this > > > > > instance and migrate. In any case, we don't support resume for the > > > > > hardware object > > > > > though it could be simulated should it be absolutely necessary. > > > > > > > > This is fine if everything is fine during the live migration. But when > > > > migration fails due to some reason, management (libvirt) may choose to > > > > restart the device in the source. > > > > > > > > This means we should either > > > > > > > > 1) support resume in the parent > > > > 2) emulate it in the qemu (with a lot of restoring of the states) > > > > > > > > > > I think it should be handled in qemu (at least the POC reset the > > > device), but I didn't exercise a lot of the failure paths there > > > because, well, it was a POC :). > > > > It looks like a must in the production environment. The failure is not > > necessarily related to shadow virtqueue itself. > > > > I don't see a specific interface to resume the device after suspend. > Reset however could do the job and we already have it.
Yes, this is fine as long as we can emulate it via set_vq_state + reset. Thanks > > > Thanks > > > > > > > > > And it is not only used for live migration, it could be used for > > > > vmstop/start. > > > > > > > > > > I think it would be easier if we dedicate a feature flag for resuming > > > the device in the future. Qemu could take advantage of it at some > > > error paths of live migration, but less than it seems because it > > > overrides things like ring addresses. And, obviously, in the > > > vmstop/vmstart. > > > > > > Actually, net devices should be ok to restore with a full reset. The > > > problem should be filesystems etc that are not part of vdpa at the > > > moment. > > > > > > Thanks! > > > > _______________________________________________ Virtualization mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/virtualization
