Re: [PATCH RFC 2/2] virtio_ring: support packed ring
On Fri, Mar 16, 2018 at 04:30:02PM +0200, Michael S. Tsirkin wrote: > On Fri, Mar 16, 2018 at 07:36:47PM +0800, Jason Wang wrote: > > > > @@ -1096,17 +1599,21 @@ struct virtqueue *vring_create_virtqueue( > > > > > > > > > if (!queue) { > > > > > > > > > /* Try to get a single page. You are my only > > > > > > > > > hope! */ > > > > > > > > > - queue = vring_alloc_queue(vdev, vring_size(num, > > > > > > > > > vring_align), > > > > > > > > > + queue = vring_alloc_queue(vdev, > > > > > > > > > __vring_size(num, vring_align, > > > > > > > > > + > > > > > > > > > packed), > > > > > > > > > &dma_addr, > > > > > > > > > GFP_KERNEL|__GFP_ZERO); > > > > > > > > > } > > > > > > > > > if (!queue) > > > > > > > > > return NULL; > > > > > > > > > - queue_size_in_bytes = vring_size(num, vring_align); > > > > > > > > > - vring_init(&vring, num, queue, vring_align); > > > > > > > > > + queue_size_in_bytes = __vring_size(num, vring_align, > > > > > > > > > packed); > > > > > > > > > + if (packed) > > > > > > > > > + vring_packed_init(&vring.vring_packed, num, > > > > > > > > > queue, vring_align); > > > > > > > > > + else > > > > > > > > > + vring_init(&vring.vring_split, num, queue, > > > > > > > > > vring_align); > > > > > > > > Let's rename vring_init to vring_init_split() like other > > > > > > > > helpers? > > > > > > > The vring_init() is a public API in > > > > > > > include/uapi/linux/virtio_ring.h. > > > > > > > I don't think we can rename it. > > > > > > I see, then this need more thoughts to unify the API. > > > > > My thought is to keep the old API as is, and introduce > > > > > new types and helpers for packed ring. > > > > I admit it's not a fault of this patch. But we'd better think of this > > > > in the > > > > future, consider we may have new kinds of ring. > > > > > > > > > More details can be found in this patch: > > > > > https://lkml.org/lkml/2018/2/23/243 > > > > > (PS. The type which has bit fields is just for reference, > > > > >and will be changed in next version.) > > > > > > > > > > Do you have any other suggestions? > > > > No. > > > Hmm.. Sorry, I didn't describe my question well. > > > I mean do you have any suggestions about the API > > > design for packed ring in uapi header? Currently > > > I introduced below two new helpers: > > > > > > static inline void vring_packed_init(struct vring_packed *vr, unsigned > > > int num, > > >void *p, unsigned long align); > > > static inline unsigned vring_packed_size(unsigned int num, unsigned long > > > align); > > > > > > When new rings are introduced in the future, above > > > helpers can't be reused. Maybe we should make the > > > helpers be able to determine the ring type? > > > > Let's wait for Michael's comment here. Generally, I fail to understand why > > vring_init() become a part of uapi. Git grep shows the only use cases are > > virtio_test/vringh_test. > > > > Thanks > > For init - I think it's a mistake that stems from lguest which sometimes > made it less than obvious which code is where. I don't see a reason to > add to it. Got it! I'll move vring_packed_init() out of uapi. Many thanks! :) Best regards, Tiwei Bie > > -- > MST
Re: [PATCH RFC 2/2] virtio_ring: support packed ring
On Fri, Mar 16, 2018 at 07:36:47PM +0800, Jason Wang wrote: > On 2018年03月16日 18:04, Tiwei Bie wrote: > > On Fri, Mar 16, 2018 at 04:34:28PM +0800, Jason Wang wrote: > > > On 2018年03月16日 15:40, Tiwei Bie wrote: > > > > On Fri, Mar 16, 2018 at 02:44:12PM +0800, Jason Wang wrote: > > > > > On 2018年03月16日 14:10, Tiwei Bie wrote: > > > > > > On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote: > > > > > > > On 2018年02月23日 19:18, Tiwei Bie wrote: > > > > > > > > Signed-off-by: Tiwei Bie > > > > > > > > --- > > > > > > > > drivers/virtio/virtio_ring.c | 699 > > > > > > > > +-- > > > > > > > > include/linux/virtio_ring.h | 8 +- > > > > > > > > 2 files changed, 618 insertions(+), 89 deletions(-) [...] > > > @@ -1096,17 +1599,21 @@ struct virtqueue *vring_create_virtqueue( > > > > > > > > if (!queue) { > > > > > > > > /* Try to get a single page. You are my only > > > > > > > > hope! */ > > > > > > > > - queue = vring_alloc_queue(vdev, vring_size(num, > > > > > > > > vring_align), > > > > > > > > + queue = vring_alloc_queue(vdev, > > > > > > > > __vring_size(num, vring_align, > > > > > > > > + > > > > > > > > packed), > > > > > > > > &dma_addr, > > > > > > > > GFP_KERNEL|__GFP_ZERO); > > > > > > > > } > > > > > > > > if (!queue) > > > > > > > > return NULL; > > > > > > > > - queue_size_in_bytes = vring_size(num, vring_align); > > > > > > > > - vring_init(&vring, num, queue, vring_align); > > > > > > > > + queue_size_in_bytes = __vring_size(num, vring_align, > > > > > > > > packed); > > > > > > > > + if (packed) > > > > > > > > + vring_packed_init(&vring.vring_packed, num, > > > > > > > > queue, vring_align); > > > > > > > > + else > > > > > > > > + vring_init(&vring.vring_split, num, queue, > > > > > > > > vring_align); > > > > > > > Let's rename vring_init to vring_init_split() like other helpers? > > > > > > The vring_init() is a public API in > > > > > > include/uapi/linux/virtio_ring.h. > > > > > > I don't think we can rename it. > > > > > I see, then this need more thoughts to unify the API. > > > > My thought is to keep the old API as is, and introduce > > > > new types and helpers for packed ring. > > > I admit it's not a fault of this patch. But we'd better think of this in > > > the > > > future, consider we may have new kinds of ring. > > > > > > > More details can be found in this patch: > > > > https://lkml.org/lkml/2018/2/23/243 > > > > (PS. The type which has bit fields is just for reference, > > > >and will be changed in next version.) > > > > > > > > Do you have any other suggestions? > > > No. > > Hmm.. Sorry, I didn't describe my question well. > > I mean do you have any suggestions about the API > > design for packed ring in uapi header? Currently > > I introduced below two new helpers: > > > > static inline void vring_packed_init(struct vring_packed *vr, unsigned int > > num, > > void *p, unsigned long align); > > static inline unsigned vring_packed_size(unsigned int num, unsigned long > > align); > > > > When new rings are introduced in the future, above > > helpers can't be reused. Maybe we should make the > > helpers be able to determine the ring type? > > Let's wait for Michael's comment here. Generally, I fail to understand why > vring_init() become a part of uapi. Git grep shows the only use cases are > virtio_test/vringh_test. Thank you very much for the review on this patch! I'll send out a new version ASAP to address these comments. :) Best regards, Tiwei Bie
Re: [PATCH RFC 2/2] virtio_ring: support packed ring
On Fri, Mar 16, 2018 at 07:36:47PM +0800, Jason Wang wrote: > > > @@ -1096,17 +1599,21 @@ struct virtqueue *vring_create_virtqueue( > > > > > > > > if (!queue) { > > > > > > > > /* Try to get a single page. You are my only > > > > > > > > hope! */ > > > > > > > > - queue = vring_alloc_queue(vdev, vring_size(num, > > > > > > > > vring_align), > > > > > > > > + queue = vring_alloc_queue(vdev, > > > > > > > > __vring_size(num, vring_align, > > > > > > > > + > > > > > > > > packed), > > > > > > > > &dma_addr, > > > > > > > > GFP_KERNEL|__GFP_ZERO); > > > > > > > > } > > > > > > > > if (!queue) > > > > > > > > return NULL; > > > > > > > > - queue_size_in_bytes = vring_size(num, vring_align); > > > > > > > > - vring_init(&vring, num, queue, vring_align); > > > > > > > > + queue_size_in_bytes = __vring_size(num, vring_align, > > > > > > > > packed); > > > > > > > > + if (packed) > > > > > > > > + vring_packed_init(&vring.vring_packed, num, > > > > > > > > queue, vring_align); > > > > > > > > + else > > > > > > > > + vring_init(&vring.vring_split, num, queue, > > > > > > > > vring_align); > > > > > > > Let's rename vring_init to vring_init_split() like other helpers? > > > > > > The vring_init() is a public API in > > > > > > include/uapi/linux/virtio_ring.h. > > > > > > I don't think we can rename it. > > > > > I see, then this need more thoughts to unify the API. > > > > My thought is to keep the old API as is, and introduce > > > > new types and helpers for packed ring. > > > I admit it's not a fault of this patch. But we'd better think of this in > > > the > > > future, consider we may have new kinds of ring. > > > > > > > More details can be found in this patch: > > > > https://lkml.org/lkml/2018/2/23/243 > > > > (PS. The type which has bit fields is just for reference, > > > >and will be changed in next version.) > > > > > > > > Do you have any other suggestions? > > > No. > > Hmm.. Sorry, I didn't describe my question well. > > I mean do you have any suggestions about the API > > design for packed ring in uapi header? Currently > > I introduced below two new helpers: > > > > static inline void vring_packed_init(struct vring_packed *vr, unsigned int > > num, > > void *p, unsigned long align); > > static inline unsigned vring_packed_size(unsigned int num, unsigned long > > align); > > > > When new rings are introduced in the future, above > > helpers can't be reused. Maybe we should make the > > helpers be able to determine the ring type? > > Let's wait for Michael's comment here. Generally, I fail to understand why > vring_init() become a part of uapi. Git grep shows the only use cases are > virtio_test/vringh_test. > > Thanks For init - I think it's a mistake that stems from lguest which sometimes made it less than obvious which code is where. I don't see a reason to add to it. -- MST
Re: [PATCH RFC 2/2] virtio_ring: support packed ring
On 2018年03月16日 18:04, Tiwei Bie wrote: On Fri, Mar 16, 2018 at 04:34:28PM +0800, Jason Wang wrote: On 2018年03月16日 15:40, Tiwei Bie wrote: On Fri, Mar 16, 2018 at 02:44:12PM +0800, Jason Wang wrote: On 2018年03月16日 14:10, Tiwei Bie wrote: On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote: On 2018年02月23日 19:18, Tiwei Bie wrote: Signed-off-by: Tiwei Bie --- drivers/virtio/virtio_ring.c | 699 +-- include/linux/virtio_ring.h | 8 +- 2 files changed, 618 insertions(+), 89 deletions(-) [...] + } + } + 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; + + flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT | + VRING_DESC_F_WRITE | + VRING_DESC_F_AVAIL(vq->wrap_counter) | + VRING_DESC_F_USED(!vq->wrap_counter)); + if (!indirect && i == head) + head_flags = flags; + else + desc[i].flags = flags; + + desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); + desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length); + desc[i].id = cpu_to_virtio32(_vq->vdev, head); + prev = i; + i++; + if (!indirect && i >= vq->vring_packed.num) { + i = 0; + vq->wrap_counter ^= 1; + } + } + } + /* Last one doesn't continue. */ + if (!indirect && (head + 1) % vq->vring_packed.num == i) + head_flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT); I can't get the why we need this here. If only one desc is used, we will need to clear the VRING_DESC_F_NEXT flag from the head_flags. Yes, I meant why following desc[prev].flags won't work for this? Because the update of desc[head].flags (in above case, prev == head) has been delayed. The flags is saved in head_flags. Ok, but let's try to avoid modular here e.g tracking the number of sgs in a counter. And I see lots of duplication in the above two loops, I believe we can unify them with a a single loop. the only difference is dma direction and write flag. The above implementation for packed ring is basically an mirror of the existing implementation in split ring as I want to keep the coding style consistent. Below is the corresponding code in split ring: static inline int virtqueue_add(struct virtqueue *_vq, struct scatterlist *sgs[], unsigned int total_sg, unsigned int out_sgs, unsigned int in_sgs, void *data, void *ctx, gfp_t gfp) { .. 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; desc[i].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT); desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length); prev = i; i = virtio16_to_cpu(_vq->vdev, desc[i].next); } } 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; desc[i].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT | VRING_DESC_F_WRITE); desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length); prev = i; i = virtio16_to_cpu(_vq->vdev, desc[i].next); } } .. } There's no need for such consistency especially consider it's a new kind of ring. Anyway, you can stick to it. [...] + } else + vq->free_head = i; ID is only valid in the last descriptor in the list, so head + 1 should be ok too? I don't really get your point. The vq->free_head stores the index of the next avail de
Re: [PATCH RFC 2/2] virtio_ring: support packed ring
On Fri, Mar 16, 2018 at 04:34:28PM +0800, Jason Wang wrote: > On 2018年03月16日 15:40, Tiwei Bie wrote: > > On Fri, Mar 16, 2018 at 02:44:12PM +0800, Jason Wang wrote: > > > On 2018年03月16日 14:10, Tiwei Bie wrote: > > > > On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote: > > > > > On 2018年02月23日 19:18, Tiwei Bie wrote: > > > > > > Signed-off-by: Tiwei Bie > > > > > > --- > > > > > > drivers/virtio/virtio_ring.c | 699 > > > > > > +-- > > > > > > include/linux/virtio_ring.h | 8 +- > > > > > > 2 files changed, 618 insertions(+), 89 deletions(-) > > [...] > > > > > > cpu_addr, size, direction); > > > > > > } > > > > > > -static void vring_unmap_one(const struct vring_virtqueue *vq, > > > > > > - struct vring_desc *desc) > > > > > > +static void vring_unmap_one(const struct vring_virtqueue *vq, void > > > > > > *_desc) > > > > > > { > > > > > Let's split the helpers to packed/split version like other helpers? > > > > > (Consider the caller has already known the type of vq). > > > > Okay. > > > > > > > [...] > > > > > > > > > + desc[i].flags = flags; > > > > > > + > > > > > > + desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); > > > > > > + desc[i].len = cpu_to_virtio32(_vq->vdev, > > > > > > sg->length); > > > > > > + desc[i].id = cpu_to_virtio32(_vq->vdev, head); > > > > > If it's a part of chain, we only need to do this for last buffer I > > > > > think. > > > > I'm not sure I've got your point about the "last buffer". > > > > But, yes, id just needs to be set for the last desc. > > > Right, I think I meant "last descriptor" :) > > > > > > > > > + prev = i; > > > > > > + i++; > > > > > It looks to me prev is always i - 1? > > > > No. prev will be (vq->vring_packed.num - 1) when i becomes 0. > > > Right, so prev = i ? i - 1 : vq->vring_packed.num - 1. > > Yes, i wraps together with vq->wrap_counter in following code: > > > > > > > > + if (!indirect && i >= vq->vring_packed.num) { > > > > > > + i = 0; > > > > > > + vq->wrap_counter ^= 1; > > > > > > + } > > > > > > > > + } > > > > > > + } > > > > > > + 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; > > > > > > + > > > > > > + flags = cpu_to_virtio16(_vq->vdev, > > > > > > VRING_DESC_F_NEXT | > > > > > > + VRING_DESC_F_WRITE | > > > > > > + > > > > > > VRING_DESC_F_AVAIL(vq->wrap_counter) | > > > > > > + > > > > > > VRING_DESC_F_USED(!vq->wrap_counter)); > > > > > > + if (!indirect && i == head) > > > > > > + head_flags = flags; > > > > > > + else > > > > > > + desc[i].flags = flags; > > > > > > + > > > > > > + desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); > > > > > > + desc[i].len = cpu_to_virtio32(_vq->vdev, > > > > > > sg->length); > > > > > > + desc[i].id = cpu_to_virtio32(_vq->vdev, head); > > > > > > + prev = i; > > > > > > + i++; > > > > > > + if (!indirect && i >= vq->vring_packed.num) { > > > > > > + i = 0; > > > > > > + vq->wrap_counter ^= 1; > > > > > > + } > > > > > > + } > > > > > > + } > > > > > > + /* Last one doesn't continue. */ > > > > > > + if (!indirect && (head + 1) % vq->vring_packed.num == i) > > > > > > + head_flags &= cpu_to_virtio16(_vq->vdev, > > > > > > ~VRING_DESC_F_NEXT); > > > > > I can't get the why we need this here. > > > > If only one desc is used, we will need to clear the > > > > VRING_DESC_F_NEXT flag from the head_flags. > > > Yes, I meant why following desc[prev].flags won't work for this? > > Because the update of desc[head].flags (in above case, > > prev == head) has been delayed. The flags is saved in > > head_flags. > > Ok, but let's try to avoid modular here e.g tracking the number of sgs in a > counter. > > And I see lots of duplication in the above two loops, I believe we can unify > them with a a single loop. the only difference is dma direction and write > flag. The above implementation for packed ring is basically an mirror of the existing implementation in split ring as I want to keep the coding style consistent. Below is the corresponding code in split ring: static
Re: [PATCH RFC 2/2] virtio_ring: support packed ring
On 2018年03月16日 15:40, Tiwei Bie wrote: On Fri, Mar 16, 2018 at 02:44:12PM +0800, Jason Wang wrote: On 2018年03月16日 14:10, Tiwei Bie wrote: On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote: On 2018年02月23日 19:18, Tiwei Bie wrote: Signed-off-by: Tiwei Bie --- drivers/virtio/virtio_ring.c | 699 +-- include/linux/virtio_ring.h | 8 +- 2 files changed, 618 insertions(+), 89 deletions(-) [...] cpu_addr, size, direction); } -static void vring_unmap_one(const struct vring_virtqueue *vq, - struct vring_desc *desc) +static void vring_unmap_one(const struct vring_virtqueue *vq, void *_desc) { Let's split the helpers to packed/split version like other helpers? (Consider the caller has already known the type of vq). Okay. [...] + desc[i].flags = flags; + + desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); + desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length); + desc[i].id = cpu_to_virtio32(_vq->vdev, head); If it's a part of chain, we only need to do this for last buffer I think. I'm not sure I've got your point about the "last buffer". But, yes, id just needs to be set for the last desc. Right, I think I meant "last descriptor" :) + prev = i; + i++; It looks to me prev is always i - 1? No. prev will be (vq->vring_packed.num - 1) when i becomes 0. Right, so prev = i ? i - 1 : vq->vring_packed.num - 1. Yes, i wraps together with vq->wrap_counter in following code: + if (!indirect && i >= vq->vring_packed.num) { + i = 0; + vq->wrap_counter ^= 1; + } + } + } + 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; + + flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT | + VRING_DESC_F_WRITE | + VRING_DESC_F_AVAIL(vq->wrap_counter) | + VRING_DESC_F_USED(!vq->wrap_counter)); + if (!indirect && i == head) + head_flags = flags; + else + desc[i].flags = flags; + + desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); + desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length); + desc[i].id = cpu_to_virtio32(_vq->vdev, head); + prev = i; + i++; + if (!indirect && i >= vq->vring_packed.num) { + i = 0; + vq->wrap_counter ^= 1; + } + } + } + /* Last one doesn't continue. */ + if (!indirect && (head + 1) % vq->vring_packed.num == i) + head_flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT); I can't get the why we need this here. If only one desc is used, we will need to clear the VRING_DESC_F_NEXT flag from the head_flags. Yes, I meant why following desc[prev].flags won't work for this? Because the update of desc[head].flags (in above case, prev == head) has been delayed. The flags is saved in head_flags. Ok, but let's try to avoid modular here e.g tracking the number of sgs in a counter. And I see lots of duplication in the above two loops, I believe we can unify them with a a single loop. the only difference is dma direction and write flag. + else + desc[prev].flags &= cpu_to_virtio16(_vq->vdev, ~VRING_DESC_F_NEXT); + + if (indirect) { + /* FIXME: to be implemented */ + + /* Now that the indirect table is filled in, map it. */ + dma_addr_t addr = vring_map_single( + vq, desc, total_sg * sizeof(struct vring_packed_desc), + DMA_TO_DEVICE); + if (vring_mapping_error(vq, addr)) + goto unmap_release; + + head_flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_INDIRECT | +VRING_DESC_F_AVAIL(wrap_counter) | +VRING_DESC_F_USED(!wrap_counter)); + vq->vring_packed.desc[head].addr = cpu_to_virtio64(_vq->vdev, addr); + vq->vring_packed.desc[head].len = cpu_to_virtio32(_vq->vdev, + total_sg * sizeof(struct vring_packed_desc)); + vq->vring_packed.desc[head].id = cpu_to_vir
Re: [PATCH RFC 2/2] virtio_ring: support packed ring
On Fri, Mar 16, 2018 at 02:44:12PM +0800, Jason Wang wrote: > On 2018年03月16日 14:10, Tiwei Bie wrote: > > On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote: > > > On 2018年02月23日 19:18, Tiwei Bie wrote: > > > > Signed-off-by: Tiwei Bie > > > > --- > > > >drivers/virtio/virtio_ring.c | 699 > > > > +-- > > > >include/linux/virtio_ring.h | 8 +- > > > >2 files changed, 618 insertions(+), 89 deletions(-) [...] > > > > cpu_addr, size, direction); > > > >} > > > > -static void vring_unmap_one(const struct vring_virtqueue *vq, > > > > - struct vring_desc *desc) > > > > +static void vring_unmap_one(const struct vring_virtqueue *vq, void > > > > *_desc) > > > >{ > > > Let's split the helpers to packed/split version like other helpers? > > > (Consider the caller has already known the type of vq). > > Okay. > > > > [...] > > > > > + desc[i].flags = flags; > > > > + > > > > + desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); > > > > + desc[i].len = cpu_to_virtio32(_vq->vdev, > > > > sg->length); > > > > + desc[i].id = cpu_to_virtio32(_vq->vdev, head); > > > If it's a part of chain, we only need to do this for last buffer I think. > > I'm not sure I've got your point about the "last buffer". > > But, yes, id just needs to be set for the last desc. > > Right, I think I meant "last descriptor" :) > > > > > > > + prev = i; > > > > + i++; > > > It looks to me prev is always i - 1? > > No. prev will be (vq->vring_packed.num - 1) when i becomes 0. > > Right, so prev = i ? i - 1 : vq->vring_packed.num - 1. Yes, i wraps together with vq->wrap_counter in following code: > > > > + if (!indirect && i >= vq->vring_packed.num) { > > > > + i = 0; > > > > + vq->wrap_counter ^= 1; > > > > + } > > > > + } > > > > + } > > > > + 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; > > > > + > > > > + flags = cpu_to_virtio16(_vq->vdev, > > > > VRING_DESC_F_NEXT | > > > > + VRING_DESC_F_WRITE | > > > > + > > > > VRING_DESC_F_AVAIL(vq->wrap_counter) | > > > > + > > > > VRING_DESC_F_USED(!vq->wrap_counter)); > > > > + if (!indirect && i == head) > > > > + head_flags = flags; > > > > + else > > > > + desc[i].flags = flags; > > > > + > > > > + desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); > > > > + desc[i].len = cpu_to_virtio32(_vq->vdev, > > > > sg->length); > > > > + desc[i].id = cpu_to_virtio32(_vq->vdev, head); > > > > + prev = i; > > > > + i++; > > > > + if (!indirect && i >= vq->vring_packed.num) { > > > > + i = 0; > > > > + vq->wrap_counter ^= 1; > > > > + } > > > > + } > > > > + } > > > > + /* Last one doesn't continue. */ > > > > + if (!indirect && (head + 1) % vq->vring_packed.num == i) > > > > + head_flags &= cpu_to_virtio16(_vq->vdev, > > > > ~VRING_DESC_F_NEXT); > > > I can't get the why we need this here. > > If only one desc is used, we will need to clear the > > VRING_DESC_F_NEXT flag from the head_flags. > > Yes, I meant why following desc[prev].flags won't work for this? Because the update of desc[head].flags (in above case, prev == head) has been delayed. The flags is saved in head_flags. > > > > > > > + else > > > > + desc[prev].flags &= cpu_to_virtio16(_vq->vdev, > > > > ~VRING_DESC_F_NEXT); > > > > + > > > > + if (indirect) { > > > > + /* FIXME: to be implemented */ > > > > + > > > > + /* Now that the indirect table is filled in, map it. */ > > > > + dma_addr_t addr = vring_map_single( > > > > + vq, desc, total_sg * sizeof(struct > > > > vring_packed_desc), > > > > + DMA_TO_DEVICE); > > > > + if (vring_mapping_error(vq, addr)) > > > > + goto unmap_release; > > > > + > > > > + head_flags = cpu_to_virtio16(_vq->vdev, > > > > VRING_DESC_F_INDIRECT | > > > > +
Re: [PATCH RFC 2/2] virtio_ring: support packed ring
On 2018年03月16日 14:10, Tiwei Bie wrote: On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote: On 2018年02月23日 19:18, Tiwei Bie wrote: Signed-off-by: Tiwei Bie --- drivers/virtio/virtio_ring.c | 699 +-- include/linux/virtio_ring.h | 8 +- 2 files changed, 618 insertions(+), 89 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index eb30f3e09a47..393778a2f809 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -58,14 +58,14 @@ struct vring_desc_state { void *data; /* Data for callback. */ - struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ + void *indir_desc; /* Indirect descriptor, if any. */ + int num;/* Descriptor list length. */ }; struct vring_virtqueue { struct virtqueue vq; - /* Actual memory layout for this queue */ - struct vring vring; + bool packed; /* Can we use weak barriers? */ bool weak_barriers; @@ -87,11 +87,28 @@ struct vring_virtqueue { /* Last used index we've seen. */ u16 last_used_idx; - /* Last written value to avail->flags */ - u16 avail_flags_shadow; - - /* Last written value to avail->idx in guest byte order */ - u16 avail_idx_shadow; + union { + /* Available for split ring */ + struct { + /* Actual memory layout for this queue */ + struct vring vring; + + /* Last written value to avail->flags */ + u16 avail_flags_shadow; + + /* Last written value to avail->idx in +* guest byte order */ + u16 avail_idx_shadow; + }; + + /* Available for packed ring */ + struct { + /* Actual memory layout for this queue */ + struct vring_packed vring_packed; + u8 wrap_counter : 1; + bool chaining; + }; + }; /* How to notify other side. FIXME: commonalize hcalls! */ bool (*notify)(struct virtqueue *vq); @@ -201,26 +218,37 @@ static dma_addr_t vring_map_single(const struct vring_virtqueue *vq, cpu_addr, size, direction); } -static void vring_unmap_one(const struct vring_virtqueue *vq, - struct vring_desc *desc) +static void vring_unmap_one(const struct vring_virtqueue *vq, void *_desc) { Let's split the helpers to packed/split version like other helpers? (Consider the caller has already known the type of vq). Okay. [...] + desc[i].flags = flags; + + desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); + desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length); + desc[i].id = cpu_to_virtio32(_vq->vdev, head); If it's a part of chain, we only need to do this for last buffer I think. I'm not sure I've got your point about the "last buffer". But, yes, id just needs to be set for the last desc. Right, I think I meant "last descriptor" :) + prev = i; + i++; It looks to me prev is always i - 1? No. prev will be (vq->vring_packed.num - 1) when i becomes 0. Right, so prev = i ? i - 1 : vq->vring_packed.num - 1. + if (!indirect && i >= vq->vring_packed.num) { + i = 0; + vq->wrap_counter ^= 1; + } + } + } + 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; + + flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT | + VRING_DESC_F_WRITE | + VRING_DESC_F_AVAIL(vq->wrap_counter) | + VRING_DESC_F_USED(!vq->wrap_counter)); + if (!indirect && i == head) + head_flags = flags; + else + desc[i].flags = flags; + + desc[i].addr = cpu_to_virtio64(_vq->vdev, addr); + desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length); + desc[i].id = cpu_to_virtio32(_vq->vdev, head); + prev = i; + i++; + if (!indirect && i >= vq->vring_packed.num) { + i = 0; + vq->wrap_counter
Re: [PATCH RFC 2/2] virtio_ring: support packed ring
On Fri, Mar 16, 2018 at 12:03:25PM +0800, Jason Wang wrote: > On 2018年02月23日 19:18, Tiwei Bie wrote: > > Signed-off-by: Tiwei Bie > > --- > > drivers/virtio/virtio_ring.c | 699 > > +-- > > include/linux/virtio_ring.h | 8 +- > > 2 files changed, 618 insertions(+), 89 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index eb30f3e09a47..393778a2f809 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -58,14 +58,14 @@ > > struct vring_desc_state { > > void *data; /* Data for callback. */ > > - struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ > > + void *indir_desc; /* Indirect descriptor, if any. */ > > + int num;/* Descriptor list length. */ > > }; > > struct vring_virtqueue { > > struct virtqueue vq; > > - /* Actual memory layout for this queue */ > > - struct vring vring; > > + bool packed; > > /* Can we use weak barriers? */ > > bool weak_barriers; > > @@ -87,11 +87,28 @@ struct vring_virtqueue { > > /* Last used index we've seen. */ > > u16 last_used_idx; > > - /* Last written value to avail->flags */ > > - u16 avail_flags_shadow; > > - > > - /* Last written value to avail->idx in guest byte order */ > > - u16 avail_idx_shadow; > > + union { > > + /* Available for split ring */ > > + struct { > > + /* Actual memory layout for this queue */ > > + struct vring vring; > > + > > + /* Last written value to avail->flags */ > > + u16 avail_flags_shadow; > > + > > + /* Last written value to avail->idx in > > +* guest byte order */ > > + u16 avail_idx_shadow; > > + }; > > + > > + /* Available for packed ring */ > > + struct { > > + /* Actual memory layout for this queue */ > > + struct vring_packed vring_packed; > > + u8 wrap_counter : 1; > > + bool chaining; > > + }; > > + }; > > /* How to notify other side. FIXME: commonalize hcalls! */ > > bool (*notify)(struct virtqueue *vq); > > @@ -201,26 +218,37 @@ static dma_addr_t vring_map_single(const struct > > vring_virtqueue *vq, > > cpu_addr, size, direction); > > } > > -static void vring_unmap_one(const struct vring_virtqueue *vq, > > - struct vring_desc *desc) > > +static void vring_unmap_one(const struct vring_virtqueue *vq, void *_desc) > > { > > Let's split the helpers to packed/split version like other helpers? > (Consider the caller has already known the type of vq). Okay. > > > + u64 addr; > > + u32 len; > > u16 flags; > > if (!vring_use_dma_api(vq->vq.vdev)) > > return; > > - flags = virtio16_to_cpu(vq->vq.vdev, desc->flags); > > + if (vq->packed) { > > + struct vring_packed_desc *desc = _desc; > > + > > + addr = virtio64_to_cpu(vq->vq.vdev, desc->addr); > > + len = virtio32_to_cpu(vq->vq.vdev, desc->len); > > + flags = virtio16_to_cpu(vq->vq.vdev, desc->flags); > > + } else { > > + struct vring_desc *desc = _desc; > > + > > + addr = virtio64_to_cpu(vq->vq.vdev, desc->addr); > > + len = virtio32_to_cpu(vq->vq.vdev, desc->len); > > + flags = virtio16_to_cpu(vq->vq.vdev, desc->flags); > > + } > > if (flags & VRING_DESC_F_INDIRECT) { > > dma_unmap_single(vring_dma_dev(vq), > > -virtio64_to_cpu(vq->vq.vdev, desc->addr), > > -virtio32_to_cpu(vq->vq.vdev, desc->len), > > +addr, len, > > (flags & VRING_DESC_F_WRITE) ? > > DMA_FROM_DEVICE : DMA_TO_DEVICE); > > } else { > > dma_unmap_page(vring_dma_dev(vq), > > - virtio64_to_cpu(vq->vq.vdev, desc->addr), > > - virtio32_to_cpu(vq->vq.vdev, desc->len), > > + addr, len, > >(flags & VRING_DESC_F_WRITE) ? > >DMA_FROM_DEVICE : DMA_TO_DEVICE); > > } > > @@ -235,8 +263,9 @@ static int vring_mapping_error(const struct > > vring_virtqueue *vq, > > return dma_mapping_error(vring_dma_dev(vq), addr); > > } > > -static struct vring_desc *alloc_indirect(struct virtqueue *_vq, > > -unsigned int total_sg, gfp_t gfp) > > +static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq, > > + unsigned int total_sg, > > + gfp_t gfp) > > { > > struct vring_desc *desc; > > unsigned int i; > > @@ -257,14
Re: [PATCH RFC 2/2] virtio_ring: support packed ring
On 2018年02月23日 19:18, Tiwei Bie wrote: Signed-off-by: Tiwei Bie --- drivers/virtio/virtio_ring.c | 699 +-- include/linux/virtio_ring.h | 8 +- 2 files changed, 618 insertions(+), 89 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index eb30f3e09a47..393778a2f809 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -58,14 +58,14 @@ struct vring_desc_state { void *data; /* Data for callback. */ - struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ + void *indir_desc; /* Indirect descriptor, if any. */ + int num;/* Descriptor list length. */ }; struct vring_virtqueue { struct virtqueue vq; - /* Actual memory layout for this queue */ - struct vring vring; + bool packed; /* Can we use weak barriers? */ bool weak_barriers; @@ -87,11 +87,28 @@ struct vring_virtqueue { /* Last used index we've seen. */ u16 last_used_idx; - /* Last written value to avail->flags */ - u16 avail_flags_shadow; - - /* Last written value to avail->idx in guest byte order */ - u16 avail_idx_shadow; + union { + /* Available for split ring */ + struct { + /* Actual memory layout for this queue */ + struct vring vring; + + /* Last written value to avail->flags */ + u16 avail_flags_shadow; + + /* Last written value to avail->idx in +* guest byte order */ + u16 avail_idx_shadow; + }; + + /* Available for packed ring */ + struct { + /* Actual memory layout for this queue */ + struct vring_packed vring_packed; + u8 wrap_counter : 1; + bool chaining; + }; + }; /* How to notify other side. FIXME: commonalize hcalls! */ bool (*notify)(struct virtqueue *vq); @@ -201,26 +218,37 @@ static dma_addr_t vring_map_single(const struct vring_virtqueue *vq, cpu_addr, size, direction); } -static void vring_unmap_one(const struct vring_virtqueue *vq, - struct vring_desc *desc) +static void vring_unmap_one(const struct vring_virtqueue *vq, void *_desc) { Let's split the helpers to packed/split version like other helpers? (Consider the caller has already known the type of vq). + u64 addr; + u32 len; u16 flags; if (!vring_use_dma_api(vq->vq.vdev)) return; - flags = virtio16_to_cpu(vq->vq.vdev, desc->flags); + if (vq->packed) { + struct vring_packed_desc *desc = _desc; + + addr = virtio64_to_cpu(vq->vq.vdev, desc->addr); + len = virtio32_to_cpu(vq->vq.vdev, desc->len); + flags = virtio16_to_cpu(vq->vq.vdev, desc->flags); + } else { + struct vring_desc *desc = _desc; + + addr = virtio64_to_cpu(vq->vq.vdev, desc->addr); + len = virtio32_to_cpu(vq->vq.vdev, desc->len); + flags = virtio16_to_cpu(vq->vq.vdev, desc->flags); + } if (flags & VRING_DESC_F_INDIRECT) { dma_unmap_single(vring_dma_dev(vq), -virtio64_to_cpu(vq->vq.vdev, desc->addr), -virtio32_to_cpu(vq->vq.vdev, desc->len), +addr, len, (flags & VRING_DESC_F_WRITE) ? DMA_FROM_DEVICE : DMA_TO_DEVICE); } else { dma_unmap_page(vring_dma_dev(vq), - virtio64_to_cpu(vq->vq.vdev, desc->addr), - virtio32_to_cpu(vq->vq.vdev, desc->len), + addr, len, (flags & VRING_DESC_F_WRITE) ? DMA_FROM_DEVICE : DMA_TO_DEVICE); } @@ -235,8 +263,9 @@ static int vring_mapping_error(const struct vring_virtqueue *vq, return dma_mapping_error(vring_dma_dev(vq), addr); } -static struct vring_desc *alloc_indirect(struct virtqueue *_vq, -unsigned int total_sg, gfp_t gfp) +static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq, + unsigned int total_sg, + gfp_t gfp) { struct vring_desc *desc; unsigned int i; @@ -257,14 +286,32 @@ static struct vring_desc *alloc_indirect(struct virtqueue *_vq, return desc; } -static inline int virtqueue_add(struct virtqueue *_vq, - struct scat
[PATCH RFC 2/2] virtio_ring: support packed ring
Signed-off-by: Tiwei Bie --- drivers/virtio/virtio_ring.c | 699 +-- include/linux/virtio_ring.h | 8 +- 2 files changed, 618 insertions(+), 89 deletions(-) diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index eb30f3e09a47..393778a2f809 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -58,14 +58,14 @@ struct vring_desc_state { void *data; /* Data for callback. */ - struct vring_desc *indir_desc; /* Indirect descriptor, if any. */ + void *indir_desc; /* Indirect descriptor, if any. */ + int num;/* Descriptor list length. */ }; struct vring_virtqueue { struct virtqueue vq; - /* Actual memory layout for this queue */ - struct vring vring; + bool packed; /* Can we use weak barriers? */ bool weak_barriers; @@ -87,11 +87,28 @@ struct vring_virtqueue { /* Last used index we've seen. */ u16 last_used_idx; - /* Last written value to avail->flags */ - u16 avail_flags_shadow; - - /* Last written value to avail->idx in guest byte order */ - u16 avail_idx_shadow; + union { + /* Available for split ring */ + struct { + /* Actual memory layout for this queue */ + struct vring vring; + + /* Last written value to avail->flags */ + u16 avail_flags_shadow; + + /* Last written value to avail->idx in +* guest byte order */ + u16 avail_idx_shadow; + }; + + /* Available for packed ring */ + struct { + /* Actual memory layout for this queue */ + struct vring_packed vring_packed; + u8 wrap_counter : 1; + bool chaining; + }; + }; /* How to notify other side. FIXME: commonalize hcalls! */ bool (*notify)(struct virtqueue *vq); @@ -201,26 +218,37 @@ static dma_addr_t vring_map_single(const struct vring_virtqueue *vq, cpu_addr, size, direction); } -static void vring_unmap_one(const struct vring_virtqueue *vq, - struct vring_desc *desc) +static void vring_unmap_one(const struct vring_virtqueue *vq, void *_desc) { + u64 addr; + u32 len; u16 flags; if (!vring_use_dma_api(vq->vq.vdev)) return; - flags = virtio16_to_cpu(vq->vq.vdev, desc->flags); + if (vq->packed) { + struct vring_packed_desc *desc = _desc; + + addr = virtio64_to_cpu(vq->vq.vdev, desc->addr); + len = virtio32_to_cpu(vq->vq.vdev, desc->len); + flags = virtio16_to_cpu(vq->vq.vdev, desc->flags); + } else { + struct vring_desc *desc = _desc; + + addr = virtio64_to_cpu(vq->vq.vdev, desc->addr); + len = virtio32_to_cpu(vq->vq.vdev, desc->len); + flags = virtio16_to_cpu(vq->vq.vdev, desc->flags); + } if (flags & VRING_DESC_F_INDIRECT) { dma_unmap_single(vring_dma_dev(vq), -virtio64_to_cpu(vq->vq.vdev, desc->addr), -virtio32_to_cpu(vq->vq.vdev, desc->len), +addr, len, (flags & VRING_DESC_F_WRITE) ? DMA_FROM_DEVICE : DMA_TO_DEVICE); } else { dma_unmap_page(vring_dma_dev(vq), - virtio64_to_cpu(vq->vq.vdev, desc->addr), - virtio32_to_cpu(vq->vq.vdev, desc->len), + addr, len, (flags & VRING_DESC_F_WRITE) ? DMA_FROM_DEVICE : DMA_TO_DEVICE); } @@ -235,8 +263,9 @@ static int vring_mapping_error(const struct vring_virtqueue *vq, return dma_mapping_error(vring_dma_dev(vq), addr); } -static struct vring_desc *alloc_indirect(struct virtqueue *_vq, -unsigned int total_sg, gfp_t gfp) +static struct vring_desc *alloc_indirect_split(struct virtqueue *_vq, + unsigned int total_sg, + gfp_t gfp) { struct vring_desc *desc; unsigned int i; @@ -257,14 +286,32 @@ static struct vring_desc *alloc_indirect(struct virtqueue *_vq, return desc; } -static inline int virtqueue_add(struct virtqueue *_vq, - struct scatterlist *sgs[], - unsigned int total_sg, - unsigned int out_sgs, - unsigned int