[PATCH 0/2] Virtio ring works with DMA coherent memory

[PATCH 0/2] Virtio ring works with DMA coherent memory

[Wendy Liang]

RPMsg uses dma_alloc_coherent() to allocate memory to shared with the remote.
In this case, as there is no pages setup in the dma_alloc_coherent(),
we cannot get the physical address back from the virtual address, and thus,
we can set the sg_dma_addr to store the DMA address and mark it already DMA
mapped.

When virtio vring sees the sg_dma_addr is ready set, do not call dma_map_page().

The issue was once discussed here:
http://virtualization.linux-foundation.narkive.com/CfVP32Vy/rfc-0-4-rpmsg-fix-init-of-dma-able-virtqueues

Edgar E. Iglesias (1):
  rpmsg: DMA map sgs passed to virtio

Wendy Liang (1):
  virtio_ring: Do not call dma_map_page if sg is already mapped.

 drivers/rpmsg/virtio_rpmsg_bus.c | 22 +++++++++++++++++++---
 drivers/virtio/virtio_ring.c     |  6 ++++++
 2 files changed, 25 insertions(+), 3 deletions(-)

-- 
1.9.1

[PATCH 1/2] virtio_ring: Do not call dma_map_page if sg is already mapped.

[Wendy Liang]

If sg is already dma mapped, do not call dma_map_page() in
vring_map_one_sg().

In case of rpmsg, rpmsg uses dma_alloc_coherent() to allocate
memory to share with the remote. There is no pages setup
in dma_alloc_coherent().

In this case, we cannot convert the virtual address back to the
physical address. In this case, we can setup the sg_dma_addr to
store the DMA address, and also mark the sg is already mapped.

In the vring, we can detect if the address is already mapped
by checking the sg_dma_addr. If yes, do not call dma_map_page().

Signed-off-by: Wendy Liang <jliang@xilinx.com>
---
 drivers/virtio/virtio_ring.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 489bfc6..9793e1f 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -180,6 +180,12 @@ static dma_addr_t vring_map_one_sg(const struct vring_virtqueue *vq,
 	if (!vring_use_dma_api(vq->vq.vdev))
 		return (dma_addr_t)sg_phys(sg);
 
+	/* If the sg is already mapped, return the DMA address */
+	if (sg_dma_address(sg)) {
+		sg->length = sg_dma_len(sg);
+		return sg_dma_address(sg);
+	}
+
 	/*
 	 * We can't use dma_map_sg, because we don't use scatterlists in
 	 * the way it expects (we don't guarantee that the scatterlist
-- 
1.9.1

Re: [PATCH 1/2] virtio_ring: Do not call dma_map_page if sg is already mapped.

[Michael S. Tsirkin]

On Wed, Dec 07, 2016 at 10:59:12PM -0800, Wendy Liang wrote:
> If sg is already dma mapped, do not call dma_map_page() in
> vring_map_one_sg().
> 
> In case of rpmsg, rpmsg uses dma_alloc_coherent() to allocate
> memory to share with the remote. There is no pages setup
> in dma_alloc_coherent().
> 
> In this case, we cannot convert the virtual address back to the
> physical address. In this case, we can setup the sg_dma_addr to
> store the DMA address, and also mark the sg is already mapped.
> 
> In the vring, we can detect if the address is already mapped
> by checking the sg_dma_addr. If yes, do not call dma_map_page().
> 
> Signed-off-by: Wendy Liang <jliang@xilinx.com>
> ---
>  drivers/virtio/virtio_ring.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 489bfc6..9793e1f 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -180,6 +180,12 @@ static dma_addr_t vring_map_one_sg(const struct vring_virtqueue *vq,
>  	if (!vring_use_dma_api(vq->vq.vdev))
>  		return (dma_addr_t)sg_phys(sg);
>  
> +	/* If the sg is already mapped, return the DMA address */

How come we even reach this code for rpmsg?
Does vring_use_dma_api return true for rpmsg?

> +	if (sg_dma_address(sg)) {
> +		sg->length = sg_dma_len(sg);
> +		return sg_dma_address(sg);
> +	}
> +

Is there a rule that says 0 is not a valid address?

>  	/*
>  	 * We can't use dma_map_sg, because we don't use scatterlists in
>  	 * the way it expects (we don't guarantee that the scatterlist
> -- 
> 1.9.1

Re: [PATCH 1/2] virtio_ring: Do not call dma_map_page if sg is already mapped.

[Michael S. Tsirkin]

On Wed, Dec 07, 2016 at 10:59:12PM -0800, Wendy Liang wrote:
> If sg is already dma mapped, do not call dma_map_page() in
> vring_map_one_sg().
> 
> In case of rpmsg, rpmsg uses dma_alloc_coherent() to allocate
> memory to share with the remote. There is no pages setup
> in dma_alloc_coherent().
> 
> In this case, we cannot convert the virtual address back to the
> physical address. In this case, we can setup the sg_dma_addr to
> store the DMA address, and also mark the sg is already mapped.
> 
> In the vring, we can detect if the address is already mapped
> by checking the sg_dma_addr. If yes, do not call dma_map_page().
> 
> Signed-off-by: Wendy Liang <jliang@xilinx.com>
> ---
>  drivers/virtio/virtio_ring.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 489bfc6..9793e1f 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -180,6 +180,12 @@ static dma_addr_t vring_map_one_sg(const struct vring_virtqueue *vq,
>  	if (!vring_use_dma_api(vq->vq.vdev))
>  		return (dma_addr_t)sg_phys(sg);
>  
> +	/* If the sg is already mapped, return the DMA address */

How come we even reach this code for rpmsg?
Does vring_use_dma_api return true for rpmsg?

> +	if (sg_dma_address(sg)) {
> +		sg->length = sg_dma_len(sg);
> +		return sg_dma_address(sg);
> +	}
> +

Is there a rule that says 0 is not a valid address?

>  	/*
>  	 * We can't use dma_map_sg, because we don't use scatterlists in
>  	 * the way it expects (we don't guarantee that the scatterlist
> -- 
> 1.9.1

Re: [PATCH 1/2] virtio_ring: Do not call dma_map_page if sg is already mapped.

[Wendy Liang]

HI Michael,

On Thu, Dec 8, 2016 at 8:46 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Dec 07, 2016 at 10:59:12PM -0800, Wendy Liang wrote:
>> If sg is already dma mapped, do not call dma_map_page() in
>> vring_map_one_sg().
>>
>> In case of rpmsg, rpmsg uses dma_alloc_coherent() to allocate
>> memory to share with the remote. There is no pages setup
>> in dma_alloc_coherent().
>>
>> In this case, we cannot convert the virtual address back to the
>> physical address. In this case, we can setup the sg_dma_addr to
>> store the DMA address, and also mark the sg is already mapped.
>>
>> In the vring, we can detect if the address is already mapped
>> by checking the sg_dma_addr. If yes, do not call dma_map_page().
>>
>> Signed-off-by: Wendy Liang <jliang@xilinx.com>
>> ---
>>  drivers/virtio/virtio_ring.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index 489bfc6..9793e1f 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -180,6 +180,12 @@ static dma_addr_t vring_map_one_sg(const struct vring_virtqueue *vq,
>>       if (!vring_use_dma_api(vq->vq.vdev))
>>               return (dma_addr_t)sg_phys(sg);
>>
>> +     /* If the sg is already mapped, return the DMA address */
>
> How come we even reach this code for rpmsg?
> Does vring_use_dma_api return true for rpmsg?
I used vdev feature bit VIRTIO_F_IOMMU_PLATFORM.

>
>> +     if (sg_dma_address(sg)) {
>> +             sg->length = sg_dma_len(sg);
>> +             return sg_dma_address(sg);
>> +     }
>> +
>
> Is there a rule that says 0 is not a valid address?
OK, i see. Is it better to check the sg_dma_len() instead?

However, I just noticed yesterday, there is another patch to
virtio_rpmsg_bus posted to linux-remoteproc mailing list trying to
solve the same issue:
[PATCH v1 2/6] rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is
not a valid kernel address. he didn't use sg_dma_address. And doesn't
require to update the virtio_ring implementation.


>
>>       /*
>>        * We can't use dma_map_sg, because we don't use scatterlists in
>>        * the way it expects (we don't guarantee that the scatterlist
>> --
>> 1.9.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] virtio_ring: Do not call dma_map_page if sg is already mapped.

[Wendy Liang]

HI Michael,

On Thu, Dec 8, 2016 at 8:46 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Wed, Dec 07, 2016 at 10:59:12PM -0800, Wendy Liang wrote:
>> If sg is already dma mapped, do not call dma_map_page() in
>> vring_map_one_sg().
>>
>> In case of rpmsg, rpmsg uses dma_alloc_coherent() to allocate
>> memory to share with the remote. There is no pages setup
>> in dma_alloc_coherent().
>>
>> In this case, we cannot convert the virtual address back to the
>> physical address. In this case, we can setup the sg_dma_addr to
>> store the DMA address, and also mark the sg is already mapped.
>>
>> In the vring, we can detect if the address is already mapped
>> by checking the sg_dma_addr. If yes, do not call dma_map_page().
>>
>> Signed-off-by: Wendy Liang <jliang@xilinx.com>
>> ---
>>  drivers/virtio/virtio_ring.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
>> index 489bfc6..9793e1f 100644
>> --- a/drivers/virtio/virtio_ring.c
>> +++ b/drivers/virtio/virtio_ring.c
>> @@ -180,6 +180,12 @@ static dma_addr_t vring_map_one_sg(const struct vring_virtqueue *vq,
>>       if (!vring_use_dma_api(vq->vq.vdev))
>>               return (dma_addr_t)sg_phys(sg);
>>
>> +     /* If the sg is already mapped, return the DMA address */
>
> How come we even reach this code for rpmsg?
> Does vring_use_dma_api return true for rpmsg?
I used vdev feature bit VIRTIO_F_IOMMU_PLATFORM.

>
>> +     if (sg_dma_address(sg)) {
>> +             sg->length = sg_dma_len(sg);
>> +             return sg_dma_address(sg);
>> +     }
>> +
>
> Is there a rule that says 0 is not a valid address?
OK, i see. Is it better to check the sg_dma_len() instead?

However, I just noticed yesterday, there is another patch to
virtio_rpmsg_bus posted to linux-remoteproc mailing list trying to
solve the same issue:
[PATCH v1 2/6] rpmsg: virtio_rpmsg_bus: fix sg_set_buf() when addr is
not a valid kernel address. he didn't use sg_dma_address. And doesn't
require to update the virtio_ring implementation.


>
>>       /*
>>        * We can't use dma_map_sg, because we don't use scatterlists in
>>        * the way it expects (we don't guarantee that the scatterlist
>> --
>> 1.9.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] rpmsg: DMA map sgs passed to virtio

[Wendy Liang]

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

As rpmsg uses dma_alloc_coherent() to allocate memory to shared
with the remote. Virtio ring requires the shared buffers to be
passed as sg struct. As the memory has already been mapped,
and we cannot convert the virtual address got from dma_alloc_coherent()
back to the physical address. We set the sg_dma_addr to store the
DMA address before we pass it to virtio.

Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
Signed-off-by: Wendy Liang <jliang@xilinx.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 3090b0d..af76187 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -192,6 +192,22 @@ static int virtio_rpmsg_trysend_offchannel(struct rpmsg_endpoint *ept, u32 src,
 	.trysend_offchannel = virtio_rpmsg_trysend_offchannel,
 };
 
+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;
+}
+
 /**
  * __ept_release() - deallocate an rpmsg endpoint
  * @kref: the ept's reference count
@@ -604,7 +620,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
 			 msg, sizeof(*msg) + msg->len, true);
 #endif
 
-	sg_init_one(&sg, msg, sizeof(*msg) + len);
+	rpmsg_msg_sg_init(vrp, &sg, msg, sizeof(*msg) + len);
 
 	mutex_lock(&vrp->tx_lock);
 
@@ -729,7 +745,7 @@ 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);
@@ -911,7 +927,7 @@ 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,
 					  GFP_KERNEL);
-- 
1.9.1