* [PATCH V4 0/2] Change vring space from nomal memory to dma coherent memory @ 2020-10-26 8:53 Sherry Sun 2020-10-26 8:53 ` [PATCH V4 1/2] misc: vop: change the way of allocating vring and device page Sherry Sun 2020-10-26 8:53 ` [PATCH V4 2/2] misc: vop: do not allocate and reassign the used ring Sherry Sun 0 siblings, 2 replies; 6+ messages in thread From: Sherry Sun @ 2020-10-26 8:53 UTC (permalink / raw) To: hch, gregkh, sudeep.dutt, ashutosh.dixit, arnd, kishon, lorenzo.pieralisi Cc: linux-kernel, linux-pci, linux-imx, fugang.duan Changes in V4: 1. Change to use dma_mmap_coherent api for vop mmap callback. 2. Add dp_mmap callback to map device page to userspace instead of get_dp_dma callback. 3. Move all the dma related changes to patch 1. 4. Kill the mic_dp_init/mic_dp_uninit and inline those into the callers as Christoph suggested. 5. Keep the code following the 80-character line rule. 6. Add endian swap for vqconfig[i].address. The original vop driver only supports dma coherent device, as it allocates and maps vring by _get_free_pages and dma_map_single, but not use dma_sync_single_for_cpu/device to sync the updates of device_page/vring between EP and RC, which will cause memory synchronization problem for device don't support hardware dma coherent. And allocate vrings use dma_alloc_coherent is a common way in kernel, as the memory interacted between two systems should use consistent memory to avoid caching effects. So here add noncoherent platform support for vop driver. Also add some related dma changes to make sure noncoherent platform works well. Sherry Sun (2): misc: vop: change the way of allocating vring and device page misc: vop: do not allocate and reassign the used ring drivers/misc/mic/bus/vop_bus.h | 2 + drivers/misc/mic/host/mic_boot.c | 9 +++ drivers/misc/mic/host/mic_main.c | 43 +++-------- drivers/misc/mic/vop/vop_debugfs.c | 2 - drivers/misc/mic/vop/vop_main.c | 51 +++---------- drivers/misc/mic/vop/vop_vringh.c | 110 +++++++++-------------------- include/uapi/linux/mic_common.h | 5 +- 7 files changed, 66 insertions(+), 156 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V4 1/2] misc: vop: change the way of allocating vring and device page 2020-10-26 8:53 [PATCH V4 0/2] Change vring space from nomal memory to dma coherent memory Sherry Sun @ 2020-10-26 8:53 ` Sherry Sun 2020-10-27 10:40 ` Christoph Hellwig 2020-10-26 8:53 ` [PATCH V4 2/2] misc: vop: do not allocate and reassign the used ring Sherry Sun 1 sibling, 1 reply; 6+ messages in thread From: Sherry Sun @ 2020-10-26 8:53 UTC (permalink / raw) To: hch, gregkh, sudeep.dutt, ashutosh.dixit, arnd, kishon, lorenzo.pieralisi Cc: linux-kernel, linux-pci, linux-imx, fugang.duan Allocate vrings use dma_alloc_coherent is a common way in kernel. As the memory interacted between two systems should use consistent memory to avoid caching effects, same as device page memory. The orginal way use __get_free_pages and dma_map_single to allocate and map vring, but not use dma_sync_single_for_cpu/device api to sync the changes of vring between EP and RC, which will cause memory synchronization problem for those devices which don't support hardware dma coherent. Also change to use dma_mmap_coherent for mmap callback to map the device page and vring memory to userspace. Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> Signed-off-by: Sherry Sun <sherry.sun@nxp.com> --- drivers/misc/mic/bus/vop_bus.h | 2 + drivers/misc/mic/host/mic_boot.c | 9 ++++ drivers/misc/mic/host/mic_main.c | 43 +++++------------ drivers/misc/mic/vop/vop_vringh.c | 79 +++++++++++++------------------ 4 files changed, 55 insertions(+), 78 deletions(-) diff --git a/drivers/misc/mic/bus/vop_bus.h b/drivers/misc/mic/bus/vop_bus.h index 4fa02808c1e2..e21c06aeda7a 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. + * @dp_mmap: Map the virtio device page to userspace. * @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); + int (*dp_mmap)(struct vop_device *vpdev, struct vm_area_struct *vma); 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 8cb85b8b3e19..44ed918b49b4 100644 --- a/drivers/misc/mic/host/mic_boot.c +++ b/drivers/misc/mic/host/mic_boot.c @@ -89,6 +89,14 @@ static void *__mic_get_dp(struct vop_device *vpdev) return mdev->dp; } +static int __mic_dp_mmap(struct vop_device *vpdev, struct vm_area_struct *vma) +{ + struct mic_device *mdev = vpdev_to_mdev(&vpdev->dev); + + return dma_mmap_coherent(&mdev->pdev->dev, vma, mdev->dp, + mdev->dp_dma_addr, MIC_DP_SIZE); +} + static void __iomem *__mic_get_remote_dp(struct vop_device *vpdev) { return NULL; @@ -120,6 +128,7 @@ static struct vop_hw_ops vop_hw_ops = { .ack_interrupt = __mic_ack_interrupt, .next_db = __mic_next_db, .get_dp = __mic_get_dp, + .dp_mmap = __mic_dp_mmap, .get_remote_dp = __mic_get_remote_dp, .send_intr = __mic_send_intr, .remap = __mic_ioremap, diff --git a/drivers/misc/mic/host/mic_main.c b/drivers/misc/mic/host/mic_main.c index ea4608527ea0..fab87a72a9a4 100644 --- a/drivers/misc/mic/host/mic_main.c +++ b/drivers/misc/mic/host/mic_main.c @@ -10,6 +10,7 @@ #include <linux/module.h> #include <linux/pci.h> #include <linux/poll.h> +#include <linux/dma-mapping.h> #include <linux/mic_common.h> #include "../common/mic_dev.h" @@ -45,33 +46,6 @@ MODULE_DEVICE_TABLE(pci, mic_pci_tbl); /* ID allocator for MIC devices */ static struct ida g_mic_ida; -/* Initialize the device page */ -static int mic_dp_init(struct mic_device *mdev) -{ - mdev->dp = kzalloc(MIC_DP_SIZE, GFP_KERNEL); - if (!mdev->dp) - return -ENOMEM; - - mdev->dp_dma_addr = mic_map_single(mdev, - mdev->dp, MIC_DP_SIZE); - if (mic_map_error(mdev->dp_dma_addr)) { - kfree(mdev->dp); - dev_err(&mdev->pdev->dev, "%s %d err %d\n", - __func__, __LINE__, -ENOMEM); - return -ENOMEM; - } - mdev->ops->write_spad(mdev, MIC_DPLO_SPAD, mdev->dp_dma_addr); - mdev->ops->write_spad(mdev, MIC_DPHI_SPAD, mdev->dp_dma_addr >> 32); - return 0; -} - -/* Uninitialize the device page */ -static void mic_dp_uninit(struct mic_device *mdev) -{ - mic_unmap_single(mdev, mdev->dp_dma_addr, MIC_DP_SIZE); - kfree(mdev->dp); -} - /** * mic_ops_init: Initialize HW specific operation tables. * @@ -227,11 +201,16 @@ static int mic_probe(struct pci_dev *pdev, pci_set_drvdata(pdev, mdev); - rc = mic_dp_init(mdev); - if (rc) { - dev_err(&pdev->dev, "mic_dp_init failed rc %d\n", rc); + mdev->dp = dma_alloc_coherent(&pdev->dev, MIC_DP_SIZE, + &mdev->dp_dma_addr, GFP_KERNEL); + if (!mdev->dp) { + dev_err(&pdev->dev, "failed to allocate device page\n"); goto smpt_uninit; } + + mdev->ops->write_spad(mdev, MIC_DPLO_SPAD, mdev->dp_dma_addr); + mdev->ops->write_spad(mdev, MIC_DPHI_SPAD, mdev->dp_dma_addr >> 32); + mic_bootparam_init(mdev); mic_create_debug_dir(mdev); @@ -244,7 +223,7 @@ static int mic_probe(struct pci_dev *pdev, return 0; cleanup_debug_dir: mic_delete_debug_dir(mdev); - mic_dp_uninit(mdev); + dma_free_coherent(&pdev->dev, MIC_DP_SIZE, mdev->dp, mdev->dp_dma_addr); smpt_uninit: mic_smpt_uninit(mdev); free_interrupts: @@ -283,7 +262,7 @@ static void mic_remove(struct pci_dev *pdev) cosm_unregister_device(mdev->cosm_dev); mic_delete_debug_dir(mdev); - mic_dp_uninit(mdev); + dma_free_coherent(&pdev->dev, MIC_DP_SIZE, mdev->dp, mdev->dp_dma_addr); mic_smpt_uninit(mdev); mic_free_interrupts(mdev, pdev); iounmap(mdev->aper.va); diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c index 7014ffe88632..df77681c97e6 100644 --- a/drivers/misc/mic/vop/vop_vringh.c +++ b/drivers/misc/mic/vop/vop_vringh.c @@ -298,9 +298,8 @@ 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)); + vr->va = dma_alloc_coherent(vop_dev(vdev), vr_size, &vr_addr, + GFP_KERNEL); if (!vr->va) { ret = -ENOMEM; dev_err(vop_dev(vdev), "%s %d err %d\n", @@ -310,15 +309,6 @@ 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; - } vqconfig[i].address = cpu_to_le64(vr_addr); vring_init(&vr->vr, num, vr->va, MIC_VIRTIO_RING_ALIGN); @@ -339,11 +329,9 @@ 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); + vvr->buf = dma_alloc_coherent(vop_dev(vdev), + VOP_INT_DMA_BUF_SIZE, + &vvr->buf_da, GFP_KERNEL); } snprintf(irqname, sizeof(irqname), "vop%dvirtio%d", vpdev->index, @@ -382,10 +370,8 @@ 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)); + dma_free_coherent(vop_dev(vdev), vvr->vring.len, vvr->vring.va, + le64_to_cpu(vqconfig[j].address)); } return ret; } @@ -433,17 +419,12 @@ 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)); + dma_free_coherent(vop_dev(vdev), VOP_INT_DMA_BUF_SIZE, + vvr->buf, vvr->buf_da); 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)); + dma_free_coherent(vop_dev(vdev), vvr->vring.len, vvr->vring.va, + le64_to_cpu(vqconfig[i].address)); } /* * Order the type update with previous stores. This write barrier @@ -1044,31 +1025,25 @@ static __poll_t vop_poll(struct file *f, poll_table *wait) static inline int vop_query_offset(struct vop_vdev *vdev, unsigned long offset, - unsigned long *size, unsigned long *pa) + unsigned long *size, unsigned long *pa, void **va) { - struct vop_device *vpdev = vdev->vpdev; + struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd); unsigned long start = MIC_DP_SIZE; int i; /* * MMAP interface is as follows: * offset region - * 0x0 virtio device_page * 0x1000 first vring * 0x1000 + size of 1st vring second vring * .... */ - if (!offset) { - *pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev)); - *size = MIC_DP_SIZE; - return 0; - } - for (i = 0; i < vdev->dd->num_vq; i++) { struct vop_vringh *vvr = &vdev->vvr[i]; if (offset == start) { - *pa = virt_to_phys(vvr->vring.va); + *pa = le64_to_cpu(vqconfig[i].address); + *va = vvr->vring.va; *size = vvr->vring.len; return 0; } @@ -1083,8 +1058,11 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset, static int vop_mmap(struct file *f, struct vm_area_struct *vma) { struct vop_vdev *vdev = f->private_data; - unsigned long offset = vma->vm_pgoff << PAGE_SHIFT; - unsigned long pa, size = vma->vm_end - vma->vm_start, size_rem = size; + unsigned long offset = (vma->vm_pgoff << PAGE_SHIFT) + MIC_DP_SIZE; + unsigned long pa, size = vma->vm_end - vma->vm_start; + unsigned long size_rem = size - MIC_DP_SIZE; + unsigned long base = vma->vm_start; + void *va; int i, err; err = vop_vdev_inited(vdev); @@ -1095,19 +1073,28 @@ static int vop_mmap(struct file *f, struct vm_area_struct *vma) goto ret; } while (size_rem) { - i = vop_query_offset(vdev, offset, &size, &pa); + i = vop_query_offset(vdev, offset, &size, &pa, &va); if (i < 0) { err = -EINVAL; goto ret; } - err = remap_pfn_range(vma, vma->vm_start + offset, - pa >> PAGE_SHIFT, size, - vma->vm_page_prot); + vma->vm_start = base + offset; + vma->vm_end = vma->vm_start + size; + err = dma_mmap_coherent(vop_dev(vdev), vma, va, pa, size); if (err) goto ret; size_rem -= size; offset += size; } + + if (vdev->vpdev->hw_ops->dp_mmap) { + vma->vm_start = base; + vma->vm_end = vma->vm_start + MIC_DP_SIZE; + err = vdev->vpdev->hw_ops->dp_mmap(vdev->vpdev, vma); + if (err) + goto ret; + } + ret: return err; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V4 1/2] misc: vop: change the way of allocating vring and device page 2020-10-26 8:53 ` [PATCH V4 1/2] misc: vop: change the way of allocating vring and device page Sherry Sun @ 2020-10-27 10:40 ` Christoph Hellwig 2020-10-27 12:30 ` Sherry Sun 0 siblings, 1 reply; 6+ messages in thread From: Christoph Hellwig @ 2020-10-27 10:40 UTC (permalink / raw) To: Sherry Sun Cc: hch, gregkh, sudeep.dutt, ashutosh.dixit, arnd, kishon, lorenzo.pieralisi, linux-kernel, linux-pci, linux-imx, fugang.duan This looks much better, but we need to be careful to restore the original vm_start / vm_end. What do you think about the version below, which also simplifies vop_mmap a lot (completely untested): --- From 2de72bf7444ee187a7576962d746d482c5bdd593 Mon Sep 17 00:00:00 2001 From: Sherry Sun <sherry.sun@nxp.com> Date: Mon, 26 Oct 2020 16:53:34 +0800 Subject: misc: vop: change the way of allocating vring and device page Allocate vrings use dma_alloc_coherent is a common way in kernel. As the memory interacted between two systems should use consistent memory to avoid caching effects, same as device page memory. The orginal way use __get_free_pages and dma_map_single to allocate and map vring, but not use dma_sync_single_for_cpu/device api to sync the changes of vring between EP and RC, which will cause memory synchronization problem for those devices which don't support hardware dma coherent. Also change to use dma_mmap_coherent for mmap callback to map the device page and vring memory to userspace. Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> Signed-off-by: Sherry Sun <sherry.sun@nxp.com> --- drivers/misc/mic/bus/vop_bus.h | 2 + drivers/misc/mic/host/mic_boot.c | 9 ++ drivers/misc/mic/host/mic_main.c | 43 +++------- drivers/misc/mic/vop/vop_vringh.c | 135 ++++++++++++------------------ 4 files changed, 77 insertions(+), 112 deletions(-) diff --git a/drivers/misc/mic/bus/vop_bus.h b/drivers/misc/mic/bus/vop_bus.h index 4fa02808c1e27d..e21c06aeda7a31 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. + * @dp_mmap: Map the virtio device page to userspace. * @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); + int (*dp_mmap)(struct vop_device *vpdev, struct vm_area_struct *vma); 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 8cb85b8b3e199b..44ed918b49b4d2 100644 --- a/drivers/misc/mic/host/mic_boot.c +++ b/drivers/misc/mic/host/mic_boot.c @@ -89,6 +89,14 @@ static void *__mic_get_dp(struct vop_device *vpdev) return mdev->dp; } +static int __mic_dp_mmap(struct vop_device *vpdev, struct vm_area_struct *vma) +{ + struct mic_device *mdev = vpdev_to_mdev(&vpdev->dev); + + return dma_mmap_coherent(&mdev->pdev->dev, vma, mdev->dp, + mdev->dp_dma_addr, MIC_DP_SIZE); +} + static void __iomem *__mic_get_remote_dp(struct vop_device *vpdev) { return NULL; @@ -120,6 +128,7 @@ static struct vop_hw_ops vop_hw_ops = { .ack_interrupt = __mic_ack_interrupt, .next_db = __mic_next_db, .get_dp = __mic_get_dp, + .dp_mmap = __mic_dp_mmap, .get_remote_dp = __mic_get_remote_dp, .send_intr = __mic_send_intr, .remap = __mic_ioremap, diff --git a/drivers/misc/mic/host/mic_main.c b/drivers/misc/mic/host/mic_main.c index ea4608527ea04d..fab87a72a9a4a5 100644 --- a/drivers/misc/mic/host/mic_main.c +++ b/drivers/misc/mic/host/mic_main.c @@ -10,6 +10,7 @@ #include <linux/module.h> #include <linux/pci.h> #include <linux/poll.h> +#include <linux/dma-mapping.h> #include <linux/mic_common.h> #include "../common/mic_dev.h" @@ -45,33 +46,6 @@ MODULE_DEVICE_TABLE(pci, mic_pci_tbl); /* ID allocator for MIC devices */ static struct ida g_mic_ida; -/* Initialize the device page */ -static int mic_dp_init(struct mic_device *mdev) -{ - mdev->dp = kzalloc(MIC_DP_SIZE, GFP_KERNEL); - if (!mdev->dp) - return -ENOMEM; - - mdev->dp_dma_addr = mic_map_single(mdev, - mdev->dp, MIC_DP_SIZE); - if (mic_map_error(mdev->dp_dma_addr)) { - kfree(mdev->dp); - dev_err(&mdev->pdev->dev, "%s %d err %d\n", - __func__, __LINE__, -ENOMEM); - return -ENOMEM; - } - mdev->ops->write_spad(mdev, MIC_DPLO_SPAD, mdev->dp_dma_addr); - mdev->ops->write_spad(mdev, MIC_DPHI_SPAD, mdev->dp_dma_addr >> 32); - return 0; -} - -/* Uninitialize the device page */ -static void mic_dp_uninit(struct mic_device *mdev) -{ - mic_unmap_single(mdev, mdev->dp_dma_addr, MIC_DP_SIZE); - kfree(mdev->dp); -} - /** * mic_ops_init: Initialize HW specific operation tables. * @@ -227,11 +201,16 @@ static int mic_probe(struct pci_dev *pdev, pci_set_drvdata(pdev, mdev); - rc = mic_dp_init(mdev); - if (rc) { - dev_err(&pdev->dev, "mic_dp_init failed rc %d\n", rc); + mdev->dp = dma_alloc_coherent(&pdev->dev, MIC_DP_SIZE, + &mdev->dp_dma_addr, GFP_KERNEL); + if (!mdev->dp) { + dev_err(&pdev->dev, "failed to allocate device page\n"); goto smpt_uninit; } + + mdev->ops->write_spad(mdev, MIC_DPLO_SPAD, mdev->dp_dma_addr); + mdev->ops->write_spad(mdev, MIC_DPHI_SPAD, mdev->dp_dma_addr >> 32); + mic_bootparam_init(mdev); mic_create_debug_dir(mdev); @@ -244,7 +223,7 @@ static int mic_probe(struct pci_dev *pdev, return 0; cleanup_debug_dir: mic_delete_debug_dir(mdev); - mic_dp_uninit(mdev); + dma_free_coherent(&pdev->dev, MIC_DP_SIZE, mdev->dp, mdev->dp_dma_addr); smpt_uninit: mic_smpt_uninit(mdev); free_interrupts: @@ -283,7 +262,7 @@ static void mic_remove(struct pci_dev *pdev) cosm_unregister_device(mdev->cosm_dev); mic_delete_debug_dir(mdev); - mic_dp_uninit(mdev); + dma_free_coherent(&pdev->dev, MIC_DP_SIZE, mdev->dp, mdev->dp_dma_addr); mic_smpt_uninit(mdev); mic_free_interrupts(mdev, pdev); iounmap(mdev->aper.va); diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c index 7014ffe88632e5..6835648d577d57 100644 --- a/drivers/misc/mic/vop/vop_vringh.c +++ b/drivers/misc/mic/vop/vop_vringh.c @@ -298,9 +298,8 @@ 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)); + vr->va = dma_alloc_coherent(vop_dev(vdev), vr_size, &vr_addr, + GFP_KERNEL); if (!vr->va) { ret = -ENOMEM; dev_err(vop_dev(vdev), "%s %d err %d\n", @@ -310,15 +309,6 @@ 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; - } vqconfig[i].address = cpu_to_le64(vr_addr); vring_init(&vr->vr, num, vr->va, MIC_VIRTIO_RING_ALIGN); @@ -339,11 +329,9 @@ 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); + vvr->buf = dma_alloc_coherent(vop_dev(vdev), + VOP_INT_DMA_BUF_SIZE, + &vvr->buf_da, GFP_KERNEL); } snprintf(irqname, sizeof(irqname), "vop%dvirtio%d", vpdev->index, @@ -382,10 +370,8 @@ 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)); + dma_free_coherent(vop_dev(vdev), vvr->vring.len, vvr->vring.va, + le64_to_cpu(vqconfig[j].address)); } return ret; } @@ -433,17 +419,12 @@ 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)); + dma_free_coherent(vop_dev(vdev), VOP_INT_DMA_BUF_SIZE, + vvr->buf, vvr->buf_da); 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)); + dma_free_coherent(vop_dev(vdev), vvr->vring.len, vvr->vring.va, + le64_to_cpu(vqconfig[i].address)); } /* * Order the type update with previous stores. This write barrier @@ -1042,13 +1023,27 @@ static __poll_t vop_poll(struct file *f, poll_table *wait) return mask; } -static inline int -vop_query_offset(struct vop_vdev *vdev, unsigned long offset, - unsigned long *size, unsigned long *pa) +/* + * Maps the device page and virtio rings to user space for readonly access. + */ +static int vop_mmap(struct file *f, struct vm_area_struct *vma) { - struct vop_device *vpdev = vdev->vpdev; - unsigned long start = MIC_DP_SIZE; - int i; + struct vop_vdev *vdev = f->private_data; + struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd); + unsigned long orig_start = vma->vm_start; + unsigned long orig_end = vma->vm_end; + int err, i; + + if (!vdev->vpdev->hw_ops->dp_mmap) + return -EINVAL; + if (vma->vm_pgoff) + return -EINVAL; + if (vma->vm_flags & VM_WRITE) + return -EACCES; + + err = vop_vdev_inited(vdev); + if (err) + return err; /* * MMAP interface is as follows: @@ -1057,58 +1052,38 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long offset, * 0x1000 first vring * 0x1000 + size of 1st vring second vring * .... + * + * We manipulate vm_start/vm_end to trick dma_mmap_coherent into + * performing partial mappings, which is a bit of a hack, but safe + * while we are under mmap_lock(). Eventually this needs to be + * replaced by a proper DMA layer API. */ - if (!offset) { - *pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev)); - *size = MIC_DP_SIZE; - return 0; - } + vma->vm_end = vma->vm_start + MIC_DP_SIZE; + err = vdev->vpdev->hw_ops->dp_mmap(vdev->vpdev, vma); + if (err) + goto out; for (i = 0; i < vdev->dd->num_vq; i++) { struct vop_vringh *vvr = &vdev->vvr[i]; - if (offset == start) { - *pa = virt_to_phys(vvr->vring.va); - *size = vvr->vring.len; - return 0; - } - start += vvr->vring.len; - } - return -1; -} - -/* - * Maps the device page and virtio rings to user space for readonly access. - */ -static int vop_mmap(struct file *f, struct vm_area_struct *vma) -{ - struct vop_vdev *vdev = f->private_data; - unsigned long offset = vma->vm_pgoff << PAGE_SHIFT; - unsigned long pa, size = vma->vm_end - vma->vm_start, size_rem = size; - int i, err; + vma->vm_start = vma->vm_end; + vma->vm_end += vvr->vring.len; - err = vop_vdev_inited(vdev); - if (err) - goto ret; - if (vma->vm_flags & VM_WRITE) { - err = -EACCES; - goto ret; - } - while (size_rem) { - i = vop_query_offset(vdev, offset, &size, &pa); - if (i < 0) { - err = -EINVAL; - goto ret; - } - err = remap_pfn_range(vma, vma->vm_start + offset, - pa >> PAGE_SHIFT, size, - vma->vm_page_prot); + err = -EINVAL; + if (vma->vm_end > orig_end) + goto out; + err = dma_mmap_coherent(vop_dev(vdev), vma, vvr->vring.va, + le64_to_cpu(vqconfig[i].address), + vvr->vring.len); if (err) - goto ret; - size_rem -= size; - offset += size; + goto out; } -ret: +out: + /* + * Restore the original vma parameters. + */ + vma->vm_start = orig_start; + vma->vm_end = orig_end; return err; } -- 2.28.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH V4 1/2] misc: vop: change the way of allocating vring and device page 2020-10-27 10:40 ` Christoph Hellwig @ 2020-10-27 12:30 ` Sherry Sun 0 siblings, 0 replies; 6+ messages in thread From: Sherry Sun @ 2020-10-27 12:30 UTC (permalink / raw) To: Christoph Hellwig Cc: gregkh, sudeep.dutt, ashutosh.dixit, arnd, kishon, lorenzo.pieralisi, linux-kernel, linux-pci, dl-linux-imx, Andy Duan Hi Christoph, Thank you for improving the code. I think your code makes vop_mmap looks clearer. I have tested it on imx8qm platform(arm64 architecture) and it works well. I will send the code together in v5. Best regards Sherry > Subject: Re: [PATCH V4 1/2] misc: vop: change the way of allocating vring and > device page > > This looks much better, but we need to be careful to restore the original > vm_start / vm_end. What do you think about the version below, which also > simplifies vop_mmap a lot (completely untested): > > --- > From 2de72bf7444ee187a7576962d746d482c5bdd593 Mon Sep 17 00:00:00 > 2001 > From: Sherry Sun <sherry.sun@nxp.com> > Date: Mon, 26 Oct 2020 16:53:34 +0800 > Subject: misc: vop: change the way of allocating vring and device page > > Allocate vrings use dma_alloc_coherent is a common way in kernel. As the > memory interacted between two systems should use consistent memory to > avoid caching effects, same as device page memory. > > The orginal way use __get_free_pages and dma_map_single to allocate and > map vring, but not use dma_sync_single_for_cpu/device api to sync the > changes of vring between EP and RC, which will cause memory > synchronization problem for those devices which don't support hardware > dma coherent. > > Also change to use dma_mmap_coherent for mmap callback to map the > device page and vring memory to userspace. > > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com> > --- > drivers/misc/mic/bus/vop_bus.h | 2 + > drivers/misc/mic/host/mic_boot.c | 9 ++ > drivers/misc/mic/host/mic_main.c | 43 +++------- > drivers/misc/mic/vop/vop_vringh.c | 135 ++++++++++++------------------ > 4 files changed, 77 insertions(+), 112 deletions(-) > > diff --git a/drivers/misc/mic/bus/vop_bus.h > b/drivers/misc/mic/bus/vop_bus.h index 4fa02808c1e27d..e21c06aeda7a31 > 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. > + * @dp_mmap: Map the virtio device page to userspace. > * @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); > + int (*dp_mmap)(struct vop_device *vpdev, struct vm_area_struct > *vma); > 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 8cb85b8b3e199b..44ed918b49b4d2 100644 > --- a/drivers/misc/mic/host/mic_boot.c > +++ b/drivers/misc/mic/host/mic_boot.c > @@ -89,6 +89,14 @@ static void *__mic_get_dp(struct vop_device *vpdev) > return mdev->dp; > } > > +static int __mic_dp_mmap(struct vop_device *vpdev, struct > +vm_area_struct *vma) { > + struct mic_device *mdev = vpdev_to_mdev(&vpdev->dev); > + > + return dma_mmap_coherent(&mdev->pdev->dev, vma, mdev->dp, > + mdev->dp_dma_addr, MIC_DP_SIZE); > +} > + > static void __iomem *__mic_get_remote_dp(struct vop_device *vpdev) { > return NULL; > @@ -120,6 +128,7 @@ static struct vop_hw_ops vop_hw_ops = { > .ack_interrupt = __mic_ack_interrupt, > .next_db = __mic_next_db, > .get_dp = __mic_get_dp, > + .dp_mmap = __mic_dp_mmap, > .get_remote_dp = __mic_get_remote_dp, > .send_intr = __mic_send_intr, > .remap = __mic_ioremap, > diff --git a/drivers/misc/mic/host/mic_main.c > b/drivers/misc/mic/host/mic_main.c > index ea4608527ea04d..fab87a72a9a4a5 100644 > --- a/drivers/misc/mic/host/mic_main.c > +++ b/drivers/misc/mic/host/mic_main.c > @@ -10,6 +10,7 @@ > #include <linux/module.h> > #include <linux/pci.h> > #include <linux/poll.h> > +#include <linux/dma-mapping.h> > > #include <linux/mic_common.h> > #include "../common/mic_dev.h" > @@ -45,33 +46,6 @@ MODULE_DEVICE_TABLE(pci, mic_pci_tbl); > /* ID allocator for MIC devices */ > static struct ida g_mic_ida; > > -/* Initialize the device page */ > -static int mic_dp_init(struct mic_device *mdev) -{ > - mdev->dp = kzalloc(MIC_DP_SIZE, GFP_KERNEL); > - if (!mdev->dp) > - return -ENOMEM; > - > - mdev->dp_dma_addr = mic_map_single(mdev, > - mdev->dp, MIC_DP_SIZE); > - if (mic_map_error(mdev->dp_dma_addr)) { > - kfree(mdev->dp); > - dev_err(&mdev->pdev->dev, "%s %d err %d\n", > - __func__, __LINE__, -ENOMEM); > - return -ENOMEM; > - } > - mdev->ops->write_spad(mdev, MIC_DPLO_SPAD, mdev- > >dp_dma_addr); > - mdev->ops->write_spad(mdev, MIC_DPHI_SPAD, mdev- > >dp_dma_addr >> 32); > - return 0; > -} > - > -/* Uninitialize the device page */ > -static void mic_dp_uninit(struct mic_device *mdev) -{ > - mic_unmap_single(mdev, mdev->dp_dma_addr, MIC_DP_SIZE); > - kfree(mdev->dp); > -} > - > /** > * mic_ops_init: Initialize HW specific operation tables. > * > @@ -227,11 +201,16 @@ static int mic_probe(struct pci_dev *pdev, > > pci_set_drvdata(pdev, mdev); > > - rc = mic_dp_init(mdev); > - if (rc) { > - dev_err(&pdev->dev, "mic_dp_init failed rc %d\n", rc); > + mdev->dp = dma_alloc_coherent(&pdev->dev, MIC_DP_SIZE, > + &mdev->dp_dma_addr, GFP_KERNEL); > + if (!mdev->dp) { > + dev_err(&pdev->dev, "failed to allocate device page\n"); > goto smpt_uninit; > } > + > + mdev->ops->write_spad(mdev, MIC_DPLO_SPAD, mdev- > >dp_dma_addr); > + mdev->ops->write_spad(mdev, MIC_DPHI_SPAD, mdev- > >dp_dma_addr >> 32); > + > mic_bootparam_init(mdev); > mic_create_debug_dir(mdev); > > @@ -244,7 +223,7 @@ static int mic_probe(struct pci_dev *pdev, > return 0; > cleanup_debug_dir: > mic_delete_debug_dir(mdev); > - mic_dp_uninit(mdev); > + dma_free_coherent(&pdev->dev, MIC_DP_SIZE, mdev->dp, > +mdev->dp_dma_addr); > smpt_uninit: > mic_smpt_uninit(mdev); > free_interrupts: > @@ -283,7 +262,7 @@ static void mic_remove(struct pci_dev *pdev) > > cosm_unregister_device(mdev->cosm_dev); > mic_delete_debug_dir(mdev); > - mic_dp_uninit(mdev); > + dma_free_coherent(&pdev->dev, MIC_DP_SIZE, mdev->dp, > +mdev->dp_dma_addr); > mic_smpt_uninit(mdev); > mic_free_interrupts(mdev, pdev); > iounmap(mdev->aper.va); > diff --git a/drivers/misc/mic/vop/vop_vringh.c > b/drivers/misc/mic/vop/vop_vringh.c > index 7014ffe88632e5..6835648d577d57 100644 > --- a/drivers/misc/mic/vop/vop_vringh.c > +++ b/drivers/misc/mic/vop/vop_vringh.c > @@ -298,9 +298,8 @@ 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)); > + vr->va = dma_alloc_coherent(vop_dev(vdev), vr_size, > &vr_addr, > + GFP_KERNEL); > if (!vr->va) { > ret = -ENOMEM; > dev_err(vop_dev(vdev), "%s %d err %d\n", @@ - > 310,15 +309,6 @@ 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; > - } > vqconfig[i].address = cpu_to_le64(vr_addr); > > vring_init(&vr->vr, num, vr->va, MIC_VIRTIO_RING_ALIGN); > @@ -339,11 +329,9 @@ 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); > + vvr->buf = dma_alloc_coherent(vop_dev(vdev), > + VOP_INT_DMA_BUF_SIZE, > + &vvr->buf_da, GFP_KERNEL); > } > > snprintf(irqname, sizeof(irqname), "vop%dvirtio%d", vpdev->index, > @@ -382,10 +370,8 @@ 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)); > + dma_free_coherent(vop_dev(vdev), vvr->vring.len, vvr- > >vring.va, > + le64_to_cpu(vqconfig[j].address)); > } > return ret; > } > @@ -433,17 +419,12 @@ 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)); > + dma_free_coherent(vop_dev(vdev), > VOP_INT_DMA_BUF_SIZE, > + vvr->buf, vvr->buf_da); > 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)); > + dma_free_coherent(vop_dev(vdev), vvr->vring.len, vvr- > >vring.va, > + le64_to_cpu(vqconfig[i].address)); > } > /* > * Order the type update with previous stores. This write barrier @@ > -1042,13 +1023,27 @@ static __poll_t vop_poll(struct file *f, poll_table *wait) > return mask; > } > > -static inline int > -vop_query_offset(struct vop_vdev *vdev, unsigned long offset, > - unsigned long *size, unsigned long *pa) > +/* > + * Maps the device page and virtio rings to user space for readonly access. > + */ > +static int vop_mmap(struct file *f, struct vm_area_struct *vma) > { > - struct vop_device *vpdev = vdev->vpdev; > - unsigned long start = MIC_DP_SIZE; > - int i; > + struct vop_vdev *vdev = f->private_data; > + struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd); > + unsigned long orig_start = vma->vm_start; > + unsigned long orig_end = vma->vm_end; > + int err, i; > + > + if (!vdev->vpdev->hw_ops->dp_mmap) > + return -EINVAL; > + if (vma->vm_pgoff) > + return -EINVAL; > + if (vma->vm_flags & VM_WRITE) > + return -EACCES; > + > + err = vop_vdev_inited(vdev); > + if (err) > + return err; > > /* > * MMAP interface is as follows: > @@ -1057,58 +1052,38 @@ vop_query_offset(struct vop_vdev *vdev, > unsigned long offset, > * 0x1000 first vring > * 0x1000 + size of 1st vring second vring > * .... > + * > + * We manipulate vm_start/vm_end to trick dma_mmap_coherent > into > + * performing partial mappings, which is a bit of a hack, but safe > + * while we are under mmap_lock(). Eventually this needs to be > + * replaced by a proper DMA layer API. > */ > - if (!offset) { > - *pa = virt_to_phys(vpdev->hw_ops->get_dp(vpdev)); > - *size = MIC_DP_SIZE; > - return 0; > - } > + vma->vm_end = vma->vm_start + MIC_DP_SIZE; > + err = vdev->vpdev->hw_ops->dp_mmap(vdev->vpdev, vma); > + if (err) > + goto out; > > for (i = 0; i < vdev->dd->num_vq; i++) { > struct vop_vringh *vvr = &vdev->vvr[i]; > > - if (offset == start) { > - *pa = virt_to_phys(vvr->vring.va); > - *size = vvr->vring.len; > - return 0; > - } > - start += vvr->vring.len; > - } > - return -1; > -} > - > -/* > - * Maps the device page and virtio rings to user space for readonly access. > - */ > -static int vop_mmap(struct file *f, struct vm_area_struct *vma) -{ > - struct vop_vdev *vdev = f->private_data; > - unsigned long offset = vma->vm_pgoff << PAGE_SHIFT; > - unsigned long pa, size = vma->vm_end - vma->vm_start, size_rem = > size; > - int i, err; > + vma->vm_start = vma->vm_end; > + vma->vm_end += vvr->vring.len; > > - err = vop_vdev_inited(vdev); > - if (err) > - goto ret; > - if (vma->vm_flags & VM_WRITE) { > - err = -EACCES; > - goto ret; > - } > - while (size_rem) { > - i = vop_query_offset(vdev, offset, &size, &pa); > - if (i < 0) { > - err = -EINVAL; > - goto ret; > - } > - err = remap_pfn_range(vma, vma->vm_start + offset, > - pa >> PAGE_SHIFT, size, > - vma->vm_page_prot); > + err = -EINVAL; > + if (vma->vm_end > orig_end) > + goto out; > + err = dma_mmap_coherent(vop_dev(vdev), vma, vvr- > >vring.va, > + le64_to_cpu(vqconfig[i].address), > + vvr->vring.len); > if (err) > - goto ret; > - size_rem -= size; > - offset += size; > + goto out; > } > -ret: > +out: > + /* > + * Restore the original vma parameters. > + */ > + vma->vm_start = orig_start; > + vma->vm_end = orig_end; > return err; > } > > -- > 2.28.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V4 2/2] misc: vop: do not allocate and reassign the used ring 2020-10-26 8:53 [PATCH V4 0/2] Change vring space from nomal memory to dma coherent memory Sherry Sun 2020-10-26 8:53 ` [PATCH V4 1/2] misc: vop: change the way of allocating vring and device page Sherry Sun @ 2020-10-26 8:53 ` Sherry Sun 2020-10-26 10:35 ` kernel test robot 1 sibling, 1 reply; 6+ messages in thread From: Sherry Sun @ 2020-10-26 8:53 UTC (permalink / raw) To: hch, gregkh, sudeep.dutt, ashutosh.dixit, arnd, kishon, lorenzo.pieralisi Cc: linux-kernel, linux-pci, linux-imx, fugang.duan We don't need to allocate and reassign the used ring here and remove the used_address_updated flag.Since RC has allocated the entire vring, including the used ring. Simply add the corresponding offset can get the used ring address. If following 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 api, 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_debugfs.c | 2 -- drivers/misc/mic/vop/vop_main.c | 51 ++++++------------------------ drivers/misc/mic/vop/vop_vringh.c | 31 ------------------ include/uapi/linux/mic_common.h | 5 ++- 4 files changed, 11 insertions(+), 78 deletions(-) diff --git a/drivers/misc/mic/vop/vop_debugfs.c b/drivers/misc/mic/vop/vop_debugfs.c index 9d4f175f4dd1..05eca4a98585 100644 --- a/drivers/misc/mic/vop/vop_debugfs.c +++ b/drivers/misc/mic/vop/vop_debugfs.c @@ -79,8 +79,6 @@ static int vop_dp_show(struct seq_file *s, void *pos) seq_printf(s, "Vdev reset %d\n", dc->vdev_reset); seq_printf(s, "Guest Ack %d ", dc->guest_ack); seq_printf(s, "Host ack %d\n", dc->host_ack); - seq_printf(s, "Used address updated %d ", - dc->used_address_updated); seq_printf(s, "Vdev 0x%llx\n", dc->vdev); seq_printf(s, "c2h doorbell %d ", dc->c2h_vdev_db); seq_printf(s, "h2c doorbell %d\n", dc->h2c_vdev_db); diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c index 714b94f42d38..07e2732b24bb 100644 --- a/drivers/misc/mic/vop/vop_main.c +++ b/drivers/misc/mic/vop/vop_main.c @@ -250,10 +250,6 @@ 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])); vring_del_virtqueue(vq); vpdev->hw_ops->unmap(vpdev, vdev->vr[n]); vdev->vr[n] = NULL; @@ -340,8 +336,9 @@ 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])); + used = (void *)va + PAGE_ALIGN(sizeof(struct vring_desc) * + le16_to_cpu(config.num) + sizeof(__u16) * + (3 + le16_to_cpu(config.num))); vdev->used_virt[index] = used; if (!used) { err = -ENOMEM; @@ -355,27 +352,17 @@ static struct virtqueue *vop_find_vq(struct virtio_device *dev, name, used); if (!vq) { err = -ENOMEM; - goto free_used; + goto unmap; } - 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; - } + vdev->used[index] = le64_to_cpu(config.address) + + PAGE_ALIGN(sizeof(struct vring_desc) * + le16_to_cpu(config.num) + sizeof(__u16) * + (3 + le16_to_cpu(config.num))); writeq(vdev->used[index], &vqconfig->used_address); vq->priv = vdev; return vq; -del_vq: - vring_del_virtqueue(vq); -free_used: - free_pages((unsigned long)used, - get_order(vdev->used_size[index])); unmap: vpdev->hw_ops->unmap(vpdev, vdev->vr[index]); return ERR_PTR(err); @@ -388,9 +375,7 @@ static int vop_find_vqs(struct virtio_device *dev, unsigned nvqs, struct irq_affinity *desc) { struct _vop_vdev *vdev = to_vopvdev(dev); - struct vop_device *vpdev = vdev->vpdev; - struct mic_device_ctrl __iomem *dc = vdev->dc; - int i, err, retry, queue_idx = 0; + int i, err, queue_idx = 0; /* We must have this many virtqueues. */ if (nvqs > ioread8(&vdev->desc->num_vq)) @@ -412,24 +397,6 @@ static int vop_find_vqs(struct virtio_device *dev, unsigned nvqs, } } - iowrite8(1, &dc->used_address_updated); - /* - * Send an interrupt to the host to inform it that used - * rings have been re-assigned. - */ - vpdev->hw_ops->send_intr(vpdev, vdev->c2h_vdev_db); - for (retry = 100; --retry;) { - if (!ioread8(&dc->used_address_updated)) - break; - msleep(100); - } - - dev_dbg(_vop_dev(vdev), "%s: retry: %d\n", __func__, retry); - if (!retry) { - err = -ENODEV; - goto error; - } - return 0; error: vop_del_vqs(dev); diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c index df77681c97e6..15dd3405789c 100644 --- a/drivers/misc/mic/vop/vop_vringh.c +++ b/drivers/misc/mic/vop/vop_vringh.c @@ -53,33 +53,6 @@ static void _vop_notify(struct vringh *vrh) vpdev->hw_ops->send_intr(vpdev, db); } -static void vop_virtio_init_post(struct vop_vdev *vdev) -{ - struct mic_vqconfig *vqconfig = mic_vq_config(vdev->dd); - struct vop_device *vpdev = vdev->vpdev; - int i, used_size; - - for (i = 0; i < vdev->dd->num_vq; i++) { - used_size = PAGE_ALIGN(sizeof(u16) * 3 + - sizeof(struct vring_used_elem) * - le16_to_cpu(vqconfig->num)); - if (!le64_to_cpu(vqconfig[i].used_address)) { - 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); - } - - vdev->dc->used_address_updated = 0; - - dev_info(vop_dev(vdev), "%s: device type %d LINKUP\n", - __func__, vdev->virtio_id); -} - static inline void vop_virtio_device_reset(struct vop_vdev *vdev) { int i; @@ -130,9 +103,6 @@ static void vop_bh_handler(struct work_struct *work) struct vop_vdev *vdev = container_of(work, struct vop_vdev, virtio_bh_work); - if (vdev->dc->used_address_updated) - vop_virtio_init_post(vdev); - if (vdev->dc->vdev_reset) vop_virtio_device_reset(vdev); @@ -250,7 +220,6 @@ static void vop_init_device_ctrl(struct vop_vdev *vdev, dc->guest_ack = 0; dc->vdev_reset = 0; dc->host_ack = 0; - dc->used_address_updated = 0; dc->c2h_vdev_db = -1; dc->h2c_vdev_db = -1; vdev->dc = dc; diff --git a/include/uapi/linux/mic_common.h b/include/uapi/linux/mic_common.h index 504e523f702c..fe660d486b20 100644 --- a/include/uapi/linux/mic_common.h +++ b/include/uapi/linux/mic_common.h @@ -56,8 +56,7 @@ struct mic_device_desc { * @vdev_reset: Set to 1 by guest to indicate virtio device has been reset. * @guest_ack: Set to 1 by guest to ack a command. * @host_ack: Set to 1 by host to ack a command. - * @used_address_updated: Set to 1 by guest when the used address should be - * updated. + * @must_be_zero: Reserved because this bit is no longer needed. * @c2h_vdev_db: The doorbell number to be used by guest. Set by host. * @h2c_vdev_db: The doorbell number to be used by host. Set by guest. */ @@ -67,7 +66,7 @@ struct mic_device_ctrl { __u8 vdev_reset; __u8 guest_ack; __u8 host_ack; - __u8 used_address_updated; + __u8 must_be_zero; __s8 c2h_vdev_db; __s8 h2c_vdev_db; } __attribute__ ((aligned(8))); -- 2.17.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V4 2/2] misc: vop: do not allocate and reassign the used ring 2020-10-26 8:53 ` [PATCH V4 2/2] misc: vop: do not allocate and reassign the used ring Sherry Sun @ 2020-10-26 10:35 ` kernel test robot 0 siblings, 0 replies; 6+ messages in thread From: kernel test robot @ 2020-10-26 10:35 UTC (permalink / raw) To: Sherry Sun, hch, gregkh, sudeep.dutt, ashutosh.dixit, arnd, kishon, lorenzo.pieralisi Cc: kbuild-all, linux-kernel, linux-pci, linux-imx [-- Attachment #1: Type: text/plain, Size: 4753 bytes --] Hi Sherry, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on char-misc/char-misc-testing] [also build test WARNING on soc/for-next linus/master v5.10-rc1 next-20201026] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Sherry-Sun/Change-vring-space-from-nomal-memory-to-dma-coherent-memory/20201026-170616 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 726eb70e0d34dc4bc4dada71f52bba8ed638431e config: i386-randconfig-s001-20201026 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.3-56-gc09e8239-dirty # https://github.com/0day-ci/linux/commit/9ec4633ad59551e4780046500b368eb7993588f3 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Sherry-Sun/Change-vring-space-from-nomal-memory-to-dma-coherent-memory/20201026-170616 git checkout 9ec4633ad59551e4780046500b368eb7993588f3 # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> "sparse warnings: (new ones prefixed by >>)" >> drivers/misc/mic/vop/vop_main.c:339:17: sparse: sparse: cast removes address space '__iomem' of expression vim +/__iomem +339 drivers/misc/mic/vop/vop_main.c 289 290 /* 291 * This routine will assign vring's allocated in host/io memory. Code in 292 * virtio_ring.c however continues to access this io memory as if it were local 293 * memory without io accessors. 294 */ 295 static struct virtqueue *vop_find_vq(struct virtio_device *dev, 296 unsigned index, 297 void (*callback)(struct virtqueue *vq), 298 const char *name, bool ctx) 299 { 300 struct _vop_vdev *vdev = to_vopvdev(dev); 301 struct vop_device *vpdev = vdev->vpdev; 302 struct mic_vqconfig __iomem *vqconfig; 303 struct mic_vqconfig config; 304 struct virtqueue *vq; 305 void __iomem *va; 306 struct _mic_vring_info __iomem *info; 307 void *used; 308 int vr_size, _vr_size, err, magic; 309 u8 type = ioread8(&vdev->desc->type); 310 311 if (index >= ioread8(&vdev->desc->num_vq)) 312 return ERR_PTR(-ENOENT); 313 314 if (!name) 315 return ERR_PTR(-ENOENT); 316 317 /* First assign the vring's allocated in host memory */ 318 vqconfig = _vop_vq_config(vdev->desc) + index; 319 memcpy_fromio(&config, vqconfig, sizeof(config)); 320 _vr_size = round_up(vring_size(le16_to_cpu(config.num), MIC_VIRTIO_RING_ALIGN), 4); 321 vr_size = PAGE_ALIGN(_vr_size + sizeof(struct _mic_vring_info)); 322 va = vpdev->hw_ops->remap(vpdev, le64_to_cpu(config.address), vr_size); 323 if (!va) 324 return ERR_PTR(-ENOMEM); 325 vdev->vr[index] = va; 326 memset_io(va, 0x0, _vr_size); 327 328 info = va + _vr_size; 329 magic = ioread32(&info->magic); 330 331 if (WARN(magic != MIC_MAGIC + type + index, "magic mismatch")) { 332 err = -EIO; 333 goto unmap; 334 } 335 336 vdev->used_size[index] = PAGE_ALIGN(sizeof(__u16) * 3 + 337 sizeof(struct vring_used_elem) * 338 le16_to_cpu(config.num)); > 339 used = (void *)va + PAGE_ALIGN(sizeof(struct vring_desc) * 340 le16_to_cpu(config.num) + sizeof(__u16) * 341 (3 + le16_to_cpu(config.num))); 342 vdev->used_virt[index] = used; 343 if (!used) { 344 err = -ENOMEM; 345 dev_err(_vop_dev(vdev), "%s %d err %d\n", 346 __func__, __LINE__, err); 347 goto unmap; 348 } 349 350 vq = vop_new_virtqueue(index, le16_to_cpu(config.num), dev, ctx, 351 (void __force *)va, vop_notify, callback, 352 name, used); 353 if (!vq) { 354 err = -ENOMEM; 355 goto unmap; 356 } 357 358 vdev->used[index] = le64_to_cpu(config.address) + 359 PAGE_ALIGN(sizeof(struct vring_desc) * 360 le16_to_cpu(config.num) + sizeof(__u16) * 361 (3 + le16_to_cpu(config.num))); 362 writeq(vdev->used[index], &vqconfig->used_address); 363 364 vq->priv = vdev; 365 return vq; 366 unmap: 367 vpdev->hw_ops->unmap(vpdev, vdev->vr[index]); 368 return ERR_PTR(err); 369 } 370 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 30479 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-27 12:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-26 8:53 [PATCH V4 0/2] Change vring space from nomal memory to dma coherent memory Sherry Sun 2020-10-26 8:53 ` [PATCH V4 1/2] misc: vop: change the way of allocating vring and device page Sherry Sun 2020-10-27 10:40 ` Christoph Hellwig 2020-10-27 12:30 ` Sherry Sun 2020-10-26 8:53 ` [PATCH V4 2/2] misc: vop: do not allocate and reassign the used ring Sherry Sun 2020-10-26 10:35 ` kernel test robot
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).