On Wed, Nov 29, 2017 at 09:55:22PM +0800, Wei Wang wrote:
> Current virtqueue_add API implementation is based on the scatterlist
> struct, which uses kaddr. This is inadequate to all the use case of
> vring. For example:
> - Some usages don't use IOMMU, in this case the user can directly pass
> in a physical address in hand, instead of going through the sg
> implementation (e.g. the VIRTIO_BALLOON_F_SG feature)
> - Sometimes, a guest physical page may not have a kaddr (e.g. high
> memory) but need to use vring (e.g. the VIRTIO_BALLOON_F_FREE_PAGE_VQ
> feature)
>
> The new API virtqueue_add_one_desc enables the caller to assign a vring
> desc with a physical address and len. Also, factor out the common code
> with virtqueue_add in vring_set_avail.
>
> Signed-off-by: Wei Wang <wei.w.wang@xxxxxxxxx>
> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
You previously managed without this patch, and it's preferable
IMHO since this patchset is already too big.
I don't really understand what is wrong with virtio_add_sgs + sg_set_page.
I don't think is assumes a kaddr.
> ---
> drivers/virtio/virtio_ring.c | 94 +++++++++++++++++++++++++++++++++++---------
> include/linux/virtio.h | 6 +++
> 2 files changed, 81 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index eb30f3e..0b87123 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -257,6 +257,79 @@ static struct vring_desc *alloc_indirect(struct virtqueue *_vq,
> return desc;
> }
>
> +static void vring_set_avail(struct virtqueue *_vq,
> + unsigned int i)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + unsigned int avail;
> +
> + avail = vq->avail_idx_shadow & (vq->vring.num - 1);
> + vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, i);
> +
> + /*
> + * Descriptors and available array need to be set before we expose the
> + * new available array entries.
> + */
> + virtio_wmb(vq->weak_barriers);
> + vq->avail_idx_shadow++;
> + vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev,
> + vq->avail_idx_shadow);
> + vq->num_added++;
> +
> + pr_debug("Added buffer head %i to %p\n", i, vq);
> +
> + /*
> + * This is very unlikely, but theoretically possible. Kick
> + * just in case.
> + */
> + if (unlikely(vq->num_added == (1 << 16) - 1))
> + virtqueue_kick(_vq);
> +}
> +
> +int virtqueue_add_one_desc(struct virtqueue *_vq,
> + uint64_t addr,
> + uint32_t len,
> + bool in_desc,
> + void *data)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + struct vring_desc *desc;
> + unsigned int i;
> +
> + START_USE(vq);
> + BUG_ON(data == NULL);
> +
> + if (unlikely(vq->broken)) {
> + END_USE(vq);
> + return -EIO;
> + }
> +
> + if (_vq->num_free < 1) {
> + END_USE(vq);
> + return -ENOSPC;
> + }
> +
> + i = vq->free_head;
> + desc = &vq->vring.desc[i];
> + desc->addr = cpu_to_virtio64(_vq->vdev, addr);
> + desc->len = cpu_to_virtio32(_vq->vdev, len);
> + if (in_desc)
> + desc->flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_WRITE);
> + else
> + desc->flags = 0;
> + vq->desc_state[i].data = data;
> + vq->desc_state[i].indir_desc = NULL;
> + vq->free_head = virtio16_to_cpu(_vq->vdev, vq->vring.desc[i].next);
> + _vq->num_free--;
> +
> + vring_set_avail(_vq, i);
> +
> + END_USE(vq);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_add_one_desc);
> +
> static inline int virtqueue_add(struct virtqueue *_vq,
> struct scatterlist *sgs[],
> unsigned int total_sg,
> @@ -269,7 +342,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> struct vring_virtqueue *vq = to_vvq(_vq);
> struct scatterlist *sg;
> struct vring_desc *desc;
> - unsigned int i, n, avail, descs_used, uninitialized_var(prev), err_idx;
> + unsigned int i, n, descs_used, uninitialized_var(prev), err_idx;
> int head;
> bool indirect;
>
> @@ -395,26 +468,9 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> else
> vq->desc_state[head].indir_desc = ctx;
>
> - /* Put entry in available array (but don't update avail->idx until they
> - * do sync). */
> - avail = vq->avail_idx_shadow & (vq->vring.num - 1);
> - vq->vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head);
> -
> - /* Descriptors and available array need to be set before we expose the
> - * new available array entries. */
> - virtio_wmb(vq->weak_barriers);
> - vq->avail_idx_shadow++;
> - vq->vring.avail->idx = cpu_to_virtio16(_vq->vdev, vq->avail_idx_shadow);
> - vq->num_added++;
> -
> - pr_debug("Added buffer head %i to %p\n", head, vq);
> + vring_set_avail(_vq, head);
> END_USE(vq);
>
> - /* This is very unlikely, but theoretically possible. Kick
> - * just in case. */
> - if (unlikely(vq->num_added == (1 << 16) - 1))
> - virtqueue_kick(_vq);
> -
> return 0;
>
> unmap_release:
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 988c735..1d89996 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -35,6 +35,12 @@ struct virtqueue {
> void *priv;
> };
>
> +int virtqueue_add_one_desc(struct virtqueue *_vq,
> + uint64_t addr,
> + uint32_t len,
> + bool in_desc,
> + void *data);
> +
> int virtqueue_add_outbuf(struct virtqueue *vq,
> struct scatterlist sg[], unsigned int num,
> void *data,
> --
> 2.7.4
