On Thu, Dec 7, 2023 at 4:04 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.
>
> 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@xxxxxxxxxxxxxxxxx>
Looks good overall. Minor comments.
> ---
> drivers/virtio/virtio_ring.c | 170 +++++++++++++++++++++++++----------
> include/linux/virtio.h | 16 ++++
> 2 files changed, 138 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 51d8f3299c10..3bcf3e6067af 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -362,6 +362,37 @@ static struct device *vring_dma_dev(const struct vring_virtqueue *vq)
> return vq->dma_dev;
> }
>
> +static bool get_dma_info(struct vring_virtqueue *vq,
> + struct virtio_dma_head *dma,
> + dma_addr_t addr, unsigned int length)
Rethink about the name, how about vring_need_unmap()?
> +{
> + if (vq->do_unmap)
> + return true;
> +
> + /* Only premapped mode. we should return the dma info to driver.
> + * If the use_dma_api is false, then do_unmap is false. But
> + * we should not return the dma info to driver.
> + */
It might be better to move this above this function? And we can
explain the return value there.
> + if (!vq->premapped)
> + return false;
> +
> + if (!dma)
> + return false;
It looks to me like it's better to check dma at the beginning.
> +
> + 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;
> +}
> +
> /* 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)
> @@ -440,12 +471,14 @@ static void virtqueue_init(struct vring_virtqueue *vq, u32 num)
> * Split ring specific functions - *_split().
> */
>
> -static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
> - const struct vring_desc *desc)
> +static void vring_unmap_one_split_indirect(struct vring_virtqueue *vq,
> + const struct vring_desc *desc,
> + struct virtio_dma_head *dma)
> {
> u16 flags;
>
> - if (!vq->do_unmap)
> + if (!get_dma_info(vq, dma, 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);
> @@ -457,8 +490,8 @@ static void vring_unmap_one_split_indirect(const struct vring_virtqueue *vq,
> DMA_FROM_DEVICE : DMA_TO_DEVICE);
> }
>
> -static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
> - unsigned int i)
> +static unsigned int vring_unmap_one_split(struct vring_virtqueue *vq,
> + unsigned int i, struct virtio_dma_head *dma)
> {
> struct vring_desc_extra *extra = vq->split.desc_extra;
> u16 flags;
> @@ -474,17 +507,16 @@ static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
> extra[i].len,
> (flags & VRING_DESC_F_WRITE) ?
> DMA_FROM_DEVICE : DMA_TO_DEVICE);
> - } else {
> - if (!vq->do_unmap)
> - goto out;
> -
> - dma_unmap_page(vring_dma_dev(vq),
> - extra[i].addr,
> - extra[i].len,
> - (flags & VRING_DESC_F_WRITE) ?
> - DMA_FROM_DEVICE : DMA_TO_DEVICE);
> + goto out;
> }
>
> + if (!get_dma_info(vq, dma, extra[i].addr, extra[i].len))
> + goto out;
> +
> + dma_unmap_page(vring_dma_dev(vq), extra[i].addr, extra[i].len,
> + (flags & VRING_DESC_F_WRITE) ?
> + DMA_FROM_DEVICE : DMA_TO_DEVICE);
> +
> out:
> return extra[i].next;
> }
> @@ -717,10 +749,10 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> if (i == err_idx)
> break;
> if (indirect) {
> - vring_unmap_one_split_indirect(vq, &desc[i]);
> + vring_unmap_one_split_indirect(vq, &desc[i], NULL);
> i = virtio16_to_cpu(_vq->vdev, desc[i].next);
> } else
> - i = vring_unmap_one_split(vq, i);
> + i = vring_unmap_one_split(vq, i, NULL);
> }
>
> free_indirect:
> @@ -763,11 +795,13 @@ static bool virtqueue_kick_prepare_split(struct virtqueue *_vq)
> }
>
> static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> - void **ctx)
> + struct virtio_dma_head *dma, void **ctx)
> {
> unsigned int i, j;
> __virtio16 nextflag = cpu_to_virtio16(vq->vq.vdev, VRING_DESC_F_NEXT);
>
> + WARN_ON_ONCE(vq->premapped && !dma);
Let's add a comment here as well as the packed version.
Thanks
