On Fri, Nov 24, 2023 at 1:56 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, 24 Nov 2023 12:14:57 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > On Wed, Nov 22, 2023 at 2:57 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, 22 Nov 2023 14:47:02 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > On Wed, Nov 22, 2023 at 2:34 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Wed, 22 Nov 2023 14:03:32 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > > > On Tue, Nov 21, 2023 at 2:27 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Thu, 16 Nov 2023 16:11:11 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > > > > > > On Tue, Nov 14, 2023 at 7:31 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> 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.
> > > > > > > >
> > > > > > > > So looking at vring_desc_extra, what we have right now is:
> > > > > > > >
> > > > > > > > struct vring_desc_extra {
> > > > > > > > dma_addr_t addr; /* Descriptor DMA addr. */
> > > > > > > > u32 len; /* Descriptor length. */
> > > > > > > > u16 flags; /* Descriptor flags. */
> > > > > > > > u16 next; /* The next desc state in a list. */
> > > > > > > > };
> > > > > > > >
> > > > > > > > And sg is
> > > > > > > >
> > > > > > > > struct scatterlist {
> > > > > > > > unsigned long page_link;
> > > > > > > > unsigned int offset;
> > > > > > > > unsigned int length;
> > > > > > > > dma_addr_t dma_address;
> > > > > > > > #ifdef CONFIG_NEED_SG_DMA_LENGTH
> > > > > > > > unsigned int dma_length;
> > > > > > > > #endif
> > > > > > > > #ifdef CONFIG_NEED_SG_DMA_FLAGS
> > > > > > > > unsigned int dma_flags;
> > > > > > > > #endif
> > > > > > > > };
> > > > > > > >
> > > > > > > > Would it better just store sg?
> > > > > > >
> > > > > > > Do you mean we expose the vring_desc_extra to dirver?
> > > > > >
> > > > > > Maybe or not, (or just us sg for desc_extra).
> > > > > >
> > > > > > Anyhow, any method needs to be benchmarked to see the performance impact.
> > > > > >
> > > > > > >
> > > > > > > How about introducing such a new structure?
> > > > > > >
> > > > > > > struct virtio_dma_item {
> > > > > > > dma_addr_t addr;
> > > > > > > u32 len;
> > > > > > > };
> > > > > > >
> > > > > > > struct virtio_dma_head {
> > > > > > > u32 num;
> > > > > > > u32 used;
> > > > > > > struct virtio_dma_item items[];
> > > > > > > };
> > > > > > >
> > > > > > > Then we just need to pass one pointer to the virtio.
> > > > > >
> > > > > > Just to make sure I understand here, where did we store those arrays?
> > > > >
> > > > >
> > > > > On the stack, or we can reuse the memory space of sq->sg.
> > > > >
> > > > > dma = (struct virtio_dma_head*)sq->sg;
> > > >
> > > > Ok, we need some benchmark for sure.
> > > >
> > > > >
> > > > >
> > > > > >
> > > > > > > num is used to pass the size of items
> > > > > > > used is used to return the num used by virtio core.
> > > > > > >
> > > > > > > >
> > > > > > > > More below
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > > > > > > > > ---
> > > > > > > > > drivers/virtio/virtio_ring.c | 148 ++++++++++++++++++++++++++---------
> > > > > > > > > include/linux/virtio.h | 2 +
> > > > > > > > > 2 files changed, 115 insertions(+), 35 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > > > > index 51d8f3299c10..0b3caee4ef9d 100644
> > > > > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > > > > @@ -362,6 +362,20 @@ static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
> > > > > > > > > return vq->dma_dev;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +static void store_dma_to_sg(struct scatterlist **sgp, dma_addr_t addr, unsigned int length)
> > > > > > > > > +{
> > > > > > > > > + struct scatterlist *sg;
> > > > > > > > > +
> > > > > > > > > + sg = *sgp;
> > > > > > > > > +
> > > > > > > > > + sg->dma_address = addr;
> > > > > > > > > + sg->length = length;
> > > > > > > > > +
> > > > > > > > > + sg = sg_next(sg);
> > > > > > > > > +
> > > > > > > > > + *sgp = sg;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > /* Map one sg entry. */
> > > > > > > > > static int vring_map_one_sg(const struct vring_virtqueue *vq, struct scatterlist *sg,
> > > > > > > > > enum dma_data_direction direction, dma_addr_t *addr)
> > > > > > > > > @@ -441,12 +455,18 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
> > > > > > > > > */
> > > > > > > > >
> > > > > > > > > static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
> > > > > > > > > - const struct vring_desc *desc)
> > > > > > > > > + const struct vring_desc *desc,
> > > > > > > > > + struct scatterlist **sg)
> > > > > > > > > {
> > > > > > > > > u16 flags;
> > > > > > > > >
> > > > > > > > > - if (!vq->do_unmap)
> > > > > > > > > + if (!vq->do_unmap) {
> > > > > > > > > + if (*sg)
> > > > > > > >
> > > > > > > > Can we simply move the
> > > > > > > >
> > > > > > > > if (*sg) to store_dma_to_sg()?
> > > > > > >
> > > > > > > OK
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > > + store_dma_to_sg(sg,
> > > > > > > > > + virtio64_to_cpu(vq->vq.vdev, desc->addr),
> > > > > > > > > + virtio32_to_cpu(vq->vq.vdev, desc->len));
> > > > > > > > > return;
> > > > > > > > > + }
> > > > > > > > >
> > > > > > > > > flags = virtio16_to_cpu(vq->vq.vdev, desc->flags);
> > > > > > > > >
> > > > > > > > > @@ -458,7 +478,7 @@ static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
> > > > > > > > > - unsigned int i)
> > > > > > > > > + unsigned int i, struct scatterlist **sg)
> > > > > > > > > {
> > > > > > > > > struct vring_desc_extra *extra = vq->split.desc_extra;
> > > > > > > > > u16 flags;
> > > > > > > > > @@ -475,8 +495,11 @@ static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
> > > > > > > > > (flags & VRING_DESC_F_WRITE) ?
> > > > > > > > > DMA_FROM_DEVICE : DMA_TO_DEVICE);
> > > > > > > > > } else {
> > > > > > > > > - if (!vq->do_unmap)
> > > > > > > > > + if (!vq->do_unmap) {
> > > > > > > >
> > > > > > > > The:
> > > > > > > >
> > > > > > > > else {
> > > > > > > > if {
> > > > > > > > if {
> > > > > > > > }
> > > > > > > >
> > > > > > > > seems odd. I think I would prefer
> > > > > > > >
> > > > > > > > if (flags & VRING_DESC_F_INDIRECT) {
> > > > > > > > } else if (!vq->do_unmap) {
> > > > > > > > } else {
> > > > > > > > }
> > > > > > >
> > > > > > >
> > > > > > > Will fix.
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > here
> > > > > > > >
> > > > > > > > Btw, I really think do_unmap is not a good name, we probably need to
> > > > > > > > rename it as "unmap_desc".
> > > > > > >
> > > > > > > How about a separate patch for this?
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > + if (*sg)
> > > > > > > > > + store_dma_to_sg(sg, extra[i].addr, extra[i].len);
> > > > > > > >
> > > > > > > > In which case we need to unmap by driver but we don't need a dma address?
> > > > > > >
> > > > > > > Sorry, do not get it.
> > > > > >
> > > > > > I meant, I don't get how we can reach to this check.
> > > > >
> > > > > This is the main way. I think you miss something.
> > > >
> > > > Or maybe you can explain the case where vq->do_unmap is false but *sg is NULL?
> > >
> > >
> > > If vq->do_unmap is false, that is premapped mode or use_dma_api is false.
> > > If the *sg is null, that means that the driver call the API
> > > virtqueue_get_buf_ctx or virtqueue_get_buf.
> > > If the *sg is not null, that means that the driver call the API
> > >
> > > void *virtqueue_get_buf_ctx_dma(struct virtqueue *_vq, unsigned int *len,
> > > struct scatterlist *sg, void **ctx);
> > >
> > > Thanks.
> >
> > Ok, but my question still:
> >
> > 1) you explain one possible way here is use_dma_api is false and the
> > driver doesn't call virtqueue_get_buf_ctx_dma().
>
> If the use_dma_api is false, then we cannot set to premapped mode.
> If the driver calls virtqueue_get_buf_ctx_dma(), that works as
> virtqueue_get_buf_ctx().
>
>
> > 2) what happens if the driver uses a preamp but doesn't use
> > virtqueue_get_buf_ctx_dma()? Should we warn in this case?
>
> The developer should know that he need to do unmap, so I think
> that is not needed.
Well, kind of layer violation here. Driver may trust the virtio core
but not verse vica.
E.g there are several WARN_ON() in the virtio_ring.
Thanks
>
> Thanks.
>
>
>
> >
> > Thanks
> >
>
