| Message ID | 20240312033557.6351-11-xuanzhuo@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | virtio: drivers maintain dma info for premapped vq | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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@linux.alibaba.com> > --- > 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? Thanks
On Thu, 21 Mar 2024 14:02:14 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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@linux.alibaba.com> > > --- > > 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. * * 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 >
On Thu, Mar 21, 2024 at 4:22 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Thu, 21 Mar 2024 14:02:14 +0800, Jason Wang <jasowang@redhat.com> wrote: > > On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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@linux.alibaba.com> > > > --- > > > 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. 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 > > >
On Fri, 22 Mar 2024 13:13:36 +0800, Jason Wang <jasowang@redhat.com> wrote: > On Thu, Mar 21, 2024 at 4:22 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > On Thu, 21 Mar 2024 14:02:14 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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@linux.alibaba.com> > > > > --- > > > > 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 > > > > > >
On Fri, Mar 22, 2024 at 2:04 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > On Fri, 22 Mar 2024 13:13:36 +0800, Jason Wang <jasowang@redhat.com> wrote: > > On Thu, Mar 21, 2024 at 4:22 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote: > > > > > > On Thu, 21 Mar 2024 14:02:14 +0800, Jason Wang <jasowang@redhat.com> wrote: > > > > On Tue, Mar 12, 2024 at 11:36 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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@linux.alibaba.com> > > > > > --- > > > > > 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. Ok, let's add it to the doc. Thanks > > 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 > > > > > > > > > >
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) { struct vring_virtqueue *vq = to_vvq(_vq); + int err = 0; u32 num; START_USE(vq); @@ -2833,24 +2835,40 @@ int virtqueue_set_dma_premapped(struct virtqueue *_vq) return -EINVAL; } + if (vq->vq.premapped == premapped) { + END_USE(vq); + return 0; + } + if (!vq->use_dma_api) { END_USE(vq); return -EINVAL; } - vq->vq.premapped = true; + if (premapped) { + vq->vq.premapped = true; + + if (vq->packed_ring) { + kfree(vq->packed.desc_dma); + vq->packed.desc_dma = NULL; + } else { + kfree(vq->split.desc_dma); + vq->split.desc_dma = NULL; + } - if (vq->packed_ring) { - kfree(vq->packed.desc_dma); - vq->packed.desc_dma = NULL; } else { - kfree(vq->split.desc_dma); - vq->split.desc_dma = NULL; + if (vq->packed_ring) + err = vring_alloc_dma_split(&vq->split, false); + else + err = vring_alloc_dma_packed(&vq->packed, false); + + if (!err) + vq->vq.premapped = false; } END_USE(vq); - return 0; + return err; } EXPORT_SYMBOL_GPL(virtqueue_set_dma_premapped); diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 407277d5a16b..4b338590abf4 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -82,7 +82,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 premapped); bool virtqueue_poll(struct virtqueue *vq, unsigned);
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@linux.alibaba.com> --- drivers/virtio/virtio_ring.c | 34 ++++++++++++++++++++++++++-------- include/linux/virtio.h | 2 +- 2 files changed, 27 insertions(+), 9 deletions(-)