On Thu, Feb 02, 2023 at 07:00:28PM +0800, Xuan Zhuo wrote:
> virtqueue_add_packed() only supports virtual addresses, dma is completed
> in virtqueue_add_packed().
>
> In some scenarios (such as the AF_XDP scenario), the memory is allocated
> and DMA is completed in advance, so it is necessary for us to support
> passing the DMA address to virtqueue_add_packed().
>
> Record this information in desc_state, we can skip unmap based on this
> when executing dma unmap.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> ---
> drivers/virtio/virtio_ring.c | 71 +++++++++++++++++++++++++-----------
> 1 file changed, 50 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index ec622403cbd5..25027a35fcf8 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -78,6 +78,7 @@ struct vring_desc_state_packed {
> struct vring_packed_desc *indir_desc; /* Indirect descriptor, if any. */
> u16 num; /* Descriptor list length. */
> u16 last; /* The last desc state in a list. */
> + bool premapped;
> };
>
> struct vring_desc_extra {
That's an extra cache line.
> @@ -1200,7 +1201,8 @@ static inline u16 packed_last_used(u16 last_used_idx)
> }
>
> static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
> - struct vring_desc_extra *extra)
> + struct vring_desc_extra *extra,
> + bool premapped)
> {
> u16 flags;
>
> @@ -1215,6 +1217,9 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
> (flags & VRING_DESC_F_WRITE) ?
> DMA_FROM_DEVICE : DMA_TO_DEVICE);
> } else {
> + if (premapped)
> + return;
> +
> dma_unmap_page(vring_dma_dev(vq),
> extra->addr, extra->len,
> (flags & VRING_DESC_F_WRITE) ?
> @@ -1262,7 +1267,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> unsigned int out_sgs,
> unsigned int in_sgs,
> void *data,
> - gfp_t gfp)
> + gfp_t gfp,
> + bool premapped)
> {
> struct vring_packed_desc *desc;
> struct scatterlist *sg;
> @@ -1288,10 +1294,15 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
>
> for (n = 0; n < out_sgs + in_sgs; n++) {
> for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> - addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> - DMA_TO_DEVICE : DMA_FROM_DEVICE);
> - if (vring_mapping_error(vq, addr))
> - goto unmap_release;
> + if (premapped) {
> + addr = sg_dma_address(sg);
> +
> + } else {
> + addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> + DMA_TO_DEVICE : DMA_FROM_DEVICE);
> + if (vring_mapping_error(vq, addr))
> + goto unmap_release;
> + }
>
> desc[i].flags = cpu_to_le16(n < out_sgs ?
> 0 : VRING_DESC_F_WRITE);
> @@ -1350,6 +1361,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> vq->packed.desc_state[id].data = data;
> vq->packed.desc_state[id].indir_desc = desc;
> vq->packed.desc_state[id].last = id;
> + vq->packed.desc_state[id].premapped = premapped;
>
> vq->num_added += 1;
>
> @@ -1359,10 +1371,11 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
> return 0;
>
> unmap_release:
> - err_idx = i;
> -
> - for (i = 0; i < err_idx; i++)
> - vring_unmap_desc_packed(vq, &desc[i]);
> + if (!premapped) {
> + err_idx = i;
> + for (i = 0; i < err_idx; i++)
> + vring_unmap_desc_packed(vq, &desc[i]);
> + }
>
> kfree(desc);
>
> @@ -1377,6 +1390,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> unsigned int in_sgs,
> void *data,
> void *ctx,
> + bool premapped,
> gfp_t gfp)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> @@ -1403,7 +1417,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
>
> if (virtqueue_use_indirect(vq, total_sg)) {
> err = virtqueue_add_indirect_packed(vq, sgs, total_sg, out_sgs,
> - in_sgs, data, gfp);
> + in_sgs, data, gfp, premapped);
> if (err != -ENOMEM) {
> END_USE(vq);
> return err;
> @@ -1435,10 +1449,17 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> c = 0;
> for (n = 0; n < out_sgs + in_sgs; n++) {
> for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> - dma_addr_t addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> - DMA_TO_DEVICE : DMA_FROM_DEVICE);
> - if (vring_mapping_error(vq, addr))
> - goto unmap_release;
> + dma_addr_t addr;
> +
> + if (premapped) {
> + addr = sg_dma_address(sg);
> +
drop this empty line pls.
> + } else {
> + addr = vring_map_one_sg(vq, sg, n < out_sgs ?
> + DMA_TO_DEVICE : DMA_FROM_DEVICE);
> + if (vring_mapping_error(vq, addr))
> + goto unmap_release;
> + }
>
> flags = cpu_to_le16(vq->packed.avail_used_flags |
> (++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
> @@ -1485,6 +1506,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> vq->packed.desc_state[id].data = data;
> vq->packed.desc_state[id].indir_desc = ctx;
> vq->packed.desc_state[id].last = prev;
> + vq->packed.desc_state[id].premapped = premapped;
>
> /*
> * A driver MUST NOT make the first descriptor in the list
> @@ -1501,22 +1523,26 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
> return 0;
>
> unmap_release:
> + vq->packed.avail_used_flags = avail_used_flags;
> +
> + if (premapped)
> + goto unmap_free;
> +
This goto branching inside error handling is too much like spaghetti code.
See Documentation/process/coding-style.rst for when goto is ok -
basically exit/error handling. This is not error handling.
Pls find a way to avoid.
> err_idx = i;
> i = head;
> curr = vq->free_head;
>
> - vq->packed.avail_used_flags = avail_used_flags;
> -
> for (n = 0; n < total_sg; n++) {
> if (i == err_idx)
> break;
> - vring_unmap_extra_packed(vq, &vq->packed.desc_extra[curr]);
> + vring_unmap_extra_packed(vq, &vq->packed.desc_extra[curr], false);
> curr = vq->packed.desc_extra[curr].next;
> i++;
> if (i >= vq->packed.vring.num)
> i = 0;
> }
>
> +unmap_free:
> END_USE(vq);
> return -EIO;
> }
> @@ -1576,8 +1602,10 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
> struct vring_desc_state_packed *state = NULL;
> struct vring_packed_desc *desc;
> unsigned int i, curr;
> + bool premapped;
>
> state = &vq->packed.desc_state[id];
> + premapped = state->premapped;
>
> /* Clear data ptr. */
> state->data = NULL;
> @@ -1590,7 +1618,8 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
> curr = id;
> for (i = 0; i < state->num; i++) {
> vring_unmap_extra_packed(vq,
> - &vq->packed.desc_extra[curr]);
> + &vq->packed.desc_extra[curr],
> + premapped);
> curr = vq->packed.desc_extra[curr].next;
> }
> }
> @@ -1603,7 +1632,7 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
> if (!desc)
> return;
>
> - if (vq->use_dma_api) {
> + if (vq->use_dma_api && !premapped) {
> len = vq->packed.desc_extra[id].len;
> for (i = 0; i < len / sizeof(struct vring_packed_desc);
> i++)
> @@ -2122,7 +2151,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> struct vring_virtqueue *vq = to_vvq(_vq);
>
> return vq->packed_ring ? virtqueue_add_packed(_vq, sgs, total_sg,
> - out_sgs, in_sgs, data, ctx, gfp) :
> + out_sgs, in_sgs, data, ctx, premapped, gfp) :
> virtqueue_add_split(_vq, sgs, total_sg,
> out_sgs, in_sgs, data, ctx, premapped, gfp);
> }
Too much if !premapped all over the place. Pls refactor so we
get common code and then have premapped and non premapped
versions call that.
> --
> 2.32.0.3.g01195cf9f
