netdev - Re: [PATCH vhost v4 10/10] virtio_ring: virtqueue_set_dma

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: <CACGkMEsMJ+r6JfrHGnQVPeuEFzbWwqHxPgkxS8-SvzqG5VLiKw@mail.gmail.com>
Date: Mon, 25 Mar 2024 15:10:39 +0800
From: Jason Wang <jasowang@...hat.com>
To: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Cc: virtualization@...ts.linux.dev, "Michael S. Tsirkin" <mst@...hat.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Subject: Re: [PATCH vhost v4 10/10] virtio_ring: virtqueue_set_dma_premapped
 support disable

On Fri, Mar 22, 2024 at 2:04 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
>
> On Fri, 22 Mar 2024 13:13:36 +0800, Jason Wang <jasowang@...hat.com> wrote:
> > On Thu, Mar 21, 2024 at 4:22 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> > >
> > > On Thu, 21 Mar 2024 14:02:14 +0800, Jason Wang <jasowang@...hat.com> wrote:
> > > > On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> > > > >
> > > > > Now, the API virtqueue_set_dma_premapped just support to
> > > > > enable premapped mode.
> > > > >
> > > > > If we allow enabling the premapped dynamically, we should
> > > > > make this API to support disable the premapped mode.
> > > > >
> > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > > > > ---
> > > > >  drivers/virtio/virtio_ring.c | 34 ++++++++++++++++++++++++++--------
> > > > >  include/linux/virtio.h       |  2 +-
> > > > >  2 files changed, 27 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 34f4b2c0c31e..3bf69cae4965 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -2801,6 +2801,7 @@ EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > > >  /**
> > > > >   * virtqueue_set_dma_premapped - set the vring premapped mode
> > > > >   * @_vq: the struct virtqueue we're talking about.
> > > > > + * @premapped: enable/disable the premapped mode.
> > > > >   *
> > > > >   * Enable the premapped mode of the vq.
> > > > >   *
> > > > > @@ -2819,9 +2820,10 @@ EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > > >   * 0: success.
> > > > >   * -EINVAL: vring does not use the dma api, so we can not enable premapped mode.
> > > > >   */
> > > > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq)
> > > > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool premapped)
> > > >
> > > > I think we need to document the requirement for calling this.
> > > >
> > > > Looking at the code, it seems it requires to stop the datapath and
> > > > detach all the used buffers?
> > >
> > >
> > > YES. The complete document is:
> > >
> > > /**
> > >  * virtqueue_set_dma_premapped - set the vring premapped mode
> > >  * @_vq: the struct virtqueue we're talking about.
> > >  *
> > >  * Enable the premapped mode of the vq.
> > >  *
> > >  * The vring in premapped mode does not do dma internally, so the driver must
> > >  * do dma mapping in advance. The driver must pass the dma_address through
> > >  * dma_address of scatterlist. When the driver got a used buffer from
> > >  * the vring, it has to unmap the dma address.
> > >  *
> > >  * This function must be called immediately after creating the vq, or after vq
> > >  * reset, and before adding any buffers to it.
> >
> > I'm not sure this is a good design but we need at least some guard for
> > this, probably WARN for num_added or others.
>
>
> int virtqueue_set_dma_premapped(struct virtqueue *_vq)
> {
>         struct vring_virtqueue *vq = to_vvq(_vq);
>         u32 num;
>
>         START_USE(vq);
>
>         num = vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num;
>
>         if (num != vq->vq.num_free) {
>                 END_USE(vq);
>                 return -EINVAL;
>         }
>
>
> Now, we have checked the num_free.

Ok, let's add it to the doc.

Thanks

>
> Thanks.
>
>
> >
> > Thanks
> >
> > >  *
> > >  * Caller must ensure we don't call this with other virtqueue operations
> > >  * at the same time (except where noted).
> > >  *
> > >  * Returns zero or a negative error.
> > >  * 0: success.
> > >  * -EINVAL: vring does not use the dma api, so we can not enable premapped mode.
> > >  */
> > >
> > > Thanks
> > >
> > >
> > > >
> > > > Thanks
> > > >
> > >
> >
>