On Mon, Dec 18, 2023 at 11:52 AM Dragos Tatulea <dtatu...@nvidia.com> wrote:
>
> On Mon, 2023-12-18 at 11:16 +0100, Eugenio Perez Martin wrote:
> > On Sat, Dec 16, 2023 at 12:03 PM Dragos Tatulea <dtatu...@nvidia.com> wrote:
> > >
> > > On Fri, 2023-12-15 at 18:56 +0100, Eugenio Perez Martin wrote:
> > > > On Fri, Dec 15, 2023 at 3:13 PM Dragos Tatulea <dtatu...@nvidia.com>
> > > > wrote:
> > > > >
> > > > > On Fri, 2023-12-15 at 12:35 +0000, Dragos Tatulea wrote:
> > > > > > On Thu, 2023-12-14 at 19:30 +0100, Eugenio Perez Martin wrote:
> > > > > > > On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea
> > > > > > > <dtatu...@nvidia.com> wrote:
> > > > > > > >
> > > > > > > > On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote:
> > > > > > > > > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea
> > > > > > > > > wrote:
> > > > > > > > > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote:
> > > > > > > > > > >
> > > > > > > > > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote:
> > > > > > > > > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea
> > > > > > > > > > > > <dtatu...@nvidia.com> wrote:
> > > > > > > > > > > > > Addresses get set by .set_vq_address. hw vq addresses
> > > > > > > > > > > > > will be updated on
> > > > > > > > > > > > > next modify_virtqueue.
> > > > > > > > > > > > >
> > > > > > > > > > > > > Signed-off-by: Dragos Tatulea <dtatu...@nvidia.com>
> > > > > > > > > > > > > Reviewed-by: Gal Pressman <g...@nvidia.com>
> > > > > > > > > > > > > Acked-by: Eugenio Pérez <epere...@redhat.com>
> > > > > > > > > > > > I'm kind of ok with this patch and the next one about
> > > > > > > > > > > > state, but I
> > > > > > > > > > > > didn't ack them in the previous series.
> > > > > > > > > > > >
> > > > > > > > > > > > My main concern is that it is not valid to change the
> > > > > > > > > > > > vq address after
> > > > > > > > > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory
> > > > > > > > > > > > maps are ok to
> > > > > > > > > > > > change at this moment. I'm not sure about vq state in
> > > > > > > > > > > > vDPA, but vhost
> > > > > > > > > > > > forbids changing it with an active backend.
> > > > > > > > > > > >
> > > > > > > > > > > > Suspend is not defined in VirtIO at this moment though,
> > > > > > > > > > > > so maybe it is
> > > > > > > > > > > > ok to decide that all of these parameters may change
> > > > > > > > > > > > during suspend.
> > > > > > > > > > > > Maybe the best thing is to protect this with a vDPA
> > > > > > > > > > > > feature flag.
> > > > > > > > > > > I think protect with vDPA feature flag could work, while
> > > > > > > > > > > on the other
> > > > > > > > > > > hand vDPA means vendor specific optimization is possible
> > > > > > > > > > > around suspend
> > > > > > > > > > > and resume (in case it helps performance), which doesn't
> > > > > > > > > > > have to be
> > > > > > > > > > > backed by virtio spec. Same applies to vhost user backend
> > > > > > > > > > > features,
> > > > > > > > > > > variations there were not backed by spec either. Of
> > > > > > > > > > > course, we should
> > > > > > > > > > > try best to make the default behavior backward compatible
> > > > > > > > > > > with
> > > > > > > > > > > virtio-based backend, but that circles back to no suspend
> > > > > > > > > > > definition in
> > > > > > > > > > > the current virtio spec, for which I hope we don't cease
> > > > > > > > > > > development on
> > > > > > > > > > > vDPA indefinitely. After all, the virtio based vdap
> > > > > > > > > > > backend can well
> > > > > > > > > > > define its own feature flag to describe (minor difference
> > > > > > > > > > > in) the
> > > > > > > > > > > suspend behavior based on the later spec once it is
> > > > > > > > > > > formed in future.
> > > > > > > > > > >
> > > > > > > > > > So what is the way forward here? From what I understand the
> > > > > > > > > > options are:
> > > > > > > > > >
> > > > > > > > > > 1) Add a vdpa feature flag for changing device properties
> > > > > > > > > > while suspended.
> > > > > > > > > >
> > > > > > > > > > 2) Drop these 2 patches from the series for now. Not sure
> > > > > > > > > > if this makes sense as
> > > > > > > > > > this. But then Si-Wei's qemu device suspend/resume poc [0]
> > > > > > > > > > that exercises this
> > > > > > > > > > code won't work anymore. This means the series would be
> > > > > > > > > > less well tested.
> > > > > > > > > >
> > > > > > > > > > Are there other possible options? What do you think?
> > > > > > > > > >
> > > > > > > > > > [0]
> > > > > > > > > > https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip
> > > > > > > > >
> > > > > > > > > I am fine with either of these.
> > > > > > > > >
> > > > > > > > How about allowing the change only under the following
> > > > > > > > conditions:
> > > > > > > > vhost_vdpa_can_suspend && vhost_vdpa_can_resume &&
> > > > > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set
> > > > > > > >
> > > > > > > > ?
> > > > > > >
> > > > > > > I think the best option by far is 1, as there is no hint in the
> > > > > > > combination of these 3 indicating that you can change device
> > > > > > > properties in the suspended state.
> > > > > > >
> > > > > > Sure. Will respin a v3 without these two patches.
> > > > > >
> > > > > > Another series can implement option 2 and add these 2 patches on
> > > > > > top.
> > > > > Hmm...I misunderstood your statement and sent a erroneus v3. You said
> > > > > that
> > > > > having a feature flag is the best option.
> > > > >
> > > > > Will add a feature flag in v4: is this similar to the
> > > > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK flag?
> > > > >
> > > >
> > > > Right, it should be easy to return it from .get_backend_features op if
> > > > the FW returns that capability, isn't it?
> > > >
> > > Yes, that's easy. But I wonder if we need one feature bit for each type of
> > > change:
> > > - VHOST_BACKEND_F_CHANGEABLE_VQ_ADDR_IN_SUSPEND
> > > - VHOST_BACKEND_F_CHANGEABLE_VQ_STATE_IN_SUSPEND
> > >
> >
> > I'd say yes. Although we could configure SVQ initial state in userland
> > as different than 0 for this first step, it would be needed in the
> > long term.
> >
> > > Or would a big one VHOST_BACKEND_F_CAN_RECONFIG_VQ_IN_SUSPEND suffice?
> > >
> >
> > I'd say "reconfig vq" is not valid as mlx driver doesn't allow
> > changing queue sizes, for example, isn't it?
> >
> Modifying the queue size for a vq is indeed not supported by the mlx device.
>
> > To define that it is
> > valid to change "all parameters" seems very confident.
> >
> Ack
>
> > > To me having individual feature bits makes sense. But it could also takes
> > > too
> > > many bits if more changes are required.
> > >
> >
> > Yes, that's a good point. Maybe it is valid to define a subset of
> > features that can be changed., but I think it is way clearer to just
> > check for individual feature bits.
> >
> I will prepare extra patches with the 2 feature bits approach.
>
> Is it necessary to add checks in the vdpa core that block changing these
> properties if the state is driver ok and the device doesn't support the
> feature?
>
Yes, I think it is better to protect for changes in vdpa core.
> > > Thanks,
> > > Dragos
> > >
> > > > > Thanks,
> > > > > Dragos
> > > > >
> > > > > > > > Thanks,
> > > > > > > > Dragos
> > > > > > > >
> > > > > > > > > > Thanks,
> > > > > > > > > > Dragos
> > > > > > > > > >
> > > > > > > > > > > Regards,
> > > > > > > > > > > -Siwei
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Jason, what do you think?
> > > > > > > > > > > >
> > > > > > > > > > > > Thanks!
> > > > > > > > > > > >
> > > > > > > > > > > > > ---
> > > > > > > > > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++
> > > > > > > > > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
> > > > > > > > > > > > > 2 files changed, 10 insertions(+)
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > > > index f8f088cced50..80e066de0866 100644
> > > > > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > > > > @@ -1209,6 +1209,7 @@ static int
> > > > > > > > > > > > > modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > > > > > > > > > > bool state_change = false;
> > > > > > > > > > > > > void *obj_context;
> > > > > > > > > > > > > void *cmd_hdr;
> > > > > > > > > > > > > + void *vq_ctx;
> > > > > > > > > > > > > void *in;
> > > > > > > > > > > > > int err;
> > > > > > > > > > > > >
> > > > > > > > > > > > > @@ -1230,6 +1231,7 @@ static int
> > > > > > > > > > > > > modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > > > > > > > > > > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr,
> > > > > > > > > > > > > uid, ndev->mvdev.res.uid);
> > > > > > > > > > > > >
> > > > > > > > > > > > > obj_context =
> > > > > > > > > > > > > MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context);
> > > > > > > > > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object,
> > > > > > > > > > > > > obj_context, virtio_q_context);
> > > > > > > > > > > > >
> > > > > > > > > > > > > if (mvq->modified_fields &
> > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE) {
> > > > > > > > > > > > > if
> > > > > > > > > > > > > (!is_valid_state_change(mvq->fw_state, state,
> > > > > > > > > > > > > is_resumable(ndev))) {
> > > > > > > > > > > > > @@ -1241,6 +1243,12 @@ static int
> > > > > > > > > > > > > modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > > > > > > > > > > state_change = true;
> > > > > > > > > > > > > }
> > > > > > > > > > > > >
> > > > > > > > > > > > > + if (mvq->modified_fields &
> > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) {
> > > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx,
> > > > > > > > > > > > > desc_addr, mvq->desc_addr);
> > > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx,
> > > > > > > > > > > > > used_addr, mvq->device_addr);
> > > > > > > > > > > > > + MLX5_SET64(virtio_q, vq_ctx,
> > > > > > > > > > > > > available_addr, mvq->driver_addr);
> > > > > > > > > > > > > + }
> > > > > > > > > > > > > +
> > > > > > > > > > > > > MLX5_SET64(virtio_net_q_object, obj_context,
> > > > > > > > > > > > > modify_field_select, mvq->modified_fields);
> > > > > > > > > > > > > err = mlx5_cmd_exec(ndev->mvdev.mdev, in,
> > > > > > > > > > > > > inlen, out, sizeof(out));
> > > > > > > > > > > > > if (err)
> > > > > > > > > > > > > @@ -2202,6 +2210,7 @@ static int
> > > > > > > > > > > > > mlx5_vdpa_set_vq_address(struct vdpa_device *vdev,
> > > > > > > > > > > > > u16 idx, u64 desc_
> > > > > > > > > > > > > mvq->desc_addr = desc_area;
> > > > > > > > > > > > > mvq->device_addr = device_area;
> > > > > > > > > > > > > mvq->driver_addr = driver_area;
> > > > > > > > > > > > > + mvq->modified_fields |=
> > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS;
> > > > > > > > > > > > > return 0;
> > > > > > > > > > > > > }
> > > > > > > > > > > > >
> > > > > > > > > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > > > > > > > > > b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > > > > > > > > > index b86d51a855f6..9594ac405740 100644
> > > > > > > > > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > > > > > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > > > > > > > > > @@ -145,6 +145,7 @@ enum {
> > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE
> > > > > > > > > > > > > = (u64)1 << 0,
> > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS
> > > > > > > > > > > > > = (u64)1 << 3,
> > > > > > > > > > > > >
> > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE =
> > > > > > > > > > > > > (u64)1 << 4,
> > > > > > > > > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS
> > > > > > > > > > > > > = (u64)1 << 6,
> > > > > > > > > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY
> > > > > > > > > > > > > = (u64)1 << 14,
> > > > > > > > > > > > > };
> > > > > > > > > > > > >
> > > > > > > > > > > > > --
> > > > > > > > > > > > > 2.42.0
> > > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
