* [PATCH vhost v2 01/12] virtio_ring: split: separate dma codes
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-14 7:05 ` Jason Wang
2023-03-08 6:44 ` [PATCH vhost v2 02/12] virtio_ring: packed: " Xuan Zhuo
` (11 subsequent siblings)
12 siblings, 1 reply; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
DMA-related logic is separated from the virtqueue_add_split() to
one new function. DMA address will be saved as sg->dma_address, then
virtqueue_add_split() will use it directly. Unmap operation will be
simpler.
The purpose of this is to facilitate subsequent support to receive
dma address mapped by drivers.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 110 ++++++++++++++++++++++++++---------
1 file changed, 82 insertions(+), 28 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 41144b5246a8..8ace2f503953 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -520,6 +520,77 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
return next;
}
+static void virtqueue_unmap_sgs(struct vring_virtqueue *vq,
+ struct scatterlist *sgs[],
+ unsigned int total_sg,
+ unsigned int out_sgs,
+ unsigned int in_sgs)
+{
+ struct scatterlist *sg;
+ unsigned int n;
+
+ if (!vq->use_dma_api)
+ return;
+
+ for (n = 0; n < out_sgs; n++) {
+ for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+ if (!sg->dma_address)
+ return;
+
+ dma_unmap_page(vring_dma_dev(vq), sg->dma_address,
+ sg->length, DMA_TO_DEVICE);
+ }
+ }
+
+ for (; n < (out_sgs + in_sgs); n++) {
+ for (sg = sgs[n]; sg; sg = sg_next(sg)) {
+ if (!sg->dma_address)
+ return;
+
+ dma_unmap_page(vring_dma_dev(vq), sg->dma_address,
+ sg->length, DMA_FROM_DEVICE);
+ }
+ }
+}
+
+static int virtqueue_map_sgs(struct vring_virtqueue *vq,
+ struct scatterlist *sgs[],
+ unsigned int total_sg,
+ unsigned int out_sgs,
+ unsigned int in_sgs)
+{
+ struct scatterlist *sg;
+ unsigned int n;
+
+ 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 err;
+
+ sg->dma_address = addr;
+ }
+ }
+
+ 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 err;
+
+ sg->dma_address = addr;
+ }
+ }
+
+ return 0;
+
+err:
+ virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
+ return -ENOMEM;
+}
+
static inline int virtqueue_add_split(struct virtqueue *_vq,
struct scatterlist *sgs[],
unsigned int total_sg,
@@ -532,9 +603,9 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
struct vring_virtqueue *vq = to_vvq(_vq);
struct scatterlist *sg;
struct vring_desc *desc;
- unsigned int i, n, avail, descs_used, prev, err_idx;
- int head;
+ unsigned int i, n, avail, descs_used, prev;
bool indirect;
+ int head;
START_USE(vq);
@@ -586,32 +657,30 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
return -ENOSPC;
}
+ if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
+ return -ENOMEM;
+
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;
-
prev = i;
/* Note that we trust indirect descriptor
* table since it use stream DMA mapping.
*/
- i = virtqueue_add_desc_split(_vq, desc, i, addr, sg->length,
+ i = virtqueue_add_desc_split(_vq, desc, i,
+ sg->dma_address,
+ sg->length,
VRING_DESC_F_NEXT,
indirect);
}
}
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;
-
prev = i;
/* Note that we trust indirect descriptor
* table since it use stream DMA mapping.
*/
- i = virtqueue_add_desc_split(_vq, desc, i, addr,
+ i = virtqueue_add_desc_split(_vq, desc, i,
+ sg->dma_address,
sg->length,
VRING_DESC_F_NEXT |
VRING_DESC_F_WRITE,
@@ -679,22 +748,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
return 0;
unmap_release:
- err_idx = i;
-
- if (indirect)
- i = 0;
- else
- i = head;
-
- for (n = 0; n < total_sg; n++) {
- if (i == err_idx)
- break;
- if (indirect) {
- vring_unmap_one_split_indirect(vq, &desc[i]);
- i = virtio16_to_cpu(_vq->vdev, desc[i].next);
- } else
- i = vring_unmap_one_split(vq, i);
- }
+ virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
if (indirect)
kfree(desc);
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 01/12] virtio_ring: split: separate dma codes
2023-03-08 6:44 ` [PATCH vhost v2 01/12] virtio_ring: split: separate dma codes Xuan Zhuo
@ 2023-03-14 7:05 ` Jason Wang
2023-03-14 9:11 ` Xuan Zhuo
0 siblings, 1 reply; 29+ messages in thread
From: Jason Wang @ 2023-03-14 7:05 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization
On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> DMA-related logic is separated from the virtqueue_add_split() to
> one new function. DMA address will be saved as sg->dma_address, then
> virtqueue_add_split() will use it directly. Unmap operation will be
> simpler.
>
> The purpose of this is to facilitate subsequent support to receive
> dma address mapped by drivers.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
> drivers/virtio/virtio_ring.c | 110 ++++++++++++++++++++++++++---------
> 1 file changed, 82 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 41144b5246a8..8ace2f503953 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -520,6 +520,77 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
> return next;
> }
>
> +static void virtqueue_unmap_sgs(struct vring_virtqueue *vq,
> + struct scatterlist *sgs[],
> + unsigned int total_sg,
> + unsigned int out_sgs,
> + unsigned int in_sgs)
> +{
> + struct scatterlist *sg;
> + unsigned int n;
> +
> + if (!vq->use_dma_api)
> + return;
> +
> + for (n = 0; n < out_sgs; n++) {
> + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> + if (!sg->dma_address)
> + return;
> +
> + dma_unmap_page(vring_dma_dev(vq), sg->dma_address,
> + sg->length, DMA_TO_DEVICE);
> + }
> + }
> +
> + for (; n < (out_sgs + in_sgs); n++) {
> + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> + if (!sg->dma_address)
> + return;
> +
> + dma_unmap_page(vring_dma_dev(vq), sg->dma_address,
> + sg->length, DMA_FROM_DEVICE);
> + }
> + }
> +}
> +
> +static int virtqueue_map_sgs(struct vring_virtqueue *vq,
> + struct scatterlist *sgs[],
> + unsigned int total_sg,
> + unsigned int out_sgs,
> + unsigned int in_sgs)
> +{
> + struct scatterlist *sg;
> + unsigned int n;
> +
So we have a use_dma_api check in the unmap path but not here. More below.
> + 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 err;
> +
> + sg->dma_address = addr;
When KMSAN is not enabled (should be the case for production
environments), the only work that is done here is to assign the
dma_address. And there will be another loop after the mapping to add
descriptors, this seems to slow down the performance. Any benchmark
for this patch? Or can we:
1) return early when use_dma_api is false
2) have a helper like
vring_sg_address() {
if (!sg->dma_address)
return sg_phys(sg);
return sg->dma_address;
}
Thanks
> + }
> + }
> +
> + 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 err;
> +
> + sg->dma_address = addr;
> + }
> + }
> +
> + return 0;
> +
> +err:
> + virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
> + return -ENOMEM;
> +}
> +
> static inline int virtqueue_add_split(struct virtqueue *_vq,
> struct scatterlist *sgs[],
> unsigned int total_sg,
> @@ -532,9 +603,9 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> struct vring_virtqueue *vq = to_vvq(_vq);
> struct scatterlist *sg;
> struct vring_desc *desc;
> - unsigned int i, n, avail, descs_used, prev, err_idx;
> - int head;
> + unsigned int i, n, avail, descs_used, prev;
> bool indirect;
> + int head;
>
> START_USE(vq);
>
> @@ -586,32 +657,30 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> return -ENOSPC;
> }
>
> + if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
> + return -ENOMEM;
> +
> 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;
> -
> prev = i;
> /* Note that we trust indirect descriptor
> * table since it use stream DMA mapping.
> */
> - i = virtqueue_add_desc_split(_vq, desc, i, addr, sg->length,
> + i = virtqueue_add_desc_split(_vq, desc, i,
> + sg->dma_address,
> + sg->length,
> VRING_DESC_F_NEXT,
> indirect);
> }
> }
> 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;
> -
> prev = i;
> /* Note that we trust indirect descriptor
> * table since it use stream DMA mapping.
> */
> - i = virtqueue_add_desc_split(_vq, desc, i, addr,
> + i = virtqueue_add_desc_split(_vq, desc, i,
> + sg->dma_address,
> sg->length,
> VRING_DESC_F_NEXT |
> VRING_DESC_F_WRITE,
> @@ -679,22 +748,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> return 0;
>
> unmap_release:
> - err_idx = i;
> -
> - if (indirect)
> - i = 0;
> - else
> - i = head;
> -
> - for (n = 0; n < total_sg; n++) {
> - if (i == err_idx)
> - break;
> - if (indirect) {
> - vring_unmap_one_split_indirect(vq, &desc[i]);
> - i = virtio16_to_cpu(_vq->vdev, desc[i].next);
> - } else
> - i = vring_unmap_one_split(vq, i);
> - }
> + virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
>
> if (indirect)
> kfree(desc);
> --
> 2.32.0.3.g01195cf9f
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 01/12] virtio_ring: split: separate dma codes
2023-03-14 7:05 ` Jason Wang
@ 2023-03-14 9:11 ` Xuan Zhuo
0 siblings, 0 replies; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-14 9:11 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, virtualization
On Tue, 14 Mar 2023 15:05:58 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > DMA-related logic is separated from the virtqueue_add_split() to
> > one new function. DMA address will be saved as sg->dma_address, then
> > virtqueue_add_split() will use it directly. Unmap operation will be
> > simpler.
> >
> > The purpose of this is to facilitate subsequent support to receive
> > dma address mapped by drivers.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> > drivers/virtio/virtio_ring.c | 110 ++++++++++++++++++++++++++---------
> > 1 file changed, 82 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 41144b5246a8..8ace2f503953 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -520,6 +520,77 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
> > return next;
> > }
> >
> > +static void virtqueue_unmap_sgs(struct vring_virtqueue *vq,
> > + struct scatterlist *sgs[],
> > + unsigned int total_sg,
> > + unsigned int out_sgs,
> > + unsigned int in_sgs)
> > +{
> > + struct scatterlist *sg;
> > + unsigned int n;
> > +
> > + if (!vq->use_dma_api)
> > + return;
> > +
> > + for (n = 0; n < out_sgs; n++) {
> > + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> > + if (!sg->dma_address)
> > + return;
> > +
> > + dma_unmap_page(vring_dma_dev(vq), sg->dma_address,
> > + sg->length, DMA_TO_DEVICE);
> > + }
> > + }
> > +
> > + for (; n < (out_sgs + in_sgs); n++) {
> > + for (sg = sgs[n]; sg; sg = sg_next(sg)) {
> > + if (!sg->dma_address)
> > + return;
> > +
> > + dma_unmap_page(vring_dma_dev(vq), sg->dma_address,
> > + sg->length, DMA_FROM_DEVICE);
> > + }
> > + }
> > +}
> > +
> > +static int virtqueue_map_sgs(struct vring_virtqueue *vq,
> > + struct scatterlist *sgs[],
> > + unsigned int total_sg,
> > + unsigned int out_sgs,
> > + unsigned int in_sgs)
> > +{
> > + struct scatterlist *sg;
> > + unsigned int n;
> > +
>
> So we have a use_dma_api check in the unmap path but not here. More below.
>
> > + 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 err;
> > +
> > + sg->dma_address = addr;
>
> When KMSAN is not enabled (should be the case for production
> environments), the only work that is done here is to assign the
> dma_address. And there will be another loop after the mapping to add
> descriptors, this seems to slow down the performance. Any benchmark
> for this patch? Or can we:
>
> 1) return early when use_dma_api is false
> 2) have a helper like
>
> vring_sg_address() {
> if (!sg->dma_address)
> return sg_phys(sg);
> return sg->dma_address;
> }
LGTM
Thanks.
>
> Thanks
>
> > + }
> > + }
> > +
> > + 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 err;
> > +
> > + sg->dma_address = addr;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err:
> > + virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
> > + return -ENOMEM;
> > +}
> > +
> > static inline int virtqueue_add_split(struct virtqueue *_vq,
> > struct scatterlist *sgs[],
> > unsigned int total_sg,
> > @@ -532,9 +603,9 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > struct vring_virtqueue *vq = to_vvq(_vq);
> > struct scatterlist *sg;
> > struct vring_desc *desc;
> > - unsigned int i, n, avail, descs_used, prev, err_idx;
> > - int head;
> > + unsigned int i, n, avail, descs_used, prev;
> > bool indirect;
> > + int head;
> >
> > START_USE(vq);
> >
> > @@ -586,32 +657,30 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > return -ENOSPC;
> > }
> >
> > + if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
> > + return -ENOMEM;
> > +
> > 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;
> > -
> > prev = i;
> > /* Note that we trust indirect descriptor
> > * table since it use stream DMA mapping.
> > */
> > - i = virtqueue_add_desc_split(_vq, desc, i, addr, sg->length,
> > + i = virtqueue_add_desc_split(_vq, desc, i,
> > + sg->dma_address,
> > + sg->length,
> > VRING_DESC_F_NEXT,
> > indirect);
> > }
> > }
> > 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;
> > -
> > prev = i;
> > /* Note that we trust indirect descriptor
> > * table since it use stream DMA mapping.
> > */
> > - i = virtqueue_add_desc_split(_vq, desc, i, addr,
> > + i = virtqueue_add_desc_split(_vq, desc, i,
> > + sg->dma_address,
> > sg->length,
> > VRING_DESC_F_NEXT |
> > VRING_DESC_F_WRITE,
> > @@ -679,22 +748,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > return 0;
> >
> > unmap_release:
> > - err_idx = i;
> > -
> > - if (indirect)
> > - i = 0;
> > - else
> > - i = head;
> > -
> > - for (n = 0; n < total_sg; n++) {
> > - if (i == err_idx)
> > - break;
> > - if (indirect) {
> > - vring_unmap_one_split_indirect(vq, &desc[i]);
> > - i = virtio16_to_cpu(_vq->vdev, desc[i].next);
> > - } else
> > - i = vring_unmap_one_split(vq, i);
> > - }
> > + virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
> >
> > if (indirect)
> > kfree(desc);
> > --
> > 2.32.0.3.g01195cf9f
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH vhost v2 02/12] virtio_ring: packed: separate dma codes
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
2023-03-08 6:44 ` [PATCH vhost v2 01/12] virtio_ring: split: separate dma codes Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-08 6:44 ` [PATCH vhost v2 03/12] virtio_ring: packed-indirect: " Xuan Zhuo
` (10 subsequent siblings)
12 siblings, 0 replies; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
DMA-related logic is separated from the virtqueue_add_packed(). DMA
address will be saved as sg->dma_address, then virtqueue_add_packed()
will use it directly. Unmap operation will be simpler.
The purpose of this is to facilitate subsequent support to receive
dma address mapped by drivers.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 37 +++++++-----------------------------
1 file changed, 7 insertions(+), 30 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 8ace2f503953..b4beb51072f7 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1419,9 +1419,9 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
struct vring_virtqueue *vq = to_vvq(_vq);
struct vring_packed_desc *desc;
struct scatterlist *sg;
- unsigned int i, n, c, descs_used, err_idx;
+ unsigned int i, n, c, descs_used;
__le16 head_flags, flags;
- u16 head, id, prev, curr, avail_used_flags;
+ u16 head, id, prev, curr;
int err;
START_USE(vq);
@@ -1450,7 +1450,6 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
}
head = vq->packed.next_avail_idx;
- avail_used_flags = vq->packed.avail_used_flags;
WARN_ON_ONCE(total_sg > vq->packed.vring.num && !vq->indirect);
@@ -1468,15 +1467,13 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
id = vq->free_head;
BUG_ON(id == vq->packed.vring.num);
+ if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
+ return -ENOMEM;
+
curr = id;
c = 0;
for (n = 0; 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, n < out_sgs ?
- DMA_TO_DEVICE : DMA_FROM_DEVICE);
- if (vring_mapping_error(vq, addr))
- goto unmap_release;
-
flags = cpu_to_le16(vq->packed.avail_used_flags |
(++c == total_sg ? 0 : VRING_DESC_F_NEXT) |
(n < out_sgs ? 0 : VRING_DESC_F_WRITE));
@@ -1485,12 +1482,12 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
else
desc[i].flags = flags;
- desc[i].addr = cpu_to_le64(addr);
+ desc[i].addr = cpu_to_le64(sg->dma_address);
desc[i].len = cpu_to_le32(sg->length);
desc[i].id = cpu_to_le16(id);
if (unlikely(vq->use_dma_api)) {
- vq->packed.desc_extra[curr].addr = addr;
+ vq->packed.desc_extra[curr].addr = sg->dma_address;
vq->packed.desc_extra[curr].len = sg->length;
vq->packed.desc_extra[curr].flags =
le16_to_cpu(flags);
@@ -1536,26 +1533,6 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
END_USE(vq);
return 0;
-
-unmap_release:
- err_idx = i;
- i = head;
- curr = vq->free_head;
-
- vq->packed.avail_used_flags = avail_used_flags;
-
- for (n = 0; n < total_sg; n++) {
- if (i == err_idx)
- break;
- vring_unmap_extra_packed(vq, &vq->packed.desc_extra[curr]);
- curr = vq->packed.desc_extra[curr].next;
- i++;
- if (i >= vq->packed.vring.num)
- i = 0;
- }
-
- END_USE(vq);
- return -EIO;
}
static bool virtqueue_kick_prepare_packed(struct virtqueue *_vq)
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH vhost v2 03/12] virtio_ring: packed-indirect: separate dma codes
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
2023-03-08 6:44 ` [PATCH vhost v2 01/12] virtio_ring: split: separate dma codes Xuan Zhuo
2023-03-08 6:44 ` [PATCH vhost v2 02/12] virtio_ring: packed: " Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-08 6:44 ` [PATCH vhost v2 04/12] virtio_ring: split: support premapped Xuan Zhuo
` (9 subsequent siblings)
12 siblings, 0 replies; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
DMA-related logic is separated from the virtqueue_add_indirect_packed().
DMA address will be saved as sg->dma_address, then
virtqueue_add_indirect_packed() will use it directly. Unmap operation
will be simpler.
The purpose of this is to facilitate subsequent support to receive
dma address mapped by drivers.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b4beb51072f7..221ff54fe58b 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1303,7 +1303,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
{
struct vring_packed_desc *desc;
struct scatterlist *sg;
- unsigned int i, n, err_idx;
+ unsigned int i, n;
u16 head, id;
dma_addr_t addr;
@@ -1323,16 +1323,14 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
id = vq->free_head;
BUG_ON(id == vq->packed.vring.num);
+ if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
+ return -ENOMEM;
+
for (n = 0; n < out_sgs + in_sgs; n++) {
for (sg = sgs[n]; sg; sg = sg_next(sg)) {
- addr = vring_map_one_sg(vq, sg, n < out_sgs ?
- DMA_TO_DEVICE : DMA_FROM_DEVICE);
- if (vring_mapping_error(vq, addr))
- goto unmap_release;
-
desc[i].flags = cpu_to_le16(n < out_sgs ?
0 : VRING_DESC_F_WRITE);
- desc[i].addr = cpu_to_le64(addr);
+ desc[i].addr = cpu_to_le64(sg->dma_address);
desc[i].len = cpu_to_le32(sg->length);
i++;
}
@@ -1396,10 +1394,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
return 0;
unmap_release:
- err_idx = i;
-
- for (i = 0; i < err_idx; i++)
- vring_unmap_desc_packed(vq, &desc[i]);
+ virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
kfree(desc);
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH vhost v2 04/12] virtio_ring: split: support premapped
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
` (2 preceding siblings ...)
2023-03-08 6:44 ` [PATCH vhost v2 03/12] virtio_ring: packed-indirect: " Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-14 7:32 ` Jason Wang
2023-03-08 6:44 ` [PATCH vhost v2 05/12] virtio_ring: packed: " Xuan Zhuo
` (8 subsequent siblings)
12 siblings, 1 reply; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
virtio core only supports virtual addresses, dma is completed in virtio
core.
In some scenarios (such as the AF_XDP), the memory is allocated
and DMA is completed in advance, so it is necessary for us to support
passing the DMA address to virtio core.
Drives can use sg->dma_address to pass the mapped dma address to virtio
core. If one sg->dma_address is used then all sgs must use sg->
dma_address, otherwise all dma_address must be null.
On the non-indirect path, if dma_address is used, extra.addr will be
set to DMA_MAPPING_ERROR. So when do unmap, we can pass it.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 221ff54fe58b..61deaf0a4faf 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -457,6 +457,9 @@ static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
(flags & VRING_DESC_F_WRITE) ?
DMA_FROM_DEVICE : DMA_TO_DEVICE);
} else {
+ if (extra[i].addr == DMA_MAPPING_ERROR)
+ goto out;
+
dma_unmap_page(vring_dma_dev(vq),
extra[i].addr,
extra[i].len,
@@ -497,7 +500,8 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
dma_addr_t addr,
unsigned int len,
u16 flags,
- bool indirect)
+ bool indirect,
+ bool do_map)
{
struct vring_virtqueue *vring = to_vvq(vq);
struct vring_desc_extra *extra = vring->split.desc_extra;
@@ -511,7 +515,7 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
next = extra[i].next;
desc[i].next = cpu_to_virtio16(vq->vdev, next);
- extra[i].addr = addr;
+ extra[i].addr = do_map ? addr : DMA_MAPPING_ERROR;
extra[i].len = len;
extra[i].flags = flags;
} else
@@ -604,7 +608,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
struct scatterlist *sg;
struct vring_desc *desc;
unsigned int i, n, avail, descs_used, prev;
- bool indirect;
+ bool indirect, do_map;
int head;
START_USE(vq);
@@ -657,7 +661,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
return -ENOSPC;
}
- if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
+ do_map = !sgs[0]->dma_address;
+ if (do_map && virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
return -ENOMEM;
for (n = 0; n < out_sgs; n++) {
@@ -670,7 +675,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
sg->dma_address,
sg->length,
VRING_DESC_F_NEXT,
- indirect);
+ indirect, do_map);
}
}
for (; n < (out_sgs + in_sgs); n++) {
@@ -684,7 +689,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
sg->length,
VRING_DESC_F_NEXT |
VRING_DESC_F_WRITE,
- indirect);
+ indirect, do_map);
}
}
/* Last one doesn't continue. */
@@ -705,7 +710,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
head, addr,
total_sg * sizeof(struct vring_desc),
VRING_DESC_F_INDIRECT,
- false);
+ false, true);
}
/* We're using some buffers from the free list. */
@@ -748,7 +753,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
return 0;
unmap_release:
- virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
+ if (do_map)
+ virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
if (indirect)
kfree(desc);
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 04/12] virtio_ring: split: support premapped
2023-03-08 6:44 ` [PATCH vhost v2 04/12] virtio_ring: split: support premapped Xuan Zhuo
@ 2023-03-14 7:32 ` Jason Wang
2023-03-14 7:34 ` Jason Wang
0 siblings, 1 reply; 29+ messages in thread
From: Jason Wang @ 2023-03-14 7:32 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization
On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> virtio core only supports virtual addresses, dma is completed in virtio
> core.
>
> In some scenarios (such as the AF_XDP), the memory is allocated
> and DMA is completed in advance, so it is necessary for us to support
Should be "DMA mapping is completed".
> passing the DMA address to virtio core.
>
> Drives can use sg->dma_address to pass the mapped dma address to virtio
> core. If one sg->dma_address is used then all sgs must use sg->
> dma_address, otherwise all dma_address must be null.
"must be null when passing it to virtqueue_add_sgs()"?
>
> On the non-indirect path,
Should it be "direct desc path"?
> if dma_address is used, extra.addr will be
> set to DMA_MAPPING_ERROR. So when do unmap, we can pass it.
I don't get the meaning of "pass" here. Or do you mean
DMA_MAPING_ERROR is a hint that the desc is mapped by the upper layer
instead of the virtio core?
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
> drivers/virtio/virtio_ring.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 221ff54fe58b..61deaf0a4faf 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -457,6 +457,9 @@ static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
> (flags & VRING_DESC_F_WRITE) ?
> DMA_FROM_DEVICE : DMA_TO_DEVICE);
> } else {
> + if (extra[i].addr == DMA_MAPPING_ERROR)
> + goto out;
> +
> dma_unmap_page(vring_dma_dev(vq),
> extra[i].addr,
> extra[i].len,
> @@ -497,7 +500,8 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
> dma_addr_t addr,
> unsigned int len,
> u16 flags,
> - bool indirect)
> + bool indirect,
> + bool do_map)
> {
> struct vring_virtqueue *vring = to_vvq(vq);
> struct vring_desc_extra *extra = vring->split.desc_extra;
> @@ -511,7 +515,7 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
> next = extra[i].next;
> desc[i].next = cpu_to_virtio16(vq->vdev, next);
>
> - extra[i].addr = addr;
> + extra[i].addr = do_map ? addr : DMA_MAPPING_ERROR;
Any reason we don't need to do the same for indirect descriptors?
Thanks
> extra[i].len = len;
> extra[i].flags = flags;
> } else
> @@ -604,7 +608,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> struct scatterlist *sg;
> struct vring_desc *desc;
> unsigned int i, n, avail, descs_used, prev;
> - bool indirect;
> + bool indirect, do_map;
> int head;
>
> START_USE(vq);
> @@ -657,7 +661,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> return -ENOSPC;
> }
>
> - if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
> + do_map = !sgs[0]->dma_address;
> + if (do_map && virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
> return -ENOMEM;
>
> for (n = 0; n < out_sgs; n++) {
> @@ -670,7 +675,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> sg->dma_address,
> sg->length,
> VRING_DESC_F_NEXT,
> - indirect);
> + indirect, do_map);
> }
> }
> for (; n < (out_sgs + in_sgs); n++) {
> @@ -684,7 +689,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> sg->length,
> VRING_DESC_F_NEXT |
> VRING_DESC_F_WRITE,
> - indirect);
> + indirect, do_map);
> }
> }
> /* Last one doesn't continue. */
> @@ -705,7 +710,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> head, addr,
> total_sg * sizeof(struct vring_desc),
> VRING_DESC_F_INDIRECT,
> - false);
> + false, true);
> }
>
> /* We're using some buffers from the free list. */
> @@ -748,7 +753,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> return 0;
>
> unmap_release:
> - virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
> + if (do_map)
> + virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
>
> if (indirect)
> kfree(desc);
> --
> 2.32.0.3.g01195cf9f
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 04/12] virtio_ring: split: support premapped
2023-03-14 7:32 ` Jason Wang
@ 2023-03-14 7:34 ` Jason Wang
0 siblings, 0 replies; 29+ messages in thread
From: Jason Wang @ 2023-03-14 7:34 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization
On Tue, Mar 14, 2023 at 3:32 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > virtio core only supports virtual addresses, dma is completed in virtio
> > core.
> >
> > In some scenarios (such as the AF_XDP), the memory is allocated
> > and DMA is completed in advance, so it is necessary for us to support
>
> Should be "DMA mapping is completed".
>
> > passing the DMA address to virtio core.
> >
> > Drives can use sg->dma_address to pass the mapped dma address to virtio
> > core. If one sg->dma_address is used then all sgs must use sg->
> > dma_address, otherwise all dma_address must be null.
>
> "must be null when passing it to virtqueue_add_sgs()"?
>
> >
> > On the non-indirect path,
>
> Should it be "direct desc path"?
>
> > if dma_address is used, extra.addr will be
> > set to DMA_MAPPING_ERROR. So when do unmap, we can pass it.
>
> I don't get the meaning of "pass" here. Or do you mean
> DMA_MAPING_ERROR is a hint that the desc is mapped by the upper layer
> instead of the virtio core?
>
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> > drivers/virtio/virtio_ring.c | 22 ++++++++++++++--------
> > 1 file changed, 14 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 221ff54fe58b..61deaf0a4faf 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -457,6 +457,9 @@ static unsigned int vring_unmap_one_split(const struct vring_virtqueue *vq,
> > (flags & VRING_DESC_F_WRITE) ?
> > DMA_FROM_DEVICE : DMA_TO_DEVICE);
> > } else {
> > + if (extra[i].addr == DMA_MAPPING_ERROR)
> > + goto out;
> > +
> > dma_unmap_page(vring_dma_dev(vq),
> > extra[i].addr,
> > extra[i].len,
> > @@ -497,7 +500,8 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
> > dma_addr_t addr,
> > unsigned int len,
> > u16 flags,
> > - bool indirect)
> > + bool indirect,
> > + bool do_map)
> > {
> > struct vring_virtqueue *vring = to_vvq(vq);
> > struct vring_desc_extra *extra = vring->split.desc_extra;
> > @@ -511,7 +515,7 @@ static inline unsigned int virtqueue_add_desc_split(struct virtqueue *vq,
> > next = extra[i].next;
> > desc[i].next = cpu_to_virtio16(vq->vdev, next);
> >
> > - extra[i].addr = addr;
> > + extra[i].addr = do_map ? addr : DMA_MAPPING_ERROR;
>
> Any reason we don't need to do the same for indirect descriptors?
Ok, it's better to mention in the changelog that the following patches
will deal with this.
But in order to ease the reviewer, I suggest to squash the indirect
support into this one.
Thanks
>
> Thanks
>
> > extra[i].len = len;
> > extra[i].flags = flags;
> > } else
> > @@ -604,7 +608,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > struct scatterlist *sg;
> > struct vring_desc *desc;
> > unsigned int i, n, avail, descs_used, prev;
> > - bool indirect;
> > + bool indirect, do_map;
> > int head;
> >
> > START_USE(vq);
> > @@ -657,7 +661,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > return -ENOSPC;
> > }
> >
> > - if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
> > + do_map = !sgs[0]->dma_address;
> > + if (do_map && virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
> > return -ENOMEM;
> >
> > for (n = 0; n < out_sgs; n++) {
> > @@ -670,7 +675,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > sg->dma_address,
> > sg->length,
> > VRING_DESC_F_NEXT,
> > - indirect);
> > + indirect, do_map);
> > }
> > }
> > for (; n < (out_sgs + in_sgs); n++) {
> > @@ -684,7 +689,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > sg->length,
> > VRING_DESC_F_NEXT |
> > VRING_DESC_F_WRITE,
> > - indirect);
> > + indirect, do_map);
> > }
> > }
> > /* Last one doesn't continue. */
> > @@ -705,7 +710,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > head, addr,
> > total_sg * sizeof(struct vring_desc),
> > VRING_DESC_F_INDIRECT,
> > - false);
> > + false, true);
> > }
> >
> > /* We're using some buffers from the free list. */
> > @@ -748,7 +753,8 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > return 0;
> >
> > unmap_release:
> > - virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
> > + if (do_map)
> > + virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
> >
> > if (indirect)
> > kfree(desc);
> > --
> > 2.32.0.3.g01195cf9f
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH vhost v2 05/12] virtio_ring: packed: support premapped
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
` (3 preceding siblings ...)
2023-03-08 6:44 ` [PATCH vhost v2 04/12] virtio_ring: split: support premapped Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-08 6:44 ` [PATCH vhost v2 06/12] virtio_ring: split-indirect: " Xuan Zhuo
` (7 subsequent siblings)
12 siblings, 0 replies; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
virtio core only supports virtual addresses, dma is completed in virtio
core.
In some scenarios (such as the AF_XDP), the memory is allocated
and DMA is completed in advance, so it is necessary for us to support
passing the DMA address to virtio core.
Drives can use sg->dma_address to pass the mapped dma address to virtio
core. If one sg->dma_address is used then all sgs must use sg->dma_address,
otherwise all dma_address must be null.
On the non-indirect path, if dma_address is used, extra.addr will be
set to DMA_MAPPING_ERROR. So when do unmap, we can pass it.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 61deaf0a4faf..66a071e3bdef 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1258,6 +1258,9 @@ static void vring_unmap_extra_packed(const struct vring_virtqueue *vq,
(flags & VRING_DESC_F_WRITE) ?
DMA_FROM_DEVICE : DMA_TO_DEVICE);
} else {
+ if (extra->addr == DMA_MAPPING_ERROR)
+ return;
+
dma_unmap_page(vring_dma_dev(vq),
extra->addr, extra->len,
(flags & VRING_DESC_F_WRITE) ?
@@ -1423,6 +1426,7 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
unsigned int i, n, c, descs_used;
__le16 head_flags, flags;
u16 head, id, prev, curr;
+ bool do_map;
int err;
START_USE(vq);
@@ -1468,7 +1472,8 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
id = vq->free_head;
BUG_ON(id == vq->packed.vring.num);
- if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
+ do_map = !sgs[0]->dma_address;
+ if (do_map && virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
return -ENOMEM;
curr = id;
@@ -1488,7 +1493,8 @@ static inline int virtqueue_add_packed(struct virtqueue *_vq,
desc[i].id = cpu_to_le16(id);
if (unlikely(vq->use_dma_api)) {
- vq->packed.desc_extra[curr].addr = sg->dma_address;
+ vq->packed.desc_extra[curr].addr =
+ do_map ? sg->dma_address : DMA_MAPPING_ERROR;
vq->packed.desc_extra[curr].len = sg->length;
vq->packed.desc_extra[curr].flags =
le16_to_cpu(flags);
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH vhost v2 06/12] virtio_ring: split-indirect: support premapped
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
` (4 preceding siblings ...)
2023-03-08 6:44 ` [PATCH vhost v2 05/12] virtio_ring: packed: " Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-14 7:57 ` Jason Wang
2023-03-08 6:44 ` [PATCH vhost v2 07/12] virtio_ring: packed-indirect: " Xuan Zhuo
` (6 subsequent siblings)
12 siblings, 1 reply; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
virtio core only supports virtual addresses, dma is completed in virtio
core.
In some scenarios (such as the AF_XDP), the memory is allocated
and DMA is completed in advance, so it is necessary for us to support
passing the DMA address to virtio core.
Drives can use sg->dma_address to pass the mapped dma address to virtio
core. If one sg->dma_address is used then all sgs must use sg->dma_address,
otherwise all dma_address must be null.
On the indirect path, if dma_address is used, desc_state.indir_desc will
be mixed with VRING_INDIRECT_PREMAPPED. So when do unmap, we can pass it.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++------
1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 66a071e3bdef..11827d2e56a8 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -231,6 +231,18 @@ static void vring_free(struct virtqueue *_vq);
* Helpers.
*/
+#define VRING_INDIRECT_PREMAPPED BIT(0)
+
+#define desc_mix_dma_map(do_map, desc) \
+ (do_map ? desc : (typeof(desc))((unsigned long)(desc) | VRING_INDIRECT_PREMAPPED))
+
+#define desc_rm_dma_map(desc) \
+ ((typeof(desc))((unsigned long)(desc) & ~VRING_INDIRECT_PREMAPPED))
+
+#define desc_map_inter(desc) \
+ !((unsigned long)(desc) & VRING_INDIRECT_PREMAPPED)
+
+
#define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
@@ -725,7 +737,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
/* Store token and indirect buffer state. */
vq->split.desc_state[head].data = data;
if (indirect)
- vq->split.desc_state[head].indir_desc = desc;
+ vq->split.desc_state[head].indir_desc = desc_mix_dma_map(do_map, desc);
else
vq->split.desc_state[head].indir_desc = ctx;
@@ -820,22 +832,26 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
vq->vq.num_free++;
if (vq->indirect) {
- struct vring_desc *indir_desc =
- vq->split.desc_state[head].indir_desc;
+ struct vring_desc *mix = vq->split.desc_state[head].indir_desc;
+ struct vring_desc *indir_desc;
u32 len;
/* Free the indirect table, if any, now that it's unmapped. */
- if (!indir_desc)
+ if (!mix)
return;
+ indir_desc = desc_rm_dma_map(mix);
+
len = vq->split.desc_extra[head].len;
BUG_ON(!(vq->split.desc_extra[head].flags &
VRING_DESC_F_INDIRECT));
BUG_ON(len == 0 || len % sizeof(struct vring_desc));
- for (j = 0; j < len / sizeof(struct vring_desc); j++)
- vring_unmap_one_split_indirect(vq, &indir_desc[j]);
+ if (desc_map_inter(mix)) {
+ for (j = 0; j < len / sizeof(struct vring_desc); j++)
+ vring_unmap_one_split_indirect(vq, &indir_desc[j]);
+ }
kfree(indir_desc);
vq->split.desc_state[head].indir_desc = NULL;
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 06/12] virtio_ring: split-indirect: support premapped
2023-03-08 6:44 ` [PATCH vhost v2 06/12] virtio_ring: split-indirect: " Xuan Zhuo
@ 2023-03-14 7:57 ` Jason Wang
2023-03-14 9:13 ` Xuan Zhuo
0 siblings, 1 reply; 29+ messages in thread
From: Jason Wang @ 2023-03-14 7:57 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization
On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> virtio core only supports virtual addresses, dma is completed in virtio
> core.
>
> In some scenarios (such as the AF_XDP), the memory is allocated
> and DMA is completed in advance, so it is necessary for us to support
> passing the DMA address to virtio core.
>
> Drives can use sg->dma_address to pass the mapped dma address to virtio
> core. If one sg->dma_address is used then all sgs must use sg->dma_address,
> otherwise all dma_address must be null.
>
> On the indirect path, if dma_address is used, desc_state.indir_desc will
> be mixed with VRING_INDIRECT_PREMAPPED. So when do unmap, we can pass it.
It's better to mention why indirect descriptors can't be done in the
same way with direct descriptors.
Btw, if we change the semantics of desc_extra.dma_addr and
desc_state.indir_desc, we should add comments to definitions of those
structures.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
> drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++------
> 1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 66a071e3bdef..11827d2e56a8 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -231,6 +231,18 @@ static void vring_free(struct virtqueue *_vq);
> * Helpers.
> */
>
> +#define VRING_INDIRECT_PREMAPPED BIT(0)
> +
> +#define desc_mix_dma_map(do_map, desc) \
> + (do_map ? desc : (typeof(desc))((unsigned long)(desc) | VRING_INDIRECT_PREMAPPED))
> +
> +#define desc_rm_dma_map(desc) \
> + ((typeof(desc))((unsigned long)(desc) & ~VRING_INDIRECT_PREMAPPED))
> +
> +#define desc_map_inter(desc) \
> + !((unsigned long)(desc) & VRING_INDIRECT_PREMAPPED)
> +
> +
> #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
>
> static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> @@ -725,7 +737,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> /* Store token and indirect buffer state. */
> vq->split.desc_state[head].data = data;
> if (indirect)
> - vq->split.desc_state[head].indir_desc = desc;
> + vq->split.desc_state[head].indir_desc = desc_mix_dma_map(do_map, desc);
So using indir_desc is kind of hacky (since we don't use indirect for
rx with extra context).
But at least I think we should seeka way to use the same metadata for
both direct and indirect descriptors.
E.g can we make them all to use indir_desc?
Thanks
> else
> vq->split.desc_state[head].indir_desc = ctx;
>
> @@ -820,22 +832,26 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> vq->vq.num_free++;
>
> if (vq->indirect) {
> - struct vring_desc *indir_desc =
> - vq->split.desc_state[head].indir_desc;
> + struct vring_desc *mix = vq->split.desc_state[head].indir_desc;
> + struct vring_desc *indir_desc;
> u32 len;
>
> /* Free the indirect table, if any, now that it's unmapped. */
> - if (!indir_desc)
> + if (!mix)
> return;
>
> + indir_desc = desc_rm_dma_map(mix);
> +
> len = vq->split.desc_extra[head].len;
>
> BUG_ON(!(vq->split.desc_extra[head].flags &
> VRING_DESC_F_INDIRECT));
> BUG_ON(len == 0 || len % sizeof(struct vring_desc));
>
> - for (j = 0; j < len / sizeof(struct vring_desc); j++)
> - vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> + if (desc_map_inter(mix)) {
> + for (j = 0; j < len / sizeof(struct vring_desc); j++)
> + vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> + }
>
> kfree(indir_desc);
> vq->split.desc_state[head].indir_desc = NULL;
> --
> 2.32.0.3.g01195cf9f
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 06/12] virtio_ring: split-indirect: support premapped
2023-03-14 7:57 ` Jason Wang
@ 2023-03-14 9:13 ` Xuan Zhuo
2023-03-15 4:47 ` Jason Wang
0 siblings, 1 reply; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-14 9:13 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, virtualization
On Tue, 14 Mar 2023 15:57:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > virtio core only supports virtual addresses, dma is completed in virtio
> > core.
> >
> > In some scenarios (such as the AF_XDP), the memory is allocated
> > and DMA is completed in advance, so it is necessary for us to support
> > passing the DMA address to virtio core.
> >
> > Drives can use sg->dma_address to pass the mapped dma address to virtio
> > core. If one sg->dma_address is used then all sgs must use sg->dma_address,
> > otherwise all dma_address must be null.
> >
> > On the indirect path, if dma_address is used, desc_state.indir_desc will
> > be mixed with VRING_INDIRECT_PREMAPPED. So when do unmap, we can pass it.
>
> It's better to mention why indirect descriptors can't be done in the
> same way with direct descriptors.
>
> Btw, if we change the semantics of desc_extra.dma_addr and
> desc_state.indir_desc, we should add comments to definitions of those
> structures.
Will fix.
>
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> > drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++------
> > 1 file changed, 22 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 66a071e3bdef..11827d2e56a8 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -231,6 +231,18 @@ static void vring_free(struct virtqueue *_vq);
> > * Helpers.
> > */
> >
> > +#define VRING_INDIRECT_PREMAPPED BIT(0)
> > +
> > +#define desc_mix_dma_map(do_map, desc) \
> > + (do_map ? desc : (typeof(desc))((unsigned long)(desc) | VRING_INDIRECT_PREMAPPED))
> > +
> > +#define desc_rm_dma_map(desc) \
> > + ((typeof(desc))((unsigned long)(desc) & ~VRING_INDIRECT_PREMAPPED))
> > +
> > +#define desc_map_inter(desc) \
> > + !((unsigned long)(desc) & VRING_INDIRECT_PREMAPPED)
> > +
> > +
> > #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> >
> > static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> > @@ -725,7 +737,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > /* Store token and indirect buffer state. */
> > vq->split.desc_state[head].data = data;
> > if (indirect)
> > - vq->split.desc_state[head].indir_desc = desc;
> > + vq->split.desc_state[head].indir_desc = desc_mix_dma_map(do_map, desc);
>
> So using indir_desc is kind of hacky (since we don't use indirect for
> rx with extra context).
>
> But at least I think we should seeka way to use the same metadata for
> both direct and indirect descriptors.
>
> E.g can we make them all to use indir_desc?
I think it may not. My original idea is to use indir_desc uniformly, but
for the scene of saving ctx, we cannot guarantee that the ctx has space for us.
Thanks.
>
> Thanks
>
> > else
> > vq->split.desc_state[head].indir_desc = ctx;
> >
> > @@ -820,22 +832,26 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > vq->vq.num_free++;
> >
> > if (vq->indirect) {
> > - struct vring_desc *indir_desc =
> > - vq->split.desc_state[head].indir_desc;
> > + struct vring_desc *mix = vq->split.desc_state[head].indir_desc;
> > + struct vring_desc *indir_desc;
> > u32 len;
> >
> > /* Free the indirect table, if any, now that it's unmapped. */
> > - if (!indir_desc)
> > + if (!mix)
> > return;
> >
> > + indir_desc = desc_rm_dma_map(mix);
> > +
> > len = vq->split.desc_extra[head].len;
> >
> > BUG_ON(!(vq->split.desc_extra[head].flags &
> > VRING_DESC_F_INDIRECT));
> > BUG_ON(len == 0 || len % sizeof(struct vring_desc));
> >
> > - for (j = 0; j < len / sizeof(struct vring_desc); j++)
> > - vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> > + if (desc_map_inter(mix)) {
> > + for (j = 0; j < len / sizeof(struct vring_desc); j++)
> > + vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> > + }
> >
> > kfree(indir_desc);
> > vq->split.desc_state[head].indir_desc = NULL;
> > --
> > 2.32.0.3.g01195cf9f
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 06/12] virtio_ring: split-indirect: support premapped
2023-03-14 9:13 ` Xuan Zhuo
@ 2023-03-15 4:47 ` Jason Wang
2023-03-15 6:01 ` Xuan Zhuo
0 siblings, 1 reply; 29+ messages in thread
From: Jason Wang @ 2023-03-15 4:47 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization
On Tue, Mar 14, 2023 at 5:17 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Tue, 14 Mar 2023 15:57:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > virtio core only supports virtual addresses, dma is completed in virtio
> > > core.
> > >
> > > In some scenarios (such as the AF_XDP), the memory is allocated
> > > and DMA is completed in advance, so it is necessary for us to support
> > > passing the DMA address to virtio core.
> > >
> > > Drives can use sg->dma_address to pass the mapped dma address to virtio
> > > core. If one sg->dma_address is used then all sgs must use sg->dma_address,
> > > otherwise all dma_address must be null.
> > >
> > > On the indirect path, if dma_address is used, desc_state.indir_desc will
> > > be mixed with VRING_INDIRECT_PREMAPPED. So when do unmap, we can pass it.
> >
> > It's better to mention why indirect descriptors can't be done in the
> > same way with direct descriptors.
> >
> > Btw, if we change the semantics of desc_extra.dma_addr and
> > desc_state.indir_desc, we should add comments to definitions of those
> > structures.
>
>
> Will fix.
>
> >
> > >
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > > drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++------
> > > 1 file changed, 22 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 66a071e3bdef..11827d2e56a8 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -231,6 +231,18 @@ static void vring_free(struct virtqueue *_vq);
> > > * Helpers.
> > > */
> > >
> > > +#define VRING_INDIRECT_PREMAPPED BIT(0)
> > > +
> > > +#define desc_mix_dma_map(do_map, desc) \
> > > + (do_map ? desc : (typeof(desc))((unsigned long)(desc) | VRING_INDIRECT_PREMAPPED))
> > > +
> > > +#define desc_rm_dma_map(desc) \
> > > + ((typeof(desc))((unsigned long)(desc) & ~VRING_INDIRECT_PREMAPPED))
> > > +
> > > +#define desc_map_inter(desc) \
> > > + !((unsigned long)(desc) & VRING_INDIRECT_PREMAPPED)
> > > +
> > > +
> > > #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> > >
> > > static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> > > @@ -725,7 +737,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > > /* Store token and indirect buffer state. */
> > > vq->split.desc_state[head].data = data;
> > > if (indirect)
> > > - vq->split.desc_state[head].indir_desc = desc;
> > > + vq->split.desc_state[head].indir_desc = desc_mix_dma_map(do_map, desc);
> >
> > So using indir_desc is kind of hacky (since we don't use indirect for
> > rx with extra context).
> >
> > But at least I think we should seeka way to use the same metadata for
> > both direct and indirect descriptors.
> >
> > E.g can we make them all to use indir_desc?
>
> I think it may not. My original idea is to use indir_desc uniformly, but
> for the scene of saving ctx, we cannot guarantee that the ctx has space for us.
Ok, but the problem is that the code became even more hacky (imagine
one day we may want to use indirect for RX?).
So I tend to change my mind to introduce dedicated metadata, instead
of trying to be packed with two types of the existing ones.
Thanks
>
> Thanks.
>
> >
> > Thanks
> >
> > > else
> > > vq->split.desc_state[head].indir_desc = ctx;
> > >
> > > @@ -820,22 +832,26 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > > vq->vq.num_free++;
> > >
> > > if (vq->indirect) {
> > > - struct vring_desc *indir_desc =
> > > - vq->split.desc_state[head].indir_desc;
> > > + struct vring_desc *mix = vq->split.desc_state[head].indir_desc;
> > > + struct vring_desc *indir_desc;
> > > u32 len;
> > >
> > > /* Free the indirect table, if any, now that it's unmapped. */
> > > - if (!indir_desc)
> > > + if (!mix)
> > > return;
> > >
> > > + indir_desc = desc_rm_dma_map(mix);
> > > +
> > > len = vq->split.desc_extra[head].len;
> > >
> > > BUG_ON(!(vq->split.desc_extra[head].flags &
> > > VRING_DESC_F_INDIRECT));
> > > BUG_ON(len == 0 || len % sizeof(struct vring_desc));
> > >
> > > - for (j = 0; j < len / sizeof(struct vring_desc); j++)
> > > - vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> > > + if (desc_map_inter(mix)) {
> > > + for (j = 0; j < len / sizeof(struct vring_desc); j++)
> > > + vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> > > + }
> > >
> > > kfree(indir_desc);
> > > vq->split.desc_state[head].indir_desc = NULL;
> > > --
> > > 2.32.0.3.g01195cf9f
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 06/12] virtio_ring: split-indirect: support premapped
2023-03-15 4:47 ` Jason Wang
@ 2023-03-15 6:01 ` Xuan Zhuo
2023-03-16 2:49 ` Jason Wang
0 siblings, 1 reply; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-15 6:01 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, virtualization
On Wed, 15 Mar 2023 12:47:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Tue, Mar 14, 2023 at 5:17 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Tue, 14 Mar 2023 15:57:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > virtio core only supports virtual addresses, dma is completed in virtio
> > > > core.
> > > >
> > > > In some scenarios (such as the AF_XDP), the memory is allocated
> > > > and DMA is completed in advance, so it is necessary for us to support
> > > > passing the DMA address to virtio core.
> > > >
> > > > Drives can use sg->dma_address to pass the mapped dma address to virtio
> > > > core. If one sg->dma_address is used then all sgs must use sg->dma_address,
> > > > otherwise all dma_address must be null.
> > > >
> > > > On the indirect path, if dma_address is used, desc_state.indir_desc will
> > > > be mixed with VRING_INDIRECT_PREMAPPED. So when do unmap, we can pass it.
> > >
> > > It's better to mention why indirect descriptors can't be done in the
> > > same way with direct descriptors.
> > >
> > > Btw, if we change the semantics of desc_extra.dma_addr and
> > > desc_state.indir_desc, we should add comments to definitions of those
> > > structures.
> >
> >
> > Will fix.
> >
> > >
> > > >
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > > drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++------
> > > > 1 file changed, 22 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 66a071e3bdef..11827d2e56a8 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -231,6 +231,18 @@ static void vring_free(struct virtqueue *_vq);
> > > > * Helpers.
> > > > */
> > > >
> > > > +#define VRING_INDIRECT_PREMAPPED BIT(0)
> > > > +
> > > > +#define desc_mix_dma_map(do_map, desc) \
> > > > + (do_map ? desc : (typeof(desc))((unsigned long)(desc) | VRING_INDIRECT_PREMAPPED))
> > > > +
> > > > +#define desc_rm_dma_map(desc) \
> > > > + ((typeof(desc))((unsigned long)(desc) & ~VRING_INDIRECT_PREMAPPED))
> > > > +
> > > > +#define desc_map_inter(desc) \
> > > > + !((unsigned long)(desc) & VRING_INDIRECT_PREMAPPED)
> > > > +
> > > > +
> > > > #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> > > >
> > > > static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> > > > @@ -725,7 +737,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > > > /* Store token and indirect buffer state. */
> > > > vq->split.desc_state[head].data = data;
> > > > if (indirect)
> > > > - vq->split.desc_state[head].indir_desc = desc;
> > > > + vq->split.desc_state[head].indir_desc = desc_mix_dma_map(do_map, desc);
> > >
> > > So using indir_desc is kind of hacky (since we don't use indirect for
> > > rx with extra context).
> > >
> > > But at least I think we should seeka way to use the same metadata for
> > > both direct and indirect descriptors.
> > >
> > > E.g can we make them all to use indir_desc?
> >
> > I think it may not. My original idea is to use indir_desc uniformly, but
> > for the scene of saving ctx, we cannot guarantee that the ctx has space for us.
>
> Ok, but the problem is that the code became even more hacky (imagine
> one day we may want to use indirect for RX?).
I think it may have nothing to do with RX, but whether ctx is used. Because ctx
and indirect cannot coexist.
static inline int virtqueue_add_split(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)
{
struct vring_virtqueue *vq = to_vvq(_vq);
struct scatterlist *sg;
struct vring_desc *desc;
unsigned int i, n, avail, descs_used, prev, err_idx;
int head;
bool indirect;
START_USE(vq);
BUG_ON(data == NULL);
> BUG_ON(ctx && vq->indirect);
If we want to use ctx with indirect, we must add an dedicated metadata for ctx.
So I think the current plan is OK.
Thanks.
>
> So I tend to change my mind to introduce dedicated metadata, instead
> of trying to be packed with two types of the existing ones.
>
> Thanks
>
> >
> > Thanks.
> >
> > >
> > > Thanks
> > >
> > > > else
> > > > vq->split.desc_state[head].indir_desc = ctx;
> > > >
> > > > @@ -820,22 +832,26 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > > > vq->vq.num_free++;
> > > >
> > > > if (vq->indirect) {
> > > > - struct vring_desc *indir_desc =
> > > > - vq->split.desc_state[head].indir_desc;
> > > > + struct vring_desc *mix = vq->split.desc_state[head].indir_desc;
> > > > + struct vring_desc *indir_desc;
> > > > u32 len;
> > > >
> > > > /* Free the indirect table, if any, now that it's unmapped. */
> > > > - if (!indir_desc)
> > > > + if (!mix)
> > > > return;
> > > >
> > > > + indir_desc = desc_rm_dma_map(mix);
> > > > +
> > > > len = vq->split.desc_extra[head].len;
> > > >
> > > > BUG_ON(!(vq->split.desc_extra[head].flags &
> > > > VRING_DESC_F_INDIRECT));
> > > > BUG_ON(len == 0 || len % sizeof(struct vring_desc));
> > > >
> > > > - for (j = 0; j < len / sizeof(struct vring_desc); j++)
> > > > - vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> > > > + if (desc_map_inter(mix)) {
> > > > + for (j = 0; j < len / sizeof(struct vring_desc); j++)
> > > > + vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> > > > + }
> > > >
> > > > kfree(indir_desc);
> > > > vq->split.desc_state[head].indir_desc = NULL;
> > > > --
> > > > 2.32.0.3.g01195cf9f
> > > >
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 06/12] virtio_ring: split-indirect: support premapped
2023-03-15 6:01 ` Xuan Zhuo
@ 2023-03-16 2:49 ` Jason Wang
2023-03-16 2:53 ` Xuan Zhuo
0 siblings, 1 reply; 29+ messages in thread
From: Jason Wang @ 2023-03-16 2:49 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization
On Wed, Mar 15, 2023 at 2:06 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Wed, 15 Mar 2023 12:47:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Tue, Mar 14, 2023 at 5:17 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > >
> > > On Tue, 14 Mar 2023 15:57:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > >
> > > > > virtio core only supports virtual addresses, dma is completed in virtio
> > > > > core.
> > > > >
> > > > > In some scenarios (such as the AF_XDP), the memory is allocated
> > > > > and DMA is completed in advance, so it is necessary for us to support
> > > > > passing the DMA address to virtio core.
> > > > >
> > > > > Drives can use sg->dma_address to pass the mapped dma address to virtio
> > > > > core. If one sg->dma_address is used then all sgs must use sg->dma_address,
> > > > > otherwise all dma_address must be null.
> > > > >
> > > > > On the indirect path, if dma_address is used, desc_state.indir_desc will
> > > > > be mixed with VRING_INDIRECT_PREMAPPED. So when do unmap, we can pass it.
> > > >
> > > > It's better to mention why indirect descriptors can't be done in the
> > > > same way with direct descriptors.
> > > >
> > > > Btw, if we change the semantics of desc_extra.dma_addr and
> > > > desc_state.indir_desc, we should add comments to definitions of those
> > > > structures.
> > >
> > >
> > > Will fix.
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > ---
> > > > > drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++------
> > > > > 1 file changed, 22 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 66a071e3bdef..11827d2e56a8 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -231,6 +231,18 @@ static void vring_free(struct virtqueue *_vq);
> > > > > * Helpers.
> > > > > */
> > > > >
> > > > > +#define VRING_INDIRECT_PREMAPPED BIT(0)
> > > > > +
> > > > > +#define desc_mix_dma_map(do_map, desc) \
> > > > > + (do_map ? desc : (typeof(desc))((unsigned long)(desc) | VRING_INDIRECT_PREMAPPED))
> > > > > +
> > > > > +#define desc_rm_dma_map(desc) \
> > > > > + ((typeof(desc))((unsigned long)(desc) & ~VRING_INDIRECT_PREMAPPED))
> > > > > +
> > > > > +#define desc_map_inter(desc) \
> > > > > + !((unsigned long)(desc) & VRING_INDIRECT_PREMAPPED)
> > > > > +
> > > > > +
> > > > > #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> > > > >
> > > > > static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> > > > > @@ -725,7 +737,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > > > > /* Store token and indirect buffer state. */
> > > > > vq->split.desc_state[head].data = data;
> > > > > if (indirect)
> > > > > - vq->split.desc_state[head].indir_desc = desc;
> > > > > + vq->split.desc_state[head].indir_desc = desc_mix_dma_map(do_map, desc);
> > > >
> > > > So using indir_desc is kind of hacky (since we don't use indirect for
> > > > rx with extra context).
> > > >
> > > > But at least I think we should seeka way to use the same metadata for
> > > > both direct and indirect descriptors.
> > > >
> > > > E.g can we make them all to use indir_desc?
> > >
> > > I think it may not. My original idea is to use indir_desc uniformly, but
> > > for the scene of saving ctx, we cannot guarantee that the ctx has space for us.
> >
> > Ok, but the problem is that the code became even more hacky (imagine
> > one day we may want to use indirect for RX?).
>
>
> I think it may have nothing to do with RX, but whether ctx is used. Because ctx
> and indirect cannot coexist.
>
> static inline int virtqueue_add_split(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)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> struct scatterlist *sg;
> struct vring_desc *desc;
> unsigned int i, n, avail, descs_used, prev, err_idx;
> int head;
> bool indirect;
>
> START_USE(vq);
>
> BUG_ON(data == NULL);
> > BUG_ON(ctx && vq->indirect);
>
> If we want to use ctx with indirect, we must add an dedicated metadata for ctx.
The reason this BUG_ON() is that we do a hack:
For the vq that use ctx it can't use indirect. The only user so far is
the mergeable RX path. This path adds one more hack on top, this seems
a burden for the future maintenance. Consider one day we may want to
use a virtqueue with both indirect and extra context.
We can hear from others.
Thanks
>
> So I think the current plan is OK.
>
> Thanks.
>
> >
> > So I tend to change my mind to introduce dedicated metadata, instead
> > of trying to be packed with two types of the existing ones.
> >
> > Thanks
> >
> > >
> > > Thanks.
> > >
> > > >
> > > > Thanks
> > > >
> > > > > else
> > > > > vq->split.desc_state[head].indir_desc = ctx;
> > > > >
> > > > > @@ -820,22 +832,26 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > > > > vq->vq.num_free++;
> > > > >
> > > > > if (vq->indirect) {
> > > > > - struct vring_desc *indir_desc =
> > > > > - vq->split.desc_state[head].indir_desc;
> > > > > + struct vring_desc *mix = vq->split.desc_state[head].indir_desc;
> > > > > + struct vring_desc *indir_desc;
> > > > > u32 len;
> > > > >
> > > > > /* Free the indirect table, if any, now that it's unmapped. */
> > > > > - if (!indir_desc)
> > > > > + if (!mix)
> > > > > return;
> > > > >
> > > > > + indir_desc = desc_rm_dma_map(mix);
> > > > > +
> > > > > len = vq->split.desc_extra[head].len;
> > > > >
> > > > > BUG_ON(!(vq->split.desc_extra[head].flags &
> > > > > VRING_DESC_F_INDIRECT));
> > > > > BUG_ON(len == 0 || len % sizeof(struct vring_desc));
> > > > >
> > > > > - for (j = 0; j < len / sizeof(struct vring_desc); j++)
> > > > > - vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> > > > > + if (desc_map_inter(mix)) {
> > > > > + for (j = 0; j < len / sizeof(struct vring_desc); j++)
> > > > > + vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> > > > > + }
> > > > >
> > > > > kfree(indir_desc);
> > > > > vq->split.desc_state[head].indir_desc = NULL;
> > > > > --
> > > > > 2.32.0.3.g01195cf9f
> > > > >
> > > >
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 06/12] virtio_ring: split-indirect: support premapped
2023-03-16 2:49 ` Jason Wang
@ 2023-03-16 2:53 ` Xuan Zhuo
0 siblings, 0 replies; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-16 2:53 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, virtualization
On Thu, 16 Mar 2023 10:49:54 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Mar 15, 2023 at 2:06 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Wed, 15 Mar 2023 12:47:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Tue, Mar 14, 2023 at 5:17 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > >
> > > > On Tue, 14 Mar 2023 15:57:06 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> > > > > >
> > > > > > virtio core only supports virtual addresses, dma is completed in virtio
> > > > > > core.
> > > > > >
> > > > > > In some scenarios (such as the AF_XDP), the memory is allocated
> > > > > > and DMA is completed in advance, so it is necessary for us to support
> > > > > > passing the DMA address to virtio core.
> > > > > >
> > > > > > Drives can use sg->dma_address to pass the mapped dma address to virtio
> > > > > > core. If one sg->dma_address is used then all sgs must use sg->dma_address,
> > > > > > otherwise all dma_address must be null.
> > > > > >
> > > > > > On the indirect path, if dma_address is used, desc_state.indir_desc will
> > > > > > be mixed with VRING_INDIRECT_PREMAPPED. So when do unmap, we can pass it.
> > > > >
> > > > > It's better to mention why indirect descriptors can't be done in the
> > > > > same way with direct descriptors.
> > > > >
> > > > > Btw, if we change the semantics of desc_extra.dma_addr and
> > > > > desc_state.indir_desc, we should add comments to definitions of those
> > > > > structures.
> > > >
> > > >
> > > > Will fix.
> > > >
> > > > >
> > > > > >
> > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > ---
> > > > > > drivers/virtio/virtio_ring.c | 28 ++++++++++++++++++++++------
> > > > > > 1 file changed, 22 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > index 66a071e3bdef..11827d2e56a8 100644
> > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > @@ -231,6 +231,18 @@ static void vring_free(struct virtqueue *_vq);
> > > > > > * Helpers.
> > > > > > */
> > > > > >
> > > > > > +#define VRING_INDIRECT_PREMAPPED BIT(0)
> > > > > > +
> > > > > > +#define desc_mix_dma_map(do_map, desc) \
> > > > > > + (do_map ? desc : (typeof(desc))((unsigned long)(desc) | VRING_INDIRECT_PREMAPPED))
> > > > > > +
> > > > > > +#define desc_rm_dma_map(desc) \
> > > > > > + ((typeof(desc))((unsigned long)(desc) & ~VRING_INDIRECT_PREMAPPED))
> > > > > > +
> > > > > > +#define desc_map_inter(desc) \
> > > > > > + !((unsigned long)(desc) & VRING_INDIRECT_PREMAPPED)
> > > > > > +
> > > > > > +
> > > > > > #define to_vvq(_vq) container_of(_vq, struct vring_virtqueue, vq)
> > > > > >
> > > > > > static inline bool virtqueue_use_indirect(struct vring_virtqueue *vq,
> > > > > > @@ -725,7 +737,7 @@ static inline int virtqueue_add_split(struct virtqueue *_vq,
> > > > > > /* Store token and indirect buffer state. */
> > > > > > vq->split.desc_state[head].data = data;
> > > > > > if (indirect)
> > > > > > - vq->split.desc_state[head].indir_desc = desc;
> > > > > > + vq->split.desc_state[head].indir_desc = desc_mix_dma_map(do_map, desc);
> > > > >
> > > > > So using indir_desc is kind of hacky (since we don't use indirect for
> > > > > rx with extra context).
> > > > >
> > > > > But at least I think we should seeka way to use the same metadata for
> > > > > both direct and indirect descriptors.
> > > > >
> > > > > E.g can we make them all to use indir_desc?
> > > >
> > > > I think it may not. My original idea is to use indir_desc uniformly, but
> > > > for the scene of saving ctx, we cannot guarantee that the ctx has space for us.
> > >
> > > Ok, but the problem is that the code became even more hacky (imagine
> > > one day we may want to use indirect for RX?).
> >
> >
> > I think it may have nothing to do with RX, but whether ctx is used. Because ctx
> > and indirect cannot coexist.
> >
> > static inline int virtqueue_add_split(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)
> > {
> > struct vring_virtqueue *vq = to_vvq(_vq);
> > struct scatterlist *sg;
> > struct vring_desc *desc;
> > unsigned int i, n, avail, descs_used, prev, err_idx;
> > int head;
> > bool indirect;
> >
> > START_USE(vq);
> >
> > BUG_ON(data == NULL);
> > > BUG_ON(ctx && vq->indirect);
> >
> > If we want to use ctx with indirect, we must add an dedicated metadata for ctx.
>
> The reason this BUG_ON() is that we do a hack:
>
> For the vq that use ctx it can't use indirect. The only user so far is
> the mergeable RX path. This path adds one more hack on top, this seems
> a burden for the future maintenance. Consider one day we may want to
> use a virtqueue with both indirect and extra context.
OK. I see.
I will add a dedicated metadata if no new advise.
Thanks.
>
> We can hear from others.
>
> Thanks
>
> >
> > So I think the current plan is OK.
> >
> > Thanks.
> >
> > >
> > > So I tend to change my mind to introduce dedicated metadata, instead
> > > of trying to be packed with two types of the existing ones.
> > >
> > > Thanks
> > >
> > > >
> > > > Thanks.
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > > else
> > > > > > vq->split.desc_state[head].indir_desc = ctx;
> > > > > >
> > > > > > @@ -820,22 +832,26 @@ static void detach_buf_split(struct vring_virtqueue *vq, unsigned int head,
> > > > > > vq->vq.num_free++;
> > > > > >
> > > > > > if (vq->indirect) {
> > > > > > - struct vring_desc *indir_desc =
> > > > > > - vq->split.desc_state[head].indir_desc;
> > > > > > + struct vring_desc *mix = vq->split.desc_state[head].indir_desc;
> > > > > > + struct vring_desc *indir_desc;
> > > > > > u32 len;
> > > > > >
> > > > > > /* Free the indirect table, if any, now that it's unmapped. */
> > > > > > - if (!indir_desc)
> > > > > > + if (!mix)
> > > > > > return;
> > > > > >
> > > > > > + indir_desc = desc_rm_dma_map(mix);
> > > > > > +
> > > > > > len = vq->split.desc_extra[head].len;
> > > > > >
> > > > > > BUG_ON(!(vq->split.desc_extra[head].flags &
> > > > > > VRING_DESC_F_INDIRECT));
> > > > > > BUG_ON(len == 0 || len % sizeof(struct vring_desc));
> > > > > >
> > > > > > - for (j = 0; j < len / sizeof(struct vring_desc); j++)
> > > > > > - vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> > > > > > + if (desc_map_inter(mix)) {
> > > > > > + for (j = 0; j < len / sizeof(struct vring_desc); j++)
> > > > > > + vring_unmap_one_split_indirect(vq, &indir_desc[j]);
> > > > > > + }
> > > > > >
> > > > > > kfree(indir_desc);
> > > > > > vq->split.desc_state[head].indir_desc = NULL;
> > > > > > --
> > > > > > 2.32.0.3.g01195cf9f
> > > > > >
> > > > >
> > > >
> > >
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH vhost v2 07/12] virtio_ring: packed-indirect: support premapped
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
` (5 preceding siblings ...)
2023-03-08 6:44 ` [PATCH vhost v2 06/12] virtio_ring: split-indirect: " Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-08 6:44 ` [PATCH vhost v2 08/12] virtio_ring: update document for virtqueue_add_* Xuan Zhuo
` (5 subsequent siblings)
12 siblings, 0 replies; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
virtio core only supports virtual addresses, dma is completed in virtio
core.
In some scenarios (such as the AF_XDP), the memory is allocated
and DMA is completed in advance, so it is necessary for us to support
passing the DMA address to virtio core.
Drives can use sg->dma_address to pass the mapped dma address to virtio
core. If one sg->dma_address is used then all sgs must use sg->dma_address,
otherwise all dma_address must be null.
On the indirect path, if dma_address is used, desc_state.indir_desc will
be mixed with VRING_INDIRECT_NO_DMA_MAP. So when do unmap, we can pass it.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 11827d2e56a8..b23d301effb5 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -1331,6 +1331,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
unsigned int i, n;
u16 head, id;
dma_addr_t addr;
+ bool do_map;
head = vq->packed.next_avail_idx;
desc = alloc_indirect_packed(total_sg, gfp);
@@ -1348,7 +1349,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
id = vq->free_head;
BUG_ON(id == vq->packed.vring.num);
- if (virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
+ do_map = !sgs[0]->dma_address;
+ if (do_map && virtqueue_map_sgs(vq, sgs, total_sg, out_sgs, in_sgs))
return -ENOMEM;
for (n = 0; n < out_sgs + in_sgs; n++) {
@@ -1408,7 +1410,7 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
/* Store token and indirect buffer state. */
vq->packed.desc_state[id].num = 1;
vq->packed.desc_state[id].data = data;
- vq->packed.desc_state[id].indir_desc = desc;
+ vq->packed.desc_state[id].indir_desc = desc_mix_dma_map(do_map, desc);
vq->packed.desc_state[id].last = id;
vq->num_added += 1;
@@ -1419,7 +1421,8 @@ static int virtqueue_add_indirect_packed(struct vring_virtqueue *vq,
return 0;
unmap_release:
- virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
+ if (do_map)
+ virtqueue_unmap_sgs(vq, sgs, total_sg, out_sgs, in_sgs);
kfree(desc);
@@ -1633,14 +1636,17 @@ static void detach_buf_packed(struct vring_virtqueue *vq,
}
if (vq->indirect) {
+ struct vring_packed_desc *mix;
u32 len;
/* Free the indirect table, if any, now that it's unmapped. */
- desc = state->indir_desc;
- if (!desc)
+ mix = state->indir_desc;
+ if (!mix)
return;
- if (vq->use_dma_api) {
+ desc = desc_rm_dma_map(mix);
+
+ if (vq->use_dma_api && desc_map_inter(mix)) {
len = vq->packed.desc_extra[id].len;
for (i = 0; i < len / sizeof(struct vring_packed_desc);
i++)
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH vhost v2 08/12] virtio_ring: update document for virtqueue_add_*
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
` (6 preceding siblings ...)
2023-03-08 6:44 ` [PATCH vhost v2 07/12] virtio_ring: packed-indirect: " Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-14 7:58 ` Jason Wang
2023-03-08 6:44 ` [PATCH vhost v2 09/12] virtio_ring: introduce virtqueue_dma_dev() Xuan Zhuo
` (4 subsequent siblings)
12 siblings, 1 reply; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
Update the document of virtqueue_add_* series API, allowing the callers to
use sg->dma_address to pass the dma address to Virtio Core.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index b23d301effb5..216ac8654982 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2190,6 +2190,10 @@ static inline int virtqueue_add(struct virtqueue *_vq,
* Caller must ensure we don't call this with other virtqueue operations
* at the same time (except where noted).
*
+ * If the caller has done dma map then use sg->dma_address to pass dma address.
+ * If one sg->dma_address is used, then all sgs must use sg->dma_address;
+ * otherwise all sg->dma_address must be NULL.
+ *
* Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
*/
int virtqueue_add_sgs(struct virtqueue *_vq,
@@ -2224,6 +2228,10 @@ EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
* Caller must ensure we don't call this with other virtqueue operations
* at the same time (except where noted).
*
+ * If the caller has done dma map then use sg->dma_address to pass dma address.
+ * If one sg->dma_address is used, then all sgs must use sg->dma_address;
+ * otherwise all sg->dma_address must be NULL.
+ *
* Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
*/
int virtqueue_add_outbuf(struct virtqueue *vq,
@@ -2246,6 +2254,10 @@ EXPORT_SYMBOL_GPL(virtqueue_add_outbuf);
* Caller must ensure we don't call this with other virtqueue operations
* at the same time (except where noted).
*
+ * If the caller has done dma map then use sg->dma_address to pass dma address.
+ * If one sg->dma_address is used, then all sgs must use sg->dma_address;
+ * otherwise all sg->dma_address must be NULL.
+ *
* Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
*/
int virtqueue_add_inbuf(struct virtqueue *vq,
@@ -2269,6 +2281,10 @@ EXPORT_SYMBOL_GPL(virtqueue_add_inbuf);
* Caller must ensure we don't call this with other virtqueue operations
* at the same time (except where noted).
*
+ * If the caller has done dma map then use sg->dma_address to pass dma address.
+ * If one sg->dma_address is used, then all sgs must use sg->dma_address;
+ * otherwise all sg->dma_address must be NULL.
+ *
* Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
*/
int virtqueue_add_inbuf_ctx(struct virtqueue *vq,
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 08/12] virtio_ring: update document for virtqueue_add_*
2023-03-08 6:44 ` [PATCH vhost v2 08/12] virtio_ring: update document for virtqueue_add_* Xuan Zhuo
@ 2023-03-14 7:58 ` Jason Wang
2023-03-14 9:18 ` Xuan Zhuo
0 siblings, 1 reply; 29+ messages in thread
From: Jason Wang @ 2023-03-14 7:58 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: Michael S. Tsirkin, virtualization
On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> Update the document of virtqueue_add_* series API, allowing the callers to
> use sg->dma_address to pass the dma address to Virtio Core.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
> drivers/virtio/virtio_ring.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index b23d301effb5..216ac8654982 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2190,6 +2190,10 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> * Caller must ensure we don't call this with other virtqueue operations
> * at the same time (except where noted).
> *
> + * If the caller has done dma map then use sg->dma_address to pass dma address.
> + * If one sg->dma_address is used, then all sgs must use sg->dma_address;
> + * otherwise all sg->dma_address must be NULL.
Is it worth adding checks for those requirements?
Thanks
> + *
> * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
> */
> int virtqueue_add_sgs(struct virtqueue *_vq,
> @@ -2224,6 +2228,10 @@ EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
> * Caller must ensure we don't call this with other virtqueue operations
> * at the same time (except where noted).
> *
> + * If the caller has done dma map then use sg->dma_address to pass dma address.
> + * If one sg->dma_address is used, then all sgs must use sg->dma_address;
> + * otherwise all sg->dma_address must be NULL.
> + *
> * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
> */
> int virtqueue_add_outbuf(struct virtqueue *vq,
> @@ -2246,6 +2254,10 @@ EXPORT_SYMBOL_GPL(virtqueue_add_outbuf);
> * Caller must ensure we don't call this with other virtqueue operations
> * at the same time (except where noted).
> *
> + * If the caller has done dma map then use sg->dma_address to pass dma address.
> + * If one sg->dma_address is used, then all sgs must use sg->dma_address;
> + * otherwise all sg->dma_address must be NULL.
> + *
> * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
> */
> int virtqueue_add_inbuf(struct virtqueue *vq,
> @@ -2269,6 +2281,10 @@ EXPORT_SYMBOL_GPL(virtqueue_add_inbuf);
> * Caller must ensure we don't call this with other virtqueue operations
> * at the same time (except where noted).
> *
> + * If the caller has done dma map then use sg->dma_address to pass dma address.
> + * If one sg->dma_address is used, then all sgs must use sg->dma_address;
> + * otherwise all sg->dma_address must be NULL.
> + *
> * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
> */
> int virtqueue_add_inbuf_ctx(struct virtqueue *vq,
> --
> 2.32.0.3.g01195cf9f
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH vhost v2 08/12] virtio_ring: update document for virtqueue_add_*
2023-03-14 7:58 ` Jason Wang
@ 2023-03-14 9:18 ` Xuan Zhuo
0 siblings, 0 replies; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-14 9:18 UTC (permalink / raw)
To: Jason Wang; +Cc: Michael S. Tsirkin, virtualization
On Tue, 14 Mar 2023 15:58:22 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Mar 8, 2023 at 2:44 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > Update the document of virtqueue_add_* series API, allowing the callers to
> > use sg->dma_address to pass the dma address to Virtio Core.
> >
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> > drivers/virtio/virtio_ring.c | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index b23d301effb5..216ac8654982 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -2190,6 +2190,10 @@ static inline int virtqueue_add(struct virtqueue *_vq,
> > * Caller must ensure we don't call this with other virtqueue operations
> > * at the same time (except where noted).
> > *
> > + * If the caller has done dma map then use sg->dma_address to pass dma address.
> > + * If one sg->dma_address is used, then all sgs must use sg->dma_address;
> > + * otherwise all sg->dma_address must be NULL.
>
> Is it worth adding checks for those requirements?
If we want to check, we need to check every SG, and I think the overhead is a
bit big.
If the driver is not implemented as these requirements, I think he will always
encounter some abnormalities when testing.
Thanks.
>
> Thanks
>
> > + *
> > * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
> > */
> > int virtqueue_add_sgs(struct virtqueue *_vq,
> > @@ -2224,6 +2228,10 @@ EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
> > * Caller must ensure we don't call this with other virtqueue operations
> > * at the same time (except where noted).
> > *
> > + * If the caller has done dma map then use sg->dma_address to pass dma address.
> > + * If one sg->dma_address is used, then all sgs must use sg->dma_address;
> > + * otherwise all sg->dma_address must be NULL.
> > + *
> > * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
> > */
> > int virtqueue_add_outbuf(struct virtqueue *vq,
> > @@ -2246,6 +2254,10 @@ EXPORT_SYMBOL_GPL(virtqueue_add_outbuf);
> > * Caller must ensure we don't call this with other virtqueue operations
> > * at the same time (except where noted).
> > *
> > + * If the caller has done dma map then use sg->dma_address to pass dma address.
> > + * If one sg->dma_address is used, then all sgs must use sg->dma_address;
> > + * otherwise all sg->dma_address must be NULL.
> > + *
> > * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
> > */
> > int virtqueue_add_inbuf(struct virtqueue *vq,
> > @@ -2269,6 +2281,10 @@ EXPORT_SYMBOL_GPL(virtqueue_add_inbuf);
> > * Caller must ensure we don't call this with other virtqueue operations
> > * at the same time (except where noted).
> > *
> > + * If the caller has done dma map then use sg->dma_address to pass dma address.
> > + * If one sg->dma_address is used, then all sgs must use sg->dma_address;
> > + * otherwise all sg->dma_address must be NULL.
> > + *
> > * Returns zero or a negative error (ie. ENOSPC, ENOMEM, EIO).
> > */
> > int virtqueue_add_inbuf_ctx(struct virtqueue *vq,
> > --
> > 2.32.0.3.g01195cf9f
> >
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH vhost v2 09/12] virtio_ring: introduce virtqueue_dma_dev()
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
` (7 preceding siblings ...)
2023-03-08 6:44 ` [PATCH vhost v2 08/12] virtio_ring: update document for virtqueue_add_* Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-08 6:44 ` [PATCH vhost v2 10/12] virtio_ring: correct the expression of the description of virtqueue_resize() Xuan Zhuo
` (3 subsequent siblings)
12 siblings, 0 replies; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
Added virtqueue_dma_dev() to get DMA device for virtio. Then the
caller can do dma operation in advance. The purpose is to keep memory
mapped across multiple add/get buf operations.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
drivers/virtio/virtio.c | 6 ++++++
drivers/virtio/virtio_ring.c | 17 +++++++++++++++++
include/linux/virtio.h | 2 ++
3 files changed, 25 insertions(+)
diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 3893dc29eb26..11c5035369e2 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -1,4 +1,5 @@
// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/dma-mapping.h>
#include <linux/virtio.h>
#include <linux/spinlock.h>
#include <linux/virtio_config.h>
@@ -243,6 +244,11 @@ static int virtio_dev_probe(struct device *_d)
u64 driver_features;
u64 driver_features_legacy;
+ _d->dma_mask = &_d->coherent_dma_mask;
+ err = dma_set_mask_and_coherent(_d, DMA_BIT_MASK(64));
+ if (err)
+ return err;
+
/* We have a driver! */
virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 216ac8654982..f63637c288a0 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2297,6 +2297,23 @@ int virtqueue_add_inbuf_ctx(struct virtqueue *vq,
}
EXPORT_SYMBOL_GPL(virtqueue_add_inbuf_ctx);
+/**
+ * virtqueue_dma_dev - get the dma dev
+ * @vq: the struct virtqueue we're talking about.
+ *
+ * Returns the dma dev. That can been used for dma api.
+ */
+struct device *virtqueue_dma_dev(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+
+ if (vq->use_dma_api)
+ return vring_dma_dev(vq);
+ else
+ return &vq->vq.vdev->dev;
+}
+EXPORT_SYMBOL_GPL(virtqueue_dma_dev);
+
/**
* virtqueue_kick_prepare - first half of split virtqueue_kick call.
* @_vq: the struct virtqueue
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 2b472514c49b..1fa50191cf0a 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -61,6 +61,8 @@ int virtqueue_add_sgs(struct virtqueue *vq,
void *data,
gfp_t gfp);
+struct device *virtqueue_dma_dev(struct virtqueue *vq);
+
bool virtqueue_kick(struct virtqueue *vq);
bool virtqueue_kick_prepare(struct virtqueue *vq);
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH vhost v2 10/12] virtio_ring: correct the expression of the description of virtqueue_resize()
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
` (8 preceding siblings ...)
2023-03-08 6:44 ` [PATCH vhost v2 09/12] virtio_ring: introduce virtqueue_dma_dev() Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-08 6:44 ` [PATCH vhost v2 11/12] virtio_ring: separate the logic of reset/enable from virtqueue_resize Xuan Zhuo
` (2 subsequent siblings)
12 siblings, 0 replies; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
Modify the "useless" to a more accurate "unused".
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
drivers/virtio/virtio_ring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index f63637c288a0..a705485fea47 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2702,7 +2702,7 @@ EXPORT_SYMBOL_GPL(vring_create_virtqueue_dma);
* virtqueue_resize - resize the vring of vq
* @_vq: the struct virtqueue we're talking about.
* @num: new ring num
- * @recycle: callback for recycle the useless buffer
+ * @recycle: callback to recycle unused buffers
*
* When it is really necessary to create a new vring, it will set the current vq
* into the reset state. Then call the passed callback to recycle the buffer
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH vhost v2 11/12] virtio_ring: separate the logic of reset/enable from virtqueue_resize
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
` (9 preceding siblings ...)
2023-03-08 6:44 ` [PATCH vhost v2 10/12] virtio_ring: correct the expression of the description of virtqueue_resize() Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-15 2:56 ` Jason Wang
2023-03-08 6:44 ` [PATCH vhost v2 12/12] virtio_ring: introduce virtqueue_reset() Xuan Zhuo
2023-03-10 9:05 ` [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Michael S. Tsirkin
12 siblings, 1 reply; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
The subsequent reset function will reuse these logic.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 58 ++++++++++++++++++++++++------------
1 file changed, 39 insertions(+), 19 deletions(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a705485fea47..f26bd7bbff5e 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2156,6 +2156,43 @@ static int virtqueue_resize_packed(struct virtqueue *_vq, u32 num)
return -ENOMEM;
}
+static int virtqueue_disable_and_recycle(struct virtqueue *_vq,
+ void (*recycle)(struct virtqueue *vq, void *buf))
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ struct virtio_device *vdev = vq->vq.vdev;
+ void *buf;
+ int err;
+
+ if (!vq->we_own_ring)
+ return -EPERM;
+
+ if (!vdev->config->disable_vq_and_reset)
+ return -ENOENT;
+
+ if (!vdev->config->enable_vq_after_reset)
+ return -ENOENT;
+
+ err = vdev->config->disable_vq_and_reset(_vq);
+ if (err)
+ return err;
+
+ while ((buf = virtqueue_detach_unused_buf(_vq)) != NULL)
+ recycle(_vq, buf);
+
+ return 0;
+}
+
+static int virtqueue_enable_after_reset(struct virtqueue *_vq)
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ struct virtio_device *vdev = vq->vq.vdev;
+
+ if (vdev->config->enable_vq_after_reset(_vq))
+ return -EBUSY;
+
+ return 0;
+}
/*
* Generic functions and exported symbols.
@@ -2726,13 +2763,8 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
void (*recycle)(struct virtqueue *vq, void *buf))
{
struct vring_virtqueue *vq = to_vvq(_vq);
- struct virtio_device *vdev = vq->vq.vdev;
- void *buf;
int err;
- if (!vq->we_own_ring)
- return -EPERM;
-
if (num > vq->vq.num_max)
return -E2BIG;
@@ -2742,28 +2774,16 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
if ((vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num) == num)
return 0;
- if (!vdev->config->disable_vq_and_reset)
- return -ENOENT;
-
- if (!vdev->config->enable_vq_after_reset)
- return -ENOENT;
-
- err = vdev->config->disable_vq_and_reset(_vq);
+ err = virtqueue_disable_and_recycle(_vq, recycle);
if (err)
return err;
- while ((buf = virtqueue_detach_unused_buf(_vq)) != NULL)
- recycle(_vq, buf);
-
if (vq->packed_ring)
err = virtqueue_resize_packed(_vq, num);
else
err = virtqueue_resize_split(_vq, num);
- if (vdev->config->enable_vq_after_reset(_vq))
- return -EBUSY;
-
- return err;
+ return virtqueue_enable_after_reset(_vq);
}
EXPORT_SYMBOL_GPL(virtqueue_resize);
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 11/12] virtio_ring: separate the logic of reset/enable from virtqueue_resize
2023-03-08 6:44 ` [PATCH vhost v2 11/12] virtio_ring: separate the logic of reset/enable from virtqueue_resize Xuan Zhuo
@ 2023-03-15 2:56 ` Jason Wang
0 siblings, 0 replies; 29+ messages in thread
From: Jason Wang @ 2023-03-15 2:56 UTC (permalink / raw)
To: Xuan Zhuo, virtualization; +Cc: Michael S. Tsirkin
在 2023/3/8 14:44, Xuan Zhuo 写道:
> The subsequent reset function will reuse these logic.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
> ---
> drivers/virtio/virtio_ring.c | 58 ++++++++++++++++++++++++------------
> 1 file changed, 39 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index a705485fea47..f26bd7bbff5e 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2156,6 +2156,43 @@ static int virtqueue_resize_packed(struct virtqueue *_vq, u32 num)
> return -ENOMEM;
> }
>
> +static int virtqueue_disable_and_recycle(struct virtqueue *_vq,
> + void (*recycle)(struct virtqueue *vq, void *buf))
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + struct virtio_device *vdev = vq->vq.vdev;
> + void *buf;
> + int err;
> +
> + if (!vq->we_own_ring)
> + return -EPERM;
> +
> + if (!vdev->config->disable_vq_and_reset)
> + return -ENOENT;
> +
> + if (!vdev->config->enable_vq_after_reset)
> + return -ENOENT;
> +
> + err = vdev->config->disable_vq_and_reset(_vq);
> + if (err)
> + return err;
> +
> + while ((buf = virtqueue_detach_unused_buf(_vq)) != NULL)
> + recycle(_vq, buf);
> +
> + return 0;
> +}
> +
> +static int virtqueue_enable_after_reset(struct virtqueue *_vq)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + struct virtio_device *vdev = vq->vq.vdev;
> +
> + if (vdev->config->enable_vq_after_reset(_vq))
> + return -EBUSY;
> +
> + return 0;
> +}
>
> /*
> * Generic functions and exported symbols.
> @@ -2726,13 +2763,8 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> void (*recycle)(struct virtqueue *vq, void *buf))
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> - struct virtio_device *vdev = vq->vq.vdev;
> - void *buf;
> int err;
>
> - if (!vq->we_own_ring)
> - return -EPERM;
> -
> if (num > vq->vq.num_max)
> return -E2BIG;
>
> @@ -2742,28 +2774,16 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> if ((vq->packed_ring ? vq->packed.vring.num : vq->split.vring.num) == num)
> return 0;
>
> - if (!vdev->config->disable_vq_and_reset)
> - return -ENOENT;
> -
> - if (!vdev->config->enable_vq_after_reset)
> - return -ENOENT;
> -
> - err = vdev->config->disable_vq_and_reset(_vq);
> + err = virtqueue_disable_and_recycle(_vq, recycle);
> if (err)
> return err;
>
> - while ((buf = virtqueue_detach_unused_buf(_vq)) != NULL)
> - recycle(_vq, buf);
> -
> if (vq->packed_ring)
> err = virtqueue_resize_packed(_vq, num);
> else
> err = virtqueue_resize_split(_vq, num);
>
> - if (vdev->config->enable_vq_after_reset(_vq))
> - return -EBUSY;
> -
> - return err;
> + return virtqueue_enable_after_reset(_vq);
> }
> EXPORT_SYMBOL_GPL(virtqueue_resize);
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH vhost v2 12/12] virtio_ring: introduce virtqueue_reset()
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
` (10 preceding siblings ...)
2023-03-08 6:44 ` [PATCH vhost v2 11/12] virtio_ring: separate the logic of reset/enable from virtqueue_resize Xuan Zhuo
@ 2023-03-08 6:44 ` Xuan Zhuo
2023-03-15 3:00 ` Jason Wang
2023-03-10 9:05 ` [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Michael S. Tsirkin
12 siblings, 1 reply; 29+ messages in thread
From: Xuan Zhuo @ 2023-03-08 6:44 UTC (permalink / raw)
To: virtualization; +Cc: Michael S. Tsirkin
Introduce virtqueue_reset() to release all buffer inside vq.
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
drivers/virtio/virtio_ring.c | 33 +++++++++++++++++++++++++++++++++
include/linux/virtio.h | 2 ++
2 files changed, 35 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index f26bd7bbff5e..1a8de916bb20 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -2787,6 +2787,39 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
}
EXPORT_SYMBOL_GPL(virtqueue_resize);
+/**
+ * virtqueue_reset - detach and recycle all unused buffers
+ * @_vq: the struct virtqueue we're talking about.
+ * @recycle: callback to recycle unused buffers
+ *
+ * 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.
+ * -EBUSY: Failed to sync with device, vq may not work properly
+ * -ENOENT: Transport or device not supported
+ * -EPERM: Operation not permitted
+ */
+int virtqueue_reset(struct virtqueue *_vq,
+ void (*recycle)(struct virtqueue *vq, void *buf))
+{
+ struct vring_virtqueue *vq = to_vvq(_vq);
+ int err;
+
+ err = virtqueue_disable_and_recycle(_vq, recycle);
+ if (err)
+ return err;
+
+ if (vq->packed_ring)
+ virtqueue_reinit_packed(vq);
+ else
+ virtqueue_reinit_split(vq);
+
+ return virtqueue_enable_after_reset(_vq);
+}
+EXPORT_SYMBOL_GPL(virtqueue_reset);
+
/* Only available for split ring */
struct virtqueue *vring_new_virtqueue(unsigned int index,
unsigned int num,
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 1fa50191cf0a..22bbd06ef8c8 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -97,6 +97,8 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
int virtqueue_resize(struct virtqueue *vq, u32 num,
void (*recycle)(struct virtqueue *vq, void *buf));
+int virtqueue_reset(struct virtqueue *vq,
+ void (*recycle)(struct virtqueue *vq, void *buf));
/**
* struct virtio_device - representation of a device using virtio
--
2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 12/12] virtio_ring: introduce virtqueue_reset()
2023-03-08 6:44 ` [PATCH vhost v2 12/12] virtio_ring: introduce virtqueue_reset() Xuan Zhuo
@ 2023-03-15 3:00 ` Jason Wang
0 siblings, 0 replies; 29+ messages in thread
From: Jason Wang @ 2023-03-15 3:00 UTC (permalink / raw)
To: Xuan Zhuo, virtualization; +Cc: Michael S. Tsirkin
在 2023/3/8 14:44, Xuan Zhuo 写道:
> Introduce virtqueue_reset() to release all buffer inside vq.
>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
> ---
> drivers/virtio/virtio_ring.c | 33 +++++++++++++++++++++++++++++++++
> include/linux/virtio.h | 2 ++
> 2 files changed, 35 insertions(+)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index f26bd7bbff5e..1a8de916bb20 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -2787,6 +2787,39 @@ int virtqueue_resize(struct virtqueue *_vq, u32 num,
> }
> EXPORT_SYMBOL_GPL(virtqueue_resize);
>
> +/**
> + * virtqueue_reset - detach and recycle all unused buffers
> + * @_vq: the struct virtqueue we're talking about.
> + * @recycle: callback to recycle unused buffers
> + *
> + * 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.
> + * -EBUSY: Failed to sync with device, vq may not work properly
> + * -ENOENT: Transport or device not supported
> + * -EPERM: Operation not permitted
> + */
> +int virtqueue_reset(struct virtqueue *_vq,
> + void (*recycle)(struct virtqueue *vq, void *buf))
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + int err;
> +
> + err = virtqueue_disable_and_recycle(_vq, recycle);
> + if (err)
> + return err;
> +
> + if (vq->packed_ring)
> + virtqueue_reinit_packed(vq);
> + else
> + virtqueue_reinit_split(vq);
> +
> + return virtqueue_enable_after_reset(_vq);
> +}
> +EXPORT_SYMBOL_GPL(virtqueue_reset);
> +
> /* Only available for split ring */
> struct virtqueue *vring_new_virtqueue(unsigned int index,
> unsigned int num,
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 1fa50191cf0a..22bbd06ef8c8 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -97,6 +97,8 @@ dma_addr_t virtqueue_get_used_addr(struct virtqueue *vq);
>
> int virtqueue_resize(struct virtqueue *vq, u32 num,
> void (*recycle)(struct virtqueue *vq, void *buf));
> +int virtqueue_reset(struct virtqueue *vq,
> + void (*recycle)(struct virtqueue *vq, void *buf));
>
> /**
> * struct virtio_device - representation of a device using virtio
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH vhost v2 00/12] virtio core prepares for AF_XDP
2023-03-08 6:44 [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Xuan Zhuo
` (11 preceding siblings ...)
2023-03-08 6:44 ` [PATCH vhost v2 12/12] virtio_ring: introduce virtqueue_reset() Xuan Zhuo
@ 2023-03-10 9:05 ` Michael S. Tsirkin
2023-03-10 9:33 ` Jason Wang
12 siblings, 1 reply; 29+ messages in thread
From: Michael S. Tsirkin @ 2023-03-10 9:05 UTC (permalink / raw)
To: Xuan Zhuo; +Cc: virtualization
On Wed, Mar 08, 2023 at 02:44:31PM +0800, Xuan Zhuo wrote:
> XDP socket(AF_XDP) is an excellent bypass kernel network framework. The zero
> copy feature of xsk (XDP socket) needs to be supported by the driver. The
> performance of zero copy is very good.
>
> ENV: Qemu with vhost.
>
> vhost cpu | Guest APP CPU |Guest Softirq CPU | PPS
> -----------------------------|---------------|------------------|------------
> xmit by sockperf: 90% | 100% | | 318967
> xmit by xsk: 100% | 30% | 33% | 1192064
> recv by sockperf: 100% | 68% | 100% | 692288
> recv by xsk: 100% | 33% | 43% | 771670
>
> Before achieving the function of Virtio-Net, we also have to let virtio core
> support these features:
>
> 1. virtio core support premapped
> 2. virtio core support reset per-queue
> 3. introduce DMA APIs to virtio core
>
> Please review.
Jason can I get some acks on this?
> Thanks.
>
> v2:
> 1. based on sgs[0]->dma_address to judgment is premapped
> 2. based on extra.addr to judgment to do unmap for no-indirect desc
> 3. based on indir_desc to judgment to do unmap for indirect desc
> 4. rename virtqueue_get_dma_dev to virtqueue_dma_dev
>
> v1:
> 1. expose dma device. NO introduce the api for dma and sync
> 2. split some commit for review.
>
> Xuan Zhuo (12):
> virtio_ring: split: separate dma codes
> virtio_ring: packed: separate dma codes
> virtio_ring: packed-indirect: separate dma codes
> virtio_ring: split: support premapped
> virtio_ring: packed: support premapped
> virtio_ring: split-indirect: support premapped
> virtio_ring: packed-indirect: support premapped
> virtio_ring: update document for virtqueue_add_*
> virtio_ring: introduce virtqueue_dma_dev()
> virtio_ring: correct the expression of the description of
> virtqueue_resize()
> virtio_ring: separate the logic of reset/enable from virtqueue_resize
> virtio_ring: introduce virtqueue_reset()
>
> drivers/virtio/virtio.c | 6 +
> drivers/virtio/virtio_ring.c | 354 +++++++++++++++++++++++++----------
> include/linux/virtio.h | 4 +
> 3 files changed, 260 insertions(+), 104 deletions(-)
>
> --
> 2.32.0.3.g01195cf9f
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread* Re: [PATCH vhost v2 00/12] virtio core prepares for AF_XDP
2023-03-10 9:05 ` [PATCH vhost v2 00/12] virtio core prepares for AF_XDP Michael S. Tsirkin
@ 2023-03-10 9:33 ` Jason Wang
0 siblings, 0 replies; 29+ messages in thread
From: Jason Wang @ 2023-03-10 9:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization
On Fri, Mar 10, 2023 at 5:06 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Mar 08, 2023 at 02:44:31PM +0800, Xuan Zhuo wrote:
> > XDP socket(AF_XDP) is an excellent bypass kernel network framework. The zero
> > copy feature of xsk (XDP socket) needs to be supported by the driver. The
> > performance of zero copy is very good.
> >
> > ENV: Qemu with vhost.
> >
> > vhost cpu | Guest APP CPU |Guest Softirq CPU | PPS
> > -----------------------------|---------------|------------------|------------
> > xmit by sockperf: 90% | 100% | | 318967
> > xmit by xsk: 100% | 30% | 33% | 1192064
> > recv by sockperf: 100% | 68% | 100% | 692288
> > recv by xsk: 100% | 33% | 43% | 771670
> >
> > Before achieving the function of Virtio-Net, we also have to let virtio core
> > support these features:
> >
> > 1. virtio core support premapped
> > 2. virtio core support reset per-queue
> > 3. introduce DMA APIs to virtio core
> >
> > Please review.
>
>
> Jason can I get some acks on this?
In my TODO list, I will do it next week.
Thanks
>
> > Thanks.
> >
> > v2:
> > 1. based on sgs[0]->dma_address to judgment is premapped
> > 2. based on extra.addr to judgment to do unmap for no-indirect desc
> > 3. based on indir_desc to judgment to do unmap for indirect desc
> > 4. rename virtqueue_get_dma_dev to virtqueue_dma_dev
> >
> > v1:
> > 1. expose dma device. NO introduce the api for dma and sync
> > 2. split some commit for review.
> >
> > Xuan Zhuo (12):
> > virtio_ring: split: separate dma codes
> > virtio_ring: packed: separate dma codes
> > virtio_ring: packed-indirect: separate dma codes
> > virtio_ring: split: support premapped
> > virtio_ring: packed: support premapped
> > virtio_ring: split-indirect: support premapped
> > virtio_ring: packed-indirect: support premapped
> > virtio_ring: update document for virtqueue_add_*
> > virtio_ring: introduce virtqueue_dma_dev()
> > virtio_ring: correct the expression of the description of
> > virtqueue_resize()
> > virtio_ring: separate the logic of reset/enable from virtqueue_resize
> > virtio_ring: introduce virtqueue_reset()
> >
> > drivers/virtio/virtio.c | 6 +
> > drivers/virtio/virtio_ring.c | 354 +++++++++++++++++++++++++----------
> > include/linux/virtio.h | 4 +
> > 3 files changed, 260 insertions(+), 104 deletions(-)
> >
> > --
> > 2.32.0.3.g01195cf9f
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 29+ messages in thread