On Tue, Jan 16, 2024 at 3:59 PM Xuan Zhuo <xuanz...@linux.alibaba.com> wrote: > > introduce virtqueue_get_buf_ctx_dma() to collect the dma info when > get buf from virtio core for premapped mode. > > If the virtio queue is premapped mode, the virtio-net send buf may > have many desc.
This feature is not specific to virtio-net, so we can let "for example ..." > Every desc dma address need to be unmap. So here we > introduce a new helper to collect the dma address of the buffer from > the virtio core. Let's explain why we can't (or suboptimal to) depend on a driver to do this. > > Because the BAD_RING is called (that may set vq->broken), so > the relative "const" of vq is removed. > > Signed-off-by: Xuan Zhuo <xuanz...@linux.alibaba.com> > --- > drivers/virtio/virtio_ring.c | 174 +++++++++++++++++++++++++---------- > include/linux/virtio.h | 16 ++++ > 2 files changed, 142 insertions(+), 48 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 49299b1f9ec7..82f72428605b 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -362,6 +362,45 @@ static struct device *vring_dma_dev(const struct > vring_virtqueue *vq) > return vq->dma_dev; > } > > +/* > + * use_dma_api premapped -> do_unmap > + * 1. false false false > + * 2. true false true > + * 3. true true false > + * > + * Only #3, we should return the DMA info to the driver. So the code has a check for dma: if (!dma) return false; For whatever the case, we need a better comment here. So we had: use_dma_api, premapped, do_unmap and dma now. I must say I'm totally lost in this maze. It's a strong hint the API is too complicated and needs to be tweaked. For example, is it legal to have do_unmap be false buf DMA is true? Here're suggestions: 1) rename premapped to buffer_is_premapped 2) rename do_unmap to buffer_need_unmap or introduce a helper bool vring_need_unmap_buffer() { return use_dma_api && !premapped; } 3) split the getting dma info logic into an helper like vritqueue_get_dma_info() so we can do if (!vring_need_unmap_buffer()) { virtqueue_get_dma_info() return; } 4) explain why we still need to check dma assuming we had vring_need_unmap_buffer(): If vring_need_unmap_buffer() is true, we don't need to care about dma at all. If vring_need_unmap_buffer() is false, we must return dma info otherwise there's a leak? > + * > + * Return: > + * true: the virtio core must unmap the desc > + * false: the virtio core skip the desc unmap Might it be better to say "It's up to the driver to unmap"? > + */ > +static bool vring_need_unmap(struct vring_virtqueue *vq, > + struct virtio_dma_head *dma, > + dma_addr_t addr, unsigned int length) > +{ > + if (vq->do_unmap) > + return true; > + > + if (!vq->premapped) > + return false; > + > + if (!dma) > + return false; So the logic here is odd. if (!dma) return false; ... return false; A strong hint to split the below getting info logic into another helper. The root cause is the function do more than just a "yes or no". > + > + if (unlikely(dma->next >= dma->num)) { > + BAD_RING(vq, "premapped vq: collect dma overflow: %pad %u\n", > + &addr, length); > + return false; > + } > + > + dma->items[dma->next].addr = addr; > + dma->items[dma->next].length = length; > + > + ++dma->next; > + > + return false; > +} > + Thanks ....