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 _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization