Re: [PATCH v18 06/10] virtio_ring: add a new API, virtqueue_add_one_desc
On 12/01/2017 03:38 AM, Michael S. Tsirkin wrote: 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 WangCc: Michael S. Tsirkin 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. OK, I will use the previous method to send sgs. Please have a check if there are other things need to be improved. Best, Wei
Re: [PATCH v18 06/10] virtio_ring: add a new API, virtqueue_add_one_desc
On 12/01/2017 03:38 AM, Michael S. Tsirkin wrote: 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 Cc: Michael S. Tsirkin 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. OK, I will use the previous method to send sgs. Please have a check if there are other things need to be improved. Best, Wei
Re: [PATCH v18 06/10] virtio_ring: add a new API, virtqueue_add_one_desc
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> Cc: Michael S. Tsirkin 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 = >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,
Re: [PATCH v18 06/10] virtio_ring: add a new API, virtqueue_add_one_desc
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 > Cc: Michael S. Tsirkin 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 = >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++; > - > -