On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
>
> DMA-related logic is separated from the virtqueue_add_split() to
> one new function. DMA address will be saved as sg->dma_address, then
> virtqueue_add_split() will use it directly. Unmap operation will be
> simpler.
>
> The purpose of this is to facilitate subsequent support to receive
> dma address mapped by drivers.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> ---
> drivers/virtio/virtio_ring.c | 110 ++++++++++++++++++++++++++---------
> 1 file changed, 82 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 41144b5246a8..8ace2f503953 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -520,6 +520,77 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
> return next;
> }
>
> +static void virtqueue_unmap_sgs(struct vring_virtqueue *vq,
> + struct scatterlist *sgs[],
> + unsigned int total_sg,
> + unsigned int out_sgs,
> + unsigned int in_sgs)
> +{
> + struct scatterlist *sg;
> + unsigned int n;
> +
> + if (!vq->use_dma_api)
> + return;
> +
> + for (n = 0; n < out_sgs; n++) {
> + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> + if (!sg->dma_address)
> + return;
> +
> + dma_unmap_page(vring_dma_dev(vq), sg->dma_address,
> + sg->length, DMA_TO_DEVICE);
> + }
> + }
> +
> + for (; n < (out_sgs + in_sgs); n++) {
> + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> + if (!sg->dma_address)
> + return;
> +
> + dma_unmap_page(vring_dma_dev(vq), sg->dma_address,
> + sg->length, DMA_FROM_DEVICE);
> + }
> + }
> +}
> +
> +static int virtqueue_map_sgs(struct vring_virtqueue *vq,
> + struct scatterlist *sgs[],
> + unsigned int total_sg,
> + unsigned int out_sgs,
> + unsigned int in_sgs)
> +{
> + struct scatterlist *sg;
> + unsigned int n;
> +
So we have a use_dma_api check in the unmap path but not here. More below.
> + for (n = 0; n < out_sgs; n++) {
> + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> + dma_addr_t addr = vring_map_one_sg(vq, sg, DMA_TO_DEVICE);
> +
> + if (vring_mapping_error(vq, addr))
> + goto err;
> +
> + sg->dma_address = addr;
When KMSAN is not enabled (should be the case for production
environments), the only work that is done here is to assign the
dma_address. And there will be another loop after the mapping to add
descriptors, this seems to slow down the performance. Any benchmark
for this patch? Or can we:
1) return early when use_dma_api is false
2) have a helper like
vring_sg_address() {
if (!sg->dma_address)
return sg_phys(sg);
return sg->dma_address;
}
Thanks
> + }
> + }
> +
> + for (; 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, DMA_FROM_DEVICE);
> +
> + if (vring_mapping_error(vq, addr))
> + goto err;
> +
> + sg->dma_address = addr;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
> + return -ENOMEM;
> +}
> +
> static inline int virtqueue_add_split(struct virtqueue *_vq,
> struct scatterlist *sgs[],
> unsigned int total_sg,
> @@ -532,9 +603,9 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> struct vring_virtqueue *vq = to_vvq(_vq);
> struct scatterlist *sg;
> struct vring_desc *desc;
> - unsigned int i, n, avail, descs_used, prev, err_idx;
> - int head;
> + unsigned int i, n, avail, descs_used, prev;
> bool indirect;
> + int head;
>
> START_USE(vq);
>
> @@ -586,32 +657,30 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> return -ENOSPC;
> }
>
> + if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
> + return -ENOMEM;
> +
> for (n = 0; n < out_sgs; n++) {
> for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> - dma_addr_t addr = vring_map_one_sg(vq, sg, DMA_TO_DEVICE);
> - if (vring_mapping_error(vq, addr))
> - goto unmap_release;
> -
> prev = i;
> /* Note that we trust indirect descriptor
> * table since it use stream DMA mapping.
> */
> - i = virtqueue_add_desc_split(_vq, desc, i, addr, sg->length,
> + i = virtqueue_add_desc_split(_vq, desc, i,
> + sg->dma_address,
> + sg->length,
> VRING_DESC_F_NEXT,
> indirect);
> }
> }
> for (; 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, DMA_FROM_DEVICE);
> - if (vring_mapping_error(vq, addr))
> - goto unmap_release;
> -
> prev = i;
> /* Note that we trust indirect descriptor
> * table since it use stream DMA mapping.
> */
> - i = virtqueue_add_desc_split(_vq, desc, i, addr,
> + i = virtqueue_add_desc_split(_vq, desc, i,
> + sg->dma_address,
> sg->length,
> VRING_DESC_F_NEXT |
> VRING_DESC_F_WRITE,
> @@ -679,22 +748,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> return 0;
>
> unmap_release:
> - err_idx = i;
> -
> - if (indirect)
> - i = 0;
> - else
> - i = head;
> -
> - for (n = 0; n < total_sg; n++) {
> - if (i == err_idx)
> - break;
> - if (indirect) {
> - vring_unmap_one_split_indirect(vq, &desc[i]);
> - i = virtio16_to_cpu(_vq->vdev, desc[i].next);
> - } else
> - i = vring_unmap_one_split(vq, i);
> - }
> + virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
>
> if (indirect)
> kfree(desc);
> --
> 2.32.0.3.g01195cf9f
>
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
