linux-kernel - Re: [PATCH vhost v2 4/8] vdpa/mlx5: Mark vq addrs for modification in hw vq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9a6465a3d6c8fde63643fbbdde60d5dd84b921d4.camel@nvidia.com>
Date:   Thu, 14 Dec 2023 15:51:27 +0000
From:   Dragos Tatulea <dtatulea@...dia.com>
To:     "mst@...hat.com" <mst@...hat.com>
CC:     "xuanzhuo@...ux.alibaba.com" <xuanzhuo@...ux.alibaba.com>,
        Parav Pandit <parav@...dia.com>, Gal Pressman <gal@...dia.com>,
        "eperezma@...hat.com" <eperezma@...hat.com>,
        "virtualization@...ts.linux-foundation.org" 
        <virtualization@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "si-wei.liu@...cle.com" <si-wei.liu@...cle.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "jasowang@...hat.com" <jasowang@...hat.com>,
        Saeed Mahameed <saeedm@...dia.com>,
        "leon@...nel.org" <leon@...nel.org>
Subject: Re: [PATCH vhost v2 4/8] vdpa/mlx5: Mark vq addrs for modification in
 hw vq

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 <dtatulea@...dia.com> wrote:
> > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on
> > > > > next modify_virtqueue.
> > > > > 
> > > > > Signed-off-by: Dragos Tatulea <dtatulea@...dia.com>
> > > > > Reviewed-by: Gal Pressman <gal@...dia.com>
> > > > > Acked-by: Eugenio Pérez <eperezma@...hat.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

?

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
> > > > > 
> > > 
> > 
>