[PATCH 0/5] Add noncoherent platform support for vop driver

[PATCH 0/5] Add noncoherent platform support for vop driver

[Sherry Sun]

Change the way of allocating vring to support noncoherent platform for vop
driver, and add some related dma changes to make sure noncoherent platform works
well. 

Sherry Sun (5):
  misc: vop: change the way of allocating vring for noncoherent platform
  misc: vop: change the way of allocating used ring
  misc: vop: simply return the saved dma address instead of virt_to_phys
  misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform
  misc: vop: mapping kernel memory to user space as noncached

 drivers/misc/mic/bus/vop_bus.h    |   2 +
 drivers/misc/mic/host/mic_boot.c  |   8 ++
 drivers/misc/mic/vop/vop_main.c   |  51 +++++++++----
 drivers/misc/mic/vop/vop_vringh.c | 117 ++++++++++++++++++++----------
 4 files changed, 125 insertions(+), 53 deletions(-)

-- 
2.17.1

[PATCH 1/5] misc: vop: change the way of allocating vring for noncoherent platform

[Sherry Sun]

For noncoherent platform, we should allocate vring through
dma_alloc_coherent api to ensure timely synchronization of vring.
The orginal way which used __get_free_pages and dma_map_single only
apply to coherent platforms.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/misc/mic/vop/vop_vringh.c | 101 ++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 34 deletions(-)

diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c
index f344209ac386..fc8d8ff9ded3 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -9,6 +9,7 @@
 #include <linux/sched.h>
 #include <linux/poll.h>
 #include <linux/dma-mapping.h>
+#include <linux/dma-noncoherent.h>
 
 #include <linux/mic_common.h>
 #include "../common/mic_dev.h"
@@ -67,11 +68,12 @@ static void vop_virtio_init_post(struct vop_vdev *vdev)
 			dev_warn(vop_dev(vdev), "used_address zero??\n");
 			continue;
 		}
-		vdev->vvr[i].vrh.vring.used =
-			(void __force *)vpdev->hw_ops->remap(
-			vpdev,
-			le64_to_cpu(vqconfig[i].used_address),
-			used_size);
+		if (dev_is_dma_coherent(vop_dev(vdev)))
+			vdev->vvr[i].vrh.vring.used =
+				(void __force *)vpdev->hw_ops->remap(
+				vpdev,
+				le64_to_cpu(vqconfig[i].used_address),
+				used_size);
 	}
 
 	vdev->dc->used_address_updated = 0;
@@ -298,9 +300,14 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
 		mutex_init(&vvr->vr_mutex);
 		vr_size = PAGE_ALIGN(round_up(vring_size(num, MIC_VIRTIO_RING_ALIGN), 4) +
 			sizeof(struct _mic_vring_info));
-		vr->va = (void *)
-			__get_free_pages(GFP_KERNEL | __GFP_ZERO,
-					 get_order(vr_size));
+
+		if (!dev_is_dma_coherent(vop_dev(vdev)))
+			vr->va = dma_alloc_coherent(vop_dev(vdev), vr_size,
+						    &vr_addr, GFP_KERNEL);
+		else
+			vr->va = (void *)
+				__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+						 get_order(vr_size));
 		if (!vr->va) {
 			ret = -ENOMEM;
 			dev_err(vop_dev(vdev), "%s %d err %d\n",
@@ -310,14 +317,17 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
 		vr->len = vr_size;
 		vr->info = vr->va + round_up(vring_size(num, MIC_VIRTIO_RING_ALIGN), 4);
 		vr->info->magic = cpu_to_le32(MIC_MAGIC + vdev->virtio_id + i);
-		vr_addr = dma_map_single(&vpdev->dev, vr->va, vr_size,
-					 DMA_BIDIRECTIONAL);
-		if (dma_mapping_error(&vpdev->dev, vr_addr)) {
-			free_pages((unsigned long)vr->va, get_order(vr_size));
-			ret = -ENOMEM;
-			dev_err(vop_dev(vdev), "%s %d err %d\n",
-				__func__, __LINE__, ret);
-			goto err;
+
+		if (dev_is_dma_coherent(vop_dev(vdev))) {
+			vr_addr = dma_map_single(&vpdev->dev, vr->va, vr_size,
+						 DMA_BIDIRECTIONAL);
+			if (dma_mapping_error(&vpdev->dev, vr_addr)) {
+				free_pages((unsigned long)vr->va, get_order(vr_size));
+				ret = -ENOMEM;
+				dev_err(vop_dev(vdev), "%s %d err %d\n",
+						__func__, __LINE__, ret);
+				goto err;
+			}
 		}
 		vqconfig[i].address = cpu_to_le64(vr_addr);
 
@@ -339,11 +349,17 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
 		dev_dbg(&vpdev->dev,
 			"%s %d index %d va %p info %p vr_size 0x%x\n",
 			__func__, __LINE__, i, vr->va, vr->info, vr_size);
-		vvr->buf = (void *)__get_free_pages(GFP_KERNEL,
-					get_order(VOP_INT_DMA_BUF_SIZE));
-		vvr->buf_da = dma_map_single(&vpdev->dev,
-					  vvr->buf, VOP_INT_DMA_BUF_SIZE,
-					  DMA_BIDIRECTIONAL);
+
+		if (!dev_is_dma_coherent(vop_dev(vdev)))
+			vvr->buf = dma_alloc_coherent(vop_dev(vdev), VOP_INT_DMA_BUF_SIZE,
+						      &vvr->buf_da, GFP_KERNEL);
+		else {
+			vvr->buf = (void *)__get_free_pages(GFP_KERNEL,
+							    get_order(VOP_INT_DMA_BUF_SIZE));
+			vvr->buf_da = dma_map_single(&vpdev->dev,
+						     vvr->buf, VOP_INT_DMA_BUF_SIZE,
+						     DMA_BIDIRECTIONAL);
+		}
 	}
 
 	snprintf(irqname, sizeof(irqname), "vop%dvirtio%d", vpdev->index,
@@ -382,10 +398,15 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
 	for (j = 0; j < i; j++) {
 		struct vop_vringh *vvr = &vdev->vvr[j];
 
-		dma_unmap_single(&vpdev->dev, le64_to_cpu(vqconfig[j].address),
-				 vvr->vring.len, DMA_BIDIRECTIONAL);
-		free_pages((unsigned long)vvr->vring.va,
-			   get_order(vvr->vring.len));
+		if (!dev_is_dma_coherent(vop_dev(vdev)))
+			dma_free_coherent(vop_dev(vdev), vvr->vring.len, vvr->vring.va,
+					  le64_to_cpu(vqconfig[j].address));
+		else {
+			dma_unmap_single(&vpdev->dev, le64_to_cpu(vqconfig[j].address),
+					 vvr->vring.len, DMA_BIDIRECTIONAL);
+			free_pages((unsigned long)vvr->vring.va,
+				   get_order(vvr->vring.len));
+		}
 	}
 	return ret;
 }
@@ -433,17 +454,29 @@ static void vop_virtio_del_device(struct vop_vdev *vdev)
 	for (i = 0; i < vdev->dd->num_vq; i++) {
 		struct vop_vringh *vvr = &vdev->vvr[i];
 
-		dma_unmap_single(&vpdev->dev,
-				 vvr->buf_da, VOP_INT_DMA_BUF_SIZE,
-				 DMA_BIDIRECTIONAL);
-		free_pages((unsigned long)vvr->buf,
-			   get_order(VOP_INT_DMA_BUF_SIZE));
+		if (!dev_is_dma_coherent(vop_dev(vdev)))
+			dma_free_coherent(vop_dev(vdev), VOP_INT_DMA_BUF_SIZE,
+					  vvr->buf, vvr->buf_da);
+		else {
+			dma_unmap_single(&vpdev->dev,
+					 vvr->buf_da, VOP_INT_DMA_BUF_SIZE,
+					 DMA_BIDIRECTIONAL);
+			free_pages((unsigned long)vvr->buf,
+				   get_order(VOP_INT_DMA_BUF_SIZE));
+		}
+
 		vringh_kiov_cleanup(&vvr->riov);
 		vringh_kiov_cleanup(&vvr->wiov);
-		dma_unmap_single(&vpdev->dev, le64_to_cpu(vqconfig[i].address),
-				 vvr->vring.len, DMA_BIDIRECTIONAL);
-		free_pages((unsigned long)vvr->vring.va,
-			   get_order(vvr->vring.len));
+
+		if (!dev_is_dma_coherent(vop_dev(vdev)))
+			dma_free_coherent(vop_dev(vdev), vvr->vring.len, vvr->vring.va,
+					  le64_to_cpu(vqconfig[i].address));
+		else {
+			dma_unmap_single(&vpdev->dev, le64_to_cpu(vqconfig[i].address),
+					 vvr->vring.len, DMA_BIDIRECTIONAL);
+			free_pages((unsigned long)vvr->vring.va,
+				   get_order(vvr->vring.len));
+		}
 	}
 	/*
 	 * Order the type update with previous stores. This write barrier
-- 
2.17.1

[PATCH 2/5] misc: vop: change the way of allocating used ring

[Sherry Sun]

For noncoherent platform, we shouldn't allocate and reassign the used
ring. Since RC has allocated the entire vring, including the used ring.
simply add the corresponding offset can get the used ring address.

If follow the orginal way to reassign the used ring, will encounter a
problem. When host finished with descriptor, it will update the used
ring with putused_kern function, if reassign used ring at EP side, used
ring will be io device memory for RC, use memcpy in putused_kern will
cause kernel panic.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/misc/mic/vop/vop_main.c | 48 ++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index 6722c726b259..d3645122c983 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -18,6 +18,7 @@
 #include <linux/module.h>
 #include <linux/sched.h>
 #include <linux/dma-mapping.h>
+#include <linux/dma-noncoherent.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 
 #include "vop_main.h"
@@ -249,10 +250,13 @@ static void vop_del_vq(struct virtqueue *vq, int n)
 	struct _vop_vdev *vdev = to_vopvdev(vq->vdev);
 	struct vop_device *vpdev = vdev->vpdev;
 
-	dma_unmap_single(&vpdev->dev, vdev->used[n],
-			 vdev->used_size[n], DMA_BIDIRECTIONAL);
-	free_pages((unsigned long)vdev->used_virt[n],
-		   get_order(vdev->used_size[n]));
+	if (dev_is_dma_coherent(_vop_dev(vdev))) {
+		dma_unmap_single(&vpdev->dev, vdev->used[n],
+				 vdev->used_size[n], DMA_BIDIRECTIONAL);
+		free_pages((unsigned long)vdev->used_virt[n],
+			   get_order(vdev->used_size[n]));
+	}
+
 	vring_del_virtqueue(vq);
 	vpdev->hw_ops->unmap(vpdev, vdev->vr[n]);
 	vdev->vr[n] = NULL;
@@ -339,8 +343,13 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev,
 	vdev->used_size[index] = PAGE_ALIGN(sizeof(__u16) * 3 +
 					     sizeof(struct vring_used_elem) *
 					     le16_to_cpu(config.num));
-	used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
-					get_order(vdev->used_size[index]));
+	if (!dev_is_dma_coherent(_vop_dev(vdev)))
+		used = va + PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) +
+				       sizeof(__u16) * (3 + le16_to_cpu(config.num)));
+	else
+		used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO,
+						get_order(vdev->used_size[index]));
+
 	vdev->used_virt[index] = used;
 	if (!used) {
 		err = -ENOMEM;
@@ -357,14 +366,20 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev,
 		goto free_used;
 	}
 
-	vdev->used[index] = dma_map_single(&vpdev->dev, used,
-					    vdev->used_size[index],
-					    DMA_BIDIRECTIONAL);
-	if (dma_mapping_error(&vpdev->dev, vdev->used[index])) {
-		err = -ENOMEM;
-		dev_err(_vop_dev(vdev), "%s %d err %d\n",
-			__func__, __LINE__, err);
-		goto del_vq;
+	if (!dev_is_dma_coherent(_vop_dev(vdev)))
+		vdev->used[index] = config.address +
+				    PAGE_ALIGN(sizeof(struct vring_desc) * le16_to_cpu(config.num) +
+				    sizeof(__u16) * (3 + le16_to_cpu(config.num)));
+	else {
+		vdev->used[index] = dma_map_single(&vpdev->dev, used,
+						   vdev->used_size[index],
+						   DMA_BIDIRECTIONAL);
+		if (dma_mapping_error(&vpdev->dev, vdev->used[index])) {
+			err = -ENOMEM;
+			dev_err(_vop_dev(vdev), "%s %d err %d\n",
+				__func__, __LINE__, err);
+			goto del_vq;
+		}
 	}
 	writeq(vdev->used[index], &vqconfig->used_address);
 
@@ -373,8 +388,9 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev,
 del_vq:
 	vring_del_virtqueue(vq);
 free_used:
-	free_pages((unsigned long)used,
-		   get_order(vdev->used_size[index]));
+	if (dev_is_dma_coherent(_vop_dev(vdev)))
+		free_pages((unsigned long)used,
+			   get_order(vdev->used_size[index]));
 unmap:
 	vpdev->hw_ops->unmap(vpdev, vdev->vr[index]);
 	return ERR_PTR(err);
-- 
2.17.1

[PATCH 3/5] misc: vop: simply return the saved dma address instead of virt_to_phys

[Sherry Sun]

For noncoherent platform, the device page and vring should allocated by
dma_alloc_coherent, when user space wants to get its physical address,
virt_to_phys cannot be used, should simply return the saved device page
dma address by get_dp_dma callback and the vring dma address saved
in mic_vqconfig.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/misc/mic/bus/vop_bus.h    |  2 ++
 drivers/misc/mic/host/mic_boot.c  |  8 ++++++++
 drivers/misc/mic/vop/vop_vringh.c | 12 ++++++++++--
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/mic/bus/vop_bus.h b/drivers/misc/mic/bus/vop_bus.h
index 4fa02808c1e2..d891eacae358 100644
--- a/drivers/misc/mic/bus/vop_bus.h
+++ b/drivers/misc/mic/bus/vop_bus.h
@@ -75,6 +75,7 @@ struct vop_driver {
  *                 node to add/remove/configure virtio devices.
  * @get_dp: Get access to the virtio device page used by the self
  *          node to add/remove/configure virtio devices.
+ * @get_dp_dma: Get dma address of the virtio device page.
  * @send_intr: Send an interrupt to the peer node on a specified doorbell.
  * @remap: Map a buffer with the specified DMA address and length.
  * @unmap: Unmap a buffer previously mapped.
@@ -92,6 +93,7 @@ struct vop_hw_ops {
 	void (*ack_interrupt)(struct vop_device *vpdev, int num);
 	void __iomem * (*get_remote_dp)(struct vop_device *vpdev);
 	void * (*get_dp)(struct vop_device *vpdev);
+	dma_addr_t (*get_dp_dma)(struct vop_device *vpdev);
 	void (*send_intr)(struct vop_device *vpdev, int db);
 	void __iomem * (*remap)(struct vop_device *vpdev,
 				  dma_addr_t pa, size_t len);
diff --git a/drivers/misc/mic/host/mic_boot.c b/drivers/misc/mic/host/mic_boot.c
index fb5b3989753d..ced03662cd8f 100644
--- a/drivers/misc/mic/host/mic_boot.c
+++ b/drivers/misc/mic/host/mic_boot.c
@@ -88,6 +88,13 @@ static void *__mic_get_dp(struct vop_device *vpdev)
 	return mdev->dp;
 }
 
+static dma_addr_t __mic_get_dp_dma(struct vop_device *vpdev)
+{
+	struct mic_device *mdev = vpdev_to_mdev(&vpdev->dev);
+
+	return mdev->dp_dma_addr;
+}
+
 static void __iomem *__mic_get_remote_dp(struct vop_device *vpdev)
 {
 	return NULL;
@@ -119,6 +126,7 @@ static struct vop_hw_ops vop_hw_ops = {
 	.ack_interrupt = __mic_ack_interrupt,
 	.next_db = __mic_next_db,
 	.get_dp = __mic_get_dp,
+	.get_dp_dma = __mic_get_dp_dma,
 	.get_remote_dp = __mic_get_remote_dp,
 	.send_intr = __mic_send_intr,
 	.remap = __mic_ioremap,
diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c
index fc8d8ff9ded3..aa2dd240c11e 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -1076,6 +1076,7 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset,
 		 unsigned long *size, unsigned long *pa)
 {
 	struct vop_device *vpdev = vdev->vpdev;
+	struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd);
 	unsigned long start = MIC_DP_SIZE;
 	int i;
 
@@ -1088,7 +1089,14 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset,
 	 * ....
 	 */
 	if (!offset) {
-		*pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev));
+		if (vpdev->hw_ops->get_dp_dma)
+			*pa = vpdev->hw_ops->get_dp_dma(vpdev);
+		else {
+			dev_err(vop_dev(vdev), "can't get device page physical address\n");
+			WARN_ON(1);
+			return -1;
+		}
+
 		*size = MIC_DP_SIZE;
 		return 0;
 	}
@@ -1097,7 +1105,7 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset,
 		struct vop_vringh *vvr = &vdev->vvr[i];
 
 		if (offset == start) {
-			*pa = virt_to_phys(vvr->vring.va);
+			*pa = vqconfig[i].address;
 			*size = vvr->vring.len;
 			return 0;
 		}
-- 
2.17.1

[PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform

[Sherry Sun]

Set VIRTIO_F_ACCESS_PLATFORM feature for nocoherent platform, since
it needs the DMA API for virtio.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/misc/mic/vop/vop_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index d3645122c983..d609f0dc6124 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -125,6 +125,9 @@ static void vop_transport_features(struct virtio_device *vdev)
 	 * creates virtio rings on preallocated memory.
 	 */
 	__virtio_clear_bit(vdev, VIRTIO_F_RING_PACKED);
+
+	if (!dev_is_dma_coherent(vdev->dev.parent))
+		__virtio_set_bit(vdev, VIRTIO_F_ACCESS_PLATFORM);
 }
 
 static int vop_finalize_features(struct virtio_device *vdev)
-- 
2.17.1

[PATCH 5/5] misc: vop: mapping kernel memory to user space as noncached

[Sherry Sun]

Mapping kernel space memory to user space as noncached for noncoherent
platform, since user space need check avail_idx and device page flags
timely.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/misc/mic/vop/vop_vringh.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c
index aa2dd240c11e..9850515e8d21 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -1139,7 +1139,9 @@ static int vop_mmap(struct file *f, struct vm_area_struct *vma)
 		}
 		err = remap_pfn_range(vma, vma->vm_start + offset,
 				      pa >> PAGE_SHIFT, size,
-				      vma->vm_page_prot);
+				      dev_is_dma_coherent(vop_dev(vdev)) ? vma->vm_page_prot :
+							  pgprot_noncached(vma->vm_page_prot));
+
 		if (err)
 			goto ret;
 		size_rem -= size;
-- 
2.17.1

Re: [PATCH 0/5] Add noncoherent platform support for vop driver

[Arnd Bergmann]

On Fri, Sep 25, 2020 at 9:27 AM Sherry Sun <sherry.sun@nxp.com> wrote:
>
> Change the way of allocating vring to support noncoherent platform for vop
> driver, and add some related dma changes to make sure noncoherent platform works
> well.

Could you describe why you are doing this? Are you using Intel MIC
devices on Arm hosts, or trying to reuse the code for other add-on
cards?

Note that we have a couple of frameworks in the kernel that try to
do some of the same things here, notably the NTB drivers and the
PCI endpoint support, both of which are designed to be somewhat
more generic than the MIC driver.

Have you considered using that instead?

         Arnd

> Sherry Sun (5):
>   misc: vop: change the way of allocating vring for noncoherent platform
>   misc: vop: change the way of allocating used ring
>   misc: vop: simply return the saved dma address instead of virt_to_phys
>   misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform
>   misc: vop: mapping kernel memory to user space as noncached
>
>  drivers/misc/mic/bus/vop_bus.h    |   2 +
>  drivers/misc/mic/host/mic_boot.c  |   8 ++
>  drivers/misc/mic/vop/vop_main.c   |  51 +++++++++----
>  drivers/misc/mic/vop/vop_vringh.c | 117 ++++++++++++++++++++----------
>  4 files changed, 125 insertions(+), 53 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH 1/5] misc: vop: change the way of allocating vring for noncoherent platform

[Greg KH]

On Fri, Sep 25, 2020 at 03:26:26PM +0800, Sherry Sun wrote:
> For noncoherent platform, we should allocate vring through
> dma_alloc_coherent api to ensure timely synchronization of vring.
> The orginal way which used __get_free_pages and dma_map_single only
> apply to coherent platforms.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> ---
>  drivers/misc/mic/vop/vop_vringh.c | 101 ++++++++++++++++++++----------
>  1 file changed, 67 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c
> index f344209ac386..fc8d8ff9ded3 100644
> --- a/drivers/misc/mic/vop/vop_vringh.c
> +++ b/drivers/misc/mic/vop/vop_vringh.c
> @@ -9,6 +9,7 @@
>  #include <linux/sched.h>
>  #include <linux/poll.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/dma-noncoherent.h>
>  
>  #include <linux/mic_common.h>
>  #include "../common/mic_dev.h"
> @@ -67,11 +68,12 @@ static void vop_virtio_init_post(struct vop_vdev *vdev)
>  			dev_warn(vop_dev(vdev), "used_address zero??\n");
>  			continue;
>  		}
> -		vdev->vvr[i].vrh.vring.used =
> -			(void __force *)vpdev->hw_ops->remap(
> -			vpdev,
> -			le64_to_cpu(vqconfig[i].used_address),
> -			used_size);
> +		if (dev_is_dma_coherent(vop_dev(vdev)))
> +			vdev->vvr[i].vrh.vring.used =
> +				(void __force *)vpdev->hw_ops->remap(
> +				vpdev,
> +				le64_to_cpu(vqconfig[i].used_address),
> +				used_size);

That's really hard to read, don't you agree?  We can go longer than 80
columns now, and why the __force?



>  	}
>  
>  	vdev->dc->used_address_updated = 0;
> @@ -298,9 +300,14 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
>  		mutex_init(&vvr->vr_mutex);
>  		vr_size = PAGE_ALIGN(round_up(vring_size(num, MIC_VIRTIO_RING_ALIGN), 4) +
>  			sizeof(struct _mic_vring_info));
> -		vr->va = (void *)
> -			__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> -					 get_order(vr_size));
> +
> +		if (!dev_is_dma_coherent(vop_dev(vdev)))
> +			vr->va = dma_alloc_coherent(vop_dev(vdev), vr_size,
> +						    &vr_addr, GFP_KERNEL);

Are you sure this is correct?  If dev_is_dma_coherent is NOT true, then
you call dma_alloc_coherent()?


> +		else
> +			vr->va = (void *)
> +				__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> +						 get_order(vr_size));
>  		if (!vr->va) {
>  			ret = -ENOMEM;
>  			dev_err(vop_dev(vdev), "%s %d err %d\n",
> @@ -310,14 +317,17 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
>  		vr->len = vr_size;
>  		vr->info = vr->va + round_up(vring_size(num, MIC_VIRTIO_RING_ALIGN), 4);
>  		vr->info->magic = cpu_to_le32(MIC_MAGIC + vdev->virtio_id + i);
> -		vr_addr = dma_map_single(&vpdev->dev, vr->va, vr_size,
> -					 DMA_BIDIRECTIONAL);
> -		if (dma_mapping_error(&vpdev->dev, vr_addr)) {
> -			free_pages((unsigned long)vr->va, get_order(vr_size));
> -			ret = -ENOMEM;
> -			dev_err(vop_dev(vdev), "%s %d err %d\n",
> -				__func__, __LINE__, ret);
> -			goto err;
> +
> +		if (dev_is_dma_coherent(vop_dev(vdev))) {
> +			vr_addr = dma_map_single(&vpdev->dev, vr->va, vr_size,
> +						 DMA_BIDIRECTIONAL);
> +			if (dma_mapping_error(&vpdev->dev, vr_addr)) {
> +				free_pages((unsigned long)vr->va, get_order(vr_size));
> +				ret = -ENOMEM;
> +				dev_err(vop_dev(vdev), "%s %d err %d\n",
> +						__func__, __LINE__, ret);
> +				goto err;
> +			}

What about if this is not true?

Same for other places in this patch.

thanks,

greg k-h

Re: [PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform

[Greg KH]

On Fri, Sep 25, 2020 at 03:26:29PM +0800, Sherry Sun wrote:
> Set VIRTIO_F_ACCESS_PLATFORM feature for nocoherent platform, since
> it needs the DMA API for virtio.
> 
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/misc/mic/vop/vop_main.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
> index d3645122c983..d609f0dc6124 100644
> --- a/drivers/misc/mic/vop/vop_main.c
> +++ b/drivers/misc/mic/vop/vop_main.c
> @@ -125,6 +125,9 @@ static void vop_transport_features(struct virtio_device *vdev)
>  	 * creates virtio rings on preallocated memory.
>  	 */
>  	__virtio_clear_bit(vdev, VIRTIO_F_RING_PACKED);
> +
> +	if (!dev_is_dma_coherent(vdev->dev.parent))
> +		__virtio_set_bit(vdev, VIRTIO_F_ACCESS_PLATFORM);

Why look at the parent and not the device itself?

Re: [PATCH 3/5] misc: vop: simply return the saved dma address instead of virt_to_phys

[Greg KH]

On Fri, Sep 25, 2020 at 03:26:28PM +0800, Sherry Sun wrote:
> For noncoherent platform, the device page and vring should allocated by
> dma_alloc_coherent, when user space wants to get its physical address,
> virt_to_phys cannot be used, should simply return the saved device page
> dma address by get_dp_dma callback and the vring dma address saved
> in mic_vqconfig.
> 
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/misc/mic/bus/vop_bus.h    |  2 ++
>  drivers/misc/mic/host/mic_boot.c  |  8 ++++++++
>  drivers/misc/mic/vop/vop_vringh.c | 12 ++++++++++--
>  3 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/mic/bus/vop_bus.h b/drivers/misc/mic/bus/vop_bus.h
> index 4fa02808c1e2..d891eacae358 100644
> --- a/drivers/misc/mic/bus/vop_bus.h
> +++ b/drivers/misc/mic/bus/vop_bus.h
> @@ -75,6 +75,7 @@ struct vop_driver {
>   *                 node to add/remove/configure virtio devices.
>   * @get_dp: Get access to the virtio device page used by the self
>   *          node to add/remove/configure virtio devices.
> + * @get_dp_dma: Get dma address of the virtio device page.
>   * @send_intr: Send an interrupt to the peer node on a specified doorbell.
>   * @remap: Map a buffer with the specified DMA address and length.
>   * @unmap: Unmap a buffer previously mapped.
> @@ -92,6 +93,7 @@ struct vop_hw_ops {
>  	void (*ack_interrupt)(struct vop_device *vpdev, int num);
>  	void __iomem * (*get_remote_dp)(struct vop_device *vpdev);
>  	void * (*get_dp)(struct vop_device *vpdev);
> +	dma_addr_t (*get_dp_dma)(struct vop_device *vpdev);
>  	void (*send_intr)(struct vop_device *vpdev, int db);
>  	void __iomem * (*remap)(struct vop_device *vpdev,
>  				  dma_addr_t pa, size_t len);
> diff --git a/drivers/misc/mic/host/mic_boot.c b/drivers/misc/mic/host/mic_boot.c
> index fb5b3989753d..ced03662cd8f 100644
> --- a/drivers/misc/mic/host/mic_boot.c
> +++ b/drivers/misc/mic/host/mic_boot.c
> @@ -88,6 +88,13 @@ static void *__mic_get_dp(struct vop_device *vpdev)
>  	return mdev->dp;
>  }
>  
> +static dma_addr_t __mic_get_dp_dma(struct vop_device *vpdev)
> +{
> +	struct mic_device *mdev = vpdev_to_mdev(&vpdev->dev);
> +
> +	return mdev->dp_dma_addr;
> +}
> +
>  static void __iomem *__mic_get_remote_dp(struct vop_device *vpdev)
>  {
>  	return NULL;
> @@ -119,6 +126,7 @@ static struct vop_hw_ops vop_hw_ops = {
>  	.ack_interrupt = __mic_ack_interrupt,
>  	.next_db = __mic_next_db,
>  	.get_dp = __mic_get_dp,
> +	.get_dp_dma = __mic_get_dp_dma,
>  	.get_remote_dp = __mic_get_remote_dp,
>  	.send_intr = __mic_send_intr,
>  	.remap = __mic_ioremap,
> diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c
> index fc8d8ff9ded3..aa2dd240c11e 100644
> --- a/drivers/misc/mic/vop/vop_vringh.c
> +++ b/drivers/misc/mic/vop/vop_vringh.c
> @@ -1076,6 +1076,7 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset,
>  		 unsigned long *size, unsigned long *pa)
>  {
>  	struct vop_device *vpdev = vdev->vpdev;
> +	struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd);
>  	unsigned long start = MIC_DP_SIZE;
>  	int i;
>  
> @@ -1088,7 +1089,14 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset,
>  	 * ....
>  	 */
>  	if (!offset) {
> -		*pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev));
> +		if (vpdev->hw_ops->get_dp_dma)
> +			*pa = vpdev->hw_ops->get_dp_dma(vpdev);
> +		else {
> +			dev_err(vop_dev(vdev), "can't get device page physical address\n");
> +			WARN_ON(1);

Don't crash kernels that have panic_on_warn set for things you can
recover from.

> +			return -1;

Use a real error value, do not make them up.

thanks,

greg k-h

Re: [PATCH 1/5] misc: vop: change the way of allocating vring for noncoherent platform

[Christoph Hellwig]

> +#include <linux/dma-noncoherent.h>

This header must not be included in drivers.

>  
> +		if (dev_is_dma_coherent(vop_dev(vdev)))

And this API must not be used in drivers either.

> +			vdev->vvr[i].vrh.vring.used =
> +				(void __force *)vpdev->hw_ops->remap(

And you must not use __force casts without a comment explanation why
they are safe.

Re: [PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform

[Christoph Hellwig]

On Fri, Sep 25, 2020 at 03:26:29PM +0800, Sherry Sun wrote:
> Set VIRTIO_F_ACCESS_PLATFORM feature for nocoherent platform, since
> it needs the DMA API for virtio.

Given that VOP is a plug-in PCIe card VIRTIO_F_ACCESS_PLATFORM must
always be set, as the DMA mapping details are not something the
virtio implementation decides on, but the host PCIe/iommu
implementation.

RE: [PATCH 0/5] Add noncoherent platform support for vop driver

[Sherry Sun]

Hi Arnd,

> Subject: Re: [PATCH 0/5] Add noncoherent platform support for vop driver
> 
> On Fri, Sep 25, 2020 at 9:27 AM Sherry Sun <sherry.sun@nxp.com> wrote:
> >
> > Change the way of allocating vring to support noncoherent platform for
> > vop driver, and add some related dma changes to make sure noncoherent
> > platform works well.
> 
> Could you describe why you are doing this? Are you using Intel MIC devices
> on Arm hosts, or trying to reuse the code for other add-on cards?
> 

We want to reuse the vop driver between two i.MX boards which are arm64 architecture. And in fact we have successfully verified it. 

But the biggest problem we currently encounter is about the noncoherent memory.
Since the hardware device of our platform is not dma coherent, 
when use the original way(__get_free_pages and dma_map_single, but without dma_sync_single_for_cpu/device) will meet errors.

Device pages/vring which are interact between ep and rc should use consistent memory without caching effects, 
so allocate them by dma_alloc_coherent is a better way.

> Note that we have a couple of frameworks in the kernel that try to do some
> of the same things here, notably the NTB drivers and the PCI endpoint
> support, both of which are designed to be somewhat more generic than the
> MIC driver.
> 
> Have you considered using that instead?
> 
>          Arnd
> 

Sorry I don't much about NTB, but for PCI endpoint driver, we will use it for pci data interaction below the vop layer.

Best regards
Sherry

> > Sherry Sun (5):
> >   misc: vop: change the way of allocating vring for noncoherent platform
> >   misc: vop: change the way of allocating used ring
> >   misc: vop: simply return the saved dma address instead of virt_to_phys
> >   misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform
> >   misc: vop: mapping kernel memory to user space as noncached
> >
> >  drivers/misc/mic/bus/vop_bus.h    |   2 +
> >  drivers/misc/mic/host/mic_boot.c  |   8 ++
> >  drivers/misc/mic/vop/vop_main.c   |  51 +++++++++----
> >  drivers/misc/mic/vop/vop_vringh.c | 117
> > ++++++++++++++++++++----------
> >  4 files changed, 125 insertions(+), 53 deletions(-)
> >
> > --
> > 2.17.1
> >

RE: [PATCH 1/5] misc: vop: change the way of allocating vring for noncoherent platform

[Sherry Sun]

Hi Greg,

> -----Original Message-----
> On Fri, Sep 25, 2020 at 03:26:26PM +0800, Sherry Sun wrote:
> > For noncoherent platform, we should allocate vring through
> > dma_alloc_coherent api to ensure timely synchronization of vring.
> > The orginal way which used __get_free_pages and dma_map_single only
> > apply to coherent platforms.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > ---
> >  drivers/misc/mic/vop/vop_vringh.c | 101
> > ++++++++++++++++++++----------
> >  1 file changed, 67 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/misc/mic/vop/vop_vringh.c
> > b/drivers/misc/mic/vop/vop_vringh.c
> > index f344209ac386..fc8d8ff9ded3 100644
> > --- a/drivers/misc/mic/vop/vop_vringh.c
> > +++ b/drivers/misc/mic/vop/vop_vringh.c
> > @@ -9,6 +9,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/poll.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/dma-noncoherent.h>
> >
> >  #include <linux/mic_common.h>
> >  #include "../common/mic_dev.h"
> > @@ -67,11 +68,12 @@ static void vop_virtio_init_post(struct vop_vdev
> *vdev)
> >  			dev_warn(vop_dev(vdev), "used_address zero??\n");
> >  			continue;
> >  		}
> > -		vdev->vvr[i].vrh.vring.used =
> > -			(void __force *)vpdev->hw_ops->remap(
> > -			vpdev,
> > -			le64_to_cpu(vqconfig[i].used_address),
> > -			used_size);
> > +		if (dev_is_dma_coherent(vop_dev(vdev)))
> > +			vdev->vvr[i].vrh.vring.used =
> > +				(void __force *)vpdev->hw_ops->remap(
> > +				vpdev,
> > +				le64_to_cpu(vqconfig[i].used_address),
> > +				used_size);
> 
> That's really hard to read, don't you agree?  We can go longer than 80
> columns now, and why the __force?
> 

Yes, it's really hard to read, here I moved the original code into the if() without change the code style. 
Sorry for that, will change it.

For the __force used in the original code, I think  this is because vpdev->hw_ops->remap() return type is void __iomem *, but vring.used need type void *. 
Maybe this is a workaround for Intel MIC platform, as it reassigns the used ring on the EP side.

But as far as I'm concerned, there is no need to reassign the used ring on the EP side. 
So move it into if (dev_is_dma_coherent(vop_dev(vdev))) to keep consistent with the code below.

> 
> 
> >  	}
> >
> >  	vdev->dc->used_address_updated = 0;
> > @@ -298,9 +300,14 @@ static int vop_virtio_add_device(struct vop_vdev
> *vdev,
> >  		mutex_init(&vvr->vr_mutex);
> >  		vr_size = PAGE_ALIGN(round_up(vring_size(num,
> MIC_VIRTIO_RING_ALIGN), 4) +
> >  			sizeof(struct _mic_vring_info));
> > -		vr->va = (void *)
> > -			__get_free_pages(GFP_KERNEL | __GFP_ZERO,
> > -					 get_order(vr_size));
> > +
> > +		if (!dev_is_dma_coherent(vop_dev(vdev)))
> > +			vr->va = dma_alloc_coherent(vop_dev(vdev), vr_size,
> > +						    &vr_addr, GFP_KERNEL);
> 
> Are you sure this is correct?  If dev_is_dma_coherent is NOT true, then you
> call dma_alloc_coherent()?
> 

Yes, we have verified it on the i.MX platform based on vop driver.

Here dev_is_dma_coherent is just a check to see if the device hardware supports dma coherent. 
If the hardware supports dma coherent, it is simple to handle the vring between EP and RC like the original MIC do(use __get_free_pages and dma_map_single, but without dma_sync_single_for_cpu/device).

But for some devices don't support hardware dma coherent, we should use dma_alloc_coherent to allocate consistent memory without caching effects, which can ensure timely synchronization of vring.

Actually the more common way to allocate vring in kernel is to use dma_alloc_coherent.

> 
> > +		else
> > +			vr->va = (void *)
> > +				__get_free_pages(GFP_KERNEL |
> __GFP_ZERO,
> > +						 get_order(vr_size));
> >  		if (!vr->va) {
> >  			ret = -ENOMEM;
> >  			dev_err(vop_dev(vdev), "%s %d err %d\n", @@ -
> 310,14 +317,17 @@
> > static int vop_virtio_add_device(struct vop_vdev *vdev,
> >  		vr->len = vr_size;
> >  		vr->info = vr->va + round_up(vring_size(num,
> MIC_VIRTIO_RING_ALIGN), 4);
> >  		vr->info->magic = cpu_to_le32(MIC_MAGIC + vdev->virtio_id
> + i);
> > -		vr_addr = dma_map_single(&vpdev->dev, vr->va, vr_size,
> > -					 DMA_BIDIRECTIONAL);
> > -		if (dma_mapping_error(&vpdev->dev, vr_addr)) {
> > -			free_pages((unsigned long)vr->va,
> get_order(vr_size));
> > -			ret = -ENOMEM;
> > -			dev_err(vop_dev(vdev), "%s %d err %d\n",
> > -				__func__, __LINE__, ret);
> > -			goto err;
> > +
> > +		if (dev_is_dma_coherent(vop_dev(vdev))) {
> > +			vr_addr = dma_map_single(&vpdev->dev, vr->va,
> vr_size,
> > +						 DMA_BIDIRECTIONAL);
> > +			if (dma_mapping_error(&vpdev->dev, vr_addr)) {
> > +				free_pages((unsigned long)vr->va,
> get_order(vr_size));
> > +				ret = -ENOMEM;
> > +				dev_err(vop_dev(vdev), "%s %d err %d\n",
> > +						__func__, __LINE__, ret);
> > +				goto err;
> > +			}
> 
> What about if this is not true?
> 
> Same for other places in this patch.

If it is not true here, means the device doesn't support hardware dma coherent, we can use dma_alloc_coherent to get both va and pa.
But for the true case, means the device hardware is dma coherent, two steps are needed to get va and pa(1.__get_free_pages 2.dma_map_single), such as here.

Best regards
Sherry 

> 
> thanks,
> 
> greg k-h

RE: [PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform

[Sherry Sun]

Hi Greg,

> -----Original Message-----
> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: 2020年9月25日 20:17
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: sudeep.dutt@intel.com; ashutosh.dixit@intel.com; arnd@arndb.de;
> wang.yi59@zte.com.cn; huang.zijiang@zte.com.cn;
> rikard.falkeborn@gmail.com; lee.jones@linaro.org; mst@redhat.com; linux-
> kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for
> nocoherent platform
> 
> On Fri, Sep 25, 2020 at 03:26:29PM +0800, Sherry Sun wrote:
> > Set VIRTIO_F_ACCESS_PLATFORM feature for nocoherent platform, since it
> > needs the DMA API for virtio.
> >
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/misc/mic/vop/vop_main.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/misc/mic/vop/vop_main.c
> > b/drivers/misc/mic/vop/vop_main.c index d3645122c983..d609f0dc6124
> > 100644
> > --- a/drivers/misc/mic/vop/vop_main.c
> > +++ b/drivers/misc/mic/vop/vop_main.c
> > @@ -125,6 +125,9 @@ static void vop_transport_features(struct
> virtio_device *vdev)
> >  	 * creates virtio rings on preallocated memory.
> >  	 */
> >  	__virtio_clear_bit(vdev, VIRTIO_F_RING_PACKED);
> > +
> > +	if (!dev_is_dma_coherent(vdev->dev.parent))
> > +		__virtio_set_bit(vdev, VIRTIO_F_ACCESS_PLATFORM);
> 
> Why look at the parent and not the device itself?

That's a good question, here use vdev->dev.parent to get the vop device instead of virtio device,  which can keep consistent with other dev_is_dma_coherent check.

Thanks
Sherry 

RE: [PATCH 3/5] misc: vop: simply return the saved dma address instead of virt_to_phys

[Sherry Sun]

Hi Greg,

> -----Original Message-----
> On Fri, Sep 25, 2020 at 03:26:28PM +0800, Sherry Sun wrote:
> > For noncoherent platform, the device page and vring should allocated
> > by dma_alloc_coherent, when user space wants to get its physical
> > address, virt_to_phys cannot be used, should simply return the saved
> > device page dma address by get_dp_dma callback and the vring dma
> > address saved in mic_vqconfig.
> >
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/misc/mic/bus/vop_bus.h    |  2 ++
> >  drivers/misc/mic/host/mic_boot.c  |  8 ++++++++
> > drivers/misc/mic/vop/vop_vringh.c | 12 ++++++++++--
> >  3 files changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/misc/mic/bus/vop_bus.h
> > b/drivers/misc/mic/bus/vop_bus.h index 4fa02808c1e2..d891eacae358
> > 100644
> > --- a/drivers/misc/mic/bus/vop_bus.h
> > +++ b/drivers/misc/mic/bus/vop_bus.h
> > @@ -75,6 +75,7 @@ struct vop_driver {
> >   *                 node to add/remove/configure virtio devices.
> >   * @get_dp: Get access to the virtio device page used by the self
> >   *          node to add/remove/configure virtio devices.
> > + * @get_dp_dma: Get dma address of the virtio device page.
> >   * @send_intr: Send an interrupt to the peer node on a specified doorbell.
> >   * @remap: Map a buffer with the specified DMA address and length.
> >   * @unmap: Unmap a buffer previously mapped.
> > @@ -92,6 +93,7 @@ struct vop_hw_ops {
> >  	void (*ack_interrupt)(struct vop_device *vpdev, int num);
> >  	void __iomem * (*get_remote_dp)(struct vop_device *vpdev);
> >  	void * (*get_dp)(struct vop_device *vpdev);
> > +	dma_addr_t (*get_dp_dma)(struct vop_device *vpdev);
> >  	void (*send_intr)(struct vop_device *vpdev, int db);
> >  	void __iomem * (*remap)(struct vop_device *vpdev,
> >  				  dma_addr_t pa, size_t len);
> > diff --git a/drivers/misc/mic/host/mic_boot.c
> > b/drivers/misc/mic/host/mic_boot.c
> > index fb5b3989753d..ced03662cd8f 100644
> > --- a/drivers/misc/mic/host/mic_boot.c
> > +++ b/drivers/misc/mic/host/mic_boot.c
> > @@ -88,6 +88,13 @@ static void *__mic_get_dp(struct vop_device *vpdev)
> >  	return mdev->dp;
> >  }
> >
> > +static dma_addr_t __mic_get_dp_dma(struct vop_device *vpdev) {
> > +	struct mic_device *mdev = vpdev_to_mdev(&vpdev->dev);
> > +
> > +	return mdev->dp_dma_addr;
> > +}
> > +
> >  static void __iomem *__mic_get_remote_dp(struct vop_device *vpdev)  {
> >  	return NULL;
> > @@ -119,6 +126,7 @@ static struct vop_hw_ops vop_hw_ops = {
> >  	.ack_interrupt = __mic_ack_interrupt,
> >  	.next_db = __mic_next_db,
> >  	.get_dp = __mic_get_dp,
> > +	.get_dp_dma = __mic_get_dp_dma,
> >  	.get_remote_dp = __mic_get_remote_dp,
> >  	.send_intr = __mic_send_intr,
> >  	.remap = __mic_ioremap,
> > diff --git a/drivers/misc/mic/vop/vop_vringh.c
> > b/drivers/misc/mic/vop/vop_vringh.c
> > index fc8d8ff9ded3..aa2dd240c11e 100644
> > --- a/drivers/misc/mic/vop/vop_vringh.c
> > +++ b/drivers/misc/mic/vop/vop_vringh.c
> > @@ -1076,6 +1076,7 @@ vop_query_offset(struct vop_vdev *vdev,
> unsigned long offset,
> >  		 unsigned long *size, unsigned long *pa)  {
> >  	struct vop_device *vpdev = vdev->vpdev;
> > +	struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd);
> >  	unsigned long start = MIC_DP_SIZE;
> >  	int i;
> >
> > @@ -1088,7 +1089,14 @@ vop_query_offset(struct vop_vdev *vdev,
> unsigned long offset,
> >  	 * ....
> >  	 */
> >  	if (!offset) {
> > -		*pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev));
> > +		if (vpdev->hw_ops->get_dp_dma)
> > +			*pa = vpdev->hw_ops->get_dp_dma(vpdev);
> > +		else {
> > +			dev_err(vop_dev(vdev), "can't get device page
> physical address\n");
> > +			WARN_ON(1);
> 
> Don't crash kernels that have panic_on_warn set for things you can recover
> from.
> 

Do you mean that we should avoid to use WARN_ON here?

> > +			return -1;
> 
> Use a real error value, do not make them up.

Ok, will change it. Thanks.

Best regards
Sherry

> 
> thanks,
> 
> greg k-h

RE: [PATCH 1/5] misc: vop: change the way of allocating vring for noncoherent platform

[Sherry Sun]

Hi Christoph,

> > +#include <linux/dma-noncoherent.h>
> 
> This header must not be included in drivers.
> 
> >
> > +		if (dev_is_dma_coherent(vop_dev(vdev)))
> 
> And this API must not be used in drivers either.

Thanks for your reply.
Can you explain why we cannot use the API and header above in drivers?
And do you know if there are any APIs that could replace this to check the device hardware dma coherent support?

> 
> > +			vdev->vvr[i].vrh.vring.used =
> > +				(void __force *)vpdev->hw_ops->remap(
> 
> And you must not use __force casts without a comment explanation why
> they are safe.

Here is the original code, I moved the original code into the if() without change it.
But I think  this is because vpdev->hw_ops->remap() return type is void __iomem *, but vring.used need type void *. 
Maybe this is a workaround for Intel MIC platform, as it reassigns the used ring on the EP side.

Regards 
Sherry

RE: [PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform

[Sherry Sun]

Hi Christoph,

> On Fri, Sep 25, 2020 at 03:26:29PM +0800, Sherry Sun wrote:
> > Set VIRTIO_F_ACCESS_PLATFORM feature for nocoherent platform, since it
> > needs the DMA API for virtio.
> 
> Given that VOP is a plug-in PCIe card VIRTIO_F_ACCESS_PLATFORM must
> always be set, as the DMA mapping details are not something the virtio
> implementation decides on, but the host PCIe/iommu implementation.

So do you mean that we should remove the check, and set VIRTIO_F_ACCESS_PLATFORM feature directly?

Regards
Sherry 

Re: [PATCH 1/5] misc: vop: change the way of allocating vring for noncoherent platform

[Christoph Hellwig]

On Sun, Sep 27, 2020 at 07:58:29AM +0000, Sherry Sun wrote:
> Thanks for your reply.
> Can you explain why we cannot use the API and header above in drivers?
> And do you know if there are any APIs that could replace this to check the device hardware dma coherent support?

If your treat the memory as if it is coherent with device access you
should always use dma_alloc_coherent.  The whole point of the DMA API
is to abstract such differences away.

> > 
> > > +			vdev->vvr[i].vrh.vring.used =
> > > +				(void __force *)vpdev->hw_ops->remap(
> > 
> > And you must not use __force casts without a comment explanation why
> > they are safe.
> 
> Here is the original code, I moved the original code into the if() without change it.
> But I think  this is because vpdev->hw_ops->remap() return type is void __iomem *, but vring.used need type void *. 
> Maybe this is a workaround for Intel MIC platform, as it reassigns the used ring on the EP side.

Well, we'll need to figure out what is going on here, and blind casts to
and from __iomem are dangerous.

Re: [PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform

[Christoph Hellwig]

On Sun, Sep 27, 2020 at 08:05:07AM +0000, Sherry Sun wrote:
> Hi Christoph,
> 
> > On Fri, Sep 25, 2020 at 03:26:29PM +0800, Sherry Sun wrote:
> > > Set VIRTIO_F_ACCESS_PLATFORM feature for nocoherent platform, since it
> > > needs the DMA API for virtio.
> > 
> > Given that VOP is a plug-in PCIe card VIRTIO_F_ACCESS_PLATFORM must
> > always be set, as the DMA mapping details are not something the virtio
> > implementation decides on, but the host PCIe/iommu implementation.
> 
> So do you mean that we should remove the check, and set VIRTIO_F_ACCESS_PLATFORM feature directly?

Yes.  In a separate prep patch.

RE: [PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform

[Sherry Sun]

Hi Christoph,

> On Sun, Sep 27, 2020 at 08:05:07AM +0000, Sherry Sun wrote:
> > Hi Christoph,
> >
> > > On Fri, Sep 25, 2020 at 03:26:29PM +0800, Sherry Sun wrote:
> > > > Set VIRTIO_F_ACCESS_PLATFORM feature for nocoherent platform,
> > > > since it needs the DMA API for virtio.
> > >
> > > Given that VOP is a plug-in PCIe card VIRTIO_F_ACCESS_PLATFORM must
> > > always be set, as the DMA mapping details are not something the
> > > virtio implementation decides on, but the host PCIe/iommu
> implementation.
> >
> > So do you mean that we should remove the check, and set
> VIRTIO_F_ACCESS_PLATFORM feature directly?
> 
> Yes.  In a separate prep patch.

Okay, will do it in V2.

Regards
Sherry

RE: [PATCH 1/5] misc: vop: change the way of allocating vring for noncoherent platform

[Sherry Sun]

Hi Christoph,

> On Sun, Sep 27, 2020 at 07:58:29AM +0000, Sherry Sun wrote:
> > Thanks for your reply.
> > Can you explain why we cannot use the API and header above in drivers?
> > And do you know if there are any APIs that could replace this to check the
> device hardware dma coherent support?
> 
> If your treat the memory as if it is coherent with device access you should
> always use dma_alloc_coherent.  The whole point of the DMA API is to
> abstract such differences away.

Well, for vring and device pages in vop driver, dma coherent memory maybe more reasonable.
Will use dma_alloc_coherent to replace the orginal memory allocate method in V2.

> 
> > >
> > > > +			vdev->vvr[i].vrh.vring.used =
> > > > +				(void __force *)vpdev->hw_ops->remap(
> > >
> > > And you must not use __force casts without a comment explanation why
> > > they are safe.
> >
> > Here is the original code, I moved the original code into the if() without
> change it.
> > But I think  this is because vpdev->hw_ops->remap() return type is void
> __iomem *, but vring.used need type void *.
> > Maybe this is a workaround for Intel MIC platform, as it reassigns the used
> ring on the EP side.
> 
> Well, we'll need to figure out what is going on here, and blind casts to and
> from __iomem are dangerous.

Yes, you are right. 

Regards
Sherry