thr3ads.net - Linux Virtualization - [RFC 0/4] rpmsg: Fix init of DMA:able virtqueues [May 2015]

If this information is useful, please help other people find it:
Share via:

Edgar E. Iglesias

2015-May-01 05:01 UTC

[RFC 0/4] rpmsg: Fix init of DMA:able virtqueues

From: "Edgar E. Iglesias" <edgar.iglesias at xilinx.com>

I'm trying to run rpmsg and remoteproc on the ZynqMP (arm64) but I'm
hitting
a DMA/mm error. The issue was discussed here:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-March/333050.html

Russel King pointed out that the arm64 is not doing anything wrong by
returning vmapped memory (which is incompatible with sg_phys()). Hence this
RFC series that tries to illustrate/fix the problem in rpmsg/virtio.

Is this going in the right direction?
Any ideas or suggestions on how to better fix this?

Something that worries me a little is that it would be nice if the DMA
capability for virtio protocols was not hardcoded like this but rather
somehow selectable by the framework. I was hoping that it would be possible
to use _any_ virtio based protocol to communicate with remote-proc/DMA and
not just rpmsg.

Thanks,
Edgar


Edgar E. Iglesias (4):
  virtio_ring: Break out vring descriptor setup code
  virtio_ring: Add option for DMA mapped sgs in virtqueue_add
  virtio: Add dma variants of virtqueue_add_in and outbuf
  rpmsg: DMA map sgs passed to virtio

 drivers/rpmsg/virtio_rpmsg_bus.c | 28 ++++++++++++++++-----
 drivers/virtio/virtio_ring.c     | 53 +++++++++++++++++++++++++++++++---------
 include/linux/virtio.h           | 10 ++++++++
 3 files changed, 74 insertions(+), 17 deletions(-)

-- 
1.9.1

Edgar E. Iglesias

2015-May-01 05:01 UTC

head link

[RFC 1/4] virtio_ring: Break out vring descriptor setup code

From: "Edgar E. Iglesias" <edgar.iglesias at xilinx.com>

Break out descriptor setup into a separate inline function.
No functional change.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias at xilinx.com>
---
 drivers/virtio/virtio_ring.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 096b857..66d2cb9 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -120,6 +120,16 @@ static struct vring_desc *alloc_indirect(struct virtqueue
*_vq,
 	return desc;
 }
 
+static inline void vring_desc_set(struct virtio_device *vdev,
+				  struct vring_desc *desc,
+				  struct scatterlist *sg,
+				  unsigned int flags)
+{
+	desc->flags = cpu_to_virtio16(vdev, flags);
+	desc->addr = cpu_to_virtio64(vdev, sg_phys(sg));
+	desc->len = cpu_to_virtio32(vdev, sg->length);
+}
+
 static inline int virtqueue_add(struct virtqueue *_vq,
 				struct scatterlist *sgs[],
 				unsigned int total_sg,
@@ -205,18 +215,16 @@ static inline int virtqueue_add(struct virtqueue *_vq,
 
 	for (n = 0; n < out_sgs; n++) {
 		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
-			desc[i].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT);
-			desc[i].addr = cpu_to_virtio64(_vq->vdev, sg_phys(sg));
-			desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
+			vring_desc_set(_vq->vdev, desc + i, sg,
+				       VRING_DESC_F_NEXT);
 			prev = i;
 			i = virtio16_to_cpu(_vq->vdev, desc[i].next);
 		}
 	}
 	for (; n < (out_sgs + in_sgs); n++) {
 		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
-			desc[i].flags = cpu_to_virtio16(_vq->vdev, VRING_DESC_F_NEXT |
VRING_DESC_F_WRITE);
-			desc[i].addr = cpu_to_virtio64(_vq->vdev, sg_phys(sg));
-			desc[i].len = cpu_to_virtio32(_vq->vdev, sg->length);
+			vring_desc_set(_vq->vdev, desc + i, sg,
+				       VRING_DESC_F_NEXT | VRING_DESC_F_WRITE);
 			prev = i;
 			i = virtio16_to_cpu(_vq->vdev, desc[i].next);
 		}
-- 
1.9.1

Edgar E. Iglesias

2015-May-01 05:01 UTC

head link

[RFC 2/4] virtio_ring: Add option for DMA mapped sgs in virtqueue_add

From: "Edgar E. Iglesias" <edgar.iglesias at xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias at xilinx.com>
---
 drivers/virtio/virtio_ring.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 66d2cb9..233f3c6 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -123,11 +123,13 @@ static struct vring_desc *alloc_indirect(struct virtqueue
*_vq,
 static inline void vring_desc_set(struct virtio_device *vdev,
 				  struct vring_desc *desc,
 				  struct scatterlist *sg,
-				  unsigned int flags)
+				  unsigned int flags,
+				  bool dma)
 {
 	desc->flags = cpu_to_virtio16(vdev, flags);
-	desc->addr = cpu_to_virtio64(vdev, sg_phys(sg));
-	desc->len = cpu_to_virtio32(vdev, sg->length);
+	desc->addr = cpu_to_virtio64(vdev,
+				     dma ? sg_dma_address(sg) : sg_phys(sg));
+	desc->len = cpu_to_virtio32(vdev, dma ? sg_dma_len(sg) : sg->length);
 }
 
 static inline int virtqueue_add(struct virtqueue *_vq,
@@ -136,7 +138,8 @@ static inline int virtqueue_add(struct virtqueue *_vq,
 				unsigned int out_sgs,
 				unsigned int in_sgs,
 				void *data,
-				gfp_t gfp)
+				gfp_t gfp,
+				bool dma)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
 	struct scatterlist *sg;
@@ -174,7 +177,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
 
 	/* If the host supports indirect descriptor tables, and we have multiple
 	 * buffers, then go indirect. FIXME: tune this threshold */
-	if (vq->indirect && total_sg > 1 && vq->vq.num_free)
+	if (!dma && vq->indirect && total_sg > 1 &&
vq->vq.num_free)
 		desc = alloc_indirect(_vq, total_sg, gfp);
 	else
 		desc = NULL;
@@ -216,7 +219,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
 	for (n = 0; n < out_sgs; n++) {
 		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
 			vring_desc_set(_vq->vdev, desc + i, sg,
-				       VRING_DESC_F_NEXT);
+				       VRING_DESC_F_NEXT, dma);
 			prev = i;
 			i = virtio16_to_cpu(_vq->vdev, desc[i].next);
 		}
@@ -224,7 +227,8 @@ static inline int virtqueue_add(struct virtqueue *_vq,
 	for (; n < (out_sgs + in_sgs); n++) {
 		for (sg = sgs[n]; sg; sg = sg_next(sg)) {
 			vring_desc_set(_vq->vdev, desc + i, sg,
-				       VRING_DESC_F_NEXT | VRING_DESC_F_WRITE);
+				       VRING_DESC_F_NEXT | VRING_DESC_F_WRITE,
+				       dma);
 			prev = i;
 			i = virtio16_to_cpu(_vq->vdev, desc[i].next);
 		}
@@ -292,7 +296,8 @@ int virtqueue_add_sgs(struct virtqueue *_vq,
 		for (sg = sgs[i]; sg; sg = sg_next(sg))
 			total_sg++;
 	}
-	return virtqueue_add(_vq, sgs, total_sg, out_sgs, in_sgs, data, gfp);
+	return virtqueue_add(_vq, sgs, total_sg, out_sgs, in_sgs, data, gfp,
+			     false);
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_sgs);
 
@@ -314,7 +319,7 @@ int virtqueue_add_outbuf(struct virtqueue *vq,
 			 void *data,
 			 gfp_t gfp)
 {
-	return virtqueue_add(vq, &sg, num, 1, 0, data, gfp);
+	return virtqueue_add(vq, &sg, num, 1, 0, data, gfp, false);
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_outbuf);
 
@@ -336,7 +341,7 @@ int virtqueue_add_inbuf(struct virtqueue *vq,
 			void *data,
 			gfp_t gfp)
 {
-	return virtqueue_add(vq, &sg, num, 0, 1, data, gfp);
+	return virtqueue_add(vq, &sg, num, 0, 1, data, gfp, false);
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_inbuf);
 
-- 
1.9.1

Edgar E. Iglesias

2015-May-01 05:01 UTC

head link

[RFC 3/4] virtio: Add dma variants of virtqueue_add_in and outbuf

From: "Edgar E. Iglesias" <edgar.iglesias at xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias at xilinx.com>
---
 drivers/virtio/virtio_ring.c | 18 ++++++++++++++++++
 include/linux/virtio.h       | 10 ++++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 233f3c6..dd6c640 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -323,6 +323,15 @@ int virtqueue_add_outbuf(struct virtqueue *vq,
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_outbuf);
 
+int dma_virtqueue_add_outbuf(struct virtqueue *vq,
+			     struct scatterlist *sg, unsigned int num,
+			     void *data,
+			     gfp_t gfp)
+{
+	return virtqueue_add(vq, &sg, num, 1, 0, data, gfp, true);
+}
+EXPORT_SYMBOL_GPL(dma_virtqueue_add_outbuf);
+
 /**
  * virtqueue_add_inbuf - expose input buffers to other end
  * @vq: the struct virtqueue we're talking about.
@@ -345,6 +354,15 @@ int virtqueue_add_inbuf(struct virtqueue *vq,
 }
 EXPORT_SYMBOL_GPL(virtqueue_add_inbuf);
 
+int dma_virtqueue_add_inbuf(struct virtqueue *vq,
+			    struct scatterlist *sg, unsigned int num,
+			    void *data,
+			    gfp_t gfp)
+{
+	return virtqueue_add(vq, &sg, num, 0, 1, data, gfp, true);
+}
+EXPORT_SYMBOL_GPL(dma_virtqueue_add_inbuf);
+
 /**
  * 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 8f4d4bf..c31b3e2 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -34,6 +34,16 @@ struct virtqueue {
 	void *priv;
 };
 
+int dma_virtqueue_add_outbuf(struct virtqueue *vq,
+			     struct scatterlist sg[], unsigned int num,
+			     void *data,
+			     gfp_t gfp);
+
+int dma_virtqueue_add_inbuf(struct virtqueue *vq,
+			    struct scatterlist sg[], unsigned int num,
+			    void *data,
+			    gfp_t gfp);
+
 int virtqueue_add_outbuf(struct virtqueue *vq,
 			 struct scatterlist sg[], unsigned int num,
 			 void *data,
-- 
1.9.1

Edgar E. Iglesias

2015-May-01 05:01 UTC

head link

[RFC 4/4] rpmsg: DMA map sgs passed to virtio

From: "Edgar E. Iglesias" <edgar.iglesias at xilinx.com>

Signed-off-by: Edgar E. Iglesias <edgar.iglesias at xilinx.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 73354ee..9ae53a0 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -210,6 +210,22 @@ static void __ept_release(struct kref *kref)
 	kfree(ept);
 }
 
+static inline dma_addr_t msg_dma_address(struct virtproc_info *vrp, void *msg)
+{
+	unsigned long offset = msg - vrp->rbufs;
+
+	return vrp->bufs_dma + offset;
+}
+
+static inline void rpmsg_msg_sg_init(struct virtproc_info *vrp,
+				     struct scatterlist *sg,
+				     void *msg, unsigned int len)
+{
+	sg_init_table(sg, 1);
+	sg_dma_address(sg) = msg_dma_address(vrp, msg);
+	sg_dma_len(sg) = len;
+}
+
 /* for more info, see below documentation of rpmsg_create_ept() */
 static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info *vrp,
 		struct rpmsg_channel *rpdev, rpmsg_rx_cb_t cb,
@@ -754,12 +770,12 @@ int rpmsg_send_offchannel_raw(struct rpmsg_channel *rpdev,
u32 src, u32 dst,
 	print_hex_dump(KERN_DEBUG, "rpmsg_virtio TX: ", DUMP_PREFIX_NONE,
16, 1,
 					msg, sizeof(*msg) + msg->len, true);
 
-	sg_init_one(&sg, msg, sizeof(*msg) + len);
+	rpmsg_msg_sg_init(vrp, &sg, msg, sizeof(*msg) + len);
 
 	mutex_lock(&vrp->tx_lock);
 
 	/* add message to the remote processor's virtqueue */
-	err = virtqueue_add_outbuf(vrp->svq, &sg, 1, msg, GFP_KERNEL);
+	err = dma_virtqueue_add_outbuf(vrp->svq, &sg, 1, msg, GFP_KERNEL);
 	if (err) {
 		/*
 		 * need to reclaim the buffer here, otherwise it's lost
@@ -828,10 +844,10 @@ static int rpmsg_recv_single(struct virtproc_info *vrp,
struct device *dev,
 		dev_warn(dev, "msg received with no recipient\n");
 
 	/* publish the real size of the buffer */
-	sg_init_one(&sg, msg, RPMSG_BUF_SIZE);
+	rpmsg_msg_sg_init(vrp, &sg, msg, RPMSG_BUF_SIZE);
 
 	/* add the buffer back to the remote processor's virtqueue */
-	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
+	err = dma_virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
 	if (err < 0) {
 		dev_err(dev, "failed to add a virtqueue buffer: %d\n", err);
 		return err;
@@ -1007,9 +1023,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		struct scatterlist sg;
 		void *cpu_addr = vrp->rbufs + i * RPMSG_BUF_SIZE;
 
-		sg_init_one(&sg, cpu_addr, RPMSG_BUF_SIZE);
+		rpmsg_msg_sg_init(vrp, &sg, cpu_addr, RPMSG_BUF_SIZE);
 
-		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
+		err = dma_virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
 								GFP_KERNEL);
 		WARN_ON(err); /* sanity check; this can't really happen */
 	}
-- 
1.9.1

Rusty Russell

2015-May-06 06:21 UTC

head link

[RFC 4/4] rpmsg: DMA map sgs passed to virtio

"Edgar E. Iglesias" <edgar.iglesias at gmail.com>
writes:> From: "Edgar E. Iglesias" <edgar.iglesias at xilinx.com>
>
> Signed-off-by: Edgar E. Iglesias <edgar.iglesias at xilinx.com>
First off, I have handed maintainership off to Michael S. Tsirkin, so
his word is now law.

That said... there's nothing fundamentally *wrong* with this, but it's
not how standard virtio works.  We decided some time ago that as we're
paravirtualized, we would not be doing address mapping.

rpmsg uses virtio, but it's with a twist: they're not talking to a
host.  Thus my preference, in order, would be:

1) Don't use non-kmalloc addresses.
2) If that's not possible, call these _dma interfaces _rpmsg instead,
   so normal virtio users don't get confused and try to use them.

Cheers,
Rusty.

> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 28 ++++++++++++++++++++++------
>  1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c
b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 73354ee..9ae53a0 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -210,6 +210,22 @@ static void __ept_release(struct kref *kref)
>  	kfree(ept);
>  }
>  
> +static inline dma_addr_t msg_dma_address(struct virtproc_info *vrp, void
*msg)
> +{
> +	unsigned long offset = msg - vrp->rbufs;
> +
> +	return vrp->bufs_dma + offset;
> +}
> +
> +static inline void rpmsg_msg_sg_init(struct virtproc_info *vrp,
> +				     struct scatterlist *sg,
> +				     void *msg, unsigned int len)
> +{
> +	sg_init_table(sg, 1);
> +	sg_dma_address(sg) = msg_dma_address(vrp, msg);
> +	sg_dma_len(sg) = len;
> +}
> +
>  /* for more info, see below documentation of rpmsg_create_ept() */
>  static struct rpmsg_endpoint *__rpmsg_create_ept(struct virtproc_info
*vrp,
>  		struct rpmsg_channel *rpdev, rpmsg_rx_cb_t cb,
> @@ -754,12 +770,12 @@ int rpmsg_send_offchannel_raw(struct rpmsg_channel
*rpdev, u32 src, u32 dst,
>  	print_hex_dump(KERN_DEBUG, "rpmsg_virtio TX: ",
DUMP_PREFIX_NONE, 16, 1,
>  					msg, sizeof(*msg) + msg->len, true);
>  
> -	sg_init_one(&sg, msg, sizeof(*msg) + len);
> +	rpmsg_msg_sg_init(vrp, &sg, msg, sizeof(*msg) + len);
>  
>  	mutex_lock(&vrp->tx_lock);
>  
>  	/* add message to the remote processor's virtqueue */
> -	err = virtqueue_add_outbuf(vrp->svq, &sg, 1, msg, GFP_KERNEL);
> +	err = dma_virtqueue_add_outbuf(vrp->svq, &sg, 1, msg, GFP_KERNEL);
>  	if (err) {
>  		/*
>  		 * need to reclaim the buffer here, otherwise it's lost
> @@ -828,10 +844,10 @@ static int rpmsg_recv_single(struct virtproc_info
*vrp, struct device *dev,
>  		dev_warn(dev, "msg received with no recipient\n");
>  
>  	/* publish the real size of the buffer */
> -	sg_init_one(&sg, msg, RPMSG_BUF_SIZE);
> +	rpmsg_msg_sg_init(vrp, &sg, msg, RPMSG_BUF_SIZE);
>  
>  	/* add the buffer back to the remote processor's virtqueue */
> -	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> +	err = dma_virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>  	if (err < 0) {
>  		dev_err(dev, "failed to add a virtqueue buffer: %d\n", err);
>  		return err;
> @@ -1007,9 +1023,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  		struct scatterlist sg;
>  		void *cpu_addr = vrp->rbufs + i * RPMSG_BUF_SIZE;
>  
> -		sg_init_one(&sg, cpu_addr, RPMSG_BUF_SIZE);
> +		rpmsg_msg_sg_init(vrp, &sg, cpu_addr, RPMSG_BUF_SIZE);
>  
> -		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> +		err = dma_virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>  								GFP_KERNEL);
>  		WARN_ON(err); /* sanity check; this can't really happen */
>  	}
> -- 
> 1.9.1

Reasonably Related Threads

Search for more possibly parallel threads

Linux Virtualization - May 2015 - [RFC 0/4] rpmsg: Fix init of DMA:able virtqueues

[RFC 0/4] rpmsg: Fix init of DMA:able virtqueues

[RFC 1/4] virtio_ring: Break out vring descriptor setup code

[RFC 2/4] virtio_ring: Add option for DMA mapped sgs in virtqueue_add

[RFC 3/4] virtio: Add dma variants of virtqueue_add_in and outbuf

[RFC 4/4] rpmsg: DMA map sgs passed to virtio

[RFC 4/4] rpmsg: DMA map sgs passed to virtio

Reasonably Related Threads