Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

5 participants
2541 discussions

by Gerd Hoffmann

A driver to let userspace turn iovecs into dma-bufs. Use case: Allows qemu create dmabufs for the vga framebuffer or virtio-gpu ressources. Then they can be passed around to display those guest things on the host. To spice client for classic full framebuffer display, and hopefully some day to wayland server for seamless guest window display. Those dma-bufs are accounted against user's shm mlock bucket as the pages are effectively locked in memory. Cc: David Airlie <airlied(a)linux.ie> Cc: Tomeu Vizoso <tomeu.vizoso(a)collabora.com> Cc: Daniel Vetter <daniel(a)ffwll.ch> Signed-off-by: Gerd Hoffmann <kraxel(a)redhat.com> --- include/uapi/linux/udmabuf.h | 23 ++ drivers/dma-buf/udmabuf.c | 261 ++++++++++++++++++++++ tools/testing/selftests/drivers/dma-buf/udmabuf.c | 69 ++++++ drivers/dma-buf/Kconfig | 7 + drivers/dma-buf/Makefile | 1 + tools/testing/selftests/drivers/dma-buf/Makefile | 5 + 6 files changed, 366 insertions(+) create mode 100644 include/uapi/linux/udmabuf.h create mode 100644 drivers/dma-buf/udmabuf.c create mode 100644 tools/testing/selftests/drivers/dma-buf/udmabuf.c create mode 100644 tools/testing/selftests/drivers/dma-buf/Makefile diff --git a/include/uapi/linux/udmabuf.h b/include/uapi/linux/udmabuf.h new file mode 100644 index 0000000000..54ceba203a --- /dev/null +++ b/include/uapi/linux/udmabuf.h @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI_LINUX_UDMABUF_H +#define _UAPI_LINUX_UDMABUF_H + +#include <linux/types.h> +#include <linux/ioctl.h> + +struct udmabuf_iovec { + __u64 base; + __u64 len; +}; + +#define UDMABUF_FLAGS_CLOEXEC 0x01 + +struct udmabuf_create { + __u32 flags; + __u32 niov; + struct udmabuf_iovec iovs[]; +}; + +#define UDMABUF_CREATE _IOW(0x42, 0x23, struct udmabuf_create) + +#endif /* _UAPI_LINUX_UDMABUF_H */ diff --git a/drivers/dma-buf/udmabuf.c b/drivers/dma-buf/udmabuf.c new file mode 100644 index 0000000000..664ab4ee4e --- /dev/null +++ b/drivers/dma-buf/udmabuf.c @@ -0,0 +1,261 @@ +/* + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#include <linux/init.h> +#include <linux/module.h> +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/miscdevice.h> +#include <linux/dma-buf.h> +#include <linux/highmem.h> +#include <linux/cred.h> + +#include <uapi/linux/udmabuf.h> + +struct udmabuf { + u32 pagecount; + struct page **pages; + struct user_struct *owner; +}; + +static int udmabuf_vm_fault(struct vm_fault *vmf) +{ + struct vm_area_struct *vma = vmf->vma; + struct udmabuf *ubuf = vma->vm_private_data; + + if (WARN_ON(vmf->pgoff >= ubuf->pagecount)) + return VM_FAULT_SIGBUS; + + vmf->page = ubuf->pages[vmf->pgoff]; + get_page(vmf->page); + return 0; +} + +static const struct vm_operations_struct udmabuf_vm_ops = { + .fault = udmabuf_vm_fault, +}; + +static int mmap_udmabuf(struct dma_buf *buf, struct vm_area_struct *vma) +{ + struct udmabuf *ubuf = buf->priv; + + if ((vma->vm_flags & VM_SHARED) == 0) + return -EINVAL; + + vma->vm_ops = &udmabuf_vm_ops; + vma->vm_private_data = ubuf; + return 0; +} + +static struct sg_table *map_udmabuf(struct dma_buf_attachment *at, + enum dma_data_direction direction) +{ + struct udmabuf *ubuf = at->dmabuf->priv; + struct sg_table *sg; + + sg = kzalloc(sizeof(*sg), GFP_KERNEL); + if (!sg) + goto err1; + if (sg_alloc_table_from_pages(sg, ubuf->pages, ubuf->pagecount, + 0, ubuf->pagecount << PAGE_SHIFT, + GFP_KERNEL) < 0) + goto err2; + if (!dma_map_sg(at->dev, sg->sgl, sg->nents, direction)) + goto err3; + + return sg; + +err3: + sg_free_table(sg); +err2: + kfree(sg); +err1: + return ERR_PTR(-ENOMEM); +} + +static void unmap_udmabuf(struct dma_buf_attachment *at, + struct sg_table *sg, + enum dma_data_direction direction) +{ + sg_free_table(sg); + kfree(sg); +} + +static void release_udmabuf(struct dma_buf *buf) +{ + struct udmabuf *ubuf = buf->priv; + pgoff_t pg; + + for (pg = 0; pg < ubuf->pagecount; pg++) + put_page(ubuf->pages[pg]); + user_shm_unlock(ubuf->pagecount << PAGE_SHIFT, ubuf->owner); + free_uid(ubuf->owner); + kfree(ubuf->pages); + kfree(ubuf); +} + +static void *kmap_atomic_udmabuf(struct dma_buf *buf, unsigned long page_num) +{ + struct udmabuf *ubuf = buf->priv; + struct page *page = ubuf->pages[page_num]; + + return kmap_atomic(page); +} + +static void *kmap_udmabuf(struct dma_buf *buf, unsigned long page_num) +{ + struct udmabuf *ubuf = buf->priv; + struct page *page = ubuf->pages[page_num]; + + return kmap(page); +} + +static struct dma_buf_ops udmabuf_ops = { + .map_dma_buf = map_udmabuf, + .unmap_dma_buf = unmap_udmabuf, + .release = release_udmabuf, + .map_atomic = kmap_atomic_udmabuf, + .map = kmap_udmabuf, + .mmap = mmap_udmabuf, +}; + +static long udmabuf_ioctl_create(struct file *filp, unsigned long arg) +{ + struct udmabuf_create create; + struct udmabuf_iovec *iovs; + struct udmabuf *ubuf; + DEFINE_DMA_BUF_EXPORT_INFO(exp_info); + struct dma_buf *buf; + pgoff_t pgoff, pgcnt; + u32 iov, flags; + int ret; + + if (copy_from_user(&create, (void __user *)arg, + sizeof(struct udmabuf_create))) + return -EFAULT; + + iovs = kmalloc_array(create.niov, sizeof(struct udmabuf_iovec), + GFP_KERNEL); + if (!iovs) + return -ENOMEM; + + arg += offsetof(struct udmabuf_create, iovs); + ret = -EFAULT; + if (copy_from_user(iovs, (void __user *)arg, + create.niov * sizeof(struct udmabuf_iovec))) + goto err_free_iovs; + + ubuf = kzalloc(sizeof(struct udmabuf), GFP_KERNEL); + if (!ubuf) + goto err_free_iovs; + + ret = -EINVAL; + for (iov = 0; iov < create.niov; iov++) { + if (!IS_ALIGNED(iovs[iov].base, PAGE_SIZE)) + goto err_free_buf; + if (!IS_ALIGNED(iovs[iov].len, PAGE_SIZE)) + goto err_free_buf; + ubuf->pagecount += iovs[iov].len >> PAGE_SHIFT; + } + + /* this effectively mlocks the pages so account it accordingly */ + ret = -ENOMEM; + ubuf->owner = current_user(); + if (!user_shm_lock(ubuf->pagecount << PAGE_SHIFT, ubuf->owner)) + goto err_free_buf; + + ubuf->pages = kmalloc_array(ubuf->pagecount, sizeof(struct page*), + GFP_KERNEL); + if (!ubuf->pages) + goto err_shm_unlock; + + pgoff = 0; + for (iov = 0; iov < create.niov; iov++) { + pgcnt = iovs[iov].len >> PAGE_SHIFT; + while (pgcnt > 0) { + ret = get_user_pages_fast(iovs[iov].base, pgcnt, + true, /* write */ + ubuf->pages + pgoff); + if (ret < 0) + goto err_put_pages; + pgoff += ret; + pgcnt -= ret; + } + } + + exp_info.ops = &udmabuf_ops; + exp_info.size = ubuf->pagecount << PAGE_SHIFT; + exp_info.priv = ubuf; + + buf = dma_buf_export(&exp_info); + if (IS_ERR(buf)) { + ret = PTR_ERR(buf); + goto err_put_pages; + } + + flags = 0; + if (create.flags & UDMABUF_FLAGS_CLOEXEC) + flags |= O_CLOEXEC; + + kfree(iovs); + return dma_buf_fd(buf, flags); + +err_put_pages: + while (pgoff > 0) + put_page(ubuf->pages[--pgoff]); +err_shm_unlock: + user_shm_unlock(ubuf->pagecount << PAGE_SHIFT, ubuf->owner); +err_free_buf: + free_uid(ubuf->owner); + kfree(ubuf->pages); + kfree(ubuf); +err_free_iovs: + kfree(iovs); + return ret; +} + +static long udmabuf_ioctl(struct file *filp, unsigned int ioctl, + unsigned long arg) +{ + long ret; + + switch (ioctl) { + case UDMABUF_CREATE: + ret = udmabuf_ioctl_create(filp, arg); + break; + default: + ret = -EINVAL; + break; + } + return ret; +} + +static const struct file_operations udmabuf_fops = { + .owner = THIS_MODULE, + .unlocked_ioctl = udmabuf_ioctl, +}; + +static struct miscdevice udmabuf_misc = { + .minor = MISC_DYNAMIC_MINOR, + .name = "udmabuf", + .fops = &udmabuf_fops, +}; + +static int __init udmabuf_dev_init(void) +{ + return misc_register(&udmabuf_misc); +} + +static void __exit udmabuf_dev_exit(void) +{ + misc_deregister(&udmabuf_misc); +} + +module_init(udmabuf_dev_init) +module_exit(udmabuf_dev_exit) + +MODULE_AUTHOR("Gerd Hoffmann <kraxel(a)redhat.com>"); +MODULE_LICENSE("GPL v2"); diff --git a/tools/testing/selftests/drivers/dma-buf/udmabuf.c b/tools/testing/selftests/drivers/dma-buf/udmabuf.c new file mode 100644 index 0000000000..3472c8ee49 --- /dev/null +++ b/tools/testing/selftests/drivers/dma-buf/udmabuf.c @@ -0,0 +1,69 @@ +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <string.h> +#include <errno.h> +#include <fcntl.h> +#include <malloc.h> + +#include <sys/ioctl.h> +#include <linux/udmabuf.h> + +#define TEST_PREFIX "drivers/dma-buf/udmabuf" +#define NUM_PAGES 4 + +int main(int argc, char *argv[]) +{ + struct udmabuf_create *create; + void *mem; + int dev, fd; + + dev = open("/dev/udmabuf", O_RDWR); + if (dev < 0) { + printf("%s: [skip]\n", TEST_PREFIX); + exit(77); + } + + mem = memalign(getpagesize(), getpagesize() * NUM_PAGES); + if (mem == NULL) { + printf("%s: [FAIL]\n", TEST_PREFIX); + exit (1); + } + + create = malloc(sizeof(struct udmabuf_create) + + sizeof(struct udmabuf_iovec)); + create->flags = 0; + create->niov = 1; + + /* should fail (base not page aligned) */ + create->iovs[0].base = (intptr_t)mem + getpagesize()/2; + create->iovs[0].len = getpagesize(); + fd = ioctl(dev, UDMABUF_CREATE, create); + if (fd >= 0) { + printf("%s: [FAIL]\n", TEST_PREFIX); + exit(1); + } + + /* should fail (size not multiple of page) */ + create->iovs[0].base = (intptr_t)mem; + create->iovs[0].len = getpagesize()/2; + fd = ioctl(dev, UDMABUF_CREATE, create); + if (fd >= 0) { + printf("%s: [FAIL]\n", TEST_PREFIX); + exit(1); + } + + /* should work */ + create->iovs[0].base = (intptr_t)mem; + create->iovs[0].len = getpagesize() * NUM_PAGES; + fd = ioctl(dev, UDMABUF_CREATE, create); + if (fd < 0) { + printf("%s: [FAIL]\n", TEST_PREFIX); + exit(1); + } + close(fd); + + fprintf(stderr, "%s: ok\n", TEST_PREFIX); + close(dev); + return 0; +} diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index ed3b785bae..19be3ec62d 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -30,4 +30,11 @@ config SW_SYNC WARNING: improper use of this can result in deadlocking kernel drivers from userspace. Intended for test and debug only. +config UDMABUF + bool "userspace dmabuf misc driver" + default n + depends on DMA_SHARED_BUFFER + ---help--- + A driver to let userspace turn iovs into dma-bufs. + endmenu diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index c33bf88631..0913a6ccab 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -1,3 +1,4 @@ obj-y := dma-buf.o dma-fence.o dma-fence-array.o reservation.o seqno-fence.o obj-$(CONFIG_SYNC_FILE) += sync_file.o obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o +obj-$(CONFIG_UDMABUF) += udmabuf.o diff --git a/tools/testing/selftests/drivers/dma-buf/Makefile b/tools/testing/selftests/drivers/dma-buf/Makefile new file mode 100644 index 0000000000..4154c3d7aa --- /dev/null +++ b/tools/testing/selftests/drivers/dma-buf/Makefile @@ -0,0 +1,5 @@ +CFLAGS += -I../../../../../usr/include/ + +TEST_GEN_PROGS := udmabuf + +include ../../lib.mk -- 2.9.3

6 years, 9 months

[PATCH] MAINTAINERS: add dri-devel&linaro-mm for Android ION

by Daniel Vetter

Most of the other cross-driver gfx infrastructure (dma_buf, dma_fence) also gets cross posted to all the relevant gfx/memory lists. Doing the same for ION means people won't miss relevant patches. Cc: Laura Abbott <labbott(a)redhat.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: devel(a)driverdev.osuosl.org Cc: dri-devel(a)lists.freedesktop.org Cc: linaro-mm-sig(a)lists.linaro.org (moderated for non-subscribers) Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 555db72d4eb7..d43cdfca3eb5 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -902,6 +902,8 @@ ANDROID ION DRIVER M: Laura Abbott <labbott(a)redhat.com> M: Sumit Semwal <sumit.semwal(a)linaro.org> L: devel(a)driverdev.osuosl.org +L: dri-devel(a)lists.freedesktop.org +L: linaro-mm-sig(a)lists.linaro.org (moderated for non-subscribers) S: Supported F: drivers/staging/android/ion F: drivers/staging/android/uapi/ion.h -- 2.16.2

6 years, 9 months

Re: [Linaro-mm-sig] [PATCH 2/8] PCI: Add pci_find_common_upstream_dev()

by Christoph Hellwig

On Thu, Mar 29, 2018 at 09:58:54PM -0400, Jerome Glisse wrote: > dma_map_resource() is the right API (thought its current implementation > is fill with x86 assumptions). So i would argue that arch can decide to > implement it or simply return dma error address which trigger fallback > path into the caller (at least for GPU drivers). SG variant can be added > on top. It isn't in general. It doesn't integrate with scatterlists (see my comment to page one), and it doesn't integrate with all the subsystems that also need a kernel virtual address.

6 years, 9 months

Re: [Linaro-mm-sig] [PATCH 2/8] PCI: Add pci_find_common_upstream_dev()

by Christian König

Am 29.03.2018 um 18:25 schrieb Logan Gunthorpe: > > On 29/03/18 10:10 AM, Christian König wrote: >> Why not? I mean the dma_map_resource() function is for P2P while other >> dma_map_* functions are only for system memory. > Oh, hmm, I wasn't aware dma_map_resource was exclusively for mapping > P2P. Though it's a bit odd seeing we've been working under the > assumption that PCI P2P is different as it has to translate the PCI bus > address. Where as P2P for devices on other buses is a big unknown. Yeah, completely agree. On my TODO list (but rather far down) is actually supporting P2P with USB devices. And no, I don't have the slightest idea how to do this at the moment. >>> And this is necessary to >>> check if the DMA ops in use support it or not. We can't have the >>> dma_map_X() functions do the wrong thing because they don't support it yet. >> Well that sounds like we should just return an error from >> dma_map_resources() when an architecture doesn't support P2P yet as Alex >> suggested. > Yes, well except in our patch-set we can't easily use > dma_map_resources() as we either have SGLs to deal with or we need to > create whole new interfaces to a number of subsystems. Agree as well. I was also in clear favor of extending the SGLs to have a flag for this instead of the dma_map_resource() interface, but for some reason that didn't made it into the kernel. >> You don't seem to understand the implications: The devices do have a >> common upstream bridge! In other words your code would currently claim >> that P2P is supported, but in practice it doesn't work. > Do they? They don't on any of the Intel machines I'm looking at. The > previous version of the patchset not only required a common upstream > bridge but two layers of upstream bridges on both devices which would > effectively limit transfers to PCIe switches only. But Bjorn did not > like this. At least to me that sounds like a good idea, it would at least disable (the incorrect) auto detection of P2P for such devices. >> You need to include both drivers which participate in the P2P >> transaction to make sure that both supports this and give them >> opportunity to chicken out and in the case of AMD APUs even redirect the >> request to another location (e.g. participate in the DMA translation). > I don't think it's the drivers responsibility to reject P2P . The > topology is what governs support or not. The discussions we had with > Bjorn settled on if the devices are all behind the same bridge they can > communicate with each other. This is essentially guaranteed by the PCI spec. Well it is not only rejecting P2P, see the devices I need to worry about are essentially part of the CPU. Their resources looks like a PCI BAR to the BIOS and OS, but are actually backed by stolen system memory. So as crazy as it sounds what you get is an operation which starts as P2P, but then the GPU drivers sees it and says: Hey please don't write that to my PCIe BAR, but rather system memory location X. >> DMA-buf fortunately seems to handle all this already, that's why we >> choose it as base for our implementation. > Well, unfortunately DMA-buf doesn't help for the drivers we are working > with as neither the block layer nor the RDMA subsystem have any > interfaces for it. A fact that gives me quite some sleepless nights as well. I think we sooner or later need to extend those interfaces to work with DMA-bufs as well. I will try to give your patch set a review when I'm back from vacation and rebase my DMA-buf work on top of that. Regards, Christian. > > Logan

6 years, 9 months

Re: [Linaro-mm-sig] [PATCH 2/8] PCI: Add pci_find_common_upstream_dev()

by Christian König

Am 29.03.2018 um 17:45 schrieb Logan Gunthorpe: > > On 29/03/18 05:44 AM, Christian König wrote: >> Am 28.03.2018 um 21:53 schrieb Logan Gunthorpe: >>> On 28/03/18 01:44 PM, Christian König wrote: >>>> Well, isn't that exactly what dma_map_resource() is good for? As far as >>>> I can see it makes sure IOMMU is aware of the access route and >>>> translates a CPU address into a PCI Bus address. >>>> I'm using that with the AMD IOMMU driver and at least there it works >>>> perfectly fine. >>> Yes, it would be nice, but no arch has implemented this yet. We are just >>> lucky in the x86 case because that arch is simple and doesn't need to do >>> anything for P2P (partially due to the Bus and CPU addresses being the >>> same). But in the general case, you can't rely on it. >> Well, that an arch hasn't implemented it doesn't mean that we don't have >> the right interface to do it. > Yes, but right now we don't have a performant way to check if we are > doing P2P or not in the dma_map_X() wrappers. Why not? I mean the dma_map_resource() function is for P2P while other dma_map_* functions are only for system memory. > And this is necessary to > check if the DMA ops in use support it or not. We can't have the > dma_map_X() functions do the wrong thing because they don't support it yet. Well that sounds like we should just return an error from dma_map_resources() when an architecture doesn't support P2P yet as Alex suggested. >> Devices integrated in the CPU usually only "claim" to be PCIe devices. >> In reality their memory request path go directly through the integrated >> north bridge. The reason for this is simple better throughput/latency. > These are just more reasons why our patchset restricts to devices behind > a switch. And more mess for someone to deal with if they need to relax > that restriction. You don't seem to understand the implications: The devices do have a common upstream bridge! In other words your code would currently claim that P2P is supported, but in practice it doesn't work. You need to include both drivers which participate in the P2P transaction to make sure that both supports this and give them opportunity to chicken out and in the case of AMD APUs even redirect the request to another location (e.g. participate in the DMA translation). DMA-buf fortunately seems to handle all this already, that's why we choose it as base for our implementation. Regards, Christian.

6 years, 9 months

Re: [Linaro-mm-sig] [PATCH 2/8] PCI: Add pci_find_common_upstream_dev()

by Alex Deucher

Sorry, didn't mean to drop the lists here. re-adding. On Wed, Mar 28, 2018 at 4:05 PM, Alex Deucher <alexdeucher(a)gmail.com> wrote: > On Wed, Mar 28, 2018 at 3:53 PM, Logan Gunthorpe <logang(a)deltatee.com> wrote: >> >> >> On 28/03/18 01:44 PM, Christian König wrote: >>> Well, isn't that exactly what dma_map_resource() is good for? As far as >>> I can see it makes sure IOMMU is aware of the access route and >>> translates a CPU address into a PCI Bus address. >> >>> I'm using that with the AMD IOMMU driver and at least there it works >>> perfectly fine. >> >> Yes, it would be nice, but no arch has implemented this yet. We are just >> lucky in the x86 case because that arch is simple and doesn't need to do >> anything for P2P (partially due to the Bus and CPU addresses being the >> same). But in the general case, you can't rely on it. > > Could we do something for the arches where it works? I feel like peer > to peer has dragged out for years because everyone is trying to boil > the ocean for all arches. There are a huge number of use cases for > peer to peer on these "simple" architectures which actually represent > a good deal of the users that want this. > > Alex > >> >>>>> Yeah, but not for ours. See if you want to do real peer 2 peer you need >>>>> to keep both the operation as well as the direction into account. >>>> Not sure what you are saying here... I'm pretty sure we are doing "real" >>>> peer 2 peer... >>>> >>>>> For example when you can do writes between A and B that doesn't mean >>>>> that writes between B and A work. And reads are generally less likely to >>>>> work than writes. etc... >>>> If both devices are behind a switch then the PCI spec guarantees that A >>>> can both read and write B and vice versa. >>> >>> Sorry to say that, but I know a whole bunch of PCI devices which >>> horrible ignores that. >> >> Can you elaborate? As far as the device is concerned it shouldn't know >> whether a request comes from a peer or from the host. If it does do >> crazy stuff like that it's well out of spec. It's up to the switch (or >> root complex if good support exists) to route the request to the device >> and it's the root complex that tends to be what drops the load requests >> which causes the asymmetries. >> >> Logan >> _______________________________________________ >> amd-gfx mailing list >> amd-gfx(a)lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

6 years, 9 months

Re: [Linaro-mm-sig] [PATCH 2/8] PCI: Add pci_find_common_upstream_dev()

by Christian König

Am 28.03.2018 um 21:53 schrieb Logan Gunthorpe: > > On 28/03/18 01:44 PM, Christian König wrote: >> Well, isn't that exactly what dma_map_resource() is good for? As far as >> I can see it makes sure IOMMU is aware of the access route and >> translates a CPU address into a PCI Bus address. >> I'm using that with the AMD IOMMU driver and at least there it works >> perfectly fine. > Yes, it would be nice, but no arch has implemented this yet. We are just > lucky in the x86 case because that arch is simple and doesn't need to do > anything for P2P (partially due to the Bus and CPU addresses being the > same). But in the general case, you can't rely on it. Well, that an arch hasn't implemented it doesn't mean that we don't have the right interface to do it. >>>> Yeah, but not for ours. See if you want to do real peer 2 peer you need >>>> to keep both the operation as well as the direction into account. >>> Not sure what you are saying here... I'm pretty sure we are doing "real" >>> peer 2 peer... >>> >>>> For example when you can do writes between A and B that doesn't mean >>>> that writes between B and A work. And reads are generally less likely to >>>> work than writes. etc... >>> If both devices are behind a switch then the PCI spec guarantees that A >>> can both read and write B and vice versa. >> Sorry to say that, but I know a whole bunch of PCI devices which >> horrible ignores that. > Can you elaborate? As far as the device is concerned it shouldn't know > whether a request comes from a peer or from the host. If it does do > crazy stuff like that it's well out of spec. It's up to the switch (or > root complex if good support exists) to route the request to the device > and it's the root complex that tends to be what drops the load requests > which causes the asymmetries. Devices integrated in the CPU usually only "claim" to be PCIe devices. In reality their memory request path go directly through the integrated north bridge. The reason for this is simple better throughput/latency. That is hidden from the software, for example the BIOS just allocates address space for the BARs as if it's a normal PCIe device. The only crux is when you then do peer2peer your request simply go into nirvana and are not handled by anything because the BARs are only visible from the CPU side of the northbridge. Regards, Christian. > > Logan > _______________________________________________ > amd-gfx mailing list > amd-gfx(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

6 years, 9 months

Re: [Linaro-mm-sig] [PATCH 2/8] PCI: Add pci_find_common_upstream_dev()

by Christian König

Am 28.03.2018 um 20:57 schrieb Logan Gunthorpe: > > On 28/03/18 12:28 PM, Christian König wrote: >> I'm just using amdgpu as blueprint because I'm the co-maintainer of it >> and know it mostly inside out. > Ah, I see. > >> The resource addresses are translated using dma_map_resource(). As far >> as I know that should be sufficient to offload all the architecture >> specific stuff to the DMA subsystem. > It's not. The dma_map infrastructure currently has no concept of > peer-to-peer mappings and is designed for system memory only. No > architecture I'm aware of will translate PCI CPU addresses into PCI Bus > addresses which is necessary for any transfer that doesn't go through > the root complex (though on arches like x86 the CPU and Bus address > happen to be the same). There's a lot of people that would like to see > this change but it's likely going to be a long road before it does. Well, isn't that exactly what dma_map_resource() is good for? As far as I can see it makes sure IOMMU is aware of the access route and translates a CPU address into a PCI Bus address. > Furthermore, one of the reasons our patch-set avoids going through the > root complex at all is that IOMMU drivers will need to be made aware > that it is operating on P2P memory and do arch-specific things > accordingly. There will also need to be flags that indicate whether a > given IOMMU driver supports this. None of this work is done or easy. I'm using that with the AMD IOMMU driver and at least there it works perfectly fine. >> Yeah, but not for ours. See if you want to do real peer 2 peer you need >> to keep both the operation as well as the direction into account. > Not sure what you are saying here... I'm pretty sure we are doing "real" > peer 2 peer... > >> For example when you can do writes between A and B that doesn't mean >> that writes between B and A work. And reads are generally less likely to >> work than writes. etc... > If both devices are behind a switch then the PCI spec guarantees that A > can both read and write B and vice versa. Sorry to say that, but I know a whole bunch of PCI devices which horrible ignores that. For example all AMD APUs fall under that category... > Only once you involve root > complexes do you have this problem. Ie. you have unknown support which > may be no support, or partial support (stores but not loads); or > sometimes bad performance; or a combination of both... and you need some > way to figure out all this mess and that is hard. Whoever tries to > implement a white list will have to sort all this out. Yes, exactly and unfortunately it looks like I'm the poor guy who needs to do this :) Regards, Christian. > > Logan > _______________________________________________ > amd-gfx mailing list > amd-gfx(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

6 years, 9 months

Re: [Linaro-mm-sig] [PATCH 2/8] PCI: Add pci_find_common_upstream_dev()

by Christian König

Am 28.03.2018 um 18:25 schrieb Logan Gunthorpe: > > On 28/03/18 10:02 AM, Christian König wrote: >> Yeah, that looks very similar to what I picked up from the older >> patches, going to read up on that after my vacation. > Yeah, I was just reading through your patchset and there are a lot of > similarities. Though, I'm not sure what you're trying to accomplish as I > could not find a cover letter and it seems to only enable one driver. Yeah, it was the last day before my easter vacation and I wanted it out of the door. > Is it meant to enable DMA transactions only between two AMD GPUs? Not really, DMA-buf is a general framework for sharing buffers between device drivers. It is widely used in the GFX stack on laptops with both Intel+AMD, Intel+NVIDIA or AMD+AMD graphics devices. Additional to that ARM uses it quite massively for their GFX stacks because they have rendering and displaying device separated. I'm just using amdgpu as blueprint because I'm the co-maintainer of it and know it mostly inside out. > I also don't see where you've taken into account the PCI bus address. On > some architectures this is not the same as the CPU physical address. The resource addresses are translated using dma_map_resource(). As far as I know that should be sufficient to offload all the architecture specific stuff to the DMA subsystem. > >> Just in general why are you interested in the "distance" of the devices? > We've taken a general approach where some drivers may provide p2p memory > (ie. an NVMe card or an RDMA NIC) and other drivers make use of it (ie. > the NVMe-of driver). The orchestrator driver needs to find the most > applicable provider device for a transaction in a situation that may > have multiple providers and multiple clients. So the most applicable > provider is the one that's closest ("distance"-wise) to all the clients > for the P2P transaction. That seems to make sense. > >> And BTW: At least for writes that Peer 2 Peer transactions between >> different root complexes work is actually more common than the other way >> around. > Maybe on x86 with hardware made in the last few years. But on PowerPC, > ARM64, and likely a lot more the chance of support is *much* less. Also, > hardware that only supports P2P stores is hardly full support and is > insufficient for our needs. Yeah, but not for ours. See if you want to do real peer 2 peer you need to keep both the operation as well as the direction into account. For example when you can do writes between A and B that doesn't mean that writes between B and A work. And reads are generally less likely to work than writes. etc... Since the use case I'm targeting for is GFX or GFX+V4L (or GFX+NIC in the future) I really need to handle all such use cases as well. > >> So I'm a bit torn between using a blacklist or a whitelist. A whitelist >> is certainly more conservative approach, but that could get a bit long. > I think a whitelist approach is correct. Given old hardware and other > architectures, a black list is going to be too long and too difficult to > comprehensively populate. Yeah, it would certainly be better if we have something in the root complex capabilities. But you're right that a whitelist sounds the less painful way. Regards, Christian. > > Logan > _______________________________________________ > amd-gfx mailing list > amd-gfx(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

6 years, 9 months

Re: [Linaro-mm-sig] [PATCH 2/8] PCI: Add pci_find_common_upstream_dev()

by Christian König

Am 28.03.2018 um 17:47 schrieb Logan Gunthorpe: > > On 28/03/18 09:07 AM, Christian König wrote: >> Am 28.03.2018 um 14:38 schrieb Christoph Hellwig: >>> On Sun, Mar 25, 2018 at 12:59:54PM +0200, Christian König wrote: >>>> From: "wdavis(a)nvidia.com" <wdavis(a)nvidia.com> >>>> >>>> Add an interface to find the first device which is upstream of both >>>> devices. >>> Please work with Logan and base this on top of the outstanding peer >>> to peer patchset. >> Can you point me to that? The last code I could find about that was from >> 2015. > The latest posted series is here: > > https://lkml.org/lkml/2018/3/12/830 > > However, we've made some significant changes to the area that's similar > to what you are doing. You can find lasted un-posted here: > > https://github.com/sbates130272/linux-p2pmem/tree/pci-p2p-v4-pre2 > > Specifically this function would be of interest to you: > > https://github.com/sbates130272/linux-p2pmem/blob/0e9468ae2a5a5198513dd1299… > > However, the difference between what we are doing is that we are > interested in the distance through the common upstream device and you > appear to be finding the actual common device. Yeah, that looks very similar to what I picked up from the older patches, going to read up on that after my vacation. Just in general why are you interested in the "distance" of the devices? And BTW: At least for writes that Peer 2 Peer transactions between different root complexes work is actually more common than the other way around. So I'm a bit torn between using a blacklist or a whitelist. A whitelist is certainly more conservative approach, but that could get a bit long. Thanks, Christian. > > Thanks, > > Logan

6 years, 9 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig