Xuan Zhuo
2023-Oct-12 09:18 UTC
[PATCH vhost 01/22] virtio_ring: virtqueue_set_dma_premapped support disable
On Thu, 12 Oct 2023 05:15:52 -0400, "Michael S. Tsirkin" <mst at redhat.com> wrote:> On Wed, Oct 11, 2023 at 05:27:07PM +0800, Xuan Zhuo wrote: > > virtqueue_set_dma_premapped() adds a new parameter to disable the > > virtqueue premapped mode. > > > > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com> > > --- > > drivers/net/virtio_net.c | 2 +- > > drivers/virtio/virtio_ring.c | 11 ++++++++--- > > include/linux/virtio.h | 2 +- > > 3 files changed, 10 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index fe7f314d65c9..6b5f47ebf9b2 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -737,7 +737,7 @@ static void virtnet_rq_set_premapped(struct virtnet_info *vi) > > return; > > > > for (i = 0; i < vi->max_queue_pairs; i++) { > > - if (virtqueue_set_dma_premapped(vi->rq[i].vq)) > > + if (virtqueue_set_dma_premapped(vi->rq[i].vq, true)) > > continue; > > > > vi->rq[i].do_dma = true; > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 51d8f3299c10..b3ded56722f4 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -2784,7 +2784,7 @@ EXPORT_SYMBOL_GPL(virtqueue_resize); > > * 0: success. > > * -EINVAL: vring does not use the dma api, so we can not enable premapped mode. > > */ > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq) > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) > > { > > struct vring_virtqueue *vq = to_vvq(_vq); > > u32 num; > > @@ -2803,8 +2803,13 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq) > > return -EINVAL; > > } > > > > - vq->premapped = true; > > - vq->do_unmap = false; > > + if (mode) { > > + vq->premapped = true; > > + vq->do_unmap = false; > > + } else { > > + vq->premapped = false; > > + vq->do_unmap = vq->use_dma_api; > > + } > > > > END_USE(vq); > > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > > index 4cc614a38376..1cf7b004348b 100644 > > --- a/include/linux/virtio.h > > +++ b/include/linux/virtio.h > > @@ -81,7 +81,7 @@ bool virtqueue_enable_cb(struct virtqueue *vq); > > > > unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq); > > > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq); > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode); > > > > bool virtqueue_poll(struct virtqueue *vq, unsigned); > > Wait a sec I thought we never change premapped. If you make this > dynamic don't you need a bunch of locking? > Or maybe queue is empty when you change this? > If yes pls add a bunch of BUG_ON checks to make sure this is not misused.Actually, this api is called immediately after the vq init or vq reset. We already have such a check. Thanks. /** * virtqueue_set_dma_premapped - set the vring premapped mode * @_vq: the struct virtqueue we're talking about. * * Enable the premapped mode of the vq. * * The vring in premapped mode does not do dma internally, so the driver must * do dma mapping in advance. The driver must pass the dma_address through * dma_address of scatterlist. When the driver got a used buffer from * the vring, it has to unmap the dma address. * * This function must be called immediately after creating the vq, or after vq * reset, and before adding any buffers to it. * * Caller must ensure we don't call this with other virtqueue operations * at the same time (except where noted). * * Returns zero or a negative error. * 0: success. * -EINVAL: vring does not use the dma api, so we can not enable premapped mode. */ int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) { struct vring_virtqueue *vq = to_vvq(_vq); u32 num; START_USE(vq); num = vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num; --> if (num != vq->vq.num_free) { END_USE(vq); return -EINVAL; } if (!vq->use_dma_api) { END_USE(vq); return -EINVAL; } if (mode) { vq->premapped = true; vq->do_unmap = false; } else { vq->premapped = false; vq->do_unmap = vq->use_dma_api; } END_USE(vq); return 0; } EXPORT_SYMBOL_GPL(virtqueue_set_dma_premapped);> > > > -- > > 2.32.0.3.g01195cf9f >
Michael S. Tsirkin
2023-Oct-12 09:40 UTC
[PATCH vhost 01/22] virtio_ring: virtqueue_set_dma_premapped support disable
On Thu, Oct 12, 2023 at 05:18:54PM +0800, Xuan Zhuo wrote:> On Thu, 12 Oct 2023 05:15:52 -0400, "Michael S. Tsirkin" <mst at redhat.com> wrote: > > On Wed, Oct 11, 2023 at 05:27:07PM +0800, Xuan Zhuo wrote: > > > virtqueue_set_dma_premapped() adds a new parameter to disable the > > > virtqueue premapped mode. > > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo at linux.alibaba.com> > > > --- > > > drivers/net/virtio_net.c | 2 +- > > > drivers/virtio/virtio_ring.c | 11 ++++++++--- > > > include/linux/virtio.h | 2 +- > > > 3 files changed, 10 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index fe7f314d65c9..6b5f47ebf9b2 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -737,7 +737,7 @@ static void virtnet_rq_set_premapped(struct virtnet_info *vi) > > > return; > > > > > > for (i = 0; i < vi->max_queue_pairs; i++) { > > > - if (virtqueue_set_dma_premapped(vi->rq[i].vq)) > > > + if (virtqueue_set_dma_premapped(vi->rq[i].vq, true)) > > > continue; > > > > > > vi->rq[i].do_dma = true; > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index 51d8f3299c10..b3ded56722f4 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -2784,7 +2784,7 @@ EXPORT_SYMBOL_GPL(virtqueue_resize); > > > * 0: success. > > > * -EINVAL: vring does not use the dma api, so we can not enable premapped mode. > > > */ > > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq) > > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) > > > { > > > struct vring_virtqueue *vq = to_vvq(_vq); > > > u32 num; > > > @@ -2803,8 +2803,13 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq) > > > return -EINVAL; > > > } > > > > > > - vq->premapped = true; > > > - vq->do_unmap = false; > > > + if (mode) { > > > + vq->premapped = true; > > > + vq->do_unmap = false; > > > + } else { > > > + vq->premapped = false; > > > + vq->do_unmap = vq->use_dma_api; > > > + } > > > > > > END_USE(vq); > > > > > > diff --git a/include/linux/virtio.h b/include/linux/virtio.h > > > index 4cc614a38376..1cf7b004348b 100644 > > > --- a/include/linux/virtio.h > > > +++ b/include/linux/virtio.h > > > @@ -81,7 +81,7 @@ bool virtqueue_enable_cb(struct virtqueue *vq); > > > > > > unsigned virtqueue_enable_cb_prepare(struct virtqueue *vq); > > > > > > -int virtqueue_set_dma_premapped(struct virtqueue *_vq); > > > +int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode); > > > > > > bool virtqueue_poll(struct virtqueue *vq, unsigned); > > > > Wait a sec I thought we never change premapped. If you make this > > dynamic don't you need a bunch of locking? > > Or maybe queue is empty when you change this? > > If yes pls add a bunch of BUG_ON checks to make sure this is not misused. > > > Actually, this api is called immediately after the vq init or vq reset. > > We already have such a check. > > Thanks. > > /** > * virtqueue_set_dma_premapped - set the vring premapped mode > * @_vq: the struct virtqueue we're talking about. > * > * Enable the premapped mode of the vq. > * > * The vring in premapped mode does not do dma internally, so the driver must > * do dma mapping in advance. The driver must pass the dma_address through > * dma_address of scatterlist. When the driver got a used buffer from > * the vring, it has to unmap the dma address. > * > * This function must be called immediately after creating the vq, or after vq > * reset, and before adding any buffers to it. > * > * Caller must ensure we don't call this with other virtqueue operations > * at the same time (except where noted). > * > * Returns zero or a negative error. > * 0: success. > * -EINVAL: vring does not use the dma api, so we can not enable premapped mode. > */ > int virtqueue_set_dma_premapped(struct virtqueue *_vq, bool mode) > { > struct vring_virtqueue *vq = to_vvq(_vq); > u32 num; > > START_USE(vq); > > num = vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num; > > --> if (num != vq->vq.num_free) { > END_USE(vq); > return -EINVAL; > }But it turns out virtnet_rq_set_premapped actually just ignores errors. So returning EINVAL here does nothing caller just proceeds? And checking num_free without locks is never safe anyway. I think the point is that this never triggers then just BUG_ON.> > if (!vq->use_dma_api) { > END_USE(vq); > return -EINVAL; > } > > if (mode) { > vq->premapped = true; > vq->do_unmap = false; > } else { > vq->premapped = false; > vq->do_unmap = vq->use_dma_api; > } > > END_USE(vq); > > return 0; > } > EXPORT_SYMBOL_GPL(virtqueue_set_dma_premapped); > > > > > > > > > -- > > > 2.32.0.3.g01195cf9f > >