RE: [PATCH V2 1/4] misc: vop: change the way of allocating vring
Hi Christoph, > > > + vr->va = dma_alloc_coherent(vop_dev(vdev), vr_size, > _addr, > > +GFP_KERNEL); > > Please stick to 80 character lines unless you have a really good reason not > to. Okay, will change it in V3. Regards Sherry
RE: [PATCH V2 2/4] misc: vop: do not allocate and reassign the used ring
Hi Christoph,
> > diff --git a/include/uapi/linux/mic_common.h
> > b/include/uapi/linux/mic_common.h index 504e523f702c..e680fe27af69
> > 100644
> > --- a/include/uapi/linux/mic_common.h
> > +++ b/include/uapi/linux/mic_common.h
> > @@ -56,8 +56,6 @@ 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.
> > * @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 +65,6 @@ struct mic_device_ctrl {
> > __u8 vdev_reset;
> > __u8 guest_ack;
> > __u8 host_ack;
> > - __u8 used_address_updated;
> > __s8 c2h_vdev_db;
> > __s8 h2c_vdev_db;
> > } __attribute__ ((aligned(8)));
>
> This is an ABI change.
Yes, it is. But I noticed that this structure is only used by the vop driver.
And until now I haven't seen any user tools use it, including the user tool
mpssd which is corresponding to the vop driver.
Regards
Sherry
RE: [PATCH V2 3/4] misc: vop: simply return the saved dma address instead of virt_to_phys
Hi Christoph,
> On Tue, Sep 29, 2020 at 04:44:24PM +0800, Sherry Sun wrote:
> > The device page and vring should use consistent memory which are
> > allocated by dma_alloc_coherent api, 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.
>
> More importantly you can't just all virt_to_phys on a return value from
> dma_alloc_coherent, so this needs to be folded into patch 1.
Okay, will move this change into patch 1.
>
> > 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");
> > + return -EINVAL;
> > + }
>
> I don't think we need the NULL check here. Wouldn't it also make sense to
> return the virtual and DMA address from ->get_dp instead of adding another
> method?
Do you mean that we should only change the original ->get_dp callback to return
virtual
and DMA address at the same time, instead of adding the ->get_dp_dma callback?
Regards
Sherry
RE: [PATCH V2 4/4] misc: vop: mapping kernel memory to user space as noncached
Hi Christoph, > On Tue, Sep 29, 2020 at 04:44:25PM +0800, Sherry Sun wrote: > > Mapping kernel space memory to user space as noncached, since user > > space need check the updates of avail_idx and device page flags timely. > > > > Signed-off-by: Joakim Zhang > > Signed-off-by: Sherry Sun > > --- > > drivers/misc/mic/vop/vop_vringh.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/misc/mic/vop/vop_vringh.c > > b/drivers/misc/mic/vop/vop_vringh.c > > index 4d5feb39aeb7..6e193bd64ef1 100644 > > --- a/drivers/misc/mic/vop/vop_vringh.c > > +++ b/drivers/misc/mic/vop/vop_vringh.c > > @@ -1057,7 +1057,7 @@ 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); > > + pgprot_noncached(vma->vm_page_prot)); > > You can't call remap_pfn_range on memory returned from > dma_alloc_coherent (which btw is not marked uncached on many platforms). > > You need to use the dma_mmap_coherent helper instead. And this also > needs to go into the first patch. Okay, will try to use dma_mmap_coherent api in V3. Thanks. Regards Sherry
RE: [PATCH V2 3/4] misc: vop: simply return the saved dma address instead of virt_to_phys
Hi Christoph,
> On Tue, Sep 29, 2020 at 01:10:12PM +0000, Sherry Sun wrote:
> > > > 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");
> > > > + return -EINVAL;
> > > > + }
> > >
> > > I don't think we need the NULL check here. Wouldn't it also make
> > > sense to return the virtual and DMA address from ->get_dp instead of
> > > adding another method?
> >
> > Do you mean that we should only change the original ->get_dp callback
> > to return virtual and DMA address at the same time, instead of adding the -
> >get_dp_dma callback?
>
> That was my intention when writing it, yes. But it seems like most other
> callers don't care. Maybe move the invocation of dma_mmap_coherent into
> a new ->mmap helper, that way it seems like the calling code doesn't need to
> know about the dma_addr_t at all.
>
> That being said the layering in the code keeps puzzling me. As far as I can
> tell
> only a single instance of struct vop_driver even exists, so we might be able
> to
> kill all the indirections entirely.
There may be some misunderstandings here.
For ->get_dp_dma callback, it is used to get the device page dma address,
which is allocated by MIC layer instead of vop layer.
For Intel mic, it still use kzalloc and dma_map_single apis, although we
recommended and we did use dma_alloc_coherent to get consistent device
page memory on our i.MX platform, but we didn't change the original
implementation
method of Intel mic till now, as our main goal is to change the vop code to
make it
more generic.
Which is means that the device page may use different allocate methods for
different platform, and now it is transparent to the vop layer.
So I think here use ->get_dp_dma callback to get device page dma address
is the most simple and convenient way.
We change to use dma_alloc_coherent in patch 1 to allocate vrings memory, as it
is
the main job that the vop layer is responsible for.
So I still suggest to use ->get_dp or ->get_dp_dma callback for device page
here, what do you think?
Regards
Sherry
RE: [PATCH V5 0/2] Change vring space from nomal memory to dma coherent memory
> Subject: Re: [PATCH V5 0/2] Change vring space from nomal memory to dma > coherent memory > > On Wed, Oct 28, 2020 at 06:05:28AM +0000, Sherry Sun wrote: > > Hi Greg, > > > > > Subject: Re: [PATCH V5 0/2] Change vring space from nomal memory to > > > dma coherent memory > > > > > > On Wed, Oct 28, 2020 at 10:03:03AM +0800, Sherry Sun wrote: > > > > Changes in V5: > > > > 1. Reorganize the vop_mmap function code in patch 1, which is done > > > > by > > > Christoph. > > > > 2. Completely remove the unnecessary code related to reassign the > > > > used ring for card in patch 2. > > > > > > > > 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 vrings 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 | 4 - > > > > drivers/misc/mic/vop/vop_main.c| 70 +--- > > > > drivers/misc/mic/vop/vop_vringh.c | 166 ++--- > > > > include/uapi/linux/mic_common.h| 9 +- > > > > 7 files changed, 85 insertions(+), 218 deletions(-) > > > > > > Have you all seen: > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F% > 25 > > > > 2Flore.kernel.org%2Fr%2F8c1443136563de34699d2c084df478181c205db4.16 > > > > 03854416.git.sudeep.dutt%40intel.comdata=04%7C01%7Csherry.sun% > > > > 40nxp.com%7Cc19c987667434969847e08d87b0685e8%7C686ea1d3bc2b4c6f > > > > a92cd99c5c301635%7C0%7C0%7C637394615238940323%7CUnknown%7CTW > > > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > > > > VCI6Mn0%3D%7C1000sdata=Zq%2FtHWTq%2BuIVBYXFGoeBmq0JJzYd > > > 9zDyv4NVN4TpC%2FU%3Dreserved=0 > > > > > > Looks like this code is asking to just be deleted, is that ok with you? > > > > Yes, I saw that patch. I'm ok with it. > > Great, can you please provide a "Reviewed-by:" or "Acked-by:" for it? > Sure. Best regards Sherry > thanks, > > greg k-h
[PATCH V5 2/2] misc: vop: do not allocate and reassign the used ring
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
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/vop/vop_debugfs.c | 4 --
drivers/misc/mic/vop/vop_main.c| 70 ++
drivers/misc/mic/vop/vop_vringh.c | 31 -
include/uapi/linux/mic_common.h| 9 ++--
4 files changed, 8 insertions(+), 106 deletions(-)
diff --git a/drivers/misc/mic/vop/vop_debugfs.c
b/drivers/misc/mic/vop/vop_debugfs.c
index 9d4f175f4dd1..27373b6dcea2 100644
--- a/drivers/misc/mic/vop/vop_debugfs.c
+++ b/drivers/misc/mic/vop/vop_debugfs.c
@@ -62,8 +62,6 @@ static int vop_dp_show(struct seq_file *s, void *pos)
seq_printf(s, "address 0x%llx ",
vqconfig->address);
seq_printf(s, "num %d ", vqconfig->num);
- seq_printf(s, "used address 0x%llx\n",
- vqconfig->used_address);
}
features = (__u32 *)mic_vq_features(d);
@@ -79,8 +77,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..226f462ab6a9 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -32,9 +32,6 @@
* @dc: Virtio device control
* @vpdev: VOP device which is the parent for this virtio device
* @vr: Buffer for accessing the VRING
- * @used_virt: Virtual address of used ring
- * @used: DMA address of used ring
- * @used_size: Size of the used buffer
* @reset_done: Track whether VOP reset is complete
* @virtio_cookie: Cookie returned upon requesting a interrupt
* @c2h_vdev_db: The doorbell used by the guest to interrupt the host
@@ -47,9 +44,6 @@ struct _vop_vdev {
struct mic_device_ctrl __iomem *dc;
struct vop_device *vpdev;
void __iomem *vr[VOP_MAX_VRINGS];
- void *used_virt[VOP_MAX_VRINGS];
- dma_addr_t used[VOP_MAX_VRINGS];
- int used_size[VOP_MAX_VRINGS];
struct completion reset_done;
struct mic_irq *virtio_cookie;
int c2h_vdev_db;
@@ -250,10 +244,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(>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;
@@ -278,14 +268,12 @@ static struct virtqueue *vop_new_virtqueue(unsigned int
index,
void *pages,
bool (*notify)(struct virtqueue *vq),
void (*callback)(struct virtqueue *vq),
- const char *name,
- void *used)
+ const char *name)
{
bool weak_barriers = false;
struct vring vring;
vring_init(, num, pages, MIC_VIRTIO_RING_ALIGN);
- vring.used = used;
return __vring_new_virtqueue(index, vring, vdev, weak_barriers, context,
notify, callback, name);
@@ -308,7 +296,6 @@ static struct virtqueue *vop_find_vq(struct virtio_device
*dev,
struct virtqueue *vq;
void __iomem *va;
struct _mic_vring_info __iomem *info;
- void *used;
int vr_size, _vr_size, err, magic;
u8 type = ioread8(>desc->type);
@@ -337,45 +324,16 @@ static struct virtqueue *vop_find_vq(struct virtio_device
*dev,
[PATCH V5 1/2] misc: vop: change the way of allocating vrings 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: Christoph Hellwig
Signed-off-by: Sherry Sun
Signed-off-by: Joakim Zhang
---
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 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(>dev);
+
+ return dma_mmap_coherent(>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
#include
#include
+#include
#include
#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(>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(>dev, "mic_dp_init failed rc %d\n", rc);
+ mdev->dp = dma_alloc_coherent(>dev, MIC_DP_SIZE,
+ >dp_dma_addr, GFP_KERNEL);
+ if (!mdev->dp) {
+ dev_err(>dev, "failed to al
RE: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Sudeep, > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > On Wed, 2020-10-28 at 01:47 +0000, Sherry Sun wrote: > > Hi Vincent, > > > > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign > > > the used ring > > > > > > On Tue, Oct 27, 2020 at 08:05:43AM +0100, Sherry Sun wrote: > > > > Can you help test the patch about removing the codes of reassign > > > > used ring, and comment on the impact for Intel MIC platform? > > > > Thanks for any help. > > > > > > I don't have access to MIC hardware myself, either. > > > > > > But this patch is quite certainly going to break it since guests > > > using a kernel without the patch will not work with hosts with a > > > kernel with the patch. > > > > Thanks for your reply. > > This patch can be used by both guests and hosts. > > I have tested it on imx8qm platform(both guest and host are ARM64 > > architecture), and it works well. > > So I guess Intel MIC won't meet big problems when both guest and > > Hi Greg/Sherry, > > Both Ashutosh and I have moved on to other projects. The MIC devices have > been discontinued. I have just sent across a patch to remove the MIC drivers > from the kernel tree. > > We are very glad to see that Sherry is able to reuse some of the VOP logic > and it is working well. It is best if the MIC drivers are removed so Sherry > can > add the specific VOP logic required for imx8qm subsequently without having > to worry about other driver dependencies. > Hoping this results in a cleaner imx8qm driver moving forward. I'm ok with your patch. Since you have deprecated the MIC related code, may I ask do you have a better solution instead of vop/scif? Best regards Sherry > > Thanks, > Sudeep Dutt
[PATCH V5 0/2] Change vring space from nomal memory to dma coherent memory
Changes in V5: 1. Reorganize the vop_mmap function code in patch 1, which is done by Christoph. 2. Completely remove the unnecessary code related to reassign the used ring for card in patch 2. 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 vrings 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 | 4 - drivers/misc/mic/vop/vop_main.c| 70 +--- drivers/misc/mic/vop/vop_vringh.c | 166 ++--- include/uapi/linux/mic_common.h| 9 +- 7 files changed, 85 insertions(+), 218 deletions(-) -- 2.17.1
RE: [PATCH V3 1/4] misc: vop: change the way of allocating vring and device page
Hi Christoph,
>
> > static int mic_dp_init(struct mic_device *mdev) {
> > - mdev->dp = kzalloc(MIC_DP_SIZE, GFP_KERNEL);
> > + mdev->dp = dma_alloc_coherent(>pdev->dev, MIC_DP_SIZE,
> > + >dp_dma_addr, 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(>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;
> > @@ -68,8 +62,7 @@ static int mic_dp_init(struct mic_device *mdev)
> > /* 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);
> > + dma_free_coherent(>pdev->dev, MIC_DP_SIZE, mdev->dp,
> > +mdev->dp_dma_addr);
> > }
>
> This adds an over 80 char line. Also please just kill mic_dp_init and
> mic_dp_uninit and inline those into the callers.
Okay, will fix them.
>
> > + vvr->buf = dma_alloc_coherent(vop_dev(vdev),
> VOP_INT_DMA_BUF_SIZE,
> > + >buf_da, GFP_KERNEL);
>
> Another overly long line.
>
> > @@ -1068,7 +1049,7 @@ vop_query_offset(struct vop_vdev *vdev,
> unsigned long offset,
> > struct vop_vringh *vvr = >vvr[i];
> >
> > if (offset == start) {
> > - *pa = virt_to_phys(vvr->vring.va);
> > + *pa = vqconfig[i].address;
>
> vqconfig[i].address is a __le64, so this needs an endian swap.
>
> But more importantly the caller of vop_query_offset, vop_mmap, uses
> remap_pfn_range and pa. You cannot mix remap_pfn_range with DMA
> coherent allocations, it can only be mmaped using dma_mmap_coherent.
Will change to use dma_mmap_coherent in V4.
Best regards
Sherry
RE: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Greg & Christoph, > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > On Thu, Oct 22, 2020 at 01:06:36PM +0800, Sherry Sun wrote: > > 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. > > Someone needs to verify this vs the existing intel implementations. Hi Greg, @gre...@linuxfoundation.org, sorry I don't have the Intel MIC devices so cannot test it, do you know anyone who can help test this patch changes on the Intel MIC platform? Thanks. > > > - used = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, > > - get_order(vdev->used_size[index])); > > + used = va + PAGE_ALIGN(sizeof(struct vring_desc) * > > +le16_to_cpu(config.num) + > > This adds an over 80 char line. Hi Christoph, will fix it in V4, thanks. > > > + vdev->used[index] = config.address + PAGE_ALIGN(sizeof(struct > > +vring_desc) * le16_to_cpu(config.num) + > > Again. Best regards Sherry
[PATCH V4 0/2] Change vring space from nomal memory to dma coherent memory
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
[PATCH V4 2/2] misc: vop: do not allocate and reassign the used ring
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
Signed-off-by: Joakim Zhang
---
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(>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(>dev, used,
- vdev->used_size[index],
- DMA_BIDIRECTIONAL);
- if (dma_mapping_error(>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], >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 virtqu
[PATCH V4 1/2] 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
Signed-off-by: Sherry Sun
---
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(>dev);
+
+ return dma_mmap_coherent(>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
#include
#include
+#include
#include
#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(>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(>dev, "mic_dp_init failed rc %d\n", rc);
+ mdev->dp = dma_alloc_coherent(>dev, MIC_DP_SIZE,
+ >dp_dma_addr, GFP_KERNEL);
+ if (!mdev->dp) {
+ dev_err(>dev, "failed to allocate device page\n");
RE: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-Transparent Bridge
Hi Kishon, > Subject: Re: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non- > Transparent Bridge > > Hi Sherry, > > On 11/11/20 8:19 am, Sherry Sun wrote: > > Hi Kishon, > > > >> Subject: Re: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express > >> Non- Transparent Bridge > >> > >> Hi Sherry, Arnd, > >> > >> On 10/11/20 8:29 pm, Arnd Bergmann wrote: > >>> On Tue, Nov 10, 2020 at 3:20 PM Kishon Vijay Abraham I > >>> > >> wrote: > >>>> On 10/11/20 7:55 am, Sherry Sun wrote: > >>> > >>>>> But for VOP, only two boards are needed(one board as host and one > >>>>> board as card) to realize the communication between the two > >>>>> systems, > >> so my question is what are the advantages of using NTB? > >>>> > >>>> NTB is a bridge that facilitates communication between two > >>>> different systems. So it by itself will not be source or sink of > >>>> any data unlike a normal EP to RP system (or the VOP) which will be > >>>> source or sink > >> of data. > >>>> > >>>>> Because I think the architecture of NTB seems more complicated. > >>>>> Many > >> thanks! > >>>> > >>>> yeah, I think it enables a different use case all together. > >>>> Consider you have two x86 HOST PCs (having RP) and they have to be > >>>> communicate using PCIe. NTB can be used in such cases for the two > >>>> x86 PCs to communicate with each other over PCIe, which wouldn't be > >>>> possible > >> without NTB. > >>> > >>> I think for VOP, we should have an abstraction that can work on > >>> either NTB or directly on the endpoint framework but provide an > >>> interface that then lets you create logical devices the same way. > >>> > >>> Doing VOP based on NTB plus the new NTB_EPF driver would also work > >>> and just move the abstraction somewhere else, but I guess it would > >>> complicate setting it up for those users that only care about the > >>> simpler endpoint case. > >> > >> I'm not sure if you've got a chance to look at [1], where I added > >> support for RP<->EP system both running Linux, with EP configured > >> using Linux EP framework (as well as HOST ports connected to NTB > >> switch, patches 20 and 21, that uses the Linux NTB framework) to > >> communicate using virtio over PCIe. > >> > > > > I saw your patches at [1], here you take a rpmsg as an example to > > communicate between two SoCs using PCIe RC<->EP and HOST1-NTB- > HOST2 for different usercases. > > The VOP code works under the PCIe RC<->EP framework, which means that > > we can also make VOP works under the Linux NTB framework, just like the > rpmsg way you did here, right? > > Does VOP really work with EP framework? At-least whatever is in upstream > doesn't seem to indicate so. > We did write a pci_epf driver to support VOP, looks like pci-epf-test.c, and it works well. So certainly VOP can work with EP framework. But it's a pity that the VOP related codes has been deleted before we send the pci_epf_vop driver patches to upstream. > The NTB framework lets one host with RP port to communicate with another > host with RP port. > > The EP Framework lets one device with EP port to communicate with a host > with RP port. > > Rest of the trick should be how you tie them together. > > PCIe framework creates "pci_device" for each of the devices it enumerates. > NTB framework works on this pci_device to communicate with the remote > host using PCIe bridge. The remote host will use NTB framework as well. > > So depends on what interfaces VOP device provides you can use either NTB > framework or EP framework. If it's going to connect two different devices in > turn creating pci_device on each of the systems, then you can use NTB > framework. > Thanks for your detailed explanation! It is clear. I think maybe VOP is suitable for the basic PCIe framework instead of NTB. Best regards Sherry > Regards > Kishon
[PATCH V3 3/4] misc: vop: simply return the saved dma address instead of virt_to_phys
The device page and vring should use consistent memory which are
allocated by dma_alloc_coherent api, 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
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/bus/vop_bus.h| 2 ++
drivers/misc/mic/host/mic_boot.c | 8
drivers/misc/mic/vop/vop_vringh.c | 8 +++-
3 files changed, 17 insertions(+), 1 deletion(-)
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 8cb85b8b3e19..9591bf609f57 100644
--- a/drivers/misc/mic/host/mic_boot.c
+++ b/drivers/misc/mic/host/mic_boot.c
@@ -89,6 +89,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(>dev);
+
+ return mdev->dp_dma_addr;
+}
+
static void __iomem *__mic_get_remote_dp(struct vop_device *vpdev)
{
return NULL;
@@ -120,6 +127,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 cc928d45af5a..b5612183dcb8 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -1009,7 +1009,13 @@ 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");
+ return -EINVAL;
+ }
+
*size = MIC_DP_SIZE;
return 0;
}
--
2.17.1
[PATCH V3 4/4] misc: vop: mapping kernel memory to user space as noncached
Mapping kernel space memory to user space as noncached, since user space need check the updates of avail_idx and device page flags timely. Signed-off-by: Joakim Zhang Signed-off-by: Sherry Sun --- drivers/misc/mic/vop/vop_vringh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c index b5612183dcb8..b75c2b713a3b 100644 --- a/drivers/misc/mic/vop/vop_vringh.c +++ b/drivers/misc/mic/vop/vop_vringh.c @@ -1058,7 +1058,7 @@ 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); + pgprot_noncached(vma->vm_page_prot)); if (err) goto ret; size_rem -= size; -- 2.17.1
[PATCH V3 0/4] Change vring space from nomal memory to dma coherent memory
Changes in V3: 1. Change the device page allocation method of the Intel mic layer in Patch 1 to align with the vring allocation. 2. Move the vring physical address changes in mmap callback from Prach 3 to 1. 3. Use must_be_zero instead of directly deleting used_address_updated in Patch 2 to avoid the influence of ABI change. (I tried to use dma_mmap_coherent api in Patch 4 as Christoph suggested, but met some issues explained here https://lore.kernel.org/patchwork/patch/1313327/, so there is no change to Patch 4, and still can't find a better way than patch 3) 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 (4): misc: vop: change the way of allocating vring and device page misc: vop: do not allocate and reassign the used ring misc: vop: simply return the saved dma address instead of virt_to_phys 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/host/mic_main.c | 15 ++ drivers/misc/mic/vop/vop_debugfs.c | 2 - drivers/misc/mic/vop/vop_main.c| 48 +++-- drivers/misc/mic/vop/vop_vringh.c | 84 +++--- include/uapi/linux/mic_common.h| 5 +- 7 files changed, 42 insertions(+), 122 deletions(-) -- 2.17.1
[PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
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
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/vop/vop_debugfs.c | 2 --
drivers/misc/mic/vop/vop_main.c| 48 --
drivers/misc/mic/vop/vop_vringh.c | 31 ---
include/uapi/linux/mic_common.h| 5 ++--
4 files changed, 8 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..1ccc94dfd6ac 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(>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,8 @@ 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 = 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 +351,15 @@ 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(>dev, used,
- vdev->used_size[index],
- DMA_BIDIRECTIONAL);
- if (dma_mapping_error(>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] = 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], >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 +372,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(>desc->num_vq))
@@ -412,24 +394,6 @@ static int vop_find_vqs(struct virtio_device *dev,
unsig
[PATCH V3 1/4] 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.
Signed-off-by: Joakim Zhang
Signed-off-by: Sherry Sun
---
drivers/misc/mic/host/mic_main.c | 15 +++
drivers/misc/mic/vop/vop_vringh.c | 43 +--
2 files changed, 16 insertions(+), 42 deletions(-)
diff --git a/drivers/misc/mic/host/mic_main.c b/drivers/misc/mic/host/mic_main.c
index ea4608527ea0..ebacaa35cb47 100644
--- a/drivers/misc/mic/host/mic_main.c
+++ b/drivers/misc/mic/host/mic_main.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#include
#include "../common/mic_dev.h"
@@ -48,18 +49,11 @@ 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);
+ mdev->dp = dma_alloc_coherent(>pdev->dev, MIC_DP_SIZE,
+ >dp_dma_addr, 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(>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;
@@ -68,8 +62,7 @@ static int mic_dp_init(struct mic_device *mdev)
/* 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);
+ dma_free_coherent(>pdev->dev, MIC_DP_SIZE, mdev->dp,
mdev->dp_dma_addr);
}
/**
diff --git a/drivers/misc/mic/vop/vop_vringh.c
b/drivers/misc/mic/vop/vop_vringh.c
index 7014ffe88632..2cc3c22482b5 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(>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, _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(>dev, vr->va, vr_size,
-DMA_BIDIRECTIONAL);
- if (dma_mapping_error(>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, num, vr->va, MIC_VIRTIO_RING_ALIGN);
@@ -339,11 +329,8 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
dev_dbg(>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(>dev,
- vvr->buf, VOP_INT_DMA_BUF_SIZE,
- DMA_BIDIRECTIONAL);
+ vvr->buf = dma_alloc_coherent(vop_dev(vdev),
VOP_INT_DMA_BUF_SIZE,
+ >buf_da, GFP_KERNEL);
}
snprintf(irqname, siz
RE: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Sudeep & Ashutosh & Vincent, Can you help test the patch about removing the codes of reassign used ring, and comment on the impact for Intel MIC platform? Thanks for any help. Best regards Sherry > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > On Mon, Oct 26, 2020 at 03:04:45AM +, Sherry Sun wrote: > > Hi Greg & Christoph, > > > > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign > > > the used ring > > > > > > On Thu, Oct 22, 2020 at 01:06:36PM +0800, Sherry Sun wrote: > > > > 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. > > > > > > Someone needs to verify this vs the existing intel implementations. > > > > Hi Greg, @gre...@linuxfoundation.org, sorry I don't have the Intel MIC > devices so cannot test it, do you know anyone who can help test this patch > changes on the Intel MIC platform? Thanks. > > Why not ask the authors/maintainers of that code? > > greg k-h
RE: [PATCH V4 1/2] misc: vop: change the way of allocating vring and device page
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
> 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
> Signed-off-by: Sherry Sun
> ---
> 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(>dev);
> +
> + return dma_mmap_coherent(>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
> #include
> #include
> +#include
>
> #include
> #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
RE: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-Transparent Bridge
Hi Kishon,
> Subject: Re: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-
> Transparent Bridge
>
> Hi Sherry,
>
> On 09/11/20 3:07 pm, Sherry Sun wrote:
> > Hi Kishon,
> >
> >> Subject: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-
> >> Transparent Bridge
> >>
> >> From: Kishon Vijay Abraham I
> >>
> >> Add support for EPF PCI-Express Non-Transparent Bridge (NTB) device.
> >> This driver is platform independent and could be used by any platform
> >> which have multiple PCIe endpoint instances configured using the pci-epf-
> ntb driver.
> >> The driver connnects to the standard NTB sub-system interface. The
> >> EPF NTB device has configurable number of memory windows (Max 4),
> >> configurable number of doorbell (Max 32), and configurable number of
> >> scratch-pad registers.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I
> >> ---
> >> drivers/ntb/hw/Kconfig | 1 +
> >> drivers/ntb/hw/Makefile | 1 +
> >> drivers/ntb/hw/epf/Kconfig | 6 +
> >> drivers/ntb/hw/epf/Makefile | 1 +
> >> drivers/ntb/hw/epf/ntb_hw_epf.c | 755
> >>
> >> 5 files changed, 764 insertions(+)
> >> create mode 100644 drivers/ntb/hw/epf/Kconfig create mode 100644
> >> drivers/ntb/hw/epf/Makefile create mode 100644
> >> drivers/ntb/hw/epf/ntb_hw_epf.c
> >>
> >> diff --git a/drivers/ntb/hw/Kconfig b/drivers/ntb/hw/Kconfig index
> >> e77c587060ff..c325be526b80 100644
> >> --- a/drivers/ntb/hw/Kconfig
> >> +++ b/drivers/ntb/hw/Kconfig
> >> @@ -2,4 +2,5 @@
> >> source "drivers/ntb/hw/amd/Kconfig"
> >> source "drivers/ntb/hw/idt/Kconfig"
> >> source "drivers/ntb/hw/intel/Kconfig"
> >> +source "drivers/ntb/hw/epf/Kconfig"
> >> source "drivers/ntb/hw/mscc/Kconfig"
> >> diff --git a/drivers/ntb/hw/Makefile b/drivers/ntb/hw/Makefile index
> >> 4714d6238845..223ca592b5f9 100644
> >> --- a/drivers/ntb/hw/Makefile
> >> +++ b/drivers/ntb/hw/Makefile
> >> @@ -2,4 +2,5 @@
> >> obj-$(CONFIG_NTB_AMD) += amd/
> >> obj-$(CONFIG_NTB_IDT) += idt/
> >> obj-$(CONFIG_NTB_INTEL) += intel/
> >> +obj-$(CONFIG_NTB_EPF) += epf/
> >> obj-$(CONFIG_NTB_SWITCHTEC) += mscc/ diff --git
> >> a/drivers/ntb/hw/epf/Kconfig b/drivers/ntb/hw/epf/Kconfig new file
> >> mode 100644 index ..6197d1aab344
> >> --- /dev/null
> >> +++ b/drivers/ntb/hw/epf/Kconfig
> >> @@ -0,0 +1,6 @@
> >> +config NTB_EPF
> >> + tristate "Generic EPF Non-Transparent Bridge support"
> >> + depends on m
> >> + help
> >> +This driver supports EPF NTB on configurable endpoint.
> >> +If unsure, say N.
> >> diff --git a/drivers/ntb/hw/epf/Makefile
> >> b/drivers/ntb/hw/epf/Makefile new file mode 100644 index
> >> ..2f560a422bc6
> >> --- /dev/null
> >> +++ b/drivers/ntb/hw/epf/Makefile
> >> @@ -0,0 +1 @@
> >> +obj-$(CONFIG_NTB_EPF) += ntb_hw_epf.o
> >> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c
> >> b/drivers/ntb/hw/epf/ntb_hw_epf.c new file mode 100644 index
> >> ..0a144987851a
> >> --- /dev/null
> >> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
> >> @@ -0,0 +1,755 @@
> > ..
> >> +static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
> >> + struct pci_dev *pdev)
> >> +{
> >> + struct device *dev = ndev->dev;
> >> + int ret;
> >> +
> >> + pci_set_drvdata(pdev, ndev);
> >> +
> >> + ret = pci_enable_device(pdev);
> >> + if (ret) {
> >> + dev_err(dev, "Cannot enable PCI device\n");
> >> + goto err_pci_enable;
> >> + }
> >> +
> >> + ret = pci_request_regions(pdev, "ntb");
> >> + if (ret) {
> >> + dev_err(dev, "Cannot obtain PCI resources\n");
> >> + goto err_pci_regions;
> >> + }
> >> +
> >> + pci_set_master(pdev);
> >> +
> >> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> >> + if (ret) {
> >> + ret = dma_set_mask_and_coherent(dev,
> >> DMA_BIT_MASK(32));
> >> + if (ret) {
> >> + dev_err(dev, "Cannot set DMA mask\n");
&g
RE: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-Transparent Bridge
Hi Kishon, > Subject: Re: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non- > Transparent Bridge > > Hi Sherry, Arnd, > > On 10/11/20 8:29 pm, Arnd Bergmann wrote: > > On Tue, Nov 10, 2020 at 3:20 PM Kishon Vijay Abraham I > wrote: > >> On 10/11/20 7:55 am, Sherry Sun wrote: > > > >>> But for VOP, only two boards are needed(one board as host and one > >>> board as card) to realize the communication between the two systems, > so my question is what are the advantages of using NTB? > >> > >> NTB is a bridge that facilitates communication between two different > >> systems. So it by itself will not be source or sink of any data > >> unlike a normal EP to RP system (or the VOP) which will be source or sink > of data. > >> > >>> Because I think the architecture of NTB seems more complicated. Many > thanks! > >> > >> yeah, I think it enables a different use case all together. Consider > >> you have two x86 HOST PCs (having RP) and they have to be communicate > >> using PCIe. NTB can be used in such cases for the two x86 PCs to > >> communicate with each other over PCIe, which wouldn't be possible > without NTB. > > > > I think for VOP, we should have an abstraction that can work on either > > NTB or directly on the endpoint framework but provide an interface > > that then lets you create logical devices the same way. > > > > Doing VOP based on NTB plus the new NTB_EPF driver would also work and > > just move the abstraction somewhere else, but I guess it would > > complicate setting it up for those users that only care about the > > simpler endpoint case. > > I'm not sure if you've got a chance to look at [1], where I added support for > RP<->EP system both running Linux, with EP configured using Linux EP > framework (as well as HOST ports connected to NTB switch, patches 20 and > 21, that uses the Linux NTB framework) to communicate using virtio over > PCIe. > I saw your patches at [1], here you take a rpmsg as an example to communicate between two SoCs using PCIe RC<->EP and HOST1-NTB-HOST2 for different usercases. The VOP code works under the PCIe RC<->EP framework, which means that we can also make VOP works under the Linux NTB framework, just like the rpmsg way you did here, right? Best regards Sherry > The cover-letter [1] shows a picture of the two use cases supported in that > series. > > [1] -> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flore.ke > rnel.org%2Fr%2F20200702082143.25259-1- > kishon%40ti.comdata=04%7C01%7Csherry.sun%40nxp.com%7C5d8b7 > 3a4b72947bea65d08d8858f5091%7C686ea1d3bc2b4c6fa92cd99c5c301635%7 > C0%7C0%7C637406197865119992%7CUnknown%7CTWFpbGZsb3d8eyJWIjoi > MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C100 > 0sdata=iRrBvQ9xjoOUYU%2FDidMLZZpW6XuU4ITVXFDA%2B%2F4rJFU > %3Dreserved=0 > > Thank You, > Kishon
RE: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-Transparent Bridge
Hi Kishon,
> Subject: [PATCH v7 15/18] NTB: Add support for EPF PCI-Express Non-
> Transparent Bridge
>
> From: Kishon Vijay Abraham I
>
> Add support for EPF PCI-Express Non-Transparent Bridge (NTB) device.
> This driver is platform independent and could be used by any platform which
> have multiple PCIe endpoint instances configured using the pci-epf-ntb driver.
> The driver connnects to the standard NTB sub-system interface. The EPF NTB
> device has configurable number of memory windows (Max 4), configurable
> number of doorbell (Max 32), and configurable number of scratch-pad
> registers.
>
> Signed-off-by: Kishon Vijay Abraham I
> ---
> drivers/ntb/hw/Kconfig | 1 +
> drivers/ntb/hw/Makefile | 1 +
> drivers/ntb/hw/epf/Kconfig | 6 +
> drivers/ntb/hw/epf/Makefile | 1 +
> drivers/ntb/hw/epf/ntb_hw_epf.c | 755
>
> 5 files changed, 764 insertions(+)
> create mode 100644 drivers/ntb/hw/epf/Kconfig create mode 100644
> drivers/ntb/hw/epf/Makefile create mode 100644
> drivers/ntb/hw/epf/ntb_hw_epf.c
>
> diff --git a/drivers/ntb/hw/Kconfig b/drivers/ntb/hw/Kconfig index
> e77c587060ff..c325be526b80 100644
> --- a/drivers/ntb/hw/Kconfig
> +++ b/drivers/ntb/hw/Kconfig
> @@ -2,4 +2,5 @@
> source "drivers/ntb/hw/amd/Kconfig"
> source "drivers/ntb/hw/idt/Kconfig"
> source "drivers/ntb/hw/intel/Kconfig"
> +source "drivers/ntb/hw/epf/Kconfig"
> source "drivers/ntb/hw/mscc/Kconfig"
> diff --git a/drivers/ntb/hw/Makefile b/drivers/ntb/hw/Makefile index
> 4714d6238845..223ca592b5f9 100644
> --- a/drivers/ntb/hw/Makefile
> +++ b/drivers/ntb/hw/Makefile
> @@ -2,4 +2,5 @@
> obj-$(CONFIG_NTB_AMD)+= amd/
> obj-$(CONFIG_NTB_IDT)+= idt/
> obj-$(CONFIG_NTB_INTEL) += intel/
> +obj-$(CONFIG_NTB_EPF)+= epf/
> obj-$(CONFIG_NTB_SWITCHTEC) += mscc/
> diff --git a/drivers/ntb/hw/epf/Kconfig b/drivers/ntb/hw/epf/Kconfig new
> file mode 100644 index ..6197d1aab344
> --- /dev/null
> +++ b/drivers/ntb/hw/epf/Kconfig
> @@ -0,0 +1,6 @@
> +config NTB_EPF
> + tristate "Generic EPF Non-Transparent Bridge support"
> + depends on m
> + help
> + This driver supports EPF NTB on configurable endpoint.
> + If unsure, say N.
> diff --git a/drivers/ntb/hw/epf/Makefile b/drivers/ntb/hw/epf/Makefile new
> file mode 100644 index ..2f560a422bc6
> --- /dev/null
> +++ b/drivers/ntb/hw/epf/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_NTB_EPF) += ntb_hw_epf.o
> diff --git a/drivers/ntb/hw/epf/ntb_hw_epf.c
> b/drivers/ntb/hw/epf/ntb_hw_epf.c new file mode 100644 index
> ..0a144987851a
> --- /dev/null
> +++ b/drivers/ntb/hw/epf/ntb_hw_epf.c
> @@ -0,0 +1,755 @@
..
> +static int ntb_epf_init_pci(struct ntb_epf_dev *ndev,
> + struct pci_dev *pdev)
> +{
> + struct device *dev = ndev->dev;
> + int ret;
> +
> + pci_set_drvdata(pdev, ndev);
> +
> + ret = pci_enable_device(pdev);
> + if (ret) {
> + dev_err(dev, "Cannot enable PCI device\n");
> + goto err_pci_enable;
> + }
> +
> + ret = pci_request_regions(pdev, "ntb");
> + if (ret) {
> + dev_err(dev, "Cannot obtain PCI resources\n");
> + goto err_pci_regions;
> + }
> +
> + pci_set_master(pdev);
> +
> + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
> + if (ret) {
> + ret = dma_set_mask_and_coherent(dev,
> DMA_BIT_MASK(32));
> + if (ret) {
> + dev_err(dev, "Cannot set DMA mask\n");
> + goto err_dma_mask;
> + }
> + dev_warn(>dev, "Cannot DMA highmem\n");
> + }
> +
> + ndev->ctrl_reg = pci_iomap(pdev, 0, 0);
The second parameter of pci_iomap should be ndev->ctrl_reg_bar instead of the
hardcode value 0, right?
> + if (!ndev->ctrl_reg) {
> + ret = -EIO;
> + goto err_dma_mask;
> + }
> +
> + ndev->peer_spad_reg = pci_iomap(pdev, 1, 0);
pci_iomap(pdev, ndev->peer_spad_reg_bar, 0);
> + if (!ndev->peer_spad_reg) {
> + ret = -EIO;
> + goto err_dma_mask;
> + }
> +
> + ndev->db_reg = pci_iomap(pdev, 2, 0);
pci_iomap(pdev, ndev->db_reg_bar, 0);
Best Regards
Sherry
[PATCH 0/4] Fix some bugs of the vop driver and mpssd user space tool
This patchset fix some bugs about the vop driver and Intel MIC user space tool -- mpssd. Sherry Sun (4): samples: mpssd: fix the build errors when enable DEBUG in mpssd.c misc: vop: build VOP based on CONFIG_VOP misc: vop: add round_up(x,4) for vring_size to avoid kernel panic mic: vop: copy data to kernel space then write to io memory drivers/misc/mic/vop/Makefile | 2 +- drivers/misc/mic/vop/vop_main.c | 2 +- drivers/misc/mic/vop/vop_vringh.c | 10 +++--- samples/mic/mpssd/mpssd.c | 24 4 files changed, 21 insertions(+), 17 deletions(-) -- 2.17.1
[PATCH 1/4] samples: mpssd: fix the build errors when enable DEBUG in mpssd.c
If enable DEBUG, will meet the following errors when build mpssd, so fix
them here. Only one error is listed here, other errors are similar.
mpssd.c: In function ‘virtio_net’:
mpssd.c:615:21: error: incompatible type for argument 2 of ‘disp_iovec’
disp_iovec(mic, copy, __func__, __LINE__);
^~~~
mpssd.c:361:1: note: expected ‘struct mic_copy_desc *’ but argument is of type
‘struct mic_copy_desc’
disp_iovec(struct mic_info *mic, struct mic_copy_desc *copy,
^~
Signed-off-by: Sherry Sun
---
samples/mic/mpssd/mpssd.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/samples/mic/mpssd/mpssd.c b/samples/mic/mpssd/mpssd.c
index a11bf6c5b53b..51d03545869e 100644
--- a/samples/mic/mpssd/mpssd.c
+++ b/samples/mic/mpssd/mpssd.c
@@ -612,7 +612,7 @@ virtio_net(void *arg)
copy.out_len, hdr->gso_type);
#endif
#ifdef DEBUG
- disp_iovec(mic, copy, __func__, __LINE__);
+ disp_iovec(mic, , __func__, __LINE__);
mpsslog("%s %s %d read from tap 0x%lx\n",
mic->name, __func__, __LINE__,
len);
@@ -632,7 +632,7 @@ virtio_net(void *arg)
if (!err)
verify_out_len(mic, );
#ifdef DEBUG
- disp_iovec(mic, copy, __func__, __LINE__);
+ disp_iovec(mic, , __func__, __LINE__);
mpsslog("%s %s %d wrote to net 0x%lx\n",
mic->name, __func__, __LINE__,
sum_iovec_len());
@@ -681,12 +681,12 @@ virtio_net(void *arg)
sizeof(struct virtio_net_hdr);
verify_out_len(mic, );
#ifdef DEBUG
- disp_iovec(mic, copy, __func__,
+ disp_iovec(mic, , __func__,
__LINE__);
mpsslog("%s %s %d ",
mic->name, __func__, __LINE__);
mpsslog("read from net 0x%lx\n",
- sum_iovec_len(copy));
+ sum_iovec_len());
#endif
len = writev(net_poll[NET_FD_TUN].fd,
copy.iov, copy.iovcnt);
@@ -814,7 +814,7 @@ virtio_console(void *arg)
len = readv(pty_fd, copy.iov, copy.iovcnt);
if (len > 0) {
#ifdef DEBUG
- disp_iovec(mic, copy, __func__, __LINE__);
+ disp_iovec(mic, , __func__, __LINE__);
mpsslog("%s %s %d read from tap 0x%lx\n",
mic->name, __func__, __LINE__,
len);
@@ -834,10 +834,10 @@ virtio_console(void *arg)
if (!err)
verify_out_len(mic, );
#ifdef DEBUG
- disp_iovec(mic, copy, __func__, __LINE__);
+ disp_iovec(mic, , __func__, __LINE__);
mpsslog("%s %s %d wrote to net 0x%lx\n",
mic->name, __func__, __LINE__,
- sum_iovec_len(copy));
+ sum_iovec_len());
#endif
/* Reinitialize IOV for next run */
iov0->iov_len = PAGE_SIZE;
@@ -866,12 +866,12 @@ virtio_console(void *arg)
iov1->iov_len = copy.out_len;
verify_out_len(mic, );
#ifdef DEBUG
- disp_iovec(mic, copy, __func__,
+ disp_iovec(mic, , __func__,
__LINE__);
mpsslog("%s %s %d ",
mic->name, __func__, __LINE__);
mpsslog("read from net 0x%lx\n",
- sum_iovec_len(copy));
+ sum_iovec_len());
#endif
len = writev(pty_fd,
copy.iov, copy.iovcnt);
@@ -883,7 +883,7 @@ virtio_console(void *arg)
[PATCH 3/4] misc: vop: add round_up(x,4) for vring_size to avoid kernel panic
Since struct _mic_vring_info and vring are allocated together and follow
vring, if the vring_size() is not four bytes aligned, which will cause
the start address of struct _mic_vring_info is not four byte aligned.
For example, when vring entries is 128, the vring_size() will be 5126
bytes. The _mic_vring_info struct layout in ddr looks like:
0x90002400: 0039 EE01 C0FF
Here 0x39 is the avail_idx member, and 0xC0FFEE01 is the magic member.
When EP use ioread32(magic) to reads the magic in RC's share memory, it
will cause kernel panic on ARM64 platform due to the cross-byte io read.
Here read magic in user space use le32toh(vr0->info->magic) will meet
the same issue.
So add round_up(x,4) for vring_size, then the struct _mic_vring_info
will store in this way:
0x90002400: 0039 C0FFEE01
Which will avoid kernel panic when read magic in struct _mic_vring_info.
Signed-off-by: Sherry Sun
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/vop/vop_main.c | 2 +-
drivers/misc/mic/vop/vop_vringh.c | 4 ++--
samples/mic/mpssd/mpssd.c | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index 55e7f21e51f4..6722c726b259 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -320,7 +320,7 @@ static struct virtqueue *vop_find_vq(struct virtio_device
*dev,
/* First assign the vring's allocated in host memory */
vqconfig = _vop_vq_config(vdev->desc) + index;
memcpy_fromio(, vqconfig, sizeof(config));
- _vr_size = vring_size(le16_to_cpu(config.num), MIC_VIRTIO_RING_ALIGN);
+ _vr_size = round_up(vring_size(le16_to_cpu(config.num),
MIC_VIRTIO_RING_ALIGN), 4);
vr_size = PAGE_ALIGN(_vr_size + sizeof(struct _mic_vring_info));
va = vpdev->hw_ops->remap(vpdev, le64_to_cpu(config.address), vr_size);
if (!va)
diff --git a/drivers/misc/mic/vop/vop_vringh.c
b/drivers/misc/mic/vop/vop_vringh.c
index 30eac172f017..45fdb394de11 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -296,7 +296,7 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
num = le16_to_cpu(vqconfig[i].num);
mutex_init(>vr_mutex);
- vr_size = PAGE_ALIGN(vring_size(num, MIC_VIRTIO_RING_ALIGN) +
+ 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,
@@ -308,7 +308,7 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
goto err;
}
vr->len = vr_size;
- vr->info = vr->va + vring_size(num, MIC_VIRTIO_RING_ALIGN);
+ 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(>dev, vr->va, vr_size,
DMA_BIDIRECTIONAL);
diff --git a/samples/mic/mpssd/mpssd.c b/samples/mic/mpssd/mpssd.c
index 51d03545869e..c03a05d498f0 100644
--- a/samples/mic/mpssd/mpssd.c
+++ b/samples/mic/mpssd/mpssd.c
@@ -403,9 +403,9 @@ mic_virtio_copy(struct mic_info *mic, int fd,
static inline unsigned _vring_size(unsigned int num, unsigned long align)
{
- return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (3 + num)
+ return _ALIGN_UP(((sizeof(struct vring_desc) * num + sizeof(__u16) * (3
+ num)
+ align - 1) & ~(align - 1))
- + sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num;
+ + sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num, 4);
}
/*
--
2.17.1
[PATCH 4/4] mic: vop: copy data to kernel space then write to io memory
Read and write io memory should address align on ARCH ARM. Change to use
memcpy_toio to avoid kernel panic caused by the address un-align issue.
Signed-off-by: Sherry Sun
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/vop/vop_vringh.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/mic/vop/vop_vringh.c
b/drivers/misc/mic/vop/vop_vringh.c
index 45fdb394de11..f344209ac386 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -602,6 +602,7 @@ static int vop_virtio_copy_from_user(struct vop_vdev *vdev,
void __user *ubuf,
size_t partlen;
bool dma = VOP_USE_DMA && vi->dma_ch;
int err = 0;
+ void *temp = NULL;
if (dma) {
dma_alignment = 1 << vi->dma_ch->device->copy_align;
@@ -655,12 +656,15 @@ static int vop_virtio_copy_from_user(struct vop_vdev
*vdev, void __user *ubuf,
* We are copying to IO below and should ideally use something
* like copy_from_user_toio(..) if it existed.
*/
- if (copy_from_user((void __force *)dbuf, ubuf, len)) {
+ temp = kmalloc(len, GFP_KERNEL);
+ if (copy_from_user(temp, ubuf, len)) {
err = -EFAULT;
dev_err(vop_dev(vdev), "%s %d err %d\n",
__func__, __LINE__, err);
goto err;
}
+ memcpy_toio((void __force *)dbuf, temp, len);
+ kfree(temp);
vdev->out_bytes += len;
err = 0;
err:
--
2.17.1
[PATCH 2/4] misc: vop: build VOP based on CONFIG_VOP
Build module or builtin VOP based on CONFIG_VOP macro. Signed-off-by: Joakim Zhang Signed-off-by: Sherry Sun --- drivers/misc/mic/vop/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/misc/mic/vop/Makefile b/drivers/misc/mic/vop/Makefile index 579da3868c8e..51b9b0022786 100644 --- a/drivers/misc/mic/vop/Makefile +++ b/drivers/misc/mic/vop/Makefile @@ -3,7 +3,7 @@ # Makefile - Intel MIC Linux driver. # Copyright(c) 2016, Intel Corporation. # -obj-m := vop.o +obj-$(CONFIG_VOP) := vop.o vop-objs += vop_main.o vop-objs += vop_debugfs.o -- 2.17.1
[PATCH 0/5] Add noncoherent platform support for vop driver
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 5/5] misc: vop: mapping kernel memory to user space as noncached
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 Signed-off-by: Sherry Sun --- 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
[PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform
Set VIRTIO_F_ACCESS_PLATFORM feature for nocoherent platform, since it needs the DMA API for virtio. Signed-off-by: Sherry Sun Signed-off-by: Joakim Zhang --- 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 1/5] misc: vop: change the way of allocating vring for noncoherent platform
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
Signed-off-by: Sherry Sun
---
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
#include
#include
+#include
#include
#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(>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,
+ _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(>dev, vr->va, vr_size,
-DMA_BIDIRECTIONAL);
- if (dma_mapping_error(>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(>dev, vr->va, vr_size,
+DMA_BIDIRECTIONAL);
+ if (dma_mapping_error(>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(>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(>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,
+ >buf_da, GFP_KERNEL);
+ else {
+ vvr->bu
[PATCH 2/5] misc: vop: change the way of allocating used ring
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
Signed-off-by: Joakim Zhang
---
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
#include
#include
+#include
#include
#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(>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(>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(>dev, used,
- vdev->used_size[index],
- DMA_BIDIRECTIONAL);
- if (dma_mapping_error(>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(>dev, used,
+ vdev->used_size[index],
+ DMA_BIDIRECTIONAL);
+ if (dma_mapping_error(>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], >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
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
Signed-off-by: Joakim Zhang
---
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(>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 = >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 0/3] Add module autoprobing support for vop and cosm driver
Add module autoprobing support for vop and cosm driver, when the vop/cosm device appears, the driver will be autoloaded. Sherry Sun (3): mic: vop: fix a written error in MODULE_DEVICE_TABLE mic: vop: module autoprobing support for vop drivers mic: cosm: module autoprobing support for cosm driver drivers/misc/mic/bus/cosm_bus.c | 8 drivers/misc/mic/bus/vop_bus.h| 7 +-- drivers/misc/mic/cosm/cosm_main.c | 7 +++ drivers/misc/mic/vop/vop_main.c | 2 +- include/linux/mod_devicetable.h | 15 +++ scripts/mod/devicetable-offsets.c | 7 +++ scripts/mod/file2alias.c | 27 +++ 7 files changed, 66 insertions(+), 7 deletions(-) -- 2.17.1
[PATCH 3/3] mic: cosm: module autoprobing support for cosm driver
Add uevent callback for cosm_bus and add cosm_device_id for cosm driver
which is needed for MODULE_DEVICE_TABLE. Also adding struct
cosm_device_id in devicetable-offsets.c and the cosm entry point in
file2alias.c.
Cosm driver will be autoloaded when cosm device appears.
Signed-off-by: Sherry Sun
---
drivers/misc/mic/bus/cosm_bus.c | 8
drivers/misc/mic/cosm/cosm_main.c | 7 +++
include/linux/mod_devicetable.h | 8
scripts/mod/devicetable-offsets.c | 3 +++
scripts/mod/file2alias.c | 11 +++
5 files changed, 37 insertions(+)
diff --git a/drivers/misc/mic/bus/cosm_bus.c b/drivers/misc/mic/bus/cosm_bus.c
index 5f2141c71738..736e27bbc9f9 100644
--- a/drivers/misc/mic/bus/cosm_bus.c
+++ b/drivers/misc/mic/bus/cosm_bus.c
@@ -14,6 +14,13 @@
/* Unique numbering for cosm devices. */
static DEFINE_IDA(cosm_index_ida);
+static int cosm_uevent(struct device *d, struct kobj_uevent_env *env)
+{
+ struct cosm_device *dev = dev_to_cosm(d);
+
+ return add_uevent_var(env, "MODALIAS=cosm:cosm-dev%u", dev->index);
+}
+
static int cosm_dev_probe(struct device *d)
{
struct cosm_device *dev = dev_to_cosm(d);
@@ -33,6 +40,7 @@ static int cosm_dev_remove(struct device *d)
static struct bus_type cosm_bus = {
.name = "cosm_bus",
+ .uevent = cosm_uevent,
.probe = cosm_dev_probe,
.remove = cosm_dev_remove,
};
diff --git a/drivers/misc/mic/cosm/cosm_main.c
b/drivers/misc/mic/cosm/cosm_main.c
index ebb0eac43754..627e7d5f3a83 100644
--- a/drivers/misc/mic/cosm/cosm_main.c
+++ b/drivers/misc/mic/cosm/cosm_main.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include "cosm_main.h"
static const char cosm_driver_name[] = "mic";
@@ -323,6 +324,12 @@ static int cosm_suspend(struct device *dev)
return 0;
}
+static struct cosm_device_id __maybe_unused cosm_driver_id_table[] = {
+ { .name = "cosm-dev*" },
+ { },
+};
+MODULE_DEVICE_TABLE(cosm, cosm_driver_id_table);
+
static const struct dev_pm_ops cosm_pm_ops = {
.suspend = cosm_suspend,
.freeze = cosm_suspend
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 736cdc236cf9..ea6cdfe1a3a3 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -845,4 +845,12 @@ struct vop_device_id {
};
#define VOP_DEV_ANY_ID 0x
+/* cosm */
+#define COSM_NAME_SIZE 32
+#define COSM_MODULE_PREFIX "cosm:"
+
+struct cosm_device_id {
+ char name[COSM_NAME_SIZE];
+};
+
#endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/scripts/mod/devicetable-offsets.c
b/scripts/mod/devicetable-offsets.c
index 393acaa5302a..499a2832878d 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -247,5 +247,8 @@ int main(void)
DEVID_FIELD(vop_device_id, device);
DEVID_FIELD(vop_device_id, vendor);
+ DEVID(cosm_device_id);
+ DEVID_FIELD(cosm_device_id, name);
+
return 0;
}
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 8063b778eedf..f7c80e4da137 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1383,6 +1383,16 @@ static int do_vop_entry(const char *filename, void
*symval,
return 1;
}
+/* Looks like: cosm:S */
+static int do_cosm_entry(const char *filename, void *symval,
+ char *alias)
+{
+ DEF_FIELD_ADDR(symval, cosm_device_id, name);
+ sprintf(alias, COSM_MODULE_PREFIX "%s", *name);
+
+ return 1;
+}
+
/* Does namelen bytes of name exactly match the symbol? */
static bool sym_is(const char *name, unsigned namelen, const char *symbol)
{
@@ -1458,6 +1468,7 @@ static const struct devtable devtable[] = {
{"wmi", SIZE_wmi_device_id, do_wmi_entry},
{"mhi", SIZE_mhi_device_id, do_mhi_entry},
{"vop", SIZE_vop_device_id, do_vop_entry},
+ {"cosm", SIZE_cosm_device_id, do_cosm_entry},
};
/* Create MODULE_ALIAS() statements.
--
2.17.1
[PATCH 1/3] mic: vop: fix a written error in MODULE_DEVICE_TABLE
For vop bus, the first parameter should be vop in MODULE_DEVICE_TABLE.
Signed-off-by: Sherry Sun
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/vop/vop_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index d609f0dc6124..589425fa78d4 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -796,7 +796,7 @@ static struct vop_driver vop_driver = {
module_vop_driver(vop_driver);
-MODULE_DEVICE_TABLE(mbus, id_table);
+MODULE_DEVICE_TABLE(vop, id_table);
MODULE_AUTHOR("Intel Corporation");
MODULE_DESCRIPTION("Intel(R) Virtio Over PCIe (VOP) driver");
MODULE_LICENSE("GPL v2");
--
2.17.1
[PATCH 2/3] mic: vop: module autoprobing support for vop drivers
Add vop autoprobing support to MODULE_DEVICE_TABLE() by adding info
about struct vop_device_id in devicetable-offsets.c and add a vop entry
point in file2alias.c.
The type argument for MODULE_DEVICE_TABLE(type, name) is vop.
Signed-off-by: Sherry Sun
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/bus/vop_bus.h| 7 +--
include/linux/mod_devicetable.h | 7 +++
scripts/mod/devicetable-offsets.c | 4
scripts/mod/file2alias.c | 16
4 files changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/misc/mic/bus/vop_bus.h b/drivers/misc/mic/bus/vop_bus.h
index d891eacae358..5b4a58757951 100644
--- a/drivers/misc/mic/bus/vop_bus.h
+++ b/drivers/misc/mic/bus/vop_bus.h
@@ -14,16 +14,11 @@
*/
#include
#include
+#include
#include "../common/mic_dev.h"
-struct vop_device_id {
- u32 device;
- u32 vendor;
-};
-
#define VOP_DEV_TRNSP 1
-#define VOP_DEV_ANY_ID 0x
/*
* Size of the internal buffer used during DMA's as an intermediate buffer
* for copy to/from user. Must be an integral number of pages.
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5b08a473cdba..736cdc236cf9 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -838,4 +838,11 @@ struct mhi_device_id {
kernel_ulong_t driver_data;
};
+/* vop */
+struct vop_device_id {
+ __u32 device;
+ __u32 vendor;
+};
+#define VOP_DEV_ANY_ID 0x
+
#endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/scripts/mod/devicetable-offsets.c
b/scripts/mod/devicetable-offsets.c
index 27007c18e754..393acaa5302a 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -243,5 +243,9 @@ int main(void)
DEVID(mhi_device_id);
DEVID_FIELD(mhi_device_id, chan);
+ DEVID(vop_device_id);
+ DEVID_FIELD(vop_device_id, device);
+ DEVID_FIELD(vop_device_id, vendor);
+
return 0;
}
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2417dd1dee33..8063b778eedf 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1368,6 +1368,21 @@ static int do_mhi_entry(const char *filename, void
*symval, char *alias)
return 1;
}
+/* Looks like: vop:dNvN */
+static int do_vop_entry(const char *filename, void *symval,
+ char *alias)
+{
+ DEF_FIELD(symval, vop_device_id, device);
+ DEF_FIELD(symval, vop_device_id, vendor);
+
+ strcpy(alias, "vop:");
+ ADD(alias, "d", device != VOP_DEV_ANY_ID, device);
+ ADD(alias, "v", vendor != VOP_DEV_ANY_ID, vendor);
+
+ add_wildcard(alias);
+ return 1;
+}
+
/* Does namelen bytes of name exactly match the symbol? */
static bool sym_is(const char *name, unsigned namelen, const char *symbol)
{
@@ -1442,6 +1457,7 @@ static const struct devtable devtable[] = {
{"tee", SIZE_tee_client_device_id, do_tee_entry},
{"wmi", SIZE_wmi_device_id, do_wmi_entry},
{"mhi", SIZE_mhi_device_id, do_mhi_entry},
+ {"vop", SIZE_vop_device_id, do_vop_entry},
};
/* Create MODULE_ALIAS() statements.
--
2.17.1
RE: [PATCH 4/4] mic: vop: copy data to kernel space then write to io memory
Hi Arnd,
> Subject: Re: [PATCH 4/4] mic: vop: copy data to kernel space then write to io
> memory
>
> On Fri, Sep 25, 2020 at 9:22 AM Sherry Sun wrote:
> >
>
> > @@ -655,12 +656,15 @@ static int vop_virtio_copy_from_user(struct
> vop_vdev *vdev, void __user *ubuf,
> > * We are copying to IO below and should ideally use something
> > * like copy_from_user_toio(..) if it existed.
> > */
> > - if (copy_from_user((void __force *)dbuf, ubuf, len)) {
> > + temp = kmalloc(len, GFP_KERNEL);
> > + if (copy_from_user(temp, ubuf, len))
>
> This needs to have error handling for a kmalloc() failure. As the length
> appears to be user-provided, you might also want to limit the size of the
> allocation and instead do a loop with multiple copies if there is more data
> than fits into the allocation.
Thanks for your quick reply, you are right, will add error handling for
kmalloc() failure and size limit handling in v2.
Regards
Sherry
>
>Arnd
RE: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Vincent, > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > On Tue, Oct 27, 2020 at 08:05:43AM +0100, Sherry Sun wrote: > > Can you help test the patch about removing the codes of reassign used > > ring, and comment on the impact for Intel MIC platform? Thanks for > > any help. > > I don't have access to MIC hardware myself, either. > > But this patch is quite certainly going to break it since guests using a > kernel > without the patch will not work with hosts with a kernel with the patch. Thanks for your reply. This patch can be used by both guests and hosts. I have tested it on imx8qm platform(both guest and host are ARM64 architecture), and it works well. So I guess Intel MIC won't meet big problems when both guest and host apply this patch. But it is best if it can be tested. Best regards Sherry
RE: [EXT] Re: [PATCH V5 0/2] Change vring space from nomal memory to dma coherent memory
Hi Greg, > Subject: Re: [EXT] Re: [PATCH V5 0/2] Change vring space from nomal > memory to dma coherent memory > > On Wed, Oct 28, 2020 at 03:11:15PM +, Andy Duan wrote: > > From: Greg KH Sent: Wednesday, October > > 28, 2020 7:14 PM > > > On Wed, Oct 28, 2020 at 10:17:39AM +, Andy Duan wrote: > > > > From: Greg KH Sent: Wednesday, > > > > October 28, 2020 3:07 PM > > > > > On Wed, Oct 28, 2020 at 06:05:28AM +, Sherry Sun wrote: > > > > > > Hi Greg, > > > > > > > > > > > > > Subject: Re: [PATCH V5 0/2] Change vring space from nomal > > > > > > > memory to dma coherent memory > > > > > > > > > > > > > > On Wed, Oct 28, 2020 at 10:03:03AM +0800, Sherry Sun wrote: > > > > > > > > Changes in V5: > > > > > > > > 1. Reorganize the vop_mmap function code in patch 1, which > > > > > > > > is done by > > > > > > > Christoph. > > > > > > > > 2. Completely remove the unnecessary code related to > > > > > > > > reassign the used ring for card in patch 2. > > > > > > > > > > > > > > > > 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 vrings 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 | 4 - > > > > > > > > drivers/misc/mic/vop/vop_main.c| 70 +--- > > > > > > > > drivers/misc/mic/vop/vop_vringh.c | 166 > > > > > > > > ++- > -- > > > > > > > > include/uapi/linux/mic_common.h| 9 +- > > > > > > > > 7 files changed, 85 insertions(+), 218 deletions(-) > > > > > > > > > > > > > > Have you all seen: > > > > > > > > > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A > > > > > > > %2F%25 > > > > > > > 25 > > > > > > > > > > > > > > > > 2Flore.kernel.org%2Fr%2F8c1443136563de34699d2c084df478181c205db4.16 > > > > > > > > > > > > > > > > 03854416.git.sudeep.dutt%40intel.comdata=04%7C01%7Csherry.sun% > > > > > > > > > > > > > > > > 40nxp.com%7Cc19c987667434969847e08d87b0685e8%7C686ea1d3bc2b4c6f > > > > > > > > > > > > > > > > a92cd99c5c301635%7C0%7C0%7C637394615238940323%7CUnknown%7CTW > > > > > > > > > > > > > > > > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > > > > > > > > > > > > > > > > VCI6Mn0%3D%7C1000sdata=Zq%2FtHWTq%2BuIVBYXFGoeBmq0JJzYd > > > > > > > 9zDyv4NVN4TpC%2FU%3Dreserved=0 > > > > > > > > > > > > > > Looks like this code is asking to jus
RE: [PATCH V5 0/2] Change vring space from nomal memory to dma coherent memory
Hi Greg, > Subject: Re: [PATCH V5 0/2] Change vring space from nomal memory to dma > coherent memory > > On Wed, Oct 28, 2020 at 10:03:03AM +0800, Sherry Sun wrote: > > Changes in V5: > > 1. Reorganize the vop_mmap function code in patch 1, which is done by > Christoph. > > 2. Completely remove the unnecessary code related to reassign the used > > ring for card in patch 2. > > > > 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 vrings 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 | 4 - > > drivers/misc/mic/vop/vop_main.c| 70 +--- > > drivers/misc/mic/vop/vop_vringh.c | 166 ++--- > > include/uapi/linux/mic_common.h| 9 +- > > 7 files changed, 85 insertions(+), 218 deletions(-) > > Have you all seen: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F% > 2Flore.kernel.org%2Fr%2F8c1443136563de34699d2c084df478181c205db4.16 > 03854416.git.sudeep.dutt%40intel.comdata=04%7C01%7Csherry.sun% > 40nxp.com%7Cc19c987667434969847e08d87b0685e8%7C686ea1d3bc2b4c6f > a92cd99c5c301635%7C0%7C0%7C637394615238940323%7CUnknown%7CTW > FpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX > VCI6Mn0%3D%7C1000sdata=Zq%2FtHWTq%2BuIVBYXFGoeBmq0JJzYd > 9zDyv4NVN4TpC%2FU%3Dreserved=0 > > Looks like this code is asking to just be deleted, is that ok with you? Yes, I saw that patch. I'm ok with it. Best regards Sherry > > thanks, > > greg k-h
RE: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used ring
Hi Arnd, > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign the used > ring > > (resending from the kernel.org address after getting bounces again) > > On Wed, Oct 28, 2020 at 7:29 AM Sherry Sun wrote: > > > Subject: Re: [PATCH V3 2/4] misc: vop: do not allocate and reassign > > > the used > > > > > > Both Ashutosh and I have moved on to other projects. The MIC devices > > > have been discontinued. I have just sent across a patch to remove > > > the MIC drivers from the kernel tree. > > > > > > We are very glad to see that Sherry is able to reuse some of the VOP > > > logic and it is working well. It is best if the MIC drivers are > > > removed so Sherry can add the specific VOP logic required for imx8qm > > > subsequently without having to worry about other driver dependencies. > > > Hoping this results in a cleaner imx8qm driver moving forward. > > > > I'm ok with your patch. > > Since you have deprecated the MIC related code, may I ask do you have > > a better solution instead of vop/scif? > > I think we should try to do something on top of the PCIe endpoint subsystem > to make it work across arbitrary combinations of host and device > implementations, and provide a superset of what the MIC driver, (out-of- > tree) Bluefield endpoint driver, and the NTB subsystem as well as a couple of > others used to do, each of them tunneling block/network/serial/... over a > PCIe link of some sort, usually with virtio. > > At the moment, there is only one driver for the endpoint framework in the > kernel, in drivers/pci/endpoint/functions/pci-epf-test.c, but I think this can > serve as a starting point. > Thanks for your detailed reply. Yes, the PCIe endpoint subsystem is the base code, actually we have implemented a set of pci endpoint code similar to pci-epf-test.c, combine with vop (Virtio Over PCIe). But now the vop code is going to be removed, we planned to change to NTB framework, I saw Kishon has done some jobs based on NTB and PCIe endpoint subsystem, will get a deep look. Maybe it is a good solution. Best regards Sherry > The PCI endpoint subsystem already uses configfs for configuring the > available devices, and this seems like a good fit for making it work in > general. > However, there are a number of use cases that have somewhat conflicting > requirements, so the first step would be to figure out what everyone actually > needs for virtio communication. > > These are some of the main differences that I have noticed in the > past: > > - The simple case would be to use one PCIe endpoint device > for each virtio device, but I think this needs to be multiplexed > so that hardware that only supports a single PCIe endpoint > can still have multiple virtio devices tunneled through it. > > - While sometimes the configuration is hardcoded in the driver, ideally > the type of virtio device(s) that is tunneled over the PCIe link should > be configurable. The configuration of the endpoint device itself is > done on the machine running on the endpoint side, but for the > virtio devices, this might be either on the host or the endpoint. > Not sure if one of the two ways is common enough, or we have to > allow both. > > - When the link is configured, you still need one side to provide a > virtio device host implementation, while the other side would > run the normal virtio device driver. Again, these could be done > either way, and it is independent of which side has configured > the link, and we might want to only allow one of the two options, > or do both, or tie it to who configures it (e.g. the side that creates > the device must be the virtio device host, while the other side > just sees the device pop up and uses a virtio driver). > >Arnd
RE: [PATCH 0/5] Add noncoherent platform support for vop driver
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 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
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
> > Signed-off-by: Sherry Sun
> > ---
> > 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
> > #include
> > #include
> > +#include
> >
> > #include
> > #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(>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,
> > + _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);
> >
RE: [PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform
Hi Greg, > -Original Message- > From: Greg KH > Sent: 2020年9月25日 20:17 > To: Sherry Sun > Cc: sudeep.d...@intel.com; ashutosh.di...@intel.com; a...@arndb.de; > wang.y...@zte.com.cn; huang.ziji...@zte.com.cn; > rikard.falkeb...@gmail.com; lee.jo...@linaro.org; m...@redhat.com; linux- > ker...@vger.kernel.org; dl-linux-imx > 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 > > Signed-off-by: Joakim Zhang > > --- > > 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
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
> > Signed-off-by: Joakim Zhang
> > ---
> > 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(>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
Hi Christoph, > > +#include > > 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
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/3] mic: vop: fix a written error in MODULE_DEVICE_TABLE
Hi Greg,
> -Original Message-
> From: Greg KH
> Sent: 2020年9月27日 18:29
> To: Sherry Sun
> Cc: sudeep.d...@intel.com; ashutosh.di...@intel.com; a...@arndb.de;
> masahi...@kernel.org; michal.l...@markovi.net; lee.jo...@linaro.org;
> rikard.falkeb...@gmail.com; m...@redhat.co; b...@suse.de;
> jh...@codeaurora.org; t...@linutronix.de;
> manivannan.sadhasi...@linaro.org; mgr...@linux.intel.com; pierre-
> louis.boss...@linux.intel.com; linux-kernel@vger.kernel.org; linux-
> kbu...@vger.kernel.org; dl-linux-imx
> Subject: Re: [PATCH 1/3] mic: vop: fix a written error in
> MODULE_DEVICE_TABLE
>
> On Fri, Sep 25, 2020 at 03:31:56PM +0800, Sherry Sun wrote:
> > For vop bus, the first parameter should be vop in MODULE_DEVICE_TABLE.
> >
> > Signed-off-by: Sherry Sun
> > Signed-off-by: Joakim Zhang
> > ---
> > drivers/misc/mic/vop/vop_main.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/mic/vop/vop_main.c
> > b/drivers/misc/mic/vop/vop_main.c index d609f0dc6124..589425fa78d4
> > 100644
> > --- a/drivers/misc/mic/vop/vop_main.c
> > +++ b/drivers/misc/mic/vop/vop_main.c
> > @@ -796,7 +796,7 @@ static struct vop_driver vop_driver = {
> >
> > module_vop_driver(vop_driver);
> >
> > -MODULE_DEVICE_TABLE(mbus, id_table);
> > +MODULE_DEVICE_TABLE(vop, id_table);
> > MODULE_AUTHOR("Intel Corporation");
> > MODULE_DESCRIPTION("Intel(R) Virtio Over PCIe (VOP) driver");
> > MODULE_LICENSE("GPL v2");
>
> Doesn't this have to go _after_ the MODULE_DEVICE_TABLE(vop...) support,
> which you add in patch 2 of this series?
Yes, this patch must be used in conjunction with Patch2.
But I think here may be a small bug, in order to distinguish it from the driver
autoloading support, make this a separate patch.
I can put this patch together with Patch2 if you think it might look more
reasonable.
>
> Does this patch here break the build? If not, how is it working?
>
> And if you only have one vop driver, why do you need autoloading for it?
>
No, it doesn't break the build. But actually it won't work(autoloaded) when
kernel boot and vop device appears.
Although we may only have one vop driver, but in the mic Kconfig, the intel
mic/vop/cosm/scif drivers all
recommended to be built as modules, if we don't add autoloading for them, we
may need modprobe them
one by one manually both on EP and RC side.
Obviously, for our use case, driver autoloading is more convenient.
Best regards
Sherry
> thanks,
>
> greg k-h
RE: [PATCH 1/3] mic: vop: fix a written error in MODULE_DEVICE_TABLE
Hi Greg,
> On Sun, Sep 27, 2020 at 12:19:50PM +0000, Sherry Sun wrote:
> > Hi Greg,
> >
> > > -Original Message-
> > > From: Greg KH
> > > Sent: 2020年9月27日 18:29
> > > To: Sherry Sun
> > > Cc: sudeep.d...@intel.com; ashutosh.di...@intel.com; a...@arndb.de;
> > > masahi...@kernel.org; michal.l...@markovi.net; lee.jo...@linaro.org;
> > > rikard.falkeb...@gmail.com; m...@redhat.co; b...@suse.de;
> > > jh...@codeaurora.org; t...@linutronix.de;
> > > manivannan.sadhasi...@linaro.org; mgr...@linux.intel.com; pierre-
> > > louis.boss...@linux.intel.com; linux-kernel@vger.kernel.org; linux-
> > > kbu...@vger.kernel.org; dl-linux-imx
> > > Subject: Re: [PATCH 1/3] mic: vop: fix a written error in
> > > MODULE_DEVICE_TABLE
> > >
> > > On Fri, Sep 25, 2020 at 03:31:56PM +0800, Sherry Sun wrote:
> > > > For vop bus, the first parameter should be vop in
> MODULE_DEVICE_TABLE.
> > > >
> > > > Signed-off-by: Sherry Sun
> > > > Signed-off-by: Joakim Zhang
> > > > ---
> > > > drivers/misc/mic/vop/vop_main.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/misc/mic/vop/vop_main.c
> > > > b/drivers/misc/mic/vop/vop_main.c index
> d609f0dc6124..589425fa78d4
> > > > 100644
> > > > --- a/drivers/misc/mic/vop/vop_main.c
> > > > +++ b/drivers/misc/mic/vop/vop_main.c
> > > > @@ -796,7 +796,7 @@ static struct vop_driver vop_driver = {
> > > >
> > > > module_vop_driver(vop_driver);
> > > >
> > > > -MODULE_DEVICE_TABLE(mbus, id_table);
> > > > +MODULE_DEVICE_TABLE(vop, id_table);
> > > > MODULE_AUTHOR("Intel Corporation");
> > > > MODULE_DESCRIPTION("Intel(R) Virtio Over PCIe (VOP) driver");
> > > > MODULE_LICENSE("GPL v2");
> > >
> > > Doesn't this have to go _after_ the MODULE_DEVICE_TABLE(vop...)
> > > support, which you add in patch 2 of this series?
> >
> > Yes, this patch must be used in conjunction with Patch2.
> > But I think here may be a small bug, in order to distinguish it from
> > the driver autoloading support, make this a separate patch.
> >
> > I can put this patch together with Patch2 if you think it might look more
> reasonable.
>
> How about _after_ patch 2, otherwise this patch will break the build, right?
This change won't break the build, actually no matter what the first parameter
of MODULE_DEVICE_TABLE is, it won’t cause any build errors.
>
> > > Does this patch here break the build? If not, how is it working?
> > >
> > > And if you only have one vop driver, why do you need autoloading for it?
> > >
> > No, it doesn't break the build. But actually it won't work(autoloaded) when
> kernel boot and vop device appears.
> >
> > Although we may only have one vop driver, but in the mic Kconfig, the
> > intel mic/vop/cosm/scif drivers all recommended to be built as
> > modules, if we don't add autoloading for them, we may need modprobe
> them one by one manually both on EP and RC side.
> >
> > Obviously, for our use case, driver autoloading is more convenient.
>
> Why are these all not "mic_SUFFIX" type drivers? Why "vop" and "cosm"
> and "scif"?
>
For VOP driver, it is designed to be a hardware independent Virtio Over PCIe
(VOP) driver.
This is why we want to reuse it on i.MX platform. In theory, it can be applied
to any platform.
With some changes, cosm driver also can be hardware independent.
So the names of them don’t use "mic_SUFFIX".
> And if you only have 1 driver, then what would cause autoloading?
As I understand it, if we add support for the driver autoloading like patch2,
driver will be autoloaded when the matched device appears.
Please correct me if I'm wrong.
Regards
Sherry
>
> thanks,
>
> greg k-h
RE: [PATCH 3/3] mic: cosm: module autoprobing support for cosm driver
Hi Greg,
>
> On Fri, Sep 25, 2020 at 03:31:58PM +0800, Sherry Sun wrote:
> > Add uevent callback for cosm_bus and add cosm_device_id for cosm
> > driver which is needed for MODULE_DEVICE_TABLE. Also adding struct
> > cosm_device_id in devicetable-offsets.c and the cosm entry point in
> > file2alias.c.
> >
> > Cosm driver will be autoloaded when cosm device appears.
> >
> > Signed-off-by: Sherry Sun
> > ---
> > drivers/misc/mic/bus/cosm_bus.c | 8
> > drivers/misc/mic/cosm/cosm_main.c | 7 +++
> > include/linux/mod_devicetable.h | 8
> > scripts/mod/devicetable-offsets.c | 3 +++
> > scripts/mod/file2alias.c | 11 +++
> > 5 files changed, 37 insertions(+)
> >
> > diff --git a/drivers/misc/mic/bus/cosm_bus.c
> > b/drivers/misc/mic/bus/cosm_bus.c index 5f2141c71738..736e27bbc9f9
> > 100644
> > --- a/drivers/misc/mic/bus/cosm_bus.c
> > +++ b/drivers/misc/mic/bus/cosm_bus.c
> > @@ -14,6 +14,13 @@
> > /* Unique numbering for cosm devices. */ static
> > DEFINE_IDA(cosm_index_ida);
> >
> > +static int cosm_uevent(struct device *d, struct kobj_uevent_env *env)
> > +{
> > + struct cosm_device *dev = dev_to_cosm(d);
> > +
> > + return add_uevent_var(env, "MODALIAS=cosm:cosm-dev%u", dev-
> >index);
> > +}
> > +
> > static int cosm_dev_probe(struct device *d) {
> > struct cosm_device *dev = dev_to_cosm(d); @@ -33,6 +40,7 @@
> static
> > int cosm_dev_remove(struct device *d)
> >
> > static struct bus_type cosm_bus = {
> > .name = "cosm_bus",
> > + .uevent = cosm_uevent,
> > .probe = cosm_dev_probe,
> > .remove = cosm_dev_remove,
> > };
> > diff --git a/drivers/misc/mic/cosm/cosm_main.c
> > b/drivers/misc/mic/cosm/cosm_main.c
> > index ebb0eac43754..627e7d5f3a83 100644
> > --- a/drivers/misc/mic/cosm/cosm_main.c
> > +++ b/drivers/misc/mic/cosm/cosm_main.c
> > @@ -12,6 +12,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include "cosm_main.h"
> >
> > static const char cosm_driver_name[] = "mic"; @@ -323,6 +324,12 @@
> > static int cosm_suspend(struct device *dev)
> > return 0;
> > }
> >
> > +static struct cosm_device_id __maybe_unused cosm_driver_id_table[] = {
> > + { .name = "cosm-dev*" },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(cosm, cosm_driver_id_table);
> > +
> > static const struct dev_pm_ops cosm_pm_ops = {
> > .suspend = cosm_suspend,
> > .freeze = cosm_suspend
> > diff --git a/include/linux/mod_devicetable.h
> > b/include/linux/mod_devicetable.h index 736cdc236cf9..ea6cdfe1a3a3
> > 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -845,4 +845,12 @@ struct vop_device_id { };
> > #define VOP_DEV_ANY_ID 0x
> >
> > +/* cosm */
> > +#define COSM_NAME_SIZE 32
> > +#define COSM_MODULE_PREFIX "cosm:"
> > +
> > +struct cosm_device_id {
> > + char name[COSM_NAME_SIZE];
> > +};
> > +
> > #endif /* LINUX_MOD_DEVICETABLE_H */
> > diff --git a/scripts/mod/devicetable-offsets.c
> > b/scripts/mod/devicetable-offsets.c
> > index 393acaa5302a..499a2832878d 100644
> > --- a/scripts/mod/devicetable-offsets.c
> > +++ b/scripts/mod/devicetable-offsets.c
> > @@ -247,5 +247,8 @@ int main(void)
> > DEVID_FIELD(vop_device_id, device);
> > DEVID_FIELD(vop_device_id, vendor);
> >
> > + DEVID(cosm_device_id);
> > + DEVID_FIELD(cosm_device_id, name);
> > +
> > return 0;
> > }
> > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c index
> > 8063b778eedf..f7c80e4da137 100644
> > --- a/scripts/mod/file2alias.c
> > +++ b/scripts/mod/file2alias.c
> > @@ -1383,6 +1383,16 @@ static int do_vop_entry(const char *filename,
> void *symval,
> > return 1;
> > }
> >
> > +/* Looks like: cosm:S */
> > +static int do_cosm_entry(const char *filename, void *symval,
> > + char *alias)
> > +{
> > + DEF_FIELD_ADDR(symval, cosm_device_id, name);
> > + sprintf(alias, COSM_MODULE_PREFIX "%s", *name);
> > +
> > + return 1;
> > +}
> > +
> > /* Does namelen bytes of name exactly match the symbol? */ static
> > bool sym_is(const char *name, unsigned namelen, const char *symbol) {
> > @@ -1458,6 +14
RE: [PATCH 4/5] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for nocoherent platform
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
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
[PATCH V2 1/2] mic: vop: module autoprobing support for vop drivers
Add vop autoprobing support to MODULE_DEVICE_TABLE() by adding info
about struct vop_device_id in devicetable-offsets.c and add a vop entry
point in file2alias.c.
The type argument for MODULE_DEVICE_TABLE(type, name) is vop.
Signed-off-by: Sherry Sun
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/bus/vop_bus.h| 7 +--
drivers/misc/mic/vop/vop_main.c | 2 +-
include/linux/mod_devicetable.h | 7 +++
scripts/mod/devicetable-offsets.c | 4
scripts/mod/file2alias.c | 16
5 files changed, 29 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/mic/bus/vop_bus.h b/drivers/misc/mic/bus/vop_bus.h
index d891eacae358..5b4a58757951 100644
--- a/drivers/misc/mic/bus/vop_bus.h
+++ b/drivers/misc/mic/bus/vop_bus.h
@@ -14,16 +14,11 @@
*/
#include
#include
+#include
#include "../common/mic_dev.h"
-struct vop_device_id {
- u32 device;
- u32 vendor;
-};
-
#define VOP_DEV_TRNSP 1
-#define VOP_DEV_ANY_ID 0x
/*
* Size of the internal buffer used during DMA's as an intermediate buffer
* for copy to/from user. Must be an integral number of pages.
diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index 1ccc94dfd6ac..f1e908286843 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -742,7 +742,7 @@ static struct vop_driver vop_driver = {
module_vop_driver(vop_driver);
-MODULE_DEVICE_TABLE(mbus, id_table);
+MODULE_DEVICE_TABLE(vop, id_table);
MODULE_AUTHOR("Intel Corporation");
MODULE_DESCRIPTION("Intel(R) Virtio Over PCIe (VOP) driver");
MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 5b08a473cdba..736cdc236cf9 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -838,4 +838,11 @@ struct mhi_device_id {
kernel_ulong_t driver_data;
};
+/* vop */
+struct vop_device_id {
+ __u32 device;
+ __u32 vendor;
+};
+#define VOP_DEV_ANY_ID 0x
+
#endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/scripts/mod/devicetable-offsets.c
b/scripts/mod/devicetable-offsets.c
index 27007c18e754..393acaa5302a 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -243,5 +243,9 @@ int main(void)
DEVID(mhi_device_id);
DEVID_FIELD(mhi_device_id, chan);
+ DEVID(vop_device_id);
+ DEVID_FIELD(vop_device_id, device);
+ DEVID_FIELD(vop_device_id, vendor);
+
return 0;
}
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 2417dd1dee33..8063b778eedf 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1368,6 +1368,21 @@ static int do_mhi_entry(const char *filename, void
*symval, char *alias)
return 1;
}
+/* Looks like: vop:dNvN */
+static int do_vop_entry(const char *filename, void *symval,
+ char *alias)
+{
+ DEF_FIELD(symval, vop_device_id, device);
+ DEF_FIELD(symval, vop_device_id, vendor);
+
+ strcpy(alias, "vop:");
+ ADD(alias, "d", device != VOP_DEV_ANY_ID, device);
+ ADD(alias, "v", vendor != VOP_DEV_ANY_ID, vendor);
+
+ add_wildcard(alias);
+ return 1;
+}
+
/* Does namelen bytes of name exactly match the symbol? */
static bool sym_is(const char *name, unsigned namelen, const char *symbol)
{
@@ -1442,6 +1457,7 @@ static const struct devtable devtable[] = {
{"tee", SIZE_tee_client_device_id, do_tee_entry},
{"wmi", SIZE_wmi_device_id, do_wmi_entry},
{"mhi", SIZE_mhi_device_id, do_mhi_entry},
+ {"vop", SIZE_vop_device_id, do_vop_entry},
};
/* Create MODULE_ALIAS() statements.
--
2.17.1
[PATCH V2 2/2] mic: cosm: module autoprobing support for cosm driver
Add uevent callback for cosm_bus and add cosm_device_id for cosm driver
which is needed for MODULE_DEVICE_TABLE. Also adding struct
cosm_device_id in devicetable-offsets.c and the cosm entry point in
file2alias.c.
Cosm driver will be autoloaded when cosm device appears.
Signed-off-by: Sherry Sun
---
drivers/misc/mic/bus/cosm_bus.c | 8
drivers/misc/mic/cosm/cosm_main.c | 7 +++
include/linux/mod_devicetable.h | 8
scripts/mod/devicetable-offsets.c | 3 +++
scripts/mod/file2alias.c | 11 +++
5 files changed, 37 insertions(+)
diff --git a/drivers/misc/mic/bus/cosm_bus.c b/drivers/misc/mic/bus/cosm_bus.c
index 5f2141c71738..736e27bbc9f9 100644
--- a/drivers/misc/mic/bus/cosm_bus.c
+++ b/drivers/misc/mic/bus/cosm_bus.c
@@ -14,6 +14,13 @@
/* Unique numbering for cosm devices. */
static DEFINE_IDA(cosm_index_ida);
+static int cosm_uevent(struct device *d, struct kobj_uevent_env *env)
+{
+ struct cosm_device *dev = dev_to_cosm(d);
+
+ return add_uevent_var(env, "MODALIAS=cosm:cosm-dev%u", dev->index);
+}
+
static int cosm_dev_probe(struct device *d)
{
struct cosm_device *dev = dev_to_cosm(d);
@@ -33,6 +40,7 @@ static int cosm_dev_remove(struct device *d)
static struct bus_type cosm_bus = {
.name = "cosm_bus",
+ .uevent = cosm_uevent,
.probe = cosm_dev_probe,
.remove = cosm_dev_remove,
};
diff --git a/drivers/misc/mic/cosm/cosm_main.c
b/drivers/misc/mic/cosm/cosm_main.c
index ebb0eac43754..627e7d5f3a83 100644
--- a/drivers/misc/mic/cosm/cosm_main.c
+++ b/drivers/misc/mic/cosm/cosm_main.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include "cosm_main.h"
static const char cosm_driver_name[] = "mic";
@@ -323,6 +324,12 @@ static int cosm_suspend(struct device *dev)
return 0;
}
+static struct cosm_device_id __maybe_unused cosm_driver_id_table[] = {
+ { .name = "cosm-dev*" },
+ { },
+};
+MODULE_DEVICE_TABLE(cosm, cosm_driver_id_table);
+
static const struct dev_pm_ops cosm_pm_ops = {
.suspend = cosm_suspend,
.freeze = cosm_suspend
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 736cdc236cf9..ea6cdfe1a3a3 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -845,4 +845,12 @@ struct vop_device_id {
};
#define VOP_DEV_ANY_ID 0x
+/* cosm */
+#define COSM_NAME_SIZE 32
+#define COSM_MODULE_PREFIX "cosm:"
+
+struct cosm_device_id {
+ char name[COSM_NAME_SIZE];
+};
+
#endif /* LINUX_MOD_DEVICETABLE_H */
diff --git a/scripts/mod/devicetable-offsets.c
b/scripts/mod/devicetable-offsets.c
index 393acaa5302a..499a2832878d 100644
--- a/scripts/mod/devicetable-offsets.c
+++ b/scripts/mod/devicetable-offsets.c
@@ -247,5 +247,8 @@ int main(void)
DEVID_FIELD(vop_device_id, device);
DEVID_FIELD(vop_device_id, vendor);
+ DEVID(cosm_device_id);
+ DEVID_FIELD(cosm_device_id, name);
+
return 0;
}
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 8063b778eedf..f7c80e4da137 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1383,6 +1383,16 @@ static int do_vop_entry(const char *filename, void
*symval,
return 1;
}
+/* Looks like: cosm:S */
+static int do_cosm_entry(const char *filename, void *symval,
+ char *alias)
+{
+ DEF_FIELD_ADDR(symval, cosm_device_id, name);
+ sprintf(alias, COSM_MODULE_PREFIX "%s", *name);
+
+ return 1;
+}
+
/* Does namelen bytes of name exactly match the symbol? */
static bool sym_is(const char *name, unsigned namelen, const char *symbol)
{
@@ -1458,6 +1468,7 @@ static const struct devtable devtable[] = {
{"wmi", SIZE_wmi_device_id, do_wmi_entry},
{"mhi", SIZE_mhi_device_id, do_mhi_entry},
{"vop", SIZE_vop_device_id, do_vop_entry},
+ {"cosm", SIZE_cosm_device_id, do_cosm_entry},
};
/* Create MODULE_ALIAS() statements.
--
2.17.1
[PATCH V2 0/2] Add module autoloading support for vop and cosm driver
Changes in V2: 1. Combine patch1 and patch2 in V1 together, as patch1 is a part of autoloading support for vop driver in patch2. Add module autoloading support for vop and cosm driver, when the vop/cosm device appears, the driver will be autoloaded. Sherry Sun (2): mic: vop: module autoprobing support for vop drivers mic: cosm: module autoprobing support for cosm driver drivers/misc/mic/bus/cosm_bus.c | 8 drivers/misc/mic/bus/vop_bus.h| 7 +-- drivers/misc/mic/cosm/cosm_main.c | 7 +++ drivers/misc/mic/vop/vop_main.c | 2 +- include/linux/mod_devicetable.h | 15 +++ scripts/mod/devicetable-offsets.c | 7 +++ scripts/mod/file2alias.c | 27 +++ 7 files changed, 66 insertions(+), 7 deletions(-) -- 2.17.1
[PATCH V2 0/4] Change vring space from nomal memory to dma coherent memory
Changes in V2: 1. Remove dev_is_dma_coherent check as Christoph suggested in the patchset. 2. Remove used_address_updated flags in Patch2. 3. Remove WARN_ON in Patch3 and return error code instead directly return -1. 4. Remove Patch4 as it is a separate patch now. 5. Add more detailed explanations in the commit of patches. 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 (4): misc: vop: change the way of allocating vring misc: vop: do not allocate and reassign the used ring misc: vop: simply return the saved dma address instead of virt_to_phys 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_debugfs.c | 2 - drivers/misc/mic/vop/vop_main.c| 48 +++-- drivers/misc/mic/vop/vop_vringh.c | 83 +++--- include/uapi/linux/mic_common.h| 3 -- 6 files changed, 35 insertions(+), 111 deletions(-) -- 2.17.1
[PATCH V2 4/4] misc: vop: mapping kernel memory to user space as noncached
Mapping kernel space memory to user space as noncached, since user space need check the updates of avail_idx and device page flags timely. Signed-off-by: Joakim Zhang Signed-off-by: Sherry Sun --- drivers/misc/mic/vop/vop_vringh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/misc/mic/vop/vop_vringh.c b/drivers/misc/mic/vop/vop_vringh.c index 4d5feb39aeb7..6e193bd64ef1 100644 --- a/drivers/misc/mic/vop/vop_vringh.c +++ b/drivers/misc/mic/vop/vop_vringh.c @@ -1057,7 +1057,7 @@ 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); + pgprot_noncached(vma->vm_page_prot)); if (err) goto ret; size_rem -= size; -- 2.17.1
[PATCH V2 1/4] misc: vop: change the way of allocating vring
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.
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.
Signed-off-by: Joakim Zhang
Signed-off-by: Sherry Sun
---
drivers/misc/mic/vop/vop_vringh.c | 39 +++
1 file changed, 9 insertions(+), 30 deletions(-)
diff --git a/drivers/misc/mic/vop/vop_vringh.c
b/drivers/misc/mic/vop/vop_vringh.c
index 7014ffe88632..08e63c81c5b2 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -298,9 +298,7 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
mutex_init(>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, _addr,
GFP_KERNEL);
if (!vr->va) {
ret = -ENOMEM;
dev_err(vop_dev(vdev), "%s %d err %d\n",
@@ -310,15 +308,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(>dev, vr->va, vr_size,
-DMA_BIDIRECTIONAL);
- if (dma_mapping_error(>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, num, vr->va, MIC_VIRTIO_RING_ALIGN);
@@ -339,11 +328,8 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
dev_dbg(>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(>dev,
- vvr->buf, VOP_INT_DMA_BUF_SIZE,
- DMA_BIDIRECTIONAL);
+ vvr->buf = dma_alloc_coherent(vop_dev(vdev),
VOP_INT_DMA_BUF_SIZE,
+ >buf_da, GFP_KERNEL);
}
snprintf(irqname, sizeof(irqname), "vop%dvirtio%d", vpdev->index,
@@ -382,10 +368,8 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
for (j = 0; j < i; j++) {
struct vop_vringh *vvr = >vvr[j];
- dma_unmap_single(>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 +417,12 @@ static void vop_virtio_del_device(struct vop_vdev *vdev)
for (i = 0; i < vdev->dd->num_vq; i++) {
struct vop_vringh *vvr = >vvr[i];
- dma_unmap_single(>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(>riov);
vringh_kiov_cleanup(>wiov);
- dma_unmap_single(>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
--
2.17.1
[PATCH V2 3/4] misc: vop: simply return the saved dma address instead of virt_to_phys
The device page and vring should use consistent memory which are
allocated by dma_alloc_coherent api, 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
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/bus/vop_bus.h| 2 ++
drivers/misc/mic/host/mic_boot.c | 8
drivers/misc/mic/vop/vop_vringh.c | 11 +--
3 files changed, 19 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(>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 422a28c1bb7a..4d5feb39aeb7 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -995,6 +995,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;
@@ -1007,7 +1008,13 @@ 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");
+ return -EINVAL;
+ }
+
*size = MIC_DP_SIZE;
return 0;
}
@@ -1016,7 +1023,7 @@ vop_query_offset(struct vop_vdev *vdev, unsigned long
offset,
struct vop_vringh *vvr = >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 V2 2/4] misc: vop: do not allocate and reassign the used ring
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
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/vop/vop_debugfs.c | 2 --
drivers/misc/mic/vop/vop_main.c| 48 --
drivers/misc/mic/vop/vop_vringh.c | 31 ---
include/uapi/linux/mic_common.h| 3 --
4 files changed, 6 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..1ccc94dfd6ac 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(>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,8 @@ 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 = 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 +351,15 @@ 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(>dev, used,
- vdev->used_size[index],
- DMA_BIDIRECTIONAL);
- if (dma_mapping_error(>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] = 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], >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 +372,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(>desc->num_vq))
@@ -412,24 +394,6 @@ static int vop_find_vqs(struct virtio_device *dev,
unsig
[PATCH] misc: vop: set VIRTIO_F_ACCESS_PLATFORM for vop driver
Set VIRTIO_F_ACCESS_PLATFORM feature for vop driver, as the DMA mapping details shouldn't decide on the virtio implementation, but the host PCIe implementation. Signed-off-by: Sherry Sun Signed-off-by: Joakim Zhang --- drivers/misc/mic/vop/vop_main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c index 6722c726b259..714b94f42d38 100644 --- a/drivers/misc/mic/vop/vop_main.c +++ b/drivers/misc/mic/vop/vop_main.c @@ -124,6 +124,7 @@ static void vop_transport_features(struct virtio_device *vdev) * creates virtio rings on preallocated memory. */ __virtio_clear_bit(vdev, VIRTIO_F_RING_PACKED); + __virtio_set_bit(vdev, VIRTIO_F_ACCESS_PLATFORM); } static int vop_finalize_features(struct virtio_device *vdev) -- 2.17.1
[PATCH V2 2/4] misc: vop: build VOP based on CONFIG_VOP
Build module or builtin VOP based on CONFIG_VOP macro. Signed-off-by: Joakim Zhang Signed-off-by: Sherry Sun --- drivers/misc/mic/vop/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/misc/mic/vop/Makefile b/drivers/misc/mic/vop/Makefile index 579da3868c8e..51b9b0022786 100644 --- a/drivers/misc/mic/vop/Makefile +++ b/drivers/misc/mic/vop/Makefile @@ -3,7 +3,7 @@ # Makefile - Intel MIC Linux driver. # Copyright(c) 2016, Intel Corporation. # -obj-m := vop.o +obj-$(CONFIG_VOP) := vop.o vop-objs += vop_main.o vop-objs += vop_debugfs.o -- 2.17.1
[PATCH V2 1/4] samples: mpssd: fix the build errors when enable DEBUG
If enable DEBUG, will meet the following errors when build mpssd, so fix
them here. Only one error is listed here, other errors are similar.
mpssd.c: In function ???virtio_net???:
mpssd.c:615:21: error: incompatible type for argument 2 of ???disp_iovec???
disp_iovec(mic, copy, __func__, __LINE__);
^~~~
mpssd.c:361:1: note: expected ???struct mic_copy_desc *??? but argument is of
type ???struct mic_copy_desc???
disp_iovec(struct mic_info *mic, struct mic_copy_desc *copy,
^~
Signed-off-by: Sherry Sun
---
samples/mic/mpssd/mpssd.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/samples/mic/mpssd/mpssd.c b/samples/mic/mpssd/mpssd.c
index a11bf6c5b53b..51d03545869e 100644
--- a/samples/mic/mpssd/mpssd.c
+++ b/samples/mic/mpssd/mpssd.c
@@ -612,7 +612,7 @@ virtio_net(void *arg)
copy.out_len, hdr->gso_type);
#endif
#ifdef DEBUG
- disp_iovec(mic, copy, __func__, __LINE__);
+ disp_iovec(mic, , __func__, __LINE__);
mpsslog("%s %s %d read from tap 0x%lx\n",
mic->name, __func__, __LINE__,
len);
@@ -632,7 +632,7 @@ virtio_net(void *arg)
if (!err)
verify_out_len(mic, );
#ifdef DEBUG
- disp_iovec(mic, copy, __func__, __LINE__);
+ disp_iovec(mic, , __func__, __LINE__);
mpsslog("%s %s %d wrote to net 0x%lx\n",
mic->name, __func__, __LINE__,
sum_iovec_len());
@@ -681,12 +681,12 @@ virtio_net(void *arg)
sizeof(struct virtio_net_hdr);
verify_out_len(mic, );
#ifdef DEBUG
- disp_iovec(mic, copy, __func__,
+ disp_iovec(mic, , __func__,
__LINE__);
mpsslog("%s %s %d ",
mic->name, __func__, __LINE__);
mpsslog("read from net 0x%lx\n",
- sum_iovec_len(copy));
+ sum_iovec_len());
#endif
len = writev(net_poll[NET_FD_TUN].fd,
copy.iov, copy.iovcnt);
@@ -814,7 +814,7 @@ virtio_console(void *arg)
len = readv(pty_fd, copy.iov, copy.iovcnt);
if (len > 0) {
#ifdef DEBUG
- disp_iovec(mic, copy, __func__, __LINE__);
+ disp_iovec(mic, , __func__, __LINE__);
mpsslog("%s %s %d read from tap 0x%lx\n",
mic->name, __func__, __LINE__,
len);
@@ -834,10 +834,10 @@ virtio_console(void *arg)
if (!err)
verify_out_len(mic, );
#ifdef DEBUG
- disp_iovec(mic, copy, __func__, __LINE__);
+ disp_iovec(mic, , __func__, __LINE__);
mpsslog("%s %s %d wrote to net 0x%lx\n",
mic->name, __func__, __LINE__,
- sum_iovec_len(copy));
+ sum_iovec_len());
#endif
/* Reinitialize IOV for next run */
iov0->iov_len = PAGE_SIZE;
@@ -866,12 +866,12 @@ virtio_console(void *arg)
iov1->iov_len = copy.out_len;
verify_out_len(mic, );
#ifdef DEBUG
- disp_iovec(mic, copy, __func__,
+ disp_iovec(mic, , __func__,
__LINE__);
mpsslog("%s %s %d ",
mic->name, __func__, __LINE__);
mpsslog("read from net 0x%lx\n",
- sum_iovec_len(copy));
+ sum_iovec_len());
#endif
len = writev(pty_fd,
copy.iov, copy.iovcnt);
@@ -883,7 +883,7 @@ virtio_console(void *arg)
[PATCH V2 3/4] misc: vop: add round_up(x,4) for vring_size to avoid kernel panic
Since struct _mic_vring_info and vring are allocated together and follow
vring, if the vring_size() is not four bytes aligned, which will cause
the start address of struct _mic_vring_info is not four byte aligned.
For example, when vring entries is 128, the vring_size() will be 5126
bytes. The _mic_vring_info struct layout in ddr looks like:
0x90002400: 0039 EE01 C0FF
Here 0x39 is the avail_idx member, and 0xC0FFEE01 is the magic member.
When EP use ioread32(magic) to reads the magic in RC's share memory, it
will cause kernel panic on ARM64 platform due to the cross-byte io read.
Here read magic in user space use le32toh(vr0->info->magic) will meet
the same issue.
So add round_up(x,4) for vring_size, then the struct _mic_vring_info
will store in this way:
0x90002400: 0039 C0FFEE01
Which will avoid kernel panic when read magic in struct _mic_vring_info.
Signed-off-by: Sherry Sun
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/vop/vop_main.c | 2 +-
drivers/misc/mic/vop/vop_vringh.c | 4 ++--
samples/mic/mpssd/mpssd.c | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/mic/vop/vop_main.c b/drivers/misc/mic/vop/vop_main.c
index 55e7f21e51f4..6722c726b259 100644
--- a/drivers/misc/mic/vop/vop_main.c
+++ b/drivers/misc/mic/vop/vop_main.c
@@ -320,7 +320,7 @@ static struct virtqueue *vop_find_vq(struct virtio_device
*dev,
/* First assign the vring's allocated in host memory */
vqconfig = _vop_vq_config(vdev->desc) + index;
memcpy_fromio(, vqconfig, sizeof(config));
- _vr_size = vring_size(le16_to_cpu(config.num), MIC_VIRTIO_RING_ALIGN);
+ _vr_size = round_up(vring_size(le16_to_cpu(config.num),
MIC_VIRTIO_RING_ALIGN), 4);
vr_size = PAGE_ALIGN(_vr_size + sizeof(struct _mic_vring_info));
va = vpdev->hw_ops->remap(vpdev, le64_to_cpu(config.address), vr_size);
if (!va)
diff --git a/drivers/misc/mic/vop/vop_vringh.c
b/drivers/misc/mic/vop/vop_vringh.c
index 30eac172f017..45fdb394de11 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -296,7 +296,7 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
num = le16_to_cpu(vqconfig[i].num);
mutex_init(>vr_mutex);
- vr_size = PAGE_ALIGN(vring_size(num, MIC_VIRTIO_RING_ALIGN) +
+ 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,
@@ -308,7 +308,7 @@ static int vop_virtio_add_device(struct vop_vdev *vdev,
goto err;
}
vr->len = vr_size;
- vr->info = vr->va + vring_size(num, MIC_VIRTIO_RING_ALIGN);
+ 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(>dev, vr->va, vr_size,
DMA_BIDIRECTIONAL);
diff --git a/samples/mic/mpssd/mpssd.c b/samples/mic/mpssd/mpssd.c
index 51d03545869e..c03a05d498f0 100644
--- a/samples/mic/mpssd/mpssd.c
+++ b/samples/mic/mpssd/mpssd.c
@@ -403,9 +403,9 @@ mic_virtio_copy(struct mic_info *mic, int fd,
static inline unsigned _vring_size(unsigned int num, unsigned long align)
{
- return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (3 + num)
+ return _ALIGN_UP(((sizeof(struct vring_desc) * num + sizeof(__u16) * (3
+ num)
+ align - 1) & ~(align - 1))
- + sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num;
+ + sizeof(__u16) * 3 + sizeof(struct vring_used_elem) * num, 4);
}
/*
--
2.17.1
[PATCH V2 0/4] Fix some bugs of the vop driver and mpssd user space tool
Changes in V2: 1. Change the way of copy data from ubuf to dbuf in patch4. Reuse the allocated vvr->buf as the temp buffer and add a loop for multiple copies if there are too much data. This patchset fix some bugs about the vop driver and Intel MIC user space tool. Sherry Sun (4): samples: mpssd: fix the build errors when enable DEBUG in mpssd.c misc: vop: build VOP based on CONFIG_VOP misc: vop: add round_up(x,4) for vring_size to avoid kernel panic mic: vop: copy data to kernel space then write to io memory drivers/misc/mic/vop/Makefile | 2 +- drivers/misc/mic/vop/vop_main.c | 2 +- drivers/misc/mic/vop/vop_vringh.c | 24 samples/mic/mpssd/mpssd.c | 24 4 files changed, 30 insertions(+), 22 deletions(-) -- 2.17.1
[PATCH V2 4/4] mic: vop: copy data to kernel space then write to io memory
Read and write io memory should address align on ARCH ARM. Change to use
memcpy_toio to avoid kernel panic caused by the address un-align issue.
Signed-off-by: Sherry Sun
Signed-off-by: Joakim Zhang
---
drivers/misc/mic/vop/vop_vringh.c | 20 ++--
1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/misc/mic/vop/vop_vringh.c
b/drivers/misc/mic/vop/vop_vringh.c
index 45fdb394de11..7014ffe88632 100644
--- a/drivers/misc/mic/vop/vop_vringh.c
+++ b/drivers/misc/mic/vop/vop_vringh.c
@@ -602,6 +602,7 @@ static int vop_virtio_copy_from_user(struct vop_vdev *vdev,
void __user *ubuf,
size_t partlen;
bool dma = VOP_USE_DMA && vi->dma_ch;
int err = 0;
+ size_t offset = 0;
if (dma) {
dma_alignment = 1 << vi->dma_ch->device->copy_align;
@@ -655,13 +656,20 @@ static int vop_virtio_copy_from_user(struct vop_vdev
*vdev, void __user *ubuf,
* We are copying to IO below and should ideally use something
* like copy_from_user_toio(..) if it existed.
*/
- if (copy_from_user((void __force *)dbuf, ubuf, len)) {
- err = -EFAULT;
- dev_err(vop_dev(vdev), "%s %d err %d\n",
- __func__, __LINE__, err);
- goto err;
+ while (len) {
+ partlen = min_t(size_t, len, VOP_INT_DMA_BUF_SIZE);
+
+ if (copy_from_user(vvr->buf, ubuf + offset, partlen)) {
+ err = -EFAULT;
+ dev_err(vop_dev(vdev), "%s %d err %d\n",
+ __func__, __LINE__, err);
+ goto err;
+ }
+ memcpy_toio(dbuf + offset, vvr->buf, partlen);
+ offset += partlen;
+ vdev->out_bytes += partlen;
+ len -= partlen;
}
- vdev->out_bytes += len;
err = 0;
err:
vpdev->hw_ops->unmap(vpdev, dbuf);
--
2.17.1
RE: [PATCH V2 4/4] misc: vop: mapping kernel memory to user space as noncached
Hi David, thanks for your information. Hi Christoph, please see my comments below. > Subject: RE: [PATCH V2 4/4] misc: vop: mapping kernel memory to user space > as noncached > > From: Christoph Hellwig > > Sent: 29 September 2020 11:29 > ... > > You can't call remap_pfn_range on memory returned from > > dma_alloc_coherent (which btw is not marked uncached on many > platforms). > > > > You need to use the dma_mmap_coherent helper instead. > I tried to use dma_mmap_coherent helper here, but I met the same problem as David said. Since the user space calls mmap() to map all the device page and vring size at one time. va = mmap(NULL, MIC_DEVICE_PAGE_END + vr_size * num_vq, PROT_READ, MAP_SHARED, fd, 0); But the physical addresses of device page and multiple vrings are not consecutive, so we called multiple remap_pfn_range before. When changing to use dma_mmap_coherent, it will return error because vma_pages(the size user space want to map) are bigger than the actual size we do multiple map(one non-continuous memory size at a time). David believes that we could modify the vm_start address before call the multiple dma_mmap_coherent to avoid the vma_pages check error and map multiple discontinuous memory. Do you have any suggestions? Best regards Sherry > H. I've a driver that does that. > Fortunately it only has to work on x86 where it doesn't matter. > However I can't easily convert it. > The 'problem' is that the mmap() request can cover multiple dma buffers and > need not start at the beginning of one. > > Basically we have a PCIe card that has an inbuilt iommu to convert internal > 32bit addresses to 64bit PCIe ones. > This has 512 16kB pages. > So we do a number of dma_alloc_coherent() for 16k blocks. > The user process then does an mmap() for part of the buffer. > This request is 4k aligned so we do multiple remap_pfn_range() calls to map > the disjoint physical (and kernel virtual) buffers into contiguous user > memory. > > So both ends see contiguous addresses even though the physical addresses > are non-contiguous. > > I guess I could modify the vm_start address and then restore it at the end. > > I found this big discussion: > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.k > ernel.org%2Fpatchwork%2Fpatch%2F351245%2Fdata=02%7C01%7Csh > erry.sun%40nxp.com%7C876724689688440581a708d8648dceb3%7C686ea1d > 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637369907516376294sdat > a=amSClQfVGhI0%2F3bZfo8HF7UmCktkPluArWW22YlQzMQ%3Dreser > ved=0 > about these functions. > > The bit about VIPT caches is problematic. > I don't think you can change the kernel address during mmap. > What you need to do is defer allocating the user address until you know the > kernel address. > Otherwise you get into problems is you try to mmap the same memory into > two processes. > This is a general problem even for mmap() of files. > ISTR SYSV on some sparc systems having to use uncached maps. > > If you might want to mmap two kernel buffers (dma or not) into adjacent > user addresses then you need some way of allocating the second buffer to > follow the first one in the VIVT cache. > > David > > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > MK1 1PT, UK Registration No: 1397386 (Wales)
RE: [PATCH V2 0/2] Add module autoloading support for vop and cosm driver
Gentle ping > Subject: [PATCH V2 0/2] Add module autoloading support for vop and cosm > driver > > Changes in V2: > 1. Combine patch1 and patch2 in V1 together, as patch1 is a part of > autoloading support for vop driver in patch2. > > Add module autoloading support for vop and cosm driver, when the > vop/cosm device appears, the driver will be autoloaded. > > Sherry Sun (2): > mic: vop: module autoprobing support for vop drivers > mic: cosm: module autoprobing support for cosm driver > > drivers/misc/mic/bus/cosm_bus.c | 8 > drivers/misc/mic/bus/vop_bus.h| 7 +-- > drivers/misc/mic/cosm/cosm_main.c | 7 +++ > drivers/misc/mic/vop/vop_main.c | 2 +- > include/linux/mod_devicetable.h | 15 +++ > scripts/mod/devicetable-offsets.c | 7 +++ > scripts/mod/file2alias.c | 27 +++ > 7 files changed, 66 insertions(+), 7 deletions(-) > > -- > 2.17.1
RE: [PATCH V2 0/2] Add module autoloading support for vop and cosm driver
Hi Greg, > Subject: Re: [PATCH V2 0/2] Add module autoloading support for vop and > cosm driver > > On Tue, Oct 13, 2020 at 08:52:01AM +0000, Sherry Sun wrote: > > Gentle ping > > It's the merge window, sorry, this fell through the cracks before that > happened. > > Please resubmit once 5.10-rc1 comes out. Thanks for the information, will resubmit the patches after 5.10-rc1. Best regards Sherry > > thanks, > > greg k-h
RE: [PATCH V2 4/4] misc: vop: mapping kernel memory to user space as noncached
Hi Christoph, Gentle ping Can you give some suggestions on the limitations of dma_mmap_coherent api? Best regards Sherry > Subject: RE: [PATCH V2 4/4] misc: vop: mapping kernel memory to user space > as noncached > > Hi David, thanks for your information. > Hi Christoph, please see my comments below. > > > Subject: RE: [PATCH V2 4/4] misc: vop: mapping kernel memory to user > > space as noncached > > > > From: Christoph Hellwig > > > Sent: 29 September 2020 11:29 > > ... > > > You can't call remap_pfn_range on memory returned from > > > dma_alloc_coherent (which btw is not marked uncached on many > > platforms). > > > > > > You need to use the dma_mmap_coherent helper instead. > > > > I tried to use dma_mmap_coherent helper here, but I met the same problem > as David said. > Since the user space calls mmap() to map all the device page and vring size at > one time. > > va = mmap(NULL, MIC_DEVICE_PAGE_END + vr_size * num_vq, > PROT_READ, MAP_SHARED, fd, 0); > > But the physical addresses of device page and multiple vrings are not > consecutive, so we called multiple remap_pfn_range before. When changing > to use dma_mmap_coherent, it will return error because vma_pages(the size > user space want to map) are bigger than the actual size we do multiple > map(one non-continuous memory size at a time). > > David believes that we could modify the vm_start address before call the > multiple dma_mmap_coherent to avoid the vma_pages check error and map > multiple discontinuous memory. > Do you have any suggestions? > > Best regards > Sherry > > > H. I've a driver that does that. > > Fortunately it only has to work on x86 where it doesn't matter. > > However I can't easily convert it. > > The 'problem' is that the mmap() request can cover multiple dma > > buffers and need not start at the beginning of one. > > > > Basically we have a PCIe card that has an inbuilt iommu to convert > > internal 32bit addresses to 64bit PCIe ones. > > This has 512 16kB pages. > > So we do a number of dma_alloc_coherent() for 16k blocks. > > The user process then does an mmap() for part of the buffer. > > This request is 4k aligned so we do multiple remap_pfn_range() calls > > to map the disjoint physical (and kernel virtual) buffers into contiguous > user memory. > > > > So both ends see contiguous addresses even though the physical > > addresses are non-contiguous. > > > > I guess I could modify the vm_start address and then restore it at the end. > > > > I found this big discussion: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore > > .k > ernel.org%2Fpatchwork%2Fpatch%2F351245%2Fdata=02%7C01%7Csh > > > erry.sun%40nxp.com%7C876724689688440581a708d8648dceb3%7C686ea1d > > > 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637369907516376294sdat > > > a=amSClQfVGhI0%2F3bZfo8HF7UmCktkPluArWW22YlQzMQ%3Dreser > > ved=0 > > about these functions. > > > > The bit about VIPT caches is problematic. > > I don't think you can change the kernel address during mmap. > > What you need to do is defer allocating the user address until you > > know the kernel address. > > Otherwise you get into problems is you try to mmap the same memory > > into two processes. > > This is a general problem even for mmap() of files. > > ISTR SYSV on some sparc systems having to use uncached maps. > > > > If you might want to mmap two kernel buffers (dma or not) into > > adjacent user addresses then you need some way of allocating the > > second buffer to follow the first one in the VIVT cache. > > > > David > > > > - > > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, > > MK1 1PT, UK Registration No: 1397386 (Wales)
