Re: [PATCH vhost v4 10/10] virtio_ring: virtqueue_set_dma_premapped support disable

[Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>]

On Fri, 22 Mar 2024 13:13:36 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> On Thu, Mar 21, 2024 at 4:22 PM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> >
> > On Thu, 21 Mar 2024 14:02:14 +0800, Jason Wang <jasowang@xxxxxxxxxx> wrote:
> > > On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > Now, the API virtqueue_set_dma_premapped just support to
> > > > enable premapped mode.
> > > >
> > > > If we allow enabling the premapped dynamically, we should
> > > > make this API to support disable the premapped mode.
> > > >
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@xxxxxxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/virtio/virtio_ring.c | 34 ++++++++++++++++++++++++++--------
> > > >  include/linux/virtio.h       |  2 +-
> > > >  2 files changed, 27 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 34f4b2c0c31e..3bf69cae4965 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -2801,6 +2801,7 @@ EXPORT_SYMBOL_GPL(virtqueue_resize);
> > > >  /**
> > > >   * virtqueue_set_dma_premapped - set the vring premapped mode
> > > >   * @_vq: the struct virtqueue we're talking about.
> > > > + * @premapped: enable/disable the premapped mode.
> > > >   *
> > > >   * Enable the premapped mode of the vq.
> > > >   *
> > > > @@ -2819,9 +2820,10 @@ 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 premapped)
> > >
> > > I think we need to document the requirement for calling this.
> > >
> > > Looking at the code, it seems it requires to stop the datapath and
> > > detach all the used buffers?
> >
> >
> > YES. The complete document is:
> >
> > /**
> >  * 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.
>
> I'm not sure this is a good design but we need at least some guard for
> this, probably WARN for num_added or others.


int virtqueue_set_dma_premapped(struct virtqueue *_vq)
{
	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;
	}


Now, we have checked the num_free.

Thanks.


>
> Thanks
>
> >  *
> >  * 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.
> >  */
> >
> > Thanks
> >
> >
> > >
> > > Thanks
> > >
> >
>