netdev - Re: [PATCH net-next v3 06/27] virtio_ring: introduce virtqueue_get_buf_ctx

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Mon, 22 Jan 2024 14:04:04 +0800
From: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
To: Jason Wang <jasowang@...hat.com>
Cc: netdev@...r.kernel.org,
 "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>,
 "Michael S. Tsirkin" <mst@...hat.com>,
 Alexei Starovoitov <ast@...nel.org>,
 Daniel Borkmann <daniel@...earbox.net>,
 Jesper Dangaard Brouer <hawk@...nel.org>,
 John Fastabend <john.fastabend@...il.com>,
 virtualization@...ts.linux-foundation.org,
 bpf@...r.kernel.org
Subject: Re: [PATCH net-next v3 06/27] virtio_ring: introduce virtqueue_get_buf_ctx_dma()

On Mon, 22 Jan 2024 12:18:51 +0800, Jason Wang <jasowang@...hat.com> wrote:
> On Tue, Jan 16, 2024 at 3:47 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> >
> > On Thu, 11 Jan 2024 16:34:09 +0800, Jason Wang <jasowang@...hat.com> wrote:
> > > On Fri, Dec 29, 2023 at 3:31 PM Xuan Zhuo <xuanzhuo@...ux.alibaba.com> wrote:
> > > >
> > > > introduce virtqueue_get_buf_ctx_dma() to collect the dma info when
> > > > get buf from virtio core for premapped mode.
> > > >
> > > > If the virtio queue is premapped mode, the virtio-net send buf may
> > > > have many desc. Every desc dma address need to be unmap. So here we
> > > > introduce a new helper to collect the dma address of the buffer from
> > > > the virtio core.
> > > >
> > > > Because the BAD_RING is called (that may set vq->broken), so
> > > > the relative "const" of vq is removed.
> > > >
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
> > > > ---
> > > >  drivers/virtio/virtio_ring.c | 174 +++++++++++++++++++++++++----------
> > > >  include/linux/virtio.h       |  16 ++++
> > > >  2 files changed, 142 insertions(+), 48 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 51d8f3299c10..1374b3fd447c 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -362,6 +362,45 @@ static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
> > > >         return vq->dma_dev;
> > > >  }
> > > >
> > > > +/*
> > > > + *     use_dma_api premapped -> do_unmap
> > > > + *  1. false       false        false
> > > > + *  2. true        false        true
> > > > + *  3. true        true         false
> > > > + *
> > > > + * Only #3, we should return the DMA info to the driver.
> > >
> > > Btw, I guess you meant "#3 is false" here?
> > >
> > > And could we reduce the size of these 3 * 3 matrices? It's usually a
> > > hint that the code is not optmized.
> >
> > On the process of doing dma map, we force the (use_dma_api, premapped).
> >
> > if premapped:
> >      virtio core skip dma map
> > else:
> >         if use_dma_api:
> >                 do dma map
> >         else:
> >                 work with the physical address.
> >
> > Here we force the (premapped, do_unmap).
> >
> > do_unmap is an optimization. We just check this to know should we do dma unmap
> > or not.
> >
> > Now, we introduced an new case, when the virtio core skip dma unmap,
> > we may need to return the dma info to the driver. That just occur when
> > the (premapped, do_unmap) is (true, false). Because that the (premmaped,
> > do_unmap) may be (false, false).
> >
> > For the matrices, I just want to show where the do_unmap comes from.
> > That is a optimization, we use this many places, not to check (use_dma_api,
> > premapped) on the process of doing unmap. And only for the case #3, we should
> > return the dma info to drivers.
>
> Ok, it tries to ease the life of the readers.
>
> I wonder if something like
>
> bool virtqueue_needs_unmap() can help, it can judge based on the value
> of use_dma_api and premapped.


I think not too much.

Because do_unmap is for this.



+static bool vring_need_unmap(struct vring_virtqueue *vq,
+			     struct virtio_dma_head *dma,
+			     dma_addr_t addr, unsigned int length)
+{
+	if (vq->do_unmap)
+		return true;

Before this, we is to judge whether we should do unmap or not.
After this, we is to judge whehter we should return dma info to driver or not.

If you want to simplify this function, I will say no.

If you want to replace "do_unmap" with virtqueue_needs_unmap(), I will say ok.
But I think we donot need to do that.

+
+	if (!vq->premapped)
+		return false;
+
+	if (!dma)
+		return false;
+
+	if (unlikely(dma->next >= dma->num)) {
+		BAD_RING(vq, "premapped vq: collect dma overflow: %pad %u\n",
+			 &addr, length);
+		return false;
+	}
+
+	dma->items[dma->next].addr = addr;
+	dma->items[dma->next].length = length;
+
+	++dma->next;
+
+	return false;
+}


Thanks.


>
> Thanks
>
> >
> > Thanks.
> >
> > >
> > > Thanks
> > >
> > >
> >
>